| Message ID | 20231101212604.1636517-4-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a few panels and use correct modes | expand |
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.
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); + 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"); + } /* * TODO: Remove once all drm drivers call diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index c613f0abe9dc..301817e00a15 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -560,6 +560,7 @@ void drm_mode_prune_invalid(struct drm_device *dev, struct list_head *mode_list, bool verbose); void drm_mode_sort(struct list_head *mode_list); void drm_connector_list_update(struct drm_connector *connector); +void drm_mode_unset_preferred_modes(struct drm_connector *connector); /* parsing cmdline modes */ bool
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(-)