Re: [PATCH] drm/bridge_connector: enable HPD by default if supported

[Tomi Valkeinen]

On 31/08/2022 16:02, Tomi Valkeinen wrote:
> Hi,
>
> On 23/02/2022 19:02, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2022-02-23 16:25:28)
> > > Hello,
> > >
> > > On Wed, Feb 23, 2022 at 04:17:22PM +0000, Kieran Bingham wrote:
> > > > Quoting Laurent Pinchart (2021-12-29 23:44:29)
> > > > > On Sat, Dec 25, 2021 at 09:31:51AM +0300, Nikita Yushchenko wrote:
> > > > > > Hotplug events reported by bridge drivers over 
> > > > > > drm_bridge_hpd_notify()
> > > > > > get ignored unless somebody calls drm_bridge_hpd_enable(). When the
> > > > > > connector for the bridge is bridge_connector, such a call is done 
> > > > > > from
> > > > > > drm_bridge_connector_enable_hpd().
> > > > > >
> > > > > > However drm_bridge_connector_enable_hpd() is never called on init 
> > > > > > paths,
> > > > > > documentation suggests that it is intended for suspend/resume paths.
> > > > >
> > > > > Hmmmm... I'm in two minds about this. The problem description is
> > > > > correct, but I wonder if HPD should be enabled unconditionally 
> > > > > here, or
> > > > > if this should be left to display drivers to control.
> > > > > drivers/gpu/drm/imx/dcss/dcss-kms.c enables HPD manually at init time,
> > > > > other drivers don't.
> > > > >
> > > > > It feels like this should be under control of the display controller
> > > > > driver, but I can't think of a use case for not enabling HPD at init
> > > > > time. Any second opinion from anyone ?
> > > >
> > > > This patch solves an issue I have where I have recently enabled HPD on
> > > > the SN65DSI86, but without this, I do not get calls to my 
> > > > .hpd_enable or
> > > > .hpd_disable hooks that I have added to the ti_sn_bridge_funcs.
> > > >
> > > > So it needs to be enabled somewhere, and this seems reasonable to me?
> > > > It's not directly related to the display controller - as it's a factor
> > > > of the bridge?
> > > >
> > > > On Falcon-V3U with HPD additions to SN65DSI86:
> > > > Tested-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> > >
> > > If you think this is right, then
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >
> > I do, and at the very least it works for me, and fixes Nikita's issue so
> > to me that's enough for:
>
> So who disables the HPD now?
>
> Is the drm_bridge_connector_enable_hpd & 
> drm_bridge_connector_disable_hpd documentation now wrong as they talk 
> about suspend/resume handlers?

To give more context, currently omapdrm enables the HPDs at init time 
and disables them at remove time. With this patch, the HPDs are enabled 
twice, leading to a WARN.

imx and msm drivers also seem to call drm_bridge_connector_enable_hpd(), 
so I would guess those also WARN with this patch.

Afaics, this patch alone is broken, as it just disregards the drivers 
that already enable the HPD, and also as it doesn't handle the disabling 
of the HPD, or give any guidelines on how the drivers should now manage 
the HPD.

My suggestion is to revert this one.

 Tomi