Re: [PATCH 3/3] drm/panel-edp: Choose correct preferred mode

[Doug Anderson]

Hi,

On Mon, Nov 6, 2023 at 12:06 AM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> On Thu, Nov 02, 2023 at 07:33:48AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Nov 1, 2023 at 11:31 PM Dmitry Baryshkov
> > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > >
> > > On Wed, 1 Nov 2023 at 23:26, Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote:
> > > >
> > > > If a non generic edp-panel is under aux-bus, the mode read from edid would
> > > > still be selected as preferred and results in multiple preferred modes,
> > > > which is ambiguous.
> > > >
> > > > If a hard-coded mode is present, unset the preferred bit of the modes read
> > > > from edid.
> > >
> > > Can we skip the EDID completely if the hardcoded override is present?
> >
> > Yeah, I wondered about that too. The blending of the hardcoded with
> > the EDID predates my involvement with the driver. You can see even as
> > of commit 280921de7241 ("drm/panel: Add simple panel support") that
> > the driver would start with the EDID modes (if it had them) and then
> > go onto add the hardcoded modes. At least for eDP panels, though,
> > nobody (or almost nobody?) actually provided panel-simple a DDC bus at
> > the same time it was given a hardcoded panel.
> >
> > I guess I could go either way, but I have a slight bias to adding the
> > extra modes and just making it clear to userspace that none of them
> > are "preferred". That seems like it would give userspace the most
> > flexibility
>
> I disagree. "Flexibility" here just means "the way to shoot itself in
> the foot without knowing it's aiming at its foot".
>
> If a mode is broken, we shouldn't expose it, just like we don't for all
> modes that require a maximum frequency higher than what the controller
> can provide on HDMI for example.

In this particular case we aren't saying that modes are broken. There
are two (somewhat separate) things in Hsin-Yi's series.

The first thing is a quirk for panels with incorrect modes in their
EDID when using the generic "edp-panel" compatible. In that case we
now _replace_ the broken mode with a more correct one because, as you
say, we shouldn't be telling userspace about a broken mode.

The second thing in Hsin-Yi's series is for when we're _not_ using the
generic "edp-panel". In that case we have a hardcoded mode from the
"compatible" string but we also have modes from the EDID and that's
what ${SUBJECT} patch is about. Here we don't truly know that the
modes in the EDID are broken.


> > and also is closer to what we've historically done (though,
> > historically, we just allowed there to be more than one "preferred"
> > mode).
>
> I have no idea what history you're referring to here

History = historical behavior? As above, I pointed out that the kernel
has been merging the hardcoded and EDID modes as far back as commit
280921de7241 ("drm/panel: Add simple panel support") in 2013.

That being said, the historical behavior has more than one mode marked
preferred which is bad, so we're changing the behavior anyway. I'm not
against changing it to just have the hardcoded mode if that's what
everyone else wants (and it sounds like it is).