On Wed, Aug 26, 2015 at 07:40:54PM +0530, Animesh Manna wrote: > > > On 8/26/2015 6:40 PM, Daniel Vetter wrote: > >On Wed, Aug 26, 2015 at 01:36:05AM +0530, 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. > > Bacause of the above reason mentioned just above we need to block firmware loading again. > So only WARN_ON will not help. > > > >> > >>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@xxxxxxxxx> > >>Cc: Damien Lespiau <damien.lespiau@xxxxxxxxx> > >>Cc: Imre Deak <imre.deak@xxxxxxxxx> > >>Cc: Sunil Kamath <sunil.kamath@xxxxxxxxx> > >>Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx> > >>Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@xxxxxxxxx> > >>--- > >> 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. > >>+ */ > >Atm we call this from driver load and resume, which doesn seem to cover > >all the cases you mention in the comment. Should this be a WARN_ON > >instead? Or do we have troubles in our init sequence where we load too > >many times? > > Yes, the above statement taken from bspec to describe about the special cases dmc will not restore the firmware. > Agree, In our cases cold boot and hibernate/suspend mainly we need to load the firmware again, so in my > second sentence I wanted to comment mainly regarding this condition check added for suspend-hibernate(reset) > and cold boot(not loaded). > > Anyways the same api later can be used to load the firmware from anywhere, so my intention to check firmware loaded or not. > If already loaded then not to overwrite the csr address space to maintain the dc5/dc6 counter value. > > Can the below comment more clear to you. > > /* > * Dmc will restore the csr the program except DC9, cold boot, > * warm reset, PCI function level reset, and hibernate/suspend. > * If firmware is restored by dmc then no need to load again which > * will keep the dc5/dc6 counter exposed by firmware. > */ > > No issue in init sequence. That seems to still cover all the callers of the function afaics - we do pci resets over suspend resume unconditionally. So I still don't understand where exactly we try to load the dmc firmware in i915.ko when it's already loaded. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
