lkml.org 
[lkml]   [2021]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending
On 2021-04-21 10:26, khsieh@codeaurora.org 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@codeaurora.org>
>>> ---
>>> 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);

\
 
 \ /
  Last update: 2021-04-21 21:01    [W:0.052 / U:0.604 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site