| Message ID | 1404795723-9630-10-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 08, 2014 at 10:31:59AM +0530, sonika.jindal@intel.com wrote: > From: Ville Syrjälä <ville.syrjala@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. > > v2: Moving rotation_property to drm_plane This change is wrong and I haven't figured out who suggested it. The actual property is global (i.e. we should only have 1), but then it can be attached to different pieces. So here's what we need to do instead: - Add the rotation property to dev->mode_config. - Convert omapdrm to also store it there by moving it out of the omap driver private structure. - Probably better to just unconditionally set it up, but not sure. - Adjust the fb helper code accordingly. - Rip the omap restore rotation code in lastclose since the fb helper will now take care of this. I've merged the drm core parts for now but dropped the i915 implementation and the fb helper patch again. Thanks, Daniel > > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 40 ++++++++++++++++++++++++++++++++++- > include/drm/drm_crtc.h | 1 + > 2 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index cbad738..aa63027 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1202,6 +1202,29 @@ out_unlock: > return ret; > } > > +static int intel_plane_set_property(struct drm_plane *plane, > + struct drm_property *prop, > + uint64_t val) > +{ > + struct intel_plane *intel_plane = to_intel_plane(plane); > + uint64_t old_val; > + int ret = -ENOENT; > + > + if (prop == plane->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 +1251,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[] = { > @@ -1338,8 +1362,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 (!intel_plane->base.rotation_property) > + intel_plane->base.rotation_property = > + drm_mode_create_rotation_property(dev, > + BIT(DRM_ROTATE_0) | > + BIT(DRM_ROTATE_180)); > + > + if (intel_plane->base.rotation_property) > + drm_object_attach_property(&intel_plane->base.base, > + intel_plane->base.rotation_property, > + intel_plane->rotation); > > + out: > return ret; > } > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 08ed55e..6006c70 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -615,6 +615,7 @@ struct drm_plane { > > const struct drm_plane_funcs *funcs; > > + struct drm_property *rotation_property; > struct drm_object_properties properties; > > enum drm_plane_type type; > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jul 08, 2014 at 10:31:59AM +0530, sonika.jindal@intel.com wrote: > From: Ville Syrjälä <ville.syrjala@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. > > v2: Moving rotation_property to drm_plane > > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > Reviewed-by: Imre Deak <imre.deak@intel.com> Also this r-b tag was for v1 (which was ok), not for v2. If you carry over such a review-by tag and make functional changes not discussed with the reviewer you _must_ at least mark the r-b with a (v1) or if it's a big change, drop the tag completely. -Daniel
On Tue, Jul 08, 2014 at 10:31:59AM +0530, sonika.jindal@intel.com wrote: > From: Ville Syrjälä <ville.syrjala@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. > > v2: Moving rotation_property to drm_plane > > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 40 ++++++++++++++++++++++++++++++++++- > include/drm/drm_crtc.h | 1 + One more: A patch titled with "drm/i915: ..." really shouldn't touch anything outside of drm/i915 directories and so shouldn't introduce any changes to core drm code. Such changes always need to be split out into a separate drm patch. Exceptions (like refactoring function interfaces) obviously apply. -Daniel
On 7/12/2014 3:29 AM, Daniel Vetter wrote: > On Tue, Jul 08, 2014 at 10:31:59AM +0530, sonika.jindal@intel.com wrote: >> From: Ville Syrjälä <ville.syrjala@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. >> >> v2: Moving rotation_property to drm_plane > > This change is wrong and I haven't figured out who suggested it. The > actual property is global (i.e. we should only have 1), but then it can be > attached to different pieces. > > So here's what we need to do instead: > - Add the rotation property to dev->mode_config. > - Convert omapdrm to also store it there by moving it out of the omap > driver private structure. > - Probably better to just unconditionally set it up, but not sure. > - Adjust the fb helper code accordingly. > - Rip the omap restore rotation code in lastclose since the fb helper will > now take care of this. > Ok, I will make this property a part of mode_config and will attach that to drm_plane, and will post the remaining patches. -Sonika > I've merged the drm core parts for now but dropped the i915 implementation > and the fb helper patch again. > > Thanks, Daniel > >> >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> Reviewed-by: Imre Deak <imre.deak@intel.com> >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 40 ++++++++++++++++++++++++++++++++++- >> include/drm/drm_crtc.h | 1 + >> 2 files changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index cbad738..aa63027 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -1202,6 +1202,29 @@ out_unlock: >> return ret; >> } >> >> +static int intel_plane_set_property(struct drm_plane *plane, >> + struct drm_property *prop, >> + uint64_t val) >> +{ >> + struct intel_plane *intel_plane = to_intel_plane(plane); >> + uint64_t old_val; >> + int ret = -ENOENT; >> + >> + if (prop == plane->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 +1251,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[] = { >> @@ -1338,8 +1362,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 (!intel_plane->base.rotation_property) >> + intel_plane->base.rotation_property = >> + drm_mode_create_rotation_property(dev, >> + BIT(DRM_ROTATE_0) | >> + BIT(DRM_ROTATE_180)); >> + >> + if (intel_plane->base.rotation_property) >> + drm_object_attach_property(&intel_plane->base.base, >> + intel_plane->base.rotation_property, >> + intel_plane->rotation); >> >> + out: >> return ret; >> } >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index 08ed55e..6006c70 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -615,6 +615,7 @@ struct drm_plane { >> >> const struct drm_plane_funcs *funcs; >> >> + struct drm_property *rotation_property; >> struct drm_object_properties properties; >> >> enum drm_plane_type type; >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index cbad738..aa63027 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1202,6 +1202,29 @@ out_unlock: return ret; } +static int intel_plane_set_property(struct drm_plane *plane, + struct drm_property *prop, + uint64_t val) +{ + struct intel_plane *intel_plane = to_intel_plane(plane); + uint64_t old_val; + int ret = -ENOENT; + + if (prop == plane->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 +1251,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[] = { @@ -1338,8 +1362,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 (!intel_plane->base.rotation_property) + intel_plane->base.rotation_property = + drm_mode_create_rotation_property(dev, + BIT(DRM_ROTATE_0) | + BIT(DRM_ROTATE_180)); + + if (intel_plane->base.rotation_property) + drm_object_attach_property(&intel_plane->base.base, + intel_plane->base.rotation_property, + intel_plane->rotation); + out: return ret; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 08ed55e..6006c70 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -615,6 +615,7 @@ struct drm_plane { const struct drm_plane_funcs *funcs; + struct drm_property *rotation_property; struct drm_object_properties properties; enum drm_plane_type type;