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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Mar 19, 2015 at 04:32:36AM +0000, Daniel Stone wrote:
> Well, that escalated quickly.
> 
> I've been looking at adding modesetting support to the atomic ioctl, and this
> is what I've ended up with so far. It's definitely not perfect, but given how
> out of hand it's got at the moment, I wanted to send this out as an RFC before
> I spent too long polishing it up.
> 
> 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.

> The reasoning behind this is that currently, we just treat modes as unboxed
> data to shovel around. With atomic modesetting, and user-supplied modes, we
> want to do better here. Whilst sketching out userspace/compositor
> requirements, I came up with the following invariants, which necessitated
> turning modes into a refcounted, immutable, type:
>   - modes can come from one of three sources: connector list, current
>     mode, userspace-created
>   - 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.

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

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.

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

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

- Since we already have a getblob ioctl I think we should just extend the
  existing drm_property_blob:

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index adc9ea5acf02..9845b634d2d3 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -217,6 +217,7 @@ struct drm_framebuffer {
 
 struct drm_property_blob {
 	struct drm_mode_object base;
+	struct kref refcount;
 	struct list_head head;
 	size_t length;
 	unsigned char data[];

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

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

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

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