| Message ID | 1436371553-32370-1-git-send-email-bob.j.paauwe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 '*'
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,
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.
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, >
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, ¶ms, &config,
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(-)