Re: [PATCH v4 4/5] drm/panel-edp: Match edp_panels with panel identity

From: Hsin-Yi Wang
Date: Wed Mar 06 2024 - 13:49:30 EST

On Wed, Mar 6, 2024 at 1:23 AM Jani Nikula <jani.nikula@linux.intelcom> wrote:
>
> On Tue, 05 Mar 2024, Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote:
> > It's found that some panels have variants that they share the same panel id
> > although their EDID and names are different. When matching generic edp
> > panels, we should first match with both panel identity, which contains both
> > panel id and panel name. If not found, match with panel id only.
>
> Do you want to start matching also with name, for all panels? That's
> totally up to you, but that's the big functional change here.
>

It might be difficult to find all the datasheets for all the panels to
verify the names. Also, some of the names in the panels are also
marked as "Unknown", so I think we still want to keep the matching
with id only.

Without really testing on the exact panel, I'm afraid that this might
break them.


> BR,
> Jani.
>
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
> > ---
> > v3->v4: combine name and id to identity.
> > ---
> > drivers/gpu/drm/panel/panel-edp.c | 45 ++++++++++++++++---------------
> > 1 file changed, 24 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> > index d094cfc43da8..fb70e97a2e71 100644
> > --- a/drivers/gpu/drm/panel/panel-edp.c
> > +++ b/drivers/gpu/drm/panel/panel-edp.c
> > @@ -210,15 +210,12 @@ struct panel_desc {
> > * struct edp_panel_entry - Maps panel ID to delay / panel name.
> > */
> > struct edp_panel_entry {
> > - /** @panel_id: 32-bit ID for panel, encoded with drm_edid_encode_panel_id(). */
> > - u32 panel_id;
> > + /** @ident: edid identity used for panel matching. */
> > + const struct drm_edid_ident ident;
> >
> > /** @delay: The power sequencing delays needed for this panel. */
> > const struct panel_delay *delay;
> >
> > - /** @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;
> > };
> > @@ -691,7 +688,7 @@ static int detected_panel_show(struct seq_file *s, void *data)
> > else if (!p->detected_panel)
> > seq_puts(s, "HARDCODED\n");
> > else
> > - seq_printf(s, "%s\n", p->detected_panel->name);
> > + seq_printf(s, "%s\n", p->detected_panel->ident.name);
> >
> > return 0;
> > }
> > @@ -761,7 +758,7 @@ static void panel_edp_parse_panel_timing_node(struct device *dev,
> > dev_err(dev, "Reject override mode: No display_timing found\n");
> > }
> >
> > -static const struct edp_panel_entry *find_edp_panel(u32 panel_id);
> > +static const struct edp_panel_entry *find_edp_panel(u32 panel_id, const struct drm_edid *edid);
> >
> > static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> > {
> > @@ -799,7 +796,6 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> > base_block = drm_edid_read_base_block(panel->ddc);
> > if (base_block) {
> > panel_id = drm_edid_get_panel_id(base_block);
> > - drm_edid_free(base_block);
> > } else {
> > dev_err(dev, "Couldn't identify panel via EDID\n");
> > ret = -EIO;
> > @@ -807,7 +803,9 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> > }
> > drm_edid_decode_panel_id(panel_id, vend, &product_id);
> >
> > - panel->detected_panel = find_edp_panel(panel_id);
> > + panel->detected_panel = find_edp_panel(panel_id, base_block);
> > +
> > + drm_edid_free(base_block);
> >
> > /*
> > * We're using non-optimized timings and want it really obvious that
> > @@ -840,7 +838,7 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> > panel->detected_panel = ERR_PTR(-EINVAL);
> > } else {
> > dev_info(dev, "Detected %s %s (%#06x)\n",
> > - vend, panel->detected_panel->name, product_id);
> > + vend, panel->detected_panel->ident.name, product_id);
> >
> > /* Update the delay; everything else comes from EDID */
> > desc->delay = *panel->detected_panel->delay;
> > @@ -1930,17 +1928,21 @@ static const struct panel_delay delay_200_500_e50_po2e200 = {
> >
> > #define EDP_PANEL_ENTRY(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _delay, _name) \
> > { \
> > - .name = _name, \
> > - .panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, \
> > - product_id), \
> > + .ident = { \
> > + .name = _name, \
> > + .panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, \
> > + product_id), \
> > + }, \
> > .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), \
> > + .ident = { \
> > + .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 \
> > }
> > @@ -2087,15 +2089,16 @@ static const struct edp_panel_entry edp_panels[] = {
> > { /* sentinal */ }
> > };
> >
> > -static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
> > +static const struct edp_panel_entry *find_edp_panel(u32 panel_id, const struct drm_edid *edid)
> > {
> > const struct edp_panel_entry *panel;
> >
> > - if (!panel_id)
> > - return NULL;
> > + for (panel = edp_panels; panel->ident.panel_id; panel++)
> > + if (drm_edid_match_identity(edid, &panel->ident))
> > + return panel;
> >
> > - for (panel = edp_panels; panel->panel_id; panel++)
> > - if (panel->panel_id == panel_id)
> > + for (panel = edp_panels; panel->ident.panel_id; panel++)
> > + if (panel->ident.panel_id == panel_id)
> > return panel;
> >
> > return NULL;
>
> --
> Jani Nikula, Intel