Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending

[aravindh]

On 2021-04-21 10:26, khsieh@xxxxxxxxxxxxxx wrote:
> On 2021-04-20 15:01, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2021-04-16 13:27:57)
> > > Some dongle may generate more than one irq_hpd events in a short 
> > > period of
> > > time. This patch will treat those irq_hpd events as single one and 
> > > service
> > > only one irq_hpd event.
> >
> > Why is it bad to get multiple irq_hpd events in a short period of 
> > time?
> > Please tell us here in the commit text.
> >
> > > Signed-off-by: Kuogee Hsieh <khsieh@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/msm/dp/dp_display.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 5a39da6..0a7d383 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -707,6 +707,9 @@ static int dp_irq_hpd_handle(struct 
> > > dp_display_private *dp, u32 data)
> > >                 return 0;
> > >         }
> > >
> > > +       /* only handle first irq_hpd in case of multiple irs_hpd 
> > > pending */
> > > +       dp_del_event(dp, EV_IRQ_HPD_INT);
> > > +
> > >         ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
> > >         if (ret == -ECONNRESET) { /* cable unplugged */
> > >                 dp->core_initialized = false;
> > > @@ -1300,6 +1303,9 @@ static int dp_pm_suspend(struct device *dev)
> > >         /* host_init will be called at pm_resume */
> > >         dp->core_initialized = false;
> > >
> > > +       /* system suspended, delete pending irq_hdps */
> > > +       dp_del_event(dp, EV_IRQ_HPD_INT);
> >
> > What happens if I suspend my device and when this function is running 
> > I
> > toggle my monitor to use the HDMI input that is connected instead of 
> > some
> > other input, maybe the second HDMI input? Wouldn't that generate an 
> > HPD
> > interrupt to grab the attention of this device?
> no,
> At this time display is off. this mean dp controller is off and
> mainlink has teared down.
> it will start with plug in interrupt to bring dp controller up and
> start link training.
> irq_hpd can be generated only panel is at run time of operation mode
> and need attention from host.
> If host is shutting down, then no need to service pending irq_hpd.
>
> > > +
> > >         mutex_unlock(&dp->event_mutex);
> > >
> > >         return 0;
> > > @@ -1496,6 +1502,9 @@ int msm_dp_display_disable(struct msm_dp *dp, 
> > > struct drm_encoder *encoder)
> > >         /* stop sentinel checking */
> > >         dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
> > >
> > > +       /* link is down, delete pending irq_hdps */
> > > +       dp_del_event(dp_display, EV_IRQ_HPD_INT);
> > > +
> >
> > I'm becoming convinced that the whole kthread design and event queue 
> > is
> > broken. These sorts of patches are working around the larger problem
> > that the kthread is running independently of the driver and irqs can
> > come in at any time but the event queue is not checked from the irq
> > handler to debounce the irq event. Is the event queue necessary at 
> > all?
> > I wonder if it would be simpler to just use an irq thread and process
> > the hpd signal from there. Then we're guaranteed to not get an irq 
> > again
> > until the irq thread is done processing the event. This would 
> > naturally
> > debounce the irq hpd event that way.
> event q just like bottom half of irq handler. it turns irq into event
> and handle them sequentially.
> irq_hpd is asynchronous event from panel to bring up attention of hsot
> during run time of operation.
> Here, the dongle is unplugged and main link had teared down so that no
> need to service pending irq_hpd if any.

As Kuogee mentioned, IRQ_HPD is a message received from the panel and is 
not like your typical HW generated IRQ. There is no guarantee that we 
will not receive an IRQ_HPD until we are finished with processing of an 
earlier HPD message or an IRQ_HPD message. For example - when you run 
the protocol compliance, when we get a HPD from the sink, we are 
expected to start reading DPCD, EDID and proceed with link training. As 
soon as link training is finished (which is marked by a specific DPCD 
register write), the sink is going to issue an IRQ_HPD. At this point, 
we may not done with processing the HPD high as after link training we 
would typically notify the user mode of the newly connected display, 
etc.
> > >         dp_display_disable(dp_display, 0);
> > >
> > >         rc = dp_display_unprepare(dp);