Message-ID: <CAKMK7uEQsiBLQGghdDvmPicc_F6+3Ra_sd7keSKTPAgsNKbdog@mail.gmail.com>
Date:   Tue, 11 Oct 2016 22:02:43 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Brian Starkey <brian.starkey@....com>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        Liviu Dudau <liviu.dudau@....com>,
        "Clark, Rob" <robdclark@...il.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Eric Anholt <eric@...olt.net>,
        "Syrjala, Ville" <ville.syrjala@...ux.intel.com>
Subject: Re: [RFC PATCH 00/11] Introduce writeback connectors

On Tue, Oct 11, 2016 at 9:44 PM, Brian Starkey <brian.starkey@....com> 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@....com>
>> 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 ;-)

> 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.

> 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.

>>>>> 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.

>>>> 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.
>>>>
>>>> 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

Powered by blists - more mailing lists