| Message ID | 20230314114451.8872-1-lujianhua000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: Fix panel mode type setting logic | expand |
Hi, On Tue, Mar 14, 2023 at 4:45 AM Jianhua Lu <lujianhua000@gmail.com> wrote: > > Some panels set mode type to DRM_MODE_TYPE_PREFERRED by the number > of modes. It isn't reasonable, so set the first mode type to > DRM_MODE_TYPE_PREFERRED. This should be more reasonable. > > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> > --- > drivers/gpu/drm/panel/panel-abt-y030xx067a.c | 2 +- > drivers/gpu/drm/panel/panel-auo-a030jtn01.c | 2 +- > drivers/gpu/drm/panel/panel-edp.c | 4 ++-- > drivers/gpu/drm/panel/panel-innolux-ej030na.c | 2 +- > drivers/gpu/drm/panel/panel-newvision-nv3051d.c | 2 +- > drivers/gpu/drm/panel/panel-newvision-nv3052c.c | 2 +- > drivers/gpu/drm/panel/panel-novatek-nt35950.c | 2 +- > drivers/gpu/drm/panel/panel-novatek-nt39016.c | 2 +- > drivers/gpu/drm/panel/panel-orisetech-ota5601a.c | 2 +- > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 4 ++-- > drivers/gpu/drm/panel/panel-simple.c | 4 ++-- > 11 files changed, 14 insertions(+), 14 deletions(-) Can you explain more about your motivation here? At least for panel-edp and panel-simple it seems like it would be better to leave the logic alone and manually add DRM_MODE_TYPE_PREFERRED to the right mode for the rare panel that actually has more than one mode listed. That feels more explicit to me. -Doug
On Tue, Mar 14, 2023 at 10:12:02AM -0700, Doug Anderson wrote: > Hi, > > On Tue, Mar 14, 2023 at 4:45 AM Jianhua Lu <lujianhua000@gmail.com> wrote: > > > > Some panels set mode type to DRM_MODE_TYPE_PREFERRED by the number > > of modes. It isn't reasonable, so set the first mode type to > > DRM_MODE_TYPE_PREFERRED. This should be more reasonable. > > > > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> > > --- > > drivers/gpu/drm/panel/panel-abt-y030xx067a.c | 2 +- > > drivers/gpu/drm/panel/panel-auo-a030jtn01.c | 2 +- > > drivers/gpu/drm/panel/panel-edp.c | 4 ++-- > > drivers/gpu/drm/panel/panel-innolux-ej030na.c | 2 +- > > drivers/gpu/drm/panel/panel-newvision-nv3051d.c | 2 +- > > drivers/gpu/drm/panel/panel-newvision-nv3052c.c | 2 +- > > drivers/gpu/drm/panel/panel-novatek-nt35950.c | 2 +- > > drivers/gpu/drm/panel/panel-novatek-nt39016.c | 2 +- > > drivers/gpu/drm/panel/panel-orisetech-ota5601a.c | 2 +- > > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 4 ++-- > > drivers/gpu/drm/panel/panel-simple.c | 4 ++-- > > 11 files changed, 14 insertions(+), 14 deletions(-) > > Can you explain more about your motivation here? At least for This demonstrates a bad way to set DRM_MODE_TYPE_PREFERRED for panels with more than one mode. It mislead the future contributors to send a patch with this piece of code. There is also a discussion for it. https://lore.kernel.org/lkml/904bc493-7160-32fd-9709-1dcb978ddbab@linaro.org/ > panel-edp and panel-simple it seems like it would be better to leave > the logic alone and manually add DRM_MODE_TYPE_PREFERRED to the right > mode for the rare panel that actually has more than one mode listed. I think we can order it to the first mode if the mode type should be DRM_MODE_TYPE_PREFERRED, It's also same. > That feels more explicit to me. > > -Doug
Hi, On Tue, Mar 14, 2023 at 4:55 PM Jianhua Lu <lujianhua000@gmail.com> wrote: > > On Tue, Mar 14, 2023 at 10:12:02AM -0700, Doug Anderson wrote: > > Hi, > > > > On Tue, Mar 14, 2023 at 4:45 AM Jianhua Lu <lujianhua000@gmail.com> wrote: > > > > > > Some panels set mode type to DRM_MODE_TYPE_PREFERRED by the number > > > of modes. It isn't reasonable, so set the first mode type to > > > DRM_MODE_TYPE_PREFERRED. This should be more reasonable. > > > > > > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> > > > --- > > > drivers/gpu/drm/panel/panel-abt-y030xx067a.c | 2 +- > > > drivers/gpu/drm/panel/panel-auo-a030jtn01.c | 2 +- > > > drivers/gpu/drm/panel/panel-edp.c | 4 ++-- > > > drivers/gpu/drm/panel/panel-innolux-ej030na.c | 2 +- > > > drivers/gpu/drm/panel/panel-newvision-nv3051d.c | 2 +- > > > drivers/gpu/drm/panel/panel-newvision-nv3052c.c | 2 +- > > > drivers/gpu/drm/panel/panel-novatek-nt35950.c | 2 +- > > > drivers/gpu/drm/panel/panel-novatek-nt39016.c | 2 +- > > > drivers/gpu/drm/panel/panel-orisetech-ota5601a.c | 2 +- > > > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 4 ++-- > > > drivers/gpu/drm/panel/panel-simple.c | 4 ++-- > > > 11 files changed, 14 insertions(+), 14 deletions(-) > > > > Can you explain more about your motivation here? At least for > This demonstrates a bad way to set DRM_MODE_TYPE_PREFERRED for panels > with more than one mode. It mislead the future contributors to send > a patch with this piece of code. There is also a discussion for it. > https://lore.kernel.org/lkml/904bc493-7160-32fd-9709-1dcb978ddbab@linaro.org/ > > panel-edp and panel-simple it seems like it would be better to leave > > the logic alone and manually add DRM_MODE_TYPE_PREFERRED to the right > > mode for the rare panel that actually has more than one mode listed. > I think we can order it to the first mode if the mode type should be > DRM_MODE_TYPE_PREFERRED, It's also same. A pointer to the original discussion would have been super helpful to be provided in your patch description. Personally, I still stand by my assertion that I'd rather that DRM_MODE_TYPE_PREFERRED be placed in the actual data instead of being done like this in post-processing. To me the old check for "num_modes == 1" is basically saying that the people creating the "static const" data in this file were lazy and didn't feel like they needed to set a DRM_MODE_TYPE_PREFERRED when there was only one mode listed. Thus, we can add it for them. When "num_modes" is more than 1 then we shouldn't allow the people making the "static const" data to be lazy. We should force them to set one of the modes to be PREFERRED rather than for them to have to know about this magic rule. Thus, for me, my order of preference would be these (note, I've mostly looked at panel-edp and panel-simple): 1. Leave the check as "num_modes == 1" and document that we're basically allowing the people writing the "static const" structure to be lazy if there's only one mode. Manually add the DRM_MODE_TYPE_PREFERRED flag to the small number of cases where there is more than one mode. Possibly add a warning printout if we end up without a PREFERRED mode. I'd give a Reviewed-by for this. 2. Fully get rid of this logic and add DRM_MODE_TYPE_PREFERRED to all of the "static const" data. I guess maybe we can't do that for the "timings" in panel-edp and panel-simple. I guess for those I'd be OK with just setting PREFERRED on the first timing like your patch is doing. I'd give a Reviewed-by for this. 3. Your patch. I won't NAK it since it seems like this is what other (more senior) DRM folks were suggesting. ...but I don't love it. I won't give a Reviewed-by for this but won't object to someone else doing so. -Doug
Hi, On 17/03/2023 01:23, Doug Anderson wrote: > Hi, > > > On Tue, Mar 14, 2023 at 4:55 PM Jianhua Lu <lujianhua000@gmail.com> wrote: >> >> On Tue, Mar 14, 2023 at 10:12:02AM -0700, Doug Anderson wrote: >>> Hi, >>> >>> On Tue, Mar 14, 2023 at 4:45 AM Jianhua Lu <lujianhua000@gmail.com> wrote: >>>> >>>> Some panels set mode type to DRM_MODE_TYPE_PREFERRED by the number >>>> of modes. It isn't reasonable, so set the first mode type to >>>> DRM_MODE_TYPE_PREFERRED. This should be more reasonable. >>>> >>>> Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> >>>> --- >>>> drivers/gpu/drm/panel/panel-abt-y030xx067a.c | 2 +- >>>> drivers/gpu/drm/panel/panel-auo-a030jtn01.c | 2 +- >>>> drivers/gpu/drm/panel/panel-edp.c | 4 ++-- >>>> drivers/gpu/drm/panel/panel-innolux-ej030na.c | 2 +- >>>> drivers/gpu/drm/panel/panel-newvision-nv3051d.c | 2 +- >>>> drivers/gpu/drm/panel/panel-newvision-nv3052c.c | 2 +- >>>> drivers/gpu/drm/panel/panel-novatek-nt35950.c | 2 +- >>>> drivers/gpu/drm/panel/panel-novatek-nt39016.c | 2 +- >>>> drivers/gpu/drm/panel/panel-orisetech-ota5601a.c | 2 +- >>>> drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 4 ++-- >>>> drivers/gpu/drm/panel/panel-simple.c | 4 ++-- >>>> 11 files changed, 14 insertions(+), 14 deletions(-) >>> >>> Can you explain more about your motivation here? At least for >> This demonstrates a bad way to set DRM_MODE_TYPE_PREFERRED for panels >> with more than one mode. It mislead the future contributors to send >> a patch with this piece of code. There is also a discussion for it. >> https://lore.kernel.org/lkml/904bc493-7160-32fd-9709-1dcb978ddbab@linaro.org/ >>> panel-edp and panel-simple it seems like it would be better to leave >>> the logic alone and manually add DRM_MODE_TYPE_PREFERRED to the right >>> mode for the rare panel that actually has more than one mode listed. >> I think we can order it to the first mode if the mode type should be >> DRM_MODE_TYPE_PREFERRED, It's also same. > > A pointer to the original discussion would have been super helpful to > be provided in your patch description. > > Personally, I still stand by my assertion that I'd rather that > DRM_MODE_TYPE_PREFERRED be placed in the actual data instead of being > done like this in post-processing. To me the old check for "num_modes > == 1" is basically saying that the people creating the "static const" > data in this file were lazy and didn't feel like they needed to set a > DRM_MODE_TYPE_PREFERRED when there was only one mode listed. Thus, we > can add it for them. When "num_modes" is more than 1 then we shouldn't > allow the people making the "static const" data to be lazy. We should > force them to set one of the modes to be PREFERRED rather than for > them to have to know about this magic rule. > > Thus, for me, my order of preference would be these (note, I've mostly > looked at panel-edp and panel-simple): > > 1. Leave the check as "num_modes == 1" and document that we're > basically allowing the people writing the "static const" structure to > be lazy if there's only one mode. Manually add the > DRM_MODE_TYPE_PREFERRED flag to the small number of cases where there > is more than one mode. Possibly add a warning printout if we end up > without a PREFERRED mode. I'd give a Reviewed-by for this. > > 2. Fully get rid of this logic and add DRM_MODE_TYPE_PREFERRED to all > of the "static const" data. I guess maybe we can't do that for the > "timings" in panel-edp and panel-simple. I guess for those I'd be OK > with just setting PREFERRED on the first timing like your patch is > doing. I'd give a Reviewed-by for this. > > 3. Your patch. I won't NAK it since it seems like this is what other > (more senior) DRM folks were suggesting. ...but I don't love it. I > won't give a Reviewed-by for this but won't object to someone else > doing so. I'm aligned with Doug's analysis, I don't have a strong opinion on what to do, but 1 or 2 would be OK. Neil > > -Doug
diff --git a/drivers/gpu/drm/panel/panel-abt-y030xx067a.c b/drivers/gpu/drm/panel/panel-abt-y030xx067a.c index 1cc0f1d09684..9ce62513e3a5 100644 --- a/drivers/gpu/drm/panel/panel-abt-y030xx067a.c +++ b/drivers/gpu/drm/panel/panel-abt-y030xx067a.c @@ -240,7 +240,7 @@ static int y030xx067a_get_modes(struct drm_panel *panel, drm_mode_set_name(mode); mode->type = DRM_MODE_TYPE_DRIVER; - if (panel_info->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); diff --git a/drivers/gpu/drm/panel/panel-auo-a030jtn01.c b/drivers/gpu/drm/panel/panel-auo-a030jtn01.c index 3c976a98de6a..3850dc5a1eb1 100644 --- a/drivers/gpu/drm/panel/panel-auo-a030jtn01.c +++ b/drivers/gpu/drm/panel/panel-auo-a030jtn01.c @@ -151,7 +151,7 @@ static int a030jtn01_get_modes(struct drm_panel *panel, drm_mode_set_name(mode); mode->type = DRM_MODE_TYPE_DRIVER; - if (panel_info->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 01bfe0783304..df7e59485793 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -260,7 +260,7 @@ static unsigned int panel_edp_get_timings_modes(struct panel_edp *panel, mode->type |= DRM_MODE_TYPE_DRIVER; - if (panel->desc->num_timings == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); @@ -289,7 +289,7 @@ static unsigned int panel_edp_get_display_modes(struct panel_edp *panel, mode->type |= DRM_MODE_TYPE_DRIVER; - if (panel->desc->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_set_name(mode); diff --git a/drivers/gpu/drm/panel/panel-innolux-ej030na.c b/drivers/gpu/drm/panel/panel-innolux-ej030na.c index b2b0ebc9e943..6c49c93eaa23 100644 --- a/drivers/gpu/drm/panel/panel-innolux-ej030na.c +++ b/drivers/gpu/drm/panel/panel-innolux-ej030na.c @@ -166,7 +166,7 @@ static int ej030na_get_modes(struct drm_panel *panel, drm_mode_set_name(mode); mode->type = DRM_MODE_TYPE_DRIVER; - if (panel_info->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3051d.c b/drivers/gpu/drm/panel/panel-newvision-nv3051d.c index a07958038ffd..65ff9a3df62a 100644 --- a/drivers/gpu/drm/panel/panel-newvision-nv3051d.c +++ b/drivers/gpu/drm/panel/panel-newvision-nv3051d.c @@ -331,7 +331,7 @@ static int panel_nv3051d_get_modes(struct drm_panel *panel, drm_mode_set_name(mode); mode->type = DRM_MODE_TYPE_DRIVER; - if (panel_info->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c index cf078f0d3cd3..70a7b36c2956 100644 --- a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c +++ b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c @@ -347,7 +347,7 @@ static int nv3052c_get_modes(struct drm_panel *panel, drm_mode_set_name(mode); mode->type = DRM_MODE_TYPE_DRIVER; - if (panel_info->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c index abf752b36a52..62f7895af072 100644 --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c @@ -508,7 +508,7 @@ static int nt35950_get_modes(struct drm_panel *panel, drm_mode_set_name(mode); mode->type |= DRM_MODE_TYPE_DRIVER; - if (nt->desc->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); diff --git a/drivers/gpu/drm/panel/panel-novatek-nt39016.c b/drivers/gpu/drm/panel/panel-novatek-nt39016.c index f58cfb10b58a..7938dd68f4f4 100644 --- a/drivers/gpu/drm/panel/panel-novatek-nt39016.c +++ b/drivers/gpu/drm/panel/panel-novatek-nt39016.c @@ -216,7 +216,7 @@ static int nt39016_get_modes(struct drm_panel *drm_panel, drm_mode_set_name(mode); mode->type = DRM_MODE_TYPE_DRIVER; - if (panel_info->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); diff --git a/drivers/gpu/drm/panel/panel-orisetech-ota5601a.c b/drivers/gpu/drm/panel/panel-orisetech-ota5601a.c index e46be5014d42..d232c02eba20 100644 --- a/drivers/gpu/drm/panel/panel-orisetech-ota5601a.c +++ b/drivers/gpu/drm/panel/panel-orisetech-ota5601a.c @@ -206,7 +206,7 @@ static int ota5601a_get_modes(struct drm_panel *drm_panel, drm_mode_set_name(mode); mode->type = DRM_MODE_TYPE_DRIVER; - if (panel_info->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c index 76160e5d43bd..dad103615c7f 100644 --- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c +++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c @@ -80,7 +80,7 @@ static int seiko_panel_get_fixed_modes(struct seiko_panel *panel, mode->type |= DRM_MODE_TYPE_DRIVER; - if (panel->desc->num_timings == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); @@ -100,7 +100,7 @@ static int seiko_panel_get_fixed_modes(struct seiko_panel *panel, mode->type |= DRM_MODE_TYPE_DRIVER; - if (panel->desc->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_set_name(mode); diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 065f378bba9d..fc617969c2be 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -185,7 +185,7 @@ static unsigned int panel_simple_get_timings_modes(struct panel_simple *panel, mode->type |= DRM_MODE_TYPE_DRIVER; - if (panel->desc->num_timings == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); @@ -214,7 +214,7 @@ static unsigned int panel_simple_get_display_modes(struct panel_simple *panel, mode->type |= DRM_MODE_TYPE_DRIVER; - if (panel->desc->num_modes == 1) + if (i == 0) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_set_name(mode);
Some panels set mode type to DRM_MODE_TYPE_PREFERRED by the number of modes. It isn't reasonable, so set the first mode type to DRM_MODE_TYPE_PREFERRED. This should be more reasonable. Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> --- drivers/gpu/drm/panel/panel-abt-y030xx067a.c | 2 +- drivers/gpu/drm/panel/panel-auo-a030jtn01.c | 2 +- drivers/gpu/drm/panel/panel-edp.c | 4 ++-- drivers/gpu/drm/panel/panel-innolux-ej030na.c | 2 +- drivers/gpu/drm/panel/panel-newvision-nv3051d.c | 2 +- drivers/gpu/drm/panel/panel-newvision-nv3052c.c | 2 +- drivers/gpu/drm/panel/panel-novatek-nt35950.c | 2 +- drivers/gpu/drm/panel/panel-novatek-nt39016.c | 2 +- drivers/gpu/drm/panel/panel-orisetech-ota5601a.c | 2 +- drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 4 ++-- drivers/gpu/drm/panel/panel-simple.c | 4 ++-- 11 files changed, 14 insertions(+), 14 deletions(-)