Re: [PATCH 04/12] drm/i915/execlists: Refactor CSB state machine

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-07-01 12:49:48)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > Daniele pointed out that the CSB status information will change with
>> > Tigerlake and suggested that we could rearrange our state machine to
>> > hide the differences in generation. gcc also prefers the explicit state
>> > machine, so make it so:
>> >
>> > process_csb                                 1980    1967     -13
>> >
>> > Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/gt/intel_lrc.c | 64 ++++++++++++++++++++---------
>> >  1 file changed, 44 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > index 471e134de186..953b3938a85f 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > @@ -1279,6 +1279,30 @@ reset_in_progress(const struct intel_engine_execlists *execlists)
>> >       return unlikely(!__tasklet_is_enabled(&execlists->tasklet));
>> >  }
>> >  
>> > +enum csb_step {
>> > +     CSB_NOP,
>> > +     CSB_PROMOTE,
>> > +     CSB_PREEMPT,
>> > +     CSB_COMPLETE,
>> > +};
>> > +
>> > +static inline enum csb_step
>> > +csb_parse(const struct intel_engine_execlists *execlists, const u32 *csb)
>> > +{
>> > +     unsigned int status = *csb;
>> 
>> Could be const u32 aswell (stylistic).
>
> No need to specify here, local register is fine, so left it as natural.
>

In this case the function is small and obvious so that is
why stylistic.

But for more complex one, it takes away reviewers
burden as you can read something as const and
then the complexity tree you need to manage between
your ears shrinks when you read further down.

I can also remember atleast few cases where
it has prevented an unwanted accidental write
into propagating past compiler.

>> Just makes me ponder why you want to read csb in here
>> and not in the callsite.
>
> Whatever gcc prefers when there is multiple csb_parsers. :)

It changes to a better produced code? Surely reason
enough.

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