* [PATCH] drm/atomic: Don't reject reflect-only rotations
@ 2016-12-07 12:18 Brian Starkey
2016-12-07 13:20 ` Gustavo Padovan
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Brian Starkey @ 2016-12-07 12:18 UTC (permalink / raw)
To: dri-devel, ville.syrjala; +Cc: liviu.dudau
The check to reject combinations of multiple rotation angles is overly
restrictive and has the side-effect of also failing any rotation value
which consists only of reflections.
Fix this by relaxing the check to ignore values which contain no
rotation flags.
Fixes: 6e0c7c3358d4 ("drm/atomic: Reject attempts to use multiple rotation angles at once")
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
drivers/gpu/drm/drm_atomic.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 362e3ea..44f4030 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -846,7 +846,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
} else if (property == config->prop_src_h) {
state->src_h = val;
} else if (property == plane->rotation_property) {
- if (!is_power_of_2(val & DRM_ROTATE_MASK))
+ if ((val & DRM_ROTATE_MASK) &&
+ !is_power_of_2(val & DRM_ROTATE_MASK))
return -EINVAL;
state->rotation = val;
} else if (property == plane->zpos_property) {
--
1.7.9.5
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] drm/atomic: Don't reject reflect-only rotations 2016-12-07 12:18 [PATCH] drm/atomic: Don't reject reflect-only rotations Brian Starkey @ 2016-12-07 13:20 ` Gustavo Padovan 2016-12-07 13:32 ` Chris Wilson 2016-12-07 14:12 ` Ville Syrjälä 2 siblings, 0 replies; 9+ messages in thread From: Gustavo Padovan @ 2016-12-07 13:20 UTC (permalink / raw) To: Brian Starkey; +Cc: liviu.dudau, dri-devel Hi Brian, 2016-12-07 Brian Starkey <brian.starkey@arm.com>: > The check to reject combinations of multiple rotation angles is overly > restrictive and has the side-effect of also failing any rotation value > which consists only of reflections. > > Fix this by relaxing the check to ignore values which contain no > rotation flags. > > Fixes: 6e0c7c3358d4 ("drm/atomic: Reject attempts to use multiple rotation angles at once") > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > --- > drivers/gpu/drm/drm_atomic.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Gustavo _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/atomic: Don't reject reflect-only rotations 2016-12-07 12:18 [PATCH] drm/atomic: Don't reject reflect-only rotations Brian Starkey 2016-12-07 13:20 ` Gustavo Padovan @ 2016-12-07 13:32 ` Chris Wilson 2016-12-07 14:52 ` Daniel Vetter 2016-12-07 14:12 ` Ville Syrjälä 2 siblings, 1 reply; 9+ messages in thread From: Chris Wilson @ 2016-12-07 13:32 UTC (permalink / raw) To: Brian Starkey; +Cc: liviu.dudau, dri-devel On Wed, Dec 07, 2016 at 12:18:19PM +0000, Brian Starkey wrote: > The check to reject combinations of multiple rotation angles is overly > restrictive and has the side-effect of also failing any rotation value > which consists only of reflections. > > Fix this by relaxing the check to ignore values which contain no > rotation flags. > > Fixes: 6e0c7c3358d4 ("drm/atomic: Reject attempts to use multiple rotation angles at once") > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > drivers/gpu/drm/drm_atomic.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 362e3ea..44f4030 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -846,7 +846,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > } else if (property == config->prop_src_h) { > state->src_h = val; > } else if (property == plane->rotation_property) { > - if (!is_power_of_2(val & DRM_ROTATE_MASK)) > + if ((val & DRM_ROTATE_MASK) && If val & DRM_ROTATE_MASK is zero, val must be REFLECT_X | REFLECT_Y. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/atomic: Don't reject reflect-only rotations 2016-12-07 13:32 ` Chris Wilson @ 2016-12-07 14:52 ` Daniel Vetter 2016-12-07 15:13 ` Ville Syrjälä 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2016-12-07 14:52 UTC (permalink / raw) To: Chris Wilson, Brian Starkey, dri-devel, ville.syrjala, liviu.dudau On Wed, Dec 07, 2016 at 01:32:57PM +0000, Chris Wilson wrote: > On Wed, Dec 07, 2016 at 12:18:19PM +0000, Brian Starkey wrote: > > The check to reject combinations of multiple rotation angles is overly > > restrictive and has the side-effect of also failing any rotation value > > which consists only of reflections. > > > > Fix this by relaxing the check to ignore values which contain no > > rotation flags. > > > > Fixes: 6e0c7c3358d4 ("drm/atomic: Reject attempts to use multiple rotation angles at once") > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > > drivers/gpu/drm/drm_atomic.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 362e3ea..44f4030 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -846,7 +846,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > } else if (property == config->prop_src_h) { > > state->src_h = val; > > } else if (property == plane->rotation_property) { > > - if (!is_power_of_2(val & DRM_ROTATE_MASK)) > > + if ((val & DRM_ROTATE_MASK) && > > If val & DRM_ROTATE_MASK is zero, val must be REFLECT_X | REFLECT_Y. While we bikeshed this check: Validation like this should be somewhere behind ->atomic_check, since if it's only here then you can sneak invalid stuff in through the legacy/compat set_property ioctls. -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/atomic: Don't reject reflect-only rotations 2016-12-07 14:52 ` Daniel Vetter @ 2016-12-07 15:13 ` Ville Syrjälä 2016-12-07 15:54 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2016-12-07 15:13 UTC (permalink / raw) To: Daniel Vetter; +Cc: liviu.dudau, dri-devel On Wed, Dec 07, 2016 at 03:52:29PM +0100, Daniel Vetter wrote: > On Wed, Dec 07, 2016 at 01:32:57PM +0000, Chris Wilson wrote: > > On Wed, Dec 07, 2016 at 12:18:19PM +0000, Brian Starkey wrote: > > > The check to reject combinations of multiple rotation angles is overly > > > restrictive and has the side-effect of also failing any rotation value > > > which consists only of reflections. > > > > > > Fix this by relaxing the check to ignore values which contain no > > > rotation flags. > > > > > > Fixes: 6e0c7c3358d4 ("drm/atomic: Reject attempts to use multiple rotation angles at once") > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > > > > drivers/gpu/drm/drm_atomic.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index 362e3ea..44f4030 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -846,7 +846,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > > } else if (property == config->prop_src_h) { > > > state->src_h = val; > > > } else if (property == plane->rotation_property) { > > > - if (!is_power_of_2(val & DRM_ROTATE_MASK)) > > > + if ((val & DRM_ROTATE_MASK) && > > > > If val & DRM_ROTATE_MASK is zero, val must be REFLECT_X | REFLECT_Y. > > While we bikeshed this check: Validation like this should be somewhere > behind ->atomic_check, since if it's only here then you can sneak invalid > stuff in through the legacy/compat set_property ioctls. Not if you use drm_atomic_helper_plane_set_property(). -- 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] 9+ messages in thread
* Re: [PATCH] drm/atomic: Don't reject reflect-only rotations 2016-12-07 15:13 ` Ville Syrjälä @ 2016-12-07 15:54 ` Daniel Vetter 2016-12-07 16:02 ` Ville Syrjälä 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2016-12-07 15:54 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel, liviu.dudau On Wed, Dec 07, 2016 at 05:13:24PM +0200, Ville Syrjälä wrote: > On Wed, Dec 07, 2016 at 03:52:29PM +0100, Daniel Vetter wrote: > > On Wed, Dec 07, 2016 at 01:32:57PM +0000, Chris Wilson wrote: > > > On Wed, Dec 07, 2016 at 12:18:19PM +0000, Brian Starkey wrote: > > > > The check to reject combinations of multiple rotation angles is overly > > > > restrictive and has the side-effect of also failing any rotation value > > > > which consists only of reflections. > > > > > > > > Fix this by relaxing the check to ignore values which contain no > > > > rotation flags. > > > > > > > > Fixes: 6e0c7c3358d4 ("drm/atomic: Reject attempts to use multiple rotation angles at once") > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > > > > > > drivers/gpu/drm/drm_atomic.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > > index 362e3ea..44f4030 100644 > > > > --- a/drivers/gpu/drm/drm_atomic.c > > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > > @@ -846,7 +846,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > > > } else if (property == config->prop_src_h) { > > > > state->src_h = val; > > > > } else if (property == plane->rotation_property) { > > > > - if (!is_power_of_2(val & DRM_ROTATE_MASK)) > > > > + if ((val & DRM_ROTATE_MASK) && > > > > > > If val & DRM_ROTATE_MASK is zero, val must be REFLECT_X | REFLECT_Y. > > > > While we bikeshed this check: Validation like this should be somewhere > > behind ->atomic_check, since if it's only here then you can sneak invalid > > stuff in through the legacy/compat set_property ioctls. > > Not if you use drm_atomic_helper_plane_set_property(). Oh right, I mixed up the callchain maze in my head again. I still feel like putting all the validation code in one place might be a good idea. But yeah not strictly needed. -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/atomic: Don't reject reflect-only rotations 2016-12-07 15:54 ` Daniel Vetter @ 2016-12-07 16:02 ` Ville Syrjälä 0 siblings, 0 replies; 9+ messages in thread From: Ville Syrjälä @ 2016-12-07 16:02 UTC (permalink / raw) To: Daniel Vetter; +Cc: liviu.dudau, dri-devel On Wed, Dec 07, 2016 at 04:54:40PM +0100, Daniel Vetter wrote: > On Wed, Dec 07, 2016 at 05:13:24PM +0200, Ville Syrjälä wrote: > > On Wed, Dec 07, 2016 at 03:52:29PM +0100, Daniel Vetter wrote: > > > On Wed, Dec 07, 2016 at 01:32:57PM +0000, Chris Wilson wrote: > > > > On Wed, Dec 07, 2016 at 12:18:19PM +0000, Brian Starkey wrote: > > > > > The check to reject combinations of multiple rotation angles is overly > > > > > restrictive and has the side-effect of also failing any rotation value > > > > > which consists only of reflections. > > > > > > > > > > Fix this by relaxing the check to ignore values which contain no > > > > > rotation flags. > > > > > > > > > > Fixes: 6e0c7c3358d4 ("drm/atomic: Reject attempts to use multiple rotation angles at once") > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > > > > > > > > drivers/gpu/drm/drm_atomic.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > > > index 362e3ea..44f4030 100644 > > > > > --- a/drivers/gpu/drm/drm_atomic.c > > > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > > > @@ -846,7 +846,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > > > > } else if (property == config->prop_src_h) { > > > > > state->src_h = val; > > > > > } else if (property == plane->rotation_property) { > > > > > - if (!is_power_of_2(val & DRM_ROTATE_MASK)) > > > > > + if ((val & DRM_ROTATE_MASK) && > > > > > > > > If val & DRM_ROTATE_MASK is zero, val must be REFLECT_X | REFLECT_Y. > > > > > > While we bikeshed this check: Validation like this should be somewhere > > > behind ->atomic_check, since if it's only here then you can sneak invalid > > > stuff in through the legacy/compat set_property ioctls. > > > > Not if you use drm_atomic_helper_plane_set_property(). > > Oh right, I mixed up the callchain maze in my head again. I still feel > like putting all the validation code in one place might be a good idea. > But yeah not strictly needed. Well, we do quite a bit of checking already earlier. Eg. drm_property_change_valid(), and obj ID lookups. I think this sort of "totally invalid use of the API" check fits in pretty well with that crowd. -- 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] 9+ messages in thread
* Re: [PATCH] drm/atomic: Don't reject reflect-only rotations 2016-12-07 12:18 [PATCH] drm/atomic: Don't reject reflect-only rotations Brian Starkey 2016-12-07 13:20 ` Gustavo Padovan 2016-12-07 13:32 ` Chris Wilson @ 2016-12-07 14:12 ` Ville Syrjälä 2016-12-07 14:48 ` Brian Starkey 2 siblings, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2016-12-07 14:12 UTC (permalink / raw) To: Brian Starkey; +Cc: liviu.dudau, dri-devel On Wed, Dec 07, 2016 at 12:18:19PM +0000, Brian Starkey wrote: > The check to reject combinations of multiple rotation angles is overly > restrictive and has the side-effect of also failing any rotation value > which consists only of reflections. > > Fix this by relaxing the check to ignore values which contain no > rotation flags. > > Fixes: 6e0c7c3358d4 ("drm/atomic: Reject attempts to use multiple rotation angles at once") > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > --- > drivers/gpu/drm/drm_atomic.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 362e3ea..44f4030 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -846,7 +846,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > } else if (property == config->prop_src_h) { > state->src_h = val; > } else if (property == plane->rotation_property) { > - if (!is_power_of_2(val & DRM_ROTATE_MASK)) > + if ((val & DRM_ROTATE_MASK) && > + !is_power_of_2(val & DRM_ROTATE_MASK)) NAK. You must always set one and only one rotation bit. > return -EINVAL; > state->rotation = val; > } else if (property == plane->zpos_property) { > -- > 1.7.9.5 -- 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] 9+ messages in thread
* Re: [PATCH] drm/atomic: Don't reject reflect-only rotations 2016-12-07 14:12 ` Ville Syrjälä @ 2016-12-07 14:48 ` Brian Starkey 0 siblings, 0 replies; 9+ messages in thread From: Brian Starkey @ 2016-12-07 14:48 UTC (permalink / raw) To: Ville Syrjälä; +Cc: liviu.dudau, dri-devel On Wed, Dec 07, 2016 at 04:12:07PM +0200, Ville Syrjälä wrote: >On Wed, Dec 07, 2016 at 12:18:19PM +0000, Brian Starkey wrote: >> The check to reject combinations of multiple rotation angles is overly >> restrictive and has the side-effect of also failing any rotation value >> which consists only of reflections. >> >> Fix this by relaxing the check to ignore values which contain no >> rotation flags. >> >> Fixes: 6e0c7c3358d4 ("drm/atomic: Reject attempts to use multiple rotation angles at once") >> Signed-off-by: Brian Starkey <brian.starkey@arm.com> >> --- >> drivers/gpu/drm/drm_atomic.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 362e3ea..44f4030 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -846,7 +846,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, >> } else if (property == config->prop_src_h) { >> state->src_h = val; >> } else if (property == plane->rotation_property) { >> - if (!is_power_of_2(val & DRM_ROTATE_MASK)) >> + if ((val & DRM_ROTATE_MASK) && >> + !is_power_of_2(val & DRM_ROTATE_MASK)) > >NAK. You must always set one and only one rotation bit. > How do you do (DRM_ROTATE_90 | DRM_REFLECT_X) then? Could you suggest the correct fix? Because right now a commit which sets rotation = "reflect-x" fails on this check Thanks, Brian >> return -EINVAL; >> state->rotation = val; >> } else if (property == plane->zpos_property) { >> -- >> 1.7.9.5 > >-- >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] 9+ messages in thread
end of thread, other threads:[~2016-12-07 16:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-07 12:18 [PATCH] drm/atomic: Don't reject reflect-only rotations Brian Starkey 2016-12-07 13:20 ` Gustavo Padovan 2016-12-07 13:32 ` Chris Wilson 2016-12-07 14:52 ` Daniel Vetter 2016-12-07 15:13 ` Ville Syrjälä 2016-12-07 15:54 ` Daniel Vetter 2016-12-07 16:02 ` Ville Syrjälä 2016-12-07 14:12 ` Ville Syrjälä 2016-12-07 14:48 ` Brian Starkey
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.