| Message ID | 20240306200353.1436198-7-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Match panel with identity | expand |
Hi, On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > @@ -1009,6 +1009,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, > @@ -1990,7 +2003,9 @@ 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, &auo_b116xak01.delay, "B116XAK01.0"), > + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"), > + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ", Remove the trailing space from the string above now? Aside from that: Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Wed, 06 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: >> >> @@ -1009,6 +1009,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, >> @@ -1990,7 +2003,9 @@ 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, &auo_b116xak01.delay, "B116XAK01.0"), >> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"), >> + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ", > > Remove the trailing space from the string above now? Maybe it actually needs to be considered part of the name; see my other reply in the earlier patch. > > Aside from that: > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Thu, Mar 7, 2024 at 5:28 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Wed, 06 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > > > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > >> > >> @@ -1009,6 +1009,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, > >> @@ -1990,7 +2003,9 @@ 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, &auo_b116xak01.delay, "B116XAK01.0"), > >> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"), > >> + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ", > > > > Remove the trailing space from the string above now? > > Maybe it actually needs to be considered part of the name; see my other > reply in the earlier patch. > I randomly checked 3 of the AUO panels that I had a datasheet with, and all of them have a white space padding before \n. The descriptor of that field is marked as "Reserved for definition", unlike other characters, representing the name, which are marked with "Manufacture P/N". For this example, do we still want to consider the white space part of the name? I know they didn't follow the spec exactly. > > > > Aside from that: > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > -- > Jani Nikula, Intel
On Thu, 07 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote: > On Thu, Mar 7, 2024 at 5:28 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: >> >> On Wed, 06 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: >> > Hi, >> > >> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: >> >> >> >> @@ -1009,6 +1009,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, >> >> @@ -1990,7 +2003,9 @@ 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, &auo_b116xak01.delay, "B116XAK01.0"), >> >> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"), >> >> + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ", >> > >> > Remove the trailing space from the string above now? >> >> Maybe it actually needs to be considered part of the name; see my other >> reply in the earlier patch. >> > I randomly checked 3 of the AUO panels that I had a datasheet with, > and all of them have a white space padding before \n. > The descriptor of that field is marked as "Reserved for definition", > unlike other characters, representing the name, which are marked with > "Manufacture P/N". > > For this example, do we still want to consider the white space part of > the name? I know they didn't follow the spec exactly. If there's one thing that's for sure, EDIDs are full of stuff like this, across the board. Ignoring the whitespace at the end seemed reasonable, initially, to me too. But the question is, if we start catering for this, what else should we cater for? Do we keep adding "reasonable" interpretations, or just go by the spec? BR, Jani. > >> > >> > Aside from that: >> > >> > Reviewed-by: Douglas Anderson <dianders@chromium.org> >> >> -- >> Jani Nikula, Intel
Hi, On Thu, Mar 7, 2024 at 12:28 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Thu, 07 Mar 2024, Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > On Thu, Mar 7, 2024 at 5:28 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> > >> On Wed, 06 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: > >> > Hi, > >> > > >> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > >> >> > >> >> @@ -1009,6 +1009,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, > >> >> @@ -1990,7 +2003,9 @@ 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, &auo_b116xak01.delay, "B116XAK01.0"), > >> >> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"), > >> >> + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ", > >> > > >> > Remove the trailing space from the string above now? > >> > >> Maybe it actually needs to be considered part of the name; see my other > >> reply in the earlier patch. > >> > > I randomly checked 3 of the AUO panels that I had a datasheet with, > > and all of them have a white space padding before \n. > > The descriptor of that field is marked as "Reserved for definition", > > unlike other characters, representing the name, which are marked with > > "Manufacture P/N". > > > > For this example, do we still want to consider the white space part of > > the name? I know they didn't follow the spec exactly. > > If there's one thing that's for sure, EDIDs are full of stuff like this, > across the board. > > Ignoring the whitespace at the end seemed reasonable, initially, to me > too. But the question is, if we start catering for this, what else > should we cater for? Do we keep adding "reasonable" interpretations, or > just go by the spec? Personally, I don't really care a whole lot either way. If I had to make a judgement call I think it's a little cleaner the way Hsin-Yi has it where we ignore whitespace at the end. Given that Dmitry also suggested ignoring whitespace at the end [1] I guess I'd believe that he also feels it's a little cleaner that way. However, If the only way to get the patch series landed is to put the space at the end of the name in panel-edp.c then I'm OK with that. In terms of what else we should cater to, I guess we'd have to answer that question when it comes up, with a bias against adding more special case rules. _Hopefully_ it won't be common that we even need this code and it will be the exception rather than the rule that panels with incompatible timings have the same panel ID anyway... In any case, hopefully the above explains my opinion on this. If you feel strongly that we should remove the code handling whitespace at the end then so be it. If you're on the fence then I guess I'd say let's keep it... [1] https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@mail.gmail.com/
On Thu, 07 Mar 2024, Doug Anderson <dianders@chromium.org> wrote: > On Thu, Mar 7, 2024 at 12:28 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: >> If there's one thing that's for sure, EDIDs are full of stuff like this, >> across the board. >> >> Ignoring the whitespace at the end seemed reasonable, initially, to me >> too. But the question is, if we start catering for this, what else >> should we cater for? Do we keep adding "reasonable" interpretations, or >> just go by the spec? > > Personally, I don't really care a whole lot either way. If I had to > make a judgement call I think it's a little cleaner the way Hsin-Yi > has it where we ignore whitespace at the end. Given that Dmitry also > suggested ignoring whitespace at the end [1] I guess I'd believe that > he also feels it's a little cleaner that way. However, If the only way > to get the patch series landed is to put the space at the end of the > name in panel-edp.c then I'm OK with that. > > In terms of what else we should cater to, I guess we'd have to answer > that question when it comes up, with a bias against adding more > special case rules. _Hopefully_ it won't be common that we even need > this code and it will be the exception rather than the rule that > panels with incompatible timings have the same panel ID anyway... > > In any case, hopefully the above explains my opinion on this. If you > feel strongly that we should remove the code handling whitespace at > the end then so be it. If you're on the fence then I guess I'd say > let's keep it... No, I don't feel strongly, let's go with this. It's not like it's cast in stone either. BR, Jani. > > > [1] https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@mail.gmail.com/
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 772bf6011d79..e3de55314bda 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -1009,6 +1009,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, @@ -1990,7 +2003,9 @@ 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, &auo_b116xak01.delay, "B116XAK01.0"), + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"), + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ", + &auo_b116xa3_mode), EDP_PANEL_ENTRY('A', 'U', 'O', 0x435c, &delay_200_500_e50, "Unknown"), 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"),
There are 2 different AUO panels using the same panel id. One of the variants requires using overridden modes to resolve glitching issue as described in commit 70e0d5550f5c ("drm/panel-edp: Add auo_b116xa3_mode"). Other variants should use the modes parsed from EDID. Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> --- v5: no change --- drivers/gpu/drm/panel/panel-edp.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)