[PATCH 0/3] Add a few panels and use correct modes

[PATCH 0/3] Add a few panels and use correct modes

[Hsin-Yi Wang]

This series contains 3 patches:
1. Add a few new generic edp panels.
2. Support a new quirk to override the mode read from edid
3. Unset preferred mode from edid if hard-coded mode presents.

Hsin-Yi Wang (3):
  drm/panel-edp: Add several generic edp panels
  drm/panel-edp: Add override_edid_mode quirk for generic edp
  drm/panel-edp: Choose correct preferred mode

 drivers/gpu/drm/drm_modes.c       |  16 ++++
 drivers/gpu/drm/panel/panel-edp.c | 135 ++++++++++++++++++++++++++++--
 include/drm/drm_modes.h           |   1 +
 3 files changed, 144 insertions(+), 8 deletions(-)

-- 
2.42.0.869.gea05f2083d-goog

[PATCH 1/3] drm/panel-edp: Add several generic edp panels

[Hsin-Yi Wang]

Add a few generic edp panels used by mt8186 chromebooks.
Besides, modify the following panel:
- AUO 0x235c B116XTN02 renamed to B116XTN02.3.
- AUO 0x405c B116XAK01 adjust the timing to delay_200_500_e50. According
to the datasheet: T3=200, T12=500, T7_max = 50.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/gpu/drm/panel/panel-edp.c | 62 ++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index 9dce4c702414..06ce3a73d740 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -1830,6 +1830,12 @@ static const struct panel_delay delay_200_500_e50 = {
 	.enable = 50,
 };
 
+static const struct panel_delay delay_200_500_e80 = {
+	.hpd_absent = 200,
+	.unprepare = 500,
+	.enable = 80,
+};
+
 static const struct panel_delay delay_200_500_e80_d50 = {
 	.hpd_absent = 200,
 	.unprepare = 500,
@@ -1849,6 +1855,25 @@ static const struct panel_delay delay_200_500_e200 = {
 	.enable = 200,
 };
 
+static const struct panel_delay delay_200_500_e200_d10 = {
+	.hpd_absent = 200,
+	.unprepare = 500,
+	.enable = 200,
+	.disable = 10,
+};
+
+static const struct panel_delay delay_200_150_e50 = {
+	.hpd_absent = 200,
+	.unprepare = 150,
+	.enable = 50,
+};
+
+static const struct panel_delay delay_200_150_e200 = {
+	.hpd_absent = 200,
+	.unprepare = 150,
+	.enable = 200,
+};
+
 #define EDP_PANEL_ENTRY(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _delay, _name) \
 { \
 	.name = _name, \
@@ -1869,38 +1894,71 @@ static const struct edp_panel_entry edp_panels[] = {
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x145c, &delay_200_500_e50, "B116XAB01.4"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x1e9b, &delay_200_500_e50, "B133UAN02.1"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x1ea5, &delay_200_500_e50, "B116XAK01.6"),
-	EDP_PANEL_ENTRY('A', 'U', 'O', 0x235c, &delay_200_500_e50, "B116XTN02"),
-	EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0x208d, &delay_200_500_e50, "B140HTN02.1"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0x235c, &delay_200_500_e50, "B116XTN02.3"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &delay_200_500_e50, "B116XAK01.0"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x582d, &delay_200_500_e50, "B133UAN01.0"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x615c, &delay_200_500_e50, "B116XAN06.1"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0x635c, &delay_200_500_e50, "B116XAN06.3"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0x639c, &delay_200_500_e50, "B140HAK02.7"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x8594, &delay_200_500_e50, "B133UAN01.0"),
+	EDP_PANEL_ENTRY('A', 'U', 'O', 0xf390, &delay_200_500_e50, "B140XTN07.7"),
 
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0715, &delay_200_150_e200, "NT116WHM-N21"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0731, &delay_200_500_e80, "NT116WHM-N42"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0741, &delay_200_500_e200, "NT116WHM-N44"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0786, &delay_200_500_p2e80, "NV116WHM-T01"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x07d1, &boe_nv133fhm_n61.delay, "NV133FHM-N61"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x07f6, &delay_200_500_e200, "NT140FHM-N44"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x082d, &boe_nv133fhm_n61.delay, "NV133FHM-N62"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x08b2, &delay_200_500_e200, "NT140WHM-N49"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x09c3, &delay_200_500_e50, "NT116WHM-N21,836X2"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x094b, &delay_200_500_e50, "NT116WHM-N21"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x095f, &delay_200_500_e50, "NE135FBM-N41 v8.1"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0979, &delay_200_500_e50, "NV116WHM-N49 V8.0"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x094b, &delay_200_500_e50, "NT116WHM-N21"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0951, &delay_200_500_e80, "NV116WHM-N47"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x098d, &boe_nv110wtm_n61.delay, "NV110WTM-N61"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x09ae, &delay_200_500_e200, "NT140FHM-N45"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x09dd, &delay_200_500_e50, "NT116WHM-N21"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0a5d, &delay_200_500_e50, "NV116WHM-N45"),
 	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0ac5, &delay_200_500_e50, "NV116WHM-N4C"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0b43, &delay_200_500_e200, "NV140FHM-T09"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0b56, &delay_200_500_e80, "NT140FHM-N47"),
+	EDP_PANEL_ENTRY('B', 'O', 'E', 0x0c20, &delay_200_500_e80, "NT140FHM-N47"),
 
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1132, &delay_200_500_e80_d50, "N116BGE-EA2"),
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1138, &innolux_n116bca_ea1.delay, "N116BCA-EA1-RC4"),
 	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1139, &delay_200_500_e80_d50, "N116BGE-EA2"),
 	EDP_PANEL_ENTRY('C', 'M', 'N', 0x114c, &innolux_n116bca_ea1.delay, "N116BCA-EA1"),
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1145, &delay_200_500_e80_d50, "N116BCN-EB1"),
 	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1152, &delay_200_500_e80_d50, "N116BCN-EA1"),
 	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1153, &delay_200_500_e80_d50, "N116BGE-EA2"),
 	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1154, &delay_200_500_e80_d50, "N116BCA-EA2"),
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1157, &delay_200_500_e80_d50, "N116BGE-EA2"),
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x115b, &delay_200_500_e80_d50, "N116BCN-EB1"),
 	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1247, &delay_200_500_e80_d50, "N120ACA-EA1"),
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x142b, &delay_200_500_e80_d50, "N140HCA-EAC"),
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x144f, &delay_200_500_e80_d50, "N140HGA-EA1"),
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x1468, &delay_200_500_e80, "N140HGA-EA1"),
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x14e5, &delay_200_500_e80_d50, "N140HGA-EA1"),
 	EDP_PANEL_ENTRY('C', 'M', 'N', 0x14d4, &delay_200_500_e80_d50, "N140HCA-EAC"),
+	EDP_PANEL_ENTRY('C', 'M', 'N', 0x14d6, &delay_200_500_e80_d50, "N140BGA-EA4"),
+
+	EDP_PANEL_ENTRY('H', 'K', 'C', 0x2d5c, &delay_200_500_e200, "MB116AN01-2"),
 
+	EDP_PANEL_ENTRY('I', 'V', 'O', 0x048e, &delay_200_500_e200_d10, "M116NWR6 R5"),
 	EDP_PANEL_ENTRY('I', 'V', 'O', 0x057d, &delay_200_500_e200, "R140NWF5 RH"),
 	EDP_PANEL_ENTRY('I', 'V', 'O', 0x854a, &delay_200_500_p2e100, "M133NW4J"),
 	EDP_PANEL_ENTRY('I', 'V', 'O', 0x854b, &delay_200_500_p2e100, "R133NW4K-R0"),
+	EDP_PANEL_ENTRY('I', 'V', 'O', 0x8c4d, &delay_200_150_e200, "R140NWFM R1"),
 
 	EDP_PANEL_ENTRY('K', 'D', 'B', 0x0624, &kingdisplay_kd116n21_30nv_a010.delay, "116N21-30NV-A010"),
 	EDP_PANEL_ENTRY('K', 'D', 'B', 0x1120, &delay_200_500_e80_d50, "116N29-30NK-C007"),
+	EDP_PANEL_ENTRY('K', 'D', 'C', 0x0809, &delay_200_150_e50, "KD116N2930A15"),
 
 	EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"),
 	EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &sharp_lq140m1jw46.delay, "LQ140M1JW46"),
-- 
2.42.0.869.gea05f2083d-goog

Re: [PATCH 1/3] drm/panel-edp: Add several generic edp panels

[Doug Anderson]

Hi,

On Wed, Nov 1, 2023 at 2:26 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Add a few generic edp panels used by mt8186 chromebooks.
> Besides, modify the following panel:
> - AUO 0x235c B116XTN02 renamed to B116XTN02.3.
> - AUO 0x405c B116XAK01 adjust the timing to delay_200_500_e50. According
> to the datasheet: T3=200, T12=500, T7_max = 50.

Can you modify this in the `auo_b116xak01` structure? That should make
timing work more correctly for anyone directly specifying this panel.
The reason I had it point to the other structure was so we didn't
treat anyone specifying this panel directly differently than anyone
autodetecting it...


> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 62 ++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 9dce4c702414..06ce3a73d740 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1830,6 +1830,12 @@ static const struct panel_delay delay_200_500_e50 = {
>         .enable = 50,
>  };
>
> +static const struct panel_delay delay_200_500_e80 = {
> +       .hpd_absent = 200,
> +       .unprepare = 500,
> +       .enable = 80,
> +};
> +
>  static const struct panel_delay delay_200_500_e80_d50 = {
>         .hpd_absent = 200,
>         .unprepare = 500,
> @@ -1849,6 +1855,25 @@ static const struct panel_delay delay_200_500_e200 = {
>         .enable = 200,
>  };
>
> +static const struct panel_delay delay_200_500_e200_d10 = {
> +       .hpd_absent = 200,
> +       .unprepare = 500,
> +       .enable = 200,
> +       .disable = 10,
> +};
> +
> +static const struct panel_delay delay_200_150_e50 = {
> +       .hpd_absent = 200,
> +       .unprepare = 150,

I worry a little bit about seeing "unprepare" of 150. It doesn't mean
it's wrong, but it's been so consistent for me to see .500 here that
it's good to double-confirm. It looks like you use this one on
"KD116N2930A15". From the datasheet I see that T12 has a min of 150 ms
and a max of 500 ms. Is that the same thing you see?

Specifying a "max" for T12 makes no sense. That's saying that it
violates timing specs to ever turn the panel off for more than half a
second which is nonsense.

Given that:
* The spec is obviously nonsense for this number.
* The 500 ms number is present in the spec and somewhat standard for eDP panels.
* Having a larger number is safer.
* 500 ms usually won't have a real world impact since it just prevents
turning on the panel again right after turning it off (and we use
autosuspend to avoid this in most cases).

Maybe it's better to just use 500 here?


> +       .enable = 50,
> +};
> +
> +static const struct panel_delay delay_200_150_e200 = {
> +       .hpd_absent = 200,
> +       .unprepare = 150,
> +       .enable = 200,

Let's look at this unprepare of 150 too. I guess it's used for two panels.

NT116WHM-N21: Wow, there sure are a lot of panels that call themselves
"NT116WHM-N21" but have different IDs. :-P If you're sure that this is
150 this is fine, but if there's any doubt (like above) then 500 is
safer.

R140NWFM R1: I think I found this spec and yeah, it's super clear. 150 ms.


NOTE: I didn't try to double-check any of the other timings, mostly I
just looked at the unprepare since it stood out. ;-)


> +};
> +
>  #define EDP_PANEL_ENTRY(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _delay, _name) \
>  { \
>         .name = _name, \
> @@ -1869,38 +1894,71 @@ static const struct edp_panel_entry edp_panels[] = {
>         EDP_PANEL_ENTRY('A', 'U', 'O', 0x145c, &delay_200_500_e50, "B116XAB01.4"),
>         EDP_PANEL_ENTRY('A', 'U', 'O', 0x1e9b, &delay_200_500_e50, "B133UAN02.1"),
>         EDP_PANEL_ENTRY('A', 'U', 'O', 0x1ea5, &delay_200_500_e50, "B116XAK01.6"),
> -       EDP_PANEL_ENTRY('A', 'U', 'O', 0x235c, &delay_200_500_e50, "B116XTN02"),
> -       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x208d, &delay_200_500_e50, "B140HTN02.1"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x235c, &delay_200_500_e50, "B116XTN02.3"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &delay_200_500_e50, "B116XAK01.0"),
>         EDP_PANEL_ENTRY('A', 'U', 'O', 0x582d, &delay_200_500_e50, "B133UAN01.0"),
>         EDP_PANEL_ENTRY('A', 'U', 'O', 0x615c, &delay_200_500_e50, "B116XAN06.1"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x635c, &delay_200_500_e50, "B116XAN06.3"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x639c, &delay_200_500_e50, "B140HAK02.7"),
>         EDP_PANEL_ENTRY('A', 'U', 'O', 0x8594, &delay_200_500_e50, "B133UAN01.0"),
> +       EDP_PANEL_ENTRY('A', 'U', 'O', 0xf390, &delay_200_500_e50, "B140XTN07.7"),
>
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0715, &delay_200_150_e200, "NT116WHM-N21"),
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0731, &delay_200_500_e80, "NT116WHM-N42"),
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0741, &delay_200_500_e200, "NT116WHM-N44"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0786, &delay_200_500_p2e80, "NV116WHM-T01"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x07d1, &boe_nv133fhm_n61.delay, "NV133FHM-N61"),
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x07f6, &delay_200_500_e200, "NT140FHM-N44"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x082d, &boe_nv133fhm_n61.delay, "NV133FHM-N62"),
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x08b2, &delay_200_500_e200, "NT140WHM-N49"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x09c3, &delay_200_500_e50, "NT116WHM-N21,836X2"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x094b, &delay_200_500_e50, "NT116WHM-N21"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x095f, &delay_200_500_e50, "NE135FBM-N41 v8.1"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0979, &delay_200_500_e50, "NV116WHM-N49 V8.0"),
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x094b, &delay_200_500_e50, "NT116WHM-N21"),

0x904b is already specified above and this is in the wrong sort order.

> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0951, &delay_200_500_e80, "NV116WHM-N47"),

Please sort 0x0951 before 0x0979.

>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x098d, &boe_nv110wtm_n61.delay, "NV110WTM-N61"),
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x09ae, &delay_200_500_e200, "NT140FHM-N45"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x09dd, &delay_200_500_e50, "NT116WHM-N21"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0a5d, &delay_200_500_e50, "NV116WHM-N45"),
>         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0ac5, &delay_200_500_e50, "NV116WHM-N4C"),
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0b43, &delay_200_500_e200, "NV140FHM-T09"),
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0b56, &delay_200_500_e80, "NT140FHM-N47"),
> +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0c20, &delay_200_500_e80, "NT140FHM-N47"),
>
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1132, &delay_200_500_e80_d50, "N116BGE-EA2"),
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1138, &innolux_n116bca_ea1.delay, "N116BCA-EA1-RC4"),
>         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1139, &delay_200_500_e80_d50, "N116BGE-EA2"),
>         EDP_PANEL_ENTRY('C', 'M', 'N', 0x114c, &innolux_n116bca_ea1.delay, "N116BCA-EA1"),
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1145, &delay_200_500_e80_d50, "N116BCN-EB1"),

Please sort 0x1145 above 0x114c

>         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1152, &delay_200_500_e80_d50, "N116BCN-EA1"),
>         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1153, &delay_200_500_e80_d50, "N116BGE-EA2"),
>         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1154, &delay_200_500_e80_d50, "N116BCA-EA2"),
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1157, &delay_200_500_e80_d50, "N116BGE-EA2"),
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x115b, &delay_200_500_e80_d50, "N116BCN-EB1"),
>         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1247, &delay_200_500_e80_d50, "N120ACA-EA1"),
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x142b, &delay_200_500_e80_d50, "N140HCA-EAC"),
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x144f, &delay_200_500_e80_d50, "N140HGA-EA1"),
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1468, &delay_200_500_e80, "N140HGA-EA1"),
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x14e5, &delay_200_500_e80_d50, "N140HGA-EA1"),
>         EDP_PANEL_ENTRY('C', 'M', 'N', 0x14d4, &delay_200_500_e80_d50, "N140HCA-EAC"),
> +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x14d6, &delay_200_500_e80_d50, "N140BGA-EA4"),
> +
> +       EDP_PANEL_ENTRY('H', 'K', 'C', 0x2d5c, &delay_200_500_e200, "MB116AN01-2"),
>
> +       EDP_PANEL_ENTRY('I', 'V', 'O', 0x048e, &delay_200_500_e200_d10, "M116NWR6 R5"),
>         EDP_PANEL_ENTRY('I', 'V', 'O', 0x057d, &delay_200_500_e200, "R140NWF5 RH"),
>         EDP_PANEL_ENTRY('I', 'V', 'O', 0x854a, &delay_200_500_p2e100, "M133NW4J"),
>         EDP_PANEL_ENTRY('I', 'V', 'O', 0x854b, &delay_200_500_p2e100, "R133NW4K-R0"),
> +       EDP_PANEL_ENTRY('I', 'V', 'O', 0x8c4d, &delay_200_150_e200, "R140NWFM R1"),
>
>         EDP_PANEL_ENTRY('K', 'D', 'B', 0x0624, &kingdisplay_kd116n21_30nv_a010.delay, "116N21-30NV-A010"),
>         EDP_PANEL_ENTRY('K', 'D', 'B', 0x1120, &delay_200_500_e80_d50, "116N29-30NK-C007"),
> +       EDP_PANEL_ENTRY('K', 'D', 'C', 0x0809, &delay_200_150_e50, "KD116N2930A15"),

Please sort 0x0809 above 0x1120.

Re: [PATCH 1/3] drm/panel-edp: Add several generic edp panels

[Hsin-Yi Wang]

On Thu, Nov 2, 2023 at 9:54 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Nov 1, 2023 at 2:26 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > Add a few generic edp panels used by mt8186 chromebooks.
> > Besides, modify the following panel:
> > - AUO 0x235c B116XTN02 renamed to B116XTN02.3.
> > - AUO 0x405c B116XAK01 adjust the timing to delay_200_500_e50. According
> > to the datasheet: T3=200, T12=500, T7_max = 50.
>
> Can you modify this in the `auo_b116xak01` structure? That should make
> timing work more correctly for anyone directly specifying this panel.
> The reason I had it point to the other structure was so we didn't
> treat anyone specifying this panel directly differently than anyone
> autodetecting it...
>
Will modify in v2.
>
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> >  drivers/gpu/drm/panel/panel-edp.c | 62 ++++++++++++++++++++++++++++++-
> >  1 file changed, 60 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> > index 9dce4c702414..06ce3a73d740 100644
> > --- a/drivers/gpu/drm/panel/panel-edp.c
> > +++ b/drivers/gpu/drm/panel/panel-edp.c
> > @@ -1830,6 +1830,12 @@ static const struct panel_delay delay_200_500_e50 = {
> >         .enable = 50,
> >  };
> >
> > +static const struct panel_delay delay_200_500_e80 = {
> > +       .hpd_absent = 200,
> > +       .unprepare = 500,
> > +       .enable = 80,
> > +};
> > +
> >  static const struct panel_delay delay_200_500_e80_d50 = {
> >         .hpd_absent = 200,
> >         .unprepare = 500,
> > @@ -1849,6 +1855,25 @@ static const struct panel_delay delay_200_500_e200 = {
> >         .enable = 200,
> >  };
> >
> > +static const struct panel_delay delay_200_500_e200_d10 = {
> > +       .hpd_absent = 200,
> > +       .unprepare = 500,
> > +       .enable = 200,
> > +       .disable = 10,
> > +};
> > +
> > +static const struct panel_delay delay_200_150_e50 = {
> > +       .hpd_absent = 200,
> > +       .unprepare = 150,
>
> I worry a little bit about seeing "unprepare" of 150. It doesn't mean
> it's wrong, but it's been so consistent for me to see .500 here that
> it's good to double-confirm. It looks like you use this one on
> "KD116N2930A15". From the datasheet I see that T12 has a min of 150 ms
> and a max of 500 ms. Is that the same thing you see?
>
> Specifying a "max" for T12 makes no sense. That's saying that it
> violates timing specs to ever turn the panel off for more than half a
> second which is nonsense.
>
> Given that:
> * The spec is obviously nonsense for this number.
> * The 500 ms number is present in the spec and somewhat standard for eDP panels.
> * Having a larger number is safer.
> * 500 ms usually won't have a real world impact since it just prevents
> turning on the panel again right after turning it off (and we use
> autosuspend to avoid this in most cases).
>
> Maybe it's better to just use 500 here?
>
The spec said min = 150 and max = 500.
I'll modify to 500 in v2.

>
> > +       .enable = 50,
> > +};
> > +
> > +static const struct panel_delay delay_200_150_e200 = {
> > +       .hpd_absent = 200,
> > +       .unprepare = 150,
> > +       .enable = 200,
>
> Let's look at this unprepare of 150 too. I guess it's used for two panels.
>
> NT116WHM-N21: Wow, there sure are a lot of panels that call themselves
> "NT116WHM-N21" but have different IDs. :-P If you're sure that this is
> 150 this is fine, but if there's any doubt (like above) then 500 is
> safer.
>
> R140NWFM R1: I think I found this spec and yeah, it's super clear. 150 ms.
>
>
> NOTE: I didn't try to double-check any of the other timings, mostly I
> just looked at the unprepare since it stood out. ;-)
>
0x0715 NT116WHM-N21 mentioned T12 >= 150  (no upper bound mentioned).

>
> > +};
> > +
> >  #define EDP_PANEL_ENTRY(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _delay, _name) \
> >  { \
> >         .name = _name, \
> > @@ -1869,38 +1894,71 @@ static const struct edp_panel_entry edp_panels[] = {
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x145c, &delay_200_500_e50, "B116XAB01.4"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x1e9b, &delay_200_500_e50, "B133UAN02.1"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x1ea5, &delay_200_500_e50, "B116XAK01.6"),
> > -       EDP_PANEL_ENTRY('A', 'U', 'O', 0x235c, &delay_200_500_e50, "B116XTN02"),
> > -       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x208d, &delay_200_500_e50, "B140HTN02.1"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x235c, &delay_200_500_e50, "B116XTN02.3"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &delay_200_500_e50, "B116XAK01.0"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x582d, &delay_200_500_e50, "B133UAN01.0"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x615c, &delay_200_500_e50, "B116XAN06.1"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x635c, &delay_200_500_e50, "B116XAN06.3"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x639c, &delay_200_500_e50, "B140HAK02.7"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x8594, &delay_200_500_e50, "B133UAN01.0"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0xf390, &delay_200_500_e50, "B140XTN07.7"),
> >
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0715, &delay_200_150_e200, "NT116WHM-N21"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0731, &delay_200_500_e80, "NT116WHM-N42"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0741, &delay_200_500_e200, "NT116WHM-N44"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0786, &delay_200_500_p2e80, "NV116WHM-T01"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x07d1, &boe_nv133fhm_n61.delay, "NV133FHM-N61"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x07f6, &delay_200_500_e200, "NT140FHM-N44"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x082d, &boe_nv133fhm_n61.delay, "NV133FHM-N62"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x08b2, &delay_200_500_e200, "NT140WHM-N49"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x09c3, &delay_200_500_e50, "NT116WHM-N21,836X2"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x094b, &delay_200_500_e50, "NT116WHM-N21"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x095f, &delay_200_500_e50, "NE135FBM-N41 v8.1"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0979, &delay_200_500_e50, "NV116WHM-N49 V8.0"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x094b, &delay_200_500_e50, "NT116WHM-N21"),
>
> 0x904b is already specified above and this is in the wrong sort order.
>
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0951, &delay_200_500_e80, "NV116WHM-N47"),
>
> Please sort 0x0951 before 0x0979.
>
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x098d, &boe_nv110wtm_n61.delay, "NV110WTM-N61"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x09ae, &delay_200_500_e200, "NT140FHM-N45"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x09dd, &delay_200_500_e50, "NT116WHM-N21"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0a5d, &delay_200_500_e50, "NV116WHM-N45"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0ac5, &delay_200_500_e50, "NV116WHM-N4C"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0b43, &delay_200_500_e200, "NV140FHM-T09"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0b56, &delay_200_500_e80, "NT140FHM-N47"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0c20, &delay_200_500_e80, "NT140FHM-N47"),
> >
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1132, &delay_200_500_e80_d50, "N116BGE-EA2"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1138, &innolux_n116bca_ea1.delay, "N116BCA-EA1-RC4"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1139, &delay_200_500_e80_d50, "N116BGE-EA2"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x114c, &innolux_n116bca_ea1.delay, "N116BCA-EA1"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1145, &delay_200_500_e80_d50, "N116BCN-EB1"),
>
> Please sort 0x1145 above 0x114c
>
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1152, &delay_200_500_e80_d50, "N116BCN-EA1"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1153, &delay_200_500_e80_d50, "N116BGE-EA2"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1154, &delay_200_500_e80_d50, "N116BCA-EA2"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1157, &delay_200_500_e80_d50, "N116BGE-EA2"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x115b, &delay_200_500_e80_d50, "N116BCN-EB1"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1247, &delay_200_500_e80_d50, "N120ACA-EA1"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x142b, &delay_200_500_e80_d50, "N140HCA-EAC"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x144f, &delay_200_500_e80_d50, "N140HGA-EA1"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1468, &delay_200_500_e80, "N140HGA-EA1"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x14e5, &delay_200_500_e80_d50, "N140HGA-EA1"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x14d4, &delay_200_500_e80_d50, "N140HCA-EAC"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x14d6, &delay_200_500_e80_d50, "N140BGA-EA4"),
> > +
> > +       EDP_PANEL_ENTRY('H', 'K', 'C', 0x2d5c, &delay_200_500_e200, "MB116AN01-2"),
> >
> > +       EDP_PANEL_ENTRY('I', 'V', 'O', 0x048e, &delay_200_500_e200_d10, "M116NWR6 R5"),
> >         EDP_PANEL_ENTRY('I', 'V', 'O', 0x057d, &delay_200_500_e200, "R140NWF5 RH"),
> >         EDP_PANEL_ENTRY('I', 'V', 'O', 0x854a, &delay_200_500_p2e100, "M133NW4J"),
> >         EDP_PANEL_ENTRY('I', 'V', 'O', 0x854b, &delay_200_500_p2e100, "R133NW4K-R0"),
> > +       EDP_PANEL_ENTRY('I', 'V', 'O', 0x8c4d, &delay_200_150_e200, "R140NWFM R1"),
> >
> >         EDP_PANEL_ENTRY('K', 'D', 'B', 0x0624, &kingdisplay_kd116n21_30nv_a010.delay, "116N21-30NV-A010"),
> >         EDP_PANEL_ENTRY('K', 'D', 'B', 0x1120, &delay_200_500_e80_d50, "116N29-30NK-C007"),
> > +       EDP_PANEL_ENTRY('K', 'D', 'C', 0x0809, &delay_200_150_e50, "KD116N2930A15"),
>
> Please sort 0x0809 above 0x1120.

[PATCH 2/3] drm/panel-edp: Add override_edid_mode quirk for generic edp

[Hsin-Yi Wang]

Generic edp gets mode from edid. However, some panels report incorrect
mode in this way, resulting in glitches on panel. Introduce a new quirk
additional_mode to the generic edid to pick a correct hardcoded mode.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/gpu/drm/panel/panel-edp.c | 68 ++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index 06ce3a73d740..0883ff312eba 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -203,6 +203,9 @@ struct edp_panel_entry {
 
 	/** @name: Name of this panel (for printing to logs). */
 	const char *name;
+
+	/** @override_edid_mode: Override the mode obtained by edid. */
+	const struct drm_display_mode *override_edid_mode;
 };
 
 struct panel_edp {
@@ -301,6 +304,25 @@ static unsigned int panel_edp_get_display_modes(struct panel_edp *panel,
 	return num;
 }
 
+static int panel_edp_override_edid_mode(struct panel_edp *panel,
+					struct drm_connector *connector,
+					const struct drm_display_mode *override_mode)
+{
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(connector->dev, override_mode);
+	if (mode) {
+		mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+		drm_mode_set_name(mode);
+		drm_mode_probed_add(connector, mode);
+		return 1;
+	}
+
+	dev_err(panel->base.dev, "failed to add additional mode\n");
+
+	return 0;
+}
+
 static int panel_edp_get_non_edid_modes(struct panel_edp *panel,
 					struct drm_connector *connector)
 {
@@ -568,6 +590,9 @@ static int panel_edp_get_modes(struct drm_panel *panel,
 {
 	struct panel_edp *p = to_panel_edp(panel);
 	int num = 0;
+	bool has_override_edid_mode = p->detected_panel &&
+				      p->detected_panel != ERR_PTR(-EINVAL) &&
+				      p->detected_panel->override_edid_mode;
 
 	/* probe EDID if a DDC bus is available */
 	if (p->ddc) {
@@ -575,9 +600,18 @@ static int panel_edp_get_modes(struct drm_panel *panel,
 
 		if (!p->edid)
 			p->edid = drm_get_edid(connector, p->ddc);
-
-		if (p->edid)
-			num += drm_add_edid_modes(connector, p->edid);
+		if (p->edid) {
+			if (has_override_edid_mode) {
+				/*
+				 * override_edid_mode is specified. Use
+				 * override_edid_mode instead of from edid.
+				 */
+				num += panel_edp_override_edid_mode(p, connector,
+						p->detected_panel->override_edid_mode);
+			} else {
+				num += drm_add_edid_modes(connector, p->edid);
+			}
+		}
 
 		pm_runtime_mark_last_busy(panel->dev);
 		pm_runtime_put_autosuspend(panel->dev);
@@ -950,6 +984,19 @@ static const struct panel_desc auo_b101ean01 = {
 	},
 };
 
+static const struct drm_display_mode auo_b116xa3_mode = {
+	.clock = 70589,
+	.hdisplay = 1366,
+	.hsync_start = 1366 + 40,
+	.hsync_end = 1366 + 40 + 40,
+	.htotal = 1366 + 40 + 40 + 32,
+	.vdisplay = 768,
+	.vsync_start = 768 + 10,
+	.vsync_end = 768 + 10 + 12,
+	.vtotal = 768 + 10 + 12 + 6,
+	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+};
+
 static const struct drm_display_mode auo_b116xak01_mode = {
 	.clock = 69300,
 	.hdisplay = 1366,
@@ -1882,6 +1929,15 @@ static const struct panel_delay delay_200_150_e200 = {
 	.delay = _delay \
 }
 
+#define EDP_PANEL_ENTRY2(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _delay, _name, _mode) \
+{ \
+	.name = _name, \
+	.panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, \
+					     product_id), \
+	.delay = _delay, \
+	.override_edid_mode = _mode \
+}
+
 /*
  * This table is used to figure out power sequencing delays for panels that
  * are detected by EDID. Entries here may point to entries in the
@@ -1899,9 +1955,11 @@ static const struct edp_panel_entry edp_panels[] = {
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
-	EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &delay_200_500_e50, "B116XAK01.0"),
+	EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &delay_200_500_e50, "B116XAK01.0",
+			 &auo_b116xa3_mode),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x582d, &delay_200_500_e50, "B133UAN01.0"),
-	EDP_PANEL_ENTRY('A', 'U', 'O', 0x615c, &delay_200_500_e50, "B116XAN06.1"),
+	EDP_PANEL_ENTRY2('A', 'U', 'O', 0x615c, &delay_200_500_e50, "B116XAN06.1",
+			 &auo_b116xa3_mode),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x635c, &delay_200_500_e50, "B116XAN06.3"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x639c, &delay_200_500_e50, "B140HAK02.7"),
 	EDP_PANEL_ENTRY('A', 'U', 'O', 0x8594, &delay_200_500_e50, "B133UAN01.0"),
-- 
2.42.0.869.gea05f2083d-goog

Re: [PATCH 2/3] drm/panel-edp: Add override_edid_mode quirk for generic edp

[Doug Anderson]

Hi,

On Wed, Nov 1, 2023 at 2:26 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Generic edp gets mode from edid. However, some panels report incorrect
> mode in this way, resulting in glitches on panel. Introduce a new quirk
> additional_mode to the generic edid to pick a correct hardcoded mode.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 68 ++++++++++++++++++++++++++++---
>  1 file changed, 63 insertions(+), 5 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

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

[Hsin-Yi Wang]

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(-)

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
-- 
2.42.0.869.gea05f2083d-goog

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

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

[Doug Anderson]

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2191 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Jani Nikula]

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.



-- 
Jani Nikula, Intel

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@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).

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

[Hsin-Yi Wang]

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.

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2985 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]