Re: [DMC_BUGFIX_SKL_V2 1/5] drm/i915/skl: Added a check for the hardware status of csr fw before loading.

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Sep 07, 2015 at 04:34:30PM +0530, Sunil Kamath wrote:
> On Wednesday 26 August 2015 01:36 AM, Animesh Manna wrote:
> >Dmc will restore the csr program except DC9, cold boot,
> >warm reset, PCI function level reset, and hibernate/suspend.
> >
> >intel_csr_load_program() function is used to load the firmware
> >data from kernel memory to csr address space.
> >
> >All values of csr address space will be zero if it got reset and
> >the first byte of csr program is always a non-zero if firmware
> >is loaded successfuly. Based on hardware status will load the
> >firmware.
> >
> >Without this condition check if we overwrite the firmware data the
> >counters exposed for dc5/dc6 (help for debugging) will be nullified.
> >
> >v1: Initial version.
> >
> >v2: Based on review comments from Daniel,
> >- Added a check to know hardware status and load the firmware if not loaded.
> >
> >Cc: Daniel Vetter <daniel.vetter@intel.com>
> >Cc: Damien Lespiau <damien.lespiau@intel.com>
> >Cc: Imre Deak <imre.deak@intel.com>
> >Cc: Sunil Kamath <sunil.kamath@intel.com>
> >Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_csr.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> >index ba1ae03..682cc26 100644
> >--- a/drivers/gpu/drm/i915/intel_csr.c
> >+++ b/drivers/gpu/drm/i915/intel_csr.c
> >@@ -252,6 +252,15 @@ void intel_csr_load_program(struct drm_device *dev)
> >  		return;
> >  	}
> >+	/*
> >+	 * Dmc will restore the csr the program except DC9, cold boot,
> >+	 * warm reset, PCI function level reset, and hibernate/suspend.
> >+	 * This condition will help to check if csr address space is reset/
> >+	 * not loaded.
> >+	 */
> >+	if (I915_READ(CSR_PROGRAM_BASE))
> >+		return;
> >+
> >  	mutex_lock(&dev_priv->csr_lock);
> >  	fw_size = dev_priv->csr.dmc_fw_size;
> >  	for (i = 0; i < fw_size; i++)
> 
> Valid fix and patch is ready for merge now.
> 
> Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>

Ok I changed the code comment to a FIXME to explain what's going on (DC9
is totally irrelevant if I understand this correctly now) and amended the
commit message with a note about what I think is really going on. Please
double-check.

Also I realized that we have 0 test coverage for suspend-to-idle (and
oversight for merging this feature a few years back). Please add a new igt
testcase (maybe subtest of drv_suspend) to exercise this feature.

Making sure that we have sufficient igt coverage is part of the review and
especially something that _must_ be checked for bugfixes like this.

Thanks, 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