[Intel-gfx] [PATCH 15/15] drm/i915: Add NV12 support to intel_framebuffer_init

[Maarten Lankhorst]

Op 30-01-18 om 07:16 schreef Kumar, Mahesh:
> Hi,
>
> On 1/30/2018 9:35 AM, Srinivas, Vidya wrote:
>>
>>> -----Original Message-----
>>> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
>>> Sent: Monday, January 29, 2018 10:47 PM
>>> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
>>> gfx at lists.freedesktop.org
>>> Subject: Re: [Intel-gfx] [PATCH 15/15] drm/i915: Add NV12 support to
>>> intel_framebuffer_init
>>>
>>> Op 20-01-18 om 22:45 schreef Vidya Srinivas:
>>>> From: Chandra Konduru <chandra.konduru at intel.com>
>>>>
>>>> This patch adds NV12 as supported format to intel_framebuffer_init and
>>>> performs various checks.
>>>>
>>>> v2:
>>>> -Fix an issue in checks added (Chandra Konduru)
>>>>
>>>> v3: rebased (me)
>>>>
>>>> v4: Review comments by Ville addressed Added platform check for NV12
>>>> in intel_framebuffer_init Removed offset checks for NV12 case
>>>>
>>>> v5: Addressed review comments by Clinton A Taylor This NV12 support
>>>> only correctly works on SKL.
>>>> Plane color space conversion is different on GLK and later platforms
>>>> causing the colors to display incorrectly.
>>>> Ville's plane color space property patch series in review will fix
>>>> this issue.
>>>> - Restricted the NV12 case in intel_framebuffer_init to SKL and BXT
>>>> only.
>>>>
>>>> v6: Rebased (me)
>>>>
>>>> v7: Addressed review comments by Ville Restricting the NV12 to BXT for
>>>> now.
>>>>
>>>> v8: Rebased (me)
>>>> Restricting the NV12 changes to BXT and KBL for now.
>>>>
>>>> v9: Rebased (me)
>>> Hey,
>>>
>>> Has NV12 been tested at all on skylake with rotation?
>>>
>>> y_min_scanlines = 16 for 90°/270° rotation with cpp=1
>>> skl_needs_memory_bw_wa() doubles the scanlines required, which will
>>> trigger the following error quite easily when patching kms_rotation_crc to
>>> use NV12:
>>>
>>> [   67.049190] [drm:skl_compute_wm_levels [i915]] Requested display
>>> configuration exceeds system watermark limitations [   67.049212]
>>> [drm:skl_compute_wm_levels [i915]] [PLANE:28:plane 1A] blocks required =
>>> 161/572, lines required = 32/31
>>>
>>> If we violate the spec for the workaround, by using 31 lines instead of 32 for
>>> 90/270 rotation, the tests work but this needs to be solved first before we
>>> can move forward.
> We are hitting this issue due to memory_bw_wa (this is applicable for GEN-9 only) & we are applying WA irrespective of it's required or not.
> I had plan to apply this WA based on our discussion, but it never materialized :(
> https://www.spinics.net/lists/intel-gfx/msg124872.html
>
>> We came across many such scenarios during NV12 testing on BXT. During downscaling,
>> the "Max supported pixel clock with scaling exceeded" is seen if the user layer tries to downscale
>> much. And rotation, as you mentioned we have seen such messages. We discussed the same during the meeting
>> long back and mentioned that if we remove the limitations, it works. But as per discussion, if limitations are removed, it may work,
>> but there may be certain conditions where it may not work. Hence we concluded that some such cases are expected not to work.
> Either we can conclude that as a limitation (as expected behavior) OR we can enable WA only when it's required (as discussed, assuming all the planes are enabled with max supported scaling to avoid taking mutex in each flip). what you say? 

I think both. We should enable the wa only when required, but also make sure we have sufficient
testing that we know whether the below patch would break. It's still almost doubling, 1.9375x
the original values for 8 bit planes, but this falls within the wm limitation checks.
---
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 26132fa6ebce..23623d1a8569 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4693,7 +4693,9 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
 		wp->y_min_scanlines = 4;
 	}
 
-	if (apply_memory_bw_wa)
+	if (apply_memory_bw_wa && wp->y_min_scanlines == 16)
+		wp->y_min_scanlines = 31;
+	else if (apply_memory_bw_wa)
 		wp->y_min_scanlines *= 2;
 
 	wp->plane_bytes_per_line = wp->width * wp->cpp;