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 at 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
