Re: [PATCH 1/6] drm: Add page_flip_target CRTC hook

On Thu, Aug 04, 2016 at 12:47:38PM +0200, Daniel Vetter wrote:

On Thu, Aug 04, 2016 at 12:39:36PM +0900, Michel Dänzer wrote:

...

From: Michel Dänzer michel.daenzer@amd.com

Mostly the same as the existing page_flip hook, but takes an additional parameter specifying the target vertical blank period when the flip should take effect.

Signed-off-by: Michel Dänzer michel.daenzer@amd.com

drivers/gpu/drm/drm_crtc.c | 23 +++++++++++++++++++---- include/drm/drm_crtc.h | 14 ++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9d3f80e..15ad7e2 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5421,6 +5421,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, struct drm_crtc *crtc; struct drm_framebuffer *fb = NULL; struct drm_pending_vblank_event *e = NULL;

• u32 target_vblank = 0; int ret = -EINVAL;

  if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||

@@ -5434,6 +5435,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, if (!crtc) return -ENOENT;

• if (crtc->funcs->page_flip_target) {

• int r;
  

• r = drm_crtc_vblank_get(crtc);
  

This leaks when e.g framebuffer_lookup fails. -Daniel

...

• if (r)
  

• 	return r;
  

• target_vblank = drm_crtc_vblank_count(crtc) +
  

• 	!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
  

• } else if (crtc->funcs->page_flip == NULL)

• return -EINVAL;
  

• drm_modeset_lock_crtc(crtc, crtc->primary); if (crtc->primary->fb == NULL) { /* The framebuffer is currently unbound, presumably

@@ -5444,9 +5457,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, goto out; }

• if (crtc->funcs->page_flip == NULL)

• goto out;
  

• fb = drm_framebuffer_lookup(dev, page_flip->fb_id); if (!fb) { ret = -ENOENT;

@@ -5487,7 +5497,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, }

crtc->primary->old_fb = crtc->primary->fb;

• ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);

• if (crtc->funcs->page_flip_target)

• ret = crtc->funcs->page_flip_target(crtc, fb, e,
  

• 				    page_flip->flags,
  

• 				    target_vblank);
  

• else

• ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
  

  if (ret) { if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) drm_event_cancel_free(dev, &e->base);

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 9e6ab4a..ae1d9b6 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -579,6 +579,20 @@ struct drm_crtc_funcs { uint32_t flags);

/**

• * @page_flip_target:
  

• *
  

• * Same as @page_flip but with an additional parameter specifying the
  

• * target vertical blank period when the flip should take effect.
  

*absolute target vertical blank period as reported by drm_crtc_vblank_count()

would imo be a good addition here.

...

• *
  

• * Note that the core code calls drm_crtc_vblank_get before this entry
  

• * point.
  

I think you should add "Drivers must drop that reference again by calling drm_crtc_vblank_put()."

Also, who should drop the reference in case ->page_flip_target fails?

It's a bit icky that we have this difference (both compared to old page flip, but also atomic doesn't grab a vblank reference either). But really can't be avoided, at least if we implement the relative mode.

With all issues addressed:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch -Daniel

-Daniel

...

• */
  

• int (*page_flip_target)(struct drm_crtc *crtc,

• 		struct drm_framebuffer *fb,
  

• 		struct drm_pending_vblank_event *event,
  

• 		uint32_t flags, uint32_t target);
  

• /**
  • @set_property:
  • This is the legacy entry point to update a property attached to the

-- 2.8.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

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