[Intel-gfx] [PATCH v1 1/1] drm/i915: Skip applying copy engine fuses

[Lucas De Marchi]

On Mon, Sep 12, 2022 at 10:12:57PM +0200, Andi Shyti wrote:
>Hi Lucas,
>
>On Mon, Sep 12, 2022 at 11:12:47AM -0700, Lucas De Marchi wrote:
>> On Mon, Sep 12, 2022 at 06:59:53PM +0200, Andi Shyti wrote:
>> > Hi Lucas,
>> >
>> > On Mon, Sep 12, 2022 at 09:19:38AM -0700, Lucas De Marchi wrote:
>> > > Support for reading the fuses to check what are the Link Copy engines
>> > > was added in commit ad5f74f34201 ("drm/i915/pvc: read fuses for link
>> > > copy engines"). However they were added unconditionally because the
>> > > FUSE3 register is present since graphics version 10.
>> > >
>> > > However the bitfield with meml3 fuses only exists since graphics version
>> > > 12. Moreover, Link Copy engines are currently only available in PVC.
>> > > Tying additional copy engines to the meml3 fuses is not correct for
>> > > other platforms.
>> > >
>> > > Make sure there is a check for  `12.60 <= ver < 12.70`. Later platforms
>> > > may extend this function later if it's needed to fuse off copy engines.
>> > >
>> > > Currently it's harmless as the Link Copy engines are still not exported:
>> > > info->engine_mask only has BCS0 set and the register is only read for
>> > > platforms that do have it.
>> > >
>> > > Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> > > index 814f83b5fe59..1f7188129cd1 100644
>> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> > > @@ -764,6 +764,10 @@ static void engine_mask_apply_copy_fuses(struct intel_gt *gt)
>> > >  	unsigned long meml3_mask;
>> > >  	unsigned long quad;
>> > >
>> > > +	if (!(GRAPHICS_VER_FULL(i915) >= IP_VER(12, 60) &&
>> > > +	      GRAPHICS_VER_FULL(i915) < IP_VER(12, 70)))
>> > > +		return;
>> > > +
>> >
>> > Isn't it easier if you wrote
>> >
>> > 	if (GRAPHICS_VER_FULL(i915) < IP_VER(12, 60) ||
>> > 	    GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70))
>> > 		return;
>> >
>> > ?
>> >
>> > You save a parenthesis and a negation '!'.
>>
>> but that makes it wrong. I'd really want the range
>> 12.60 <= version < 12.70, so it applies to PVC  but is then disabled
>> again for MTL.
>
>But it's negated... so that if it's not in the range, it's
>outside of the range... right?
>
> NOT(12.60 <= ver < 12.70)   <--- you wrote
>
>is the same as:
>
> ver < 12.60 or ver >= 12.70 <--- I suggested
>
>and it would mean (just to see if I'm not getting confused by
>something and the negations do always confuse me):
>
>
>                 12.60          12.70
>        return     |              |     return
>  ver: ------------[--------------[---------------
>
>
>But it's the same, taht's why I r-b it anyway.

yeah, when I read your reply I missed the fact you changed the
comparisons <=, >

thanks
Lucas De Marchi