On Thu, Mar 28, 2024 at 10:41:55PM +0100, Andi Shyti wrote:
> Hi Rodrigo,
>
> On Thu, Mar 28, 2024 at 05:31:07PM -0400, Rodrigo Vivi wrote:
> > This null check is bogus because we are already using 'ce' stuff
> > in many places before this function is called.
> >
> > Having this here is useless and confuses static analyzer tools
> > that can see:
> >
> > struct intel_engine_cs *engine = ce->engine;
> >
> > before this check, in the same function.
> >
> > Fixes: cec82816d0d0 ("drm/i915/guc: Use context hints for GT frequency")
>
> there is no need to have the Fixes tag here.
why not? I imagine distros that have this commit cec82816d0d0 and use
static analyzers would also want this patch ported to silent those, no?!
>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > Closes: https://lore.kernel.org/r/202403101225.7AheJhZJ-lkp@xxxxxxxxx/
> > Cc: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> > Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index 01d0ec1b30f2..24a82616f844 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -2677,7 +2677,7 @@ static int guc_context_policy_init_v70(struct intel_context *ce, bool loop)
> > execution_quantum = engine->props.timeslice_duration_ms * 1000;
> > preemption_timeout = engine->props.preempt_timeout_ms * 1000;
> >
> > - if (ce && (ce->flags & BIT(CONTEXT_LOW_LATENCY)))
> > + if (ce->flags & BIT(CONTEXT_LOW_LATENCY))
>
> We could keep the check but make it earlier.
yes, that's another alternative.
-struct intel_engine_cs *engine = ce->engine;
+struct intel_engine_cs *engine;
if (!ce)
return;
engine = ce->engine.
But looking to the 2 places where this function is getting called,
we already have ce->something used.
I can make the change to be like that if you believe that there's
a possibility in the future that we change that, just to be on
the safe side.
or anything else I might be missing?
Thanks for looking into this,
Rodrigo.
>
> Thanks,
> Andi
>
> > slpc_ctx_freq_req |= SLPC_CTX_FREQ_REQ_IS_COMPUTE;
> >
> > __guc_context_policy_start_klv(&policy, ce->guc_id.id);
> > --
> > 2.44.0
