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

["Chauhan, Shekhar" <shekhar.chauhan@intel.com>]

On 10/8/2024 23:36, Matt Roper wrote:
> 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@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.
Got it.
>
>>>> 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@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.
Okay. Will keep this in mind!
>
>
> Matt
>
>>>
>>> Matt
>>>
>>>> Reviewed-by: Shekhar Chauhan <shekhar.chauhan@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@intel.com>
>>>>> Signed-off-by: Haridhar Kalvala <haridhar.kalvala@intel.com>
>>>>> Signed-off-by: Matt Atwood <matthew.s.atwood@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
>>
-- 
-shekhar