| Message ID | 20240219-adv7511-cec-edid-v3-2-445aed2f1cd7@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 |
Sorry to ask for this again, but this series has non-trivial merge conflicts with upstream again. Could you rebase it and send out an updated version?
Hello Alvin, Thank you for the patch. On Mon, Feb 19, 2024 at 09:12:59PM +0100, Alvin Šipraga 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. 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@kernel.org> > Signed-off-by: Alvin Šipraga <alsi@bang-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, >
On Tue, 05 Mar 2024, Robert Foss <rfoss@kernel.org> wrote: > Sorry to ask for this again, but this series has non-trivial merge > conflicts with upstream again. > > Could you rebase it and send out an updated version? Not just rebase, but all of bridge has been switched over to struct drm_edid based interfaces. BR, Jani.
On Mon, 19 Feb 2024, 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. > > Reviewed-by: Robert Foss <rfoss@kernel.org> > Signed-off-by: Alvin Šipraga <alsi@bang-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); It really would be better to do drm_edid_read_custom(), drm_edid_connector_update(), and cec_s_phys_addr() with the physical address from connector->display_info.source_physical_address initialized by drm_edid_connector_update(). The point is, cec_s_phys_addr_from_edid() has its own duplicate EDID parsing, which is slightly different from drm_edid_connector_update() and of course redundant. BR, Jani. > + > + 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); > + } > + > 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,
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); + } + 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,