| Message ID | 20190523082420.10352-7-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] drm/i915/dmc: use kernel types | expand |
>-----Original Message----- >From: De Marchi, Lucas >Sent: Thursday, May 23, 2019 1:24 AM >To: intel-gfx@lists.freedesktop.org >Cc: Jani Nikula <jani.nikula@linux.intel.com>; Srivatsa, Anusha ><anusha.srivatsa@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; De >Marchi, Lucas <lucas.demarchi@intel.com> >Subject: [PATCH 07/10] drm/i915/dmc: add support to load dmc_header version 3 > >Main difference is that now there are up to 20 MMIOs that can be set and a lot of >noise due to the struct changing the fields in the middle. > >Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Checked with spec. The header struct looks good. Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com> >--- > drivers/gpu/drm/i915/i915_drv.h | 4 +- > drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++-------- > 2 files changed, 83 insertions(+), 28 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >index 1ad3818d2676..04a6b59256fd 100644 >--- a/drivers/gpu/drm/i915/i915_drv.h >+++ b/drivers/gpu/drm/i915/i915_drv.h >@@ -354,8 +354,8 @@ struct intel_csr { > u32 dmc_fw_size; /* dwords */ > u32 version; > u32 mmio_count; >- i915_reg_t mmioaddr[8]; >- u32 mmiodata[8]; >+ i915_reg_t mmioaddr[20]; >+ u32 mmiodata[20]; > u32 dc_state; > u32 allowed_dc_mask; > intel_wakeref_t wakeref; >diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c >index 20dd4bd5feae..ad4ee55a8c5e 100644 >--- a/drivers/gpu/drm/i915/intel_csr.c >+++ b/drivers/gpu/drm/i915/intel_csr.c >@@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH); > #define CSR_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 > > struct intel_css_header { > /* 0x09 for DMC */ >@@ -143,7 +145,7 @@ struct intel_package_header { > u32 num_entries; > } __packed; > >-struct intel_dmc_header { >+struct intel_dmc_header_base { > /* always value would be 0x40403E3E */ > u32 signature; > >@@ -164,22 +166,47 @@ struct intel_dmc_header { > > /* Major Minor version */ > u32 fw_version; >+} __packed; >+ >+struct intel_dmc_header_v1 { >+ struct intel_dmc_header_base base; > > /* Number of valid MMIO cycles present. */ > u32 mmio_count; > > /* MMIO address */ >- u32 mmioaddr[8]; >+ u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT]; > > /* MMIO data */ >- u32 mmiodata[8]; >+ u32 mmiodata[DMC_V1_MAX_MMIO_COUNT]; > > /* FW filename */ >- unsigned char dfile[32]; >+ char dfile[32]; > > u32 reserved1[2]; > } __packed; > >+struct intel_dmc_header_v3 { >+ struct intel_dmc_header_base base; >+ >+ /* DMC RAM start MMIO address */ >+ u32 start_mmioaddr; >+ >+ u32 reserved[9]; >+ >+ /* FW filename */ >+ char dfile[32]; >+ >+ /* Number of valid MMIO cycles present. */ >+ u32 mmio_count; >+ >+ /* MMIO address */ >+ u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT]; >+ >+ /* MMIO data */ >+ u32 mmiodata[DMC_V3_MAX_MMIO_COUNT]; >+} __packed; >+ > struct stepping_info { > char stepping; > char substepping; >@@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct >intel_fw_info *fw_info, } > > static u32 parse_csr_fw_dmc(struct intel_csr *csr, >- const struct intel_dmc_header *dmc_header) >+ const struct intel_dmc_header_base *dmc_header) > { >- unsigned int i, payload_size; >- u32 r; >+ unsigned int header_len_bytes, dmc_header_size, payload_size, i; >+ const u32 *mmioaddr, *mmiodata; >+ u32 mmio_count, mmio_count_max; > u8 *payload; > >- if (sizeof(struct intel_dmc_header) != dmc_header->header_len) { >+ BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < >DMC_V3_MAX_MMIO_COUNT || >+ ARRAY_SIZE(csr->mmioaddr) < >DMC_V1_MAX_MMIO_COUNT); >+ >+ /* Cope with small differences between v1 and v3 */ >+ if (dmc_header->header_ver == 3) { >+ const struct intel_dmc_header_v3 *v3 = >+ (const struct intel_dmc_header_v3 *)dmc_header; >+ >+ mmioaddr = v3->mmioaddr; >+ mmiodata = v3->mmiodata; >+ mmio_count = v3->mmio_count; >+ mmio_count_max = DMC_V3_MAX_MMIO_COUNT; >+ /* header_len is in dwords */ >+ header_len_bytes = dmc_header->header_len * 4; >+ dmc_header_size = sizeof(*v3); >+ } else if (dmc_header->header_ver == 1) { >+ const struct intel_dmc_header_v1 *v1 = >+ (const struct intel_dmc_header_v1 *)dmc_header; >+ >+ mmioaddr = v1->mmioaddr; >+ mmiodata = v1->mmiodata; >+ mmio_count = v1->mmio_count; >+ mmio_count_max = DMC_V1_MAX_MMIO_COUNT; >+ header_len_bytes = dmc_header->header_len; >+ dmc_header_size = sizeof(*v1); >+ } else { >+ DRM_ERROR("Unknown DMC fw header version: %u\n", >+ dmc_header->header_ver); >+ return 0; >+ } >+ >+ if (header_len_bytes != dmc_header_size) { > DRM_ERROR("DMC firmware has wrong dmc header length " >- "(%u bytes)\n", >- (dmc_header->header_len)); >+ "(%u bytes)\n", header_len_bytes); > return 0; > } > > /* Cache the dmc header info. */ >- if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) { >- DRM_ERROR("DMC firmware has wrong mmio count %u\n", >- dmc_header->mmio_count); >+ if (mmio_count > mmio_count_max) { >+ DRM_ERROR("DMC firmware has wrong mmio count %u\n", >mmio_count); > return 0; > } > >- csr->mmio_count = dmc_header->mmio_count; >- for (i = 0; i < dmc_header->mmio_count; i++) { >- if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE || >- dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) { >+ for (i = 0; i < mmio_count; i++) { >+ if (mmioaddr[i] < CSR_MMIO_START_RANGE || >+ mmioaddr[i] > CSR_MMIO_END_RANGE) { > DRM_ERROR("DMC firmware has wrong mmio address >0x%x\n", >- dmc_header->mmioaddr[i]); >+ mmioaddr[i]); > return 0; > } >- csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]); >- csr->mmiodata[i] = dmc_header->mmiodata[i]; >+ csr->mmioaddr[i] = _MMIO(mmioaddr[i]); >+ csr->mmiodata[i] = mmiodata[i]; > } >+ csr->mmio_count = mmio_count; > > /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ > payload_size = dmc_header->fw_size * 4; @@ -379,12 +436,10 @@ >static u32 parse_csr_fw_dmc(struct intel_csr *csr, > return 0; > } > >- r = sizeof(struct intel_dmc_header); >- payload = (u8 *)(dmc_header) + r; >+ payload = (u8 *)(dmc_header) + header_len_bytes; > memcpy(csr->dmc_payload, payload, payload_size); >- r += payload_size; > >- return r; >+ return header_len_bytes + payload_size; > } > > static u32 >@@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private >*dev_priv, { > struct intel_css_header *css_header; > struct intel_package_header *package_header; >- struct intel_dmc_header *dmc_header; >+ struct intel_dmc_header_base *dmc_header; > struct intel_csr *csr = &dev_priv->csr; > const struct stepping_info *si = intel_get_stepping_info(dev_priv); > u32 readcount = 0; >@@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private >*dev_priv, > readcount += r; > > /* Extract dmc_header information. */ >- dmc_header = (struct intel_dmc_header *)&fw->data[readcount]; >+ dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount]; > r = parse_csr_fw_dmc(csr, dmc_header); > if (!r) > return NULL; >-- >2.21.0
On Thu, May 23, 2019 at 01:24:17AM -0700, Lucas De Marchi wrote: > Main difference is that now there are up to 20 MMIOs that can be set and > a lot of noise due to the struct changing the fields in the middle. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 +- > drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++-------- > 2 files changed, 83 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1ad3818d2676..04a6b59256fd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -354,8 +354,8 @@ struct intel_csr { > u32 dmc_fw_size; /* dwords */ > u32 version; > u32 mmio_count; > - i915_reg_t mmioaddr[8]; > - u32 mmiodata[8]; > + i915_reg_t mmioaddr[20]; > + u32 mmiodata[20]; > u32 dc_state; > u32 allowed_dc_mask; > intel_wakeref_t wakeref; > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 20dd4bd5feae..ad4ee55a8c5e 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH); > #define CSR_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 > > struct intel_css_header { > /* 0x09 for DMC */ > @@ -143,7 +145,7 @@ struct intel_package_header { > u32 num_entries; > } __packed; > > -struct intel_dmc_header { > +struct intel_dmc_header_base { > /* always value would be 0x40403E3E */ > u32 signature; > > @@ -164,22 +166,47 @@ struct intel_dmc_header { > > /* Major Minor version */ > u32 fw_version; > +} __packed; > + > +struct intel_dmc_header_v1 { > + struct intel_dmc_header_base base; > > /* Number of valid MMIO cycles present. */ > u32 mmio_count; > > /* MMIO address */ > - u32 mmioaddr[8]; > + u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT]; > > /* MMIO data */ > - u32 mmiodata[8]; > + u32 mmiodata[DMC_V1_MAX_MMIO_COUNT]; > > /* FW filename */ > - unsigned char dfile[32]; > + char dfile[32]; > > u32 reserved1[2]; > } __packed; > > +struct intel_dmc_header_v3 { > + struct intel_dmc_header_base base; > + > + /* DMC RAM start MMIO address */ > + u32 start_mmioaddr; > + > + u32 reserved[9]; > + > + /* FW filename */ > + char dfile[32]; > + > + /* Number of valid MMIO cycles present. */ > + u32 mmio_count; > + > + /* MMIO address */ > + u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT]; > + > + /* MMIO data */ > + u32 mmiodata[DMC_V3_MAX_MMIO_COUNT]; > +} __packed; > + > struct stepping_info { > char stepping; > char substepping; > @@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, > } > > static u32 parse_csr_fw_dmc(struct intel_csr *csr, > - const struct intel_dmc_header *dmc_header) > + const struct intel_dmc_header_base *dmc_header) > { > - unsigned int i, payload_size; > - u32 r; > + unsigned int header_len_bytes, dmc_header_size, payload_size, i; > + const u32 *mmioaddr, *mmiodata; > + u32 mmio_count, mmio_count_max; > u8 *payload; > > - if (sizeof(struct intel_dmc_header) != dmc_header->header_len) { > + BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < DMC_V3_MAX_MMIO_COUNT || > + ARRAY_SIZE(csr->mmioaddr) < DMC_V1_MAX_MMIO_COUNT); > + > + /* Cope with small differences between v1 and v3 */ > + if (dmc_header->header_ver == 3) { I'm missing on this patch something like: - /* 0x01, 0x02 */ + /* 0x01, 0x02, 0x03 */ near header_ver definition or maybe kill that comment at all... The rest seems right so feel free to use Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > + const struct intel_dmc_header_v3 *v3 = > + (const struct intel_dmc_header_v3 *)dmc_header; > + > + mmioaddr = v3->mmioaddr; > + mmiodata = v3->mmiodata; > + mmio_count = v3->mmio_count; > + mmio_count_max = DMC_V3_MAX_MMIO_COUNT; > + /* header_len is in dwords */ > + header_len_bytes = dmc_header->header_len * 4; > + dmc_header_size = sizeof(*v3); > + } else if (dmc_header->header_ver == 1) { > + const struct intel_dmc_header_v1 *v1 = > + (const struct intel_dmc_header_v1 *)dmc_header; > + > + mmioaddr = v1->mmioaddr; > + mmiodata = v1->mmiodata; > + mmio_count = v1->mmio_count; > + mmio_count_max = DMC_V1_MAX_MMIO_COUNT; > + header_len_bytes = dmc_header->header_len; > + dmc_header_size = sizeof(*v1); > + } else { > + DRM_ERROR("Unknown DMC fw header version: %u\n", > + dmc_header->header_ver); > + return 0; > + } > + > + if (header_len_bytes != dmc_header_size) { > DRM_ERROR("DMC firmware has wrong dmc header length " > - "(%u bytes)\n", > - (dmc_header->header_len)); > + "(%u bytes)\n", header_len_bytes); > return 0; > } > > /* Cache the dmc header info. */ > - if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) { > - DRM_ERROR("DMC firmware has wrong mmio count %u\n", > - dmc_header->mmio_count); > + if (mmio_count > mmio_count_max) { > + DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count); > return 0; > } > > - csr->mmio_count = dmc_header->mmio_count; > - for (i = 0; i < dmc_header->mmio_count; i++) { > - if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE || > - dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) { > + for (i = 0; i < mmio_count; i++) { > + if (mmioaddr[i] < CSR_MMIO_START_RANGE || > + mmioaddr[i] > CSR_MMIO_END_RANGE) { > DRM_ERROR("DMC firmware has wrong mmio address 0x%x\n", > - dmc_header->mmioaddr[i]); > + mmioaddr[i]); > return 0; > } > - csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]); > - csr->mmiodata[i] = dmc_header->mmiodata[i]; > + csr->mmioaddr[i] = _MMIO(mmioaddr[i]); > + csr->mmiodata[i] = mmiodata[i]; > } > + csr->mmio_count = mmio_count; > > /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ > payload_size = dmc_header->fw_size * 4; > @@ -379,12 +436,10 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, > return 0; > } > > - r = sizeof(struct intel_dmc_header); > - payload = (u8 *)(dmc_header) + r; > + payload = (u8 *)(dmc_header) + header_len_bytes; > memcpy(csr->dmc_payload, payload, payload_size); > - r += payload_size; > > - return r; > + return header_len_bytes + payload_size; > } > > static u32 > @@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > { > struct intel_css_header *css_header; > struct intel_package_header *package_header; > - struct intel_dmc_header *dmc_header; > + struct intel_dmc_header_base *dmc_header; > struct intel_csr *csr = &dev_priv->csr; > const struct stepping_info *si = intel_get_stepping_info(dev_priv); > u32 readcount = 0; > @@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > readcount += r; > > /* Extract dmc_header information. */ > - dmc_header = (struct intel_dmc_header *)&fw->data[readcount]; > + dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount]; > r = parse_csr_fw_dmc(csr, dmc_header); > if (!r) > return NULL; > -- > 2.21.0 >
On Thu, May 23, 2019 at 11:57:19AM -0700, Rodrigo Vivi wrote: >On Thu, May 23, 2019 at 01:24:17AM -0700, Lucas De Marchi wrote: >> Main difference is that now there are up to 20 MMIOs that can be set and >> a lot of noise due to the struct changing the fields in the middle. >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 4 +- >> drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++-------- >> 2 files changed, 83 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 1ad3818d2676..04a6b59256fd 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -354,8 +354,8 @@ struct intel_csr { >> u32 dmc_fw_size; /* dwords */ >> u32 version; >> u32 mmio_count; >> - i915_reg_t mmioaddr[8]; >> - u32 mmiodata[8]; >> + i915_reg_t mmioaddr[20]; >> + u32 mmiodata[20]; >> u32 dc_state; >> u32 allowed_dc_mask; >> intel_wakeref_t wakeref; >> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c >> index 20dd4bd5feae..ad4ee55a8c5e 100644 >> --- a/drivers/gpu/drm/i915/intel_csr.c >> +++ b/drivers/gpu/drm/i915/intel_csr.c >> @@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH); >> #define CSR_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 >> >> struct intel_css_header { >> /* 0x09 for DMC */ >> @@ -143,7 +145,7 @@ struct intel_package_header { >> u32 num_entries; >> } __packed; >> >> -struct intel_dmc_header { >> +struct intel_dmc_header_base { >> /* always value would be 0x40403E3E */ >> u32 signature; >> >> @@ -164,22 +166,47 @@ struct intel_dmc_header { >> >> /* Major Minor version */ >> u32 fw_version; >> +} __packed; >> + >> +struct intel_dmc_header_v1 { >> + struct intel_dmc_header_base base; >> >> /* Number of valid MMIO cycles present. */ >> u32 mmio_count; >> >> /* MMIO address */ >> - u32 mmioaddr[8]; >> + u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT]; >> >> /* MMIO data */ >> - u32 mmiodata[8]; >> + u32 mmiodata[DMC_V1_MAX_MMIO_COUNT]; >> >> /* FW filename */ >> - unsigned char dfile[32]; >> + char dfile[32]; >> >> u32 reserved1[2]; >> } __packed; >> >> +struct intel_dmc_header_v3 { >> + struct intel_dmc_header_base base; >> + >> + /* DMC RAM start MMIO address */ >> + u32 start_mmioaddr; >> + >> + u32 reserved[9]; >> + >> + /* FW filename */ >> + char dfile[32]; >> + >> + /* Number of valid MMIO cycles present. */ >> + u32 mmio_count; >> + >> + /* MMIO address */ >> + u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT]; >> + >> + /* MMIO data */ >> + u32 mmiodata[DMC_V3_MAX_MMIO_COUNT]; >> +} __packed; >> + >> struct stepping_info { >> char stepping; >> char substepping; >> @@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, >> } >> >> static u32 parse_csr_fw_dmc(struct intel_csr *csr, >> - const struct intel_dmc_header *dmc_header) >> + const struct intel_dmc_header_base *dmc_header) >> { >> - unsigned int i, payload_size; >> - u32 r; >> + unsigned int header_len_bytes, dmc_header_size, payload_size, i; >> + const u32 *mmioaddr, *mmiodata; >> + u32 mmio_count, mmio_count_max; >> u8 *payload; >> >> - if (sizeof(struct intel_dmc_header) != dmc_header->header_len) { >> + BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < DMC_V3_MAX_MMIO_COUNT || >> + ARRAY_SIZE(csr->mmioaddr) < DMC_V1_MAX_MMIO_COUNT); >> + >> + /* Cope with small differences between v1 and v3 */ >> + if (dmc_header->header_ver == 3) { > >I'm missing on this patch something like: > >- /* 0x01, 0x02 */ >+ /* 0x01, 0x02, 0x03 */ >near header_ver definition > >or maybe kill that comment at all... nah, that is another version and it's a bit confusing. But we have *package* header version: 1 or 2 *dmc* header version: 1 or 3 Overall structure of the firmware: .------------. | CSS Header | |------------| | Package | | Header | |------------| | 20 or 32 | * 20 for package header v1 | fw_info |--.-.-. 32 for package header v2 |------------|<-' | | | DMC header | | | | DMC payload| | | |------------|<---' | | DMC header | | | DMC payload| | |------------| . ... . |------------|<-----' | DMC header | | DMC payload| '------------' here the dmc_header v3 will dictate how the *DMC header* will be parsed, that can have either 8 or 20 MMIOs to be programmed, besides the firmware to be loaded. I guess I could add a patch on top with this sketch as a comment to better explain how is the layout of the file. Lucas De Marchi > >The rest seems right so feel free to use >Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> + const struct intel_dmc_header_v3 *v3 = >> + (const struct intel_dmc_header_v3 *)dmc_header; >> + >> + mmioaddr = v3->mmioaddr; >> + mmiodata = v3->mmiodata; >> + mmio_count = v3->mmio_count; >> + mmio_count_max = DMC_V3_MAX_MMIO_COUNT; >> + /* header_len is in dwords */ >> + header_len_bytes = dmc_header->header_len * 4; >> + dmc_header_size = sizeof(*v3); >> + } else if (dmc_header->header_ver == 1) { >> + const struct intel_dmc_header_v1 *v1 = >> + (const struct intel_dmc_header_v1 *)dmc_header; >> + >> + mmioaddr = v1->mmioaddr; >> + mmiodata = v1->mmiodata; >> + mmio_count = v1->mmio_count; >> + mmio_count_max = DMC_V1_MAX_MMIO_COUNT; >> + header_len_bytes = dmc_header->header_len; >> + dmc_header_size = sizeof(*v1); >> + } else { >> + DRM_ERROR("Unknown DMC fw header version: %u\n", >> + dmc_header->header_ver); >> + return 0; >> + } >> + >> + if (header_len_bytes != dmc_header_size) { >> DRM_ERROR("DMC firmware has wrong dmc header length " >> - "(%u bytes)\n", >> - (dmc_header->header_len)); >> + "(%u bytes)\n", header_len_bytes); >> return 0; >> } >> >> /* Cache the dmc header info. */ >> - if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) { >> - DRM_ERROR("DMC firmware has wrong mmio count %u\n", >> - dmc_header->mmio_count); >> + if (mmio_count > mmio_count_max) { >> + DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count); >> return 0; >> } >> >> - csr->mmio_count = dmc_header->mmio_count; >> - for (i = 0; i < dmc_header->mmio_count; i++) { >> - if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE || >> - dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) { >> + for (i = 0; i < mmio_count; i++) { >> + if (mmioaddr[i] < CSR_MMIO_START_RANGE || >> + mmioaddr[i] > CSR_MMIO_END_RANGE) { >> DRM_ERROR("DMC firmware has wrong mmio address 0x%x\n", >> - dmc_header->mmioaddr[i]); >> + mmioaddr[i]); >> return 0; >> } >> - csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]); >> - csr->mmiodata[i] = dmc_header->mmiodata[i]; >> + csr->mmioaddr[i] = _MMIO(mmioaddr[i]); >> + csr->mmiodata[i] = mmiodata[i]; >> } >> + csr->mmio_count = mmio_count; >> >> /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ >> payload_size = dmc_header->fw_size * 4; >> @@ -379,12 +436,10 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, >> return 0; >> } >> >> - r = sizeof(struct intel_dmc_header); >> - payload = (u8 *)(dmc_header) + r; >> + payload = (u8 *)(dmc_header) + header_len_bytes; >> memcpy(csr->dmc_payload, payload, payload_size); >> - r += payload_size; >> >> - return r; >> + return header_len_bytes + payload_size; >> } >> >> static u32 >> @@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, >> { >> struct intel_css_header *css_header; >> struct intel_package_header *package_header; >> - struct intel_dmc_header *dmc_header; >> + struct intel_dmc_header_base *dmc_header; >> struct intel_csr *csr = &dev_priv->csr; >> const struct stepping_info *si = intel_get_stepping_info(dev_priv); >> u32 readcount = 0; >> @@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, >> readcount += r; >> >> /* Extract dmc_header information. */ >> - dmc_header = (struct intel_dmc_header *)&fw->data[readcount]; >> + dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount]; >> r = parse_csr_fw_dmc(csr, dmc_header); >> if (!r) >> return NULL; >> -- >> 2.21.0 >>
On Thu, May 23, 2019 at 12:25:34PM -0700, Lucas De Marchi wrote: > On Thu, May 23, 2019 at 11:57:19AM -0700, Rodrigo Vivi wrote: > > On Thu, May 23, 2019 at 01:24:17AM -0700, Lucas De Marchi wrote: > > > Main difference is that now there are up to 20 MMIOs that can be set and > > > a lot of noise due to the struct changing the fields in the middle. > > > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 4 +- > > > drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++-------- > > > 2 files changed, 83 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index 1ad3818d2676..04a6b59256fd 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -354,8 +354,8 @@ struct intel_csr { > > > u32 dmc_fw_size; /* dwords */ > > > u32 version; > > > u32 mmio_count; > > > - i915_reg_t mmioaddr[8]; > > > - u32 mmiodata[8]; > > > + i915_reg_t mmioaddr[20]; > > > + u32 mmiodata[20]; > > > u32 dc_state; > > > u32 allowed_dc_mask; > > > intel_wakeref_t wakeref; > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > > > index 20dd4bd5feae..ad4ee55a8c5e 100644 > > > --- a/drivers/gpu/drm/i915/intel_csr.c > > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > > @@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH); > > > #define CSR_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 > > > > > > struct intel_css_header { > > > /* 0x09 for DMC */ > > > @@ -143,7 +145,7 @@ struct intel_package_header { > > > u32 num_entries; > > > } __packed; > > > > > > -struct intel_dmc_header { > > > +struct intel_dmc_header_base { > > > /* always value would be 0x40403E3E */ > > > u32 signature; > > > > > > @@ -164,22 +166,47 @@ struct intel_dmc_header { > > > > > > /* Major Minor version */ > > > u32 fw_version; > > > +} __packed; > > > + > > > +struct intel_dmc_header_v1 { > > > + struct intel_dmc_header_base base; > > > > > > /* Number of valid MMIO cycles present. */ > > > u32 mmio_count; > > > > > > /* MMIO address */ > > > - u32 mmioaddr[8]; > > > + u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT]; > > > > > > /* MMIO data */ > > > - u32 mmiodata[8]; > > > + u32 mmiodata[DMC_V1_MAX_MMIO_COUNT]; > > > > > > /* FW filename */ > > > - unsigned char dfile[32]; > > > + char dfile[32]; > > > > > > u32 reserved1[2]; > > > } __packed; > > > > > > +struct intel_dmc_header_v3 { > > > + struct intel_dmc_header_base base; > > > + > > > + /* DMC RAM start MMIO address */ > > > + u32 start_mmioaddr; > > > + > > > + u32 reserved[9]; > > > + > > > + /* FW filename */ > > > + char dfile[32]; > > > + > > > + /* Number of valid MMIO cycles present. */ > > > + u32 mmio_count; > > > + > > > + /* MMIO address */ > > > + u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT]; > > > + > > > + /* MMIO data */ > > > + u32 mmiodata[DMC_V3_MAX_MMIO_COUNT]; > > > +} __packed; > > > + > > > struct stepping_info { > > > char stepping; > > > char substepping; > > > @@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, > > > } > > > > > > static u32 parse_csr_fw_dmc(struct intel_csr *csr, > > > - const struct intel_dmc_header *dmc_header) > > > + const struct intel_dmc_header_base *dmc_header) > > > { > > > - unsigned int i, payload_size; > > > - u32 r; > > > + unsigned int header_len_bytes, dmc_header_size, payload_size, i; > > > + const u32 *mmioaddr, *mmiodata; > > > + u32 mmio_count, mmio_count_max; > > > u8 *payload; > > > > > > - if (sizeof(struct intel_dmc_header) != dmc_header->header_len) { > > > + BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < DMC_V3_MAX_MMIO_COUNT || > > > + ARRAY_SIZE(csr->mmioaddr) < DMC_V1_MAX_MMIO_COUNT); > > > + > > > + /* Cope with small differences between v1 and v3 */ > > > + if (dmc_header->header_ver == 3) { > > > > I'm missing on this patch something like: > > > > - /* 0x01, 0x02 */ > > + /* 0x01, 0x02, 0x03 */ > > near header_ver definition > > > > or maybe kill that comment at all... > > nah, that is another version and it's a bit confusing. But we have > *package* header version: 1 or 2 > *dmc* header version: 1 or 3 > > Overall structure of the firmware: > > .------------. > | CSS Header | > |------------| > | Package | > | Header | > |------------| > | 20 or 32 | * 20 for package header v1 > | fw_info |--.-.-. 32 for package header v2 > |------------|<-' | | > | DMC header | | | > | DMC payload| | | > |------------|<---' | > | DMC header | | > | DMC payload| | > |------------| . > ... . > |------------|<-----' > | DMC header | > | DMC payload| > '------------' > > here the dmc_header v3 will dictate how the *DMC header* > will be parsed, that can have either 8 or 20 MMIOs to be programmed, > besides the firmware to be loaded. Ahh! I got it now. Thanks for detailing it out. > > I guess I could add a patch on top with this sketch as a comment to > better explain how is the layout of the file. That is a good idea! :) Thanks, Rodrigo. > > > Lucas De Marchi > > > > > The rest seems right so feel free to use > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > + const struct intel_dmc_header_v3 *v3 = > > > + (const struct intel_dmc_header_v3 *)dmc_header; > > > + > > > + mmioaddr = v3->mmioaddr; > > > + mmiodata = v3->mmiodata; > > > + mmio_count = v3->mmio_count; > > > + mmio_count_max = DMC_V3_MAX_MMIO_COUNT; > > > + /* header_len is in dwords */ > > > + header_len_bytes = dmc_header->header_len * 4; > > > + dmc_header_size = sizeof(*v3); > > > + } else if (dmc_header->header_ver == 1) { > > > + const struct intel_dmc_header_v1 *v1 = > > > + (const struct intel_dmc_header_v1 *)dmc_header; > > > + > > > + mmioaddr = v1->mmioaddr; > > > + mmiodata = v1->mmiodata; > > > + mmio_count = v1->mmio_count; > > > + mmio_count_max = DMC_V1_MAX_MMIO_COUNT; > > > + header_len_bytes = dmc_header->header_len; > > > + dmc_header_size = sizeof(*v1); > > > + } else { > > > + DRM_ERROR("Unknown DMC fw header version: %u\n", > > > + dmc_header->header_ver); > > > + return 0; > > > + } > > > + > > > + if (header_len_bytes != dmc_header_size) { > > > DRM_ERROR("DMC firmware has wrong dmc header length " > > > - "(%u bytes)\n", > > > - (dmc_header->header_len)); > > > + "(%u bytes)\n", header_len_bytes); > > > return 0; > > > } > > > > > > /* Cache the dmc header info. */ > > > - if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) { > > > - DRM_ERROR("DMC firmware has wrong mmio count %u\n", > > > - dmc_header->mmio_count); > > > + if (mmio_count > mmio_count_max) { > > > + DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count); > > > return 0; > > > } > > > > > > - csr->mmio_count = dmc_header->mmio_count; > > > - for (i = 0; i < dmc_header->mmio_count; i++) { > > > - if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE || > > > - dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) { > > > + for (i = 0; i < mmio_count; i++) { > > > + if (mmioaddr[i] < CSR_MMIO_START_RANGE || > > > + mmioaddr[i] > CSR_MMIO_END_RANGE) { > > > DRM_ERROR("DMC firmware has wrong mmio address 0x%x\n", > > > - dmc_header->mmioaddr[i]); > > > + mmioaddr[i]); > > > return 0; > > > } > > > - csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]); > > > - csr->mmiodata[i] = dmc_header->mmiodata[i]; > > > + csr->mmioaddr[i] = _MMIO(mmioaddr[i]); > > > + csr->mmiodata[i] = mmiodata[i]; > > > } > > > + csr->mmio_count = mmio_count; > > > > > > /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ > > > payload_size = dmc_header->fw_size * 4; > > > @@ -379,12 +436,10 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, > > > return 0; > > > } > > > > > > - r = sizeof(struct intel_dmc_header); > > > - payload = (u8 *)(dmc_header) + r; > > > + payload = (u8 *)(dmc_header) + header_len_bytes; > > > memcpy(csr->dmc_payload, payload, payload_size); > > > - r += payload_size; > > > > > > - return r; > > > + return header_len_bytes + payload_size; > > > } > > > > > > static u32 > > > @@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > > > { > > > struct intel_css_header *css_header; > > > struct intel_package_header *package_header; > > > - struct intel_dmc_header *dmc_header; > > > + struct intel_dmc_header_base *dmc_header; > > > struct intel_csr *csr = &dev_priv->csr; > > > const struct stepping_info *si = intel_get_stepping_info(dev_priv); > > > u32 readcount = 0; > > > @@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > > > readcount += r; > > > > > > /* Extract dmc_header information. */ > > > - dmc_header = (struct intel_dmc_header *)&fw->data[readcount]; > > > + dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount]; > > > r = parse_csr_fw_dmc(csr, dmc_header); > > > if (!r) > > > return NULL; > > > -- > > > 2.21.0 > > >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1ad3818d2676..04a6b59256fd 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -354,8 +354,8 @@ struct intel_csr { u32 dmc_fw_size; /* dwords */ u32 version; u32 mmio_count; - i915_reg_t mmioaddr[8]; - u32 mmiodata[8]; + i915_reg_t mmioaddr[20]; + u32 mmiodata[20]; u32 dc_state; u32 allowed_dc_mask; intel_wakeref_t wakeref; diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 20dd4bd5feae..ad4ee55a8c5e 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -72,6 +72,8 @@ MODULE_FIRMWARE(BXT_CSR_PATH); #define CSR_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 struct intel_css_header { /* 0x09 for DMC */ @@ -143,7 +145,7 @@ struct intel_package_header { u32 num_entries; } __packed; -struct intel_dmc_header { +struct intel_dmc_header_base { /* always value would be 0x40403E3E */ u32 signature; @@ -164,22 +166,47 @@ struct intel_dmc_header { /* Major Minor version */ u32 fw_version; +} __packed; + +struct intel_dmc_header_v1 { + struct intel_dmc_header_base base; /* Number of valid MMIO cycles present. */ u32 mmio_count; /* MMIO address */ - u32 mmioaddr[8]; + u32 mmioaddr[DMC_V1_MAX_MMIO_COUNT]; /* MMIO data */ - u32 mmiodata[8]; + u32 mmiodata[DMC_V1_MAX_MMIO_COUNT]; /* FW filename */ - unsigned char dfile[32]; + char dfile[32]; u32 reserved1[2]; } __packed; +struct intel_dmc_header_v3 { + struct intel_dmc_header_base base; + + /* DMC RAM start MMIO address */ + u32 start_mmioaddr; + + u32 reserved[9]; + + /* FW filename */ + char dfile[32]; + + /* Number of valid MMIO cycles present. */ + u32 mmio_count; + + /* MMIO address */ + u32 mmioaddr[DMC_V3_MAX_MMIO_COUNT]; + + /* MMIO data */ + u32 mmiodata[DMC_V3_MAX_MMIO_COUNT]; +} __packed; + struct stepping_info { char stepping; char substepping; @@ -333,37 +360,67 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, } static u32 parse_csr_fw_dmc(struct intel_csr *csr, - const struct intel_dmc_header *dmc_header) + const struct intel_dmc_header_base *dmc_header) { - unsigned int i, payload_size; - u32 r; + unsigned int header_len_bytes, dmc_header_size, payload_size, i; + const u32 *mmioaddr, *mmiodata; + u32 mmio_count, mmio_count_max; u8 *payload; - if (sizeof(struct intel_dmc_header) != dmc_header->header_len) { + BUILD_BUG_ON(ARRAY_SIZE(csr->mmioaddr) < DMC_V3_MAX_MMIO_COUNT || + ARRAY_SIZE(csr->mmioaddr) < DMC_V1_MAX_MMIO_COUNT); + + /* Cope with small differences between v1 and v3 */ + if (dmc_header->header_ver == 3) { + const struct intel_dmc_header_v3 *v3 = + (const struct intel_dmc_header_v3 *)dmc_header; + + mmioaddr = v3->mmioaddr; + mmiodata = v3->mmiodata; + mmio_count = v3->mmio_count; + mmio_count_max = DMC_V3_MAX_MMIO_COUNT; + /* header_len is in dwords */ + header_len_bytes = dmc_header->header_len * 4; + dmc_header_size = sizeof(*v3); + } else if (dmc_header->header_ver == 1) { + const struct intel_dmc_header_v1 *v1 = + (const struct intel_dmc_header_v1 *)dmc_header; + + mmioaddr = v1->mmioaddr; + mmiodata = v1->mmiodata; + mmio_count = v1->mmio_count; + mmio_count_max = DMC_V1_MAX_MMIO_COUNT; + header_len_bytes = dmc_header->header_len; + dmc_header_size = sizeof(*v1); + } else { + DRM_ERROR("Unknown DMC fw header version: %u\n", + dmc_header->header_ver); + return 0; + } + + if (header_len_bytes != dmc_header_size) { DRM_ERROR("DMC firmware has wrong dmc header length " - "(%u bytes)\n", - (dmc_header->header_len)); + "(%u bytes)\n", header_len_bytes); return 0; } /* Cache the dmc header info. */ - if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) { - DRM_ERROR("DMC firmware has wrong mmio count %u\n", - dmc_header->mmio_count); + if (mmio_count > mmio_count_max) { + DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count); return 0; } - csr->mmio_count = dmc_header->mmio_count; - for (i = 0; i < dmc_header->mmio_count; i++) { - if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE || - dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) { + for (i = 0; i < mmio_count; i++) { + if (mmioaddr[i] < CSR_MMIO_START_RANGE || + mmioaddr[i] > CSR_MMIO_END_RANGE) { DRM_ERROR("DMC firmware has wrong mmio address 0x%x\n", - dmc_header->mmioaddr[i]); + mmioaddr[i]); return 0; } - csr->mmioaddr[i] = _MMIO(dmc_header->mmioaddr[i]); - csr->mmiodata[i] = dmc_header->mmiodata[i]; + csr->mmioaddr[i] = _MMIO(mmioaddr[i]); + csr->mmiodata[i] = mmiodata[i]; } + csr->mmio_count = mmio_count; /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ payload_size = dmc_header->fw_size * 4; @@ -379,12 +436,10 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, return 0; } - r = sizeof(struct intel_dmc_header); - payload = (u8 *)(dmc_header) + r; + payload = (u8 *)(dmc_header) + header_len_bytes; memcpy(csr->dmc_payload, payload, payload_size); - r += payload_size; - return r; + return header_len_bytes + payload_size; } static u32 @@ -470,7 +525,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, { struct intel_css_header *css_header; struct intel_package_header *package_header; - struct intel_dmc_header *dmc_header; + struct intel_dmc_header_base *dmc_header; struct intel_csr *csr = &dev_priv->csr; const struct stepping_info *si = intel_get_stepping_info(dev_priv); u32 readcount = 0; @@ -496,7 +551,7 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, readcount += r; /* Extract dmc_header information. */ - dmc_header = (struct intel_dmc_header *)&fw->data[readcount]; + dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount]; r = parse_csr_fw_dmc(csr, dmc_header); if (!r) return NULL;
Main difference is that now there are up to 20 MMIOs that can be set and a lot of noise due to the struct changing the fields in the middle. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 4 +- drivers/gpu/drm/i915/intel_csr.c | 107 +++++++++++++++++++++++-------- 2 files changed, 83 insertions(+), 28 deletions(-)