* [PATCH] drm: don't override possible_crtcs for primary/cursor planes
@ 2016-10-20 19:06 Rob Clark
2016-10-20 19:19 ` Ville Syrjälä
2016-10-20 19:22 ` Sean Paul
0 siblings, 2 replies; 6+ messages in thread
From: Rob Clark @ 2016-10-20 19:06 UTC (permalink / raw)
To: dri-devel
It is kind of a pointless restriction. If userspace does silly things
like using crtcA's cursor plane on crtcB, and then setcursor on crtcA,
it will end up with the overlay disabled on crtcB. But userspace is
allowed to shoot itself like this.
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
Note that cursor and primary planes are slightly useless to non-atomic
userspace, since userspace has no way to know *which* crtc a primary
or cursor plane belongs to. So lighting up a 2nd display may or may
not make your overlay planes go *poof*.
So basically use of cursor/primary planes plus legacy ioctls is an
undefined behavior if we go with this patch.
*Maybe* we want to restrict this to atomic userspace. (Ie. advertise
the more limited possible_crtcs for legacy userspace.) Depends on
whether we can (slightly retroactively) declare that mixing atomic
and legacy APIs is undefined behavior. Or if really needed, add
DRM_CLIENT_CAP_REALLY_REALLY_ATOMIC_NO_LEGACY_PLS. (Or just pass in
value 2 for existing DRM_CLIENT_CAP_ATOMIC)
drivers/gpu/drm/drm_crtc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index c215802..1e6d37f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -707,9 +707,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
crtc->primary = primary;
crtc->cursor = cursor;
if (primary)
- primary->possible_crtcs = 1 << drm_crtc_index(crtc);
+ WARN_ON(!(primary->possible_crtcs & (1 << drm_crtc_index(crtc))));
if (cursor)
- cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
+ WARN_ON(!(cursor->possible_crtcs & (1 << drm_crtc_index(crtc))));
if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
drm_object_attach_property(&crtc->base, config->prop_active, 0);
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm: don't override possible_crtcs for primary/cursor planes
2016-10-20 19:06 [PATCH] drm: don't override possible_crtcs for primary/cursor planes Rob Clark
@ 2016-10-20 19:19 ` Ville Syrjälä
2016-10-20 20:02 ` Rob Clark
2016-10-20 19:22 ` Sean Paul
1 sibling, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2016-10-20 19:19 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel
On Thu, Oct 20, 2016 at 03:06:06PM -0400, Rob Clark wrote:
> It is kind of a pointless restriction. If userspace does silly things
> like using crtcA's cursor plane on crtcB, and then setcursor on crtcA,
> it will end up with the overlay disabled on crtcB. But userspace is
> allowed to shoot itself like this.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> Note that cursor and primary planes are slightly useless to non-atomic
> userspace, since userspace has no way to know *which* crtc a primary
> or cursor plane belongs to. So lighting up a 2nd display may or may
> not make your overlay planes go *poof*.
>
> So basically use of cursor/primary planes plus legacy ioctls is an
> undefined behavior if we go with this patch.
>
> *Maybe* we want to restrict this to atomic userspace. (Ie. advertise
> the more limited possible_crtcs for legacy userspace.) Depends on
> whether we can (slightly retroactively) declare that mixing atomic
> and legacy APIs is undefined behavior. Or if really needed, add
> DRM_CLIENT_CAP_REALLY_REALLY_ATOMIC_NO_LEGACY_PLS. (Or just pass in
> value 2 for existing DRM_CLIENT_CAP_ATOMIC)
>
> drivers/gpu/drm/drm_crtc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index c215802..1e6d37f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -707,9 +707,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> crtc->primary = primary;
> crtc->cursor = cursor;
> if (primary)
> - primary->possible_crtcs = 1 << drm_crtc_index(crtc);
> + WARN_ON(!(primary->possible_crtcs & (1 << drm_crtc_index(crtc))));
> if (cursor)
> - cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
> + WARN_ON(!(cursor->possible_crtcs & (1 << drm_crtc_index(crtc))));
The slight issue here is that this would require the caller to know the
upcoming crtc index before its even assigned. So might be better just
having the caller populate these however it likes after the crtc_init()?
>
> if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> drm_object_attach_property(&crtc->base, config->prop_active, 0);
> --
> 2.7.4
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm: don't override possible_crtcs for primary/cursor planes
2016-10-20 19:19 ` Ville Syrjälä
@ 2016-10-20 20:02 ` Rob Clark
0 siblings, 0 replies; 6+ messages in thread
From: Rob Clark @ 2016-10-20 20:02 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: dri-devel@lists.freedesktop.org
On Thu, Oct 20, 2016 at 3:19 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 20, 2016 at 03:06:06PM -0400, Rob Clark wrote:
>> It is kind of a pointless restriction. If userspace does silly things
>> like using crtcA's cursor plane on crtcB, and then setcursor on crtcA,
>> it will end up with the overlay disabled on crtcB. But userspace is
>> allowed to shoot itself like this.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> Note that cursor and primary planes are slightly useless to non-atomic
>> userspace, since userspace has no way to know *which* crtc a primary
>> or cursor plane belongs to. So lighting up a 2nd display may or may
>> not make your overlay planes go *poof*.
>>
>> So basically use of cursor/primary planes plus legacy ioctls is an
>> undefined behavior if we go with this patch.
>>
>> *Maybe* we want to restrict this to atomic userspace. (Ie. advertise
>> the more limited possible_crtcs for legacy userspace.) Depends on
>> whether we can (slightly retroactively) declare that mixing atomic
>> and legacy APIs is undefined behavior. Or if really needed, add
>> DRM_CLIENT_CAP_REALLY_REALLY_ATOMIC_NO_LEGACY_PLS. (Or just pass in
>> value 2 for existing DRM_CLIENT_CAP_ATOMIC)
>>
>> drivers/gpu/drm/drm_crtc.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index c215802..1e6d37f 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -707,9 +707,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>> crtc->primary = primary;
>> crtc->cursor = cursor;
>> if (primary)
>> - primary->possible_crtcs = 1 << drm_crtc_index(crtc);
>> + WARN_ON(!(primary->possible_crtcs & (1 << drm_crtc_index(crtc))));
>> if (cursor)
>> - cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
>> + WARN_ON(!(cursor->possible_crtcs & (1 << drm_crtc_index(crtc))));
>
> The slight issue here is that this would require the caller to know the
> upcoming crtc index before its even assigned. So might be better just
> having the caller populate these however it likes after the crtc_init()?
hmm, bleh.. I wanted to make sure we catch drivers that left it as zero..
but how 'bout:
if (cursor && !cursor->possible_crtcs)
cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
BR,
-R
>>
>> if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>> drm_object_attach_property(&crtc->base, config->prop_active, 0);
>> --
>> 2.7.4
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: don't override possible_crtcs for primary/cursor planes
2016-10-20 19:06 [PATCH] drm: don't override possible_crtcs for primary/cursor planes Rob Clark
2016-10-20 19:19 ` Ville Syrjälä
@ 2016-10-20 19:22 ` Sean Paul
1 sibling, 0 replies; 6+ messages in thread
From: Sean Paul @ 2016-10-20 19:22 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel
On Thu, Oct 20, 2016 at 3:06 PM, Rob Clark <robdclark@gmail.com> wrote:
> It is kind of a pointless restriction. If userspace does silly things
> like using crtcA's cursor plane on crtcB, and then setcursor on crtcA,
> it will end up with the overlay disabled on crtcB. But userspace is
> allowed to shoot itself like this.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> Note that cursor and primary planes are slightly useless to non-atomic
> userspace, since userspace has no way to know *which* crtc a primary
> or cursor plane belongs to. So lighting up a 2nd display may or may
> not make your overlay planes go *poof*.
>
> So basically use of cursor/primary planes plus legacy ioctls is an
> undefined behavior if we go with this patch.
>
> *Maybe* we want to restrict this to atomic userspace. (Ie. advertise
> the more limited possible_crtcs for legacy userspace.) Depends on
> whether we can (slightly retroactively) declare that mixing atomic
> and legacy APIs is undefined behavior. Or if really needed, add
> DRM_CLIENT_CAP_REALLY_REALLY_ATOMIC_NO_LEGACY_PLS. (Or just pass in
> value 2 for existing DRM_CLIENT_CAP_ATOMIC)
Being biased and selfish, I'd like to avoid adding another behavior
bit. So I'm fine with this path.
Sean
>
> drivers/gpu/drm/drm_crtc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index c215802..1e6d37f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -707,9 +707,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> crtc->primary = primary;
> crtc->cursor = cursor;
> if (primary)
> - primary->possible_crtcs = 1 << drm_crtc_index(crtc);
> + WARN_ON(!(primary->possible_crtcs & (1 << drm_crtc_index(crtc))));
> if (cursor)
> - cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
> + WARN_ON(!(cursor->possible_crtcs & (1 << drm_crtc_index(crtc))));
>
> if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> drm_object_attach_property(&crtc->base, config->prop_active, 0);
> --
> 2.7.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm: don't override possible_crtcs for primary/cursor planes
@ 2016-11-05 14:52 Rob Clark
2016-11-08 10:19 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Rob Clark @ 2016-11-05 14:52 UTC (permalink / raw)
To: dri-devel
It is kind of a pointless restriction. If userspace does silly things
like using crtcA's cursor plane on crtcB, and then setcursor on crtcA,
it will end up with the overlay disabled on crtcB. But userspace is
allowed to shoot itself like this.
v2: don't WARN_ON() if caller did not set ->possible_crtcs. This keeps
the existing behavior by default, if caller does not set the
->possible_crtcs.
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
drivers/gpu/drm/drm_crtc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 13441e2..ce274ed 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -229,9 +229,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
crtc->primary = primary;
crtc->cursor = cursor;
- if (primary)
+ if (primary && !primary->possible_crtcs)
primary->possible_crtcs = 1 << drm_crtc_index(crtc);
- if (cursor)
+ if (cursor && !cursor->possible_crtcs)
cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
ret = drm_crtc_crc_init(crtc);
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm: don't override possible_crtcs for primary/cursor planes
2016-11-05 14:52 Rob Clark
@ 2016-11-08 10:19 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2016-11-08 10:19 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel
On Sat, Nov 05, 2016 at 10:52:01AM -0400, Rob Clark wrote:
> It is kind of a pointless restriction. If userspace does silly things
> like using crtcA's cursor plane on crtcB, and then setcursor on crtcA,
> it will end up with the overlay disabled on crtcB. But userspace is
> allowed to shoot itself like this.
>
> v2: don't WARN_ON() if caller did not set ->possible_crtcs. This keeps
> the existing behavior by default, if caller does not set the
> ->possible_crtcs.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
Applied to drm-misc, thanks. Should we maybe also update the kerneldoc a
bit in struct drm_plane?
-Daniel
> ---
> drivers/gpu/drm/drm_crtc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 13441e2..ce274ed 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -229,9 +229,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>
> crtc->primary = primary;
> crtc->cursor = cursor;
> - if (primary)
> + if (primary && !primary->possible_crtcs)
> primary->possible_crtcs = 1 << drm_crtc_index(crtc);
> - if (cursor)
> + if (cursor && !cursor->possible_crtcs)
> cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
>
> ret = drm_crtc_crc_init(crtc);
> --
> 2.7.4
>
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-08 10:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-20 19:06 [PATCH] drm: don't override possible_crtcs for primary/cursor planes Rob Clark
2016-10-20 19:19 ` Ville Syrjälä
2016-10-20 20:02 ` Rob Clark
2016-10-20 19:22 ` Sean Paul
-- strict thread matches above, loose matches on Subject: below --
2016-11-05 14:52 Rob Clark
2016-11-08 10:19 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.