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@xxxxxx>
>> ---
>>  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