| Message ID | 20210120151611.132591-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915: Nuke not needed members of dram_info | expand |
On Wed, Jan 20, 2021 at 07:16:09AM -0800, Jose Souza wrote: >Up to now we were reading some DRAM information from MCHBAR register >and from pcode what is already not good but some GEN12(TGL-H and ADL-S) >platforms have MCHBAR DRAM information in different offsets. > >This was notified to HW team that decided that the best alternative is >always apply the 16gb_dimm watermark adjustment for GEN12+ platforms >and read the remaning DRAM information needed to other display >programming from pcode. > >So here moving the DRAM pcode function to intel_dram.c, removing >the duplicated fields from intel_qgv_info, setting and using >information from dram_info. > >Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >--- > drivers/gpu/drm/i915/display/intel_bw.c | 98 +++++-------------------- > drivers/gpu/drm/i915/i915_drv.c | 5 +- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_dram.c | 77 ++++++++++++++++++- > 4 files changed, 97 insertions(+), 84 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c >index bd060404d249..1368bd96ed73 100644 >--- a/drivers/gpu/drm/i915/display/intel_bw.c >+++ b/drivers/gpu/drm/i915/display/intel_bw.c >@@ -19,77 +19,9 @@ struct intel_qgv_point { > > struct intel_qgv_info { > struct intel_qgv_point points[I915_NUM_QGV_POINTS]; >- u8 num_points; >- u8 num_channels; > u8 t_bl; >- enum intel_dram_type dram_type; humn... given this struct already has padding, we could very well leave the num_points field. See below. > static int icl_pcode_read_qgv_point_info(struct drm_i915_private *dev_priv, > struct intel_qgv_point *sp, > int point) >@@ -139,17 +71,19 @@ int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv, > static int icl_get_qgv_points(struct drm_i915_private *dev_priv, > struct intel_qgv_info *qi) > { >+ struct dram_info *dram_info = &dev_priv->dram_info; > int i, ret; > >- ret = icl_pcode_read_mem_global_info(dev_priv, qi); >- if (ret) >- return ret; >+ if (IS_GEN(dev_priv, 12)) >+ qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 16; >+ else if (IS_GEN(dev_priv, 11)) >+ qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 8; > > if (drm_WARN_ON(&dev_priv->drm, >- qi->num_points > ARRAY_SIZE(qi->points))) >- qi->num_points = ARRAY_SIZE(qi->points); >+ dram_info->qgv_points > ARRAY_SIZE(qi->points))) >+ dram_info->qgv_points = ARRAY_SIZE(qi->points); previously we were overriding a member of qi. Now we are overriding a member of dram_info, which seems to cross the boundaries of what this code should be doing. So maybe: qi->num_points = dev_priv->dram_info->qgv_points; and leave the check alone and also the renames in this file? Also, dram_info->qgv_points should be named dram_info->num_qgv_points for consistency with other structs. > >- for (i = 0; i < qi->num_points; i++) { >+ for (i = 0; i < dram_info->qgv_points; i++) { > struct intel_qgv_point *sp = &qi->points[i]; > > ret = icl_pcode_read_qgv_point_info(dev_priv, sp, i); >@@ -171,12 +105,13 @@ static int icl_calc_bw(int dclk, int num, int den) > return DIV_ROUND_CLOSEST(num * dclk * 100, den * 6); > } > >-static int icl_sagv_max_dclk(const struct intel_qgv_info *qi) >+static int icl_sagv_max_dclk(struct drm_i915_private *dev_priv, >+ const struct intel_qgv_info *qi) > { > u16 dclk = 0; > int i; > >- for (i = 0; i < qi->num_points; i++) >+ for (i = 0; i < dev_priv->dram_info.qgv_points; i++) > dclk = max(dclk, qi->points[i].dclk); > > return dclk; >@@ -207,6 +142,7 @@ static const struct intel_sa_info rkl_sa_info = { > > static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel_sa_info *sa) > { >+ struct dram_info *dram_info = &dev_priv->dram_info; > struct intel_qgv_info qi = {}; > bool is_y_tile = true; /* assume y tile may be used */ > int num_channels; >@@ -222,10 +158,10 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel > "Failed to get memory subsystem information, ignoring bandwidth limits"); > return ret; > } >- num_channels = qi.num_channels; >+ num_channels = dram_info->num_channels; > > deinterleave = DIV_ROUND_UP(num_channels, is_y_tile ? 4 : 2); >- dclk_max = icl_sagv_max_dclk(&qi); >+ dclk_max = icl_sagv_max_dclk(dev_priv, &qi); > > ipqdepthpch = 16; > >@@ -241,9 +177,9 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel > clpchgroup = (sa->deburst * deinterleave / num_channels) << i; > bi->num_planes = (ipqdepth - clpchgroup) / clpchgroup + 1; > >- bi->num_qgv_points = qi.num_points; >+ bi->num_qgv_points = dram_info->qgv_points; see above, we already have num_ prefix here, let's keep it in dram_info as well. > >- for (j = 0; j < qi.num_points; j++) { >+ for (j = 0; j < dram_info->qgv_points; j++) { > const struct intel_qgv_point *sp = &qi.points[j]; > int ct, bw; > >@@ -274,7 +210,7 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel > * SAGV point, but we can't send PCode commands to restrict it > * as it will fail and pointless anyway. > */ >- if (qi.num_points == 1) >+ if (dram_info->qgv_points == 1) > dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED; > else > dev_priv->sagv_status = I915_SAGV_ENABLED; >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >index a1cc60de99f0..66f763fe7a83 100644 >--- a/drivers/gpu/drm/i915/i915_drv.c >+++ b/drivers/gpu/drm/i915/i915_drv.c >@@ -608,14 +608,15 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv) > goto err_msi; > > intel_opregion_setup(dev_priv); >+ >+ intel_pcode_init(dev_priv); >+ > /* > * Fill the dram structure to get the system dram info. This will be > * used for memory latency calculation. > */ > intel_dram_detect(dev_priv); > >- intel_pcode_init(dev_priv); >- > intel_bw_init_hw(dev_priv); > > return 0; >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >index 250e92910fa1..a2ae21082b34 100644 >--- a/drivers/gpu/drm/i915/i915_drv.h >+++ b/drivers/gpu/drm/i915/i915_drv.h >@@ -1144,6 +1144,7 @@ struct drm_i915_private { > INTEL_DRAM_LPDDR3, > INTEL_DRAM_LPDDR4 > } type; >+ u8 qgv_points; > } dram_info; > > struct intel_bw_info { >diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c >index 694fbd8c9cd4..1298823c957c 100644 >--- a/drivers/gpu/drm/i915/intel_dram.c >+++ b/drivers/gpu/drm/i915/intel_dram.c >@@ -5,6 +5,7 @@ > > #include "i915_drv.h" > #include "intel_dram.h" >+#include "intel_sideband.h" > > struct dram_dimm_info { > u16 size; >@@ -408,6 +409,78 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) > return 0; > } > >+static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv) >+{ >+ struct dram_info *dram_info = &dev_priv->dram_info; >+ u32 val = 0; >+ int ret; >+ >+ ret = sandybridge_pcode_read(dev_priv, >+ ICL_PCODE_MEM_SUBSYSYSTEM_INFO | >+ ICL_PCODE_MEM_SS_READ_GLOBAL_INFO, >+ &val, NULL); >+ if (ret) >+ return ret; >+ >+ if (IS_GEN(dev_priv, 12)) { >+ switch (val & 0xf) { >+ case 0: >+ dram_info->type = INTEL_DRAM_DDR4; >+ break; >+ case 3: >+ dram_info->type = INTEL_DRAM_LPDDR4; >+ break; >+ case 4: >+ dram_info->type = INTEL_DRAM_DDR3; >+ break; >+ case 5: >+ dram_info->type = INTEL_DRAM_LPDDR3; >+ break; >+ default: >+ MISSING_CASE(val & 0xf); >+ return -1; >+ } >+ } else { >+ switch (val & 0xf) { >+ case 0: >+ dram_info->type = INTEL_DRAM_DDR4; >+ break; >+ case 1: >+ dram_info->type = INTEL_DRAM_DDR3; >+ break; >+ case 2: >+ dram_info->type = INTEL_DRAM_LPDDR3; >+ break; >+ case 3: >+ dram_info->type = INTEL_DRAM_LPDDR4; >+ break; >+ default: >+ MISSING_CASE(val & 0xf); >+ return -1; >+ } >+ } >+ >+ dram_info->num_channels = (val & 0xf0) >> 4; >+ dram_info->qgv_points = (val & 0xf00) >> 8; >+ >+ return 0; >+} >+ >+static int gen11_get_dram_info(struct drm_i915_private *i915) >+{ >+ if (INTEL_GEN(i915) == 11) { >+ int ret = skl_get_dram_info(i915); >+ >+ if (ret) >+ return ret; >+ } else { >+ /* Always needed for GEN12+ */ >+ i915->dram_info.is_16gb_dimm = true; >+ } >+ >+ return icl_pcode_read_mem_global_info(i915); >+} >+ > void intel_dram_detect(struct drm_i915_private *i915) > { > struct dram_info *dram_info = &i915->dram_info; >@@ -423,7 +496,9 @@ void intel_dram_detect(struct drm_i915_private *i915) > if (INTEL_GEN(i915) < 9 || !HAS_DISPLAY(i915)) > return; > >- if (IS_GEN9_LP(i915)) >+ if (INTEL_GEN(i915) >= 11) >+ ret = gen11_get_dram_info(i915); gen11 and gen12 implementation above are sufficiently different: better to keep the if/else chain here only static int gen11_get_dram_info(struct drm_i915_private *i915) { int ret = skl_get_dram_info(i915); if (ret) return ret; return icl_pcode_read_mem_global_info(i915); } static int gen12_get_dram_info(struct drm_i915_private *i915) { /* Always needed for GEN12+ */ i915->dram_info.is_16gb_dimm = true; return icl_pcode_read_mem_global_info(i915); } Also, now it seems we have lots of dead code since we are not calling skl_get_dram_info() for tgl. See tgl_get_dram_type(), skl_dram_get_channels_info, etc Lucas De Marchi
On Wed, 2021-01-27 at 06:49 -0800, Lucas De Marchi wrote: > On Wed, Jan 20, 2021 at 07:16:09AM -0800, Jose Souza wrote: > > Up to now we were reading some DRAM information from MCHBAR register > > and from pcode what is already not good but some GEN12(TGL-H and ADL-S) > > platforms have MCHBAR DRAM information in different offsets. > > > > This was notified to HW team that decided that the best alternative is > > always apply the 16gb_dimm watermark adjustment for GEN12+ platforms > > and read the remaning DRAM information needed to other display > > programming from pcode. > > > > So here moving the DRAM pcode function to intel_dram.c, removing > > the duplicated fields from intel_qgv_info, setting and using > > information from dram_info. > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_bw.c | 98 +++++-------------------- > > drivers/gpu/drm/i915/i915_drv.c | 5 +- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_dram.c | 77 ++++++++++++++++++- > > 4 files changed, 97 insertions(+), 84 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c > > index bd060404d249..1368bd96ed73 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bw.c > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > > @@ -19,77 +19,9 @@ struct intel_qgv_point { > > > > struct intel_qgv_info { > > struct intel_qgv_point points[I915_NUM_QGV_POINTS]; > > - u8 num_points; > > - u8 num_channels; > > u8 t_bl; > > - enum intel_dram_type dram_type; > > humn... given this struct already has padding, we could very well leave > the num_points field. See below. > > > static int icl_pcode_read_qgv_point_info(struct drm_i915_private *dev_priv, > > struct intel_qgv_point *sp, > > int point) > > @@ -139,17 +71,19 @@ int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv, > > static int icl_get_qgv_points(struct drm_i915_private *dev_priv, > > struct intel_qgv_info *qi) > > { > > + struct dram_info *dram_info = &dev_priv->dram_info; > > int i, ret; > > > > - ret = icl_pcode_read_mem_global_info(dev_priv, qi); > > - if (ret) > > - return ret; > > + if (IS_GEN(dev_priv, 12)) > > + qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 16; > > + else if (IS_GEN(dev_priv, 11)) > > + qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 8; > > > > if (drm_WARN_ON(&dev_priv->drm, > > - qi->num_points > ARRAY_SIZE(qi->points))) > > - qi->num_points = ARRAY_SIZE(qi->points); > > + dram_info->qgv_points > ARRAY_SIZE(qi->points))) > > + dram_info->qgv_points = ARRAY_SIZE(qi->points); > > previously we were overriding a member of qi. Now we are overriding a > member of dram_info, which seems to cross the boundaries of what this > code should be doing. > > So maybe: > > qi->num_points = dev_priv->dram_info->qgv_points; Okay, reasonable enough to keep this duplicated. > > and leave the check alone and also the renames in this file? > Also, dram_info->qgv_points should be named dram_info->num_qgv_points > for consistency with other structs. > > > > > - for (i = 0; i < qi->num_points; i++) { > > + for (i = 0; i < dram_info->qgv_points; i++) { > > struct intel_qgv_point *sp = &qi->points[i]; > > > > ret = icl_pcode_read_qgv_point_info(dev_priv, sp, i); > > @@ -171,12 +105,13 @@ static int icl_calc_bw(int dclk, int num, int den) > > return DIV_ROUND_CLOSEST(num * dclk * 100, den * 6); > > } > > > > -static int icl_sagv_max_dclk(const struct intel_qgv_info *qi) > > +static int icl_sagv_max_dclk(struct drm_i915_private *dev_priv, > > + const struct intel_qgv_info *qi) > > { > > u16 dclk = 0; > > int i; > > > > - for (i = 0; i < qi->num_points; i++) > > + for (i = 0; i < dev_priv->dram_info.qgv_points; i++) > > dclk = max(dclk, qi->points[i].dclk); > > > > return dclk; > > @@ -207,6 +142,7 @@ static const struct intel_sa_info rkl_sa_info = { > > > > static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel_sa_info *sa) > > { > > + struct dram_info *dram_info = &dev_priv->dram_info; > > struct intel_qgv_info qi = {}; > > bool is_y_tile = true; /* assume y tile may be used */ > > int num_channels; > > @@ -222,10 +158,10 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel > > "Failed to get memory subsystem information, ignoring bandwidth limits"); > > return ret; > > } > > - num_channels = qi.num_channels; > > + num_channels = dram_info->num_channels; > > > > deinterleave = DIV_ROUND_UP(num_channels, is_y_tile ? 4 : 2); > > - dclk_max = icl_sagv_max_dclk(&qi); > > + dclk_max = icl_sagv_max_dclk(dev_priv, &qi); > > > > ipqdepthpch = 16; > > > > @@ -241,9 +177,9 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel > > clpchgroup = (sa->deburst * deinterleave / num_channels) << i; > > bi->num_planes = (ipqdepth - clpchgroup) / clpchgroup + 1; > > > > - bi->num_qgv_points = qi.num_points; > > + bi->num_qgv_points = dram_info->qgv_points; > > > see above, we already have num_ prefix here, let's keep it in dram_info > as well. Done, but will keep num_point in intel_qgv_info. We can rename it latter. > > > > > - for (j = 0; j < qi.num_points; j++) { > > + for (j = 0; j < dram_info->qgv_points; j++) { > > const struct intel_qgv_point *sp = &qi.points[j]; > > int ct, bw; > > > > @@ -274,7 +210,7 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel > > * SAGV point, but we can't send PCode commands to restrict it > > * as it will fail and pointless anyway. > > */ > > - if (qi.num_points == 1) > > + if (dram_info->qgv_points == 1) > > dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED; > > else > > dev_priv->sagv_status = I915_SAGV_ENABLED; > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index a1cc60de99f0..66f763fe7a83 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -608,14 +608,15 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv) > > goto err_msi; > > > > intel_opregion_setup(dev_priv); > > + > > + intel_pcode_init(dev_priv); > > + > > /* > > * Fill the dram structure to get the system dram info. This will be > > * used for memory latency calculation. > > */ > > intel_dram_detect(dev_priv); > > > > - intel_pcode_init(dev_priv); > > - > > intel_bw_init_hw(dev_priv); > > > > return 0; > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 250e92910fa1..a2ae21082b34 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1144,6 +1144,7 @@ struct drm_i915_private { > > INTEL_DRAM_LPDDR3, > > INTEL_DRAM_LPDDR4 > > } type; > > + u8 qgv_points; > > } dram_info; > > > > struct intel_bw_info { > > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c > > index 694fbd8c9cd4..1298823c957c 100644 > > --- a/drivers/gpu/drm/i915/intel_dram.c > > +++ b/drivers/gpu/drm/i915/intel_dram.c > > @@ -5,6 +5,7 @@ > > > > #include "i915_drv.h" > > #include "intel_dram.h" > > +#include "intel_sideband.h" > > > > struct dram_dimm_info { > > u16 size; > > @@ -408,6 +409,78 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) > > return 0; > > } > > > > +static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv) > > +{ > > + struct dram_info *dram_info = &dev_priv->dram_info; > > + u32 val = 0; > > + int ret; > > + > > + ret = sandybridge_pcode_read(dev_priv, > > + ICL_PCODE_MEM_SUBSYSYSTEM_INFO | > > + ICL_PCODE_MEM_SS_READ_GLOBAL_INFO, > > + &val, NULL); > > + if (ret) > > + return ret; > > + > > + if (IS_GEN(dev_priv, 12)) { > > + switch (val & 0xf) { > > + case 0: > > + dram_info->type = INTEL_DRAM_DDR4; > > + break; > > + case 3: > > + dram_info->type = INTEL_DRAM_LPDDR4; > > + break; > > + case 4: > > + dram_info->type = INTEL_DRAM_DDR3; > > + break; > > + case 5: > > + dram_info->type = INTEL_DRAM_LPDDR3; > > + break; > > + default: > > + MISSING_CASE(val & 0xf); > > + return -1; > > + } > > + } else { > > + switch (val & 0xf) { > > + case 0: > > + dram_info->type = INTEL_DRAM_DDR4; > > + break; > > + case 1: > > + dram_info->type = INTEL_DRAM_DDR3; > > + break; > > + case 2: > > + dram_info->type = INTEL_DRAM_LPDDR3; > > + break; > > + case 3: > > + dram_info->type = INTEL_DRAM_LPDDR4; > > + break; > > + default: > > + MISSING_CASE(val & 0xf); > > + return -1; > > + } > > + } > > + > > + dram_info->num_channels = (val & 0xf0) >> 4; > > + dram_info->qgv_points = (val & 0xf00) >> 8; > > + > > + return 0; > > +} > > + > > +static int gen11_get_dram_info(struct drm_i915_private *i915) > > +{ > > + if (INTEL_GEN(i915) == 11) { > > + int ret = skl_get_dram_info(i915); > > + > > + if (ret) > > + return ret; > > + } else { > > + /* Always needed for GEN12+ */ > > + i915->dram_info.is_16gb_dimm = true; > > + } > > + > > + return icl_pcode_read_mem_global_info(i915); > > +} > > + > > void intel_dram_detect(struct drm_i915_private *i915) > > { > > struct dram_info *dram_info = &i915->dram_info; > > @@ -423,7 +496,9 @@ void intel_dram_detect(struct drm_i915_private *i915) > > if (INTEL_GEN(i915) < 9 || !HAS_DISPLAY(i915)) > > return; > > > > - if (IS_GEN9_LP(i915)) > > + if (INTEL_GEN(i915) >= 11) > > + ret = gen11_get_dram_info(i915); > > gen11 and gen12 implementation above are sufficiently different: better > to keep the if/else chain here only > > static int gen11_get_dram_info(struct drm_i915_private *i915) > { > int ret = skl_get_dram_info(i915); > if (ret) > return ret; > > return icl_pcode_read_mem_global_info(i915); > } > > > static int gen12_get_dram_info(struct drm_i915_private *i915) > { > /* Always needed for GEN12+ */ > i915->dram_info.is_16gb_dimm = true; > > return icl_pcode_read_mem_global_info(i915); > } Done > > Also, now it seems we have lots of dead code since we are not calling > skl_get_dram_info() for tgl. See > tgl_get_dram_type(), skl_dram_get_channels_info, etc Can't find those. > > Lucas De Marchi
diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c index bd060404d249..1368bd96ed73 100644 --- a/drivers/gpu/drm/i915/display/intel_bw.c +++ b/drivers/gpu/drm/i915/display/intel_bw.c @@ -19,77 +19,9 @@ struct intel_qgv_point { struct intel_qgv_info { struct intel_qgv_point points[I915_NUM_QGV_POINTS]; - u8 num_points; - u8 num_channels; u8 t_bl; - enum intel_dram_type dram_type; }; -static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv, - struct intel_qgv_info *qi) -{ - u32 val = 0; - int ret; - - ret = sandybridge_pcode_read(dev_priv, - ICL_PCODE_MEM_SUBSYSYSTEM_INFO | - ICL_PCODE_MEM_SS_READ_GLOBAL_INFO, - &val, NULL); - if (ret) - return ret; - - if (IS_GEN(dev_priv, 12)) { - switch (val & 0xf) { - case 0: - qi->dram_type = INTEL_DRAM_DDR4; - break; - case 3: - qi->dram_type = INTEL_DRAM_LPDDR4; - break; - case 4: - qi->dram_type = INTEL_DRAM_DDR3; - break; - case 5: - qi->dram_type = INTEL_DRAM_LPDDR3; - break; - default: - MISSING_CASE(val & 0xf); - break; - } - } else if (IS_GEN(dev_priv, 11)) { - switch (val & 0xf) { - case 0: - qi->dram_type = INTEL_DRAM_DDR4; - break; - case 1: - qi->dram_type = INTEL_DRAM_DDR3; - break; - case 2: - qi->dram_type = INTEL_DRAM_LPDDR3; - break; - case 3: - qi->dram_type = INTEL_DRAM_LPDDR4; - break; - default: - MISSING_CASE(val & 0xf); - break; - } - } else { - MISSING_CASE(INTEL_GEN(dev_priv)); - qi->dram_type = INTEL_DRAM_LPDDR3; /* Conservative default */ - } - - qi->num_channels = (val & 0xf0) >> 4; - qi->num_points = (val & 0xf00) >> 8; - - if (IS_GEN(dev_priv, 12)) - qi->t_bl = qi->dram_type == INTEL_DRAM_DDR4 ? 4 : 16; - else if (IS_GEN(dev_priv, 11)) - qi->t_bl = qi->dram_type == INTEL_DRAM_DDR4 ? 4 : 8; - - return 0; -} - static int icl_pcode_read_qgv_point_info(struct drm_i915_private *dev_priv, struct intel_qgv_point *sp, int point) @@ -139,17 +71,19 @@ int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv, static int icl_get_qgv_points(struct drm_i915_private *dev_priv, struct intel_qgv_info *qi) { + struct dram_info *dram_info = &dev_priv->dram_info; int i, ret; - ret = icl_pcode_read_mem_global_info(dev_priv, qi); - if (ret) - return ret; + if (IS_GEN(dev_priv, 12)) + qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 16; + else if (IS_GEN(dev_priv, 11)) + qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 8; if (drm_WARN_ON(&dev_priv->drm, - qi->num_points > ARRAY_SIZE(qi->points))) - qi->num_points = ARRAY_SIZE(qi->points); + dram_info->qgv_points > ARRAY_SIZE(qi->points))) + dram_info->qgv_points = ARRAY_SIZE(qi->points); - for (i = 0; i < qi->num_points; i++) { + for (i = 0; i < dram_info->qgv_points; i++) { struct intel_qgv_point *sp = &qi->points[i]; ret = icl_pcode_read_qgv_point_info(dev_priv, sp, i); @@ -171,12 +105,13 @@ static int icl_calc_bw(int dclk, int num, int den) return DIV_ROUND_CLOSEST(num * dclk * 100, den * 6); } -static int icl_sagv_max_dclk(const struct intel_qgv_info *qi) +static int icl_sagv_max_dclk(struct drm_i915_private *dev_priv, + const struct intel_qgv_info *qi) { u16 dclk = 0; int i; - for (i = 0; i < qi->num_points; i++) + for (i = 0; i < dev_priv->dram_info.qgv_points; i++) dclk = max(dclk, qi->points[i].dclk); return dclk; @@ -207,6 +142,7 @@ static const struct intel_sa_info rkl_sa_info = { static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel_sa_info *sa) { + struct dram_info *dram_info = &dev_priv->dram_info; struct intel_qgv_info qi = {}; bool is_y_tile = true; /* assume y tile may be used */ int num_channels; @@ -222,10 +158,10 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel "Failed to get memory subsystem information, ignoring bandwidth limits"); return ret; } - num_channels = qi.num_channels; + num_channels = dram_info->num_channels; deinterleave = DIV_ROUND_UP(num_channels, is_y_tile ? 4 : 2); - dclk_max = icl_sagv_max_dclk(&qi); + dclk_max = icl_sagv_max_dclk(dev_priv, &qi); ipqdepthpch = 16; @@ -241,9 +177,9 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel clpchgroup = (sa->deburst * deinterleave / num_channels) << i; bi->num_planes = (ipqdepth - clpchgroup) / clpchgroup + 1; - bi->num_qgv_points = qi.num_points; + bi->num_qgv_points = dram_info->qgv_points; - for (j = 0; j < qi.num_points; j++) { + for (j = 0; j < dram_info->qgv_points; j++) { const struct intel_qgv_point *sp = &qi.points[j]; int ct, bw; @@ -274,7 +210,7 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel * SAGV point, but we can't send PCode commands to restrict it * as it will fail and pointless anyway. */ - if (qi.num_points == 1) + if (dram_info->qgv_points == 1) dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED; else dev_priv->sagv_status = I915_SAGV_ENABLED; diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index a1cc60de99f0..66f763fe7a83 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -608,14 +608,15 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv) goto err_msi; intel_opregion_setup(dev_priv); + + intel_pcode_init(dev_priv); + /* * Fill the dram structure to get the system dram info. This will be * used for memory latency calculation. */ intel_dram_detect(dev_priv); - intel_pcode_init(dev_priv); - intel_bw_init_hw(dev_priv); return 0; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 250e92910fa1..a2ae21082b34 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1144,6 +1144,7 @@ struct drm_i915_private { INTEL_DRAM_LPDDR3, INTEL_DRAM_LPDDR4 } type; + u8 qgv_points; } dram_info; struct intel_bw_info { diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c index 694fbd8c9cd4..1298823c957c 100644 --- a/drivers/gpu/drm/i915/intel_dram.c +++ b/drivers/gpu/drm/i915/intel_dram.c @@ -5,6 +5,7 @@ #include "i915_drv.h" #include "intel_dram.h" +#include "intel_sideband.h" struct dram_dimm_info { u16 size; @@ -408,6 +409,78 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) return 0; } +static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv) +{ + struct dram_info *dram_info = &dev_priv->dram_info; + u32 val = 0; + int ret; + + ret = sandybridge_pcode_read(dev_priv, + ICL_PCODE_MEM_SUBSYSYSTEM_INFO | + ICL_PCODE_MEM_SS_READ_GLOBAL_INFO, + &val, NULL); + if (ret) + return ret; + + if (IS_GEN(dev_priv, 12)) { + switch (val & 0xf) { + case 0: + dram_info->type = INTEL_DRAM_DDR4; + break; + case 3: + dram_info->type = INTEL_DRAM_LPDDR4; + break; + case 4: + dram_info->type = INTEL_DRAM_DDR3; + break; + case 5: + dram_info->type = INTEL_DRAM_LPDDR3; + break; + default: + MISSING_CASE(val & 0xf); + return -1; + } + } else { + switch (val & 0xf) { + case 0: + dram_info->type = INTEL_DRAM_DDR4; + break; + case 1: + dram_info->type = INTEL_DRAM_DDR3; + break; + case 2: + dram_info->type = INTEL_DRAM_LPDDR3; + break; + case 3: + dram_info->type = INTEL_DRAM_LPDDR4; + break; + default: + MISSING_CASE(val & 0xf); + return -1; + } + } + + dram_info->num_channels = (val & 0xf0) >> 4; + dram_info->qgv_points = (val & 0xf00) >> 8; + + return 0; +} + +static int gen11_get_dram_info(struct drm_i915_private *i915) +{ + if (INTEL_GEN(i915) == 11) { + int ret = skl_get_dram_info(i915); + + if (ret) + return ret; + } else { + /* Always needed for GEN12+ */ + i915->dram_info.is_16gb_dimm = true; + } + + return icl_pcode_read_mem_global_info(i915); +} + void intel_dram_detect(struct drm_i915_private *i915) { struct dram_info *dram_info = &i915->dram_info; @@ -423,7 +496,9 @@ void intel_dram_detect(struct drm_i915_private *i915) if (INTEL_GEN(i915) < 9 || !HAS_DISPLAY(i915)) return; - if (IS_GEN9_LP(i915)) + if (INTEL_GEN(i915) >= 11) + ret = gen11_get_dram_info(i915); + else if (IS_GEN9_LP(i915)) ret = bxt_get_dram_info(i915); else ret = skl_get_dram_info(i915);
Up to now we were reading some DRAM information from MCHBAR register and from pcode what is already not good but some GEN12(TGL-H and ADL-S) platforms have MCHBAR DRAM information in different offsets. This was notified to HW team that decided that the best alternative is always apply the 16gb_dimm watermark adjustment for GEN12+ platforms and read the remaning DRAM information needed to other display programming from pcode. So here moving the DRAM pcode function to intel_dram.c, removing the duplicated fields from intel_qgv_info, setting and using information from dram_info. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_bw.c | 98 +++++-------------------- drivers/gpu/drm/i915/i915_drv.c | 5 +- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_dram.c | 77 ++++++++++++++++++- 4 files changed, 97 insertions(+), 84 deletions(-)