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 <[email protected]>
---
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(-)
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);
--
2.39.2
Hi,
On Tue, Mar 14, 2023 at 4:45 AM Jianhua Lu <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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/[email protected]/
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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/[email protected]/
> > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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/[email protected]/
>>> 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