Hello, Ville!
On 02/06/2018 06:04 PM, Ville Syrjälä wrote:
On Tue, Feb 06, 2018 at 11:59:37AM +0200, Oleksandr Andrushchenko wrote:
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?
You will get the plane explicitly as part of the drm_atomic_state.
Normally you shouldn't need to go find it via other means.
Oh, and never use the plane->fb etc. pointers in an atomic driver.
Those are for legacy purposes only. Atomic drivers should only ever
deal with proper state objects.
I am using fb from corresponding state now, thank you
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
.atomic_commit() is a hook the driver has to provide. Most drivers use
the helper for it, which in turn requires the driver to provide other
hooks such as .atomic_flush(). That is what you appear to be doing.
you are correct
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
Oh right. That way of passing the old state only dates back to earlier
days of atomic. To find the new state what you should do these days
is something like:
foo(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
{
struct drm_atomic_state *state = old_state->state;
struct drm_crtc_state *new_state =
drm_atomic_get_new_crtc_state(state, crtc);
...
The old way was to use the crtc->state pointer as either the old
or new state depending on where you are in the commit sequence.
But that wasn't very good so Maarten changed things so that we
now have explicit old and new states for each object tracked in
the drm_atomic_state.
I think what we should really do is change most of the hooks to
pass crtc+drm_atomic_state instead, and let each function grab
the old and/or new crtc state from the drm_atomic_state as needed.
I believe that would avoid confusing people with just the old or
new state.
That would be great
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
I see. Well, without evidence to the contrary I'd probably just assume
it'll never fail.
I am not sure I can guarantee this
That avoids complicating the code with potentially
useless logic.
Unfortunately, I'll have to put something in
Hammering it with something like igt for a while might
serve as a good way to test that assumption. Not sure how many tests in
igt currently run on non-i915 though.
The first test suite I tried was IGT indeed, but unfortunately
it is coupled with i915 too much, so only very basic tests
could be run. Then I found [1] which can be easily extended
without much efforts, so I stick to fork of it (hope to contribute
there later, for example, by adding ION tests)
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.
There must be something running or you never get anything on the screen.
Yes, sorry for not being precise: I meant that nothing
driver specific, e.g. which knows internals of the driver.
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.
Usually one would handle page flips from the plane's .atomic_update()
hook instead of the crtc's .atomic_flush(). But if you only have the one
plane then this could work. But you should get at the plane fb via
drm_atomic_get_new_plane_state() etc.
Generally looks like you need some more work on the plane .atomic_check()
function. Using drm_atomic_helper_check_plane_state() should get you
most of the way there I'd imagine. Not sure if disabling the plane
independently of the crtc makes any sense, but if not you should
look at drm_simple_kms_helper.c for an example. Hmm. Not sure if you
couldn't just use drm_simple_kms_helper.c yourself actually.
Great! As I am moving away from legacy stuff this is a
perfect fit now. So, I will base on drm_simple_kms_helper.
BTW, it has a small issue which can be easily solved in the
DRM core I believe: if your driver needs to work with vblanks,
then you have to provide drm_driver.enable_vblank/disable_vblank
because drm_simple_kms_helper.drm_crtc_funcs do not provide any.
The problem here is that drm_driver.enable_vblank/disable_vblank
are most probably dummy functions and what is more they are marked
as deprecated.
