| Message ID | 20220405171429.3149199-1-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dmc: Add MMIO range restrictions | expand |
On Tue, Apr 05, 2022 at 10:14:29AM -0700, Anusha Srivatsa wrote: >Bspec has added some steps that check for DMC MMIO range before >programming them. > >v2: Fix for CI failure for v1 > >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 | 42 ++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > >diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c >index 257cf662f9f4..05d8e90854ec 100644 >--- a/drivers/gpu/drm/i915/display/intel_dmc.c >+++ b/drivers/gpu/drm/i915/display/intel_dmc.c >@@ -103,6 +103,18 @@ MODULE_FIRMWARE(BXT_DMC_PATH); > #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 > > struct intel_css_header { > /* 0x09 for DMC */ >@@ -374,6 +386,30 @@ 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) >+{ >+ struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), dmc); >+ int i; >+ >+ if (IS_DG2(i915) || IS_ALDERLAKE_P(i915)) { >+ for (i = 0; i < mmio_count; i++) { >+ if (!((mmioaddr[i] >= TGL_MAIN_MMIO_START && mmioaddr[i] <= TGL_MAIN_MMIO_END) || >+ (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_MAIN_MMIO_START && mmioaddr[i] <= TGL_MAIN_MMIO_END) || >+ (mmioaddr[i] >= TGL_PIPEA_MMIO_START && mmioaddr[i] <= TGL_PIPEA_MMIO_END) || >+ (mmioaddr[i] >= TGL_PIPEB_MMIO_START && mmioaddr[i] <= TGL_PIPEB_MMIO_END) || >+ (mmioaddr[i] >= TGL_PIPEC_MMIO_START && mmioaddr[i] <= TGL_PIPEC_MMIO_END) || >+ (mmioaddr[i] >= TGL_PIPED_MMIO_START && mmioaddr[i] <= TGL_PIPEC_MMIO_END))) >+ return false; wonder if we should check for each pipe DMC range independently rather than just checking all the ranges. >+ } >+ 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 +479,12 @@ static u32 parse_dmc_fw_header(struct intel_dmc *dmc, > return 0; > } > >+ if (dmc_header->header_ver == 3) { this also needs to be done for ver 2 Lucas De Marchi
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 v5.18-rc1 next-20220405]
[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/20220406-011821
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-c002 (https://download.01.org/0day-ci/archive/20220406/202204060704.ALs2bEHY-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/53017f72f2ddb095da8d5ef1cb92d0b1d02c8a2b
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/20220406-011821
git checkout 53017f72f2ddb095da8d5ef1cb92d0b1d02c8a2b
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=x86_64 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 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:17: error: this 'if' clause does not guard... [-Werror=misleading-indentation]
476 | if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count))
| ^~
drivers/gpu/drm/i915/display/intel_dmc.c:478:25: 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
405
406 static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
407 const struct intel_dmc_header_base *dmc_header,
408 size_t rem_size, u8 dmc_id)
409 {
410 struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), dmc);
411 struct dmc_fw_info *dmc_info = &dmc->dmc_info[dmc_id];
412 unsigned int header_len_bytes, dmc_header_size, payload_size, i;
413 const u32 *mmioaddr, *mmiodata;
414 u32 mmio_count, mmio_count_max, start_mmioaddr;
415 u8 *payload;
416
417 BUILD_BUG_ON(ARRAY_SIZE(dmc_info->mmioaddr) < DMC_V3_MAX_MMIO_COUNT ||
418 ARRAY_SIZE(dmc_info->mmioaddr) < DMC_V1_MAX_MMIO_COUNT);
419
420 /*
421 * Check if we can access common fields, we will checkc again below
422 * after we have read the version
423 */
424 if (rem_size < sizeof(struct intel_dmc_header_base))
425 goto error_truncated;
426
427 /* Cope with small differences between v1 and v3 */
428 if (dmc_header->header_ver == 3) {
429 const struct intel_dmc_header_v3 *v3 =
430 (const struct intel_dmc_header_v3 *)dmc_header;
431
432 if (rem_size < sizeof(struct intel_dmc_header_v3))
433 goto error_truncated;
434
435 mmioaddr = v3->mmioaddr;
436 mmiodata = v3->mmiodata;
437 mmio_count = v3->mmio_count;
438 mmio_count_max = DMC_V3_MAX_MMIO_COUNT;
439 /* header_len is in dwords */
440 header_len_bytes = dmc_header->header_len * 4;
441 start_mmioaddr = v3->start_mmioaddr;
442 dmc_header_size = sizeof(*v3);
443 } else if (dmc_header->header_ver == 1) {
444 const struct intel_dmc_header_v1 *v1 =
445 (const struct intel_dmc_header_v1 *)dmc_header;
446
447 if (rem_size < sizeof(struct intel_dmc_header_v1))
448 goto error_truncated;
449
450 mmioaddr = v1->mmioaddr;
451 mmiodata = v1->mmiodata;
452 mmio_count = v1->mmio_count;
453 mmio_count_max = DMC_V1_MAX_MMIO_COUNT;
454 header_len_bytes = dmc_header->header_len;
455 start_mmioaddr = DMC_V1_MMIO_START_RANGE;
456 dmc_header_size = sizeof(*v1);
457 } else {
458 drm_err(&i915->drm, "Unknown DMC fw header version: %u\n",
459 dmc_header->header_ver);
460 return 0;
461 }
462
463 if (header_len_bytes != dmc_header_size) {
464 drm_err(&i915->drm, "DMC firmware has wrong dmc header length "
465 "(%u bytes)\n", header_len_bytes);
466 return 0;
467 }
468
469 /* Cache the dmc header info. */
470 if (mmio_count > mmio_count_max) {
471 drm_err(&i915->drm, "DMC firmware has wrong mmio count %u\n", mmio_count);
472 return 0;
473 }
474
475 if (dmc_header->header_ver == 3) {
> 476 if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count))
477 drm_err(&i915->drm, "DMC firmware has Wrong MMIO Addresses\n");
478 return 0;
479 }
480
481 for (i = 0; i < mmio_count; i++) {
482 dmc_info->mmioaddr[i] = _MMIO(mmioaddr[i]);
483 dmc_info->mmiodata[i] = mmiodata[i];
484 }
485 dmc_info->mmio_count = mmio_count;
486 dmc_info->start_mmioaddr = start_mmioaddr;
487
488 rem_size -= header_len_bytes;
489
490 /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
491 payload_size = dmc_header->fw_size * 4;
492 if (rem_size < payload_size)
493 goto error_truncated;
494
495 if (payload_size > dmc->max_fw_size) {
496 drm_err(&i915->drm, "DMC FW too big (%u bytes)\n", payload_size);
497 return 0;
498 }
499 dmc_info->dmc_fw_size = dmc_header->fw_size;
500
501 dmc_info->payload = kmalloc(payload_size, GFP_KERNEL);
502 if (!dmc_info->payload)
503 return 0;
504
505 payload = (u8 *)(dmc_header) + header_len_bytes;
506 memcpy(dmc_info->payload, payload, payload_size);
507
508 return header_len_bytes + payload_size;
509
510 error_truncated:
511 drm_err(&i915->drm, "Truncated DMC firmware, refusing.\n");
512 return 0;
513 }
514
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 v5.18-rc1 next-20220405]
[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/20220406-011821
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20220406/202204061418.8NyZZGOk-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/53017f72f2ddb095da8d5ef1cb92d0b1d02c8a2b
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/20220406-011821
git checkout 53017f72f2ddb095da8d5ef1cb92d0b1d02c8a2b
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=x86_64 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:17: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
476 | if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count))
| ^~
drivers/gpu/drm/i915/display/intel_dmc.c:478:25: 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
405
406 static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
407 const struct intel_dmc_header_base *dmc_header,
408 size_t rem_size, u8 dmc_id)
409 {
410 struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), dmc);
411 struct dmc_fw_info *dmc_info = &dmc->dmc_info[dmc_id];
412 unsigned int header_len_bytes, dmc_header_size, payload_size, i;
413 const u32 *mmioaddr, *mmiodata;
414 u32 mmio_count, mmio_count_max, start_mmioaddr;
415 u8 *payload;
416
417 BUILD_BUG_ON(ARRAY_SIZE(dmc_info->mmioaddr) < DMC_V3_MAX_MMIO_COUNT ||
418 ARRAY_SIZE(dmc_info->mmioaddr) < DMC_V1_MAX_MMIO_COUNT);
419
420 /*
421 * Check if we can access common fields, we will checkc again below
422 * after we have read the version
423 */
424 if (rem_size < sizeof(struct intel_dmc_header_base))
425 goto error_truncated;
426
427 /* Cope with small differences between v1 and v3 */
428 if (dmc_header->header_ver == 3) {
429 const struct intel_dmc_header_v3 *v3 =
430 (const struct intel_dmc_header_v3 *)dmc_header;
431
432 if (rem_size < sizeof(struct intel_dmc_header_v3))
433 goto error_truncated;
434
435 mmioaddr = v3->mmioaddr;
436 mmiodata = v3->mmiodata;
437 mmio_count = v3->mmio_count;
438 mmio_count_max = DMC_V3_MAX_MMIO_COUNT;
439 /* header_len is in dwords */
440 header_len_bytes = dmc_header->header_len * 4;
441 start_mmioaddr = v3->start_mmioaddr;
442 dmc_header_size = sizeof(*v3);
443 } else if (dmc_header->header_ver == 1) {
444 const struct intel_dmc_header_v1 *v1 =
445 (const struct intel_dmc_header_v1 *)dmc_header;
446
447 if (rem_size < sizeof(struct intel_dmc_header_v1))
448 goto error_truncated;
449
450 mmioaddr = v1->mmioaddr;
451 mmiodata = v1->mmiodata;
452 mmio_count = v1->mmio_count;
453 mmio_count_max = DMC_V1_MAX_MMIO_COUNT;
454 header_len_bytes = dmc_header->header_len;
455 start_mmioaddr = DMC_V1_MMIO_START_RANGE;
456 dmc_header_size = sizeof(*v1);
457 } else {
458 drm_err(&i915->drm, "Unknown DMC fw header version: %u\n",
459 dmc_header->header_ver);
460 return 0;
461 }
462
463 if (header_len_bytes != dmc_header_size) {
464 drm_err(&i915->drm, "DMC firmware has wrong dmc header length "
465 "(%u bytes)\n", header_len_bytes);
466 return 0;
467 }
468
469 /* Cache the dmc header info. */
470 if (mmio_count > mmio_count_max) {
471 drm_err(&i915->drm, "DMC firmware has wrong mmio count %u\n", mmio_count);
472 return 0;
473 }
474
475 if (dmc_header->header_ver == 3) {
> 476 if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count))
477 drm_err(&i915->drm, "DMC firmware has Wrong MMIO Addresses\n");
478 return 0;
479 }
480
481 for (i = 0; i < mmio_count; i++) {
482 dmc_info->mmioaddr[i] = _MMIO(mmioaddr[i]);
483 dmc_info->mmiodata[i] = mmiodata[i];
484 }
485 dmc_info->mmio_count = mmio_count;
486 dmc_info->start_mmioaddr = start_mmioaddr;
487
488 rem_size -= header_len_bytes;
489
490 /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
491 payload_size = dmc_header->fw_size * 4;
492 if (rem_size < payload_size)
493 goto error_truncated;
494
495 if (payload_size > dmc->max_fw_size) {
496 drm_err(&i915->drm, "DMC FW too big (%u bytes)\n", payload_size);
497 return 0;
498 }
499 dmc_info->dmc_fw_size = dmc_header->fw_size;
500
501 dmc_info->payload = kmalloc(payload_size, GFP_KERNEL);
502 if (!dmc_info->payload)
503 return 0;
504
505 payload = (u8 *)(dmc_header) + header_len_bytes;
506 memcpy(dmc_info->payload, payload, payload_size);
507
508 return header_len_bytes + payload_size;
509
510 error_truncated:
511 drm_err(&i915->drm, "Truncated DMC firmware, refusing.\n");
512 return 0;
513 }
514
> -----Original Message----- > From: De Marchi, Lucas <lucas.demarchi@intel.com> > Sent: Tuesday, April 5, 2022 11:03 AM > To: Srivatsa, Anusha <anusha.srivatsa@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/dmc: Add MMIO range restrictions > > On Tue, Apr 05, 2022 at 10:14:29AM -0700, Anusha Srivatsa wrote: > >Bspec has added some steps that check for DMC MMIO range before > >programming them. > > > >v2: Fix for CI failure for v1 > > > >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 | 42 > ++++++++++++++++++++++++ > > 1 file changed, 42 insertions(+) > > > >diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c > >b/drivers/gpu/drm/i915/display/intel_dmc.c > >index 257cf662f9f4..05d8e90854ec 100644 > >--- a/drivers/gpu/drm/i915/display/intel_dmc.c > >+++ b/drivers/gpu/drm/i915/display/intel_dmc.c > >@@ -103,6 +103,18 @@ MODULE_FIRMWARE(BXT_DMC_PATH); > > #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 > > > > struct intel_css_header { > > /* 0x09 for DMC */ > >@@ -374,6 +386,30 @@ 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) > >+{ > >+ struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), > dmc); > >+ int i; > >+ > >+ if (IS_DG2(i915) || IS_ALDERLAKE_P(i915)) { > >+ for (i = 0; i < mmio_count; i++) { > >+ if (!((mmioaddr[i] >= TGL_MAIN_MMIO_START && > mmioaddr[i] <= TGL_MAIN_MMIO_END) || > >+ (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_MAIN_MMIO_START && > mmioaddr[i] <= TGL_MAIN_MMIO_END) || > >+ (mmioaddr[i] >= TGL_PIPEA_MMIO_START && > mmioaddr[i] <= TGL_PIPEA_MMIO_END) || > >+ (mmioaddr[i] >= TGL_PIPEB_MMIO_START && > mmioaddr[i] <= TGL_PIPEB_MMIO_END) || > >+ (mmioaddr[i] >= TGL_PIPEC_MMIO_START && > mmioaddr[i] <= TGL_PIPEC_MMIO_END) || > >+ (mmioaddr[i] >= TGL_PIPED_MMIO_START && > mmioaddr[i] <= TGL_PIPEC_MMIO_END))) > >+ return false; > > wonder if we should check for each pipe DMC range independently rather > than just checking all the ranges. Can convert this to a switch case in that scenario. "If it is PIPE A then it must be within this range". But it will be 2 switches one for DG2 and ADLP and one for TGL and the rest which have individual ranges for every pipe. > >+ } > >+ 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 +479,12 @@ static u32 parse_dmc_fw_header(struct intel_dmc > *dmc, > > return 0; > > } > > > >+ if (dmc_header->header_ver == 3) { > > this also needs to be done for ver 2 For v2 though there has been no update about the start range. As in this mmio range is different from the RAM_MMIO_START range. Anusha > Lucas De Marchi
On Wed, Apr 06, 2022 at 10:16:55AM -0700, Anusha Srivatsa wrote: > > >> -----Original Message----- >> From: De Marchi, Lucas <lucas.demarchi@intel.com> >> Sent: Tuesday, April 5, 2022 11:03 AM >> To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >> Cc: intel-gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dmc: Add MMIO range restrictions >> >> On Tue, Apr 05, 2022 at 10:14:29AM -0700, Anusha Srivatsa wrote: >> >Bspec has added some steps that check for DMC MMIO range before >> >programming them. >> > >> >v2: Fix for CI failure for v1 >> > >> >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 | 42 >> ++++++++++++++++++++++++ >> > 1 file changed, 42 insertions(+) >> > >> >diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c >> >b/drivers/gpu/drm/i915/display/intel_dmc.c >> >index 257cf662f9f4..05d8e90854ec 100644 >> >--- a/drivers/gpu/drm/i915/display/intel_dmc.c >> >+++ b/drivers/gpu/drm/i915/display/intel_dmc.c >> >@@ -103,6 +103,18 @@ MODULE_FIRMWARE(BXT_DMC_PATH); >> > #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 >> > >> > struct intel_css_header { >> > /* 0x09 for DMC */ >> >@@ -374,6 +386,30 @@ 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) >> >+{ >> >+ struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), >> dmc); >> >+ int i; >> >+ >> >+ if (IS_DG2(i915) || IS_ALDERLAKE_P(i915)) { >> >+ for (i = 0; i < mmio_count; i++) { >> >+ if (!((mmioaddr[i] >= TGL_MAIN_MMIO_START && >> mmioaddr[i] <= TGL_MAIN_MMIO_END) || >> >+ (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_MAIN_MMIO_START && >> mmioaddr[i] <= TGL_MAIN_MMIO_END) || >> >+ (mmioaddr[i] >= TGL_PIPEA_MMIO_START && >> mmioaddr[i] <= TGL_PIPEA_MMIO_END) || >> >+ (mmioaddr[i] >= TGL_PIPEB_MMIO_START && >> mmioaddr[i] <= TGL_PIPEB_MMIO_END) || >> >+ (mmioaddr[i] >= TGL_PIPEC_MMIO_START && >> mmioaddr[i] <= TGL_PIPEC_MMIO_END) || >> >+ (mmioaddr[i] >= TGL_PIPED_MMIO_START && >> mmioaddr[i] <= TGL_PIPEC_MMIO_END))) >> >+ return false; >> >> wonder if we should check for each pipe DMC range independently rather >> than just checking all the ranges. > Can convert this to a switch case in that scenario. "If it is PIPE A then it must be within this range". But it will be 2 switches one for DG2 and ADLP and one for TGL and the rest which have individual ranges for every pipe. I was thinking more about like this: #define _TGL_PIPEA_MMIO 0x92000 #define _TGL_PIPEB_MMIO 0x96000 #define TGL_PIPE_MMIO(pipe) _MMIO_PIPE(pipe, _TGL_PIPEA_MMIO, _TGL_PIPEB_MMIO) #define TGL_PIPE_MMIO_SIZE 0x1000 This of course means that each blob is supposed to update only addresses on their own ranges. Is this true? Do we care? > >> >+ } >> >+ 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 +479,12 @@ static u32 parse_dmc_fw_header(struct intel_dmc >> *dmc, >> > return 0; >> > } >> > >> >+ if (dmc_header->header_ver == 3) { >> >> this also needs to be done for ver 2 >For v2 though there has been no update about the start range. As in this mmio range is different from the RAM_MMIO_START range. it is the same situation as v3. We read it from firmware. Why do you simply trust the value in v2 but you don't trust it in v3? You removed the check in 3d5928a168a9 ("drm/i915/xelpd: Pipe A DMC plugging") for (i = 0; i < mmio_count; i++) { - if (mmioaddr[i] < DMC_MMIO_START_RANGE || - mmioaddr[i] > DMC_MMIO_END_RANGE) { - drm_err(&i915->drm, "DMC firmware has wrong mmio address 0x%x\n", - mmioaddr[i]); - return 0; - } dmc_info->mmioaddr[i] = _MMIO(mmioaddr[i]); dmc_info->mmiodata[i] = mmiodata[i]; } I remember mentioning this during review, but let it pass. Lucas De Marchi > >Anusha > >> Lucas De Marchi
> -----Original Message----- > From: De Marchi, Lucas <lucas.demarchi@intel.com> > Sent: Wednesday, April 6, 2022 10:46 AM > To: Srivatsa, Anusha <anusha.srivatsa@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/dmc: Add MMIO range restrictions > > On Wed, Apr 06, 2022 at 10:16:55AM -0700, Anusha Srivatsa wrote: > > > > > >> -----Original Message----- > >> From: De Marchi, Lucas <lucas.demarchi@intel.com> > >> Sent: Tuesday, April 5, 2022 11:03 AM > >> To: Srivatsa, Anusha <anusha.srivatsa@intel.com> > >> Cc: intel-gfx@lists.freedesktop.org > >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dmc: Add MMIO range > >> restrictions > >> > >> On Tue, Apr 05, 2022 at 10:14:29AM -0700, Anusha Srivatsa wrote: > >> >Bspec has added some steps that check for DMC MMIO range before > >> >programming them. > >> > > >> >v2: Fix for CI failure for v1 > >> > > >> >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 | 42 > >> ++++++++++++++++++++++++ > >> > 1 file changed, 42 insertions(+) > >> > > >> >diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c > >> >b/drivers/gpu/drm/i915/display/intel_dmc.c > >> >index 257cf662f9f4..05d8e90854ec 100644 > >> >--- a/drivers/gpu/drm/i915/display/intel_dmc.c > >> >+++ b/drivers/gpu/drm/i915/display/intel_dmc.c > >> >@@ -103,6 +103,18 @@ MODULE_FIRMWARE(BXT_DMC_PATH); > >> > #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 > >> > > >> > struct intel_css_header { > >> > /* 0x09 for DMC */ > >> >@@ -374,6 +386,30 @@ 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) > >> >+{ > >> >+ struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), > >> dmc); > >> >+ int i; > >> >+ > >> >+ if (IS_DG2(i915) || IS_ALDERLAKE_P(i915)) { > >> >+ for (i = 0; i < mmio_count; i++) { > >> >+ if (!((mmioaddr[i] >= TGL_MAIN_MMIO_START && > >> mmioaddr[i] <= TGL_MAIN_MMIO_END) || > >> >+ (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_MAIN_MMIO_START && > >> mmioaddr[i] <= TGL_MAIN_MMIO_END) || > >> >+ (mmioaddr[i] >= TGL_PIPEA_MMIO_START && > >> mmioaddr[i] <= TGL_PIPEA_MMIO_END) || > >> >+ (mmioaddr[i] >= TGL_PIPEB_MMIO_START && > >> mmioaddr[i] <= TGL_PIPEB_MMIO_END) || > >> >+ (mmioaddr[i] >= TGL_PIPEC_MMIO_START && > >> mmioaddr[i] <= TGL_PIPEC_MMIO_END) || > >> >+ (mmioaddr[i] >= TGL_PIPED_MMIO_START && > >> mmioaddr[i] <= TGL_PIPEC_MMIO_END))) > >> >+ return false; > >> > >> wonder if we should check for each pipe DMC range independently > >> rather than just checking all the ranges. > > Can convert this to a switch case in that scenario. "If it is PIPE A then it must > be within this range". But it will be 2 switches one for DG2 and ADLP and one > for TGL and the rest which have individual ranges for every pipe. > > I was thinking more about like this: > > #define _TGL_PIPEA_MMIO 0x92000 > #define _TGL_PIPEB_MMIO 0x96000 > #define TGL_PIPE_MMIO(pipe) _MMIO_PIPE(pipe, _TGL_PIPEA_MMIO, > _TGL_PIPEB_MMIO) > #define TGL_PIPE_MMIO_SIZE 0x1000 Hmm, does it make sense to add something like: #define DMC_MMIO(dmc_id) _MMIO(_PICK(DMC_ID, DMC_FW_MAIN, DMC_FW_PIPEA, DMC_FW_PIPEB, DMC_FW_PIPEC, DMC_FW_PIPED) > This of course means that each blob is supposed to update only addresses > on their own ranges. Is this true? Do we care? Yes, pipe A will update its own range and so on. > > > >> >+ } > >> >+ 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 +479,12 > @@ static > >> >u32 parse_dmc_fw_header(struct intel_dmc > >> *dmc, > >> > return 0; > >> > } > >> > > >> >+ if (dmc_header->header_ver == 3) { > >> > >> this also needs to be done for ver 2 > >For v2 though there has been no update about the start range. As in this > mmio range is different from the RAM_MMIO_START range. > > it is the same situation as v3. We read it from firmware. Why do you simply > trust the value in v2 but you don't trust it in v3? You removed the check in > 3d5928a168a9 ("drm/i915/xelpd: Pipe A DMC plugging") > > for (i = 0; i < mmio_count; i++) { > - if (mmioaddr[i] < DMC_MMIO_START_RANGE || > - mmioaddr[i] > DMC_MMIO_END_RANGE) { > - drm_err(&i915->drm, "DMC firmware has wrong mmio address > 0x%x\n", > - mmioaddr[i]); > - return 0; > - } > dmc_info->mmioaddr[i] = _MMIO(mmioaddr[i]); > dmc_info->mmiodata[i] = mmiodata[i]; > } > > I remember mentioning this during review, but let it pass. Thanks for the above patch, will do the needful. Anusha > Lucas De Marchi > > > > >Anusha > > > >> Lucas De Marchi
On Wed, Apr 06, 2022 at 08:53:56PM +0000, Anusha Srivatsa wrote: > > >> -----Original Message----- >> From: De Marchi, Lucas <lucas.demarchi@intel.com> >> Sent: Wednesday, April 6, 2022 10:46 AM >> To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >> Cc: intel-gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dmc: Add MMIO range restrictions >> >> On Wed, Apr 06, 2022 at 10:16:55AM -0700, Anusha Srivatsa wrote: >> > >> > >> >> -----Original Message----- >> >> From: De Marchi, Lucas <lucas.demarchi@intel.com> >> >> Sent: Tuesday, April 5, 2022 11:03 AM >> >> To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >> >> Cc: intel-gfx@lists.freedesktop.org >> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dmc: Add MMIO range >> >> restrictions >> >> >> >> On Tue, Apr 05, 2022 at 10:14:29AM -0700, Anusha Srivatsa wrote: >> >> >Bspec has added some steps that check for DMC MMIO range before >> >> >programming them. >> >> > >> >> >v2: Fix for CI failure for v1 >> >> > >> >> >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 | 42 >> >> ++++++++++++++++++++++++ >> >> > 1 file changed, 42 insertions(+) >> >> > >> >> >diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c >> >> >b/drivers/gpu/drm/i915/display/intel_dmc.c >> >> >index 257cf662f9f4..05d8e90854ec 100644 >> >> >--- a/drivers/gpu/drm/i915/display/intel_dmc.c >> >> >+++ b/drivers/gpu/drm/i915/display/intel_dmc.c >> >> >@@ -103,6 +103,18 @@ MODULE_FIRMWARE(BXT_DMC_PATH); >> >> > #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 >> >> > >> >> > struct intel_css_header { >> >> > /* 0x09 for DMC */ >> >> >@@ -374,6 +386,30 @@ 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) >> >> >+{ >> >> >+ struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), >> >> dmc); >> >> >+ int i; >> >> >+ >> >> >+ if (IS_DG2(i915) || IS_ALDERLAKE_P(i915)) { >> >> >+ for (i = 0; i < mmio_count; i++) { >> >> >+ if (!((mmioaddr[i] >= TGL_MAIN_MMIO_START && >> >> mmioaddr[i] <= TGL_MAIN_MMIO_END) || >> >> >+ (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_MAIN_MMIO_START && >> >> mmioaddr[i] <= TGL_MAIN_MMIO_END) || >> >> >+ (mmioaddr[i] >= TGL_PIPEA_MMIO_START && >> >> mmioaddr[i] <= TGL_PIPEA_MMIO_END) || >> >> >+ (mmioaddr[i] >= TGL_PIPEB_MMIO_START && >> >> mmioaddr[i] <= TGL_PIPEB_MMIO_END) || >> >> >+ (mmioaddr[i] >= TGL_PIPEC_MMIO_START && >> >> mmioaddr[i] <= TGL_PIPEC_MMIO_END) || >> >> >+ (mmioaddr[i] >= TGL_PIPED_MMIO_START && >> >> mmioaddr[i] <= TGL_PIPEC_MMIO_END))) >> >> >+ return false; >> >> >> >> wonder if we should check for each pipe DMC range independently >> >> rather than just checking all the ranges. >> > Can convert this to a switch case in that scenario. "If it is PIPE A then it must >> be within this range". But it will be 2 switches one for DG2 and ADLP and one >> for TGL and the rest which have individual ranges for every pipe. >> >> I was thinking more about like this: >> >> #define _TGL_PIPEA_MMIO 0x92000 >> #define _TGL_PIPEB_MMIO 0x96000 >> #define TGL_PIPE_MMIO(pipe) _MMIO_PIPE(pipe, _TGL_PIPEA_MMIO, >> _TGL_PIPEB_MMIO) >> #define TGL_PIPE_MMIO_SIZE 0x1000 > >Hmm, does it make sense to add something like: > >#define DMC_MMIO(dmc_id) _MMIO(_PICK(DMC_ID, DMC_FW_MAIN, DMC_FW_PIPEA, DMC_FW_PIPEB, DMC_FW_PIPEC, DMC_FW_PIPED) typo here ----------------------------------^^^^^^ _PICK(dmc_id, DMC_FW_MAIN, DMC_FW_PIPEA, ...) would return 0, 1, .... Why are you converting it to _MMIO? Did you mean to use the address? If the main blob is not handled differently than it could make sense, yes. 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..05d8e90854ec 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc.c +++ b/drivers/gpu/drm/i915/display/intel_dmc.c @@ -103,6 +103,18 @@ MODULE_FIRMWARE(BXT_DMC_PATH); #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 struct intel_css_header { /* 0x09 for DMC */ @@ -374,6 +386,30 @@ 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) +{ + struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), dmc); + int i; + + if (IS_DG2(i915) || IS_ALDERLAKE_P(i915)) { + for (i = 0; i < mmio_count; i++) { + if (!((mmioaddr[i] >= TGL_MAIN_MMIO_START && mmioaddr[i] <= TGL_MAIN_MMIO_END) || + (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_MAIN_MMIO_START && mmioaddr[i] <= TGL_MAIN_MMIO_END) || + (mmioaddr[i] >= TGL_PIPEA_MMIO_START && mmioaddr[i] <= TGL_PIPEA_MMIO_END) || + (mmioaddr[i] >= TGL_PIPEB_MMIO_START && mmioaddr[i] <= TGL_PIPEB_MMIO_END) || + (mmioaddr[i] >= TGL_PIPEC_MMIO_START && mmioaddr[i] <= TGL_PIPEC_MMIO_END) || + (mmioaddr[i] >= TGL_PIPED_MMIO_START && mmioaddr[i] <= TGL_PIPEC_MMIO_END))) + 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 +479,12 @@ static u32 parse_dmc_fw_header(struct intel_dmc *dmc, return 0; } + if (dmc_header->header_ver == 3) { + if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count)) + 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];
Bspec has added some steps that check for DMC MMIO range before programming them. v2: Fix for CI failure for v1 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 | 42 ++++++++++++++++++++++++ 1 file changed, 42 insertions(+)