Re: [PATCH 07/10] drm/i915/dmc: add support to load dmc_header version 3

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx