Re: [PATCH v4 6/6] drm/i915: Add skl_check_nv12_surface for NV12

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 18-04-18 om 17:32 schreef Ville Syrjälä:
> On Wed, Apr 18, 2018 at 09:38:13AM +0530, Vidya Srinivas wrote:
>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>
>> We skip src trunction/adjustments for
>> NV12 case and handle the sizes directly.
>> Without this, pipe fifo underruns are seen on APL/KBL.
>>
>> v2: For NV12, making the src coordinates multiplier of 4
>>
>> v3: Moving all the src coords handling code for NV12
>> to skl_check_nv12_surface
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_sprite.c  | 15 ++++++++++----
>>  2 files changed, 50 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 925402e..b8dbaca 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3118,6 +3118,42 @@ static int skl_check_main_surface(const struct intel_crtc_state *crtc_state,
>>  	return 0;
>>  }
>>  
>> +static int
>> +skl_check_nv12_surface(const struct intel_crtc_state *crtc_state,
>> +		       struct intel_plane_state *plane_state)
>> +{
>> +	int crtc_x2 = plane_state->base.crtc_x + plane_state->base.crtc_w;
>> +	int crtc_y2 = plane_state->base.crtc_y + plane_state->base.crtc_h;
>> +
>> +	if (((plane_state->base.src_x >> 16) % 4) != 0 ||
>> +	    ((plane_state->base.src_y >> 16) % 4) != 0 ||
>> +	    ((plane_state->base.src_w >> 16) % 4) != 0 ||
>> +	    ((plane_state->base.src_h >> 16) % 4) != 0) {
>> +		DRM_DEBUG_KMS("src coords must be multiple of 4 for NV12\n");
>> +		return -EINVAL;
>> +	}
> I don't really see why we should check these. The clipped coordinates
> are what matters.

To propagate our limits to the userspace. I think we should do it for all formats,
but NV12 is the first YUV format we have tests for. If we could we should do
something similar for the other YUV formats, but they have different requirements.

In case of NV12 we don't have existing userspace, there will be nothing that
breaks if we enforce limits from the start.

>> +
>> +	/* Clipping would cause a 1-3 pixel gap at the edge of the screen? */
>> +	if ((crtc_x2 > crtc_state->pipe_src_w && crtc_state->pipe_src_w % 4) ||
>> +	    (crtc_y2 > crtc_state->pipe_src_h && crtc_state->pipe_src_h % 4)) {
>> +		DRM_DEBUG_KMS("It's not possible to clip %u,%u to %u,%u\n",
>> +			      crtc_x2, crtc_y2,
>> +			      crtc_state->pipe_src_w, crtc_state->pipe_src_h);
>> +		return -EINVAL;
>> +	}
> Why should we care? The current code already plays it fast and loose
> and allows the dst rectangle to shrink to accomodate the hw limits.
> If we want to change that we should change it universally.

Unfortunately for the other formats we already have an existing userspace
(X.org) that doesn't perform any validation. We can't change it for that,
but we can prevent future mistakes.

>> +
>> +	plane_state->base.src.x1 =
>> +		DIV_ROUND_CLOSEST(plane_state->base.src.x1, 1 << 18) << 18;
>> +	plane_state->base.src.x2 =
>> +		DIV_ROUND_CLOSEST(plane_state->base.src.x2, 1 << 18) << 18;
>> +	plane_state->base.src.y1 =
>> +		DIV_ROUND_CLOSEST(plane_state->base.src.y1, 1 << 18) << 18;
>> +	plane_state->base.src.y2 =
>> +		DIV_ROUND_CLOSEST(plane_state->base.src.y2, 1 << 18) << 18;
> Since this can now increase the size of the source rectangle our
> scaling factor checks are no longer 100% valid. We might end up with
> a scaling factor that is too high.
>
> I don't really like any of these "let's make NV12 behave special"
> tricks. We should make the code behave the same way for all pixel
> formats instead of adding format specific hacks.

This is not nivalid because we restrict the original src coordinates to be
a multiple of 4, you can only clip to something smaller, not to something
bigger. :)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx