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

[Michel Dänzer]

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