On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > > On Fri, 23 Feb 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. Besides panel id, now we need > > the hash of entire EDID base block to distinguish these panel variants. > > > > Add drm_edid_get_base_block to returns the EDID base block, so caller can > > further use it to get panel id and/or the hash. > > Please reconsider the whole approach here. > > Please let's not add single-use special case functions to read an EDID > base block. > > Please consider reading the whole EDID, using the regular EDID reading > functions, and use that instead. > > Most likely you'll only have 1-2 blocks anyway. And you might consider > caching the EDID in struct panel_edp if reading the entire EDID is too > slow. (And if it is, this is probably sensible even if the EDID only > consists of one block.) > > Anyway, please do *not* merge this as-is. > hi Jani, I sent a v2 here implementing this method: https://lore.kernel.org/lkml/20240228011133.1238439-2-hsinyi@xxxxxxxxxxxx/ We still have to read edid twice due to: 1. The first caller is in panel probe, at that time, connector is still unknown, so we can't update connector status (eg. update edid_corrupt). 2. It's possible that the connector can have some override (drm_edid_override_get) to EDID, that is still unknown during the first read. > BR, > Jani. > > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> > > --- > > drivers/gpu/drm/drm_edid.c | 55 +++++++++++++++++-------------- > > drivers/gpu/drm/panel/panel-edp.c | 8 +++-- > > include/drm/drm_edid.h | 3 +- > > 3 files changed, 38 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 923c4423151c..55598ca4a5d1 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -2770,58 +2770,63 @@ static u32 edid_extract_panel_id(const struct edid *edid) > > } > > > > /** > > - * drm_edid_get_panel_id - Get a panel's ID through DDC > > - * @adapter: I2C adapter to use for DDC > > + * drm_edid_get_panel_id - Get a panel's ID from EDID base block > > + * @base_bock: EDID base block that contains panel ID. > > * > > - * This function reads the first block of the EDID of a panel and (assuming > > + * This function uses the first block of the EDID of a panel and (assuming > > * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value > > * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's > > * supposed to be different for each different modem of panel. > > * > > + * Return: A 32-bit ID that should be different for each make/model of panel. > > + * See the functions drm_edid_encode_panel_id() and > > + * drm_edid_decode_panel_id() for some details on the structure of this > > + * ID. > > + */ > > +u32 drm_edid_get_panel_id(void *base_block) > > +{ > > + return edid_extract_panel_id(base_block); > > +} > > +EXPORT_SYMBOL(drm_edid_get_panel_id); > > + > > +/** > > + * drm_edid_get_base_block - Get a panel's EDID base block > > + * @adapter: I2C adapter to use for DDC > > + * > > + * This function returns the first block of the EDID of a panel. > > + * > > * This function is intended to be used during early probing on devices where > > * more than one panel might be present. Because of its intended use it must > > - * assume that the EDID of the panel is correct, at least as far as the ID > > - * is concerned (in other words, we don't process any overrides here). > > + * assume that the EDID of the panel is correct, at least as far as the base > > + * block is concerned (in other words, we don't process any overrides here). > > * > > * NOTE: it's expected that this function and drm_do_get_edid() will both > > * be read the EDID, but there is no caching between them. Since we're only > > * reading the first block, hopefully this extra overhead won't be too big. > > * > > - * Return: A 32-bit ID that should be different for each make/model of panel. > > - * See the functions drm_edid_encode_panel_id() and > > - * drm_edid_decode_panel_id() for some details on the structure of this > > - * ID. > > + * Caller should free the base block after use. > > */ > > - > > -u32 drm_edid_get_panel_id(struct i2c_adapter *adapter) > > +void *drm_edid_get_base_block(struct i2c_adapter *adapter) > > { > > enum edid_block_status status; > > void *base_block; > > - u32 panel_id = 0; > > - > > - /* > > - * There are no manufacturer IDs of 0, so if there is a problem reading > > - * the EDID then we'll just return 0. > > - */ > > > > base_block = kzalloc(EDID_LENGTH, GFP_KERNEL); > > if (!base_block) > > - return 0; > > + return NULL; > > > > status = edid_block_read(base_block, 0, drm_do_probe_ddc_edid, adapter); > > > > edid_block_status_print(status, base_block, 0); > > > > - if (edid_block_status_valid(status, edid_block_tag(base_block))) > > - panel_id = edid_extract_panel_id(base_block); > > - else > > + if (!edid_block_status_valid(status, edid_block_tag(base_block))) { > > edid_block_dump(KERN_NOTICE, base_block, 0); > > + return NULL; > > + } > > > > - kfree(base_block); > > - > > - return panel_id; > > + return base_block; > > } > > -EXPORT_SYMBOL(drm_edid_get_panel_id); > > +EXPORT_SYMBOL(drm_edid_get_base_block); > > > > /** > > * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output > > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > > index bd71d239272a..f6ddbaa103b5 100644 > > --- a/drivers/gpu/drm/panel/panel-edp.c > > +++ b/drivers/gpu/drm/panel/panel-edp.c > > @@ -763,6 +763,7 @@ static const struct edp_panel_entry *find_edp_panel(u32 panel_id); > > static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel) > > { > > struct panel_desc *desc; > > + void *base_block; > > u32 panel_id; > > char vend[4]; > > u16 product_id; > > @@ -792,8 +793,11 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel) > > goto exit; > > } > > > > - panel_id = drm_edid_get_panel_id(panel->ddc); > > - if (!panel_id) { > > + base_block = drm_edid_get_base_block(panel->ddc); > > + if (base_block) { > > + panel_id = drm_edid_get_panel_id(base_block); > > + kfree(base_block); > > + } else { > > dev_err(dev, "Couldn't identify panel via EDID\n"); > > ret = -EIO; > > goto exit; > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > > index 7923bc00dc7a..56b60f9204d3 100644 > > --- a/include/drm/drm_edid.h > > +++ b/include/drm/drm_edid.h > > @@ -410,7 +410,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, > > void *data); > > struct edid *drm_get_edid(struct drm_connector *connector, > > struct i2c_adapter *adapter); > > -u32 drm_edid_get_panel_id(struct i2c_adapter *adapter); > > +void *drm_edid_get_base_block(struct i2c_adapter *adapter); > > +u32 drm_edid_get_panel_id(void *base_block); > > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, > > struct i2c_adapter *adapter); > > struct edid *drm_edid_duplicate(const struct edid *edid); > > -- > Jani Nikula, Intel
