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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
