[Intel-gfx] [PATCH 3/6] drm/i915: Fix comparison in intel_dram.

[Intel-gfx] [PATCH 3/6] drm/i915: Fix comparison in intel_dram.

[Maarten Lankhorst]

Gen13 should probably be the same as gen12, not gen11.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/i915/soc/intel_dram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
index 9f0651d48d41..9649051ed8ed 100644
--- a/drivers/gpu/drm/i915/soc/intel_dram.c
+++ b/drivers/gpu/drm/i915/soc/intel_dram.c
@@ -548,7 +548,7 @@ static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv)
 	if (ret)
 		return ret;
 
-	if (GRAPHICS_VER(dev_priv) == 12) {
+	if (GRAPHICS_VER(dev_priv) >= 12) {
 		switch (val & 0xf) {
 		case 0:
 			dram_info->type = INTEL_DRAM_DDR4;
-- 
2.34.1

Re: [Intel-gfx] [Intel-xe] [PATCH 3/6] drm/i915: Fix comparison in intel_dram.

[Matt Roper]

On Fri, Mar 31, 2023 at 12:24:16PM +0200, Maarten Lankhorst wrote:
> Gen13 should probably be the same as gen12, not gen11.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/soc/intel_dram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
> index 9f0651d48d41..9649051ed8ed 100644
> --- a/drivers/gpu/drm/i915/soc/intel_dram.c
> +++ b/drivers/gpu/drm/i915/soc/intel_dram.c
> @@ -548,7 +548,7 @@ static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv)

I don't think we need this change.  We were only reading the this pcode
mailbox for display purposes, and even on the latest graphics version 12
platforms we shouldn't make it into this function anymore.  Instead the
display IP now provides this information itself so there's no need to go
through the GT's pcode mailbox; we read it directly from display
registers in xelpdp_get_dram_info().  It looks like this condition here
would only matter if we had a future platform with graphics version
higher than 12, but display version lower than 14, which seems very
unlikely.


Matt

>  	if (ret)
>  		return ret;
>  
> -	if (GRAPHICS_VER(dev_priv) == 12) {
> +	if (GRAPHICS_VER(dev_priv) >= 12) {
>  		switch (val & 0xf) {
>  		case 0:
>  			dram_info->type = INTEL_DRAM_DDR4;
> -- 
> 2.34.1
> 

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

Re: [Intel-gfx] [Intel-xe] [PATCH 3/6] drm/i915: Fix comparison in intel_dram.

[Ville Syrjälä]

On Mon, Apr 03, 2023 at 01:35:40PM -0700, Matt Roper wrote:
> On Fri, Mar 31, 2023 at 12:24:16PM +0200, Maarten Lankhorst wrote:
> > Gen13 should probably be the same as gen12, not gen11.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/i915/soc/intel_dram.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
> > index 9f0651d48d41..9649051ed8ed 100644
> > --- a/drivers/gpu/drm/i915/soc/intel_dram.c
> > +++ b/drivers/gpu/drm/i915/soc/intel_dram.c
> > @@ -548,7 +548,7 @@ static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv)
> 
> I don't think we need this change.  We were only reading the this pcode
> mailbox for display purposes, and even on the latest graphics version 12
> platforms we shouldn't make it into this function anymore.  Instead the
> display IP now provides this information itself so there's no need to go
> through the GT's pcode mailbox; we read it directly from display
> registers in xelpdp_get_dram_info().  It looks like this condition here
> would only matter if we had a future platform with graphics version
> higher than 12, but display version lower than 14, which seems very
> unlikely.
> 
> 
> Matt
> 
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (GRAPHICS_VER(dev_priv) == 12) {
> > +	if (GRAPHICS_VER(dev_priv) >= 12) {

We should perhaps change all the checks here to DISPLAY_VER
since it's the display code that needs this information.

Though I suppose technically neither DISPLAY_VER nor
GRAPHICS_VER is entirely correct since it's really SoC
level stuff that we're looking at here. But the current 
mismash of GRAPHICS_VER and DISPLAY_VER is certainly not
helping reduce the confusion.

> >  		switch (val & 0xf) {
> >  		case 0:
> >  			dram_info->type = INTEL_DRAM_DDR4;
> > -- 
> > 2.34.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Fix comparison in intel_dram.

[Lucas De Marchi]

On Fri, Mar 31, 2023 at 12:24:16PM +0200, Maarten Lankhorst wrote:
>Gen13 should probably be the same as gen12, not gen11.

there is no such thing as gen13. Please reword commit message to use
graphics version, although here it's probably more related to the
display engine (and hence DISPLAY_VER()). But then the code you are
adding here is probably dead code since 825477e77912 ("drm/i915/mtl:
Obtain SAGV values from MMIO instead of GT pcode mailbox")

If this is fixing anything what exactly is it? Otherwise it'd be more
"extend to next platforms" rather than "Fix".

Lucas De Marchi

>
>Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Cc: intel-gfx@lists.freedesktop.org
>---
> drivers/gpu/drm/i915/soc/intel_dram.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
>index 9f0651d48d41..9649051ed8ed 100644
>--- a/drivers/gpu/drm/i915/soc/intel_dram.c
>+++ b/drivers/gpu/drm/i915/soc/intel_dram.c
>@@ -548,7 +548,7 @@ static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv)
> 	if (ret)
> 		return ret;
>
>-	if (GRAPHICS_VER(dev_priv) == 12) {
>+	if (GRAPHICS_VER(dev_priv) >= 12) {
> 		switch (val & 0xf) {
> 		case 0:
> 			dram_info->type = INTEL_DRAM_DDR4;
>-- 
>2.34.1
>

Re: [Intel-gfx] [Intel-xe] [PATCH 3/6] drm/i915: Fix comparison in intel_dram.

[Maarten Lankhorst]

Hey,

On 2023-04-03 22:35, Matt Roper wrote:
> On Fri, Mar 31, 2023 at 12:24:16PM +0200, Maarten Lankhorst wrote:
>> Gen13 should probably be the same as gen12, not gen11.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/i915/soc/intel_dram.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
>> index 9f0651d48d41..9649051ed8ed 100644
>> --- a/drivers/gpu/drm/i915/soc/intel_dram.c
>> +++ b/drivers/gpu/drm/i915/soc/intel_dram.c
>> @@ -548,7 +548,7 @@ static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv)
> I don't think we need this change.  We were only reading the this pcode
> mailbox for display purposes, and even on the latest graphics version 12
> platforms we shouldn't make it into this function anymore.  Instead the
> display IP now provides this information itself so there's no need to go
> through the GT's pcode mailbox; we read it directly from display
> registers in xelpdp_get_dram_info().  It looks like this condition here
> would only matter if we had a future platform with graphics version
> higher than 12, but display version lower than 14, which seems very
> unlikely.

Probably not, but it's at least cosmetically more correct. We only need 
the dram code for display, so in theory we could check for display_ver 
 >= 12 instead and have it unified.

If something breaks the pattern, we can fix it then. :)

~Maarten