Re: [RFC PATCH] drm: Add plane event

[Marcus Lorentzon <marcus.xm.lorentzon@stericsson.com>]

On 04/18/2012 04:26 PM, Ville Syrjälä wrote:
> On Wed, Apr 18, 2012 at 04:04:56PM +0200, Marcus Lorentzon wrote:
>> On 04/18/2012 02:25 PM, Rob Clark wrote:
>>> On Wed, Apr 18, 2012 at 5:11 AM, Joonyoung Shim<jy0922.shim@samsung.com>   wrote:
>>>> On 04/18/2012 05:46 PM, Daniel Vetter wrote:
>>>>> On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote:
>>>>>> DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is
>>>>>> for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to
>>>>>> provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl
>>>>>> can change the framebuffer of plane but user can't know completion of
>>>>>> changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is
>>>>>> added, we can also do pageflip of a plane.
>>>>>>
>>>>>> Signed-off-by: Joonyoung Shim<jy0922.shim@samsung.com>
>>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>>> If I understand the current kms api correctly, set_plane is akin to
>>>>> set_base and should not generate an asynchronous flip completion event.
>>>>> To
>>>>> do that we need a new pageflip ioctl which changes a complete set of fb +
>>>>> planes + any crtc attributes that might be in an atomic fashion. At which
>>>>> point we can just reuse the existing page flip event mechanism.
>>>> It seems better way to add new pageflip ioctl for plane. I will try it.
>>> fwiw, an atomic mode set which can update crtc and zero or more plane
>>> layers is, I think, the way to go.  Jesse Barnes had an RFC for this,
>>> although IIRC it was only the API and not the implementation.  And
>>> combination w/ the plane/crtc properties patchset to allow setting
>>> properties as part of the update might not be a bad thing either.
>>>
>>> BR,
>>> -R
>> Before planes and rotation properties modeset was atomic (single ioctl).
>> Would it not be possible to define atomic modeset for planes and
>> properties like this?
>>
>> SETPROPERTY and SETPLANE (maybe more) should not be commited when set,
>> only on SETCRTC ioctl like before planes. All properties and plane
>> changes before SETCRTC should be considered staged settings for the next
>> SETCRTC.
>> If this would break API backwards compatibility, maybe a new "standard"
>> boolean property called EnableAtomicMode could be used to trigger this
>> mode in the drivers. If a driver doesn't support it, things will just
>> work as currently with modeset not being atomic across planes.
>> Maybe this could be implemented in the generic parts of KMS, but it
>> could be tried out first inside a driver.
> Multi ioctl solution can have some issues since you can't hold any
> locks around the whole operation. Also there could be issues if the
> process performing the operation dies or hangs in mid operation.
> Error handling can also be difficult since the intermediate steps
> may violate some constraints in the system.
>
> Also SETCRTC itself is a per-CRTC operation, but we actually want
> per-device atomic operations instead.
>
> So I think a single ioctl solution is a better idea. I'm currently
> pondering on what the API would look like. Ideally I would like to
> go with the "everything is a property" approach, and I'd like to get
> rid of some of the other limitations in the current API at the same
> time.
>
Yes, from previous emails I have seen that we are quite aligned on the 
single-atomic-modeset-with-properties version.

Do you have any actual proposal for this? Like the API at least and some 
comments on "the other limitations" you fix with it?
I only recall seeing Jesse's API proposal, but not yours, only some 
ideas about a generic list of properties/values for modeset when I 
proposed something similar.

I'm about to implement atomic modeset now one way or the other, so the 
more proposals I have to choose from the better ;)
I found that the per-crtc modeset above covers my requirements, so I 
might just take the easy route for now. But I welcome any work/proposal 
on generic support for atomic modeset.

/BR
/Marcus