Re: [PATCH 3/5] drm/i915: Introduce HAS_NV12 and define WA 0870.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 29, 2018 at 04:00:42PM -0700, Rodrigo Vivi wrote:
> Let's introduce HAS_NV12 to check for feature itself
> than spread the platform checks everywhere.
> 
> Also let's introduce the WA number that is the
> cause of having NV12 disabled on both SLK and BXT.
> 
> According to Spec:
> 
> WA 0870: "Display flickers with NV12 video playback in
> Y tiling mode.
> WA: Use YUV422 surface format instead of NV12."
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          | 1 +
>  drivers/gpu/drm/i915/i915_pci.c          | 5 +++++
>  drivers/gpu/drm/i915/intel_device_info.h | 1 +
>  drivers/gpu/drm/i915/intel_display.c     | 7 +++----
>  drivers/gpu/drm/i915/intel_sprite.c      | 2 +-
>  5 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c9e5bab6861b..57ea094054d5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2630,6 +2630,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define HAS_RC6pp(dev_priv)		 (false) /* HW was never validated */
>  
>  #define HAS_CSR(dev_priv)	((dev_priv)->info.has_csr)
> +#define HAS_NV12(dev_priv)	((dev_priv)->info.has_nv12)
>  
>  #define HAS_RUNTIME_PM(dev_priv) ((dev_priv)->info.has_runtime_pm)
>  #define HAS_64BIT_RELOC(dev_priv) ((dev_priv)->info.has_64bit_reloc)
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 44e745921ac1..eb797c1ef842 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -462,6 +462,7 @@ static const struct intel_device_info intel_cherryview_info = {
>  	GEN(9), \
>  	GEN9_DEFAULT_PAGE_SIZES, \
>  	.has_logical_ring_preemption = 1, \
> +	.has_nv12 = 1, \
>  	.has_csr = 1, \
>  	.has_guc = 1, \
>  	.has_ipc = 1, \
> @@ -471,6 +472,8 @@ static const struct intel_device_info intel_cherryview_info = {
>  	GEN9_FEATURES, \
>  	/* Display WA #0477 WaDisableIPC: skl */ \
>  	.has_ipc = 0, \
> +	/* Display WA #0870: skl */ \
> +	.has_nv12 = 0, \
>  	PLATFORM(INTEL_SKYLAKE)
>  
>  static const struct intel_device_info intel_skylake_gt1_info = {
> @@ -531,6 +534,8 @@ static const struct intel_device_info intel_broxton_info = {
>  	GEN9_LP_FEATURES,
>  	PLATFORM(INTEL_BROXTON),
>  	.ddb_size = 512,
> +	/* Display WA #0870: bxt */
> +	.has_nv12 = 0,
>  };
>  
>  static const struct intel_device_info intel_geminilake_info = {
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index b4c2c4eae78b..ba9e9c59dc6c 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -104,6 +104,7 @@ enum intel_ppgtt {
>  	func(has_logical_ring_contexts); \
>  	func(has_logical_ring_elsq); \
>  	func(has_logical_ring_preemption); \
> +	func(has_nv12); \
>  	func(has_overlay); \
>  	func(has_pooled_eu); \
>  	func(has_psr); \
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dd88ffe9e273..14f6f66b00d2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -470,7 +470,7 @@ skl_wa_528(struct drm_i915_private *dev_priv, int pipe, bool enable)
>  static void
>  skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool enable)
>  {
> -	if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv))
> +	if (!HAS_NV12(dev_priv))
>  		return;
>  
>  	if (enable)
> @@ -5239,7 +5239,7 @@ static bool needs_nv12_wa(struct drm_i915_private *dev_priv,
>  	if (!crtc_state->nv12_planes)
>  		return false;
>  
> -	if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv))
> +	if (!HAS_NV12(dev_priv))
>  		return false;

The above three checks (you missed skl_wa_528()) can be removed entirely.
The code won't be reached unless at least one plane uses NV12.

>  
>  	/* WA Display #0827: Gen9:all */
> @@ -14519,8 +14519,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  		}
>  		break;
>  	case DRM_FORMAT_NV12:
> -		if (INTEL_GEN(dev_priv) < 9 || IS_SKYLAKE(dev_priv) ||
> -		    IS_BROXTON(dev_priv)) {
> +		if (!HAS_NV12(dev_priv)) {
>  			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
>  				      drm_get_format_name(mode_cmd->pixel_format,
>  							  &format_name));

Undead code. The kill-a-zombie (tm) potion should kick in soon:
https://patchwork.freedesktop.org/patch/259041/

> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index e7c95ec879cc..582a2972c90d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1868,7 +1868,7 @@ static bool skl_plane_has_planar(struct drm_i915_private *dev_priv,
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		return plane_id <= PLANE_SPRITE3;
>  
> -	if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv))
> +	if (!HAS_NV12(dev_priv))
>  		return false;

So this is the only thing left in the end. Adding a device info flag
for this is overkill IMO. The w/a name we should add of course.

>  
>  	if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv) && pipe == PIPE_C)
> -- 
> 2.19.1

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