[PATCH] drm/i915/guc: Remove bogus null check

[Andi Shyti]

Hi Rodrigo,

On Thu, Mar 28, 2024 at 09:39:17PM -0400, Rodrigo Vivi wrote:
> On Thu, Mar 28, 2024 at 10:41:55PM +0100, Andi Shyti wrote:
> > 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?!

Still... it's not a bug. The tag "Fixes:" should be used when a
bug is fixed, but not for harmless static analyzer reports.

Besides, if we want to keep the Fixes tag we should also Cc
stable, i guess checkpatch.pl complains about it.

(BTW, Cc'ed in this mail we have the inventor of the tag and he
can confirm after having had his morning coffee :-) ).

> > > Reported-by: kernel test robot <lkp at intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter at linaro.org>
> > > Closes: https://lore.kernel.org/r/202403101225.7AheJhZJ-lkp@intel.com/
> > > Cc: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> > > Cc: John Harrison <John.C.Harrison at Intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > ---
> > >  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.

this is what I meant...

> But looking to the 2 places where this function is getting called,
> we already have ce->something used.

... and I also checked where the function is called and how it's
called: I see that if we get here then for sure 'ce' is not NULL.
But for robustness we could still keep it.

Either way I think your patch is good except for the "Fixes:"
tag:

Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>

Thanks,
Andi

> 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