Re: [PATCH 10/11] drm/i915: Add 180 degree primary plane rotation support

Daniel Vetter
2014-06-19T07:07:08Z

On Thu, Jun 19, 2014 at 12:13:54PM +0530, Jindal, Sonika wrote:
> On 6/18/2014 10:32 PM, Damien Lespiau wrote:
> >On Wed, Jun 18, 2014 at 02:27:26PM +0530, sonika.jindal@intel.com wrote:
> >>From: Sonika Jindal <sonika.jindal@intel.com>

> >>diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >>index 5e583a1..4c91fbc 100644
> >>--- a/drivers/gpu/drm/i915/i915_dma.c
> >>+++ b/drivers/gpu/drm/i915/i915_dma.c
> >>@@ -1939,6 +1939,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
> >> void i915_driver_lastclose(struct drm_device *dev)
> >> {
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >>+ struct intel_crtc *crtc;
> >>+ struct intel_plane *plane;
> >>
> >> /* On gen6+ we refuse to init without kms enabled, but then the drm core
> >> * goes right around and calls lastclose. Check for this and don't clean
> >>@@ -1946,6 +1948,21 @@ void i915_driver_lastclose(struct drm_device *dev)
> >> if (!dev_priv)
> >> return;
> >>
> >>+ if (dev_priv->rotation_property) {
> >>+ list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> >>+ to_intel_plane(crtc->base.primary)->rotation = BIT(DRM_ROTATE_0);
> >>+ drm_object_property_set_value(&crtc->base.base,
> >>+ dev_priv->rotation_property,
> >>+ to_intel_plane(crtc->base.primary)->rotation);
> >>+ }
> >>+ list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> >>+ plane->rotation = BIT(DRM_ROTATE_0);
> >>+ drm_object_property_set_value(&plane->base.base,
> >>+ dev_priv->rotation_property,
> >>+ plane->rotation);
> >>+ }
> >>+ }
> >>+
> >
> >The purpose of this seems to be restoring rotation to 0 and rely on the next
> >modeset to re-program the register. This code, is orthogonal to the pure
> >primary plane rotation enabling, spans through both sprite and primary planes
> >and may actually be a bit tricky to get right.
> >
> >-> This shouldn't be part of the same commit as the primary plane rotation.
> >Please span a new commit for it.
> Sure I will add another patch.
> >
> >Now, we also need something like this when switching VT, and probably for
> >kernel panic and debug handling as well, so lastclose() is not enough (and we
> >can probably do better).
> >
> >One idea would be for the core DRM code to know about this property, and make
> >sure we put the rotation back to 0 in restore_fbdev_mode(), we do something
> >similar for the for the sprite planes in there already. Another idea would be
> >to add a vfunc to execute driver specific code there in restore_fbdev_mode().
> >
> >There is probably a better way to do it, I have to say I'm not super familiar
> >with this part of the driver.
> >
> I see that in omap driver too it is done in lastclose of the driver. Also,
> from driver fbdev_restore is only called during lastclose.
> Again I don't have more knowledge on this.
> Can we keep it here in this lastclose function to comply with omap driver?

No, this really should be done in
drm_fb_helper_restore_fbdev_mode_unlocked in drm_fb_helper.c. Well, in the
restore_fbdev_mode function in there. Once that's done and once omap is
also using the generic rotation properties (I think it is already) we can
remove the rotation handling code from omap's last_close. Please also
throw a (compile-tested-only) patch on top for that so that Rob Clark can
pick it up.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch