Re: [RFC PATCH 00/11] Introduce writeback connectors
From: Brian Starkey
Date: Tue Oct 11 2016 - 17:26:17 EST
On Tue, Oct 11, 2016 at 10:02:43PM +0200, Daniel Vetter wrote:
On Tue, Oct 11, 2016 at 9:44 PM, Brian Starkey <brian.starkey@xxxxxxx> wrote:
Hi,
On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote:
On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkey <brian.starkey@xxxxxxx>
wrote:
Hi Daniel,
Firstly thanks very much for having a look.
On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote:
On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote:
Hi,
This RFC series introduces a new connector type:
DRM_MODE_CONNECTOR_WRITEBACK
It is a follow-on from a previous discussion: [1]
Writeback connectors are used to expose the memory writeback engines
found in some display controllers, which can write a CRTC's
composition result to a memory buffer.
This is useful e.g. for testing, screen-recording, screenshots,
wireless display, display cloning, memory-to-memory composition.
Patches 1-7 include the core framework changes required, and patches
8-11 implement a writeback connector for the Mali-DP writeback engine.
The Mali-DP patches depend on this other series: [2].
The connector is given the FB_ID property for the output framebuffer,
and two new read-only properties: PIXEL_FORMATS and
PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
formats of the engine.
The EDID property is not exposed for writeback connectors.
Writeback connector usage:
--------------------------
Due to connector routing changes being treated as "full modeset"
operations, any client which wishes to use a writeback connector
should include the connector in every modeset. The writeback will not
actually become active until a framebuffer is attached.
Erhm, this is just the default, drivers can override this. And we could
change the atomic helpers to not mark a modeset as a modeset if the
connector that changed is a writeback one.
Hmm, maybe. I don't think it's ideal - the driver would need to
re-implement drm_atomic_helper_check_modeset, which is quite a chunk
of code (along with exposing update_connector_routing, mode_fixup,
maybe others), and even after that it would have to lie and set
crtc_state->connectors_changed to false so that
drm_crtc_needs_modeset returns false to drm_atomic_check_only.
You only need to update the property in your encoders's ->atomic_check
function. No need for more, and I think being consistent with
computing when you need a modeset is really a crucial part of the
atomic ioctl that we should imo try to implement correctly as much as
possible.
Sorry I really don't follow. Which property? CRTC_ID?
Userspace changing CRTC_ID will change connector_state->crtc (before
we even get to a driver callback).
After that, drm_atomic_helper_check_modeset derives connectors_changed
based on the ->crtc pointers.
After that, my encoder ->atomic_check *could* clear
connectors_changed (or I could achieve the same thing by wrapping
drm_atomic_helper_check), but it seems wrong to do so, considering
that the connector routing *has* changed.
If you think changing CRTC_ID shouldn't require a full modeset, I'd
rather give drivers a ->needs_modeset callback to override the default
drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into
returning false.
The problem with just that is that there's lots of different things
that can feed into the overall needs_modeset variable. That's why we
split it up into multiple booleans.
So yes you're supposed to clear connectors_changed if there is some
change that you can handle without a full modeset. If you want, think
of connectors_changed as
needs_modeset_due_to_change_in_connnector_state, but that's cumbersome
to type and too long ;-)
All right, got it :-). This intention wasn't clear to me from the
comments in the code.
I can imagine some hardware will need a full modeset to changed the
writeback CRTC binding anyway.
Yup, and then they can upgrade this again. With all these flow-control
booleans the idea is very much that helpers give a default that works
for 90% of all cases, and driver callbacks can then change it for the
other 10%.
I tried to keep special-casing of writeback connectors in the
core to
a bare minimum, because I think it will quickly get messy and fragile
otherwise.
Please always make the disdinction between core and shared drm
helpers. Special cases in core == probably not good. Special cases in
helpers == perfectly fine imo.
Honestly, I don't see modesetting the writeback connectors at
start-of-day as a big problem.
It's inconsistent. Claiming it needs a modeset when it doesn't isn't
great. Making that more discoverable to userspace is the entire point
of atomic. And there might be hw where reconfiguring for writeback
might need a full modeset.
I'm a little confused - what bit exactly is inconsistent?
Not being truthful for when you need a modeset and when not.
My implementation here is consistent with other connectors.
Updating the writeback connector CRTC_ID property requires a full
modeset, the same as other connectors.
It's not about consistency with other implementations, it's about
consistency with what your hw can do. E.g. i915 clears
crtc_state->mode_changed when we can do a mode change without a full
modeset. The goal of atomic is to expose the full features of each hw
(including all quirks), not reduce it all to a least common set of
shared features.
Understood, I will make sure that we don't require a modeset unless
absolutely necessary.
Changing the FB_ID does *not* require a full modeset, because our
hardware has no such restriction. This is analogous to updating the
FB_ID on our planes, and is consistent with the other instances of the
FB_ID property.
Well that's inconsistent with connector properties, because in general
they all do require a full modeset to change ;-) I.e. consistency with
other drivers really isn't a good argument.
If there is hardware which does have a restriction on changing FB_ID, I
think that driver must be responsible for handling it in the same
way as drivers which can't handle plane updates without a full
modeset.
Are you saying that because setting CRTC_ID on Mali-DP is a no-op, it
shouldn't require a full modeset? I'd rather somehow hard-code the
CRTC_ID for our writeback connector to have it always attached to
the CRTC in that case.
Yup, I think if changing the CRTC_ID of the writeback connector
doesn't require a modeset, then your driver better not require a full
modeset to do that change. Maybe there's only one writeback port, and
userspace wants to move it around. And if the hw supports that without
a full modeset, then I think we should allow that. I also think that
most hw will get away with changing the writeback routing without
doing a full modeset. I might be mistaken about that though. And if
it's not clear-cut we could add a new writeback_changed boolean to
track this.
And from a user experience pov I really think we should avoid modesets
like the plague. Plugging in a chromecast stick and then watching how
your panel flickers is just not nice.
Yup, makes sense. I think my mindset is still a bit stuck in SETCRTC-
land.
The writeback itself is enabled by attaching a framebuffer to the
FB_ID property of the connector. The driver must then ensure that the
CRTC content of that atomic commit is written into the framebuffer.
The writeback works in a one-shot mode with each atomic commit. This
prevents the same content from being written multiple times.
In some cases (front-buffer rendering) there might be a desire for
continuous operation - I think a property could be added later for
this kind of control.
Writeback can be disabled by setting FB_ID to zero.
This seems to contradict itself: If it's one-shot, there's no need to
disable it - it will auto-disable.
I should have explained one-shot more clearly. What I mean is, one
drmModeAtomicCommit == one write to memory. This is as opposed to
writing the same thing to memory every vsync until it is stopped
(which our HW is capable of doing).
A subsequent drmModeAtomicCommit which doesn't touch the writeback FB_ID
will write (again - but with whatever scene updates) to the same
framebuffer.
This continues for every drmModeAtomicCommit until FB_ID is set to
zero - to disable writing - or changed to a different framebuffer, in
which case we write to the new one.
IMO this behaviour makes sense in the context of the rest of Atomic,
and as the FB_ID is indeed persistent across atomic commits, I think
it should be read-able.
tbh I don't like that, I think it'd be better to make this truly
one-shot. Otherwise we'll have real fun problems with hw where the
writeback can take longer than a vblank (it happens ...). So one-shot,
with auto-clearing to NULL/0 is imo the right approach.
That's an interesting point about hardware which won't finish within
one frame; but I don't see how "true one-shot" helps.
What's the expected behaviour if userspace makes a new atomic commit
with a writeback framebuffer whilst a previous writeback is ongoing?
In both cases, you either need to block or fail the commit - whether
the framebuffer gets removed when it's done is immaterial.
See Eric's question. We need to define that, and I think the simplest
approach is a completion fence/sync_file. It's destaged now in 4.9, we
can use them. I think the simplest uabi would be a pointer property
(u64) where we write the fd of the fence we'll signal when write-out
completes.
That tells userspace that the previous writeback is finished, I agree
that's needed. It doesn't define any behaviour in case userspace asks
for another writeback before that fence fires though.
In other cases where we write a property as a one-shot thing (fences for
android). In that case when you read that property it's always 0 (well,
-1
for fences since file descriptor). That also avoids the issues when
userspace unconditionally saves/restores all properties (this is needed
for generic compositor switching).
I think a better behaviour would be to do the same trick, with FB_ID on
the connector always returning 0 as the current value. That encodes the
one-shot behaviour directly.
I had more of a think about this. I think you're right that
one-shot-write-only makes sense for the framebuffer - at least I can't
think of a decent use case needing the persistent behaviour which
couldn't easily be achieved using the one-shot style.
Thanks!
-Brian
For one-shot vs continuous: Maybe we want to simply have a separate
writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and
FB_WRITEBACK_CONTINUOUS_ID.
Known issues:
-------------
* I'm not sure what "DPMS" should mean for writeback connectors.
It could be used to disable writeback (even when a framebuffer is
attached), or it could be hidden entirely (which would break the
legacy DPMS call for writeback connectors).
dpms is legacy, in atomic land the only thing you have is "ACTIVE" on
the
crtc. it disables everything, i.e. also writeback.
So removing the DPMS property is a viable option for writeback connectors
in
your opinion?
Nah, that's part of the abi now. But atomic internally remaps it to
"ACTIVE", in short you don't need to care (as long as you fill out the
dpms hook with the provided helper. drm_writeback_connector_init
should probably do that).
A connector can still be DPMS-ed individually, so a CRTC can be
"ACTIVE", attached to an "OFF" writeback connector, and the writeback
connector would still be able to actively write to memory.
Yes, but atomic drivers ignore that. You should too. I won't take
patches which create special behaviour for dpms on the writeback
connector. If you want to change the writeback separately, then we can
change the CRTC_ID of the writeback connector. And the driver should
report correctly whether that needs a modeset or not.
I'm OK with that, and it's what I already implemented, but I thought
that userspace might reasonably expect a writeback connector with DPMS
set to "OFF" to be completely inert.
Nope, DPMS turned out to be a mistake in kms (no one supports the
intermediate stages, they don't make sense) and we nerfed it in
atomic. Please don't resurrect zombies ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch