[Intel-gfx] [PATCH] drm/i915/dmc: Add MMIO range restrictions
Lucas De Marchi
lucas.demarchi at intel.com
Fri Apr 29 20:49:37 UTC 2022
On Fri, Apr 29, 2022 at 01:39:03PM -0700, Anusha Srivatsa wrote:
>
>
>> -----Original Message-----
>> From: De Marchi, Lucas <lucas.demarchi at intel.com>
>> Sent: Tuesday, April 26, 2022 10:42 PM
>> To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
>> Cc: intel-gfx at lists.freedesktop.org; stable at vger.kernel.org
>> Subject: Re: [PATCH] drm/i915/dmc: Add MMIO range restrictions
>>
>> On Tue, Apr 26, 2022 at 05:35:09PM -0700, Anusha Srivatsa wrote:
>> >Bspec has added some steps that check forDMC MMIO range before
>> >programming them
>> >
>> >v2: Fix for CI
>> >v3: move register defines to .h (Anusha)
>> >- Check MMIO restrictions per pipe
>> >- Add MMIO restricton for v1 dmc header as well (Lucas)
>> >
>> >BSpec: 49193
>> >
>> >Cc: <stable at vger.kernel.org>
>> >Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> >Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> >---
>> > drivers/gpu/drm/i915/display/intel_dmc.c | 48 ++++++++++++++++---
>> > drivers/gpu/drm/i915/display/intel_dmc_regs.h | 31 ++++++++++++
>> > 2 files changed, 72 insertions(+), 7 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c
>> >b/drivers/gpu/drm/i915/display/intel_dmc.c
>> >index 257cf662f9f4..ac7896835bfa 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_dmc.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>> >@@ -97,13 +97,6 @@ MODULE_FIRMWARE(SKL_DMC_PATH);
>> > #define BXT_DMC_MAX_FW_SIZE 0x3000
>> > MODULE_FIRMWARE(BXT_DMC_PATH);
>> >
>> >-#define DMC_DEFAULT_FW_OFFSET 0xFFFFFFFF
>> >-#define PACKAGE_MAX_FW_INFO_ENTRIES 20
>> >-#define PACKAGE_V2_MAX_FW_INFO_ENTRIES 32
>> >-#define DMC_V1_MAX_MMIO_COUNT 8
>> >-#define DMC_V3_MAX_MMIO_COUNT 20
>> >-#define DMC_V1_MMIO_START_RANGE 0x80000
>> >-
>> > struct intel_css_header {
>> > /* 0x09 for DMC */
>> > u32 module_type;
>> >@@ -374,6 +367,43 @@ static void dmc_set_fw_offset(struct intel_dmc
>> *dmc,
>> > }
>> > }
>> >
>> >+static bool dmc_mmio_addr_sanity_check(struct intel_dmc *dmc, const
>> u32 *mmioaddr,
>> >+ u32 mmio_count, int header_ver, u8
>> dmc_id) {
>> >+ struct drm_i915_private *i915 = container_of(dmc, typeof(*i915),
>> dmc);
>> >+ int i;
>> >+
>> >+ if (header_ver == 1) {
>> >+ for (i = 0; i < mmio_count; i++) {
>> >+ if (mmioaddr[i] < DMC_MMIO_START_RANGE ||
>> mmioaddr[i] > DMC_MMIO_END_RANGE)
>> >+ return false;
>> >+ }
>>
>> return missing here
>>
>> >+ }
>> >+
>> >+ /* Main DMC MMIO check */
>> >+ if (dmc_id == DMC_FW_MAIN) {
>> >+ for (i = 0; i < mmio_count; i++) {
>> >+ if (mmioaddr[i] < TGL_DMC_MMIO_START(dmc_id)
>> || mmioaddr[i] > TGL_DMC_MMIO_END(dmc_id))
>> >+ return false;
>> >+ }
>> >+ }
>> >+
>> >+ /* Pipe DMC MMIO check */
>> >+ if (IS_DG2(i915) || IS_ALDERLAKE_P(i915)) {
>> >+ for (i = 0; i < mmio_count; i++) {
>> >+ if (mmioaddr[i] < ADLP_PIPE_MMIO_START &&
>> mmioaddr[i] > ADLP_PIPE_MMIO_END)
>> >+ return false;
>> >+ }
>>
>> for DG2, main should use TGL_DMC_MMIO_START? and then fail here
>> because of another missing return above?
>>
>> >+ } else if (IS_TIGERLAKE(i915) || IS_DG1(i915) ||
>> IS_ALDERLAKE_S(i915)) {
>> >+ for (i = 0; i < mmio_count; i++) {
>> >+ if (mmioaddr[i] < TGL_DMC_MMIO_START(dmc_id)
>> || mmioaddr[i] > TGL_DMC_MMIO_END(dmc_id))
>> >+ return false;
>>
>> this is handling DMC_FW_MAIN twice.
>>
>>
>> Maybe something like this:
>>
>> u32 start, end;
>>
>> if (header_ver == 1) {
>> start = DMC_MMIO_START_RANGE;
>> end = DMC_MMIO_END_RANGE;
>> } else if (dmc_id == DMC_FW_MAIN || IS_TIGERLAKE(i915) ||
>> IS_DG1(i915) || IS_ALDERLAKE_S(i915)) {
>> start = TGL_DMC_MMIO_START(dmc_id);
>> end = TGL_DMC_MMIO_END(dmc_id);
>> } else if (IS_DG2(i915) || IS_ALDERLAKE_P(i915)) {
>> start = ADLP_PIPE_MMIO_START;
>> end = ADLP_PIPE_MMIO_END;
>> } else {
>> drm_warn(&i915->drm, "Unknown mmio range for sanity
>> check");
>> return false;
>> }
>>
>> for (i = 0; i < mmio_count; i++)
>> if (mmioaddr[i] < start || mmioaddr[i] > end)
>> return false;
>>
>> return true;
>>
>>
>> ... untested, and may need tweaks depending on the answer to the question
>> above on what range to use for ADL-P/DG2 on main DMC.
>The above approach is definitely neater.
>The main DMC offset is the same for all platforms.
>
>> >+ }
>> >+ }
>> >+
>> >+ return true;
>> >+}
>> >+
>> > static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
>> > const struct intel_dmc_header_base
>> *dmc_header,
>> > size_t rem_size, u8 dmc_id)
>> >@@ -443,6 +473,10 @@ static u32 parse_dmc_fw_header(struct intel_dmc
>> *dmc,
>> > return 0;
>> > }
>> >
>> >+ if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count,
>> dmc_header->header_ver, dmc_id))
>> >+ drm_err(&i915->drm, "DMC firmware has Wrong MMIO
>> Addresses\n");
>> >+ return 0;
>>
>> you don't like DMC and decided to fail it for all platforms?
>
><facepalm>
>
>> >+
>> > for (i = 0; i < mmio_count; i++) {
>> > dmc_info->mmioaddr[i] = _MMIO(mmioaddr[i]);
>> > dmc_info->mmiodata[i] = mmiodata[i]; diff --git
>> >a/drivers/gpu/drm/i915/display/intel_dmc_regs.h
>> >b/drivers/gpu/drm/i915/display/intel_dmc_regs.h
>> >index d65e698832eb..235d1b721334 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_dmc_regs.h
>> >+++ b/drivers/gpu/drm/i915/display/intel_dmc_regs.h
>> >@@ -11,12 +11,43 @@
>> > #define DMC_PROGRAM(addr, i) _MMIO((addr) + (i) * 4)
>> > #define DMC_SSP_BASE_ADDR_GEN9 0x00002FC0
>> > #define DMC_HTP_ADDR_SKL 0x00500034
>> >+#define DMC_DEFAULT_FW_OFFSET 0xFFFFFFFF
>> > #define DMC_SSP_BASE _MMIO(0x8F074)
>> > #define DMC_HTP_SKL _MMIO(0x8F004)
>> > #define DMC_LAST_WRITE _MMIO(0x8F034)
>> > #define DMC_LAST_WRITE_VALUE 0xc003b400
>> > #define DMC_MMIO_START_RANGE 0x80000
>> > #define DMC_MMIO_END_RANGE 0x8FFFF
>> >+#define PACKAGE_MAX_FW_INFO_ENTRIES 20
>> >+#define PACKAGE_V2_MAX_FW_INFO_ENTRIES 32
>> >+#define DMC_V1_MAX_MMIO_COUNT 8
>> >+#define DMC_V3_MAX_MMIO_COUNT 20
>>
>>
>> why are you moving these to _regs? these describe the DMC header/blob
>
>Yeah my mistake. While making the change they seemed like the right thing to do.
>
>>
>> >+#define DMC_V1_MMIO_START_RANGE 0x80000
>> >+#define _TGL_MAIN_MMIO_START 0x8F000
>> >+#define _TGL_MAIN_MMIO_END 0x8FFFF
>> >+#define _TGL_PIPEA_MMIO_START 0x92000
>> >+#define _TGL_PIPEA_MMIO_END 0x93FFF
>> >+#define _TGL_PIPEB_MMIO_START 0x96000
>> >+#define _TGL_PIPEB_MMIO_END 0x97FFF
>> >+#define _TGL_PIPEC_MMIO_START 0x9A000
>> >+#define _TGL_PIPEC_MMIO_END 0x9BFFF
>> >+#define _TGL_PIPED_MMIO_START 0x9E000
>> >+#define _TGL_PIPED_MMIO_END 0x9FFFF
>> >+#define ADLP_PIPE_MMIO_START 0x5F000
>> >+#define ADLP_PIPE_MMIO_END 0x5FFFF
>> >+
>> >+#define TGL_DMC_MMIO_START(pipe) _PICK(pipe,
>> _TGL_MAIN_MMIO_START,\
>>
>> _PICK? Did you miss my previous review?
>
>No. the thing is Main DMC with the Pipe DMCs are not evenly spaced. So using PICK_EVEN is not the right choice here. We also don't need to do _MMIO really.....
>Unless I am missing something, this seems like the right approach.
Because the name you chose for your variable:
TGL_DMC_MMIO_START(pipe) _PICK(pipe,
I was expecting this to be used only with the pipe DMC address, which
are evenly spaced. It seems the argument you're expecting here is a
dmc_id. But.... you said:
>The main DMC offset is the same for all platforms.
So, maybe just handle that separately and keep using pipe here? Then you
can switch to _PICK_EVEN()
Lucas De Marchi
More information about the Intel-gfx
mailing list