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 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?
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
