| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJMQK-jSPg6vU3SLmRy7zwNHJ4yqO2hT6RaiYxA4ifZ7CzwD9Q@mail.gmail.com>
Date: Tue, 27 Feb 2024 17:27:30 -0800
From: Hsin-Yi Wang <hsinyi@...omium.org>
To: Jani Nikula <jani.nikula@...ux.intel.com>
Cc: Douglas Anderson <dianders@...omium.org>, Neil Armstrong <neil.armstrong@...aro.org>,
Jessica Zhang <quic_jesszhan@...cinc.com>, Sam Ravnborg <sam@...nborg.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block
On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula <jani.nikula@...ux.intel.com> wrote:
>
> On Fri, 23 Feb 2024, Hsin-Yi Wang <hsinyi@...omium.org> 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@chromium.org/
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@...omium.org>
> > ---
> > 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
Powered by blists - more mailing lists