[PATCH] drm/simple-helpers: Always add planes to the state update

[Noralf Trønnes]

Den 23.08.2016 08:25, skrev Daniel Vetter:
> Our update function is hooked to the single plane, which might not get
> called for crtc-only updates. Which is surprising, so fix this by
> always adding the plane.
>
> While at it document how&when the event should be sent out better in
> the kerneldoc.
>
> Cc: Noralf Trønnes <noralf at tronnes.org>
> Cc: andrea.merello at gmail.com
> Tested-and-Reported-by: andrea.merello at gmail.com
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
> I'm not entirely sure we really want to put the responsibility for
> this onto driver. Plan B) would be to remove the kerneldoc I added
> here and call the right function from drm_simple_kms_plane_atomic_update.
> That way simple drivers don't need to deal with that detail, and in
> general those drivers don't care that much about the miniscule
> possible race a generic implementation would cause. What do you
> suggest as the best approach?

If the driver is responsible, a helper like this would be nice:

void drm_simple_display_pipe_handle_vblank_event(struct 
drm_simple_display_pipe *pipe)
{
     struct drm_crtc *crtc = &pipe->crtc;

     if (crtc->state && crtc->state->event) {
         spin_lock_irq(&crtc->dev->event_lock);
         drm_crtc_send_vblank_event(crtc, crtc->state->event);
         spin_unlock_irq(&crtc->dev->event_lock);
         crtc->state->event = NULL;
     }
}

Then if the update() callback is not set, drm_simple_kms_plane_atomic_update
calls this, and if it is set, then the driver has to do it or something
similar.

It is difficult to predict what future drivers will need.
Unless there are drivers on the horizon that will need to handle the
event themselves, I suggest that we follow your plan B. If the need arises,
we just add a one-liner to all drivers that has the update() callback set.

Noralf.

Sidenote:
When I converted simpledrm I had problems with flip_done timeouts.
In the end I had to check for the event in disable/enable/update.
Trying it again now, it's enough to do it in update().
Not sure what has changed.

> -Daniel
> ---
>   drivers/gpu/drm/drm_simple_kms_helper.c | 7 +++++++
>   include/drm/drm_simple_kms_helper.h     | 6 ++++++
>   2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index bada17166512..447631018426 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -34,6 +34,12 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
>   	.destroy = drm_encoder_cleanup,
>   };
>   
> +static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
> +				     struct drm_crtc_state *state)
> +{
> +	return drm_atomic_add_affected_planes(state->state, crtc);
> +}
> +
>   static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
>   {
>   	struct drm_simple_display_pipe *pipe;
> @@ -57,6 +63,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
>   }
>   
>   static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> +	.atomic_check = drm_simple_kms_crtc_check,
>   	.disable = drm_simple_kms_crtc_disable,
>   	.enable = drm_simple_kms_crtc_enable,
>   };
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index 269039722f91..826946ca2b82 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -60,6 +60,12 @@ struct drm_simple_display_pipe_funcs {
>   	 *
>   	 * This function is called when the underlying plane state is updated.
>   	 * This hook is optional.
> +	 *
> +	 * This is the function drivers should submit the
> +	 * &drm_pending_vblank_event from. Using either
> +	 * drm_crtc_arm_vblank_event(), when the driver supports vblank
> +	 * interrupt handling, or drm_crtc_send_vblank_event() directly in case
> +	 * the hardware lacks vblank support entirely.
>   	 */
>   	void (*update)(struct drm_simple_display_pipe *pipe,
>   		       struct drm_plane_state *plane_state);