Re: [Intel-gfx] [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch

2018-02-08 Thread Srinivas, Vidya 
Apologies. This patch was pushed wrongly. Not a part of the 16 patch series of 
NV12.

Regards
Vidya

> -Original Message-
> From: Sharma, Shashank
> Sent: Thursday, February 8, 2018 6:08 PM
> To: Srinivas, Vidya ; intel-
> g...@lists.freedesktop.org
> Cc: maarten.lankho...@linux.intel.com; Kamath, Sunil
> ; Shankar, Uma ;
> Konduru, Chandra 
> Subject: Re: [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch
> 
> Regards
> 
> Shashank
> 
> 
> On 2/6/2018 4:36 PM, Vidya Srinivas wrote:
> > From: Chandra Konduru 
> >
> > Display WA #0827:
> > Switching the plane format from NV12 to RGB and leaving system idle
> > results in display underrun and corruption. WA: Set the bit 15 & bit
> > 19 to 1b in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane
> is enabled.
> >
> > Signed-off-by: Chandra Konduru 
> > Signed-off-by: Vidya Srinivas 
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h  |  3 +++
> >   drivers/gpu/drm/i915/intel_display.c | 16 
> >   2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 8f36023..c4af05e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3822,6 +3822,9 @@ enum {
> >   #define _CLKGATE_DIS_PSL_A0x46520
> >   #define _CLKGATE_DIS_PSL_B0x46524
> >   #define _CLKGATE_DIS_PSL_C0x46528
> > +#define DUPS1_GATING_DIS   (1 << 15)
> > +#define DUPS2_GATING_DIS   (1 << 19)
> > +#define DUPS3_GATING_DIS   (1 << 23)
> Bit definition should be aligned by one extra space (like below), also the bit
> sequence should be high -> low (so 23,19 and then 15)
> >   #define   DPF_GATING_DIS  (1 << 10)
> >   #define   DPF_RAM_GATING_DIS  (1 << 9)
> >   #define   DPFR_GATING_DIS (1 << 8)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 551c970..94faf3e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5495,6 +5495,20 @@ static void
> glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv,
> > I915_WRITE(CLKGATE_DIS_PSL(pipe), val);
> >   }
> >
> > +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> > +   int pipe, int enable)
> Do we need an int ? or bool enable ? also This line should be aligned to
> opening brace '(' above.
> > +{
> > +   if (pipe == PIPE_A || pipe == PIPE_B) {
> > +   if (enable)
> > +   I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > +   DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> Alignment, also we are overwriting all other bits here, this should be
> I915_WRITE(CLKGATE_DIS_PSL(pipe), I915_READ(CLKGATE_DIS_PSL(pipe))
> |= (DUPS1_GATING_DIS | DUPS2_GATING_DIS) )
> > +   else
> > +   I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > +   I915_READ(CLKGATE_DIS_PSL(pipe)) &
> This line should be aligned to the '(' above
> > +   ~(DUPS1_GATING_DIS|DUPS2_GATING_DIS));
> > +   }
> > +}
> > +
> >   static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> > struct drm_atomic_state *old_state)
> >   {
> > @@ -5599,6 +5613,7 @@ static void haswell_crtc_enable(struct
> intel_crtc_state *pipe_config,
> > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
> > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
> > }
> > +   skl_wa_clkgate(dev_priv, pipe, 1);
> send true from here (instead of 1)
> >   }
> >
> >   static void ironlake_pfit_disable(struct intel_crtc *crtc, bool
> > force) @@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct
> intel_crtc_state *old_crtc_state,
> > intel_ddi_disable_pipe_clock(intel_crtc->config);
> >
> > intel_encoders_post_disable(crtc, old_crtc_state, old_state);
> > +   skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0);
> send false from here (instead of 0)
> >   }
> >
> >   static void i9xx_pfit_enable(struct intel_crtc *crtc)

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

Re: [Intel-gfx] [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch

2018-02-08 Thread Sharma, Shashank 

Regards

Shashank


On 2/6/2018 4:36 PM, Vidya Srinivas wrote:

From: Chandra Konduru 

Display WA #0827:
Switching the plane format from NV12 to RGB and leaving system idle results
in display underrun and corruption. WA: Set the bit 15 & bit 19 to 1b
in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane is enabled.

Signed-off-by: Chandra Konduru 
Signed-off-by: Vidya Srinivas 
---
  drivers/gpu/drm/i915/i915_reg.h  |  3 +++
  drivers/gpu/drm/i915/intel_display.c | 16 
  2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8f36023..c4af05e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3822,6 +3822,9 @@ enum {
  #define _CLKGATE_DIS_PSL_A0x46520
  #define _CLKGATE_DIS_PSL_B0x46524
  #define _CLKGATE_DIS_PSL_C0x46528
+#define DUPS1_GATING_DIS   (1 << 15)
+#define DUPS2_GATING_DIS   (1 << 19)
+#define DUPS3_GATING_DIS   (1 << 23)
Bit definition should be aligned by one extra space (like below), also 
the bit sequence should be high -> low (so 23,19 and then 15)

  #define   DPF_GATING_DIS  (1 << 10)
  #define   DPF_RAM_GATING_DIS  (1 << 9)
  #define   DPFR_GATING_DIS (1 << 8)
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 551c970..94faf3e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5495,6 +5495,20 @@ static void glk_pipe_scaler_clock_gating_wa(struct 
drm_i915_private *dev_priv,
I915_WRITE(CLKGATE_DIS_PSL(pipe), val);
  }
  
+static void skl_wa_clkgate(struct drm_i915_private *dev_priv,

+   int pipe, int enable)
Do we need an int ? or bool enable ? also This line should be aligned to 
opening brace '(' above.

+{
+   if (pipe == PIPE_A || pipe == PIPE_B) {
+   if (enable)
+   I915_WRITE(CLKGATE_DIS_PSL(pipe),
+   DUPS1_GATING_DIS | DUPS2_GATING_DIS);
Alignment, also we are overwriting all other bits here, this should be 
I915_WRITE(CLKGATE_DIS_PSL(pipe), I915_READ(CLKGATE_DIS_PSL(pipe)) |= 
(DUPS1_GATING_DIS | DUPS2_GATING_DIS) )

+   else
+   I915_WRITE(CLKGATE_DIS_PSL(pipe),
+   I915_READ(CLKGATE_DIS_PSL(pipe)) &

This line should be aligned to the '(' above

+   ~(DUPS1_GATING_DIS|DUPS2_GATING_DIS));
+   }
+}
+
  static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
struct drm_atomic_state *old_state)
  {
@@ -5599,6 +5613,7 @@ static void haswell_crtc_enable(struct intel_crtc_state 
*pipe_config,
intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
}
+   skl_wa_clkgate(dev_priv, pipe, 1);

send true from here (instead of 1)

  }
  
  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force)

@@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct intel_crtc_state 
*old_crtc_state,
intel_ddi_disable_pipe_clock(intel_crtc->config);
  
  	intel_encoders_post_disable(crtc, old_crtc_state, old_state);

+   skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0);

send false from here (instead of 0)

  }
  
  static void i9xx_pfit_enable(struct intel_crtc *crtc)


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

Re: [Intel-gfx] [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch

2018-02-06 Thread Srinivas, Vidya 
Hi Maarten

Sorry, my bad. This was a wrong push from my end. I have changed the tag to Not 
applicable.
Apologies.

Have sent out the NV12 series separately.

Regards
Vidya

> -Original Message-
> From: Maarten Lankhorst [mailto:maarten.lankho...@linux.intel.com]
> Sent: Tuesday, February 6, 2018 7:33 PM
> To: Srinivas, Vidya ; intel-
> g...@lists.freedesktop.org
> Cc: Kamath, Sunil ; Sharma, Shashank
> ; Shankar, Uma ;
> Konduru, Chandra 
> Subject: Re: [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch
> 
> Hey,
> 
> Op 06-02-18 om 12:06 schreef Vidya Srinivas:
> > From: Chandra Konduru 
> >
> > Display WA #0827:
> > Switching the plane format from NV12 to RGB and leaving system idle
> > results in display underrun and corruption. WA: Set the bit 15 & bit
> > 19 to 1b in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane
> is enabled.
> >
> > Signed-off-by: Chandra Konduru 
> > Signed-off-by: Vidya Srinivas 
> 
> Is it required to leave the workaround enabled all the time? And how can I
> reproduce it? I tried to write a dumb testcase (below) to expose this issue,
> but didn't have much luck..
> ---
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources index
> 870c9093550b..f536db0fa433 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -192,6 +192,7 @@ TESTS_progs = \
>   kms_legacy_colorkey \
>   kms_mmap_write_crc \
>   kms_mmio_vs_cs_flip \
> + kms_nv12 \
>   kms_panel_fitting \
>   kms_pipe_b_c_ivb \
>   kms_pipe_crc_basic \
> diff --git a/tests/kms_nv12.c b/tests/kms_nv12.c new file mode 100644
> index ..384a15afde52
> --- /dev/null
> +++ b/tests/kms_nv12.c
> @@ -0,0 +1,195 @@
> +/*
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a
> + * copy of this software and associated documentation files (the
> +"Software"),
> + * to deal in the Software without restriction, including without
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> +sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> +included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT
> +SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR
> +OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> OTHERWISE,
> +ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> OR OTHER
> +DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *Maarten Lankhorst
> + */
> +#include "config.h"
> +
> +#include "igt.h"
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +typedef struct {
> + igt_display_t display;
> +
> + igt_pipe_crc_t *pipe_crc;
> + struct igt_fb fb[4];
> +} data_t;
> +
> +static bool plane_supports_format(igt_plane_t *plane, uint32_t format)
> +{
> + int i;
> +
> + if (!igt_fb_supported_format(format))
> + return false;
> +
> + for (i = 0; i < plane->drm_plane->count_formats; i++)
> + if (plane->drm_plane->formats[i] == format)
> + return true;
> +
> + return false;
> +}
> +
> +static bool pipe_supports_format(igt_display_t *display, enum pipe
> +pipe, uint32_t format) {
> + igt_plane_t *plane;
> +
> + for_each_plane_on_pipe(display, pipe, plane)
> + if (plane_supports_format(plane, format))
> + return true;
> +
> + return false;
> +}
> +
> +static void remove_fbs(data_t *data)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(data->fb); i++)
> + igt_remove_fb(data->display.drm_fd, >fb[i]); }
> +
> +static void prepare_crtc(data_t *data, enum pipe pipe, igt_output_t
> +*output) {
> + igt_display_t *display = >display;
> +
> + remove_fbs(data);
> + igt_display_reset(display);
> + igt_output_set_pipe(output, pipe);
> + igt_display_commit2(display, COMMIT_ATOMIC);
> +
> + igt_pipe_crc_free(data->pipe_crc);
> + data->pipe_crc = igt_pipe_crc_new(display->drm_fd, pipe,
> +INTEL_PIPE_CRC_SOURCE_AUTO); }
> +
> +static void set_fb(igt_plane_t *plane, struct igt_fb *fb) {
> + igt_plane_set_fb(plane, fb);
> +
> + if (fb && fb->tiling == LOCAL_I915_FORMAT_MOD_Y_TILED) {
> + igt_plane_set_rotation(plane, IGT_ROTATION_90);
> + igt_plane_set_size(plane, fb->height,

Re: [Intel-gfx] [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch

2018-02-06 Thread Maarten Lankhorst 
Hey,

Op 06-02-18 om 12:06 schreef Vidya Srinivas:
> From: Chandra Konduru 
>
> Display WA #0827:
> Switching the plane format from NV12 to RGB and leaving system idle results
> in display underrun and corruption. WA: Set the bit 15 & bit 19 to 1b
> in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane is enabled.
>
> Signed-off-by: Chandra Konduru 
> Signed-off-by: Vidya Srinivas 

Is it required to leave the workaround enabled all the time? And how can I 
reproduce it? I tried to write a
dumb testcase (below) to expose this issue, but didn't have much luck..
---
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 870c9093550b..f536db0fa433 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -192,6 +192,7 @@ TESTS_progs = \
kms_legacy_colorkey \
kms_mmap_write_crc \
kms_mmio_vs_cs_flip \
+   kms_nv12 \
kms_panel_fitting \
kms_pipe_b_c_ivb \
kms_pipe_crc_basic \
diff --git a/tests/kms_nv12.c b/tests/kms_nv12.c
new file mode 100644
index ..384a15afde52
--- /dev/null
+++ b/tests/kms_nv12.c
@@ -0,0 +1,195 @@
+/*
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Maarten Lankhorst 
+ */
+#include "config.h"
+
+#include "igt.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+
+typedef struct {
+   igt_display_t display;
+
+   igt_pipe_crc_t *pipe_crc;
+   struct igt_fb fb[4];
+} data_t;
+
+static bool plane_supports_format(igt_plane_t *plane, uint32_t format)
+{
+   int i;
+
+   if (!igt_fb_supported_format(format))
+   return false;
+
+   for (i = 0; i < plane->drm_plane->count_formats; i++)
+   if (plane->drm_plane->formats[i] == format)
+   return true;
+
+   return false;
+}
+
+static bool pipe_supports_format(igt_display_t *display, enum pipe pipe, 
uint32_t format)
+{
+   igt_plane_t *plane;
+
+   for_each_plane_on_pipe(display, pipe, plane)
+   if (plane_supports_format(plane, format))
+   return true;
+
+   return false;
+}
+
+static void remove_fbs(data_t *data)
+{
+   int i;
+
+   for (i = 0; i < ARRAY_SIZE(data->fb); i++)
+   igt_remove_fb(data->display.drm_fd, >fb[i]);
+}
+
+static void prepare_crtc(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+   igt_display_t *display = >display;
+
+   remove_fbs(data);
+   igt_display_reset(display);
+   igt_output_set_pipe(output, pipe);
+   igt_display_commit2(display, COMMIT_ATOMIC);
+
+   igt_pipe_crc_free(data->pipe_crc);
+   data->pipe_crc = igt_pipe_crc_new(display->drm_fd, pipe, 
INTEL_PIPE_CRC_SOURCE_AUTO);
+}
+
+static void set_fb(igt_plane_t *plane, struct igt_fb *fb)
+{
+   igt_plane_set_fb(plane, fb);
+
+   if (fb && fb->tiling == LOCAL_I915_FORMAT_MOD_Y_TILED) {
+   igt_plane_set_rotation(plane, IGT_ROTATION_90);
+   igt_plane_set_size(plane, fb->height, fb->width);
+   } else
+   igt_plane_set_rotation(plane, IGT_ROTATION_0);
+}
+
+static void nv12_rgb_switch(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+   drmModeModeInfo *mode = igt_output_get_mode(output);
+   igt_display_t *display = >display;
+   igt_plane_t *plane;
+   int i;
+   igt_crc_t ref_crc[4], crc;
+
+   prepare_crtc(data, pipe, output);
+
+   igt_create_pattern_fb(display->drm_fd, mode->hdisplay, mode->vdisplay,
+ DRM_FORMAT_NV12, LOCAL_I915_FORMAT_MOD_X_TILED,
+ >fb[0]);
+
+   igt_create_pattern_fb(display->drm_fd, mode->vdisplay, mode->hdisplay,
+ DRM_FORMAT_NV12, LOCAL_I915_FORMAT_MOD_Y_TILED,
+ >fb[1]);
+
+   igt_create_pattern_fb(display->drm_fd, mode->hdisplay,

Re: [Intel-gfx] [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch

2018-02-06 Thread Srinivas, Vidya 
Sorry, my bad. This was a wrong push from my end. I have changed the tag to Not 
applicable.
Apologies.

Have sent out the NV12 series separately.

Regards
Vidya

> -Original Message-
> From: David Weinehall [mailto:david.weineh...@linux.intel.com]
> Sent: Tuesday, February 6, 2018 5:34 PM
> To: Srinivas, Vidya <vidya.srini...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Display WA #0827 for NV12 to
> RGB switch
> 
> On Tue, Feb 06, 2018 at 04:36:42PM +0530, Vidya Srinivas wrote:
> > From: Chandra Konduru <chandra.kond...@intel.com>
> >
> > Display WA #0827:
> > Switching the plane format from NV12 to RGB and leaving system idle
> > results in display underrun and corruption. WA: Set the bit 15 & bit
> > 19 to 1b in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane
> is enabled.
> >
> > Signed-off-by: Chandra Konduru <chandra.kond...@intel.com>
> > Signed-off-by: Vidya Srinivas <vidya.srini...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  3 +++
> >  drivers/gpu/drm/i915/intel_display.c | 16 
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 8f36023..c4af05e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3822,6 +3822,9 @@ enum {
> >  #define _CLKGATE_DIS_PSL_A 0x46520
> >  #define _CLKGATE_DIS_PSL_B 0x46524
> >  #define _CLKGATE_DIS_PSL_C 0x46528
> > +#define DUPS1_GATING_DIS   (1 << 15)
> > +#define DUPS2_GATING_DIS   (1 << 19)
> > +#define DUPS3_GATING_DIS   (1 << 23)
> >  #define   DPF_GATING_DIS   (1 << 10)
> >  #define   DPF_RAM_GATING_DIS   (1 << 9)
> >  #define   DPFR_GATING_DIS  (1 << 8)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 551c970..94faf3e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5495,6 +5495,20 @@ static void
> glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv,
> > I915_WRITE(CLKGATE_DIS_PSL(pipe), val);  }
> >
> > +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> > +   int pipe, int enable)
> > +{
> > +   if (pipe == PIPE_A || pipe == PIPE_B) {
> > +   if (enable)
> > +   I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > +   DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> > +   else
> > +   I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > +   I915_READ(CLKGATE_DIS_PSL(pipe)) &
> > +   ~(DUPS1_GATING_DIS|DUPS2_GATING_DIS));
> > +   }
> > +}
> > +
> >  static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> > struct drm_atomic_state *old_state)  { @@
> -5599,6 +5613,7 @@
> > static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
> > intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
> > }
> > +   skl_wa_clkgate(dev_priv, pipe, 1);
> >  }
> >
> >  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool
> > force) @@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct
> intel_crtc_state *old_crtc_state,
> > intel_ddi_disable_pipe_clock(intel_crtc->config);
> >
> > intel_encoders_post_disable(crtc, old_crtc_state, old_state);
> > +   skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0);
> >  }
> 
> Unless I'm misreading the context of this patch you're applying a
> workaround, that by name seems to be for Skylake only, to: Haswell,
> Broadwell, and gen9+.
> 
> Either the name is incorrect, or the application of it.
> 
> As per BSpec the workaround seems to be for all of gen9 and only A-
> stepping of gen10.
> I don't see it listed for Haswell or Broadwell.
> 
> Cross-referencing the WA-database with Bspec, based on the HSD link, it
> seems that this issue *might* be
> WaDups1GatingDisableClockGatingForMPO; if this is the case it might make
> sense to include that WA name too. At the very least there should always be
> a comment mentioning the workaround name/number and the platform(s)
> it applies to.
> 
> Also, according to the WA database, if the above mentioned issue really is
> the same, the WA is *NOT* necessary on GLK (seeing as GLK uses gen10
> display this might make sense, though the WA database sometimes
> contains mistakes).
> 
> 
> Regards, David
> 
> >  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> > --
> > 1.9.1
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch

2018-02-06 Thread David Weinehall 
On Tue, Feb 06, 2018 at 04:36:42PM +0530, Vidya Srinivas wrote:
> From: Chandra Konduru 
> 
> Display WA #0827:
> Switching the plane format from NV12 to RGB and leaving system idle results
> in display underrun and corruption. WA: Set the bit 15 & bit 19 to 1b
> in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane is enabled.
> 
> Signed-off-by: Chandra Konduru 
> Signed-off-by: Vidya Srinivas 
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  3 +++
>  drivers/gpu/drm/i915/intel_display.c | 16 
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8f36023..c4af05e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3822,6 +3822,9 @@ enum {
>  #define _CLKGATE_DIS_PSL_A   0x46520
>  #define _CLKGATE_DIS_PSL_B   0x46524
>  #define _CLKGATE_DIS_PSL_C   0x46528
> +#define DUPS1_GATING_DIS (1 << 15)
> +#define DUPS2_GATING_DIS (1 << 19)
> +#define DUPS3_GATING_DIS (1 << 23)
>  #define   DPF_GATING_DIS (1 << 10)
>  #define   DPF_RAM_GATING_DIS (1 << 9)
>  #define   DPFR_GATING_DIS(1 << 8)
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 551c970..94faf3e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5495,6 +5495,20 @@ static void glk_pipe_scaler_clock_gating_wa(struct 
> drm_i915_private *dev_priv,
>   I915_WRITE(CLKGATE_DIS_PSL(pipe), val);
>  }
>  
> +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> + int pipe, int enable)
> +{
> + if (pipe == PIPE_A || pipe == PIPE_B) {
> + if (enable)
> + I915_WRITE(CLKGATE_DIS_PSL(pipe),
> + DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> + else
> + I915_WRITE(CLKGATE_DIS_PSL(pipe),
> + I915_READ(CLKGATE_DIS_PSL(pipe)) &
> + ~(DUPS1_GATING_DIS|DUPS2_GATING_DIS));
> + }
> +}
> +
>  static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>   struct drm_atomic_state *old_state)
>  {
> @@ -5599,6 +5613,7 @@ static void haswell_crtc_enable(struct intel_crtc_state 
> *pipe_config,
>   intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
>   intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
>   }
> + skl_wa_clkgate(dev_priv, pipe, 1);
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force)
> @@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct 
> intel_crtc_state *old_crtc_state,
>   intel_ddi_disable_pipe_clock(intel_crtc->config);
>  
>   intel_encoders_post_disable(crtc, old_crtc_state, old_state);
> + skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0);
>  }

Unless I'm misreading the context of this patch you're applying a workaround,
that by name seems to be for Skylake only, to: Haswell, Broadwell, and gen9+.

Either the name is incorrect, or the application of it.

As per BSpec the workaround seems to be for all of gen9 and only A-stepping of 
gen10.
I don't see it listed for Haswell or Broadwell.

Cross-referencing the WA-database with Bspec, based on the HSD
link, it seems that this issue *might* be
WaDups1GatingDisableClockGatingForMPO; if this is the case it might
make sense to include that WA name too. At the very least
there should always be a comment mentioning the workaround name/number
and the platform(s) it applies to.

Also, according to the WA database, if the above mentioned issue really
is the same, the WA is *NOT* necessary on GLK (seeing as GLK uses gen10
display this might make sense, though the WA database sometimes contains
mistakes).


Regards, David

>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> -- 
> 1.9.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Display WA #0827 for NV12 to RGB switch

2018-02-06 Thread Vidya Srinivas 
From: Chandra Konduru 

Display WA #0827:
Switching the plane format from NV12 to RGB and leaving system idle results
in display underrun and corruption. WA: Set the bit 15 & bit 19 to 1b
in the CLKGATE_DIS_PSL register for the pipe in which NV12 plane is enabled.

Signed-off-by: Chandra Konduru 
Signed-off-by: Vidya Srinivas 
---
 drivers/gpu/drm/i915/i915_reg.h  |  3 +++
 drivers/gpu/drm/i915/intel_display.c | 16 
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8f36023..c4af05e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3822,6 +3822,9 @@ enum {
 #define _CLKGATE_DIS_PSL_A 0x46520
 #define _CLKGATE_DIS_PSL_B 0x46524
 #define _CLKGATE_DIS_PSL_C 0x46528
+#define DUPS1_GATING_DIS   (1 << 15)
+#define DUPS2_GATING_DIS   (1 << 19)
+#define DUPS3_GATING_DIS   (1 << 23)
 #define   DPF_GATING_DIS   (1 << 10)
 #define   DPF_RAM_GATING_DIS   (1 << 9)
 #define   DPFR_GATING_DIS  (1 << 8)
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 551c970..94faf3e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5495,6 +5495,20 @@ static void glk_pipe_scaler_clock_gating_wa(struct 
drm_i915_private *dev_priv,
I915_WRITE(CLKGATE_DIS_PSL(pipe), val);
 }
 
+static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
+   int pipe, int enable)
+{
+   if (pipe == PIPE_A || pipe == PIPE_B) {
+   if (enable)
+   I915_WRITE(CLKGATE_DIS_PSL(pipe),
+   DUPS1_GATING_DIS | DUPS2_GATING_DIS);
+   else
+   I915_WRITE(CLKGATE_DIS_PSL(pipe),
+   I915_READ(CLKGATE_DIS_PSL(pipe)) &
+   ~(DUPS1_GATING_DIS|DUPS2_GATING_DIS));
+   }
+}
+
 static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
struct drm_atomic_state *old_state)
 {
@@ -5599,6 +5613,7 @@ static void haswell_crtc_enable(struct intel_crtc_state 
*pipe_config,
intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
intel_wait_for_vblank(dev_priv, hsw_workaround_pipe);
}
+   skl_wa_clkgate(dev_priv, pipe, 1);
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force)
@@ -5709,6 +5724,7 @@ static void haswell_crtc_disable(struct intel_crtc_state 
*old_crtc_state,
intel_ddi_disable_pipe_clock(intel_crtc->config);
 
intel_encoders_post_disable(crtc, old_crtc_state, old_state);
+   skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
-- 
1.9.1

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