drm/kms-helper: disable hpd_irq handling when poll=0
diff mbox

Message ID 1365264303-27813-1-git-send-email-daniel.vetter@ffwll.ch
State New, archived
Headers show

Commit Message

Daniel Vetter April 6, 2013, 4:05 p.m. UTC
When inlining the actual hpd output probing in

commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Oct 23 18:23:34 2012 +0000

    drm: run the hpd irq event code directly

the check for the drm_kms_hlper.poll module option was lost. This
regressed systems where this option is used to work-around output
probing issues (like irq storms). Restore the old behaviour.

Reported-by: George Amanakis <g_amanakis@yahoo.com>
Cc: George Amanakis <g_amanakis@yahoo.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: stable@vger.kernel.org (for 3.8)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alex Deucher April 8, 2013, 12:28 p.m. UTC | #1
On Sat, Apr 6, 2013 at 12:05 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> When inlining the actual hpd output probing in
>
> commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Tue Oct 23 18:23:34 2012 +0000
>
>     drm: run the hpd irq event code directly
>
> the check for the drm_kms_hlper.poll module option was lost. This
> regressed systems where this option is used to work-around output
> probing issues (like irq storms). Restore the old behaviour.

NAK.  It's been a while since I looked at it, but the whole point of
this patch set was to be able to disabling polling independently of
hpd.  If you add this check back, setting poll=0 disables both polling
and hpd.  I'd suggest we add a separate hpd option to disable hpd.

Alex

>
> Reported-by: George Amanakis <g_amanakis@yahoo.com>
> Cc: George Amanakis <g_amanakis@yahoo.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: stable@vger.kernel.org (for 3.8)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_crtc_helper.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 7b2d378..3260736 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -1067,7 +1067,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev)
>         enum drm_connector_status old_status;
>         bool changed = false;
>
> -       if (!dev->mode_config.poll_enabled)
> +       if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
>                 return;
>
>         mutex_lock(&dev->mode_config.mutex);
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter April 8, 2013, 12:43 p.m. UTC | #2
On Mon, Apr 8, 2013 at 2:28 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Sat, Apr 6, 2013 at 12:05 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> When inlining the actual hpd output probing in
>>
>> commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
>> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Date:   Tue Oct 23 18:23:34 2012 +0000
>>
>>     drm: run the hpd irq event code directly
>>
>> the check for the drm_kms_hlper.poll module option was lost. This
>> regressed systems where this option is used to work-around output
>> probing issues (like irq storms). Restore the old behaviour.
>
> NAK.  It's been a while since I looked at it, but the whole point of
> this patch set was to be able to disabling polling independently of
> hpd.  If you add this check back, setting poll=0 disables both polling
> and hpd.  I'd suggest we add a separate hpd option to disable hpd.

The point for me was that the _driver_ can separate hpd handling from
polling. Which it still can with this patch applied.

The issue at hand is that the old poll=0 also managed to paper over
some hpd handling irq storms and so breaking that is a regression. To
fix that we should imo apply this patch.

We can revert this again once i915 has the hpd irq storm rate-limiting
stuff applied (presuming there's no other bug report for other
drivers). Also in 3.9 we have the reworked kms locking, so the usual
reason that polling causes cursor/pageflip stalls to set poll=0
evaporated. So imo the only people who should still set poll=0 on 3.9
are those with irq storms. And we've just broken that part ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Alex Deucher April 8, 2013, 3 p.m. UTC | #3
On Mon, Apr 8, 2013 at 8:43 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Mon, Apr 8, 2013 at 2:28 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Sat, Apr 6, 2013 at 12:05 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> When inlining the actual hpd output probing in
>>>
>>> commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
>>> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Date:   Tue Oct 23 18:23:34 2012 +0000
>>>
>>>     drm: run the hpd irq event code directly
>>>
>>> the check for the drm_kms_hlper.poll module option was lost. This
>>> regressed systems where this option is used to work-around output
>>> probing issues (like irq storms). Restore the old behaviour.
>>
>> NAK.  It's been a while since I looked at it, but the whole point of
>> this patch set was to be able to disabling polling independently of
>> hpd.  If you add this check back, setting poll=0 disables both polling
>> and hpd.  I'd suggest we add a separate hpd option to disable hpd.
>
> The point for me was that the _driver_ can separate hpd handling from
> polling. Which it still can with this patch applied.
>
> The issue at hand is that the old poll=0 also managed to paper over
> some hpd handling irq storms and so breaking that is a regression. To
> fix that we should imo apply this patch.
>
> We can revert this again once i915 has the hpd irq storm rate-limiting
> stuff applied (presuming there's no other bug report for other
> drivers). Also in 3.9 we have the reworked kms locking, so the usual
> reason that polling causes cursor/pageflip stalls to set poll=0
> evaporated. So imo the only people who should still set poll=0 on 3.9
> are those with irq storms. And we've just broken that part ...

There are also a lot of people that have non-HPD capable ports
(usually old analog stuff like VGA) that they aren't using and they
don't want to waste the CPU cycles polling them, but they still want
hpd.  I expect we'll get regression reports about the poll option
getting broken again (hpd stops working with polling disabled, or
monitor detection no longer works) if this patch is applied.

Alex

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 7b2d378..3260736 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -1067,7 +1067,7 @@  void drm_helper_hpd_irq_event(struct drm_device *dev)
 	enum drm_connector_status old_status;
 	bool changed = false;
 
-	if (!dev->mode_config.poll_enabled)
+	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
 		return;
 
 	mutex_lock(&dev->mode_config.mutex);