diff mbox series

[01/53] drm/i915: Add "release id" version

Message ID 20210701202427.1547543-2-matthew.d.roper@intel.com (mailing list archive)
State New, archived
Headers show
Series Begin enabling Xe_HP SDV and DG2 platforms | expand

Commit Message

Matt Roper July 1, 2021, 8:23 p.m. UTC 
From: Lucas De Marchi <lucas.demarchi@intel.com>

Besides the arch version returned by GRAPHICS_VER(), new platforms
contain a "release id" to make clear the difference from one platform to
another. Although for the first ones we may use them as if they were a
major/minor version, that is not true for all platforms: we may have a
`release_id == n` that is closer to `n - 2` than to `n - 1`.

However the release id number is not defined by hardware until we start
using the GMD_ID register. For the platforms before that register is
useful we will set the values in software and we can set them as we
please. So the plan is to set them so we can group different features
under a single GRAPHICS_VER_FULL() check.

After GMD_ID is used, the usefulness of a "full version check" will be
greatly reduced and will be mostly used for deciding workarounds and a
few code paths. So it makes sense to keep it as a separate field from
graphics_ver.

Also, currently there is not much use for the release id in media and
display, so keep them out.

This is a mix of 2 independent changes: one by me and the other by Matt
Roper.

Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          | 6 ++++++
 drivers/gpu/drm/i915/intel_device_info.c | 2 ++
 drivers/gpu/drm/i915/intel_device_info.h | 2 ++
 3 files changed, 10 insertions(+)

Comments

Tvrtko Ursulin July 2, 2021, 12:33 p.m. UTC | #1 
On 01/07/2021 21:23, Matt Roper wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Besides the arch version returned by GRAPHICS_VER(), new platforms
> contain a "release id" to make clear the difference from one platform to
> another. Although for the first ones we may use them as if they were a

What does "first ones" refer to here?

> major/minor version, that is not true for all platforms: we may have a
> `release_id == n` that is closer to `n - 2` than to `n - 1`.

Hm this is a bit confusing. Is the sentence simply trying to say that, 
as the release id number is growing, hw capabilities are not simply 
accumulating but can be removed as well? Otherwise I am not sure how the 
user of these macros is supposed to act on this sentence.

> However the release id number is not defined by hardware until we start
> using the GMD_ID register. For the platforms before that register is
> useful we will set the values in software and we can set them as we
> please. So the plan is to set them so we can group different features
> under a single GRAPHICS_VER_FULL() check.
> 
> After GMD_ID is used, the usefulness of a "full version check" will be
> greatly reduced and will be mostly used for deciding workarounds and a
> few code paths. So it makes sense to keep it as a separate field from
> graphics_ver.
> 
> Also, currently there is not much use for the release id in media and
> display, so keep them out.
> 
> This is a mix of 2 independent changes: one by me and the other by Matt
> Roper.
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h          | 6 ++++++
>   drivers/gpu/drm/i915/intel_device_info.c | 2 ++
>   drivers/gpu/drm/i915/intel_device_info.h | 2 ++
>   3 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6dff4ca01241..9639800485b9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1258,11 +1258,17 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev)
>    */
>   #define IS_GEN(dev_priv, n)		(GRAPHICS_VER(dev_priv) == (n))
>   
> +#define IP_VER(ver, release)		((ver) << 8 | (release))
> +
>   #define GRAPHICS_VER(i915)		(INTEL_INFO(i915)->graphics_ver)
> +#define GRAPHICS_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->graphics_ver, \
> +					       INTEL_INFO(i915)->graphics_ver_release)
>   #define IS_GRAPHICS_VER(i915, from, until) \
>   	(GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= (until))
>   
>   #define MEDIA_VER(i915)			(INTEL_INFO(i915)->media_ver)
> +#define MEDIA_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->media_ver, \
> +					       INTEL_INFO(i915)->media_ver_release)
>   #define IS_MEDIA_VER(i915, from, until) \
>   	(MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until))
>   
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 7eaa92fee421..e8ad14f002c1 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -97,7 +97,9 @@ void intel_device_info_print_static(const struct intel_device_info *info,
>   				    struct drm_printer *p)
>   {
>   	drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);
> +	drm_printf(p, "graphics_ver_release: %u\n", info->graphics_ver_release);

I get the VER and VER_FULL in the macros but could 'ver' and 
'ver_release' here and in the code simply be renamed to 'ver'/'version' 
and 'release'? Maybe it is just me but don't think I encountered the 
term "version release" before.

Regards,

Tvrtko

>   	drm_printf(p, "media_ver: %u\n", info->media_ver);
> +	drm_printf(p, "media_ver_release: %u\n", info->media_ver_release);
>   	drm_printf(p, "display_ver: %u\n", info->display.ver);
>   	drm_printf(p, "gt: %d\n", info->gt);
>   	drm_printf(p, "iommu: %s\n", iommu_name());
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index b326aff65cd6..944a5ff4df49 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -162,7 +162,9 @@ enum intel_ppgtt_type {
>   
>   struct intel_device_info {
>   	u8 graphics_ver;
> +	u8 graphics_ver_release;
>   	u8 media_ver;
> +	u8 media_ver_release;
>   
>   	u8 gt; /* GT number, 0 if undefined */
>   	intel_engine_mask_t platform_engine_mask; /* Engines supported by the HW */
>
Jani Nikula July 5, 2021, 11:52 a.m. UTC | #2 
On Fri, 02 Jul 2021, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 01/07/2021 21:23, Matt Roper wrote:
>> From: Lucas De Marchi <lucas.demarchi@intel.com>
>> 
>> Besides the arch version returned by GRAPHICS_VER(), new platforms
>> contain a "release id" to make clear the difference from one platform to
>> another. Although for the first ones we may use them as if they were a
>
> What does "first ones" refer to here?
>
>> major/minor version, that is not true for all platforms: we may have a
>> `release_id == n` that is closer to `n - 2` than to `n - 1`.
>
> Hm this is a bit confusing. Is the sentence simply trying to say that, 
> as the release id number is growing, hw capabilities are not simply 
> accumulating but can be removed as well? Otherwise I am not sure how the 
> user of these macros is supposed to act on this sentence.
>
>> However the release id number is not defined by hardware until we start
>> using the GMD_ID register. For the platforms before that register is
>> useful we will set the values in software and we can set them as we
>> please. So the plan is to set them so we can group different features
>> under a single GRAPHICS_VER_FULL() check.
>> 
>> After GMD_ID is used, the usefulness of a "full version check" will be
>> greatly reduced and will be mostly used for deciding workarounds and a
>> few code paths. So it makes sense to keep it as a separate field from
>> graphics_ver.
>> 
>> Also, currently there is not much use for the release id in media and
>> display, so keep them out.
>> 
>> This is a mix of 2 independent changes: one by me and the other by Matt
>> Roper.
>> 
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h          | 6 ++++++
>>   drivers/gpu/drm/i915/intel_device_info.c | 2 ++
>>   drivers/gpu/drm/i915/intel_device_info.h | 2 ++
>>   3 files changed, 10 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 6dff4ca01241..9639800485b9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1258,11 +1258,17 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev)
>>    */
>>   #define IS_GEN(dev_priv, n)		(GRAPHICS_VER(dev_priv) == (n))
>>   
>> +#define IP_VER(ver, release)		((ver) << 8 | (release))
>> +
>>   #define GRAPHICS_VER(i915)		(INTEL_INFO(i915)->graphics_ver)
>> +#define GRAPHICS_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->graphics_ver, \
>> +					       INTEL_INFO(i915)->graphics_ver_release)
>>   #define IS_GRAPHICS_VER(i915, from, until) \
>>   	(GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= (until))
>>   
>>   #define MEDIA_VER(i915)			(INTEL_INFO(i915)->media_ver)
>> +#define MEDIA_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->media_ver, \
>> +					       INTEL_INFO(i915)->media_ver_release)
>>   #define IS_MEDIA_VER(i915, from, until) \
>>   	(MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until))
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
>> index 7eaa92fee421..e8ad14f002c1 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -97,7 +97,9 @@ void intel_device_info_print_static(const struct intel_device_info *info,
>>   				    struct drm_printer *p)
>>   {
>>   	drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);
>> +	drm_printf(p, "graphics_ver_release: %u\n", info->graphics_ver_release);
>
> I get the VER and VER_FULL in the macros but could 'ver' and 
> 'ver_release' here and in the code simply be renamed to 'ver'/'version' 
> and 'release'? Maybe it is just me but don't think I encountered the 
> term "version release" before.

Just bikeshedding here, but I thought of:

	if (info->grapics_ver_release)
		drm_printf(p, "graphics_ver: %u.%u\n", info->graphics_ver, info->graphics_ver_release);
	else
		drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);

Also, I thought "x_ver" and "x_ver_release" sounds a bit odd, perhaps
having "x_ver" and "x_rel" is more natural?

Ultimately I think we've historically always sucked at trying to figure
this out up front, so maybe the right answer is merging something and
fixing later...


BR,
Jani.





>
> Regards,
>
> Tvrtko
>
>>   	drm_printf(p, "media_ver: %u\n", info->media_ver);
>> +	drm_printf(p, "media_ver_release: %u\n", info->media_ver_release);
>>   	drm_printf(p, "display_ver: %u\n", info->display.ver);
>>   	drm_printf(p, "gt: %d\n", info->gt);
>>   	drm_printf(p, "iommu: %s\n", iommu_name());
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
>> index b326aff65cd6..944a5ff4df49 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>> @@ -162,7 +162,9 @@ enum intel_ppgtt_type {
>>   
>>   struct intel_device_info {
>>   	u8 graphics_ver;
>> +	u8 graphics_ver_release;
>>   	u8 media_ver;
>> +	u8 media_ver_release;
>>   
>>   	u8 gt; /* GT number, 0 if undefined */
>>   	intel_engine_mask_t platform_engine_mask; /* Engines supported by the HW */
>>
Lucas De Marchi July 6, 2021, 8:57 p.m. UTC | #3 
On Fri, Jul 02, 2021 at 01:33:50PM +0100, Tvrtko Ursulin wrote:
>
>On 01/07/2021 21:23, Matt Roper wrote:
>>From: Lucas De Marchi <lucas.demarchi@intel.com>
>>
>>Besides the arch version returned by GRAPHICS_VER(), new platforms
>>contain a "release id" to make clear the difference from one platform to
>>another. Although for the first ones we may use them as if they were a
>
>What does "first ones" refer to here?

XeHP-SDV and DG2. The additional register that commit
01eb15c9165e ("drm/i915: Add DISPLAY_VER() and related macros")
talks about is not available for these platforms. Here we hardcode them
in software to something that makes sense and it allows the driver to
be properly prepared for future platforms.

>
>>major/minor version, that is not true for all platforms: we may have a
>>`release_id == n` that is closer to `n - 2` than to `n - 1`.
>
>Hm this is a bit confusing. Is the sentence simply trying to say that, 
>as the release id number is growing, hw capabilities are not simply 
>accumulating but can be removed as well? Otherwise I am not sure how 
>the user of these macros is supposed to act on this sentence.

this is explaining why those numbers can't be interpreted as
major/minor and hence why here it's called "release" rather than
"minor".

Your interpretation is correct, except that a feature not being there
doesn't necessarily mean it got removed at some point. I might just had
never been in that particular release: i.e. GRAPHICS_VER_FULL() == 14.5
doesn't necessarily mean it comes after 14.3, with additional
features/fixes.

Lucas De Marchi
Lucas De Marchi July 6, 2021, 9:09 p.m. UTC | #4 
On Mon, Jul 05, 2021 at 02:52:31PM +0300, Jani Nikula wrote:
>On Fri, 02 Jul 2021, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 01/07/2021 21:23, Matt Roper wrote:
>>> From: Lucas De Marchi <lucas.demarchi@intel.com>
>>>
>>> Besides the arch version returned by GRAPHICS_VER(), new platforms
>>> contain a "release id" to make clear the difference from one platform to
>>> another. Although for the first ones we may use them as if they were a
>>
>> What does "first ones" refer to here?
>>
>>> major/minor version, that is not true for all platforms: we may have a
>>> `release_id == n` that is closer to `n - 2` than to `n - 1`.
>>
>> Hm this is a bit confusing. Is the sentence simply trying to say that,
>> as the release id number is growing, hw capabilities are not simply
>> accumulating but can be removed as well? Otherwise I am not sure how the
>> user of these macros is supposed to act on this sentence.
>>
>>> However the release id number is not defined by hardware until we start
>>> using the GMD_ID register. For the platforms before that register is
>>> useful we will set the values in software and we can set them as we
>>> please. So the plan is to set them so we can group different features
>>> under a single GRAPHICS_VER_FULL() check.
>>>
>>> After GMD_ID is used, the usefulness of a "full version check" will be
>>> greatly reduced and will be mostly used for deciding workarounds and a
>>> few code paths. So it makes sense to keep it as a separate field from
>>> graphics_ver.
>>>
>>> Also, currently there is not much use for the release id in media and
>>> display, so keep them out.
>>>
>>> This is a mix of 2 independent changes: one by me and the other by Matt
>>> Roper.
>>>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h          | 6 ++++++
>>>   drivers/gpu/drm/i915/intel_device_info.c | 2 ++
>>>   drivers/gpu/drm/i915/intel_device_info.h | 2 ++
>>>   3 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 6dff4ca01241..9639800485b9 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1258,11 +1258,17 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev)
>>>    */
>>>   #define IS_GEN(dev_priv, n)		(GRAPHICS_VER(dev_priv) == (n))
>>>
>>> +#define IP_VER(ver, release)		((ver) << 8 | (release))
>>> +
>>>   #define GRAPHICS_VER(i915)		(INTEL_INFO(i915)->graphics_ver)
>>> +#define GRAPHICS_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->graphics_ver, \
>>> +					       INTEL_INFO(i915)->graphics_ver_release)
>>>   #define IS_GRAPHICS_VER(i915, from, until) \
>>>   	(GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= (until))
>>>
>>>   #define MEDIA_VER(i915)			(INTEL_INFO(i915)->media_ver)
>>> +#define MEDIA_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->media_ver, \
>>> +					       INTEL_INFO(i915)->media_ver_release)
>>>   #define IS_MEDIA_VER(i915, from, until) \
>>>   	(MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until))
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
>>> index 7eaa92fee421..e8ad14f002c1 100644
>>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>>> @@ -97,7 +97,9 @@ void intel_device_info_print_static(const struct intel_device_info *info,
>>>   				    struct drm_printer *p)
>>>   {
>>>   	drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);
>>> +	drm_printf(p, "graphics_ver_release: %u\n", info->graphics_ver_release);
>>
>> I get the VER and VER_FULL in the macros but could 'ver' and
>> 'ver_release' here and in the code simply be renamed to 'ver'/'version'
>> and 'release'? Maybe it is just me but don't think I encountered the
>> term "version release" before.
>
>Just bikeshedding here, but I thought of:
>
>	if (info->grapics_ver_release)
>		drm_printf(p, "graphics_ver: %u.%u\n", info->graphics_ver, info->graphics_ver_release);
>	else
>		drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);

humn... a suggestion that I got internally a few week ago and I forgot
to update this was that this doesn't need to be abbreviated in debugfs
and could very well be:

	drm_printf(p, "graphics version: %u\n", info->graphics_ver);
	drm_printf(p, "graphics release: %u\n", info->graphics_ver_release);
>
>Also, I thought "x_ver" and "x_ver_release" sounds a bit odd, perhaps
>having "x_ver" and "x_rel" is more natural?

Not sure what direction to go now though. Maybe trying to put all
suggestions together:

	if (info->graphics_rel)
		drm_printf(p, "graphics version: %u.%u\n", info->graphics_ver, info->graphics_rel);
	else
		drm_printf(p, "graphics version: %u\n", info->graphics_ver);

One thing  I like is that doing `| grep "graphics version"`
gives all info you are searching for.


thanks
Lucas De Marchi
Jani Nikula July 7, 2021, 8:34 a.m. UTC | #5 
On Tue, 06 Jul 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Mon, Jul 05, 2021 at 02:52:31PM +0300, Jani Nikula wrote:
>>On Fri, 02 Jul 2021, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> On 01/07/2021 21:23, Matt Roper wrote:
>>>> From: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>
>>>> Besides the arch version returned by GRAPHICS_VER(), new platforms
>>>> contain a "release id" to make clear the difference from one platform to
>>>> another. Although for the first ones we may use them as if they were a
>>>
>>> What does "first ones" refer to here?
>>>
>>>> major/minor version, that is not true for all platforms: we may have a
>>>> `release_id == n` that is closer to `n - 2` than to `n - 1`.
>>>
>>> Hm this is a bit confusing. Is the sentence simply trying to say that,
>>> as the release id number is growing, hw capabilities are not simply
>>> accumulating but can be removed as well? Otherwise I am not sure how the
>>> user of these macros is supposed to act on this sentence.
>>>
>>>> However the release id number is not defined by hardware until we start
>>>> using the GMD_ID register. For the platforms before that register is
>>>> useful we will set the values in software and we can set them as we
>>>> please. So the plan is to set them so we can group different features
>>>> under a single GRAPHICS_VER_FULL() check.
>>>>
>>>> After GMD_ID is used, the usefulness of a "full version check" will be
>>>> greatly reduced and will be mostly used for deciding workarounds and a
>>>> few code paths. So it makes sense to keep it as a separate field from
>>>> graphics_ver.
>>>>
>>>> Also, currently there is not much use for the release id in media and
>>>> display, so keep them out.
>>>>
>>>> This is a mix of 2 independent changes: one by me and the other by Matt
>>>> Roper.
>>>>
>>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.h          | 6 ++++++
>>>>   drivers/gpu/drm/i915/intel_device_info.c | 2 ++
>>>>   drivers/gpu/drm/i915/intel_device_info.h | 2 ++
>>>>   3 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 6dff4ca01241..9639800485b9 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1258,11 +1258,17 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev)
>>>>    */
>>>>   #define IS_GEN(dev_priv, n)		(GRAPHICS_VER(dev_priv) == (n))
>>>>
>>>> +#define IP_VER(ver, release)		((ver) << 8 | (release))
>>>> +
>>>>   #define GRAPHICS_VER(i915)		(INTEL_INFO(i915)->graphics_ver)
>>>> +#define GRAPHICS_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->graphics_ver, \
>>>> +					       INTEL_INFO(i915)->graphics_ver_release)
>>>>   #define IS_GRAPHICS_VER(i915, from, until) \
>>>>   	(GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= (until))
>>>>
>>>>   #define MEDIA_VER(i915)			(INTEL_INFO(i915)->media_ver)
>>>> +#define MEDIA_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->media_ver, \
>>>> +					       INTEL_INFO(i915)->media_ver_release)
>>>>   #define IS_MEDIA_VER(i915, from, until) \
>>>>   	(MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until))
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
>>>> index 7eaa92fee421..e8ad14f002c1 100644
>>>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>>>> @@ -97,7 +97,9 @@ void intel_device_info_print_static(const struct intel_device_info *info,
>>>>   				    struct drm_printer *p)
>>>>   {
>>>>   	drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);
>>>> +	drm_printf(p, "graphics_ver_release: %u\n", info->graphics_ver_release);
>>>
>>> I get the VER and VER_FULL in the macros but could 'ver' and
>>> 'ver_release' here and in the code simply be renamed to 'ver'/'version'
>>> and 'release'? Maybe it is just me but don't think I encountered the
>>> term "version release" before.
>>
>>Just bikeshedding here, but I thought of:
>>
>>	if (info->grapics_ver_release)
>>		drm_printf(p, "graphics_ver: %u.%u\n", info->graphics_ver, info->graphics_ver_release);
>>	else
>>		drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);
>
> humn... a suggestion that I got internally a few week ago and I forgot
> to update this was that this doesn't need to be abbreviated in debugfs
> and could very well be:
>
> 	drm_printf(p, "graphics version: %u\n", info->graphics_ver);
> 	drm_printf(p, "graphics release: %u\n", info->graphics_ver_release);
>>
>>Also, I thought "x_ver" and "x_ver_release" sounds a bit odd, perhaps
>>having "x_ver" and "x_rel" is more natural?
>
> Not sure what direction to go now though. Maybe trying to put all
> suggestions together:
>
> 	if (info->graphics_rel)
> 		drm_printf(p, "graphics version: %u.%u\n", info->graphics_ver, info->graphics_rel);
> 	else
> 		drm_printf(p, "graphics version: %u\n", info->graphics_ver);
>
> One thing  I like is that doing `| grep "graphics version"`
> gives all info you are searching for.

I'd like that, but this is all bikeshedding, really.

BR,
Jani.

>
>
> thanks
> Lucas De Marchi
Lucas De Marchi July 7, 2021, 3:40 p.m. UTC | #6 
On Wed, Jul 07, 2021 at 11:34:36AM +0300, Jani Nikula wrote:
>On Tue, 06 Jul 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> On Mon, Jul 05, 2021 at 02:52:31PM +0300, Jani Nikula wrote:
>>>On Fri, 02 Jul 2021, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>>> On 01/07/2021 21:23, Matt Roper wrote:
>>>>> From: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>>
>>>>> Besides the arch version returned by GRAPHICS_VER(), new platforms
>>>>> contain a "release id" to make clear the difference from one platform to
>>>>> another. Although for the first ones we may use them as if they were a
>>>>
>>>> What does "first ones" refer to here?
>>>>
>>>>> major/minor version, that is not true for all platforms: we may have a
>>>>> `release_id == n` that is closer to `n - 2` than to `n - 1`.
>>>>
>>>> Hm this is a bit confusing. Is the sentence simply trying to say that,
>>>> as the release id number is growing, hw capabilities are not simply
>>>> accumulating but can be removed as well? Otherwise I am not sure how the
>>>> user of these macros is supposed to act on this sentence.
>>>>
>>>>> However the release id number is not defined by hardware until we start
>>>>> using the GMD_ID register. For the platforms before that register is
>>>>> useful we will set the values in software and we can set them as we
>>>>> please. So the plan is to set them so we can group different features
>>>>> under a single GRAPHICS_VER_FULL() check.
>>>>>
>>>>> After GMD_ID is used, the usefulness of a "full version check" will be
>>>>> greatly reduced and will be mostly used for deciding workarounds and a
>>>>> few code paths. So it makes sense to keep it as a separate field from
>>>>> graphics_ver.
>>>>>
>>>>> Also, currently there is not much use for the release id in media and
>>>>> display, so keep them out.
>>>>>
>>>>> This is a mix of 2 independent changes: one by me and the other by Matt
>>>>> Roper.
>>>>>
>>>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_drv.h          | 6 ++++++
>>>>>   drivers/gpu/drm/i915/intel_device_info.c | 2 ++
>>>>>   drivers/gpu/drm/i915/intel_device_info.h | 2 ++
>>>>>   3 files changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index 6dff4ca01241..9639800485b9 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -1258,11 +1258,17 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev)
>>>>>    */
>>>>>   #define IS_GEN(dev_priv, n)		(GRAPHICS_VER(dev_priv) == (n))
>>>>>
>>>>> +#define IP_VER(ver, release)		((ver) << 8 | (release))
>>>>> +
>>>>>   #define GRAPHICS_VER(i915)		(INTEL_INFO(i915)->graphics_ver)
>>>>> +#define GRAPHICS_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->graphics_ver, \
>>>>> +					       INTEL_INFO(i915)->graphics_ver_release)
>>>>>   #define IS_GRAPHICS_VER(i915, from, until) \
>>>>>   	(GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= (until))
>>>>>
>>>>>   #define MEDIA_VER(i915)			(INTEL_INFO(i915)->media_ver)
>>>>> +#define MEDIA_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->media_ver, \
>>>>> +					       INTEL_INFO(i915)->media_ver_release)
>>>>>   #define IS_MEDIA_VER(i915, from, until) \
>>>>>   	(MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until))
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
>>>>> index 7eaa92fee421..e8ad14f002c1 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>>>>> @@ -97,7 +97,9 @@ void intel_device_info_print_static(const struct intel_device_info *info,
>>>>>   				    struct drm_printer *p)
>>>>>   {
>>>>>   	drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);
>>>>> +	drm_printf(p, "graphics_ver_release: %u\n", info->graphics_ver_release);
>>>>
>>>> I get the VER and VER_FULL in the macros but could 'ver' and
>>>> 'ver_release' here and in the code simply be renamed to 'ver'/'version'
>>>> and 'release'? Maybe it is just me but don't think I encountered the
>>>> term "version release" before.
>>>
>>>Just bikeshedding here, but I thought of:
>>>
>>>	if (info->grapics_ver_release)
>>>		drm_printf(p, "graphics_ver: %u.%u\n", info->graphics_ver, info->graphics_ver_release);
>>>	else
>>>		drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);
>>
>> humn... a suggestion that I got internally a few week ago and I forgot
>> to update this was that this doesn't need to be abbreviated in debugfs
>> and could very well be:
>>
>> 	drm_printf(p, "graphics version: %u\n", info->graphics_ver);
>> 	drm_printf(p, "graphics release: %u\n", info->graphics_ver_release);
>>>
>>>Also, I thought "x_ver" and "x_ver_release" sounds a bit odd, perhaps
>>>having "x_ver" and "x_rel" is more natural?
>>
>> Not sure what direction to go now though. Maybe trying to put all
>> suggestions together:
>>
>> 	if (info->graphics_rel)
>> 		drm_printf(p, "graphics version: %u.%u\n", info->graphics_ver, info->graphics_rel);
>> 	else
>> 		drm_printf(p, "graphics version: %u\n", info->graphics_ver);
>>
>> One thing  I like is that doing `| grep "graphics version"`
>> gives all info you are searching for.
>
>I'd like that, but this is all bikeshedding, really.

I already updated our local copy with all of this, so I'm fine including
that in the next version... since this is the first patch, I can also
send it standalone.

Lucas De Marchi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6dff4ca01241..9639800485b9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1258,11 +1258,17 @@  static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev)
  */
 #define IS_GEN(dev_priv, n)		(GRAPHICS_VER(dev_priv) == (n))
 
+#define IP_VER(ver, release)		((ver) << 8 | (release))
+
 #define GRAPHICS_VER(i915)		(INTEL_INFO(i915)->graphics_ver)
+#define GRAPHICS_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->graphics_ver, \
+					       INTEL_INFO(i915)->graphics_ver_release)
 #define IS_GRAPHICS_VER(i915, from, until) \
 	(GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= (until))
 
 #define MEDIA_VER(i915)			(INTEL_INFO(i915)->media_ver)
+#define MEDIA_VER_FULL(i915)		IP_VER(INTEL_INFO(i915)->media_ver, \
+					       INTEL_INFO(i915)->media_ver_release)
 #define IS_MEDIA_VER(i915, from, until) \
 	(MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until))
 
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 7eaa92fee421..e8ad14f002c1 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -97,7 +97,9 @@  void intel_device_info_print_static(const struct intel_device_info *info,
 				    struct drm_printer *p)
 {
 	drm_printf(p, "graphics_ver: %u\n", info->graphics_ver);
+	drm_printf(p, "graphics_ver_release: %u\n", info->graphics_ver_release);
 	drm_printf(p, "media_ver: %u\n", info->media_ver);
+	drm_printf(p, "media_ver_release: %u\n", info->media_ver_release);
 	drm_printf(p, "display_ver: %u\n", info->display.ver);
 	drm_printf(p, "gt: %d\n", info->gt);
 	drm_printf(p, "iommu: %s\n", iommu_name());
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index b326aff65cd6..944a5ff4df49 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -162,7 +162,9 @@  enum intel_ppgtt_type {
 
 struct intel_device_info {
 	u8 graphics_ver;
+	u8 graphics_ver_release;
 	u8 media_ver;
+	u8 media_ver_release;
 
 	u8 gt; /* GT number, 0 if undefined */
 	intel_engine_mask_t platform_engine_mask; /* Engines supported by the HW */