[PATCH 2/6] drm/xe/xe3: Define Xe3 feature flags

[Matt Roper]

On Tue, Oct 08, 2024 at 11:19:55PM +0530, Chauhan, Shekhar wrote:
> 
> On 10/8/2024 21:45, Matt Roper wrote:
> > On Tue, Oct 08, 2024 at 07:53:00AM +0530, Chauhan, Shekhar wrote:
> > > On 10/8/2024 7:05, Matt Atwood wrote:
> > > > From: Haridhar Kalvala <haridhar.kalvala at intel.com>
> > > > 
> > > > Define a common set of Xe3 feature flags and definitions that will be
> > > > used for all platforms in this family.
> > > > 
> > > > The feature flags are inherited unchanged from the Xe2 (XE2_FEATURES)
> > > > platform.
> > > > 
> > > > Following B-spec details inherited from Xe2 feature flag definition
> > > > commit.
> > > > 
> > > > v2: reuse graphics_xe2 defintion
> > > The patch itself LGTM, but since we're re-using Xe3_LPM with media_xe2, I
> > > believe we can have a v3: Reuse media_xe2 definition. Also, there's a typo
> > That wasn't a v3 change; this patch has been using media_xe2 since its
> > original version.
> I meant with the v3 being sent and then there were back and forth changes,
> might as well document them.

Yeah, but the media definition isn't something that ever changed here,
only the graphics definition.  The changelog generally just indicates
what's different since the previous revision of the patch that was sent
out.

> > > in the above graphics_xe2 'definition'.
> > > Both of these are minor things which can be addressed while applying, so,
> > > with that,
> > > 
> > > Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> > I think you copy-pasted my r-b here by accident?
> Ah, nope. Your review did most of the work, mine was an overview.
> Henceforth, I didn't mean to "take" it away. I've done something similar on
> the other patches as well in this series, where most of the review was done
> by you, but then I had a final look over it.
> Let me know if this isn't allowed, i.e., having 2 reviewers for a patch.

Generally every r-b line is an indication that the person has fully
reviewed the patch and approves it for merging.  If someone raised
concerns on an earlier version, but then doesn't have time to re-visit
the updated patch, it's not really appropriate to put their r-b there
since they haven't confirmed that the fixes are correct (credit for
their review feedback is usually given in the change log as "v2: fix foo
(Matt)." It's fine for someone other than the first reviewer to to do
the final review of the patch as you did here, but then only the final
reviewer's r-b should be included unless the first reviewer also has
time to come back and check it themselves too.


Matt

> > 
> > 
> > Matt
> > 
> > > Reviewed-by: Shekhar Chauhan <shekhar.chauhan at intel.com>
> > > 
> > > > Bspec: 58695
> > > >    - dma_mask_size remains 46   (not documented in bspec)
> > > >    - supports_usm=1             (Bspec 59651)
> > > >    - has_flatccs=1              (Bspec 58797)
> > > >    - has_4tile=1                (Bspec 58788)
> > > >    - has_asid=1                 (Bspec 59654, 59265, 60288)
> > > >    - has_range_tlb_invalidate=1 (Bspec 71126)
> > > >    - five-level page table      (Bspec 59505)
> > > >    - 1 VD + 1 VE + 1 SFC        (Bspec 67103, 70819)
> > > >    - platform engine mask       (Bspec 60149)
> > > > 
> > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > Signed-off-by: Haridhar Kalvala <haridhar.kalvala at intel.com>
> > > > Signed-off-by: Matt Atwood <matthew.s.atwood at intel.com>
> > > > ---
> > > >    drivers/gpu/drm/xe/xe_pci.c | 5 ++++-
> > > >    1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > > index 7ffee06fab13..2139edba9062 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > > @@ -208,7 +208,7 @@ static const struct xe_media_desc media_xelpmp = {
> > > >    };
> > > >    static const struct xe_media_desc media_xe2 = {
> > > > -	.name = "Xe2_LPM / Xe2_HPM",
> > > > +	.name = "Xe2_LPM / Xe2_HPM / Xe3_LPM",
> > > >    	.hw_engine_mask =
> > > >    		GENMASK(XE_HW_ENGINE_VCS7, XE_HW_ENGINE_VCS0) |
> > > >    		GENMASK(XE_HW_ENGINE_VECS3, XE_HW_ENGINE_VECS0) |
> > > > @@ -360,6 +360,8 @@ static const struct gmdid_map graphics_ip_map[] = {
> > > >    	{ 1274, &graphics_xelpg },	/* Xe_LPG+ */
> > > >    	{ 2001, &graphics_xe2 },
> > > >    	{ 2004, &graphics_xe2 },
> > > > +	{ 3000, &graphics_xe2 },
> > > > +	{ 3001, &graphics_xe2 },
> > > >    };
> > > >    /* Map of GMD_ID values to media IP */
> > > > @@ -367,6 +369,7 @@ static const struct gmdid_map media_ip_map[] = {
> > > >    	{ 1300, &media_xelpmp },
> > > >    	{ 1301, &media_xe2 },
> > > >    	{ 2000, &media_xe2 },
> > > > +	{ 3000, &media_xe2 },
> > > >    };
> > > >    #define INTEL_VGA_DEVICE(id, info) {			\
> > > -- 
> > > -shekhar
> > > 
> -- 
> -shekhar
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation