[RFC 0/9] nuclear pageflip

[Rob Clark]

On Sat, Sep 15, 2012 at 9:53 AM, Ville Syrjälä <syrjala at sci.fi> wrote:
> On Fri, Sep 14, 2012 at 05:46:35PM -0400, Kristian Høgsberg wrote:
>> I think (hope) the consensus coming out of this thread is something
>> along these lines:
>>
>>  - We use properties for specifying what to change to be future
>> compatible with new crtc features, but also to allow exposing
>> hw-specific properties and tie them into the atomicity of the
>> pageflip.  The KMS overlays are a lowest-common denominator for all
>> the various overlay types out there and it should be possible to write
>> a piece of chipset specific compositor code to use features that can't
>> be expressed through KMS overlays.
>
> Properties are good. Check.
>
>>  - We have two types of properties: dynamic and non-dynamic ones.
>> Dynamic properties can always be changed in the next frame (fb bos, hw
>> cursor position, overlay position, for example), non-dynamic
>> properties typically involve changing the way bandwidth are allocated
>> and changing them may fail.
>
> There's just no way to make such a general split. The simple fact is
> that even moving an overlay can fail due to timing/bandwith related
> constraints.

fwiw, the driver should indicate this by setting a flag on the
property, this way userspace knows what can be changed dynamically and
what can not.

>>  - We need a test ioctl that can verify whether changing non-dynamic
>> properties will work.  Using the atomic modeset for that with a
>> test-only flag seems like a good option since that already has the
>> logic to analyze bandwidth allocation across all crtcs.  On the other
>> hand, it may make more sense to use the multiflip ioctl as well here.
>> What we need to check is whether the change made by a multifflip is
>> possible, so it seems natural to communicate that change to the kernel
>> using the same ioctl and data structs as the multiflip itself.  The
>> bandwidth calculation is a global decision and involves all crtcs and
>> the current state, so the kernel can decide just fine if a multiflip
>> is possible or not, based on the current state and the requested
>> multiflip.
>
> Ie. multiflip and atomic modeset need exactly the same thing here.
>
>>  - Atomic multiflip for one crtc is essential for avoiding flicker and
>> artifacts, but ill-defined for multiple crtcs simultaneously and even
>> in the genlock case, the failure mode is hardly noticable (one crtc
>> may drop a frame in case the compositor is racing with vsync, in which
>> case multiflip just means both crtcs drop a frame).
>
> Sorry I don't follow. With two ioctls in the genlocked case, one crtc
> could drop, and the other might not. That is going to be a problem if
> both crtcs handle parts of the same physical display. Apart from the
> possible IVI and phone/tablet/gadget uses, I can imagine this being
> useful for large advertisement/presentation/simulation displays too.
>
> Also allowing multi crtc flips in the non-genlocked case makes cloned
> displays trivial to implement. This is especially useful if the system
> is push based like surfaceflinger.
>
>> For flipping
>> multiple fbs and planes, on one crtc, however, atomicity means that we
>> can combine gpu rendering and overlays in a reliable way, without
>> having to worry about flicker when sprites turn on a frame later after
>> we've already erased the surface contents from the main fb.  We need
>> to be able to render the scene graph split across various planes at
>> certain positions and know for certain that when we flip, that's the
>> configuration that ends up on the output.
>
> Sure, that's the main goal of this work.
>
>>  - Pageflip events can be controlled by a flag (as for the current
>> pageflip ioctl) or perhaps disabled by setting user_data to 0, but the
>> user data is passed in with each nuclear pageflip ioctl and each ioctl
>> generates one event (if requested) which returns the user data that
>> was passed in at ioctl time.  This is how it currently works, the
>> event mechanism is already in place, I see no reason to change this
>> behaviour.  Surely, we're not concerned about 8 extra bytes in the
>> ioctl struct?  The atomic modeset event (in test mode or not) never
>> generates an event, so there's no need for user data there.
>
> There's no reason why you couldn't send the event in the blocking
> modeset case too. Also it would open the door for asynchronous modeset,
> if someone has the cojones to implement it.
>
>>  - Pageflip for multiple crtc may be useful in case of gen-locked
>> crtc, but it is a corner case and not likely to be present or relevant
>> in mainstream hw.
>
> I've already provided many ideas where it could be used, and I don't
> even consider myself a very imaginative person.
>
> I don't see the point of forcing everyone with such a setup to add
> hacks in order to work around artificial restrictions imposed by the
> API. Do we want to make a system that people *want* to use, or one they
> *have* to use.
>
>> With the properties being an extensible mechanism,
>> we could probably expose gen-locked crtcs through the properties or
>> such and in worst case make a new ioctl as Jesses suggests.
>
> Well, I just don't see the point of going about it in such a
> roundabout way.
>
> My current prototype code basically handles this case already, except
> that I added an artifical restriction to avoid the async apply code
> path in the multi-crtc case since some people were suggesting that.
> With just a few lines of code change I will lift that restriction
> and it'll work just fine.
>
> I honestly do not see why some people want this restriction. From
> where I'm standing it doesn't make the code any less complex. What
> is the benefit you're trying to extract from the restriction?
>
> And even if someone thinks that they can't implement the multi crtc
> case, then they're free to return an error from the check ioctl. So
> there's no harm in allowing it.
>
>
> So I propose that we have:
> - One ioctl that takes an arbitrary number of obj/prop/value tuples

well, to be fair, if we convert everything to properties, maybe
drm/kms only needs one ioctl for everything :-P

But different ioctls are cheap..  I don't think it hurts to have two
instead of one.  I really don't see modeset and pageflip as the same
thing.  Maybe from the front-end of the video pipe, they are.  But
from encoder and back they are different.  Modeset can take many
vblank cycles to complete.  And is infrequent.  Introducing a
state-machine to try to make this asynchronous is just adding a lot of
complexity and potential fail for not really much gain.

Even flip on multiple crtcs introduces some new edge cases, like
moving a plane from one crtc to another.  If this is split into two
ioctl calls, then the test on the 2nd crtc can -EBUSY because the
plane is still pending disconnect from the first.  But the test on the
1st crtc can succeed.  I can see the usefulness of flip on
multi-crtc... but since it isn't nearly as useful/important as
flipping multiple planes on a single crtc, I don't see the harm in
starting simple and adding this later.

BR,
-R

> - A flag to specify "test only" mode
> - A flag to demand asynchronous operation
> - Flags to request completion events for each crtc. A bitmask of crtc
>   index will do. Each event will contain the relevant crtc ID.
> - user_data is passed to the ioctl, and included in the events
>   originating from that operation.
>
> From my POV the only significant API issues left are:
> - Truly useful error reporting. Perhaps there is no nice way to do it.
> - Returning a list of retired FBs in the events
>
> --
> Ville Syrjälä
> syrjala at sci.fi
> http://www.sci.fi/~syrjala/
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel