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);
WARNING: multiple messages have this Message-ID (diff)
From: aravindh@codeaurora.org To: khsieh@codeaurora.org Cc: freedreno@lists.freedesktop.org, airlied@linux.ie, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, abhinavk@codeaurora.org, Stephen Boyd <swboyd@chromium.org>, dri-devel@lists.freedesktop.org, sean@poorly.run 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); _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-04-21 18:55 UTC|newest] Thread overview: 22+ 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-16 20:27 ` Kuogee Hsieh 2021-04-20 22:01 ` Stephen Boyd 2021-04-20 22:01 ` Stephen Boyd 2021-04-21 17:26 ` khsieh 2021-04-21 17:26 ` khsieh 2021-04-21 18:55 ` aravindh [this message] 2021-04-21 18:55 ` aravindh 2021-04-28 0:00 ` Stephen Boyd 2021-04-28 0:00 ` Stephen Boyd 2021-04-28 17:38 ` khsieh 2021-04-28 17:38 ` khsieh 2021-04-29 9:26 ` Stephen Boyd 2021-04-29 9:26 ` Stephen Boyd 2021-04-29 17:23 ` khsieh 2021-04-29 17:23 ` khsieh 2021-04-30 3:11 ` Stephen Boyd 2021-04-30 3:11 ` Stephen Boyd 2021-05-03 19:23 ` khsieh 2021-05-03 19:23 ` khsieh 2021-05-04 4:28 ` Stephen Boyd 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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.