| Message ID | 20231124-adv7511-cec-edid-v2-2-f0e5eeafdfc2@bang-olufsen.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: adv7511: get edid in hpd_work to update CEC phys address | expand |
On Fri, Nov 24, 2023 at 4:20 PM Alvin Šipraga <alvin@pqrs.dk> wrote: > > From: Alvin Šipraga <alsi@bang-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. 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. > > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 74 ++++++++++++++++++---------- > 1 file changed, 48 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..1f1d3a440895 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,24 @@ 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); > + if (edid) > + kfree(edid); kfree(NULL) is safe, so the if statement can be removed. With this fixed, feel free to add my r-b to this full series. Reviewed-by: Robert Foss <rfoss@kernel.org>
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 5ffc5904bd59..1f1d3a440895 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,24 @@ 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); + if (edid) + kfree(edid); + } + drm_kms_helper_hotplug_event(adv7511->connector.dev); } else { drm_bridge_hpd_notify(&adv7511->bridge, status); @@ -611,31 +657,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,