Re: [Intel-gfx] [PATCH v3 4/9] drm/i915: Decode system memory bandwidth
Hi
Em Sex, 2016-09-09 às 13:31 +0530, Kumar, Mahesh escreveu:
> From: Mahesh Kumar
>
> This patch adds support to decode system memory bandwidth
> which will be used for arbitrated display memory percentage
> calculation in GEN9 based system.
This is not a complete review of this patch since I can't find the
documentation for the registers used by the patch, but I'll try to
provide some early feedback. Most of it is about styling, so feel free
to provide counter arguments if you disagree.
>
> Signed-off-by: Mahesh Kumar
> ---
> drivers/gpu/drm/i915/i915_drv.c | 96
> +
> drivers/gpu/drm/i915/i915_drv.h | 18
> drivers/gpu/drm/i915/i915_reg.h | 25 +++
> 3 files changed, 139 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 02c34d6..0a4f18d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -973,6 +973,96 @@ static void intel_sanitize_options(struct
> drm_i915_private *dev_priv)
> DRM_DEBUG_DRIVER("use GPU sempahores? %s\n",
> yesno(i915.semaphores));
> }
>
> +static void
> +intel_get_memdev_info(struct drm_device *dev)
In our current standards we prefer that you use drm_i915_private as the
parameter here. And the new function doesn't even use drm_device for
anything, so that reinforces the argument.
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + uint32_t val = 0;
> + uint32_t mem_speed = 0;
> + uint8_t dram_type;
> + uint32_t dram_channel;
> + uint8_t num_channel;
> + bool rank_valid = false;
We generally don't zero-initialize things that don't need to be zero-
initialized, like these 3 variables. I know this is an eternal
discussion and each side has pros and cons, but following the already
used coding style usually wins.
Also, please just add this:
struct memdev_info *memdev_info = &dev_priv->memdev_info;
All those "dev_priv->memdev_info" statements below are already making
my fingers hurt, and I didn't even type them :)
> +
> + if (!IS_GEN9(dev_priv))
> + goto exit;
If you just set memdev_info.valid to false right at the beginning of
the function then you can just return here and below without using the
goto. IMHO it will look better. Or you could even rely on the fact that
we used kzalloc anyway.
> +
> + val = I915_READ(P_CR_MC_BIOS_REQ_0_0_0);
> + mem_speed = div_u64((uint64_t) (val & REQ_DATA_MASK) *
> + MEMORY_FREQ_MULTIPLIER, 1000);
> +
> + if (mem_speed == 0)
> + goto exit;
Perhaps a DRM_DEBUG_KMS("something something memory data is not valid")
would be useful here too.
> +
> + dev_priv->memdev_info.valid = true;
> + dev_priv->memdev_info.mem_speed = mem_speed;
> + dram_type = (val >> DRAM_TYPE_SHIFT) & DRAM_TYPE_MASK;
> + dram_channel = (val >> DRAM_CHANNEL_SHIFT) &
> DRAM_CHANNEL_MASK;
> + num_channel = hweight32(dram_channel);
> +
> + /*
> + * The lpddr3 and lpddr4 technologies can have 1-4 channels
> and the
> + * channels are 32bits wide; while ddr3l technologies can
> have 1-2
> + * channels and the channels are 64 bits wide. But SV team
> found that in
> + * case of single 64 bit wide DDR3L dimms two bits were set
> and system
> + * with two DDR3L 64bit dimm all four bits were set.
> + */
I still don't have access to the spec, but this comment suggests the
spec doesn't match the SV team findings. So can't the SV team request
the specs to reflect their findings? As an outsider, I see this as the
SV team's word against the HW team's words, and I don't know who made
the mistake: the SV engineer or the HW engineer. So I expect a
discussion between the two and the conclusions being reflected on the
specification :). Errata knowledge shouldn't go straight to the
drivers, that's a recipe for eternal doubt to the future people
maintaining this code.
> +
> + switch (dram_type) {
> + case DRAM_TYPE_LPDDR3:
> + case DRAM_TYPE_LPDDR4:
> + dev_priv->memdev_info.data_width = 4;
> + dev_priv->memdev_info.num_channel = num_channel;
> + break;
> + case DRAM_TYPE_DDR3L:
> + dev_priv->memdev_info.data_width = 8;
> + dev_priv->memdev_info.num_channel = num_channel / 2;
> + break;
> + default:
Again, no access to the spec here, but shouldn't this case reset
memdev_info.valid to false and then return (possibly with a
DRM_DEBUG_KMS)?
> + dev_priv->memdev_info.data_width = 4;
> + dev_priv->memdev_info.num_channel = num_channel;
> + }
> +
> + /*
> + * Now read each DUNIT8/9/10/11 to check the rank of each
> dimms.
> + * all the dimms should have same rank as in first valid
> Dimm
> + */
>
> +#define D_CR_DRP0_DUNIT_INVALID 0x
I'm not a huge fan of these mid-file defines with later undefines.
Can't we just move thi
Re: [Intel-gfx] [PATCH v3 4/9] drm/i915: Decode system memory bandwidth
On Friday 16 September 2016 01:32 PM, Pandiyan, Dhinakaran wrote:
On Fri, 2016-09-09 at 13:31 +0530, Kumar, Mahesh wrote:
From: Mahesh Kumar
This patch adds support to decode system memory bandwidth
which will be used for arbitrated display memory percentage
calculation in GEN9 based system.
Signed-off-by: Mahesh Kumar
---
drivers/gpu/drm/i915/i915_drv.c | 96 +
drivers/gpu/drm/i915/i915_drv.h | 18
drivers/gpu/drm/i915/i915_reg.h | 25 +++
3 files changed, 139 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 02c34d6..0a4f18d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -973,6 +973,96 @@ static void intel_sanitize_options(struct drm_i915_private
*dev_priv)
DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores));
}
+static void
+intel_get_memdev_info(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ uint32_t val = 0;
+ uint32_t mem_speed = 0;
+ uint8_t dram_type;
+ uint32_t dram_channel;
+ uint8_t num_channel;
+ bool rank_valid = false;
+
+ if (!IS_GEN9(dev_priv))
+ goto exit;
+
+ val = I915_READ(P_CR_MC_BIOS_REQ_0_0_0);
+ mem_speed = div_u64((uint64_t) (val & REQ_DATA_MASK) *
+ MEMORY_FREQ_MULTIPLIER, 1000);
+
+ if (mem_speed == 0)
+ goto exit;
+
+ dev_priv->memdev_info.valid = true;
+ dev_priv->memdev_info.mem_speed = mem_speed;
+ dram_type = (val >> DRAM_TYPE_SHIFT) & DRAM_TYPE_MASK;
+ dram_channel = (val >> DRAM_CHANNEL_SHIFT) & DRAM_CHANNEL_MASK;
+ num_channel = hweight32(dram_channel);
+
+ /*
+* The lpddr3 and lpddr4 technologies can have 1-4 channels and the
+* channels are 32bits wide; while ddr3l technologies can have 1-2
+* channels and the channels are 64 bits wide. But SV team found that in
+* case of single 64 bit wide DDR3L dimms two bits were set and system
+* with two DDR3L 64bit dimm all four bits were set.
What bits are set? It would be good to clarify the register.
number of channel-active bits in P_CR_MC_BIOS_REQ_0_0_0_MCHBAR register,
Will update the comment.
+*/
+
+ switch (dram_type) {
+ case DRAM_TYPE_LPDDR3:
+ case DRAM_TYPE_LPDDR4:
+ dev_priv->memdev_info.data_width = 4;
+ dev_priv->memdev_info.num_channel = num_channel;
+ break;
+ case DRAM_TYPE_DDR3L:
+ dev_priv->memdev_info.data_width = 8;
+ dev_priv->memdev_info.num_channel = num_channel / 2;
Why is this /2 done here?
This is the same reason as in comment section above, 2 bits per channel
are set in P_CR_MC_BIOS_REQ_0_0_0 register in-case of DDR3L DRAM,
So we need to divide "number of bit set" by 2 to get actual number of
channel for DDR3L.
+ break;
+ default:
+ dev_priv->memdev_info.data_width = 4;
+ dev_priv->memdev_info.num_channel = num_channel;
+ }
+
+ /*
+* Now read each DUNIT8/9/10/11 to check the rank of each dimms.
+* all the dimms should have same rank as in first valid Dimm
+*/
+#define D_CR_DRP0_DUNIT_INVALID0x
+
+ dev_priv->memdev_info.rank_valid = true;
+ if (I915_READ(D_CR_DRP0_DUNIT8) != D_CR_DRP0_DUNIT_INVALID) {
+ val = I915_READ(D_CR_DRP0_DUNIT8);
+ rank_valid = true;
+ } else if (I915_READ(D_CR_DRP0_DUNIT9) != D_CR_DRP0_DUNIT_INVALID) {
+ val = I915_READ(D_CR_DRP0_DUNIT9);
+ rank_valid = true;
+ } else if (I915_READ(D_CR_DRP0_DUNIT10) != D_CR_DRP0_DUNIT_INVALID) {
+ val = I915_READ(D_CR_DRP0_DUNIT10);
+ rank_valid = true;
+ } else if (I915_READ(D_CR_DRP0_DUNIT11) != D_CR_DRP0_DUNIT_INVALID) {
+ val = I915_READ(D_CR_DRP0_DUNIT11);
+ rank_valid = true;
+ }
+#undef D_CR_DRP0_DUNIT_INVALID
+
+ if (rank_valid) {
+ dev_priv->memdev_info.rank_valid = true;
+ dev_priv->memdev_info.rank = (val & DRAM_RANK_MASK);
+ }
+
+ DRM_DEBUG_DRIVER("valid:%s speed-%d width-%d num_channel-%d\n",
+ dev_priv->memdev_info.valid ? "true" : "false",
+ dev_priv->memdev_info.mem_speed,
+ dev_priv->memdev_info.data_width,
+ dev_priv->memdev_info.num_channel);
+ DRM_DEBUG_DRIVER("rank_valid:%s rank-%d\n",
+ dev_priv->memdev_info.rank_valid ? "true" : "false",
+ dev_priv->memdev_info.rank);
+ return;
+exit:
+ dev_priv->memdev_info.valid = false;
+}
+
/**
* i915_driver_init_hw - setup state requiring device access
* @dev_priv: device private
@@ -1076,6 +1166,12 @@ static int i915_driver_init_hw(struct drm_i915_private
*de
Re: [Intel-gfx] [PATCH v3 4/9] drm/i915: Decode system memory bandwidth
On Fri, 2016-09-09 at 13:31 +0530, Kumar, Mahesh wrote:
> From: Mahesh Kumar
>
> This patch adds support to decode system memory bandwidth
> which will be used for arbitrated display memory percentage
> calculation in GEN9 based system.
>
> Signed-off-by: Mahesh Kumar
> ---
> drivers/gpu/drm/i915/i915_drv.c | 96
> +
> drivers/gpu/drm/i915/i915_drv.h | 18
> drivers/gpu/drm/i915/i915_reg.h | 25 +++
> 3 files changed, 139 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 02c34d6..0a4f18d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -973,6 +973,96 @@ static void intel_sanitize_options(struct
> drm_i915_private *dev_priv)
> DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores));
> }
>
> +static void
> +intel_get_memdev_info(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + uint32_t val = 0;
> + uint32_t mem_speed = 0;
> + uint8_t dram_type;
> + uint32_t dram_channel;
> + uint8_t num_channel;
> + bool rank_valid = false;
> +
> + if (!IS_GEN9(dev_priv))
> + goto exit;
> +
> + val = I915_READ(P_CR_MC_BIOS_REQ_0_0_0);
> + mem_speed = div_u64((uint64_t) (val & REQ_DATA_MASK) *
> + MEMORY_FREQ_MULTIPLIER, 1000);
> +
> + if (mem_speed == 0)
> + goto exit;
> +
> + dev_priv->memdev_info.valid = true;
> + dev_priv->memdev_info.mem_speed = mem_speed;
> + dram_type = (val >> DRAM_TYPE_SHIFT) & DRAM_TYPE_MASK;
> + dram_channel = (val >> DRAM_CHANNEL_SHIFT) & DRAM_CHANNEL_MASK;
> + num_channel = hweight32(dram_channel);
> +
> + /*
> + * The lpddr3 and lpddr4 technologies can have 1-4 channels and the
> + * channels are 32bits wide; while ddr3l technologies can have 1-2
> + * channels and the channels are 64 bits wide. But SV team found that in
> + * case of single 64 bit wide DDR3L dimms two bits were set and system
> + * with two DDR3L 64bit dimm all four bits were set.
What bits are set? It would be good to clarify the register.
> + */
> +
> + switch (dram_type) {
> + case DRAM_TYPE_LPDDR3:
> + case DRAM_TYPE_LPDDR4:
> + dev_priv->memdev_info.data_width = 4;
> + dev_priv->memdev_info.num_channel = num_channel;
> + break;
> + case DRAM_TYPE_DDR3L:
> + dev_priv->memdev_info.data_width = 8;
> + dev_priv->memdev_info.num_channel = num_channel / 2;
Why is this /2 done here?
> + break;
> + default:
> + dev_priv->memdev_info.data_width = 4;
> + dev_priv->memdev_info.num_channel = num_channel;
> + }
> +
> + /*
> + * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
> + * all the dimms should have same rank as in first valid Dimm
> + */
> +#define D_CR_DRP0_DUNIT_INVALID 0x
> +
> + dev_priv->memdev_info.rank_valid = true;
> + if (I915_READ(D_CR_DRP0_DUNIT8) != D_CR_DRP0_DUNIT_INVALID) {
> + val = I915_READ(D_CR_DRP0_DUNIT8);
> + rank_valid = true;
> + } else if (I915_READ(D_CR_DRP0_DUNIT9) != D_CR_DRP0_DUNIT_INVALID) {
> + val = I915_READ(D_CR_DRP0_DUNIT9);
> + rank_valid = true;
> + } else if (I915_READ(D_CR_DRP0_DUNIT10) != D_CR_DRP0_DUNIT_INVALID) {
> + val = I915_READ(D_CR_DRP0_DUNIT10);
> + rank_valid = true;
> + } else if (I915_READ(D_CR_DRP0_DUNIT11) != D_CR_DRP0_DUNIT_INVALID) {
> + val = I915_READ(D_CR_DRP0_DUNIT11);
> + rank_valid = true;
> + }
> +#undef D_CR_DRP0_DUNIT_INVALID
> +
> + if (rank_valid) {
> + dev_priv->memdev_info.rank_valid = true;
> + dev_priv->memdev_info.rank = (val & DRAM_RANK_MASK);
> + }
> +
> + DRM_DEBUG_DRIVER("valid:%s speed-%d width-%d num_channel-%d\n",
> + dev_priv->memdev_info.valid ? "true" : "false",
> + dev_priv->memdev_info.mem_speed,
> + dev_priv->memdev_info.data_width,
> + dev_priv->memdev_info.num_channel);
> + DRM_DEBUG_DRIVER("rank_valid:%s rank-%d\n",
> + dev_priv->memdev_info.rank_valid ? "true" : "false",
> + dev_priv->memdev_info.rank);
> + return;
> +exit:
> + dev_priv->memdev_info.valid = false;
> +}
> +
> /**
> * i915_driver_init_hw - setup state requiring device access
> * @dev_priv: device private
> @@ -1076,6 +1166,12 @@ static int i915_driver_init_hw(struct drm_i915_private
> *dev_priv)
> DRM_DEBUG_DRIVER("can't enable MSI");
> }
>
> + /*
> + * Fill the memdev structure to get the system raw bandwidth
> + * This will be used by WM algorithm, to implement GEN9 based WA
> + */
> + intel_get_memdev_info(dev);
> +
> return 0;
>
> out_ggtt:
>
