From: aravindh@codeaurora.org
To: khsieh@codeaurora.org
Cc: Stephen Boyd <swboyd@chromium.org>,
robdclark@gmail.com, sean@poorly.run, abhinavk@codeaurora.org,
airlied@linux.ie, daniel@ffwll.ch, linux-arm-msm@vger.kernel.org,
dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending
Date: Wed, 21 Apr 2021 11:55:21 -0700 [thread overview]
Message-ID: <ddc1e372c5f864cd62c4e056ef2e6404@codeaurora.org> (raw)
In-Reply-To: <e3c3ef96ac507da6f138106f70c78ed2@codeaurora.org>
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);
next prev parent reply other threads:[~2021-04-21 18:55 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-16 20:27 [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending Kuogee Hsieh
2021-04-20 22:01 ` Stephen Boyd
2021-04-21 17:26 ` khsieh
2021-04-21 18:55 ` aravindh [this message]
2021-04-28 0:00 ` Stephen Boyd
2021-04-28 17:38 ` khsieh
2021-04-29 9:26 ` Stephen Boyd
2021-04-29 17:23 ` khsieh
2021-04-30 3:11 ` Stephen Boyd
2021-05-03 19:23 ` khsieh
2021-05-04 4:28 ` Stephen Boyd
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ddc1e372c5f864cd62c4e056ef2e6404@codeaurora.org \
--to=aravindh@codeaurora.org \
--cc=abhinavk@codeaurora.org \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=khsieh@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robdclark@gmail.com \
--cc=sean@poorly.run \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).