| Message ID | 20220427003509.267683-1-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dmc: Add MMIO range restrictions | expand |
Hi Anusha,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-tip/drm-tip next-20220426]
[cannot apply to v5.18-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Anusha-Srivatsa/drm-i915-dmc-Add-MMIO-range-restrictions/20220427-084021
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-a012-20220425 (https://download.01.org/0day-ci/archive/20220427/202204271216.6t3eWj4f-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/f79241ea04e8815b3c1b0ab6b9d6136efc8646d3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anusha-Srivatsa/drm-i915-dmc-Add-MMIO-range-restrictions/20220427-084021
git checkout f79241ea04e8815b3c1b0ab6b9d6136efc8646d3
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/display/intel_dmc.c: In function 'parse_dmc_fw_header':
>> drivers/gpu/drm/i915/display/intel_dmc.c:476:9: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
476 | if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count, dmc_header->header_ver, dmc_id))
| ^~
drivers/gpu/drm/i915/display/intel_dmc.c:478:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
478 | return 0;
| ^~~~~~
vim +/if +476 drivers/gpu/drm/i915/display/intel_dmc.c
406
407 static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
408 const struct intel_dmc_header_base *dmc_header,
409 size_t rem_size, u8 dmc_id)
410 {
411 struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), dmc);
412 struct dmc_fw_info *dmc_info = &dmc->dmc_info[dmc_id];
413 unsigned int header_len_bytes, dmc_header_size, payload_size, i;
414 const u32 *mmioaddr, *mmiodata;
415 u32 mmio_count, mmio_count_max, start_mmioaddr;
416 u8 *payload;
417
418 BUILD_BUG_ON(ARRAY_SIZE(dmc_info->mmioaddr) < DMC_V3_MAX_MMIO_COUNT ||
419 ARRAY_SIZE(dmc_info->mmioaddr) < DMC_V1_MAX_MMIO_COUNT);
420
421 /*
422 * Check if we can access common fields, we will checkc again below
423 * after we have read the version
424 */
425 if (rem_size < sizeof(struct intel_dmc_header_base))
426 goto error_truncated;
427
428 /* Cope with small differences between v1 and v3 */
429 if (dmc_header->header_ver == 3) {
430 const struct intel_dmc_header_v3 *v3 =
431 (const struct intel_dmc_header_v3 *)dmc_header;
432
433 if (rem_size < sizeof(struct intel_dmc_header_v3))
434 goto error_truncated;
435
436 mmioaddr = v3->mmioaddr;
437 mmiodata = v3->mmiodata;
438 mmio_count = v3->mmio_count;
439 mmio_count_max = DMC_V3_MAX_MMIO_COUNT;
440 /* header_len is in dwords */
441 header_len_bytes = dmc_header->header_len * 4;
442 start_mmioaddr = v3->start_mmioaddr;
443 dmc_header_size = sizeof(*v3);
444 } else if (dmc_header->header_ver == 1) {
445 const struct intel_dmc_header_v1 *v1 =
446 (const struct intel_dmc_header_v1 *)dmc_header;
447
448 if (rem_size < sizeof(struct intel_dmc_header_v1))
449 goto error_truncated;
450
451 mmioaddr = v1->mmioaddr;
452 mmiodata = v1->mmiodata;
453 mmio_count = v1->mmio_count;
454 mmio_count_max = DMC_V1_MAX_MMIO_COUNT;
455 header_len_bytes = dmc_header->header_len;
456 start_mmioaddr = DMC_V1_MMIO_START_RANGE;
457 dmc_header_size = sizeof(*v1);
458 } else {
459 drm_err(&i915->drm, "Unknown DMC fw header version: %u\n",
460 dmc_header->header_ver);
461 return 0;
462 }
463
464 if (header_len_bytes != dmc_header_size) {
465 drm_err(&i915->drm, "DMC firmware has wrong dmc header length "
466 "(%u bytes)\n", header_len_bytes);
467 return 0;
468 }
469
470 /* Cache the dmc header info. */
471 if (mmio_count > mmio_count_max) {
472 drm_err(&i915->drm, "DMC firmware has wrong mmio count %u\n", mmio_count);
473 return 0;
474 }
475
> 476 if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count, dmc_header->header_ver, dmc_id))
477 drm_err(&i915->drm, "DMC firmware has Wrong MMIO Addresses\n");
478 return 0;
479
480 for (i = 0; i < mmio_count; i++) {
481 dmc_info->mmioaddr[i] = _MMIO(mmioaddr[i]);
482 dmc_info->mmiodata[i] = mmiodata[i];
483 }
484 dmc_info->mmio_count = mmio_count;
485 dmc_info->start_mmioaddr = start_mmioaddr;
486
487 rem_size -= header_len_bytes;
488
489 /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
490 payload_size = dmc_header->fw_size * 4;
491 if (rem_size < payload_size)
492 goto error_truncated;
493
494 if (payload_size > dmc->max_fw_size) {
495 drm_err(&i915->drm, "DMC FW too big (%u bytes)\n", payload_size);
496 return 0;
497 }
498 dmc_info->dmc_fw_size = dmc_header->fw_size;
499
500 dmc_info->payload = kmalloc(payload_size, GFP_KERNEL);
501 if (!dmc_info->payload)
502 return 0;
503
504 payload = (u8 *)(dmc_header) + header_len_bytes;
505 memcpy(dmc_info->payload, payload, payload_size);
506
507 return header_len_bytes + payload_size;
508
509 error_truncated:
510 drm_err(&i915->drm, "Truncated DMC firmware, refusing.\n");
511 return 0;
512 }
513
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@vger.kernel.org> >Cc: Lucas De Marchi <lucas.demarchi@intel.com> >Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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. >+ } >+ } >+ >+ 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? >+ > 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 >+#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? Lucas De Marchi >+ _TGL_PIPEA_MMIO_START,\ >+ _TGL_PIPEB_MMIO_START,\ >+ _TGL_PIPEC_MMIO_START,\ >+ _TGL_PIPEB_MMIO_START) >+ >+#define TGL_DMC_MMIO_END(pipe) _PICK(pipe, _TGL_MAIN_MMIO_END,\ >+ _TGL_PIPEA_MMIO_END,\ >+ _TGL_PIPEB_MMIO_END,\ >+ _TGL_PIPEC_MMIO_END,\ >+ _TGL_PIPEB_MMIO_END) >+ > #define SKL_DMC_DC3_DC5_COUNT _MMIO(0x80030) > #define SKL_DMC_DC5_DC6_COUNT _MMIO(0x8002C) > #define BXT_DMC_DC3_DC5_COUNT _MMIO(0x80038) >-- >2.25.1 >
Hi Anusha,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip next-20220426]
[cannot apply to v5.18-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Anusha-Srivatsa/drm-i915-dmc-Add-MMIO-range-restrictions/20220427-084021
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20220427/202204271502.BuTprbqW-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/f79241ea04e8815b3c1b0ab6b9d6136efc8646d3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anusha-Srivatsa/drm-i915-dmc-Add-MMIO-range-restrictions/20220427-084021
git checkout f79241ea04e8815b3c1b0ab6b9d6136efc8646d3
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/display/intel_dmc.c: In function 'parse_dmc_fw_header':
>> drivers/gpu/drm/i915/display/intel_dmc.c:476:9: error: this 'if' clause does not guard... [-Werror=misleading-indentation]
476 | if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count, dmc_header->header_ver, dmc_id))
| ^~
drivers/gpu/drm/i915/display/intel_dmc.c:478:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
478 | return 0;
| ^~~~~~
cc1: all warnings being treated as errors
vim +/if +476 drivers/gpu/drm/i915/display/intel_dmc.c
406
407 static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
408 const struct intel_dmc_header_base *dmc_header,
409 size_t rem_size, u8 dmc_id)
410 {
411 struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), dmc);
412 struct dmc_fw_info *dmc_info = &dmc->dmc_info[dmc_id];
413 unsigned int header_len_bytes, dmc_header_size, payload_size, i;
414 const u32 *mmioaddr, *mmiodata;
415 u32 mmio_count, mmio_count_max, start_mmioaddr;
416 u8 *payload;
417
418 BUILD_BUG_ON(ARRAY_SIZE(dmc_info->mmioaddr) < DMC_V3_MAX_MMIO_COUNT ||
419 ARRAY_SIZE(dmc_info->mmioaddr) < DMC_V1_MAX_MMIO_COUNT);
420
421 /*
422 * Check if we can access common fields, we will checkc again below
423 * after we have read the version
424 */
425 if (rem_size < sizeof(struct intel_dmc_header_base))
426 goto error_truncated;
427
428 /* Cope with small differences between v1 and v3 */
429 if (dmc_header->header_ver == 3) {
430 const struct intel_dmc_header_v3 *v3 =
431 (const struct intel_dmc_header_v3 *)dmc_header;
432
433 if (rem_size < sizeof(struct intel_dmc_header_v3))
434 goto error_truncated;
435
436 mmioaddr = v3->mmioaddr;
437 mmiodata = v3->mmiodata;
438 mmio_count = v3->mmio_count;
439 mmio_count_max = DMC_V3_MAX_MMIO_COUNT;
440 /* header_len is in dwords */
441 header_len_bytes = dmc_header->header_len * 4;
442 start_mmioaddr = v3->start_mmioaddr;
443 dmc_header_size = sizeof(*v3);
444 } else if (dmc_header->header_ver == 1) {
445 const struct intel_dmc_header_v1 *v1 =
446 (const struct intel_dmc_header_v1 *)dmc_header;
447
448 if (rem_size < sizeof(struct intel_dmc_header_v1))
449 goto error_truncated;
450
451 mmioaddr = v1->mmioaddr;
452 mmiodata = v1->mmiodata;
453 mmio_count = v1->mmio_count;
454 mmio_count_max = DMC_V1_MAX_MMIO_COUNT;
455 header_len_bytes = dmc_header->header_len;
456 start_mmioaddr = DMC_V1_MMIO_START_RANGE;
457 dmc_header_size = sizeof(*v1);
458 } else {
459 drm_err(&i915->drm, "Unknown DMC fw header version: %u\n",
460 dmc_header->header_ver);
461 return 0;
462 }
463
464 if (header_len_bytes != dmc_header_size) {
465 drm_err(&i915->drm, "DMC firmware has wrong dmc header length "
466 "(%u bytes)\n", header_len_bytes);
467 return 0;
468 }
469
470 /* Cache the dmc header info. */
471 if (mmio_count > mmio_count_max) {
472 drm_err(&i915->drm, "DMC firmware has wrong mmio count %u\n", mmio_count);
473 return 0;
474 }
475
> 476 if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count, dmc_header->header_ver, dmc_id))
477 drm_err(&i915->drm, "DMC firmware has Wrong MMIO Addresses\n");
478 return 0;
479
480 for (i = 0; i < mmio_count; i++) {
481 dmc_info->mmioaddr[i] = _MMIO(mmioaddr[i]);
482 dmc_info->mmiodata[i] = mmiodata[i];
483 }
484 dmc_info->mmio_count = mmio_count;
485 dmc_info->start_mmioaddr = start_mmioaddr;
486
487 rem_size -= header_len_bytes;
488
489 /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
490 payload_size = dmc_header->fw_size * 4;
491 if (rem_size < payload_size)
492 goto error_truncated;
493
494 if (payload_size > dmc->max_fw_size) {
495 drm_err(&i915->drm, "DMC FW too big (%u bytes)\n", payload_size);
496 return 0;
497 }
498 dmc_info->dmc_fw_size = dmc_header->fw_size;
499
500 dmc_info->payload = kmalloc(payload_size, GFP_KERNEL);
501 if (!dmc_info->payload)
502 return 0;
503
504 payload = (u8 *)(dmc_header) + header_len_bytes;
505 memcpy(dmc_info->payload, payload, payload_size);
506
507 return header_len_bytes + payload_size;
508
509 error_truncated:
510 drm_err(&i915->drm, "Truncated DMC firmware, refusing.\n");
511 return 0;
512 }
513
[...] > + 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; > + mh? :)
> -----Original Message----- > From: De Marchi, Lucas <lucas.demarchi@intel.com> > Sent: Tuesday, April 26, 2022 10:42 PM > To: Srivatsa, Anusha <anusha.srivatsa@intel.com> > Cc: intel-gfx@lists.freedesktop.org; stable@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@vger.kernel.org> > >Cc: Lucas De Marchi <lucas.demarchi@intel.com> > >Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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. Anusha > Lucas De Marchi > > >+ _TGL_PIPEA_MMIO_START,\ > >+ _TGL_PIPEB_MMIO_START,\ > >+ _TGL_PIPEC_MMIO_START,\ > >+ _TGL_PIPEB_MMIO_START) > >+ > >+#define TGL_DMC_MMIO_END(pipe) _PICK(pipe, > _TGL_MAIN_MMIO_END,\ > >+ _TGL_PIPEA_MMIO_END,\ > >+ _TGL_PIPEB_MMIO_END,\ > >+ _TGL_PIPEC_MMIO_END,\ > >+ _TGL_PIPEB_MMIO_END) > >+ > > #define SKL_DMC_DC3_DC5_COUNT _MMIO(0x80030) > > #define SKL_DMC_DC5_DC6_COUNT _MMIO(0x8002C) > > #define BXT_DMC_DC3_DC5_COUNT _MMIO(0x80038) > >-- > >2.25.1 > >
On Fri, Apr 29, 2022 at 01:39:03PM -0700, Anusha Srivatsa wrote: > > >> -----Original Message----- >> From: De Marchi, Lucas <lucas.demarchi@intel.com> >> Sent: Tuesday, April 26, 2022 10:42 PM >> To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >> Cc: intel-gfx@lists.freedesktop.org; stable@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@vger.kernel.org> >> >Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> >Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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
> -----Original Message----- > From: De Marchi, Lucas <lucas.demarchi@intel.com> > Sent: Friday, April 29, 2022 1:50 PM > To: Srivatsa, Anusha <anusha.srivatsa@intel.com> > Cc: intel-gfx@lists.freedesktop.org; stable@vger.kernel.org > Subject: Re: [PATCH] drm/i915/dmc: Add MMIO range restrictions > > On Fri, Apr 29, 2022 at 01:39:03PM -0700, Anusha Srivatsa wrote: > > > > > >> -----Original Message----- > >> From: De Marchi, Lucas <lucas.demarchi@intel.com> > >> Sent: Tuesday, April 26, 2022 10:42 PM > >> To: Srivatsa, Anusha <anusha.srivatsa@intel.com> > >> Cc: intel-gfx@lists.freedesktop.org; stable@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@vger.kernel.org> > >> >Cc: Lucas De Marchi <lucas.demarchi@intel.com> > >> >Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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. Ah, yes. I see the confusion now. It is expecting the dmc_id ,yes. In the usage of the macro in the beginning of the patch though, I am using dmc_id. Thought that would make it clearer, still pipe was the wrong choice of var name. > 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() While the Pipe DMC s are evenly spaced and _PICK_EVEN is the right choice. The dmc_id for PipeA, PipeB ....will be 1,2....and not 0,1,2 so the helper will return the wrong values. But you are suggesting to use PIPE_A, PIPE_B etc which will be 0 indexed. But here in the code we are parsing the dmc binary to see if it has Pipe DMC and if so what the MMIO offsets are they in and if it is a valid blob or not. The data we can use at this point is the dmc_id...... Unless we do a conversion from dmc-id to the pipe: If (DMC_FW_PIPEA) TGL_DMC_MMIO_START(PIPE_A) _PICK( That will lead to individual such conditions per pipe. Anusha > Lucas De Marchi
On Fri, Apr 29, 2022 at 01:49:37PM -0700, Lucas De Marchi wrote: >On Fri, Apr 29, 2022 at 01:39:03PM -0700, Anusha Srivatsa wrote: >> >> >>>-----Original Message----- >>>From: De Marchi, Lucas <lucas.demarchi@intel.com> >>>Sent: Tuesday, April 26, 2022 10:42 PM >>>To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >>>Cc: intel-gfx@lists.freedesktop.org; stable@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@vger.kernel.org> >>>>Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>>>Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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() usually we have helper functions/macros to do that kind of conversion. dmc_id to pipe is id - 1. *With the proper range checks* somewhere to ensure you aren't accessing the wrong address you could even embed the conversion: #define TGL_PIPE_DMC_MMIO_START(dmc_id) _PICK_EVEN((dmc_id - 1), ... or add a helper macro to avoid the repetition and document what this is about. Lucas De Marchi
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; + } + } + + /* 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; + } + } 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; + } + } + + 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; + 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 +#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,\ + _TGL_PIPEA_MMIO_START,\ + _TGL_PIPEB_MMIO_START,\ + _TGL_PIPEC_MMIO_START,\ + _TGL_PIPEB_MMIO_START) + +#define TGL_DMC_MMIO_END(pipe) _PICK(pipe, _TGL_MAIN_MMIO_END,\ + _TGL_PIPEA_MMIO_END,\ + _TGL_PIPEB_MMIO_END,\ + _TGL_PIPEC_MMIO_END,\ + _TGL_PIPEB_MMIO_END) + #define SKL_DMC_DC3_DC5_COUNT _MMIO(0x80030) #define SKL_DMC_DC5_DC6_COUNT _MMIO(0x8002C) #define BXT_DMC_DC3_DC5_COUNT _MMIO(0x80038)
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@vger.kernel.org> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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(-)