Re: [RFC PATCH 00/37] Modesetting for atomic modesetting

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 23, 2015 at 04:58:47PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 23 March 2015 at 08:20, Daniel Vetter <daniel@ffwll.ch> 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.

Well setcrtc is bound to stay around, and userspace using it will also
stick around for quite a bit I think. If we add new semantic guarantees
and then userspace can't rely on them we bear all the cost without much
benefit.

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

Current kernels should send you a hotplug event, so with those that
strategy works - you can cache until the next hotplug. But on older
kernels (and iirc we've only fixed this very recently) sometimes hotplug
events get lost. So there if someone asks for a full reprobe, you have to
do the full dance.

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

Hm let's backtrack a bit: What's the upside of this defined lifetime? Atm
I see a lot of cost on the kernel side (getting this series in will take a
few releases probably because there's so many interlocked subsystem-wide
changes). But I don't see a benefit.

The only place we currently read out the current configuration is when
the compositor starts up so that it can perfectly take over whatever's
been set up by the firmware/boot splash for fastboot. Afterwards everyone
just bashes in their own config, even when switching between compositors
and all that.

Without a clear use-case that justifies the work I don't think we should
do this - someone will come up with clever abuse for it and then we have
another hard-to-work-around regression at hand. So from my side the only
requirement is that atomic clients should be able to somehow get at the
current blob properties like mode, gamma ...

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

Well yeah, but for the mode property the only place it changes (besides
the atomic ioctl) is in drm_atomic_helper_set_config. We have a lot more
compat glue already than the few lines you'd need to add in there. The
crucial bit is that we only need to make this work for atomic drivers,
since non-atomic drivers won't ever expose the MODE_ID property. And
non-atomic userspace doesn't care anyway.

Also the important part here isn't so much the mode (userspace can get at
that), but other new blob properties like per-plane gamma or other color
corrections tables.

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

Not just but yes. The other result is that we'd need to duplicate the
property decoding code for each type in the atomic ioctl/set_property
functions. And you need per-type reference/unreference functions.

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

Why refcount drm_display_mode? That's the drm-internal representation, we
can just keep that as-is everywhere. I think that's the main point of why
you opted for this much more invasive refactoring, and I don't see why
we'd need it. The added kref for drm_blob_property is imo really all the
krefs I think we need.

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

Hm, maybe I need to explain my motiviation for the blob prop stuff: I want
to use it for gamma tables (not just the per-crtc one we have, but
per-plane tables too) and other bigger properties. Using it for the mode
by packing the existing drm_mode_modeinfo into a blob too was just a bit
an afterthought - for the mode we could as well just add properties for
all the individual members, like we've done with all the other structures.

But loading gamma tables with gamma-0 up to gamma-1024 is way too
unwielding, so that's why we imo really need blobs.

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

Which fb refcount bug? We've had piles of them in the past, simply because
refcounting is apparently hard. And I don't really see a problem with
refcounted objects with atomic, normal driver code shouldn't ever care.
The only place we need to be careful is in
- duplicate/destroy_state functions: Can be fixed by extracting suitable
  helpers instead of copypasting.
- atomic_set_property: For all the core properties the decoding is done in
  the core anyway.

And for the oddball driver-private blob property we can provide
convienence functions to update pointers which will do the correct
unref/ref dance.

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

Hm, how does a const crtc->mode as a non-pointer work? We do need to
update this one in set_config. Or would you do a forced cast in these
cases?

But even for that I don't see why we'd need to change crtc->mode really to
make the atomic mode a blob property.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel