[PATCH] drm: drm_probe_helper: Fix output_poll_work scheduling

[PATCH] drm: drm_probe_helper: Fix output_poll_work scheduling

[Peter Ujfalusi]

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.

With this change we can react more quickly to output status changes after
enabling the poll but most importantly it will correct the behavior when
none of the connector have POLL_CONNECT or POLL_DISCONNECT flag set - they
are either POLL_HPD or the .polled is 0. The initial status change is going
to be handled correctly as well.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 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

Re: [PATCH] drm: drm_probe_helper: Fix output_poll_work scheduling

[Daniel Vetter]

On Mon, Aug 29, 2016 at 12:50:22PM +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.
> 
> With this change we can react more quickly to output status changes after
> enabling the poll but most importantly it will correct the behavior when
> none of the connector have POLL_CONNECT or POLL_DISCONNECT flag set - they
> are either POLL_HPD or the .polled is 0. The initial status change is going
> to be handled correctly as well.

Don't we need to set delayed_event somewhere too first? At least I don't
really understand how this speeds things up ... The patch itself looks
like a valid bugfix, but the description confuses me a bit. I think if you
delete the above paragraph (starting with "With this change we can react
...") then it's all good.
-Daniel

> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  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

Re: [PATCH] drm: drm_probe_helper: Fix output_poll_work scheduling

[Peter Ujfalusi]

On 08/29/16 15:58, Daniel Vetter wrote:
> On Mon, Aug 29, 2016 at 12:50:22PM +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.
>>
>> With this change we can react more quickly to output status changes after
>> enabling the poll but most importantly it will correct the behavior when
>> none of the connector have POLL_CONNECT or POLL_DISCONNECT flag set - they
>> are either POLL_HPD or the .polled is 0. The initial status change is going
>> to be handled correctly as well.
> 
> Don't we need to set delayed_event somewhere too first? At least I don't
> really understand how this speeds things up

The delayed_event is set first in the
drm_helper_probe_single_connector_modes_merge_bits() when the force is not set.
The drm_kms_helper_poll_init() is called by drivers when they have their
connectors set up. So when probing the drivers we will first set up the
connectors, they will move from connector_status_unknown to connected or
disconnected and the delayed_event is set.
Later the drm_kms_helper_poll_init() is called and that will schedule the work
with DRM_OUTPUT_POLL_PERIOD. So even if we now know the state of the
connectors, they will be checked by the poll_work when the work is running
with a delay. If we set the delay to 0 the detection will run much faster.

Is this makes more sense?

> ... The patch itself looks
> like a valid bugfix, but the description confuses me a bit. I think if you
> delete the above paragraph (starting with "With this change we can react
> ...") then it's all good.

I can drop the last paragraph.

> -Daniel
> 
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  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