| Message ID | 1403081847-4364-10-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 18, 2014 at 02:27:25PM +0530, sonika.jindal@intel.com wrote: > From: Ville Syrjälä <ville.syrjala at linux.intel.com> > > Sprite planes support 180 degree rotation. The lower layers are now in > place, so hook in the standard rotation property to expose the feature > to the users. > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > Cc: Jani Nikula <jani.nikula at linux.intel.com> > Cc: David Airlie <airlied at linux.ie> > Cc: dri-devel at lists.freedesktop.org > Cc: linux-kernel at vger.kernel.org > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_sprite.c | 42 ++++++++++++++++++++++++++++++++++- > 2 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 0640071..b56a1a5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1514,6 +1514,7 @@ struct drm_i915_private { > > struct drm_property *broadcast_rgb_property; > struct drm_property *force_audio_property; > + struct drm_property *rotation_property; > > uint32_t hw_context_size; > struct list_head context_list; > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index cbad738..b9af256 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1202,6 +1202,30 @@ out_unlock: > return ret; > } > > +static int intel_plane_set_property(struct drm_plane *plane, > + struct drm_property *prop, > + uint64_t val) > +{ > + struct drm_i915_private *dev_priv = plane->dev->dev_private; > + struct intel_plane *intel_plane = to_intel_plane(plane); > + uint64_t old_val; > + int ret = -ENOENT; > + > + if (prop == dev_priv->rotation_property) { Shouldn't we add a: if (val & (BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_180))) return -EINVAL; To ensure userspace doesn't send garbage in the upper bits so we can reuse them down the road?
On 6/18/2014 4:42 PM, Damien Lespiau wrote: > On Wed, Jun 18, 2014 at 02:27:25PM +0530, sonika.jindal@intel.com wrote: >> From: Ville Syrjälä <ville.syrjala at linux.intel.com> >> >> Sprite planes support 180 degree rotation. The lower layers are now in >> place, so hook in the standard rotation property to expose the feature >> to the users. >> >> Cc: Daniel Vetter <daniel.vetter at ffwll.ch> >> Cc: Jani Nikula <jani.nikula at linux.intel.com> >> Cc: David Airlie <airlied at linux.ie> >> Cc: dri-devel at lists.freedesktop.org >> Cc: linux-kernel at vger.kernel.org >> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_sprite.c | 42 ++++++++++++++++++++++++++++++++++- >> 2 files changed, 42 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 0640071..b56a1a5 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1514,6 +1514,7 @@ struct drm_i915_private { >> >> struct drm_property *broadcast_rgb_property; >> struct drm_property *force_audio_property; >> + struct drm_property *rotation_property; >> >> uint32_t hw_context_size; >> struct list_head context_list; >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index cbad738..b9af256 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -1202,6 +1202,30 @@ out_unlock: >> return ret; >> } >> >> +static int intel_plane_set_property(struct drm_plane *plane, >> + struct drm_property *prop, >> + uint64_t val) >> +{ >> + struct drm_i915_private *dev_priv = plane->dev->dev_private; >> + struct intel_plane *intel_plane = to_intel_plane(plane); >> + uint64_t old_val; >> + int ret = -ENOENT; >> + >> + if (prop == dev_priv->rotation_property) { > > Shouldn't we add a: > > if (val & (BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_180))) > return -EINVAL; > > To ensure userspace doesn't send garbage in the upper bits so we can > reuse them down the road? > But we are checking if more than one bit is set, we return EINVAL. So we only care for one rotation angle being sent from user. Shouldn't that suffice?
On Wed, Jun 18, 2014 at 12:12:52PM +0100, Damien Lespiau wrote: > On Wed, Jun 18, 2014 at 02:27:25PM +0530, sonika.jindal@intel.com wrote: > > From: Ville Syrjälä <ville.syrjala at linux.intel.com> > > > > Sprite planes support 180 degree rotation. The lower layers are now in > > place, so hook in the standard rotation property to expose the feature > > to the users. > > > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > > Cc: Jani Nikula <jani.nikula at linux.intel.com> > > Cc: David Airlie <airlied at linux.ie> > > Cc: dri-devel at lists.freedesktop.org > > Cc: linux-kernel at vger.kernel.org > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_sprite.c | 42 ++++++++++++++++++++++++++++++++++- > > 2 files changed, 42 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 0640071..b56a1a5 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1514,6 +1514,7 @@ struct drm_i915_private { > > > > struct drm_property *broadcast_rgb_property; > > struct drm_property *force_audio_property; > > + struct drm_property *rotation_property; > > > > uint32_t hw_context_size; > > struct list_head context_list; > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index cbad738..b9af256 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -1202,6 +1202,30 @@ out_unlock: > > return ret; > > } > > > > +static int intel_plane_set_property(struct drm_plane *plane, > > + struct drm_property *prop, > > + uint64_t val) > > +{ > > + struct drm_i915_private *dev_priv = plane->dev->dev_private; > > + struct intel_plane *intel_plane = to_intel_plane(plane); > > + uint64_t old_val; > > + int ret = -ENOENT; > > + > > + if (prop == dev_priv->rotation_property) { > > Shouldn't we add a: > > if (val & (BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_180))) > return -EINVAL; > > To ensure userspace doesn't send garbage in the upper bits so we can > reuse them down the road? drm_property_change_is_valid() should already check that no unsupported bits can escape into the driver.
On Wed, Jun 18, 2014 at 05:24:56PM +0530, Jindal, Sonika wrote: > >Shouldn't we add a: > > > > if (val & (BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_180))) > > return -EINVAL; > > > >To ensure userspace doesn't send garbage in the upper bits so we can > >reuse them down the road? > > > But we are checking if more than one bit is set, we return EINVAL. > So we only care for one rotation angle being sent from user. > Shouldn't that suffice? Nop. If given (1 << 50) we'd still pass the test but with an invalid (reserved) value. I didn't spot the generic drm_property_change_is_valid() (Ville's answer), so it should be handled for us by the DRM core already. All is fine.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0640071..b56a1a5 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1514,6 +1514,7 @@ struct drm_i915_private { struct drm_property *broadcast_rgb_property; struct drm_property *force_audio_property; + struct drm_property *rotation_property; uint32_t hw_context_size; struct list_head context_list; diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index cbad738..b9af256 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1202,6 +1202,30 @@ out_unlock: return ret; } +static int intel_plane_set_property(struct drm_plane *plane, + struct drm_property *prop, + uint64_t val) +{ + struct drm_i915_private *dev_priv = plane->dev->dev_private; + struct intel_plane *intel_plane = to_intel_plane(plane); + uint64_t old_val; + int ret = -ENOENT; + + if (prop == dev_priv->rotation_property) { + /* exactly one rotation angle please */ + if (hweight32(val & 0xf) != 1) + return -EINVAL; + + old_val = intel_plane->rotation; + intel_plane->rotation = val; + ret = intel_plane_restore(plane); + if (ret) + intel_plane->rotation = old_val; + } + + return ret; +} + int intel_plane_restore(struct drm_plane *plane) { struct intel_plane *intel_plane = to_intel_plane(plane); @@ -1228,6 +1252,7 @@ static const struct drm_plane_funcs intel_plane_funcs = { .update_plane = intel_update_plane, .disable_plane = intel_disable_plane, .destroy = intel_destroy_plane, + .set_property = intel_plane_set_property, }; static uint32_t ilk_plane_formats[] = { @@ -1264,6 +1289,7 @@ static uint32_t vlv_plane_formats[] = { int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) { + struct drm_i915_private *dev_priv = dev->dev_private; struct intel_plane *intel_plane; unsigned long possible_crtcs; const uint32_t *plane_formats; @@ -1338,8 +1364,22 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) &intel_plane_funcs, plane_formats, num_plane_formats, false); - if (ret) + if (ret) { kfree(intel_plane); + goto out; + } + + if (!dev_priv->rotation_property) + dev_priv->rotation_property = + drm_mode_create_rotation_property(dev, + BIT(DRM_ROTATE_0) | + BIT(DRM_ROTATE_180)); + + if (dev_priv->rotation_property) + drm_object_attach_property(&intel_plane->base.base, + dev_priv->rotation_property, + intel_plane->rotation); + out: return ret; }