drm/i915: Don't clear all watermarks when updating.
diff mbox

Message ID 1436371553-32370-1-git-send-email-bob.j.paauwe@intel.com
State New
Headers show

Commit Message

Bob Paauwe July 8, 2015, 4:05 p.m. UTC
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(-)

Comments

Shuang He July 8, 2015, 9:55 p.m. UTC | #1
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 '*'
Lespiau, Damien July 16, 2015, 12:30 p.m. UTC | #2
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,
Lespiau, Damien July 16, 2015, 12:31 p.m. UTC | #3
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.
Bob Paauwe July 16, 2015, 5:15 p.m. UTC | #4
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,
>

Patch
diff mbox

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,