Re: [PATCH v2 6/6] drm/i915/uc: do not free err log on uc_fini

[Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>]

On 3/25/20 10:58 AM, John Harrison wrote:
> On 3/11/2020 18:16, Daniele Ceraolo Spurio wrote:
> > we do call uc_fini if there is an issue while loading the GuC, so we
> > can't delete in there the logs we need to debug the load failure.
> > Moving the log free to driver remove ensures the logs stick around ong
> > enough for us to dump them.
> I think this could be worded better and has a couple of typos.

How about:

"We need to keep the GuC error logs around to debug the load failure, so 
we can't clean them in the error unwind, which includes uc_fini(). 
Moving the cleanup to driver remove ensures that the logs stick around 
long enough for us to dump them."

?

Daniele

> Otherwise it looks plausible.
> Reviewed-by: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_gt.c    | 3 +--
> >   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++--
> >   drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 +
> >   3 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
> > b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index 3dea8881e915..eda66b0d44bd 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -635,8 +635,7 @@ void intel_gt_driver_remove(struct intel_gt *gt)
> >   {
> >       __intel_gt_disable(gt);
> > -    intel_uc_fini_hw(&gt->uc);
> > -    intel_uc_fini(&gt->uc);
> > +    intel_uc_driver_remove(&gt->uc);
> >       intel_engines_release(gt);
> >   }
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > index a4cbe06e06bd..b11e564ef22e 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > @@ -131,6 +131,13 @@ static void __uc_free_load_err_log(struct 
> > intel_uc *uc)
> >           i915_gem_object_put(log);
> >   }
> > +void intel_uc_driver_remove(struct intel_uc *uc)
> > +{
> > +    intel_uc_fini_hw(uc);
> > +    intel_uc_fini(uc);
> > +    __uc_free_load_err_log(uc);
> > +}
> > +
> >   static inline bool guc_communication_enabled(struct intel_guc *guc)
> >   {
> >       return intel_guc_ct_enabled(&guc->ct);
> > @@ -311,8 +318,6 @@ static void __uc_fini(struct intel_uc *uc)
> >   {
> >       intel_huc_fini(&uc->huc);
> >       intel_guc_fini(&uc->guc);
> > -
> > -    __uc_free_load_err_log(uc);
> >   }
> >   static int __uc_sanitize(struct intel_uc *uc)
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> > index 5ae7b50b7dc1..9c954c589edf 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> > @@ -34,6 +34,7 @@ struct intel_uc {
> >   void intel_uc_init_early(struct intel_uc *uc);
> >   void intel_uc_driver_late_release(struct intel_uc *uc);
> > +void intel_uc_driver_remove(struct intel_uc *uc);
> >   void intel_uc_init_mmio(struct intel_uc *uc);
> >   void intel_uc_reset_prepare(struct intel_uc *uc);
> >   void intel_uc_suspend(struct intel_uc *uc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx