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

[Tvrtko Ursulin]

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. :)

Regards,

Tvrtko