| Message ID | 20230331102419.521392-4-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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 >
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
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 >
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
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;
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(-)