diff mbox series

drm: bridge: adv7511: get edid in hpd_work to update CEC phys address

Message ID 20231014-adv7511-cec-edid-v1-1-a58ceae0b57e@bang-olufsen.dk
State New
Headers
Series drm: bridge: adv7511: get edid in hpd_work to update CEC phys address |

Commit Message

Alvin Šipraga Oct. 14, 2023, 7:43 p.m. UTC 
  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.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
Pardon the insertion of the ugly adv7511_get_edid() prototype, but I did
not want to clobber git history by rearranging a bunch of functions. If
this is the preferred approach I will happily re-spin the patch.
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)


---
base-commit: 4366faf43308bd91c59a20c43a9f853a9c3bb6e4
change-id: 20231014-adv7511-cec-edid-ff75bd3ac929

Comments

Alvin Šipraga Oct. 14, 2023, 7:48 p.m. UTC | #1 
On Sat, Oct 14, 2023 at 09:43:01PM +0200, 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. 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.
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>


Oops, I think that I got the subject prefix wrong. It should be

    drm/bridge:

and not

    drm: bridge:

right?

Thanks.

Kind regards,
Alvin
Laurent Pinchart Oct. 16, 2023, 8:17 a.m. UTC | #2 
Hi Alvin,

Thank you for the patch.

CC'ing Hans Verkuil, to review the CEC side.

On Sat, Oct 14, 2023 at 09:43:01PM +0200, 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. 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.
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
> Pardon the insertion of the ugly adv7511_get_edid() prototype, but I did
> not want to clobber git history by rearranging a bunch of functions. If
> this is the preferred approach I will happily re-spin the patch.

There's nothing wrong in rearranging functions, it is actually preferred
to adding forward declarations. You can submit a set of two patches, one
to reorder the functions, and then a second one to fix the problem. This
makes review easier by isolating the refactoring with no functional
change from the functional changes.

> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 2611afd2c1c1..3d32c109963c 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -424,6 +424,9 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
>  	return false;
>  }
>  
> +static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
> +				     struct drm_connector *connector);
> +
>  static void adv7511_hpd_work(struct work_struct *work)
>  {
>  	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
> @@ -457,6 +460,9 @@ static void adv7511_hpd_work(struct work_struct *work)
>  		if (adv7511->connector.dev) {
>  			if (status == connector_status_disconnected)
>  				cec_phys_addr_invalidate(adv7511->cec_adap);
> +			else
> +				adv7511_get_edid(adv7511, &adv7511->connector);
> +
>  			drm_kms_helper_hotplug_event(adv7511->connector.dev);
>  		} else {
>  			drm_bridge_hpd_notify(&adv7511->bridge, status);
>
Alvin Šipraga Oct. 16, 2023, 8:46 a.m. UTC | #3 
Hi Laurent,

On Mon, Oct 16, 2023 at 11:17:49AM +0300, Laurent Pinchart wrote:
> Hi Alvin,
> 
> Thank you for the patch.
> 
> CC'ing Hans Verkuil, to review the CEC side.
> 
> On Sat, Oct 14, 2023 at 09:43:01PM +0200, 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. 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.
> > 
> > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> > ---
> > Pardon the insertion of the ugly adv7511_get_edid() prototype, but I did
> > not want to clobber git history by rearranging a bunch of functions. If
> > this is the preferred approach I will happily re-spin the patch.
> 
> There's nothing wrong in rearranging functions, it is actually preferred
> to adding forward declarations. You can submit a set of two patches, one
> to reorder the functions, and then a second one to fix the problem. This
> makes review easier by isolating the refactoring with no functional
> change from the functional changes.

Alright, good to know. I will wait a while for Hans to take a look before
sending a v2 series with two patches. Thanks!

Kind regards,
Alvin
Jani Nikula Oct. 23, 2023, 11:42 a.m. UTC | #4 
On Sat, 14 Oct 2023, 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.
>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
> Pardon the insertion of the ugly adv7511_get_edid() prototype, but I did
> not want to clobber git history by rearranging a bunch of functions. If
> this is the preferred approach I will happily re-spin the patch.
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 2611afd2c1c1..3d32c109963c 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -424,6 +424,9 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
>  	return false;
>  }
>  
> +static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
> +				     struct drm_connector *connector);
> +
>  static void adv7511_hpd_work(struct work_struct *work)
>  {
>  	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
> @@ -457,6 +460,9 @@ static void adv7511_hpd_work(struct work_struct *work)
>  		if (adv7511->connector.dev) {
>  			if (status == connector_status_disconnected)
>  				cec_phys_addr_invalidate(adv7511->cec_adap);
> +			else
> +				adv7511_get_edid(adv7511, &adv7511->connector);

This leaks the returned EDID.

BR,
Jani.

> +
>  			drm_kms_helper_hotplug_event(adv7511->connector.dev);
>  		} else {
>  			drm_bridge_hpd_notify(&adv7511->bridge, status);
>
> ---
> base-commit: 4366faf43308bd91c59a20c43a9f853a9c3bb6e4
> change-id: 20231014-adv7511-cec-edid-ff75bd3ac929
>
Alvin Šipraga Oct. 25, 2023, 7:26 a.m. UTC | #5 
Hi Jani,

On Mon, Oct 23, 2023 at 02:42:56PM +0300, Jani Nikula wrote:
> On Sat, 14 Oct 2023, Alvin Šipraga <alvin@pqrs.dk> wrote:
> > @@ -457,6 +460,9 @@ static void adv7511_hpd_work(struct work_struct *work)
> >  		if (adv7511->connector.dev) {
> >  			if (status == connector_status_disconnected)
> >  				cec_phys_addr_invalidate(adv7511->cec_adap);
> > +			else
> > +				adv7511_get_edid(adv7511, &adv7511->connector);
> 
> This leaks the returned EDID.

Thanks for catching this - I will fix in v2.

Kind regards,
Alvin
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 2611afd2c1c1..3d32c109963c 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -424,6 +424,9 @@  static bool adv7511_hpd(struct adv7511 *adv7511)
 	return false;
 }
 
+static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
+				     struct drm_connector *connector);
+
 static void adv7511_hpd_work(struct work_struct *work)
 {
 	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
@@ -457,6 +460,9 @@  static void adv7511_hpd_work(struct work_struct *work)
 		if (adv7511->connector.dev) {
 			if (status == connector_status_disconnected)
 				cec_phys_addr_invalidate(adv7511->cec_adap);
+			else
+				adv7511_get_edid(adv7511, &adv7511->connector);
+
 			drm_kms_helper_hotplug_event(adv7511->connector.dev);
 		} else {
 			drm_bridge_hpd_notify(&adv7511->bridge, status);