| Message ID | 20180227125230.13000-7-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2/27/2018 6:22 PM, Michał Winiarski wrote: > Having both guc_flush_logs and guc_log_flush functions is confusing. > While we could just rename things, guc_flush_logs implementation is > quite simple. Let's get rid of it and move its content to unregister. > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> this patch reminds me of need to do guc_log_flush in capture_uc_state prior to reset in case user wants to read updated log vma from error state. is this need valid? > --- > drivers/gpu/drm/i915/intel_guc_log.c | 35 +++++++++++++++-------------------- > 1 file changed, 15 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > index cbbdb400fa17..0800c5317510 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -443,25 +443,6 @@ static void guc_log_capture_logs(struct intel_guc *guc) > intel_runtime_pm_put(dev_priv); > } > > -static void guc_flush_logs(struct intel_guc *guc) > -{ > - struct drm_i915_private *dev_priv = guc_to_i915(guc); > - > - /* > - * Before initiating the forceful flush, wait for any pending/ongoing > - * flush to complete otherwise forceful flush may not actually happen. > - */ > - flush_work(&guc->log.runtime.flush_work); > - > - /* Ask GuC to update the log buffer state */ > - intel_runtime_pm_get(dev_priv); > - guc_log_flush(guc); > - intel_runtime_pm_put(dev_priv); > - > - /* GuC would have updated log buffer by now, so capture it */ > - guc_log_capture_logs(guc); > -} > - > int intel_guc_log_create(struct intel_guc *guc) > { > struct i915_vma *vma; > @@ -628,14 +609,28 @@ int intel_guc_log_register(struct intel_guc *guc) > > void intel_guc_log_unregister(struct intel_guc *guc) > { > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > + > guc_log_flush_irq_disable(guc); > + > + /* > + * Before initiating the forceful flush, wait for any pending/ongoing > + * flush to complete otherwise forceful flush may not actually happen. > + */ > + flush_work(&guc->log.runtime.flush_work); > + > /* > * Once logging is disabled, GuC won't generate logs & send an > * interrupt. But there could be some data in the log buffer > * which is yet to be captured. So request GuC to update the log > * buffer state and then collect the left over logs. > */ > - guc_flush_logs(guc); > + intel_runtime_pm_get(dev_priv); > + guc_log_flush(guc); > + intel_runtime_pm_put(dev_priv); > + > + /* GuC would have updated log buffer by now, so capture it */ > + guc_log_capture_logs(guc); > > mutex_lock(&guc->log.runtime.lock); >
On Mon, Mar 05, 2018 at 05:28:33PM +0530, Sagar Arun Kamble wrote: > > > On 2/27/2018 6:22 PM, Michał Winiarski wrote: > > Having both guc_flush_logs and guc_log_flush functions is confusing. > > While we could just rename things, guc_flush_logs implementation is > > quite simple. Let's get rid of it and move its content to unregister. > > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > this patch reminds me of need to do guc_log_flush in capture_uc_state prior > to reset in case user wants to read > updated log vma from error state. is this need valid? I don't think so - force flush Host to GuC is related to "bookkeeping" (think metadata describing log buffer state - HEAD/TAIL pointers etc). In error state we don't care - we're dumping the whole log vma. In case of relay - we're moving data around, and we need to know what data to move. -Michał > > --- > > drivers/gpu/drm/i915/intel_guc_log.c | 35 +++++++++++++++-------------------- > > 1 file changed, 15 insertions(+), 20 deletions(-) > >
On 3/5/2018 5:40 PM, Michał Winiarski wrote: > On Mon, Mar 05, 2018 at 05:28:33PM +0530, Sagar Arun Kamble wrote: >> >> On 2/27/2018 6:22 PM, Michał Winiarski wrote: >>> Having both guc_flush_logs and guc_log_flush functions is confusing. >>> While we could just rename things, guc_flush_logs implementation is >>> quite simple. Let's get rid of it and move its content to unregister. >>> >>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> >> this patch reminds me of need to do guc_log_flush in capture_uc_state prior >> to reset in case user wants to read >> updated log vma from error state. is this need valid? > I don't think so - force flush Host to GuC is related to "bookkeeping" (think > metadata describing log buffer state - HEAD/TAIL pointers etc). > In error state we don't care - we're dumping the whole log vma. But while decoding from log vma in error state we will need up to date read/write pointers. Or is the error handling logic requires not to invoke any H2G post hang. Agree that this change has to be taken up later based on consensus on requirement but just wanted understand the scenario more. Thanks, Sagar > In case of relay - we're moving data around, and we need to know what data to > move. > > -Michał > >>> --- >>> drivers/gpu/drm/i915/intel_guc_log.c | 35 +++++++++++++++-------------------- >>> 1 file changed, 15 insertions(+), 20 deletions(-) >>>
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index cbbdb400fa17..0800c5317510 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -443,25 +443,6 @@ static void guc_log_capture_logs(struct intel_guc *guc) intel_runtime_pm_put(dev_priv); } -static void guc_flush_logs(struct intel_guc *guc) -{ - struct drm_i915_private *dev_priv = guc_to_i915(guc); - - /* - * Before initiating the forceful flush, wait for any pending/ongoing - * flush to complete otherwise forceful flush may not actually happen. - */ - flush_work(&guc->log.runtime.flush_work); - - /* Ask GuC to update the log buffer state */ - intel_runtime_pm_get(dev_priv); - guc_log_flush(guc); - intel_runtime_pm_put(dev_priv); - - /* GuC would have updated log buffer by now, so capture it */ - guc_log_capture_logs(guc); -} - int intel_guc_log_create(struct intel_guc *guc) { struct i915_vma *vma; @@ -628,14 +609,28 @@ int intel_guc_log_register(struct intel_guc *guc) void intel_guc_log_unregister(struct intel_guc *guc) { + struct drm_i915_private *dev_priv = guc_to_i915(guc); + guc_log_flush_irq_disable(guc); + + /* + * Before initiating the forceful flush, wait for any pending/ongoing + * flush to complete otherwise forceful flush may not actually happen. + */ + flush_work(&guc->log.runtime.flush_work); + /* * Once logging is disabled, GuC won't generate logs & send an * interrupt. But there could be some data in the log buffer * which is yet to be captured. So request GuC to update the log * buffer state and then collect the left over logs. */ - guc_flush_logs(guc); + intel_runtime_pm_get(dev_priv); + guc_log_flush(guc); + intel_runtime_pm_put(dev_priv); + + /* GuC would have updated log buffer by now, so capture it */ + guc_log_capture_logs(guc); mutex_lock(&guc->log.runtime.lock);
Having both guc_flush_logs and guc_log_flush functions is confusing. While we could just rename things, guc_flush_logs implementation is quite simple. Let's get rid of it and move its content to unregister. Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> --- drivers/gpu/drm/i915/intel_guc_log.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-)