Hi
Em Sex, 2016-09-09 às 13:31 +0530, Kumar, Mahesh escreveu:
> From: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>
>
> 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 <mahesh1.kumar@xxxxxxxxx>
> ---
> 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 0xFFFFFFFF
I'm not a huge fan of these mid-file defines with later undefines.
Can't we just move this to the .h file, or just use a variable, or just
go with the plain 0xFFFFFFFF?
> +
> + dev_priv->memdev_info.rank_valid = true;
We never set this to false. Bug?
> + 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;
> + }
You can restructure this to avoid the double I915_READ for the valid
case. There are many ways to do this.
Also, you could get rid of the rank_valid variable if you reorganize
things a little bit and then just add an extra "else" for the
"rank_valid is false" case.
> +#undef D_CR_DRP0_DUNIT_INVALID
> +
> + if (rank_valid) {
> + dev_priv->memdev_info.rank_valid = true;
But we just set dev_priv->memdev_info.rank_valid to true above, so no
need to redo it here.
> + dev_priv->memdev_info.rank = (val & DRAM_RANK_MASK);
> + }
> +
> + DRM_DEBUG_DRIVER("valid:%s speed-%d width-%d num_channel-
> %d\n",
Speed, width and num_channel are unsigned, and not all of them are 32
bits. Please replace the %d with the more appropriate modifiers and
specifiers.
> + dev_priv->memdev_info.valid ? "true" : "false",
Perhaps you could use yesno() here and below.
Also, you could prettify the text a little bit so people would be able
to read dmesg and figure out interesting information about their DRAM
without needing to look at the code.
> + 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:
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 4ec23e5..4313992 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2036,6 +2036,24 @@ struct drm_i915_private {
> bool distrust_bios_wm;
> } wm;
>
> + struct {
> + /*
> + * memory device info
> + * valid: memory info is valid or not
> + * mem_speed: memory freq in KHz
> + * channel_width: Channel width in bytes
channel_width or data_width?
> + * num_channel: total number of channels
> + * rank: 0-rank disable, 1-Single rank, 2-dual rank
> + */
> + bool valid;
> + uint32_t mem_speed;
> + uint8_t data_width;
> + uint8_t num_channel;
> + bool rank_valid;
> + uint8_t rank;
You can remove the comments if you do this:
struct memdev_info {
bool valid;
uint32_t mem_speed_khz;
uint8_t channel/data_width_bytes;
uint8_t num_channels; (note the plural)
enum {
SOMETHING_RANK_DISABLE = 0,
SOMETHING_RANK_SINGLE,
SOMETHING_RANK_DUAL
} rank;
} memdev_info;
I *really* like documenting units in the variable names, since it
avoids constant doc checking.
> + } memdev_info;
> +
> +
> struct i915_runtime_pm pm;
>
> /* Abstract the submission mechanism (legacy ringbuffer or
> execlists) away */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index a29d707..b38445c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7716,6 +7716,31 @@ enum {
> #define DC_STATE_DEBUG_MASK_CORES (1<<0)
> #define DC_STATE_DEBUG_MASK_MEMORY_UP (1<<1)
>
> +#define P_CR_MC_BIOS_REQ_0_0_0 _MMIO(MCHBAR_MIRROR_BA
> SE_SNB + 0x7114)
> +#define REQ_DATA_MASK (0x3F << 0)
> +#define DRAM_TYPE_SHIFT 24
> +#define DRAM_TYPE_MASK 0x7
> +#define DRAM_CHANNEL_SHIFT 12
> +#define DRAM_CHANNEL_MASK 0xF
> +
> +#define DRAM_TYPE_LPDDR3 0x1
> +#define DRAM_TYPE_LPDDR4 0x2
> +#define DRAM_TYPE_DDR3L 0x4
> +/*
> + * BIOS programs this field of REQ_DATA [5:0] in integer
> + * multiple of 133330 KHz (133.33MHz)
> + */
> +#define MEMORY_FREQ_MULTIPLIER 0x208D2
> +#define D_CR_DRP0_DUNIT8 _MMIO(MCHBAR_MIRROR_BASE_SNB
> + 0x1000)
> +#define D_CR_DRP0_DUNIT9 _MMIO(MCHBAR_MIRROR_BASE_SNB
> + 0x1200)
> +#define D_CR_DRP0_DUNIT10 _MMIO(MCHBAR_MIRROR_BASE_SN
> B + 0x1400)
> +#define D_CR_DRP0_DUNIT11 _MMIO(MCHBAR_MIRROR_BASE_SN
> B + 0x1600)
> +#define D_CR_DRP0_RKEN0 (1 << 0)
> +#define D_CR_DRP0_RKEN1 (1 << 1)
> +#define DRAM_RANK_MASK 0x3
> +#define DRAM_SINGLE_RANK 0x1
> +#define DRAM_DUAL_RANK 0x3
I'd prefix all/most of these definitions with something to group them
all. Generic names such as MEMORY_FREQ_MULTIPLIER or DRAM_TYPE_LPDD3
may be confusing and/or cause problems later.
Thanks,
Paulo
> +
> /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using
> this register,
> * since on HSW we can't write to it using I915_WRITE. */
> #define D_COMP_HSW _MMIO(MCHBAR_MIRROR_BASE_S
> NB + 0x5F0C)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
