[RFC PATCH 1/6] drm: add helper for arming crtc completion event

[Andrzej Hajda]

Hi Daniel,


On 29.09.2016 11:44, Daniel Vetter wrote:
> On Tue, Sep 27, 2016 at 03:36:14PM +0200, Andrzej Hajda wrote:
>> A lot of drivers need to fire pageflip completion event at very next vblank
>> interrupt. The patch adds helper to perform this operation.
>> drm_crtc_arm_completion_event checks if there is an event to handle,
>> if vblank reference get succeeds it arms the event, otherwise it sends the
>> event immediately.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda at samsung.com>
>> ---
>>  drivers/gpu/drm/drm_irq.c | 25 +++++++++++++++++++++++++
>>  include/drm/drm_irq.h     |  1 +
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 77f357b..313d323 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -1053,6 +1053,31 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>>  EXPORT_SYMBOL(drm_crtc_send_vblank_event);
>>  
>>  /**
>> + * drm_crtc_arm_completion_event - conditionally arm crtc completion event
>> + * @crtc: the source CRTC of the completion event
>> + *
>> + * A lot of drivers need to fire pageflip completion event at very next vblank
>> + * interrupt. This helper tries to arm the event in case of successful vblank
>> + * get otherwise it sends the event immediately.
> This function is copypasted tons of times over all drivers because they
> were broken, and this hack at least got the events working. But in general
> this here is racy, and if Mario ever runs on your hw he'll get upset about
> the wrong timings.

Could you explain what is racy here? I am not vblank expert, but the code
for me looked more or less OK.

>
> If we go with this (and I'm really not convinced it's a good idea) then it
> should have real big warning that you should never use this in a new
> driver.

It looked to me as an easy copy/paste cleaning :)
Anyway I would like to have correct solution, at least in case of exynos.

>
>> + */
>> +void drm_crtc_arm_completion_event(struct drm_crtc *crtc)
>> +{
>> +	struct drm_pending_vblank_event *event = crtc->state->event;
>> +
>> +	if (event) {
>> +		crtc->state->event = NULL;
>> +
>> +		spin_lock_irq(&crtc->dev->event_lock);
>> +		if (drm_crtc_vblank_get(crtc) == 0)
> This check here completely torpedoes the design principle of atomic
> helpers that drivers always know the state of the crtc. Again I only did
> this because I didn't want to buy hw&test it for all the drivers which got
> this wrong.

If you mean driver should know that getting vblank should be possible
or not this code could be probably parametrized, for example:

drm_crtc_handle_completion_event(struct drm_crtc *crtc, bool send_immediately)


>> +			drm_crtc_arm_vblank_event(crtc, event);
>> +		else
>> +			drm_crtc_send_vblank_event(crtc, event);
>> +		spin_unlock_irq(&crtc->dev->event_lock);
>> +	}
>> +}
>> +EXPORT_SYMBOL(drm_crtc_arm_completion_event);
> Maybe we need an EXPORT_SYMBOL_BROKEN for this .... btw the kerneldoc of
> the functions you're called do explain that this is not as easy as it
> seems. That's also something you throw under the rug here.
> -Daniel

After quick look I have not found these kerneldocs, where can read about it.

Regards
Andrzej