[PATCH 1/2] drm/xe: Always setup GT MMIO adjustment data

[Lucas De Marchi]

On Tue, Dec 03, 2024 at 12:37:16PM -0800, Matt Roper wrote:
>On Tue, Nov 19, 2024 at 02:33:28PM -0600, Lucas De Marchi wrote:
>> On Thu, Nov 14, 2024 at 08:36:46PM +0100, Michal Wajdeczko wrote:
>> >
>> >
>> > On 14.11.2024 20:23, Lucas De Marchi wrote:
>> > > On Thu, Nov 14, 2024 at 06:59:54PM +0100, Michal Wajdeczko wrote:
>> > > > While we believed that xe_gt_mmio_init() will be called just once
>> > > > per GT, this might not be a case due to some tweaks that need to
>> > > > performed by the VF driver during early probe.  To avoid leaving
>> > >
>> > > can you be explicit where this is being set on the VF case?
>> >
>> > it will be used in read_gmdid(), see next patch
>> >
>> > > Shouldn't VF
>> > > itself deal with fixing up what it did?
>> >
>> > in read_gmdid() we already said
>> >
>> > 516      /*
>> > 517       * Only undo xe_gt.info here, the remaining changes made above
>> > 518       * will be overwritten as part of the regular initialization.
>> > 519       */
>> >
>> > and it looks that xe_gt_mmio_init() is not overwriting everything as we
>> > would expect
>>
>> We only need to read the gmdid and for that need the mmio struct to be
>> filled out. We end up needing the gt as a context, but this all looks to
>> be fixing things that shouldn't have been done.
>
>I think the problem is that when you say we "only need to read the
>gmdid" that isn't a simple register read when running on an SRIOV VF.
>Rather it's a handshake and query sent to the GuC, which actually
>involves reading/writing lots of GuC registers to eventually get back
>that single version number we want.

and IMO all the preparation for handling the SR-IOV special case needs
to be explicit in the init sequence of the driver rather than poking
holes like this.

>
>SRIOV has a lot of chicken-and-egg problems; in this case the problem is
>that our driver needs to know what IP version it's running on in order
>to know how to do basic GT initialization.  However at the same time we
>need to have already done basic GT + GuC initialization in order to even
>find out the IP version since the VF doesn't have any direct access to
>the GMD_ID registers.  So we wind up relying on some ugly hacks like
>this that do a partial/fake initialization of the GT (under the
>understanding that a lot of other GT functionality like MCR, forcewake,
>etc. won't get used) to do things "out of order" compared to native
>execution, and then we come back and fix things up later.
>
>I think some of the headaches here are really oversights in the hardware
>design (e.g., why doesn't the VF BAR expose GMD_ID values somewhere so
>that we don't have to go through the GuC?).  But I don't have any good
>ideas for handling things in a cleaner manner, aside from having a
>completely independent .ko with different code flows to run on VFs,
>which I don't think anyone really wants.

I don't think we need a different/independent .ko. We only need the init
sequence to be clearer and do explicit initialization for VF instead of
hacking "the leaf functions".

Anyway, I'm adding my ack here for this to be applied as is, because
fixing it means a lot more refactors.

I think the problem from start was "we want to keep the same driver init
sequence in the VF case", when the HW is not really made to allow the
same init sequence. So why do we keep insisting in doing that?


Acked-by: Lucas De Marchi <lucas.demarchi at intel.com>

Lucas De Marchi