Re: [PATCH v3] drm/msm/dp: enable HDP plugin/unplugged interrupts at hpd_enable/disable

[Bjorn Andersson <andersson@kernel.org>]

On Mon, May 22, 2023 at 03:35:13PM -0700, Kuogee Hsieh wrote:
> 
> 
> > > >   -static void dp_display_config_hpd(struct dp_display_private *dp)
> > > > -{
> > > > -
> > > > -    dp_display_host_init(dp);
> > > > -    dp_catalog_ctrl_hpd_config(dp->catalog);
> > > > -
> > > > -    /* Enable plug and unplug interrupts only if requested */
> > > > -    if (dp->dp_display.internal_hpd)
> > > > -        dp_catalog_hpd_config_intr(dp->catalog,
> > > > -                DP_DP_HPD_PLUG_INT_MASK |
> > > > -                DP_DP_HPD_UNPLUG_INT_MASK,
> > > > -                true);
> > > > -
> > > > -    /* Enable interrupt first time
> > > > -     * we are leaving dp clocks on during disconnect
> > > > -     * and never disable interrupt
> > > > -     */
> > > > -    enable_irq(dp->irq);
> > > 
> > > ...we need dp->irq enabled for handling the other interrupts, otherwise
> > > e.g. AUX transfers will time out.
> > > 
> > > I added enable_irq(dp_priv->irq) to the EV_HPD_INIT_SETUP case below,
> > > just for testing, and with that the patch seems to be working fine.
> > > 
> > > 
> > > Is there any reason why we need to delay its enablement to after we
> > > unmask the HPD interrupts?
> 
> my though is aux transaction should happen after plugin interrupt detected.
> 

I have a next_bridge which implements DRM_BRIDGE_OP_HPD, in which case
dp_bridge_hpd_enable() will never be called (the next_bridge's
hpd_enable will be called instead).

> can you please let me know what did you do to trigger aux timeout?
> 

In this setup I just connect the cable, wait a few seconds and the
transfers fails - as there's no interrupts signalling them being
completed.

> It does not happen on me at my test.
> 

Given that you have the HPD signal on a GPIO, you should be able to
rewrite your DTS in line with:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28

And pinmux the HPD signal as GPIO instead.

I believe this would allow you to test both code paths - without the
actual TCPM known to Linux.

> > 
> > As I wrote, I'd probably prefer to see enable_irq() and disable_irq()
> > calls gone.
> 
> ok, i will move enable_irq() out of here.
> 

Thanks,
Bjorn

> 
> > > 
> > > Regards,
> > > Bjorn
> > > 
> >