Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

From: Daniel Vetter <daniel@ffwll.ch>
To: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()
Date: Tue, 19 Nov 2019 11:14:43 +0100	[thread overview]
Message-ID: <20191119101443.GA30416@phenom.ffwll.local> (raw)
In-Reply-To: <20191115194204.22244-4-ville.syrjala@linux.intel.com>

On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Yank out the code for the plane->fb/old_fb/crtc handling from
> the page flip path into page_flip_internal(), and provide a
> simpler variant for atomic drivers.
> 
> We'll also move the fb vs. src viewport checks into the new
> functions as they are slightly different between the two paths.
> If the atomic .page_flip() implementations are guaranteed
> to call drm_atomic_plane_check() we could even drop the check
> entirely from page_flip_atomic(). For now toss in a FIXME.
> 
> v2: Bit more polish in page_flip_internal()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
>  1 file changed, 102 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 38878da5b704..6052475a20a5 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
>  	return drm_mode_cursor_common(dev, req, file_priv);
>  }
>  
> +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> +			       const struct drm_framebuffer *old_fb)
> +{
> +	/* The framebuffer is currently unbound, presumably
> +	 * due to a hotplug event, that userspace has not
> +	 * yet discovered.
> +	 */
> +	if (!old_fb)
> +		return -EBUSY;
> +
> +	if (old_fb->format != fb->format) {
> +		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int page_flip_internal(struct drm_crtc *crtc,
> +			      struct drm_framebuffer *fb,
> +			      struct drm_pending_vblank_event *e,
> +			      u32 flags,
> +			      u32 target_vblank,
> +			      struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	int ret;
> +
> +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> +
> +	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> +				      &crtc->mode, fb);
> +	if (ret)
> +		return ret;
> +
> +	ret = page_flip_check_fbs(fb, plane->fb);
> +	if (ret)
> +		return ret;
> +
> +	plane->old_fb = plane->fb;
> +	if (crtc->funcs->page_flip_target)
> +		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> +						    target_vblank, ctx);
> +	else
> +		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> +	if (ret) {
> +		plane->old_fb = NULL;
> +		return ret;
> +	}
> +
> +	plane->fb = fb;
> +	drm_framebuffer_get(plane->fb);
> +
> +	drm_framebuffer_put(plane->old_fb);
> +	plane->old_fb = NULL;
> +
> +	return 0;
> +}
> +
> +static int page_flip_atomic(struct drm_crtc *crtc,
> +			    struct drm_framebuffer *fb,
> +			    struct drm_pending_vblank_event *e,
> +			    u32 flags,
> +			    u32 target_vblank,
> +			    struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = plane->state;
> +	int ret;
> +
> +	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> +
> +	/*
> +	 * FIXME: Can we assume all drivers end up calling
> +	 * drm_atomic_plane_check() in their page flip paths?
> +	 * If so we could remove this.
> +	 */

This is definitely wrong, core has to check these. Currently no driver
checks this at all. I think you're thinking of
drm_atomic_helper_check_plane_state().

That aside I'm kinda not getting the point of the shuffling/cleanup in the
first 4 patches here, so will skip them.
-Daniel

> +	ret = drm_framebuffer_check_src_coords(plane_state->src_x,
> +					       plane_state->src_y,
> +					       plane_state->src_w,
> +					       plane_state->src_h,
> +					       fb);
> +	if (ret)
> +		return ret;
> +
> +	ret = page_flip_check_fbs(fb, plane_state->fb);
> +	if (ret)
> +		return ret;
> +
> +	if (crtc->funcs->page_flip_target)
> +		return crtc->funcs->page_flip_target(crtc, fb, e, flags,
> +						     target_vblank, ctx);
> +	else
> +		return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> +}
> +
>  int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  			     void *data, struct drm_file *file_priv)
>  {
>  	struct drm_mode_crtc_page_flip_target *page_flip = data;
>  	struct drm_crtc *crtc;
>  	struct drm_plane *plane;
> -	struct drm_framebuffer *fb, *old_fb;
> +	struct drm_framebuffer *fb;
>  	struct drm_pending_vblank_event *e = NULL;
>  	u32 target_vblank = page_flip->sequence;
>  	struct drm_modeset_acquire_ctx ctx;
> @@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (plane->state)
> -		old_fb = plane->state->fb;
> +	if (drm_drv_uses_atomic_modeset(dev))
> +		ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
> +				       target_vblank, &ctx);
>  	else
> -		old_fb = plane->fb;
> -
> -	if (old_fb == NULL) {
> -		/* The framebuffer is currently unbound, presumably
> -		 * due to a hotplug event, that userspace has not
> -		 * yet discovered.
> -		 */
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -
> -	if (plane->state) {
> -		const struct drm_plane_state *state = plane->state;
> -
> -		ret = drm_framebuffer_check_src_coords(state->src_x,
> -						       state->src_y,
> -						       state->src_w,
> -						       state->src_h,
> -						       fb);
> -	} else {
> -		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> -					      &crtc->mode, fb);
> -	}
> -	if (ret)
> -		goto out;
> -
> -	if (old_fb->format != fb->format) {
> -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	plane->old_fb = plane->fb;
> -	if (crtc->funcs->page_flip_target)
> -		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> -						    page_flip->flags,
> -						    target_vblank,
> -						    &ctx);
> -	else
> -		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
> -					     &ctx);
> -	if (ret) {
> -		/* Keep the old fb, don't unref it. */
> -		plane->old_fb = NULL;
> -	} else {
> -		if (!plane->state) {
> -			plane->fb = fb;
> -			drm_framebuffer_get(fb);
> -		}
> -	}
> +		ret = page_flip_internal(crtc, fb, e, page_flip->flags,
> +					 target_vblank, &ctx);
>  
>  out:
> -	if (plane->old_fb)
> -		drm_framebuffer_put(plane->old_fb);
> -	plane->old_fb = NULL;
> -
>  	if (ret == -EDEADLK) {
>  		ret = drm_modeset_backoff(&ctx);
>  		if (!ret)
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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