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

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

On 8/11/2014 5:53 PM, Daniel Vetter wrote:
> On Mon, Aug 11, 2014 at 05:37:20PM +0530, Jindal, Sonika wrote:
>>
>>
>> On 8/11/2014 5:12 PM, Daniel Vetter wrote:
>>> On Mon, Aug 11, 2014 at 04:37:00PM +0530, Jindal, Sonika wrote:
>>>>
>>>> Hi Daniel,
>>>> Are you taking this patch back in drm-intel?
>>>
>>> We should still call the primary_plane->update hook directly instead of
>>> open-coding it.
>>> -Daniel
>>>
>> But we are already doing dev_priv->display.update_primary_plane. Can you
>> please elaborate? Last time we had a discussion on this same point, and we
>> thought for some platforms more work might be needed but for current
>> platforms this looks ok. Following was the discussion:
>
> That's a different cook used internally in the driver (mostly for
> historical reasons nowadays). I'm talking about the
> drm_plane->fops->update_plane hook, which is implemented for primary
> planes by intel_primary_plane_setplane.
>
Ok, let me add that and post a new patch for this.

-Sonika
> The idea is to have the exact same code flow as for sprite planes, since
> once we have atomic modesets that will be what we need. In the sprite
> set_prop function you pretty directly call the update_plane hook, and I
> think we should do the same here. Actually we might as well directly reuse
> the intel_plane_restore function, it seems to exactly do what we want.
>
>> "> >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.
>>>>
>>> Can you please elaborate on this? What needs to be done?
>>
>> Apparently nothing, it turned out that on the platforms currently supported
>> everything is fine with your patch. Damien promised to follow up with the
>> details on internal mailing lists.
>
> Damien follow up internally. Since we're now allowed to talk about skl
> I'll paste this here:
>
> "On SKL, for 90/270, we'll also need to update the watermarks and remap
> the fb.
>
> "We already duplicate some of the logic: the FBC update, wait for
> pending flips, ...
>
> "So maybe we should try to find a way to take the same path for all
> updates. I'm not too sure how practical this is though."
>
> Damien also said that he doesn't want to still the series on this, but
> since we now have universal plane support for the primary plane I think it
> makes a lot of sense.
> -Daniel
>
>
>> -Daniel
>> "
>>
>>>>
>>>> -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
>>>>>
>>>
>