Re: [PATCH 04/11] drm/i915/guc: Sanitize module parameter guc_log_level

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 18/10/2017 16:50, Sagar Arun Kamble wrote:
> On 10/18/2017 6:29 PM, Tvrtko Ursulin wrote:
> > On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> > > Parameter guc_log_level needs to be sanitized based on GuC support and
> > > enable_guc_loading parameter since it depends on them like
> > > enable_guc_submission. This will make GuC logging paths independent of
> > > enable_guc_submission parameter in further patches.
> > >
> > > v2: Added documentation to intel_guc_log.c and param description about
> > > GuC loading dependency. (Michal Wajdeczko)
> > >
> > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/i915/i915_params.c   |  4 +++-
> > >   drivers/gpu/drm/i915/intel_guc_log.c |  1 +
> > >   drivers/gpu/drm/i915/intel_uc.c      | 10 +++++++---
> > >   3 files changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_params.c 
> > > b/drivers/gpu/drm/i915/i915_params.c
> > > index b4faeb6..774c56e 100644
> > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = {
> > >       "(-1=auto, 0=never [default], 1=if available, 2=required)");
> > >     i915_param_named(guc_log_level, int, 0400,
> > > -    "GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
> > > +    "GuC firmware logging level. This takes effect only if GuC is to 
> > > be "
> > > +    "loaded (depends on enable_guc_loading) (-1:disabled (default), "
> > > +    "0-3:enabled)");
> > >     i915_param_named_unsafe(guc_firmware_path, charp, 0400,
> > >       "GuC firmware path to use instead of the default one");
> > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
> > > b/drivers/gpu/drm/i915/intel_guc_log.c
> > > index f53c663..200f0a1 100644
> > > --- a/drivers/gpu/drm/i915/intel_guc_log.c
> > > +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> > > @@ -34,6 +34,7 @@
> > >    * DOC: GuC firmware log
> > >    *
> > >    * Firmware log is enabled by setting i915.guc_log_level to 
> > > non-negative level.
> > > + * This takes effect only if GuC is to be loaded based on 
> > > enable_guc_loading.
> > >    * Log data is printed out via reading debugfs i915_guc_log_dump. 
> > > Reading from
> > >    * i915_guc_load_status will print out firmware loading status and 
> > > scratch
> > >    * registers value.
> > > diff --git a/drivers/gpu/drm/i915/intel_uc.c 
> > > b/drivers/gpu/drm/i915/intel_uc.c
> > > index 62738ad..8feefcd 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.c
> > > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > > @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct 
> > > drm_i915_private *dev_priv)
> > >   {
> > >       if (!HAS_GUC(dev_priv)) {
> > >           if (i915_modparams.enable_guc_loading > 0 ||
> > > -            i915_modparams.enable_guc_submission > 0)
> > > +            i915_modparams.enable_guc_submission > 0 ||
> > > +            i915_modparams.guc_log_level > 0)
> > >               DRM_INFO("Ignoring GuC options, no hardware\n");
> >
> > Hm, this won't fire on <=gen8 once enable_guc_submission starts to 
> > default to one? Or we can worry about that when we get there...
> This will fire for <=gen8 for +ve values which is expected.
> > > i915_modparams.enable_guc_loading = 0;
> > >           i915_modparams.enable_guc_submission = 0;
> > > +        i915_modparams.guc_log_level = -1;
> > >           return;
> > >       }
> > >   @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct 
> > > drm_i915_private *dev_priv)
> > >               i915_modparams.enable_guc_loading = 0;
> > >       }
> > >   -    /* Can't enable guc submission without guc loaded */
> > > -    if (!i915_modparams.enable_guc_loading)
> > > +    /* Can't enable guc submission and logging without guc loaded */
> > > +    if (!i915_modparams.enable_guc_loading) {
> > >           i915_modparams.enable_guc_submission = 0;
> > > +        i915_modparams.guc_log_level = -1;
> >
> > Hm2, how does this interact with the changes which are removing 
> > enable_guc_loading? Or it is a race?
> It interacts. Will race. I think I should pause this series till the 
> other one gets merged.

Okay, it's your call (as in you guys who are refactoring GuC heavily at 
the moment).

Should I continue to the end of this one then? It is not that big so I 
just as well can.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx