Re: [PATCH 5/6] drm/i915: Add 180 degree primary plane rotation support

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

Hi Daniel,
Are you taking this patch back in drm-intel?

-Sonika

On 8/7/2014 5:41 PM, Daniel Vetter wrote:
> On Thu, Aug 07, 2014 at 01:45:31PM +0200, Daniel Vetter wrote:
>> On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote:
>>> On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote:
>>>> +		/* FBC does not work on some platforms for rotated planes */
>>>> +			if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) {
>>>> +				if (dev_priv->fbc.plane == intel_crtc->plane &&
>>>> +				intel_plane->rotation != BIT(DRM_ROTATE_0))
>>>> +					intel_disable_fbc(dev);
>>>> +			/* If rotation was set earlier and new rotation is 0,
>>>> +			we might have disabled fbc earlier. So update it now */
>>>> +				else if (intel_plane->rotation == BIT(DRM_ROTATE_0)
>>>> +					&& old_val != BIT(DRM_ROTATE_0)) {
>>>> +					mutex_lock(&dev->struct_mutex);
>>>> +					intel_update_fbc(dev);
>>>> +					mutex_unlock(&dev->struct_mutex);
>>>> +				}
>>>> +			}
>>>
>>> Indentation is screwed up here. Also if we convert some of the checks into
>>> early bails we could de-indent this by one level.
>>>
>>> Also Chris mentioned that on some platforms this won't work and it's more
>>> future-proof to just do a full modeset until we have the proper
>>> infrastructure.
>>
>> Apparently this review here was never addressed, as Chris just pointed out
>> on irc. I've dropped the patch again.
>>
>> I think we need:
>> - The same sequence as with the sprite set_property function, i.e. we need
>>    to call the update_plane function (not the raw low-level one, the
>>    high-level with all the checks).
>> - The fbc check is wrong and will miss updates when the crtc is off. We
>>    need to move this into the general list of checks in intel_update_fbc.
>> - Since this seems to be buggy I want added testcases to combine fbc
>>    correctness with screen rotation. Probably best to reuse the existing
>>    fbc testcase and add a bunch or rotated tests.
>
> Ok, the check in update_fbc is there, I've been blind. Sorry about all the
> confusion. So just amounts of calling the higher level function and we can
> forgo the fbc testing.
> -Daniel
>