Hi,
On 23 March 2015 at 08:20, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Thu, Mar 19, 2015 at 04:32:36AM +0000, Daniel Stone wrote:
>> This series ends up touching pretty much all the drivers, by virtue of turning
>> crtc->mode (in particular) into both a const and a pointer.
>
> Ok this is quite a bit a different beast than what I expected. I think
> it's way too intrusive for drivers to land quickly, and there's a big
> depency chain linking everything. I think we need something much simpler.
Yeah, I'm uneasy with how invasive it's got: hence the proposal to
drop crtc->mode back into being inlined. I don't think we lose
anything from doing that (as crtc->state->mode still changes), and
that makes the diff much less scary. The constness changes no longer
become required, but I think are still pretty useful from the point of
view of setting (and enforcing) our existing expectations. I'd still
look to push those anyway, but at a much more sedate pace.
>> - as far as possible, modes should be relateable to their source, e.g. if
>> Plymouth pulls a particular mode from the encoder, and you pick up on that
>> mode as part of current configuration during handover, you should be able
>> to work backwards to where Plymouth sourced it, i.e. the encoder list
>
> With legacy setcrtc we already lose this information and thus far no one
> seems to have cared. And I don't see the use-case since simply comparing
> it to sources works well enough, in case you want to know where a mode is
> from.
On the other hand, I wouldn't suggest SetCrtc to really be a model to
follow. I really don't mind SetCrtc breaking all these expectations
above, mind.
>> - userspace should be able to tell the current status by looking at the IDs
>> returned by property queries, rather than having to pull the entire mode
>> out: if we make them do that, they won't bother minimising the deltas and
>> will just dump the full state in every time, and that makes debugging the
>> entire thing that much harder
>
> That doesn't work since idr eagerly reuses ids. Just by looking at the id
> you can't tell whether it's the same object or a new one accidentally
> reusing the same id slot. You always have to re-read the blob property too
> to reconstruct state.
Do we make any guarantee on connector->modes lifetimes? If it can't
change without a hotplug event, then we know the ID is valid until the
connector goes dark, at which point we have to drop any local cache
relating to it.
Saying that userspace must read every single mode property back every
single time is pretty horrible from the point of view of requiring
more userspace->kernel->userspace->kernel->[...] trips to do discovery
every time, just because we decided not to work out a sensible
lifetime strategy to expose to userspace.
> This blew up with edid blob properties where SNA had one clever trick too
> many and thought that matching edid blob prop id means the edid is
> unchanged. But since we remove the old blob before we add the new one
> you're pretty much guranteed to reuse the same slot.
Sure, if you're not paying attention to the defined lifetime, then
don't do any caching. But this is about _defining_ such a usable
lifetime that userspace can.
>> - setting a mode current should hold a reference for as long as it's current,
>> e.g. if you create a magic user-supplied mode, set that on the CRTC and
>> then terminate the connection, the mode should still live on for handover
>> purposes
>
> I think we need much less: If your driver supports atomic (and hence
> userspace might be asking for the mode blob prop id) then that blob should
> survive as long as the mode is in use.
Which not only requires the most invasive of the changes anyway
(modulo those to crtc->mode, which can regardless be dropped), but
also a bunch of 'every time the state changes, go consult some
auxiliary property and potentially expire its lifetime'.
>> - persistence of system-supplied (from-connector or from-CRTC) modes is not
>> important beyond their natural lifetime: if you pick a mode up from a
>> connector's probed list and then that connector disappears, setting that
>> mode is unlikely to be what you want, so failure because the mode no
>> longer exists is entirely acceptable
>
> Somewhat unordered, but here's what I think we need:
> - Subtyping blob properties is not needed, at least I can't think of a
> use-case. It will result though in lots of duplicated code for
> duplicated ref/unref functions and atomic prop handling.
When you say 'subtyping', do you mean what I've done with getblob?
> - Since we already have a getblob ioctl I think we should just extend the
> existing drm_property_blob:
That was actually my initial implementation, but didn't like the
resulting reference tie-up between drm_display_mode and
drm_property_blob (aka drm_mode_modeinfo). I couldn't figure out a
really clean way to do it that didn't provably fall down at some
point, so abandoned it and went for referencing drm_display_mode
instead.
> Plus ofc changing drm_property_create/destroy_blob into ref/unref
> functions. And doing the same weak reference trick for idrs as we're
> using for framebuffers now.
>
> For mode properties the data contained would be struct drm_mode_modeinfo
> (i.e. the ABI struct we already use for the setcrtc/getconnector ioctls,
> not the internal one).
Right, this is what's there: 25/37 is explicitly always returning
modeinfo, and the creation does as well. I don't see any benefit to
exposing the internal struct.
> - Imo requiring all the legacy users to be converted to pointers isn't
> needed. crtc->mode/hwmode are deprecated for atomic drivers. Not that I
> don't think doing this isn't useful, I just think it's not needed to
> have a minimal atomic mode blob support.
Yeah, I agree. I'm moving all this out to be tracked in a separate
series. A lot of it was to convince myself that in the process of the
mode work I was doing, I wouldn't be breaking drivers.
> Aside: We don't even need to convert mode structs to pointers if we
> embedded the kref into the mode - I've done similar horrible conversion
> tricks with framebuffers, where drivers tend to embed the static fbdev
> framebuffer into the fbdev emulation struct. You just need raw
> free/destroy entry points which for safety check that the kref count is
> exactly 1.
>
> - For the actual atomic integration I think we need a blob prop pointer
> separate from the mode struct. The blob prop contains the userspace ABI
> struct, and the embedded one is the one used internally. Doing any kind
> of conversion just leads to lots of churn, which is especially bad with
> around 4 still incomplete/unmerged atomic conversion. So just
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index adc9ea5acf02..d6a7a5247b64 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -296,6 +296,7 @@ struct drm_crtc_state {
> struct drm_display_mode adjusted_mode;
>
> struct drm_display_mode mode;
> + struct drm_property_blob * mode_blob;
>
> struct drm_pending_vblank_event *event;
>
> Getting the refcounting right for the atomic ioctl should be simple. For
> the legacy ->set_config entrypoint we need to fix things up in the
> helper when we update the mode, by manually releasing the old mode blob
> and creating a new one (owned by the kernel to avoid leaks because old
> userspace won't clean them up - we've had this bug with compat cursor
> fbs just recently).
To be honest I think the fb refcnt bug is pretty apropos here - by
keeping these awkwardly split out, independently reference-counted,
not sensibly linked to each other, and not even verifying the
correctness of the users (const), you're inviting another disaster
along the same lines. Especially as drivers start to move away from
the helpers and are expected to manage more state themselves.
I agree the initial series was too invasive and there's value in
having a much more minimal series, so how about I split out the
crtc->mode constness (but not pointer - I don't think I have any
desire to ever push that through tbqh) changes into another series we
can track separately, and then see if we can get a much more minimal
patchset that doesn't make me shudder and think of previous
refcnt/state-consistency horrors, nor you of atomic conversions past,
together. I'm happy to pull in the other guys doing atomic conversions
and get some testing out of them to see if it'll work for them and how
well, but having it run on Tegra was about as painless as I could've
hoped for.
Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
