[PATCH] drm/i915: Don't clear all watermarks when updating.

[PATCH] drm/i915: Don't clear all watermarks when updating.

[Bob Paauwe]

Clearing the watermarks for all pipes/planes when updating the
watermarks for a single CRTC change seems like the wrong thing to
do here. As is, this code will update any pipe/plane watermarks
that need updating and leave the remaining set to zero.  Later, the
watermark checks in check_wm_state() will flag these zero'd out pipe/plane
watermarks and throw errors.

By not clearing all pipe/plane watermark values, only those that
require changes are changed and the remaining stay unchanged.

Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c2f8956..25fc319 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3734,8 +3734,6 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct skl_pipe_wm pipe_wm = {};
 	struct intel_wm_config config = {};
 
-	memset(results, 0, sizeof(*results));
-
 	skl_compute_wm_global_parameters(dev, &config);
 
 	if (!skl_update_pipe_wm(crtc, &params, &config,
-- 
2.1.0

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

Re: [PATCH] drm/i915: Don't clear all watermarks when updating.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6753
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -1              287/287              286/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't clear all watermarks when updating.

[Damien Lespiau]

On Wed, Jul 08, 2015 at 09:05:53AM -0700, Bob Paauwe wrote:
> Clearing the watermarks for all pipes/planes when updating the
> watermarks for a single CRTC change seems like the wrong thing to
> do here. As is, this code will update any pipe/plane watermarks
> that need updating and leave the remaining set to zero.  Later, the
> watermark checks in check_wm_state() will flag these zero'd out pipe/plane
> watermarks and throw errors.
> 
> By not clearing all pipe/plane watermark values, only those that
> require changes are changed and the remaining stay unchanged.
> 
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>

Hi Bob,

The spirit of this patch looks good to me (thanks for looking into
this!), but we I think we still need to clear the data for the pipe
we're are updating as it will contain stale values from the previous
skl_update_wm().

For instance we don't reset ->dirty[pipe] to false after the register
write and were relying on that memset. 2 options I can see:

  - selectively zero all the fields for that pipe (not helped by the
    fact we have arrays indexed by the pipe (array of structure Vs
    structure of arrays))
  - make sure we don't rely on any field from the previous call (eg.
    update ->dirty in skl_write_wm_values())

I have a slight preference for the first solution as it seems a bit more
fool proof, but don't mind either way.

Thanks,

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c2f8956..25fc319 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3734,8 +3734,6 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	struct skl_pipe_wm pipe_wm = {};
>  	struct intel_wm_config config = {};
>  
> -	memset(results, 0, sizeof(*results));
> -
>  	skl_compute_wm_global_parameters(dev, &config);
>  
>  	if (!skl_update_pipe_wm(crtc, &params, &config,
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't clear all watermarks when updating.

[Damien Lespiau]

On Wed, Jul 08, 2015 at 09:05:53AM -0700, Bob Paauwe wrote:
> Clearing the watermarks for all pipes/planes when updating the
> watermarks for a single CRTC change seems like the wrong thing to
> do here. As is, this code will update any pipe/plane watermarks
> that need updating and leave the remaining set to zero.  Later, the
> watermark checks in check_wm_state() will flag these zero'd out pipe/plane
> watermarks and throw errors.
> 
> By not clearing all pipe/plane watermark values, only those that
> require changes are changed and the remaining stay unchanged.
> 
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>

Also, could you please prefix the subject of this patch by
'drm/i915/skl'? it will ensure people can easily find the SKL/BXT
related patches to backport in their own tree.

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c2f8956..25fc319 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3734,8 +3734,6 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	struct skl_pipe_wm pipe_wm = {};
>  	struct intel_wm_config config = {};
>  
> -	memset(results, 0, sizeof(*results));
> -
>  	skl_compute_wm_global_parameters(dev, &config);
>  
>  	if (!skl_update_pipe_wm(crtc, &params, &config,
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't clear all watermarks when updating.

[Bob Paauwe]

On Thu, 16 Jul 2015 13:30:11 +0100
Damien Lespiau <damien.lespiau@intel.com> wrote:

> On Wed, Jul 08, 2015 at 09:05:53AM -0700, Bob Paauwe wrote:
> > Clearing the watermarks for all pipes/planes when updating the
> > watermarks for a single CRTC change seems like the wrong thing to
> > do here. As is, this code will update any pipe/plane watermarks
> > that need updating and leave the remaining set to zero.  Later, the
> > watermark checks in check_wm_state() will flag these zero'd out pipe/plane
> > watermarks and throw errors.
> > 
> > By not clearing all pipe/plane watermark values, only those that
> > require changes are changed and the remaining stay unchanged.
> > 
> > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> 
> Hi Bob,
> 
> The spirit of this patch looks good to me (thanks for looking into
> this!), but we I think we still need to clear the data for the pipe
> we're are updating as it will contain stale values from the previous
> skl_update_wm().
> 
> For instance we don't reset ->dirty[pipe] to false after the register
> write and were relying on that memset. 2 options I can see:
> 
>   - selectively zero all the fields for that pipe (not helped by the
>     fact we have arrays indexed by the pipe (array of structure Vs
>     structure of arrays))
>   - make sure we don't rely on any field from the previous call (eg.
>     update ->dirty in skl_write_wm_values())
> 
> I have a slight preference for the first solution as it seems a bit more
> fool proof, but don't mind either way.

Thanks Damien,

I'll look into implementing your first suggestion and see how it goes.

I wasn't sure if anything needed to be cleared out or not. Based on
tracing the code, it looked like the pipe (or pipes) that needed
updating would get updated properly in the structure. And this did
eliminate the DRM ERRORs so it seemed like it might be a valid
solution. But I didn't look at the dirty flag to see if something might
be wrong there.

> 
> Thanks,
> 

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

[PATCH] drm/i915/skl: Don't clear all watermarks when updating. (v2)

[Bob Paauwe]

Clearing the watermarks for all pipes/planes when updating the
watermarks for a single CRTC change seems like the wrong thing to
do here. As is, this code will ony update any pipe/plane watermarks
that need updating and leave the remaining set to zero.  Later, the
watermark checks in check_wm_state() will flag these zero'd out pipe/plane
watermarks and throw errors.

By clearing only the watermark values associated with the specific crtc
the other watermark values may remain unchanged.

v2: Make sure all the dirty flags are cleared. Damien
    Clear all values assoicated with crtc/pipe being updated.  Damien

Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a1d92b7..27c3126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3670,6 +3670,26 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
 	}
 }
 
+static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
+{
+	watermarks->wm_linetime[pipe] = 0;
+	memset(watermarks->plane[pipe], 0,
+	       sizeof(uint32_t) * 8 * I915_MAX_PLANES);
+	memset(watermarks->cursor[pipe], 0, sizeof(uint32_t) * 8);
+	memset(watermarks->plane_trans[pipe],
+	       0, sizeof(uint32_t) * I915_MAX_PLANES);
+	watermarks->cursor_trans[pipe] = 0;
+
+	/* Clear ddb entries for pipe */
+	memset(&watermarks->ddb.pipe[pipe], 0, sizeof(struct skl_ddb_entry));
+	memset(&watermarks->ddb.plane[pipe], 0,
+	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
+	memset(&watermarks->ddb.y_plane[pipe], 0,
+	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
+	memset(&watermarks->ddb.cursor[pipe], 0, sizeof(struct skl_ddb_entry));
+
+}
+
 static void skl_update_wm(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -3680,7 +3700,11 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct skl_pipe_wm pipe_wm = {};
 	struct intel_wm_config config = {};
 
-	memset(results, 0, sizeof(*results));
+
+	/* Clear all dirty flags */
+	memset(results->dirty, 0, sizeof(bool) * I915_MAX_PIPES);
+
+	skl_clear_wm(results, intel_crtc->pipe);
 
 	skl_compute_wm_global_parameters(dev, &config);
 
-- 
2.1.0

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

Re: [PATCH] drm/i915/skl: Don't clear all watermarks when updating. (v2)

[Bob Paauwe]

Damien,

You reviewed v1 and then went on vacation for v2. Any chance you can
review v2?

Thanks,
Bob

 On Tue, 21 Jul 2015 10:42:53 -0700
Bob Paauwe <bob.j.paauwe@intel.com> wrote:

> Clearing the watermarks for all pipes/planes when updating the
> watermarks for a single CRTC change seems like the wrong thing to
> do here. As is, this code will ony update any pipe/plane watermarks
> that need updating and leave the remaining set to zero.  Later, the
> watermark checks in check_wm_state() will flag these zero'd out pipe/plane
> watermarks and throw errors.
> 
> By clearing only the watermark values associated with the specific crtc
> the other watermark values may remain unchanged.
> 
> v2: Make sure all the dirty flags are cleared. Damien
>     Clear all values assoicated with crtc/pipe being updated.  Damien
> 
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a1d92b7..27c3126 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3670,6 +3670,26 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
>  	}
>  }
>  
> +static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
> +{
> +	watermarks->wm_linetime[pipe] = 0;
> +	memset(watermarks->plane[pipe], 0,
> +	       sizeof(uint32_t) * 8 * I915_MAX_PLANES);
> +	memset(watermarks->cursor[pipe], 0, sizeof(uint32_t) * 8);
> +	memset(watermarks->plane_trans[pipe],
> +	       0, sizeof(uint32_t) * I915_MAX_PLANES);
> +	watermarks->cursor_trans[pipe] = 0;
> +
> +	/* Clear ddb entries for pipe */
> +	memset(&watermarks->ddb.pipe[pipe], 0, sizeof(struct skl_ddb_entry));
> +	memset(&watermarks->ddb.plane[pipe], 0,
> +	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> +	memset(&watermarks->ddb.y_plane[pipe], 0,
> +	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> +	memset(&watermarks->ddb.cursor[pipe], 0, sizeof(struct skl_ddb_entry));
> +
> +}
> +
>  static void skl_update_wm(struct drm_crtc *crtc)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -3680,7 +3700,11 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	struct skl_pipe_wm pipe_wm = {};
>  	struct intel_wm_config config = {};
>  
> -	memset(results, 0, sizeof(*results));
> +
> +	/* Clear all dirty flags */
> +	memset(results->dirty, 0, sizeof(bool) * I915_MAX_PIPES);
> +
> +	skl_clear_wm(results, intel_crtc->pipe);
>  
>  	skl_compute_wm_global_parameters(dev, &config);
>  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

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

Re: [PATCH] drm/i915/skl: Don't clear all watermarks when updating. (v2)

[Damien Lespiau]

On Tue, Jul 21, 2015 at 10:42:53AM -0700, Bob Paauwe wrote:
> Clearing the watermarks for all pipes/planes when updating the
> watermarks for a single CRTC change seems like the wrong thing to
> do here. As is, this code will ony update any pipe/plane watermarks
> that need updating and leave the remaining set to zero.  Later, the
> watermark checks in check_wm_state() will flag these zero'd out pipe/plane
> watermarks and throw errors.
> 
> By clearing only the watermark values associated with the specific crtc
> the other watermark values may remain unchanged.
> 
> v2: Make sure all the dirty flags are cleared. Damien
>     Clear all values assoicated with crtc/pipe being updated.  Damien
> 
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

- Moving to a "array of structs" from a "struct of arrays" organization
  as suggested by Ville would make that much better
- I'd move clearing the dirty flag right after writing the registers

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a1d92b7..27c3126 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3670,6 +3670,26 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
>  	}
>  }
>  
> +static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
> +{
> +	watermarks->wm_linetime[pipe] = 0;
> +	memset(watermarks->plane[pipe], 0,
> +	       sizeof(uint32_t) * 8 * I915_MAX_PLANES);
> +	memset(watermarks->cursor[pipe], 0, sizeof(uint32_t) * 8);
> +	memset(watermarks->plane_trans[pipe],
> +	       0, sizeof(uint32_t) * I915_MAX_PLANES);
> +	watermarks->cursor_trans[pipe] = 0;
> +
> +	/* Clear ddb entries for pipe */
> +	memset(&watermarks->ddb.pipe[pipe], 0, sizeof(struct skl_ddb_entry));
> +	memset(&watermarks->ddb.plane[pipe], 0,
> +	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> +	memset(&watermarks->ddb.y_plane[pipe], 0,
> +	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> +	memset(&watermarks->ddb.cursor[pipe], 0, sizeof(struct skl_ddb_entry));
> +
> +}
> +
>  static void skl_update_wm(struct drm_crtc *crtc)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -3680,7 +3700,11 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	struct skl_pipe_wm pipe_wm = {};
>  	struct intel_wm_config config = {};
>  
> -	memset(results, 0, sizeof(*results));
> +
> +	/* Clear all dirty flags */
> +	memset(results->dirty, 0, sizeof(bool) * I915_MAX_PIPES);
> +
> +	skl_clear_wm(results, intel_crtc->pipe);
>  
>  	skl_compute_wm_global_parameters(dev, &config);
>  
> -- 
> 2.1.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/skl: Don't clear all watermarks when updating. (v2)

[Daniel Vetter]

On Mon, Sep 21, 2015 at 02:48:56PM +0100, Damien Lespiau wrote:
> On Tue, Jul 21, 2015 at 10:42:53AM -0700, Bob Paauwe wrote:
> > Clearing the watermarks for all pipes/planes when updating the
> > watermarks for a single CRTC change seems like the wrong thing to
> > do here. As is, this code will ony update any pipe/plane watermarks
> > that need updating and leave the remaining set to zero.  Later, the
> > watermark checks in check_wm_state() will flag these zero'd out pipe/plane
> > watermarks and throw errors.
> > 
> > By clearing only the watermark values associated with the specific crtc
> > the other watermark values may remain unchanged.
> > 
> > v2: Make sure all the dirty flags are cleared. Damien
> >     Clear all values assoicated with crtc/pipe being updated.  Damien
> > 
> > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> - Moving to a "array of structs" from a "struct of arrays" organization
>   as suggested by Ville would make that much better
> - I'd move clearing the dirty flag right after writing the registers

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx