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

Daniel Vetter daniel at ffwll.ch
Mon Nov 25 09:02:38 UTC 2019


On Fri, Nov 22, 2019 at 08:35:13PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at 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 at 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().
> 
> No, I'm thinking of drm_atomic_plane_check(). That one already does the
> "does the src rect fit into the fb" check. It gets called from
> drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
> drivers might take some short circuit path past that for page_flip().

tbh if you're worried about this, I'm tempted to rip out the option for
atomic drivers to overwrite their page_flip implementation, and force the
default function onto everyone (drm_atomic_helper_page_flip_target and
friends would need to be moved to drm_plane.c for that ofc).

We seem to have 0 need for hacks for old userspace in this area, which is
great. So the option to overwrite could only lead to trouble.

> > That aside I'm kinda not getting the point of the shuffling/cleanup in the
> > first 4 patches here, so will skip them.
> 
> The point was to get that legacy plane->fb/old_fb stuff totally
> out from the atomic codepath. Right now it's polluting the
> supposedly higher level code shared by both atomic and legacy
> drivers.

Hm ... I think if you sell it like that, and maybe even go one further and
inline page_flip for all atomic drivers, I can be sold on this. As-is I
just didn't really get why you're reworking all this.
-Daniel

> 
> > -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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the Intel-gfx mailing list