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@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
