Re: [Intel-gfx] [PATCH v2 3/9] drm/i915/dg2: Add DG2_NB_MBD subplatform

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 16 Jun 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 16/06/2022 15:15, Jani Nikula wrote:
>> On Thu, 16 Jun 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> On 16/06/2022 13:01, Anshuman Gupta wrote:
>>>> DG2 NB SKU need to distinguish between MBD and AIC to probe
>>>> the VRAM Self Refresh feature support. Adding those sub platform
>>>> accordingly.
>>>>
>>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h          |  3 +++
>>>>    drivers/gpu/drm/i915/intel_device_info.c | 21 +++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_device_info.h | 11 +++++++----
>>>>    include/drm/i915_pciids.h                | 23 ++++++++++++++++-------
>>>>    4 files changed, 47 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index a5bc6a774c5a..f1f8699eedfd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1007,10 +1007,13 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>>>>    #define IS_PONTEVECCHIO(dev_priv) IS_PLATFORM(dev_priv, INTEL_PONTEVECCHIO)
>>>>    
>>>>    #define IS_DG2_G10(dev_priv) \
>>>> +	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G10_NB_MBD) || \
>>>>    	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G10)
>>>>    #define IS_DG2_G11(dev_priv) \
>>>> +	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G11_NB_MBD) || \
>>>>    	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G11)
>>>>    #define IS_DG2_G12(dev_priv) \
>>>> +	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G12_NB_MBD) || \
>>>>    	IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_G12)
>>>>    #define IS_ADLS_RPLS(dev_priv) \
>>>>    	IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_S, INTEL_SUBPLATFORM_RPL)
>>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
>>>> index f0bf23726ed8..93da555adc4e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>>>> @@ -187,6 +187,18 @@ static const u16 subplatform_rpl_ids[] = {
>>>>    	INTEL_RPLP_IDS(0),
>>>>    };
>>>>    
>>>> +static const u16 subplatform_g10_mb_mbd_ids[] = {
>>>> +	INTEL_DG2_G10_NB_MBD_IDS(0),
>>>> +};
>>>> +
>>>> +static const u16 subplatform_g11_mb_mbd_ids[] = {
>>>> +	INTEL_DG2_G11_NB_MBD_IDS(0),
>>>> +};
>>>> +
>>>> +static const u16 subplatform_g12_mb_mbd_ids[] = {
>>>> +	INTEL_DG2_G12_NB_MBD_IDS(0),
>>>> +};
>>>> +
>>>>    static const u16 subplatform_g10_ids[] = {
>>>>    	INTEL_DG2_G10_IDS(0),
>>>>    	INTEL_ATS_M150_IDS(0),
>>>> @@ -246,6 +258,15 @@ void intel_device_info_subplatform_init(struct drm_i915_private *i915)
>>>>    	} else if (find_devid(devid, subplatform_rpl_ids,
>>>>    			      ARRAY_SIZE(subplatform_rpl_ids))) {
>>>>    		mask = BIT(INTEL_SUBPLATFORM_RPL);
>>>> +	} else if (find_devid(devid, subplatform_g10_mb_mbd_ids,
>>>> +			      ARRAY_SIZE(subplatform_g10_mb_mbd_ids))) {
>>>> +		mask = BIT(INTEL_SUBPLATFORM_G10_NB_MBD);
>>>> +	} else if (find_devid(devid, subplatform_g11_mb_mbd_ids,
>>>> +			      ARRAY_SIZE(subplatform_g11_mb_mbd_ids))) {
>>>> +		mask = BIT(INTEL_SUBPLATFORM_G11_NB_MBD);
>>>> +	} else if (find_devid(devid, subplatform_g12_mb_mbd_ids,
>>>> +			      ARRAY_SIZE(subplatform_g12_mb_mbd_ids))) {
>>>> +		mask = BIT(INTEL_SUBPLATFORM_G12_NB_MBD);
>>>>    	} else if (find_devid(devid, subplatform_g10_ids,
>>>>    			      ARRAY_SIZE(subplatform_g10_ids))) {
>>>>    		mask = BIT(INTEL_SUBPLATFORM_G10);
>>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
>>>> index 08341174ee0a..c929e2d7e59c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>>>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>>>> @@ -97,7 +97,7 @@ enum intel_platform {
>>>>     * it is fine for the same bit to be used on multiple parent platforms.
>>>>     */
>>>>    
>>>> -#define INTEL_SUBPLATFORM_BITS (3)
>>>> +#define INTEL_SUBPLATFORM_BITS (6)
>>>>    #define INTEL_SUBPLATFORM_MASK (BIT(INTEL_SUBPLATFORM_BITS) - 1)
>>>>    
>>>>    /* HSW/BDW/SKL/KBL/CFL */
>>>> @@ -111,9 +111,12 @@ enum intel_platform {
>>>>    #define INTEL_SUBPLATFORM_UY	(0)
>>>>    
>>>>    /* DG2 */
>>>> -#define INTEL_SUBPLATFORM_G10	0
>>>> -#define INTEL_SUBPLATFORM_G11	1
>>>> -#define INTEL_SUBPLATFORM_G12	2
>>>> +#define INTEL_SUBPLATFORM_G10_NB_MBD	0
>>>> +#define INTEL_SUBPLATFORM_G11_NB_MBD	1
>>>> +#define INTEL_SUBPLATFORM_G12_NB_MBD	2
>>>> +#define INTEL_SUBPLATFORM_G10	3
>>>> +#define INTEL_SUBPLATFORM_G11	4
>>>> +#define INTEL_SUBPLATFORM_G12	5
>>>
>>> Ugh I feel this "breaks" the subplatform idea.. feels like it is just
>>> too many bits when two separate sets of information get tracked (Gxx
>>> plus MBD).
>> 
>> I think they could be specified independent of each other, though. The
>> subplatform if-else ladder would have to be replaced with independent
>> ifs. You'd have the G10/G11/G12 and 1 bit separately for MBD.
>> 
>> Only the macros for PCI IDs need to be separate (MBD vs not). You'll
>> then have:
>> 
>> static const u16 subplatform_g10_ids[] = {
>> 	INTEL_DG2_G10_IDS(0),
>> 	INTEL_DG2_G10_NB_MBD_IDS(0),
>> 	INTEL_ATS_M150_IDS(0),
>> };
>> 
>> Ditto for g11 and g12, and separately:
>> 
>> static const u16 subplatform_mbd_ids[] = {
>> 	INTEL_DG2_G10_NB_MBD_IDS(0),
>> 	INTEL_DG2_G11_NB_MBD_IDS(0),
>> 	INTEL_DG2_G12_NB_MBD_IDS(0),
>> };
>> 
>> The IS_DG2_G10() etc. macros would remain unchanged. IS_DG2_MBD() would
>> only be IS_SUBPLATFORM(dev_priv, INTEL_DG2, INTEL_SUBPLATFORM_MBD).
>> 
>> Main point is, a platform could belong to multiple independent
>> subplatforms.
>> 
>> Unless I'm missing something. ;)
>> 
>>> How about a separate "is_mbd" flag in runtime_info? You can split the
>>> PCI IDs split as you have done, but do a search against the MBD ones and
>>> set the flag.
>> 
>> What I dislike about this is that it's really not *runtime* info in any
>> sense, and it adds another way to define platform features. And we
>> already have too many.
>
> I was reluctant to suggest extending usage of subplatform bits in this 
> way but it would be acceptable. My reservation/uncertainty was whether 
> MBP is a "proper" subplatform. I see it's separate PCI IDs and even 
> separate HW features, as seen in this series, but wasn't sure. Anyway, 
> your proposal works for me. Better 4 bits than 6 so as much as possible 
> remain for platform bits.

The alternative is separate struct intel_device_info with a static
is_mbd flag. But the duplication there is also getting out of hands. C
is really crap at this.

BR,
Jani.

>
> Regards,
>
> Tvrtko
>
>> 
>> BR,
>> Jani.
>> 
>> 
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>    
>>>>    /* ADL */
>>>>    #define INTEL_SUBPLATFORM_RPL	0
>>>> diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
>>>> index 4585fed4e41e..198be417bb2d 100644
>>>> --- a/include/drm/i915_pciids.h
>>>> +++ b/include/drm/i915_pciids.h
>>>> @@ -693,32 +693,41 @@
>>>>    	INTEL_VGA_DEVICE(0xA7A9, info)
>>>>    
>>>>    /* DG2 */
>>>> -#define INTEL_DG2_G10_IDS(info) \
>>>> +#define INTEL_DG2_G10_NB_MBD_IDS(info) \
>>>>    	INTEL_VGA_DEVICE(0x5690, info), \
>>>>    	INTEL_VGA_DEVICE(0x5691, info), \
>>>> -	INTEL_VGA_DEVICE(0x5692, info), \
>>>> +	INTEL_VGA_DEVICE(0x5692, info)
>>>> +
>>>> +#define INTEL_DG2_G11_NB_MBD_IDS(info) \
>>>> +	INTEL_VGA_DEVICE(0x5693, info), \
>>>> +	INTEL_VGA_DEVICE(0x5694, info), \
>>>> +	INTEL_VGA_DEVICE(0x5695, info)
>>>> +
>>>> +#define INTEL_DG2_G12_NB_MBD_IDS(info) \
>>>> +	INTEL_VGA_DEVICE(0x5696, info), \
>>>> +	INTEL_VGA_DEVICE(0x5697, info)
>>>> +
>>>> +#define INTEL_DG2_G10_IDS(info) \
>>>>    	INTEL_VGA_DEVICE(0x56A0, info), \
>>>>    	INTEL_VGA_DEVICE(0x56A1, info), \
>>>>    	INTEL_VGA_DEVICE(0x56A2, info)
>>>>    
>>>>    #define INTEL_DG2_G11_IDS(info) \
>>>> -	INTEL_VGA_DEVICE(0x5693, info), \
>>>> -	INTEL_VGA_DEVICE(0x5694, info), \
>>>> -	INTEL_VGA_DEVICE(0x5695, info), \
>>>>    	INTEL_VGA_DEVICE(0x56A5, info), \
>>>>    	INTEL_VGA_DEVICE(0x56A6, info), \
>>>>    	INTEL_VGA_DEVICE(0x56B0, info), \
>>>>    	INTEL_VGA_DEVICE(0x56B1, info)
>>>>    
>>>>    #define INTEL_DG2_G12_IDS(info) \
>>>> -	INTEL_VGA_DEVICE(0x5696, info), \
>>>> -	INTEL_VGA_DEVICE(0x5697, info), \
>>>>    	INTEL_VGA_DEVICE(0x56A3, info), \
>>>>    	INTEL_VGA_DEVICE(0x56A4, info), \
>>>>    	INTEL_VGA_DEVICE(0x56B2, info), \
>>>>    	INTEL_VGA_DEVICE(0x56B3, info)
>>>>    
>>>>    #define INTEL_DG2_IDS(info) \
>>>> +	INTEL_DG2_G10_NB_MBD_IDS(info), \
>>>> +	INTEL_DG2_G11_NB_MBD_IDS(info), \
>>>> +	INTEL_DG2_G12_NB_MBD_IDS(info), \
>>>>    	INTEL_DG2_G10_IDS(info), \
>>>>    	INTEL_DG2_G11_IDS(info), \
>>>>    	INTEL_DG2_G12_IDS(info)
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center