| Message ID | 1470281981-18172-2-git-send-email-michel@daenzer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + * > + * 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()." 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. -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
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
On Wed, Aug 3, 2016 at 11:39 PM, Michel Dänzer <michel@daenzer.net> 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); > + 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; > + According to kernel coding style, this last block should have parens because the previous block did. With that and Daniel's comments addressed: Reviewed-by: Alex Deucher <alexander.deucher@amd.com> Alex > 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. > + * > + * Note that the core code calls drm_crtc_vblank_get before this entry > + * point. > + */ > + 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
On 05.08.2016 00:13, Alex Deucher wrote: > On Wed, Aug 3, 2016 at 11:39 PM, Michel Dänzer <michel@daenzer.net> wrote: >> >> @@ -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); >> + 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; >> + > > According to kernel coding style, this last block should have parens > because the previous block did. Right, I can never remember which project wants this which way. :) > With that and Daniel's comments addressed: > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> Thanks! I'll send out a v2 patch.
On 04.08.2016 20:01, Daniel Vetter wrote: > 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: >>> >>> @@ -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. Good catch. >>> 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()." Thanks for the suggestions. > Also, who should drop the reference in case ->page_flip_target fails? The core DRM code. > With all issues addressed: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks! I'll send out a v2 patch with your and Alex's feedback addressed. Will it be okay to merge this via Alex's tree?
On Mon, Aug 08, 2016 at 12:54:45PM +0900, Michel Dänzer wrote: > On 04.08.2016 20:01, Daniel Vetter wrote: > > 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: > >>> > >>> @@ -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. > > Good catch. > > > >>> 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()." > > Thanks for the suggestions. > > > > Also, who should drop the reference in case ->page_flip_target fails? > > The core DRM code. Please add that to the kerneldoc, too, when the code is fixed. > > With all issues addressed: > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Thanks! I'll send out a v2 patch with your and Alex's feedback > addressed. Will it be okay to merge this via Alex's tree? Sure, ack on merging through amdgpu. Would be good to then send an earlier pull request with it to avoid conflicts when it's too long outside of drm-next. -Daniel
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); + 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. + * + * Note that the core code calls drm_crtc_vblank_get before this entry + * point. + */ + 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