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

[Lucas De Marchi]

On Mon, Dec 09, 2024 at 06:44:36AM -0800, Lucas De Marchi wrote:
>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>

feel free to use

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

instead of the a-b to merge this.

Lucas De Marchi

>
>Lucas De Marchi