Re: Questions on page flips and atomic modeset

[Oleksandr Andrushchenko <andr2000@gmail.com>]

Hello, Ville!

Thank you very much for such a comprehensive answer.

Please see inline


On 02/05/2018 06:47 PM, Ville Syrjälä wrote:
> On Mon, Feb 05, 2018 at 06:03:25PM +0200, Oleksandr Andrushchenko wrote:
>> Hello,
>>
>>
>> I have a DRM driver which implements display protocol for Xen [1]
>> and this protocol has a dedicated XENDISPL_OP_PG_FLIP request which
>> tells the other end that some display buffer needs to be displayed,
>> e.g. it is issued by the frontend DRM driver to the corresponding
>> backend when the former wants to display a buffer.
>> Two notes:
>> 1. Communication between two remote parties can obviously fail.
>> 2. At the moment only primary plane is supported, so we can think of
>> the display buffer as of CRTC's primary fb.
>>
>> There are two APIs available for user-space to initiate a page-flip
>> (please correct me if I am wrong here): legacy via DRM_IOCTL_MODE_PAGE_FLIP
>> and more recent via DRM_IOCTL_MODE_ATOMIC. My current implementation
>> (which is 4.9 based) uses drm_crtc_funcs.page_flip callback to send
>> XENDISPL_OP_PG_FLIP request to the backend, so if communication fails
>> I can return an error code, so the DRM core knows that page flip
>> cannot be done.
>>
>> But now I am about to enable page flipping via DRM_IOCTL_MODE_ATOMIC for
>> which this happens without DRM_IOCTL_MODE_PAGE_FLIP, but via .atomic_check +
>> .atomic_flush callbacks.
>>
>> The solution I have in mind is that I move the XENDISPL_OP_PG_FLIP request
>> to the .atomic_flush callback which seems to be the right place to serve
>> both DRM_IOCTL_MODE_PAGE_FLIP and DRM_IOCTL_MODE_ATOMIC (is this?).
>>
>> The questions I have with this are:
>>
>> 1. What is the framebuffer I can send to the backend?
>> I assume I can use crtc->primary->fb from
>> drm_crtc_helper_funcs.atomic_flush,
>> is this assumption correct?
> Planes are explicit in the atomic world, so you should just deal with
> the plane if and when it's part of the drm_atomic_state. Look at eg.
> drm_atomic_helper_commit_planes() how to figure out what's in the
> state.
Yes, this is clear. The question was about the frame buffer
I get in drm_crtc_helper_funcs.atomic_flush which only has
old_crtc_state, so I assumed I can use crtc->primary->fb there.
Or you mean I have to dig into old_crtc_state->state to find
out if there is a plane and if it has a frambuffer and if so
use it instead of crtc->primary->fb?
>   And I guess you're already planning on using that helper since
> you mention .atomic_flush().
Not directly, but via drm_mode_config_funcs.atomic_commit
>
> One should really think of the drm_atomic_state as more of a transaction
> rather than as a state (since not all objects need be part of it).
Thank you
>> 2. Is the check (either to send or not) for crtc->primary->fb != NULL
>> enough?
>> I assume there are other cases when .atomic_flush gets called,
>> so is there a way I can filter out unneeded cases? E.g. those which
>> are not for page flipping?
> Each object taking part in the transaction will have an associated
> new state and old state.
As I replied above I only have old state in .atomic_flush
>   You can compare the two to figure out what
> changed, and in addition there may already some flags in the base
> class for the state that indicate what may have changed (eg.
> drm_crtc_state::mode_changed etc.). Such flags may be set by the
> core to help figure out what needs doing.
Yes, thank you
>> 3. If communication with the backend fails there is no way for me
>> to tell the DRM core about this as .atomic_flush is called after
>> "the point of no return". Do you think I can remember the error
>> code and report it next time .atomic_check is called? So, other page
>> flips will not run on broken connection, for example.
> Do you know when it might fail?
Not really, this is a software protocol to talk from
the frontend para-virtual DRM driver to its backend
in other Xen domain
>   If so you should implement the appropriate
> checks in .atomic_check() etc. to try and make sure it never happens
> (barring a total hardware failure for example).
>
> Generally what we (i915) do is try to check everything up front, but if
> an unexpected error does happen later we just muddle through and log an
> error.
>
> For us I think the most common late failure is DP link training failure.
> That we can't fully check up front since it depends on external factors:
> sink, dongles, cabling, etc. For those we added a new connector property
> to signal to userspace that the link is having issues, allowing
> userspace to reconfigure things such that we lower the link bandwidth
> requirements.
I cannot do that this way, because the driver has to run
seamlessly for user-space, no specific utilities  are
expected to run.
>
> The link_status mechanism could perhaps be used to to work around other
> "late errors". But in general if you want to somehow fix that error you
> have to know what caused it, no?
That is correct, so I will print an error message on
page flip error so user has at list a clue on what's
wrong
>   So if you just get a random error for
> seemingly no reason there's very little you can do apart from blindly
> retrying and logging an error. For the DP case the fallback mechanism is
> pretty clear: reduce link rate and/or number of lanes.
>
> As for signalling the error in the next ioctl call, that could be done
> I suppose.
I tried that approach and it seems to work.
>   But again the question is how would userspace know what
> (if anything) it can do to fix the problem?
Well, this would be seen as just an error to user-space
and unfortunately if it is not prepared to deal with then it will
close. Not sure I can do anything smart about it
>
>> 4. As per my understanding I cannot put XENDISPL_OP_PG_FLIP request
>> into .atomic_check as the check doesn't guarantee that .atomic_flush
>> will follow. Is this correct or there is a more neat solution exists?
> Thou shalt not touch the hardware until .atomic_commit(). Note that
> .atomic_commit() itself is still allowed to fail, but only if you
> can fail atomically.
>
> The .atomic_flush() & co. helper stuff is designed in a way that
> expects no failures at that late stage of the commit. If that
> doesn't suit your hardware design then you may opt to not use the
> helper.
>
> Also note that non-blocking operations can make this even more difficult
> because in that case even .atomic_commit() doesn't generally touch
> the hardware and instead that task is delegated to eg. a work queue.
> So by the time the hardware indicates an error the ioctl has long since
> returned to userspace.
>
This is my case as I send a page flip request to the backend
and later in time receive the corresponding response.

Do you mind looking at my current WIP implementation of
atomic commit [1], [2]? This work is done as a preparation for
upstreaming the driver and is still in progress though.

Thank you very much,
Oleksandr

[1] 
https://github.com/andr2000/linux/blob/drm_pre_v0_drm_next/drivers/gpu/drm/xen/xen_drm_front_crtc.c#L320
[2] 
https://github.com/andr2000/linux/blob/drm_pre_v0_drm_next/drivers/gpu/drm/xen/xen_drm_front_crtc.c#L335
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel