| Message ID | 1481113099-4788-1-git-send-email-brian.starkey@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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().
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
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.
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) {
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(-)