[RFC] drm: add hint to userspace about whether a plane can scale

[RFC] drm: add hint to userspace about whether a plane can scale

[Rob Clark]

When you have a mix of planes that can scale and those that cannot
scale, userspace really wants to have some hint to know which planes
can definitely not scale so it knows to assign them to unscaled layers.
I don't think it is fully possible to describe scaling constraints in
a generic way, so I don't think it is even worth trying, so this is
not a substitute for atomic TESTONLY step, but it does reduce the
search-space for userspace.  In the common case, most layers will not
be scaled so knowing the best planes to pick for those layers is
useful.
---
 drivers/gpu/drm/drm_crtc.c                | 1 +
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 1 +
 include/drm/drm_crtc.h                    | 2 ++
 include/uapi/drm/drm_mode.h               | 3 +++
 4 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b4b973f..d7e0e0d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2389,6 +2389,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	plane_resp->plane_id = plane->base.id;
 	plane_resp->possible_crtcs = plane->possible_crtcs;
 	plane_resp->gamma_size = 0;
+	plane_resp->can_scale = plane->can_scale;
 
 	/*
 	 * This ioctl is called twice, once to determine how much space is
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
index 692c888..2061c83 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
@@ -908,6 +908,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
 	mdp5_plane->pipe = pipe;
 	mdp5_plane->name = pipe2name(pipe);
 	mdp5_plane->caps = caps;
+	plane->can_scale = !!(caps & MDP_PIPE_CAP_SCALE);
 
 	mdp5_plane->nformats = mdp_get_formats(mdp5_plane->formats,
 		ARRAY_SIZE(mdp5_plane->formats),
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index d74d47a..6e290b6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1679,6 +1679,7 @@ enum drm_plane_type {
  * @format_types: array of formats supported by this plane
  * @format_count: number of formats supported
  * @format_default: driver hasn't supplied supported formats for the plane
+ * @can_scale: a hint to userspace that this plane can (or cannot) scale
  * @crtc: currently bound CRTC
  * @fb: currently bound fb
  * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by
@@ -1710,6 +1711,7 @@ struct drm_plane {
 	uint32_t *format_types;
 	unsigned int format_count;
 	bool format_default;
+	bool can_scale;
 
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index ce71ad5..5bf9361 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -191,6 +191,9 @@ struct drm_mode_get_plane {
 
 	__u32 count_format_types;
 	__u64 format_type_ptr;
+
+	__u32 can_scale;
+	__u32 pad;
 };
 
 struct drm_mode_get_plane_res {
-- 
2.7.4

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

Re: [RFC] drm: add hint to userspace about whether a plane can scale

[Brian Starkey]

Hi Rob,

On Thu, Oct 20, 2016 at 04:17:14PM -0400, Rob Clark wrote:
>When you have a mix of planes that can scale and those that cannot
>scale, userspace really wants to have some hint to know which planes
>can definitely not scale so it knows to assign them to unscaled layers.
>I don't think it is fully possible to describe scaling constraints in
>a generic way, so I don't think it is even worth trying, so this is
>not a substitute for atomic TESTONLY step, but it does reduce the
>search-space for userspace.  In the common case, most layers will not
>be scaled so knowing the best planes to pick for those layers is
>useful.

Somewhat related to what Daniel mentioned on IRC about driver
consistency - how about making it "cannot_scale". This is then opt-in
for drivers, and should mean userspace can always trust it if it's
set.

i.e. if cannot_scale == false, userspace can give it a shot, but if
cannot_scale == true it should never bother.

Either way, even with a device-specific planner we would want this
hint to manage different HW versions so I'm in favour. But...

>---
> drivers/gpu/drm/drm_crtc.c                | 1 +
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 1 +
> include/drm/drm_crtc.h                    | 2 ++
> include/uapi/drm/drm_mode.h               | 3 +++
> 4 files changed, 7 insertions(+)
>
>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>index b4b973f..d7e0e0d 100644
>--- a/drivers/gpu/drm/drm_crtc.c
>+++ b/drivers/gpu/drm/drm_crtc.c
>@@ -2389,6 +2389,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
> 	plane_resp->plane_id = plane->base.id;
> 	plane_resp->possible_crtcs = plane->possible_crtcs;
> 	plane_resp->gamma_size = 0;
>+	plane_resp->can_scale = plane->can_scale;
>
> 	/*
> 	 * This ioctl is called twice, once to determine how much space is
>diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>index 692c888..2061c83 100644
>--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>@@ -908,6 +908,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
> 	mdp5_plane->pipe = pipe;
> 	mdp5_plane->name = pipe2name(pipe);
> 	mdp5_plane->caps = caps;
>+	plane->can_scale = !!(caps & MDP_PIPE_CAP_SCALE);
>
> 	mdp5_plane->nformats = mdp_get_formats(mdp5_plane->formats,
> 		ARRAY_SIZE(mdp5_plane->formats),
>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>index d74d47a..6e290b6 100644
>--- a/include/drm/drm_crtc.h
>+++ b/include/drm/drm_crtc.h
>@@ -1679,6 +1679,7 @@ enum drm_plane_type {
>  * @format_types: array of formats supported by this plane
>  * @format_count: number of formats supported
>  * @format_default: driver hasn't supplied supported formats for the plane
>+ * @can_scale: a hint to userspace that this plane can (or cannot) scale
>  * @crtc: currently bound CRTC
>  * @fb: currently bound fb
>  * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by
>@@ -1710,6 +1711,7 @@ struct drm_plane {
> 	uint32_t *format_types;
> 	unsigned int format_count;
> 	bool format_default;
>+	bool can_scale;
>
> 	struct drm_crtc *crtc;
> 	struct drm_framebuffer *fb;
>diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>index ce71ad5..5bf9361 100644
>--- a/include/uapi/drm/drm_mode.h
>+++ b/include/uapi/drm/drm_mode.h
>@@ -191,6 +191,9 @@ struct drm_mode_get_plane {
>
> 	__u32 count_format_types;
> 	__u64 format_type_ptr;
>+
>+	__u32 can_scale;

... 32 bits for a bool does seem a bit wasteful.

Thanks,
Brian

>+	__u32 pad;
> };
>
> struct drm_mode_get_plane_res {
>-- 
>2.7.4
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: add hint to userspace about whether a plane can scale

[Daniel Vetter]

On Fri, Oct 21, 2016 at 09:58:45AM +0100, Brian Starkey wrote:
> Hi Rob,
> 
> On Thu, Oct 20, 2016 at 04:17:14PM -0400, Rob Clark wrote:
> > When you have a mix of planes that can scale and those that cannot
> > scale, userspace really wants to have some hint to know which planes
> > can definitely not scale so it knows to assign them to unscaled layers.
> > I don't think it is fully possible to describe scaling constraints in
> > a generic way, so I don't think it is even worth trying, so this is
> > not a substitute for atomic TESTONLY step, but it does reduce the
> > search-space for userspace.  In the common case, most layers will not
> > be scaled so knowing the best planes to pick for those layers is
> > useful.
> 
> Somewhat related to what Daniel mentioned on IRC about driver
> consistency - how about making it "cannot_scale". This is then opt-in
> for drivers, and should mean userspace can always trust it if it's
> set.
> 
> i.e. if cannot_scale == false, userspace can give it a shot, but if
> cannot_scale == true it should never bother.
> 
> Either way, even with a device-specific planner we would want this
> hint to manage different HW versions so I'm in favour. But...

I think first thing we should do is add some backoff heuristics to
drm_hwcomposer to try the same plane also with a non-scaled surface (after
having tried it with a scaled one). That would probably be as effective as
adding "can_scale", but with the upshot that we're not at the mercy of
drivers exposing it correctly.

I'm always vary when we have the same limit checks 2 (in this case once in
atomic_check, and once in the code that registers the property). And I'd
like to avoid that as much as possible. We could avoid this by making the
can_scale property mandatory, and enforcing it in the drm core. I.e. if
it's not set, we reject scaled planes. That should give some decent
motivation for driver writers to update them correctly. But without such a
self-consistency check I don't really like this. It would be akin to
adding "can_rotate" besides the "rotation" prop, and allowing drivers to
botch things up and not register stuff consistently.
-Daniel

> 
> > ---
> > drivers/gpu/drm/drm_crtc.c                | 1 +
> > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 1 +
> > include/drm/drm_crtc.h                    | 2 ++
> > include/uapi/drm/drm_mode.h               | 3 +++
> > 4 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index b4b973f..d7e0e0d 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -2389,6 +2389,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
> > 	plane_resp->plane_id = plane->base.id;
> > 	plane_resp->possible_crtcs = plane->possible_crtcs;
> > 	plane_resp->gamma_size = 0;
> > +	plane_resp->can_scale = plane->can_scale;
> > 
> > 	/*
> > 	 * This ioctl is called twice, once to determine how much space is
> > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> > index 692c888..2061c83 100644
> > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> > @@ -908,6 +908,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
> > 	mdp5_plane->pipe = pipe;
> > 	mdp5_plane->name = pipe2name(pipe);
> > 	mdp5_plane->caps = caps;
> > +	plane->can_scale = !!(caps & MDP_PIPE_CAP_SCALE);
> > 
> > 	mdp5_plane->nformats = mdp_get_formats(mdp5_plane->formats,
> > 		ARRAY_SIZE(mdp5_plane->formats),
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index d74d47a..6e290b6 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1679,6 +1679,7 @@ enum drm_plane_type {
> >  * @format_types: array of formats supported by this plane
> >  * @format_count: number of formats supported
> >  * @format_default: driver hasn't supplied supported formats for the plane
> > + * @can_scale: a hint to userspace that this plane can (or cannot) scale
> >  * @crtc: currently bound CRTC
> >  * @fb: currently bound fb
> >  * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by
> > @@ -1710,6 +1711,7 @@ struct drm_plane {
> > 	uint32_t *format_types;
> > 	unsigned int format_count;
> > 	bool format_default;
> > +	bool can_scale;
> > 
> > 	struct drm_crtc *crtc;
> > 	struct drm_framebuffer *fb;
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index ce71ad5..5bf9361 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -191,6 +191,9 @@ struct drm_mode_get_plane {
> > 
> > 	__u32 count_format_types;
> > 	__u64 format_type_ptr;
> > +
> > +	__u32 can_scale;
> 
> ... 32 bits for a bool does seem a bit wasteful.
> 
> Thanks,
> Brian
> 
> > +	__u32 pad;
> > };
> > 
> > struct drm_mode_get_plane_res {
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [RFC] drm: add hint to userspace about whether a plane can scale

[Rob Clark]

On Fri, Oct 21, 2016 at 4:58 AM, Brian Starkey <brian.starkey@arm.com> wrote:
> Hi Rob,
>
> On Thu, Oct 20, 2016 at 04:17:14PM -0400, Rob Clark wrote:
>>
>> When you have a mix of planes that can scale and those that cannot
>> scale, userspace really wants to have some hint to know which planes
>> can definitely not scale so it knows to assign them to unscaled layers.
>> I don't think it is fully possible to describe scaling constraints in
>> a generic way, so I don't think it is even worth trying, so this is
>> not a substitute for atomic TESTONLY step, but it does reduce the
>> search-space for userspace.  In the common case, most layers will not
>> be scaled so knowing the best planes to pick for those layers is
>> useful.
>
>
> Somewhat related to what Daniel mentioned on IRC about driver
> consistency - how about making it "cannot_scale". This is then opt-in
> for drivers, and should mean userspace can always trust it if it's
> set.

yeah, I thought about it.. but a negative-cap sounded funny.  I could
just initialize plane->can_scale to true in drm_universal_plane_init()
if needed.

> i.e. if cannot_scale == false, userspace can give it a shot, but if
> cannot_scale == true it should never bother.
>
> Either way, even with a device-specific planner we would want this
> hint to manage different HW versions so I'm in favour. But...
>
>
>> ---
>> drivers/gpu/drm/drm_crtc.c                | 1 +
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 1 +
>> include/drm/drm_crtc.h                    | 2 ++
>> include/uapi/drm/drm_mode.h               | 3 +++
>> 4 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index b4b973f..d7e0e0d 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -2389,6 +2389,7 @@ int drm_mode_getplane(struct drm_device *dev, void
>> *data,
>>         plane_resp->plane_id = plane->base.id;
>>         plane_resp->possible_crtcs = plane->possible_crtcs;
>>         plane_resp->gamma_size = 0;
>> +       plane_resp->can_scale = plane->can_scale;
>>
>>         /*
>>          * This ioctl is called twice, once to determine how much space is
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> index 692c888..2061c83 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> @@ -908,6 +908,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device
>> *dev,
>>         mdp5_plane->pipe = pipe;
>>         mdp5_plane->name = pipe2name(pipe);
>>         mdp5_plane->caps = caps;
>> +       plane->can_scale = !!(caps & MDP_PIPE_CAP_SCALE);
>>
>>         mdp5_plane->nformats = mdp_get_formats(mdp5_plane->formats,
>>                 ARRAY_SIZE(mdp5_plane->formats),
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index d74d47a..6e290b6 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1679,6 +1679,7 @@ enum drm_plane_type {
>>  * @format_types: array of formats supported by this plane
>>  * @format_count: number of formats supported
>>  * @format_default: driver hasn't supplied supported formats for the plane
>> + * @can_scale: a hint to userspace that this plane can (or cannot) scale
>>  * @crtc: currently bound CRTC
>>  * @fb: currently bound fb
>>  * @old_fb: Temporary tracking of the old fb while a modeset is ongoing.
>> Used by
>> @@ -1710,6 +1711,7 @@ struct drm_plane {
>>         uint32_t *format_types;
>>         unsigned int format_count;
>>         bool format_default;
>> +       bool can_scale;
>>
>>         struct drm_crtc *crtc;
>>         struct drm_framebuffer *fb;
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index ce71ad5..5bf9361 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -191,6 +191,9 @@ struct drm_mode_get_plane {
>>
>>         __u32 count_format_types;
>>         __u64 format_type_ptr;
>> +
>> +       __u32 can_scale;
>
>
> ... 32 bits for a bool does seem a bit wasteful.

alternative is to make it a bitmask of caps, although not sure how
much else we would add.  And isn't like this ioctl is critical path,
or anything like that.

BR,
-R

> Thanks,
> Brian
>
>> +       __u32 pad;
>> };
>>
>> struct drm_mode_get_plane_res {
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: add hint to userspace about whether a plane can scale

[Rob Clark]

On Fri, Oct 21, 2016 at 8:35 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Oct 21, 2016 at 09:58:45AM +0100, Brian Starkey wrote:
>> Hi Rob,
>>
>> On Thu, Oct 20, 2016 at 04:17:14PM -0400, Rob Clark wrote:
>> > When you have a mix of planes that can scale and those that cannot
>> > scale, userspace really wants to have some hint to know which planes
>> > can definitely not scale so it knows to assign them to unscaled layers.
>> > I don't think it is fully possible to describe scaling constraints in
>> > a generic way, so I don't think it is even worth trying, so this is
>> > not a substitute for atomic TESTONLY step, but it does reduce the
>> > search-space for userspace.  In the common case, most layers will not
>> > be scaled so knowing the best planes to pick for those layers is
>> > useful.
>>
>> Somewhat related to what Daniel mentioned on IRC about driver
>> consistency - how about making it "cannot_scale". This is then opt-in
>> for drivers, and should mean userspace can always trust it if it's
>> set.
>>
>> i.e. if cannot_scale == false, userspace can give it a shot, but if
>> cannot_scale == true it should never bother.
>>
>> Either way, even with a device-specific planner we would want this
>> hint to manage different HW versions so I'm in favour. But...
>
> I think first thing we should do is add some backoff heuristics to
> drm_hwcomposer to try the same plane also with a non-scaled surface (after
> having tried it with a scaled one). That would probably be as effective as
> adding "can_scale", but with the upshot that we're not at the mercy of
> drivers exposing it correctly.

well, ignoring the fact that drm-hwc2 just tries one thing then falls
back to gl (which should be fixed but it is a big pile of c++11 code
so that will be someone elses job ;-))...

I did think about this approach, but two many changing variables so
making userspace guess about this sort of thing just seems evil.

> I'm always vary when we have the same limit checks 2 (in this case once in
> atomic_check, and once in the code that registers the property). And I'd
> like to avoid that as much as possible. We could avoid this by making the
> can_scale property mandatory, and enforcing it in the drm core. I.e. if
> it's not set, we reject scaled planes. That should give some decent
> motivation for driver writers to update them correctly. But without such a
> self-consistency check I don't really like this. It would be akin to
> adding "can_rotate" besides the "rotation" prop, and allowing drivers to
> botch things up and not register stuff consistently.

Fair enough, I'll add a check in drm core.  That is simple enough.  I
guess remaining question should can_scale default to true?

I guess the first time someone tries to bring up drm-hwc or similar
(ie. something more complex than single fullscreen layer) on their hw,
they are going to run into a few bugs in their driver, so I guess I'd
be ok for it to default to false and let people fix it when the time
comes.

BR,
-R

> -Daniel
>
>>
>> > ---
>> > drivers/gpu/drm/drm_crtc.c                | 1 +
>> > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 1 +
>> > include/drm/drm_crtc.h                    | 2 ++
>> > include/uapi/drm/drm_mode.h               | 3 +++
>> > 4 files changed, 7 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> > index b4b973f..d7e0e0d 100644
>> > --- a/drivers/gpu/drm/drm_crtc.c
>> > +++ b/drivers/gpu/drm/drm_crtc.c
>> > @@ -2389,6 +2389,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>> >     plane_resp->plane_id = plane->base.id;
>> >     plane_resp->possible_crtcs = plane->possible_crtcs;
>> >     plane_resp->gamma_size = 0;
>> > +   plane_resp->can_scale = plane->can_scale;
>> >
>> >     /*
>> >      * This ioctl is called twice, once to determine how much space is
>> > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> > index 692c888..2061c83 100644
>> > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>> > @@ -908,6 +908,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
>> >     mdp5_plane->pipe = pipe;
>> >     mdp5_plane->name = pipe2name(pipe);
>> >     mdp5_plane->caps = caps;
>> > +   plane->can_scale = !!(caps & MDP_PIPE_CAP_SCALE);
>> >
>> >     mdp5_plane->nformats = mdp_get_formats(mdp5_plane->formats,
>> >             ARRAY_SIZE(mdp5_plane->formats),
>> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> > index d74d47a..6e290b6 100644
>> > --- a/include/drm/drm_crtc.h
>> > +++ b/include/drm/drm_crtc.h
>> > @@ -1679,6 +1679,7 @@ enum drm_plane_type {
>> >  * @format_types: array of formats supported by this plane
>> >  * @format_count: number of formats supported
>> >  * @format_default: driver hasn't supplied supported formats for the plane
>> > + * @can_scale: a hint to userspace that this plane can (or cannot) scale
>> >  * @crtc: currently bound CRTC
>> >  * @fb: currently bound fb
>> >  * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by
>> > @@ -1710,6 +1711,7 @@ struct drm_plane {
>> >     uint32_t *format_types;
>> >     unsigned int format_count;
>> >     bool format_default;
>> > +   bool can_scale;
>> >
>> >     struct drm_crtc *crtc;
>> >     struct drm_framebuffer *fb;
>> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> > index ce71ad5..5bf9361 100644
>> > --- a/include/uapi/drm/drm_mode.h
>> > +++ b/include/uapi/drm/drm_mode.h
>> > @@ -191,6 +191,9 @@ struct drm_mode_get_plane {
>> >
>> >     __u32 count_format_types;
>> >     __u64 format_type_ptr;
>> > +
>> > +   __u32 can_scale;
>>
>> ... 32 bits for a bool does seem a bit wasteful.
>>
>> Thanks,
>> Brian
>>
>> > +   __u32 pad;
>> > };
>> >
>> > struct drm_mode_get_plane_res {
>> > --
>> > 2.7.4
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: add hint to userspace about whether a plane can scale

[Daniel Vetter]

On Fri, Oct 21, 2016 at 09:26:22AM -0400, Rob Clark wrote:
> On Fri, Oct 21, 2016 at 8:35 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Oct 21, 2016 at 09:58:45AM +0100, Brian Starkey wrote:
> >> Hi Rob,
> >>
> >> On Thu, Oct 20, 2016 at 04:17:14PM -0400, Rob Clark wrote:
> >> > When you have a mix of planes that can scale and those that cannot
> >> > scale, userspace really wants to have some hint to know which planes
> >> > can definitely not scale so it knows to assign them to unscaled layers.
> >> > I don't think it is fully possible to describe scaling constraints in
> >> > a generic way, so I don't think it is even worth trying, so this is
> >> > not a substitute for atomic TESTONLY step, but it does reduce the
> >> > search-space for userspace.  In the common case, most layers will not
> >> > be scaled so knowing the best planes to pick for those layers is
> >> > useful.
> >>
> >> Somewhat related to what Daniel mentioned on IRC about driver
> >> consistency - how about making it "cannot_scale". This is then opt-in
> >> for drivers, and should mean userspace can always trust it if it's
> >> set.
> >>
> >> i.e. if cannot_scale == false, userspace can give it a shot, but if
> >> cannot_scale == true it should never bother.
> >>
> >> Either way, even with a device-specific planner we would want this
> >> hint to manage different HW versions so I'm in favour. But...
> >
> > I think first thing we should do is add some backoff heuristics to
> > drm_hwcomposer to try the same plane also with a non-scaled surface (after
> > having tried it with a scaled one). That would probably be as effective as
> > adding "can_scale", but with the upshot that we're not at the mercy of
> > drivers exposing it correctly.
> 
> well, ignoring the fact that drm-hwc2 just tries one thing then falls
> back to gl (which should be fixed but it is a big pile of c++11 code
> so that will be someone elses job ;-))...
> 
> I did think about this approach, but two many changing variables so
> making userspace guess about this sort of thing just seems evil.

Imo drm-hwc2 needs to be fixed. Adding new uabi because the current
userspace makes a few too many assumption about how hw works (the only
thing which is supposed to always work is one full-screen unscaled primary
buffer) feels wrong. Imo the proper fix is to fix userspace to not scale
in the absolute last-resort fallback path. Because that won't work on many
i915 platforms either (or on many other chips fwiw).

> > I'm always vary when we have the same limit checks 2 (in this case once in
> > atomic_check, and once in the code that registers the property). And I'd
> > like to avoid that as much as possible. We could avoid this by making the
> > can_scale property mandatory, and enforcing it in the drm core. I.e. if
> > it's not set, we reject scaled planes. That should give some decent
> > motivation for driver writers to update them correctly. But without such a
> > self-consistency check I don't really like this. It would be akin to
> > adding "can_rotate" besides the "rotation" prop, and allowing drivers to
> > botch things up and not register stuff consistently.
> 
> Fair enough, I'll add a check in drm core.  That is simple enough.  I
> guess remaining question should can_scale default to true?

What's the point in making it true by default, i.e. lying by default?

> I guess the first time someone tries to bring up drm-hwc or similar
> (ie. something more complex than single fullscreen layer) on their hw,
> they are going to run into a few bugs in their driver, so I guess I'd
> be ok for it to default to false and let people fix it when the time
> comes.

Still not convinced that can_scale is worth it. I'd like to fix userspace
first, before we roll out the duct-tape on the kernel ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel