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

["Jindal, Sonika" <sonika.jindal@intel.com>]

On 6/19/2014 12:37 PM, Daniel Vetter wrote:
> 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
>
Ok, I will add it. So should I add a function pointer say 
reset_properties in crtc, which will be called from restore_fbdev_mode?