[Intel-gfx] [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Tvrtko Ursulin]

On 08/10/15 09:58, Tvrtko Ursulin wrote:
> On 07/10/15 15:19, Daniel Vetter wrote:
>> On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote:
>>> On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote:
>>>> On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote:
>>>>> On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote:
>>>>>> On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote:
>>>>>>>
>>>>>>> On 10/04/15 10:07, Sonika Jindal wrote:
>>>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal at intel.com>
>>>>>>>> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
>>>>>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> index ceb2e61..f0bbc22 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct
>>>>>>>> drm_plane *plane,
>>>>>>>>       struct drm_rect *dest = &state->dst;
>>>>>>>>       struct drm_rect *src = &state->src;
>>>>>>>>       const struct drm_rect *clip = &state->clip;
>>>>>>>> +    bool can_position = false;
>>>>>>>>       int ret;
>>>>>>>>
>>>>>>>>       crtc = crtc ? crtc : plane->crtc;
>>>>>>>>       intel_crtc = to_intel_crtc(crtc);
>>>>>>>>
>>>>>>>> +    if (INTEL_INFO(dev)->gen >= 9)
>>>>>>>> +        can_position = true;
>>>>>>>> +
>>>>>>>>       ret = drm_plane_helper_check_update(plane, crtc, fb,
>>>>>>>>                           src, dest, clip,
>>>>>>>>                           DRM_PLANE_HELPER_NO_SCALING,
>>>>>>>>                           DRM_PLANE_HELPER_NO_SCALING,
>>>>>>>> -                        false, true, &state->visible);
>>>>>>>> +                        can_position, true,
>>>>>>>> +                        &state->visible);
>>>>>>>>       if (ret)
>>>>>>>>           return ret;
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> I have discovered today that, while this allows SetCrtc and SetPlane
>>>>>>> ioctls to work with frame buffers which do not cover the plane, page
>>>>>>> flips are not that lucky and fail roughly with:
>>>>>>>
>>>>>>> [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC
>>>>>>> viewport 1920x1080+0+0.
>>>>>>
>>>>>> Maybe I'm misunderstanding your explanation, but a framebuffer is
>>>>>> always
>>>>>> required to fill/cover the plane scanning out of it.  What this
>>>>>> patch is
>>>>>> supposed to be allowing is for the primary plane to not cover the
>>>>>> entire
>>>>>> CRTC (since that's something that only became possible for Intel
>>>>>> hardware on the gen9+ platforms).  I.e., the primary plane is now
>>>>>> allowed to positioned and resized to cover a subset of the CRTC area,
>>>>>> just like "sprite" planes have always been able to.
>>>>>>
>>>>>> If you've got a 1080x1080 framebuffer, then it's legal to have a
>>>>>> 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT.
>>>>>> However it is not legal to size the primary plane as 1920x1080 and
>>>>>> use
>>>>>> this same 1080x1080 framebuffer with any of our interfaces (setplane,
>>>>>> setcrtc, pageflip, or atomic).
>>>>>>
>>>>>> Are you using ioctls/libdrm directly or are you using igt_kms
>>>>>> helpers?
>>>>>> IIRC, the IGT helpers will try to be extra helpful and automatically
>>>>>> size the plane to match the framebuffer (unless you override that
>>>>>> behavior), so that might be what's causing the confusion here.
>>>>>
>>>>> The problem is clear as day in drm_mode_page_flip_ioctl():
>>>>> ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode,
>>>>> fb);
>>>>> if (ret)
>>>>>     goto out;
>>>>>
>>>>> The fix should be easy; just extract the current src coordinates from
>>>>> the plane state and check those against the new fb size. And then hope
>>>>> that the plane state is really up to date.
>>>>
>>>> Yep, that's the conclusion we came to once Tvrtko explained what he was
>>>> seeing on IRC.  I'm not sure whether non-atomic drivers have enough
>>>> state setup by the default helpers to work properly.  Worst case we'll
>>>> just assume that a non-atomic driver won't support primary plane
>>>> windowing (since none have in the past) and fall back to looking at the
>>>> mode for legacy non-atomic drivers.
>>>>
>>>>>
>>>>> And I'm sure rotated cases will go boom in some other ways. Probably
>>>>> we should just switch over to using the full plane update for mmio
>>>>> flips to fix it.
>>>>
>>>> Yeah; the core looks at a drm_plane->invert_dimensions field that's
>>>> only
>>>> set by omap.  That should probably be updated to look at the state's
>>>> rotation on atomic-capable drivers.
>>>
>>> We can just look at the src coordinates. Those always match the fb
>>> orientation.
>>
>> Can we just not bother with legacy pageflips on rotated planes? setplane
>> works and once you rotate it kinda gets nasty anyway.
>
> I don't know - thought it is simple enough to make it work so why not?
> Just " [PATCH] drm/i915: Consider plane rotation when calculating stride
> in skl_do_mmio_flip" I posted, plus Matt's "[PATCH] drm: Check fb
> against plane size rather than CRTC mode for pageflip​" to allow smaller
> than mode planes.
>
>> The problem I see is that with legacy pageflip we also need to hack up
>> something that doesn't look at plane->state for legacy and all that for a
>> grand total of about 2 drivers, both getting converted to atomic.
>
> I'll leave the legacy/atomic/etc considerations to the experts. :)

Are we sure any efforts to support rotation in legacy page flips is not 
worth it?

So far there were three patches for this: Plane programming fix (very 
simple) and an IGT test case (simple as well) from me, and a sub-crtc 
size plane support from Matt.

It kind of remained hanging a bit so I think it would be good to make a 
definitive decision.

Regards,

Tvrtko