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

  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.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/gpu/drm/drm_modes.c       | 16 ++++++++++++++++
 drivers/gpu/drm/panel/panel-edp.c |  7 +++++--
 include/drm/drm_modes.h           |  1 +
 3 files changed, 22 insertions(+), 2 deletions(-)
  

Hi Hsin-Yi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[cannot apply to linus/master v6.6 next-20231101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hsin-Yi-Wang/drm-panel-edp-Add-several-generic-edp-panels/20231102-053007
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231101212604.1636517-4-hsinyi%40chromium.org
patch subject: [PATCH 3/3] drm/panel-edp: Choose correct preferred mode
config: arc-randconfig-001-20231102 (https://download.01.org/0day-ci/archive/20231102/202311021208.ekIThlkq-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231102/202311021208.ekIThlkq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311021208.ekIThlkq-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_modes.c:1944: warning: expecting prototype for drm_mode_unset_preferred(). Prototype was for drm_mode_unset_preferred_modes() instead


vim +1944 drivers/gpu/drm/drm_modes.c

  1935	
  1936	/**
  1937	 * drm_mode_unset_preferred - clear the preferred status on existing modes.
  1938	 * @connector: the connector to update
  1939	 *
  1940	 * Walk the mode list for connector, clearing the preferred status on existing
  1941	 * modes.
  1942	 */
  1943	void drm_mode_unset_preferred_modes(struct drm_connector *connector)
> 1944	{
  1945		struct drm_display_mode *cur_mode;
  1946	
  1947		list_for_each_entry(cur_mode, &connector->probed_modes, head)
  1948			cur_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
  1949	}
  1950	EXPORT_SYMBOL_GPL(drm_mode_unset_preferred_modes);
  1951
  

On Wed, 1 Nov 2023 at 23:26, Hsin-Yi Wang <hsinyi@chromium.org> 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?

>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/gpu/drm/drm_modes.c       | 16 ++++++++++++++++
>  drivers/gpu/drm/panel/panel-edp.c |  7 +++++--
>  include/drm/drm_modes.h           |  1 +
>  3 files changed, 22 insertions(+), 2 deletions(-)

Anyway, this should be split into two patches. One for drm_modes.c,
another one for the panel driver.
  

Hi,

On Wed, Nov 1, 2023 at 11:31 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, 1 Nov 2023 at 23:26, Hsin-Yi Wang <hsinyi@chromium.org> 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 and also is closer to what we've historically done
(though, historically, we just allowed there to be more than one
"preferred" mode).

One thing we definitely want to do, though, is to still expose the
EDID to userspace even if we're using a hardcoded mode. I believe
that, at least on ChromeOS, there are some tools that look at the EDID
directly for some reason or another.


> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_modes.c       | 16 ++++++++++++++++
> >  drivers/gpu/drm/panel/panel-edp.c |  7 +++++--
> >  include/drm/drm_modes.h           |  1 +
> >  3 files changed, 22 insertions(+), 2 deletions(-)
>
> Anyway, this should be split into two patches. One for drm_modes.c,
> another one for the panel driver.

Yeah, that's probably a good idea.

-Doug
  

Hi,

On Wed, Nov 01, 2023 at 02:20:11PM -0700, Hsin-Yi Wang 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.
> 
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/gpu/drm/drm_modes.c       | 16 ++++++++++++++++
>  drivers/gpu/drm/panel/panel-edp.c |  7 +++++--
>  include/drm/drm_modes.h           |  1 +
>  3 files changed, 22 insertions(+), 2 deletions(-)

This should be in two separate patches, with kunit tests for the helper
you create.

> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac9a406250c5..35927467f4b0 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1933,6 +1933,22 @@ void drm_connector_list_update(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_connector_list_update);
>  
> +/**
> + * drm_mode_unset_preferred - clear the preferred status on existing modes.
> + * @connector: the connector to update
> + *
> + * Walk the mode list for connector, clearing the preferred status on existing
> + * modes.
> + */
> +void drm_mode_unset_preferred_modes(struct drm_connector *connector)
> +{
> +	struct drm_display_mode *cur_mode;
> +
> +	list_for_each_entry(cur_mode, &connector->probed_modes, head)
> +		cur_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
> +}
> +EXPORT_SYMBOL_GPL(drm_mode_unset_preferred_modes);
> +

More importantly, why do you need a helper for this at all? If it's only
useful in a single driver for now, then just add it to that driver.

>  static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr,
>  				      struct drm_cmdline_mode *mode)
>  {
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 0883ff312eba..b3ac473b2554 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -622,10 +622,13 @@ static int panel_edp_get_modes(struct drm_panel *panel,
>  	 * and no modes (the generic edp-panel case) because it will clobber
>  	 * the display_info that was already set by drm_add_edid_modes().
>  	 */
> -	if (p->desc->num_timings || p->desc->num_modes)
> +	if (p->desc->num_timings || p->desc->num_modes) {
> +		/* hard-coded modes present, unset preferred modes from edid. */
> +		drm_mode_unset_preferred_modes(connector);
>  		num += panel_edp_get_non_edid_modes(p, connector);
> -	else if (!num)
> +	} else if (!num) {
>  		dev_warn(p->base.dev, "No display modes\n");
> +	}

It's also not clear to me why you need to mess with the modes at all. If
the mode is unreliable and needs to be overloaded, then just ignore the
EDIDs entirely and report the mode you have hardcoded in the driver. You
then don't need to play with the flags at all.

Maxime
  

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@linaro.org> wrote:
> >
> > On Wed, 1 Nov 2023 at 23:26, Hsin-Yi Wang <hsinyi@chromium.org> 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.

> 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

> One thing we definitely want to do, though, is to still expose the
> EDID to userspace even if we're using a hardcoded mode. I believe
> that, at least on ChromeOS, there are some tools that look at the EDID
> directly for some reason or another.

If the EDID is known to be broken and unreliable, what's the point?

Maxime
  

On Mon, 06 Nov 2023, Maxime Ripard <mripard@kernel.org> 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@linaro.org> wrote:
>> >
>> > On Wed, 1 Nov 2023 at 23:26, Hsin-Yi Wang <hsinyi@chromium.org> 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.

Agreed. This is exactly what the ->mode_valid connector helper callback
is for.

>
>> 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
>
>> One thing we definitely want to do, though, is to still expose the
>> EDID to userspace even if we're using a hardcoded mode. I believe
>> that, at least on ChromeOS, there are some tools that look at the EDID
>> directly for some reason or another.
>
> If the EDID is known to be broken and unreliable, what's the point?

I don't think it's unheard of to have bogus modes in the EDID while
other stuff is required.

I think the current agreement pretty much is that the kernel handles the
modes, either from the EDID or some other channel, and the userspace
does not look at the EDID for modes.

BR,
Jani.
  

Hi,

On Mon, Nov 6, 2023 at 12:06 AM Maxime Ripard <mripard@kernel.org> 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@linaro.org> wrote:
> > >
> > > On Wed, 1 Nov 2023 at 23:26, Hsin-Yi Wang <hsinyi@chromium.org> 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).
  

On Mon, Nov 6, 2023 at 8:21 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Nov 6, 2023 at 12:06 AM Maxime Ripard <mripard@kernel.org> 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@linaro.org> wrote:
> > > >
> > > > On Wed, 1 Nov 2023 at 23:26, Hsin-Yi Wang <hsinyi@chromium.org> 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).

I'll change the behavior to: if hard-coded mode presents, don't add
edid mode since it will result in multiple preferred modes.