[v2,2/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address
Commit Message
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(-)
Comments
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>
@@ -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,