* Re: [PATCH] drm: bridge: adv7511: get edid in hpd_work to update CEC phys address
2023-10-14 19:43 [PATCH] drm: bridge: adv7511: get edid in hpd_work to update CEC phys address Alvin Šipraga
@ 2023-10-14 19:48 ` Alvin Šipraga
2023-10-16 8:17 ` Laurent Pinchart
2023-10-23 11:42 ` Jani Nikula
2 siblings, 0 replies; 6+ messages in thread
From: Alvin Šipraga @ 2023-10-14 19:48 UTC (permalink / raw)
To: Alvin Šipraga
Cc: Neil Armstrong, Robert Foss, Jonas Karlman, dri-devel,
linux-kernel, Jernej Skrabec, Laurent Pinchart, Andrzej Hajda
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: bridge: adv7511: get edid in hpd_work to update CEC phys address
2023-10-14 19:43 [PATCH] drm: bridge: adv7511: get edid in hpd_work to update CEC phys address Alvin Šipraga
2023-10-14 19:48 ` Alvin Šipraga
@ 2023-10-16 8:17 ` Laurent Pinchart
2023-10-16 8:46 ` Alvin Šipraga
2023-10-23 11:42 ` Jani Nikula
2 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2023-10-16 8:17 UTC (permalink / raw)
To: Alvin Šipraga
Cc: Neil Armstrong, Robert Foss, Jonas Karlman, linux-kernel,
Jernej Skrabec, dri-devel, Andrzej Hajda, Alvin Šipraga,
Hans Verkuil
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);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: bridge: adv7511: get edid in hpd_work to update CEC phys address
2023-10-16 8:17 ` Laurent Pinchart
@ 2023-10-16 8:46 ` Alvin Šipraga
0 siblings, 0 replies; 6+ messages in thread
From: Alvin Šipraga @ 2023-10-16 8:46 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Neil Armstrong, Robert Foss, Jonas Karlman, linux-kernel,
Jernej Skrabec, dri-devel, Andrzej Hajda, Alvin Šipraga,
Hans Verkuil
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: bridge: adv7511: get edid in hpd_work to update CEC phys address
2023-10-14 19:43 [PATCH] drm: bridge: adv7511: get edid in hpd_work to update CEC phys address Alvin Šipraga
2023-10-14 19:48 ` Alvin Šipraga
2023-10-16 8:17 ` Laurent Pinchart
@ 2023-10-23 11:42 ` Jani Nikula
2023-10-25 7:26 ` Alvin Šipraga
2 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2023-10-23 11:42 UTC (permalink / raw)
To: Alvin Šipraga, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie,
Daniel Vetter
Cc: linux-kernel, dri-devel, Alvin Šipraga
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
>
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: bridge: adv7511: get edid in hpd_work to update CEC phys address
2023-10-23 11:42 ` Jani Nikula
@ 2023-10-25 7:26 ` Alvin Šipraga
0 siblings, 0 replies; 6+ messages in thread
From: Alvin Šipraga @ 2023-10-25 7:26 UTC (permalink / raw)
To: Jani Nikula
Cc: Neil Armstrong, Robert Foss, Jonas Karlman, dri-devel,
linux-kernel, Jernej Skrabec, Alvin Šipraga,
Laurent Pinchart, Andrzej Hajda
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
^ permalink raw reply [flat|nested] 6+ messages in thread