| 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: <20240305152254.GG12482@pendragon.ideasonboard.com>
Date: Tue, 5 Mar 2024 17:22:54 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Alvin Šipraga <alvin@...s.dk>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>, Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...il.com>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Alvin Šipraga <alsi@...g-olufsen.dk>
Subject: Re: [PATCH v3 2/2] drm/bridge: adv7511: get edid in hpd_work to
update CEC phys address
Hello Alvin,
Thank you for the patch.
On Mon, Feb 19, 2024 at 09:12:59PM +0100, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@...g-olufsen.dk>
>
> The adv7511 driver is solely responsible for setting the physical
> address of its CEC adapter. To do this, it must read the EDID. However,
> EDID is only read when either the drm_bridge_funcs :: get_edid or
> drm_connector_helper_funcs :: get_modes ops are called. Without loss of
> generality, it cannot be assumed that these ops are called when a sink
> gets attached.
I wonder if that should be fixed, but it would likely be a quite big
rework of the DRM core. I've been thinking for several years now that
hotplug handling could do with more love.
> Therefore there exist scenarios in which the CEC physical
> address will be invalid (f.f.f.f), rendering the CEC adapter inoperable.
>
> Address this problem by always fetching the EDID in the HPD work when we
> detect a connection. The CEC physical address is set in the process.
> This is done by moving the EDID DRM helper into an internal helper
> function so that it can be cleanly called from an earlier section of
> the code. The EDID getter has not changed in practice.
>
> Reviewed-by: Robert Foss <rfoss@...nel.org>
> Signed-off-by: Alvin Šipraga <alsi@...g-olufsen.dk>
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 73 ++++++++++++++++++----------
> 1 file changed, 47 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 5ffc5904bd59..d823b372ff43 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -542,6 +542,36 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
> return 0;
> }
>
> +static struct edid *__adv7511_get_edid(struct adv7511 *adv7511,
> + struct drm_connector *connector)
> +{
> + struct edid *edid;
> +
> + /* Reading the EDID only works if the device is powered */
> + if (!adv7511->powered) {
> + unsigned int edid_i2c_addr =
> + (adv7511->i2c_edid->addr << 1);
> +
> + __adv7511_power_on(adv7511);
> +
> + /* Reset the EDID_I2C_ADDR register as it might be cleared */
> + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> + edid_i2c_addr);
> + }
> +
> + edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> +
> + if (!adv7511->powered)
> + __adv7511_power_off(adv7511);
> +
> + adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
> + drm_detect_hdmi_monitor(edid));
> +
> + cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
> +
> + return edid;
> +}
> +
> /* -----------------------------------------------------------------------------
> * Hotplug handling
> */
> @@ -595,8 +625,23 @@ static void adv7511_hpd_work(struct work_struct *work)
> adv7511->connector.status = status;
>
> if (adv7511->connector.dev) {
> - if (status == connector_status_disconnected)
> + if (status == connector_status_disconnected) {
> cec_phys_addr_invalidate(adv7511->cec_adap);
> + } else {
> + struct edid *edid;
> +
> + /*
> + * Get the updated EDID so that the CEC
> + * subsystem gets informed of any change in CEC
> + * address. The helper returns a newly allocated
> + * edid structure, so free it to prevent
> + * leakage.
> + */
> + edid = __adv7511_get_edid(adv7511,
> + &adv7511->connector);
> + kfree(edid);
This means that we will, in most case, fetch EDID twice when a monitor
is plugged in: once here, and once when the DRM core will call the
get_edid() operation. I wonder, would it make sense to cache the EDID
here, and return the cache version from adv7511_get_edid() ?
> + }
> +
> drm_kms_helper_hotplug_event(adv7511->connector.dev);
> } else {
> drm_bridge_hpd_notify(&adv7511->bridge, status);
> @@ -611,31 +656,7 @@ static void adv7511_hpd_work(struct work_struct *work)
> static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
> struct drm_connector *connector)
> {
> - struct edid *edid;
> -
> - /* Reading the EDID only works if the device is powered */
> - if (!adv7511->powered) {
> - unsigned int edid_i2c_addr =
> - (adv7511->i2c_edid->addr << 1);
> -
> - __adv7511_power_on(adv7511);
> -
> - /* Reset the EDID_I2C_ADDR register as it might be cleared */
> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> - edid_i2c_addr);
> - }
> -
> - edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> -
> - if (!adv7511->powered)
> - __adv7511_power_off(adv7511);
> -
> - adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
> - drm_detect_hdmi_monitor(edid));
> -
> - cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
> -
> - return edid;
> + return __adv7511_get_edid(adv7511, connector);
> }
>
> static int adv7511_get_modes(struct adv7511 *adv7511,
>
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists