[RFC] drm: atomic mode set API

[RFC] drm: atomic mode set API

[Jesse Barnes]

Many of us really want (and need) a way to set the whole display
configuration atomically, as well as test a global config.

In talking with Rob and Alex here at ELC a bit, I think this may be
enough:

diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index 2a2acda..2864b02 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -157,6 +157,26 @@ struct drm_mode_get_plane_res {
 	__u32 count_planes;
 };
 
+#define DRM_SET_CONFIG_TEST (1<<0) /* don't change the config, just test it for validity */
+
+struct drm_mode_set_config {
+	__u64 crtcs;
+	__u64 crtc_fbs;
+	__u64 crtc_xpos; /* array of x coords for crtcs */
+	__u64 crtc_ypos; /* array of y coords for crtcs */
+	__u32 count_crtcs;
+
+	__u64 plane_sets; /* array of set_plane structs */
+
+	__u32 count_planes;
+
+	__u64 connectors;
+	__u64 connector_modes;
+	__u32 count_connectors;
+
+	__u32 flags;
+};
+
 #define DRM_MODE_ENCODER_NONE	0
 #define DRM_MODE_ENCODER_DAC	1
 #define DRM_MODE_ENCODER_TMDS	2

This allows you to bind a bunch of fbs to crtcs with independent
positions, as well as set a bunch of planes to specific fbs and
layouts.  Finally, it lets you change the connector config at the same
time, with a flag to simply test a config instead of actually setting
it.

Any comments?  Do we also need to set gamma or other properties as part
of this?  What about cursors?

Thanks,
Jesse

Re: [RFC] drm: atomic mode set API

[Adam Jackson]

On 2/15/12 5:42 PM, Jesse Barnes wrote:

> +#define DRM_SET_CONFIG_TEST (1<<0) /* don't change the config, just test it for validity */
> +
> +struct drm_mode_set_config {
> +	__u64 crtcs;
> +	__u64 crtc_fbs;
> +	__u64 crtc_xpos; /* array of x coords for crtcs */
> +	__u64 crtc_ypos; /* array of y coords for crtcs */
> +	__u32 count_crtcs;
> +
> +	__u64 plane_sets; /* array of set_plane structs */
> +
> +	__u32 count_planes;
> +
> +	__u64 connectors;
> +	__u64 connector_modes;
> +	__u32 count_connectors;
> +
> +	__u32 flags;
> +};

This appears to be missing some []s, but I think the intent is clear.

>   #define DRM_MODE_ENCODER_NONE	0
>   #define DRM_MODE_ENCODER_DAC	1
>   #define DRM_MODE_ENCODER_TMDS	2
>
> This allows you to bind a bunch of fbs to crtcs with independent
> positions, as well as set a bunch of planes to specific fbs and
> layouts.  Finally, it lets you change the connector config at the same
> time, with a flag to simply test a config instead of actually setting
> it.
>
> Any comments?  Do we also need to set gamma or other properties as part
> of this?  What about cursors?

I guess you might want to set gamma atomically, but I can't imagine it 
being a factor in anyone's "can I do this" logic.

How do you pass in pixel format?  Do you just assume the existing fb is 
already in the correct format?  That could work but it kind of sucks for 
low-memory environments since you'd need to have enough room to 
pre-create all the fbs.  You could still do the "tear everything down 
first" approach to work around that, but then you'd still have the 
possibility of having nothing lit up _and_ not being able to set what 
was requested, and then needing to unwind in userspace.

I'd sort of also want to see audio reflected in this (sigh), since 
that's going to affect the bandwidth math.  DP 1.2 makes that even worse.

- ajax

Re: [RFC] drm: atomic mode set API

[Daniel Vetter]

On Wed, Feb 15, 2012 at 05:59:45PM -0500, Adam Jackson wrote:
> On 2/15/12 5:42 PM, Jesse Barnes wrote:
> 
> >+#define DRM_SET_CONFIG_TEST (1<<0) /* don't change the config, just test it for validity */
> >+
> >+struct drm_mode_set_config {
> >+	__u64 crtcs;
> >+	__u64 crtc_fbs;
> >+	__u64 crtc_xpos; /* array of x coords for crtcs */
> >+	__u64 crtc_ypos; /* array of y coords for crtcs */
> >+	__u32 count_crtcs;
> >+
> >+	__u64 plane_sets; /* array of set_plane structs */
> >+
> >+	__u32 count_planes;
> >+
> >+	__u64 connectors;
> >+	__u64 connector_modes;
> >+	__u32 count_connectors;
> >+
> >+	__u32 flags;
> >+};
> 
> This appears to be missing some []s, but I think the intent is clear.
> 
> >  #define DRM_MODE_ENCODER_NONE	0
> >  #define DRM_MODE_ENCODER_DAC	1
> >  #define DRM_MODE_ENCODER_TMDS	2
> >
> >This allows you to bind a bunch of fbs to crtcs with independent
> >positions, as well as set a bunch of planes to specific fbs and
> >layouts.  Finally, it lets you change the connector config at the same
> >time, with a flag to simply test a config instead of actually setting
> >it.
> >
> >Any comments?  Do we also need to set gamma or other properties as part
> >of this?  What about cursors?
> 
> I guess you might want to set gamma atomically, but I can't imagine
> it being a factor in anyone's "can I do this" logic.
> 
> How do you pass in pixel format?  Do you just assume the existing fb
> is already in the correct format?  That could work but it kind of
> sucks for low-memory environments since you'd need to have enough
> room to pre-create all the fbs.  You could still do the "tear
> everything down first" approach to work around that, but then you'd
> still have the possibility of having nothing lit up _and_ not being
> able to set what was requested, and then needing to unwind in
> userspace.
> 
> I'd sort of also want to see audio reflected in this (sigh), since
> that's going to affect the bandwidth math.  DP 1.2 makes that even
> worse.

Yeah, I think we should include any funky connector, crtc, plane
properties (the latter don't exist yet, but I guess they will sooner or
later) because they all might affect how many and which hw resources we
need (I'm thinking e.g. of plane setups for hw that reuses crtc engines as
planes, but where you can use the crtc scanout engine for something else
if it's completely occluded or set to just scan out the black borders with
a parameter).
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [RFC] drm: atomic mode set API

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 2865 bytes --]

On Thu, 16 Feb 2012 00:12:49 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Feb 15, 2012 at 05:59:45PM -0500, Adam Jackson wrote:
> > On 2/15/12 5:42 PM, Jesse Barnes wrote:
> > 
> > >+#define DRM_SET_CONFIG_TEST (1<<0) /* don't change the config, just test it for validity */
> > >+
> > >+struct drm_mode_set_config {
> > >+	__u64 crtcs;
> > >+	__u64 crtc_fbs;
> > >+	__u64 crtc_xpos; /* array of x coords for crtcs */
> > >+	__u64 crtc_ypos; /* array of y coords for crtcs */
> > >+	__u32 count_crtcs;
> > >+
> > >+	__u64 plane_sets; /* array of set_plane structs */
> > >+
> > >+	__u32 count_planes;
> > >+
> > >+	__u64 connectors;
> > >+	__u64 connector_modes;
> > >+	__u32 count_connectors;
> > >+
> > >+	__u32 flags;
> > >+};
> > 
> > This appears to be missing some []s, but I think the intent is clear.
> > 
> > >  #define DRM_MODE_ENCODER_NONE	0
> > >  #define DRM_MODE_ENCODER_DAC	1
> > >  #define DRM_MODE_ENCODER_TMDS	2
> > >
> > >This allows you to bind a bunch of fbs to crtcs with independent
> > >positions, as well as set a bunch of planes to specific fbs and
> > >layouts.  Finally, it lets you change the connector config at the same
> > >time, with a flag to simply test a config instead of actually setting
> > >it.
> > >
> > >Any comments?  Do we also need to set gamma or other properties as part
> > >of this?  What about cursors?
> > 
> > I guess you might want to set gamma atomically, but I can't imagine
> > it being a factor in anyone's "can I do this" logic.
> > 
> > How do you pass in pixel format?  Do you just assume the existing fb
> > is already in the correct format?  That could work but it kind of
> > sucks for low-memory environments since you'd need to have enough
> > room to pre-create all the fbs.  You could still do the "tear
> > everything down first" approach to work around that, but then you'd
> > still have the possibility of having nothing lit up _and_ not being
> > able to set what was requested, and then needing to unwind in
> > userspace.
> > 
> > I'd sort of also want to see audio reflected in this (sigh), since
> > that's going to affect the bandwidth math.  DP 1.2 makes that even
> > worse.
> 
> Yeah, I think we should include any funky connector, crtc, plane
> properties (the latter don't exist yet, but I guess they will sooner or
> later) because they all might affect how many and which hw resources we
> need (I'm thinking e.g. of plane setups for hw that reuses crtc engines as
> planes, but where you can use the crtc scanout engine for something else
> if it's completely occluded or set to just scan out the black borders with
> a parameter).

So just an array of set_property structs per object as well?  That came
up at today's meeting... Seems doable.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: atomic mode set API

[Ville Syrjälä]

On Thu, Feb 16, 2012 at 12:12:49AM +0100, Daniel Vetter wrote:
> On Wed, Feb 15, 2012 at 05:59:45PM -0500, Adam Jackson wrote:
> > On 2/15/12 5:42 PM, Jesse Barnes wrote:
> > 
> > >+#define DRM_SET_CONFIG_TEST (1<<0) /* don't change the config, just test it for validity */

This would be OK for cases like checking whether the scene
can be composited with planes, of if you need to fall back
to full GPU composition.

For actaully trying to find out why the config is bad, this
is of course insufficient. For that we'd need something a lot
more fine grained for each logical part of the full state. And 
even that could be a bit vague, since it might not be any single
piece of the state that's wrong, it could be the combination of
some of the pieces.

> > >+
> > >+struct drm_mode_set_config {
> > >+	__u64 crtcs;
> > >+	__u64 crtc_fbs;
> > >+	__u64 crtc_xpos; /* array of x coords for crtcs */
> > >+	__u64 crtc_ypos; /* array of y coords for crtcs */
> > >+	__u32 count_crtcs;
> > >+
> > >+	__u64 plane_sets; /* array of set_plane structs */
> > >+
> > >+	__u32 count_planes;
> > >+
> > >+	__u64 connectors;
> > >+	__u64 connector_modes;
> > >+	__u32 count_connectors;
> > >+
> > >+	__u32 flags;
> > >+};
> > 
> > This appears to be missing some []s, but I think the intent is clear.
> > 
> > >  #define DRM_MODE_ENCODER_NONE	0
> > >  #define DRM_MODE_ENCODER_DAC	1
> > >  #define DRM_MODE_ENCODER_TMDS	2
> > >
> > >This allows you to bind a bunch of fbs to crtcs with independent
> > >positions, as well as set a bunch of planes to specific fbs and
> > >layouts.  Finally, it lets you change the connector config at the same
> > >time, with a flag to simply test a config instead of actually setting
> > >it.
> > >
> > >Any comments?  Do we also need to set gamma or other properties as part
> > >of this?  What about cursors?
> > 
> > I guess you might want to set gamma atomically, but I can't imagine
> > it being a factor in anyone's "can I do this" logic.
> > 
> > How do you pass in pixel format?  Do you just assume the existing fb
> > is already in the correct format?  That could work but it kind of
> > sucks for low-memory environments since you'd need to have enough
> > room to pre-create all the fbs.  You could still do the "tear
> > everything down first" approach to work around that, but then you'd
> > still have the possibility of having nothing lit up _and_ not being
> > able to set what was requested, and then needing to unwind in
> > userspace.
> > 
> > I'd sort of also want to see audio reflected in this (sigh), since
> > that's going to affect the bandwidth math.  DP 1.2 makes that even
> > worse.
> 
> Yeah, I think we should include any funky connector, crtc, plane
> properties

How about taking this idea to its ultimate conclusion: everything is a
property.

Then this ioctl would just be a collection of objects and a bunch of
changed properties for each.

It would require standardising the properties, or some of them at least
and having two property namespaces: one standard, one driver specific.

It would be easy to extend w/o ABI breaks, whereas in the current model
you either need to introduce a new "set everything v2" ioctl, or use
properties anyway.

It would also make it slightly easier to optimize use cases like page
flipping N planes/crtcs at the same time. Ideally you'd just do a few
sanity checks and program the new fb addresses into the hardware. With
the current structure you either need to redo all scaling calculations,
or check that none of the coordinates changed before you can take the
optimized path.

> (the latter don't exist yet, but I guess they will sooner or
> later)

That would be one way to implement color space conversion settings,
color adjustments etc., and I agree, they should be part of the atomic
mode set. The atomic mode set should include pretty much everything.

> because they all might affect how many and which hw resources we
> need (I'm thinking e.g. of plane setups for hw that reuses crtc engines as
> planes, but where you can use the crtc scanout engine for something else
> if it's completely occluded or set to just scan out the black borders with
> a parameter).

Another one of my favorite ideas would be to deprecate the crtc as a
scanout engine concept and just represent all scanout engines as planes.
As there will be a new API anyway, it might be a good time to make that
change.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] drm: atomic mode set API

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 3085 bytes --]

On Wed, 15 Feb 2012 17:59:45 -0500
Adam Jackson <ajax@redhat.com> wrote:

> On 2/15/12 5:42 PM, Jesse Barnes wrote:
> 
> > +#define DRM_SET_CONFIG_TEST (1<<0) /* don't change the config, just test it for validity */
> > +
> > +struct drm_mode_set_config {
> > +	__u64 crtcs;
> > +	__u64 crtc_fbs;
> > +	__u64 crtc_xpos; /* array of x coords for crtcs */
> > +	__u64 crtc_ypos; /* array of y coords for crtcs */
> > +	__u32 count_crtcs;
> > +
> > +	__u64 plane_sets; /* array of set_plane structs */
> > +
> > +	__u32 count_planes;
> > +
> > +	__u64 connectors;
> > +	__u64 connector_modes;
> > +	__u32 count_connectors;
> > +
> > +	__u32 flags;
> > +};
> 
> This appears to be missing some []s, but I think the intent is clear.

Yeah, all the u64s are arrays.

> >   #define DRM_MODE_ENCODER_NONE	0
> >   #define DRM_MODE_ENCODER_DAC	1
> >   #define DRM_MODE_ENCODER_TMDS	2
> >
> > This allows you to bind a bunch of fbs to crtcs with independent
> > positions, as well as set a bunch of planes to specific fbs and
> > layouts.  Finally, it lets you change the connector config at the same
> > time, with a flag to simply test a config instead of actually setting
> > it.
> >
> > Any comments?  Do we also need to set gamma or other properties as part
> > of this?  What about cursors?
> 
> I guess you might want to set gamma atomically, but I can't imagine it 
> being a factor in anyone's "can I do this" logic.
> 
> How do you pass in pixel format?  Do you just assume the existing fb is 
> already in the correct format?  That could work but it kind of sucks for 
> low-memory environments since you'd need to have enough room to 
> pre-create all the fbs.  You could still do the "tear everything down 
> first" approach to work around that, but then you'd still have the 
> possibility of having nothing lit up _and_ not being able to set what 
> was requested, and then needing to unwind in userspace.

Yeah, the assumption is that the fbs have already been set up.  You'll
need to do that anyway if you want to queue to a flip chain or
whatever.  I don't see an easy way of linking that in here, since to
format an fb you need an underlying buffer allocated, which is where
the memory issues you mentioned come in.  So in that sense, it works
like the other APIs in that it assumes you have an object with content
and the right pixel format all ready to go.

> I'd sort of also want to see audio reflected in this (sigh), since 
> that's going to affect the bandwidth math.  DP 1.2 makes that even worse.

Requesting a specific audio config?  Or just returning an informative
error code about why the mode was rejected (e.g. you're playing 7.1
audio on this link and there's not enough bw for the config you wanted)?

How to reasonably return errors from this is an open question; should
we return an array of error codes to indicate all the possible issues?
Or just try our best with a single dimension and hope userspace can
figure things out?

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: atomic mode set API

[Rob Clark]

On Wed, Feb 15, 2012 at 4:59 PM, Adam Jackson <ajax@redhat.com> wrote:
> On 2/15/12 5:42 PM, Jesse Barnes wrote:
>
>> +#define DRM_SET_CONFIG_TEST (1<<0) /* don't change the config, just test
>> it for validity */
>> +
>> +struct drm_mode_set_config {
>> +       __u64 crtcs;
>> +       __u64 crtc_fbs;
>> +       __u64 crtc_xpos; /* array of x coords for crtcs */
>> +       __u64 crtc_ypos; /* array of y coords for crtcs */
>> +       __u32 count_crtcs;
>> +
>> +       __u64 plane_sets; /* array of set_plane structs */
>> +
>> +       __u32 count_planes;
>> +
>> +       __u64 connectors;
>> +       __u64 connector_modes;
>> +       __u32 count_connectors;
>> +
>> +       __u32 flags;
>> +};
>
>
> This appears to be missing some []s, but I think the intent is clear.
>
>
>>  #define DRM_MODE_ENCODER_NONE 0
>>  #define DRM_MODE_ENCODER_DAC  1
>>  #define DRM_MODE_ENCODER_TMDS 2
>>
>> This allows you to bind a bunch of fbs to crtcs with independent
>> positions, as well as set a bunch of planes to specific fbs and
>> layouts.  Finally, it lets you change the connector config at the same
>> time, with a flag to simply test a config instead of actually setting
>> it.
>>
>> Any comments?  Do we also need to set gamma or other properties as part
>> of this?  What about cursors?
>
>
> I guess you might want to set gamma atomically, but I can't imagine it being
> a factor in anyone's "can I do this" logic.
>
> How do you pass in pixel format?  Do you just assume the existing fb is
> already in the correct format?  That could work but it kind of sucks for
> low-memory environments since you'd need to have enough room to pre-create
> all the fbs.  You could still do the "tear everything down first" approach
> to work around that, but then you'd still have the possibility of having
> nothing lit up _and_ not being able to set what was requested, and then
> needing to unwind in userspace.
>
> I'd sort of also want to see audio reflected in this (sigh), since that's
> going to affect the bandwidth math.  DP 1.2 makes that even worse.

Just to be devil's advocate here..  audio settings wouldn't change
before/after as a result of modeset change (I guess).  And on the
other side, if you are changing some audio setting via alsa, which
makes your previous valid mode configuration invalid, then you are
kinda screwed..

As far as other sort of properties that matter (beyond color format,
which is already an attribute of the fb), the one that immediately
comes to mind is z-order.

BR,
-R

> - ajax
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: atomic mode set API

[Jakob Bornecrantz]

----- Original Message -----
> Many of us really want (and need) a way to set the whole display
> configuration atomically, as well as test a global config.
> 
> In talking with Rob and Alex here at ELC a bit, I think this may be
> enough:
> 
> diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
> index 2a2acda..2864b02 100644
> --- a/include/drm/drm_mode.h
> +++ b/include/drm/drm_mode.h
> @@ -157,6 +157,26 @@ struct drm_mode_get_plane_res {
>  	__u32 count_planes;
>  };
>  
> +#define DRM_SET_CONFIG_TEST (1<<0) /* don't change the config, just test it for validity */
> +
> +struct drm_mode_set_config {
> +	__u64 crtcs;
> +	__u64 crtc_fbs;
> +	__u64 crtc_xpos; /* array of x coords for crtcs */
> +	__u64 crtc_ypos; /* array of y coords for crtcs */
> +	__u32 count_crtcs;
> +
> +	__u64 plane_sets; /* array of set_plane structs */
> +
> +	__u32 count_planes;
> +
> +	__u64 connectors;
> +	__u64 connector_modes;
> +	__u32 count_connectors;

How do you do mirroring? Why not just send down a array of
drm_mode_crtc?

> +
> +	__u32 flags;
> +};
> +
>  #define DRM_MODE_ENCODER_NONE	0
>  #define DRM_MODE_ENCODER_DAC	1
>  #define DRM_MODE_ENCODER_TMDS	2
> 
> This allows you to bind a bunch of fbs to crtcs with independent
> positions, as well as set a bunch of planes to specific fbs and
> layouts.  Finally, it lets you change the connector config at the
> same time, with a flag to simply test a config instead of
> actually setting it.
> 
> Any comments?  Do we also need to set gamma or other properties as
> part of this?  What about cursors?

I would like to see properties added as well.

Cheers, Jakob.

Re: [RFC] drm: atomic mode set API

[Jesse Barnes]

On Thu, 16 Feb 2012 04:46:39 -0800 (PST)
Jakob Bornecrantz <jakob@vmware.com> wrote:

> ----- Original Message -----
> > Many of us really want (and need) a way to set the whole display
> > configuration atomically, as well as test a global config.
> > 
> > In talking with Rob and Alex here at ELC a bit, I think this may be
> > enough:
> > 
> > diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
> > index 2a2acda..2864b02 100644
> > --- a/include/drm/drm_mode.h
> > +++ b/include/drm/drm_mode.h
> > @@ -157,6 +157,26 @@ struct drm_mode_get_plane_res {
> >  	__u32 count_planes;
> >  };
> >  
> > +#define DRM_SET_CONFIG_TEST (1<<0) /* don't change the config,
> > just test it for validity */ +
> > +struct drm_mode_set_config {
> > +	__u64 crtcs;
> > +	__u64 crtc_fbs;
> > +	__u64 crtc_xpos; /* array of x coords for crtcs */
> > +	__u64 crtc_ypos; /* array of y coords for crtcs */
> > +	__u32 count_crtcs;
> > +
> > +	__u64 plane_sets; /* array of set_plane structs */
> > +
> > +	__u32 count_planes;
> > +
> > +	__u64 connectors;
> > +	__u64 connector_modes;
> > +	__u32 count_connectors;
> 
> How do you do mirroring? Why not just send down a array of
> drm_mode_crtc?

You can pass the same fb for multiple crtcs if you want to mirror.
Using an array of mode_crtcs would fix one problem though; the mode
array should be tied to the crtc, not the connector (i.e. you can't set
two different modes on two different connectors with only a single
crtc).

> > Any comments?  Do we also need to set gamma or other properties as
> > part of this?  What about cursors?
> 
> I would like to see properties added as well.

That's +3 now I think. :)  I'll try to figure out a way to add that...

Jesse