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

[Daniel Vetter <daniel@ffwll.ch>]

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

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.

> + */
> +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.

> +			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

> +
> +/**
>   * drm_vblank_enable - enable the vblank interrupt on a CRTC
>   * @dev: DRM device
>   * @pipe: CRTC index
> diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
> index 2401b14..7bd9690 100644
> --- a/include/drm/drm_irq.h
> +++ b/include/drm/drm_irq.h
> @@ -144,6 +144,7 @@ extern void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  				       struct drm_pending_vblank_event *e);
>  extern void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  				      struct drm_pending_vblank_event *e);
> +extern void drm_crtc_arm_completion_event(struct drm_crtc *crtc);
>  extern bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>  extern bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
>  extern int drm_crtc_vblank_get(struct drm_crtc *crtc);
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel