linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] drm: drm_probe_helper: Fix output_poll_work scheduling
@ 2016-08-31 11:09 Peter Ujfalusi
  2016-08-31 11:23 ` Daniel Vetter
  0 siblings, 1 reply; 4+ messages in thread
From: Peter Ujfalusi @ 2016-08-31 11:09 UTC (permalink / raw)
  To: airlied, daniel.vetter
  Cc: dri-devel, linux-kernel, ville.syrjala, tomi.valkeinen

drm_kms_helper_poll_enable_locked() should check if we have delayed event
pending and if we have, schedule the work to run without delay.

Currently the output_poll_work is only scheduled if any of the connectors
have DRM_CONNECTOR_POLL_CONNECT or DRM_CONNECTOR_POLL_DISCONNECT with
DRM_OUTPUT_POLL_PERIOD delay. It does not matter if we have delayed event
already registered to be handled. The detection will be delayd by
DRM_OUTPUT_POLL_PERIOD in any case.
Furthermore if none of the connectors are marked as POLL_CONNECT or
POLL_DISCONNECT because all connectors are either POLL_HPD or they are
always connected: the output_poll_work will not run at all even if we
have delayed event marked.

When none of the connectors require polling, their initial status change
from unknown to connected/disconnected is not going to be handled until
the first kms application starts or if we have fb console enabled.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
Hi,

Changes since v1:
- dropped the last paragraph from the commit message.

Regards,
Peter

 drivers/gpu/drm/drm_probe_helper.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index a0df377d7d1c..f6b64d7d3528 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -129,6 +129,7 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
 {
 	bool poll = false;
 	struct drm_connector *connector;
+	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;

 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));

@@ -141,8 +142,13 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
 			poll = true;
 	}

+	if (dev->mode_config.delayed_event) {
+		poll = true;
+		delay = 0;
+	}
+
 	if (poll)
-		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
+		schedule_delayed_work(&dev->mode_config.output_poll_work, delay);
 }
 EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);

--
2.9.3

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] drm: drm_probe_helper: Fix output_poll_work scheduling
  2016-08-31 11:09 [PATCH v2] drm: drm_probe_helper: Fix output_poll_work scheduling Peter Ujfalusi
@ 2016-08-31 11:23 ` Daniel Vetter
  2016-12-19  9:54   ` Jani Nikula
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2016-08-31 11:23 UTC (permalink / raw)
  To: Peter Ujfalusi
  Cc: airlied, daniel.vetter, dri-devel, linux-kernel, ville.syrjala,
	tomi.valkeinen

On Wed, Aug 31, 2016 at 02:09:05PM +0300, Peter Ujfalusi wrote:
> drm_kms_helper_poll_enable_locked() should check if we have delayed event
> pending and if we have, schedule the work to run without delay.
> 
> Currently the output_poll_work is only scheduled if any of the connectors
> have DRM_CONNECTOR_POLL_CONNECT or DRM_CONNECTOR_POLL_DISCONNECT with
> DRM_OUTPUT_POLL_PERIOD delay. It does not matter if we have delayed event
> already registered to be handled. The detection will be delayd by
> DRM_OUTPUT_POLL_PERIOD in any case.
> Furthermore if none of the connectors are marked as POLL_CONNECT or
> POLL_DISCONNECT because all connectors are either POLL_HPD or they are
> always connected: the output_poll_work will not run at all even if we
> have delayed event marked.
> 
> When none of the connectors require polling, their initial status change
> from unknown to connected/disconnected is not going to be handled until
> the first kms application starts or if we have fb console enabled.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> Hi,
> 
> Changes since v1:
> - dropped the last paragraph from the commit message.

I added a few more words to the commit message to explain when exactly
this is a problem and applied your patch to drm-misc.

Thanks, Daniel
> 
> Regards,
> Peter
> 
>  drivers/gpu/drm/drm_probe_helper.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index a0df377d7d1c..f6b64d7d3528 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -129,6 +129,7 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
>  {
>  	bool poll = false;
>  	struct drm_connector *connector;
> +	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
> 
>  	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> 
> @@ -141,8 +142,13 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
>  			poll = true;
>  	}
> 
> +	if (dev->mode_config.delayed_event) {
> +		poll = true;
> +		delay = 0;
> +	}
> +
>  	if (poll)
> -		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
> +		schedule_delayed_work(&dev->mode_config.output_poll_work, delay);
>  }
>  EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
> 
> --
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] drm: drm_probe_helper: Fix output_poll_work scheduling
  2016-08-31 11:23 ` Daniel Vetter
@ 2016-12-19  9:54   ` Jani Nikula
  2016-12-19 12:15     ` Peter Ujfalusi
  0 siblings, 1 reply; 4+ messages in thread
From: Jani Nikula @ 2016-12-19  9:54 UTC (permalink / raw)
  To: Daniel Vetter, Peter Ujfalusi
  Cc: daniel.vetter, linux-kernel, dri-devel, tomi.valkeinen,
	Tahvanainen, Jari

On Wed, 31 Aug 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 31, 2016 at 02:09:05PM +0300, Peter Ujfalusi wrote:
>> drm_kms_helper_poll_enable_locked() should check if we have delayed event
>> pending and if we have, schedule the work to run without delay.
>> 
>> Currently the output_poll_work is only scheduled if any of the connectors
>> have DRM_CONNECTOR_POLL_CONNECT or DRM_CONNECTOR_POLL_DISCONNECT with
>> DRM_OUTPUT_POLL_PERIOD delay. It does not matter if we have delayed event
>> already registered to be handled. The detection will be delayd by
>> DRM_OUTPUT_POLL_PERIOD in any case.
>> Furthermore if none of the connectors are marked as POLL_CONNECT or
>> POLL_DISCONNECT because all connectors are either POLL_HPD or they are
>> always connected: the output_poll_work will not run at all even if we
>> have delayed event marked.
>> 
>> When none of the connectors require polling, their initial status change
>> from unknown to connected/disconnected is not going to be handled until
>> the first kms application starts or if we have fb console enabled.
>> 
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>> Hi,
>> 
>> Changes since v1:
>> - dropped the last paragraph from the commit message.
>
> I added a few more words to the commit message to explain when exactly
> this is a problem and applied your patch to drm-misc.

Hi Peter, sadly looks like this regresses users out there [1]. Seems to
be a reliable bisect. We need to have this fixed or reverted.

BR,
Jani.


[1] https://bugs.freedesktop.org/show_bug.cgi?id=98690



>
> Thanks, Daniel
>> 
>> Regards,
>> Peter
>> 
>>  drivers/gpu/drm/drm_probe_helper.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index a0df377d7d1c..f6b64d7d3528 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -129,6 +129,7 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
>>  {
>>  	bool poll = false;
>>  	struct drm_connector *connector;
>> +	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
>> 
>>  	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>> 
>> @@ -141,8 +142,13 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
>>  			poll = true;
>>  	}
>> 
>> +	if (dev->mode_config.delayed_event) {
>> +		poll = true;
>> +		delay = 0;
>> +	}
>> +
>>  	if (poll)
>> -		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
>> +		schedule_delayed_work(&dev->mode_config.output_poll_work, delay);
>>  }
>>  EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
>> 
>> --
>> 2.9.3
>> 

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] drm: drm_probe_helper: Fix output_poll_work scheduling
  2016-12-19  9:54   ` Jani Nikula
@ 2016-12-19 12:15     ` Peter Ujfalusi
  0 siblings, 0 replies; 4+ messages in thread
From: Peter Ujfalusi @ 2016-12-19 12:15 UTC (permalink / raw)
  To: Jani Nikula, Daniel Vetter
  Cc: daniel.vetter, linux-kernel, dri-devel, tomi.valkeinen,
	Tahvanainen, Jari

On 12/19/2016 11:54 AM, Jani Nikula wrote:
> On Wed, 31 Aug 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Aug 31, 2016 at 02:09:05PM +0300, Peter Ujfalusi wrote:
>>> drm_kms_helper_poll_enable_locked() should check if we have delayed event
>>> pending and if we have, schedule the work to run without delay.
>>>
>>> Currently the output_poll_work is only scheduled if any of the connectors
>>> have DRM_CONNECTOR_POLL_CONNECT or DRM_CONNECTOR_POLL_DISCONNECT with
>>> DRM_OUTPUT_POLL_PERIOD delay. It does not matter if we have delayed event
>>> already registered to be handled. The detection will be delayd by
>>> DRM_OUTPUT_POLL_PERIOD in any case.
>>> Furthermore if none of the connectors are marked as POLL_CONNECT or
>>> POLL_DISCONNECT because all connectors are either POLL_HPD or they are
>>> always connected: the output_poll_work will not run at all even if we
>>> have delayed event marked.
>>>
>>> When none of the connectors require polling, their initial status change
>>> from unknown to connected/disconnected is not going to be handled until
>>> the first kms application starts or if we have fb console enabled.
>>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> ---
>>> Hi,
>>>
>>> Changes since v1:
>>> - dropped the last paragraph from the commit message.
>>
>> I added a few more words to the commit message to explain when exactly
>> this is a problem and applied your patch to drm-misc.
> 
> Hi Peter, sadly looks like this regresses users out there [1]. Seems to
> be a reliable bisect. We need to have this fixed or reverted.

When I sent the patch I did booted my laptop (with Intel Corporation Haswell-ULT
Integrated Graphics Controller (rev 0b)) and my old desktop with some nVidia Quadro
using nouveau stack and seen no issue.

But with 4.9 now on my Dell laptop I see lots of warnings during boot:
[    0.573119] ------------[ cut here ]------------
[    0.573127] WARNING: CPU: 3 PID: 874 at drivers/gpu/drm/i915/intel_dp.c:1062 intel_dp_aux_transfer+0x1d5/0x210
[    0.573132] WARN_ON(!msg->buffer != !msg->size)
[    0.573134] Modules linked in:
[    0.573140] CPU: 3 PID: 874 Comm: kworker/u8:3 Not tainted 4.9.0-gentoo #1
[    0.573143] Hardware name: Dell Inc. Latitude E7440/06MFX3, BIOS A14 02/02/2015
[    0.573150] Workqueue: events_unbound async_run_entry_fn
[    0.573154]  ffffc900021afaf0 ffffffff813c3bad ffffc900021afb40 0000000000000000
[    0.573160]  ffffc900021afb30 ffffffff8106a596 0000042614b82908 ffffc900021afc00
[    0.573166]  ffff8802144040e0 0000000000000003 0000000000000000 ffff880214404158
[    0.573172] Call Trace:
[    0.573178]  [<ffffffff813c3bad>] dump_stack+0x4f/0x72
[    0.573182]  [<ffffffff8106a596>] __warn+0xc6/0xe0
[    0.573185]  [<ffffffff8106a5fa>] warn_slowpath_fmt+0x4a/0x50
[    0.573189]  [<ffffffff815926e9>] ? intel_dp_aux_transfer+0xc9/0x210
[    0.573193]  [<ffffffff815927f5>] intel_dp_aux_transfer+0x1d5/0x210
[    0.573198]  [<ffffffff817d3e53>] ? _raw_write_unlock_irqrestore+0x13/0x30
[    0.573202]  [<ffffffff817d3e79>] ? _raw_spin_unlock_irqrestore+0x9/0x10
[    0.573207]  [<ffffffff814b9e48>] drm_dp_dpcd_access+0x58/0xf0
[    0.573210]  [<ffffffff814b9ef6>] drm_dp_dpcd_write+0x16/0x20
[    0.573214]  [<ffffffff8158d8c3>] intel_dp_start_link_train+0x2b3/0x4a0
[    0.573218]  [<ffffffff8158ecd2>] intel_dp_check_link_status+0xb2/0xf0
[    0.573222]  [<ffffffff81593586>] intel_dp_detect+0x7d6/0xb40
[    0.573226]  [<ffffffff814bb1db>] drm_helper_probe_single_connector_modes+0x41b/0x4e0
[    0.573233]  [<ffffffff814c8d7c>] drm_fb_helper_initial_config+0x7c/0x3f0
[    0.573237]  [<ffffffff817d3e39>] ? _raw_spin_unlock_irq+0x9/0x10
[    0.573242]  [<ffffffff815858b3>] intel_fbdev_initial_config+0x13/0x30
[    0.573245]  [<ffffffff8108b832>] async_run_entry_fn+0x32/0xe0
[    0.573249]  [<ffffffff81083208>] process_one_work+0x148/0x4c0
[    0.573253]  [<ffffffff810835c3>] worker_thread+0x43/0x4e0
[    0.573257]  [<ffffffff81083580>] ? process_one_work+0x4c0/0x4c0
[    0.573260]  [<ffffffff81083580>] ? process_one_work+0x4c0/0x4c0
[    0.573264]  [<ffffffff8107f3c7>] ? call_usermodehelper_exec_async+0x137/0x140
[    0.573269]  [<ffffffff81088a45>] kthread+0xc5/0xe0
[    0.573273]  [<ffffffff81088980>] ? kthread_park+0x60/0x60
[    0.573277]  [<ffffffff8107f290>] ? umh_complete+0x40/0x40
[    0.573280]  [<ffffffff817d44f2>] ret_from_fork+0x22/0x30
[    0.573285] ---[ end trace a544f5d689389b41 ]---

If I revert the patch in question I have tons of:

[    0.569127] ------------[ cut here ]------------
[    0.569136] WARNING: CPU: 3 PID: 564 at drivers/gpu/drm/i915/intel_dp.c:1062 intel_dp_aux_transfer+0x1d5/0x210
[    0.569141] WARN_ON(!msg->buffer != !msg->size)
[    0.569143] Modules linked in:
[    0.569149] CPU: 3 PID: 564 Comm: kworker/3:2 Not tainted 4.9.0-gentoo #2
[    0.569152] Hardware name: Dell Inc. Latitude E7440/06MFX3, BIOS A14 02/02/2015
[    0.569159] Workqueue: events i915_hotplug_work_func
[    0.569164]  ffffc90001a93ba0 ffffffff813c3bad ffffc90001a93bf0 0000000000000000
[    0.569174]  ffffc90001a93be0 ffffffff8106a596 0000042614b12908 ffffc90001a93cb0
[    0.569184]  ffff88021482b0e0 0000000000000003 0000000000000000 ffff88021482b158
[    0.569191] Call Trace:
[    0.569198]  [<ffffffff813c3bad>] dump_stack+0x4f/0x72
[    0.569205]  [<ffffffff8106a596>] __warn+0xc6/0xe0
[    0.569210]  [<ffffffff8106a5fa>] warn_slowpath_fmt+0x4a/0x50
[    0.569214]  [<ffffffff815926c9>] ? intel_dp_aux_transfer+0xc9/0x210
[    0.569218]  [<ffffffff815927d5>] intel_dp_aux_transfer+0x1d5/0x210
[    0.569223]  [<ffffffff817d3e53>] ? _raw_write_unlock_irqrestore+0x13/0x30
[    0.569224] loop: module loaded
[    0.569233]  [<ffffffff817d3e79>] ? _raw_spin_unlock_irqrestore+0x9/0x10
[    0.569241]  [<ffffffff814b9e48>] drm_dp_dpcd_access+0x58/0xf0
[    0.569248]  [<ffffffff814b9ef6>] drm_dp_dpcd_write+0x16/0x20
[    0.569254]  [<ffffffff8158d8a3>] intel_dp_start_link_train+0x2b3/0x4a0
[    0.569261]  [<ffffffff8158ecb2>] intel_dp_check_link_status+0xb2/0xf0
[    0.569268]  [<ffffffff81593566>] intel_dp_detect+0x7d6/0xb40
[    0.569275]  [<ffffffff8157c739>] i915_hotplug_work_func+0x1d9/0x2a0
[    0.569283]  [<ffffffff81083208>] process_one_work+0x148/0x4c0
[    0.569290]  [<ffffffff810835c3>] worker_thread+0x43/0x4e0
[    0.569296]  [<ffffffff81083580>] ? process_one_work+0x4c0/0x4c0
[    0.569302]  [<ffffffff81083580>] ? process_one_work+0x4c0/0x4c0
[    0.569309]  [<ffffffff81088a45>] kthread+0xc5/0xe0
[    0.569316]  [<ffffffff81088980>] ? kthread_park+0x60/0x60
[    0.569322]  [<ffffffff817d44f2>] ret_from_fork+0x22/0x30
[    0.569330] ---[ end trace bc0abba135d2cb5d ]---

So the behaviour is changed, no question about it. But I still believe that
the patch itself fixes a valid bug (or shortcoming). atm I don't see
how it can lock the kernel.
The only thing the patch does is to schedule the output_poll_work w/o delay
if we already have delayed_event to reduce the time to handle pending
event(s) when the poll is enabled for the first time.
>From the log it is not clear for me why Xorg would lock up, but we have
intel and nouveau drivers probed, it might be because we have two GPUs?

> 
> BR,
> Jani.
> 
> 
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=98690
> 
> 
> 
>>
>> Thanks, Daniel
>>>
>>> Regards,
>>> Peter
>>>
>>>  drivers/gpu/drm/drm_probe_helper.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>>> index a0df377d7d1c..f6b64d7d3528 100644
>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>> @@ -129,6 +129,7 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
>>>  {
>>>  	bool poll = false;
>>>  	struct drm_connector *connector;
>>> +	unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
>>>
>>>  	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>>>
>>> @@ -141,8 +142,13 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
>>>  			poll = true;
>>>  	}
>>>
>>> +	if (dev->mode_config.delayed_event) {
>>> +		poll = true;
>>> +		delay = 0;
>>> +	}
>>> +
>>>  	if (poll)
>>> -		schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
>>> +		schedule_delayed_work(&dev->mode_config.output_poll_work, delay);
>>>  }
>>>  EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
>>>
>>> --
>>> 2.9.3
>>>
> 


-- 
Péter

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-12-19 12:16 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-31 11:09 [PATCH v2] drm: drm_probe_helper: Fix output_poll_work scheduling Peter Ujfalusi
2016-08-31 11:23 ` Daniel Vetter
2016-12-19  9:54   ` Jani Nikula
2016-12-19 12:15     ` Peter Ujfalusi

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).