* [PATCH 0/6] Several GuC related patches
@ 2015-09-10 23:56 yu.dai
2015-09-10 23:56 ` [PATCH 1/6] drm/i915/guc: Fix a bug in GuC status check yu.dai
` (6 more replies)
0 siblings, 7 replies; 35+ messages in thread
From: yu.dai @ 2015-09-10 23:56 UTC (permalink / raw)
To: intel-gfx
From: Alex Dai <yu.dai@intel.com>
The first two had been submitted to upstream. They are collected here as a
summry of latest GuC changes.
drm/i915/guc: Fix a bug in GuC status check
drm/i915/guc: Add GuC css header parser
The others are fixes for RC6 or suspend/resume problems when GuC submission is
enabled. And last, enable GuC command submission.
Alex Dai (4):
drm/i915/guc: Fix a bug in GuC status check
drm/i915/guc: Add GuC css header parser
drm/i915/guc: Add host2guc notification for suspend and resume
drm/i915/guc: Media domain bit needed when notify GuC rc6 state
Sagar Arun Kamble (1):
drm/i915/guc: Don't send flips to GuC
Dave Gordon (1):
drm/i915/guc: Enable GuC submission, where supported
Documentation/DocBook/drm.tmpl | 24 +++++-
drivers/gpu/drm/i915/i915_drv.c | 1 +
drivers/gpu/drm/i915/i915_gem.c | 1 +
drivers/gpu/drm/i915/i915_guc_reg.h | 1 +
drivers/gpu/drm/i915/i915_guc_submission.c | 53 +++++++++++-
drivers/gpu/drm/i915/i915_params.c | 4 +-
drivers/gpu/drm/i915/intel_guc.h | 4 +-
drivers/gpu/drm/i915/intel_guc_fwif.h | 44 ++++++++++
drivers/gpu/drm/i915/intel_guc_loader.c | 124 +++++++++++++++++++----------
9 files changed, 208 insertions(+), 48 deletions(-)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/6] drm/i915/guc: Fix a bug in GuC status check 2015-09-10 23:56 [PATCH 0/6] Several GuC related patches yu.dai @ 2015-09-10 23:56 ` yu.dai 2015-09-10 23:56 ` [PATCH 2/6] drm/i915/guc: Add GuC css header parser yu.dai ` (5 subsequent siblings) 6 siblings, 0 replies; 35+ messages in thread From: yu.dai @ 2015-09-10 23:56 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> Bit 16 of GuC status indicates resuming from RC6. The LAPIC_DONE status is a reliable readiness flag only when resuming from RC6. This fix a racing issue that allocation of doorbell fails whilst GuC init is not finished. Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_guc_reg.h | 1 + drivers/gpu/drm/i915/intel_guc_loader.c | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h index 8c8e574..dd0e1e8 100644 --- a/drivers/gpu/drm/i915/i915_guc_reg.h +++ b/drivers/gpu/drm/i915/i915_guc_reg.h @@ -37,6 +37,7 @@ #define GS_UKERNEL_READY (0xF0 << GS_UKERNEL_SHIFT) #define GS_MIA_SHIFT 16 #define GS_MIA_MASK (0x07 << GS_MIA_SHIFT) +#define GS_MIA_CORE_STATE (1 << GS_MIA_SHIFT) #define SOFT_SCRATCH(n) (0xc180 + ((n) * 4)) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 5eafd31..5823615 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -209,9 +209,10 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, u32 *status) { u32 val = I915_READ(GUC_STATUS); + u32 uk_val = val & GS_UKERNEL_MASK; *status = val; - return ((val & GS_UKERNEL_MASK) == GS_UKERNEL_READY || - (val & GS_UKERNEL_MASK) == GS_UKERNEL_LAPIC_DONE); + return (uk_val == GS_UKERNEL_READY || + ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); } /* -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 2/6] drm/i915/guc: Add GuC css header parser 2015-09-10 23:56 [PATCH 0/6] Several GuC related patches yu.dai 2015-09-10 23:56 ` [PATCH 1/6] drm/i915/guc: Fix a bug in GuC status check yu.dai @ 2015-09-10 23:56 ` yu.dai 2015-09-14 9:28 ` Daniel Vetter 2015-09-10 23:56 ` [PATCH 3/6] drm/i915/guc: Add host2guc notification for suspend and resume yu.dai ` (4 subsequent siblings) 6 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-10 23:56 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> By using information from GuC css header, we can eliminate some hard code w.r.t size of some components of firmware. v1: 1) guc_css_header is defined as __packed now 2) Add and correct GuC related topics in kernel/Doc Signed-off-by: Alex Dai <yu.dai@intel.com> --- Documentation/DocBook/drm.tmpl | 24 +++++++- drivers/gpu/drm/i915/intel_guc.h | 2 +- drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++ drivers/gpu/drm/i915/intel_guc_loader.c | 105 +++++++++++++++++++++----------- 4 files changed, 130 insertions(+), 37 deletions(-) diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 66bc646..f430da6 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -4155,14 +4155,34 @@ int num_ioctls;</synopsis> <title>GuC-based Command Submission</title> <sect2> <title>GuC</title> -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC !Idrivers/gpu/drm/i915/intel_guc_loader.c </sect2> <sect2> <title>GuC Client</title> -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command submissison +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client !Idrivers/gpu/drm/i915/i915_guc_submission.c </sect2> + <sect2> + <title>GuC Firmware Layout</title> + <para>The GuC firmware layout looks like this:</para> +<programlisting><![CDATA[ + +-------------------------------+ + | guc_css_header | + | contains major/minor version | + +-------------------------------+ + | uCode | + +-------------------------------+ + | RSA signature | + +-------------------------------+ + | modulus key | + +-------------------------------+ + | exponent val | + +-------------------------------+ +]]></programlisting> +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout +!Idrivers/gpu/drm/i915/intel_guc_loader.c + </sect2> </sect1> <sect1> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 4ec2d27..e1389fc 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -71,6 +71,7 @@ struct intel_guc_fw { struct drm_i915_gem_object * guc_fw_obj; enum intel_guc_fw_status guc_fw_fetch_status; enum intel_guc_fw_status guc_fw_load_status; + struct guc_css_header guc_fw_header; uint16_t guc_fw_major_wanted; uint16_t guc_fw_minor_wanted; @@ -80,7 +81,6 @@ struct intel_guc_fw { struct intel_guc { struct intel_guc_fw guc_fw; - uint32_t log_flags; struct drm_i915_gem_object *log_obj; diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index e1f47ba..006dc0d 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -122,6 +122,42 @@ #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1) +struct guc_css_header { + uint32_t module_type; + uint32_t header_len; /* header length plus size of all other keys */ + uint32_t header_version; + uint32_t module_id; + uint32_t module_vendor; + union { + struct { + uint8_t day; + uint8_t month; + uint16_t year; + }; + uint32_t date; + }; + uint32_t size; /* uCode size plus header_len */ + uint32_t key_size; + uint32_t modulus_size; + uint32_t exponent_size; + union { + struct { + uint8_t hour; + uint8_t min; + uint16_t sec; + }; + uint32_t time; + }; + + char username[8]; + char buildnumber[12]; + uint32_t device_id; + uint32_t guc_sw_version; + uint32_t prod_preprod_fw; + uint32_t reserved[12]; + uint32_t header_info; +} __packed; + struct guc_doorbell_info { u32 db_status; u32 cookie; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 5823615..0c3c4c9 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -215,18 +215,27 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); } -/* - * Transfer the firmware image to RAM for execution by the microcontroller. +/** + * DOC: GuC Firmware Layout * - * GuC Firmware layout: - * +-------------------------------+ ---- - * | CSS header | 128B - * | contains major/minor version | - * +-------------------------------+ ---- - * | uCode | - * +-------------------------------+ ---- - * | RSA signature | 256B - * +-------------------------------+ ---- + +-------------------------------+ + | guc_css_header | + | contains major/minor version | + +-------------------------------+ + | uCode | + +-------------------------------+ + | RSA signature | + +-------------------------------+ + | modulus key | + +-------------------------------+ + | exponent val | + +-------------------------------+ + * + * The firmware may or may not have modulus key and exponent data. The header, + * uCode and RSA signature are must-have components that will be used by driver. + * Size of each components (in dwords) can be found in header. In the case that + * modulus and exponent are not present in fw, the size value still appears in + * header. * * Architecturally, the DMA engine is bidirectional, and can potentially even * transfer between GTT locations. This functionality is left out of the API @@ -236,30 +245,42 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, * DMA engine in one operation, whereas the RSA signature is loaded via MMIO. */ -#define UOS_CSS_HEADER_OFFSET 0 -#define UOS_VER_MINOR_OFFSET 0x44 -#define UOS_VER_MAJOR_OFFSET 0x46 -#define UOS_CSS_HEADER_SIZE 0x80 -#define UOS_RSA_SIG_SIZE 0x100 - +/* + * Transfer the firmware image to RAM for execution by the microcontroller. + */ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) { struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; + struct guc_css_header *header = &guc_fw->guc_fw_header; struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj; unsigned long offset; struct sg_table *sg = fw_obj->pages; - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)]; + u32 status, header_size, rsa_size, ucode_size, *rsa; int i, ret = 0; - /* uCode size, also is where RSA signature starts */ - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE; - I915_WRITE(DMA_COPY_SIZE, ucode_size); + /* The header plus uCode will be copied to WOPCM via DMA, excluding any + * other components */ + header_size = (header->header_len - header->key_size - + header->modulus_size - header->exponent_size) * sizeof(u32); + ucode_size = (header->size - header->header_len) * sizeof(u32); + + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size); + + /* where RSA signature starts */ + offset = header_size + ucode_size; + + rsa_size = header->key_size * sizeof(u32); + rsa = kmalloc(rsa_size, GFP_KERNEL); + if (!rsa) + return -ENOMEM; /* Copy RSA signature from the fw image to HW for verification */ - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, offset); - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++) + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset); + for (i = 0; i < rsa_size / sizeof(u32); i++) I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]); + kfree(rsa); + /* Set the source address for the new blob */ offset = i915_gem_obj_ggtt_offset(fw_obj); I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); @@ -458,10 +479,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) { struct drm_i915_gem_object *obj; const struct firmware *fw; - const u8 *css_header; - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE; - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE - - 0x8000; /* 32k reserved (8K stack + 24k context) */ + struct guc_css_header *css_header = &guc_fw->guc_fw_header; + size_t size; int err; DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n", @@ -475,12 +494,31 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n", guc_fw->guc_fw_path, fw); - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum %zu)\n", - fw->size, minsize, maxsize); - /* Check the size of the blob befoe examining buffer contents */ - if (fw->size < minsize || fw->size > maxsize) + /* Check the size of the blob before examining buffer contents */ + if (fw->size < sizeof(struct guc_css_header)) { + DRM_ERROR("Firmware header is missing\n"); + goto fail; + } + + memcpy(css_header, fw->data, sizeof(struct guc_css_header)); + + /* At least, it should have header, uCode and RSA. Size of all three. */ + size = (css_header->size - css_header->modulus_size - + css_header->exponent_size) * sizeof(u32); + if (fw->size < size) { + DRM_ERROR("Missing firmware components\n"); goto fail; + } + + /* Header and uCode will be loaded to WOPCM. Size of the two. */ + size -= css_header->key_size * sizeof(u32); + + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { + DRM_ERROR("Firmware is too large to fit in WOPCM\n"); + goto fail; + } /* * The GuC firmware image has the version number embedded at a well-known @@ -488,9 +526,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) * TWO bytes each (i.e. u16), although all pointers and offsets are defined * in terms of bytes (u8). */ - css_header = fw->data + UOS_CSS_HEADER_OFFSET; - guc_fw->guc_fw_major_found = *(u16 *)(css_header + UOS_VER_MAJOR_OFFSET); - guc_fw->guc_fw_minor_found = *(u16 *)(css_header + UOS_VER_MINOR_OFFSET); + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16; + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF; if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/6] drm/i915/guc: Add GuC css header parser 2015-09-10 23:56 ` [PATCH 2/6] drm/i915/guc: Add GuC css header parser yu.dai @ 2015-09-14 9:28 ` Daniel Vetter 0 siblings, 0 replies; 35+ messages in thread From: Daniel Vetter @ 2015-09-14 9:28 UTC (permalink / raw) To: yu.dai; +Cc: intel-gfx On Thu, Sep 10, 2015 at 04:56:08PM -0700, yu.dai@intel.com wrote: > From: Alex Dai <yu.dai@intel.com> > > By using information from GuC css header, we can eliminate some > hard code w.r.t size of some components of firmware. > > v1: 1) guc_css_header is defined as __packed now > 2) Add and correct GuC related topics in kernel/Doc > > Signed-off-by: Alex Dai <yu.dai@intel.com> > --- > Documentation/DocBook/drm.tmpl | 24 +++++++- > drivers/gpu/drm/i915/intel_guc.h | 2 +- > drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++ > drivers/gpu/drm/i915/intel_guc_loader.c | 105 +++++++++++++++++++++----------- > 4 files changed, 130 insertions(+), 37 deletions(-) > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > index 66bc646..f430da6 100644 > --- a/Documentation/DocBook/drm.tmpl > +++ b/Documentation/DocBook/drm.tmpl > @@ -4155,14 +4155,34 @@ int num_ioctls;</synopsis> > <title>GuC-based Command Submission</title> > <sect2> > <title>GuC</title> > -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC > !Idrivers/gpu/drm/i915/intel_guc_loader.c > </sect2> > <sect2> > <title>GuC Client</title> > -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command submissison > +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client > !Idrivers/gpu/drm/i915/i915_guc_submission.c > </sect2> > + <sect2> > + <title>GuC Firmware Layout</title> > + <para>The GuC firmware layout looks like this:</para> > +<programlisting><![CDATA[ > + +-------------------------------+ > + | guc_css_header | > + | contains major/minor version | > + +-------------------------------+ > + | uCode | > + +-------------------------------+ > + | RSA signature | > + +-------------------------------+ > + | modulus key | > + +-------------------------------+ > + | exponent val | > + +-------------------------------+ > +]]></programlisting> Nope, this can be done as DOC: sections directly with latest -nightly. DOC: sections now support markdown, so you only have do indent it either 4 spaces (for fixed-with layout) or drop the lines in between for table layout (iirc, you might want to check that). Needs pandoc installed though, otherwise you can't see the new pretty. But if you have pandoc just run $ make htmldocs and double-check that it all looks good. Thanks, Daniel > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout > +!Idrivers/gpu/drm/i915/intel_guc_loader.c > + </sect2> > </sect1> > > <sect1> > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index 4ec2d27..e1389fc 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -71,6 +71,7 @@ struct intel_guc_fw { > struct drm_i915_gem_object * guc_fw_obj; > enum intel_guc_fw_status guc_fw_fetch_status; > enum intel_guc_fw_status guc_fw_load_status; > + struct guc_css_header guc_fw_header; > > uint16_t guc_fw_major_wanted; > uint16_t guc_fw_minor_wanted; > @@ -80,7 +81,6 @@ struct intel_guc_fw { > > struct intel_guc { > struct intel_guc_fw guc_fw; > - > uint32_t log_flags; > struct drm_i915_gem_object *log_obj; > > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > index e1f47ba..006dc0d 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -122,6 +122,42 @@ > > #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1) > > +struct guc_css_header { > + uint32_t module_type; > + uint32_t header_len; /* header length plus size of all other keys */ > + uint32_t header_version; > + uint32_t module_id; > + uint32_t module_vendor; > + union { > + struct { > + uint8_t day; > + uint8_t month; > + uint16_t year; > + }; > + uint32_t date; > + }; > + uint32_t size; /* uCode size plus header_len */ > + uint32_t key_size; > + uint32_t modulus_size; > + uint32_t exponent_size; > + union { > + struct { > + uint8_t hour; > + uint8_t min; > + uint16_t sec; > + }; > + uint32_t time; > + }; > + > + char username[8]; > + char buildnumber[12]; > + uint32_t device_id; > + uint32_t guc_sw_version; > + uint32_t prod_preprod_fw; > + uint32_t reserved[12]; > + uint32_t header_info; > +} __packed; > + > struct guc_doorbell_info { > u32 db_status; > u32 cookie; > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 5823615..0c3c4c9 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -215,18 +215,27 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, > ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); > } > > -/* > - * Transfer the firmware image to RAM for execution by the microcontroller. > +/** > + * DOC: GuC Firmware Layout > * > - * GuC Firmware layout: > - * +-------------------------------+ ---- > - * | CSS header | 128B > - * | contains major/minor version | > - * +-------------------------------+ ---- > - * | uCode | > - * +-------------------------------+ ---- > - * | RSA signature | 256B > - * +-------------------------------+ ---- > + +-------------------------------+ > + | guc_css_header | > + | contains major/minor version | > + +-------------------------------+ > + | uCode | > + +-------------------------------+ > + | RSA signature | > + +-------------------------------+ > + | modulus key | > + +-------------------------------+ > + | exponent val | > + +-------------------------------+ > + * > + * The firmware may or may not have modulus key and exponent data. The header, > + * uCode and RSA signature are must-have components that will be used by driver. > + * Size of each components (in dwords) can be found in header. In the case that > + * modulus and exponent are not present in fw, the size value still appears in > + * header. > * > * Architecturally, the DMA engine is bidirectional, and can potentially even > * transfer between GTT locations. This functionality is left out of the API > @@ -236,30 +245,42 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, > * DMA engine in one operation, whereas the RSA signature is loaded via MMIO. > */ > > -#define UOS_CSS_HEADER_OFFSET 0 > -#define UOS_VER_MINOR_OFFSET 0x44 > -#define UOS_VER_MAJOR_OFFSET 0x46 > -#define UOS_CSS_HEADER_SIZE 0x80 > -#define UOS_RSA_SIG_SIZE 0x100 > - > +/* > + * Transfer the firmware image to RAM for execution by the microcontroller. > + */ > static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) > { > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > + struct guc_css_header *header = &guc_fw->guc_fw_header; > struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj; > unsigned long offset; > struct sg_table *sg = fw_obj->pages; > - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)]; > + u32 status, header_size, rsa_size, ucode_size, *rsa; > int i, ret = 0; > > - /* uCode size, also is where RSA signature starts */ > - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE; > - I915_WRITE(DMA_COPY_SIZE, ucode_size); > + /* The header plus uCode will be copied to WOPCM via DMA, excluding any > + * other components */ > + header_size = (header->header_len - header->key_size - > + header->modulus_size - header->exponent_size) * sizeof(u32); > + ucode_size = (header->size - header->header_len) * sizeof(u32); > + > + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size); > + > + /* where RSA signature starts */ > + offset = header_size + ucode_size; > + > + rsa_size = header->key_size * sizeof(u32); > + rsa = kmalloc(rsa_size, GFP_KERNEL); > + if (!rsa) > + return -ENOMEM; > > /* Copy RSA signature from the fw image to HW for verification */ > - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, offset); > - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++) > + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset); > + for (i = 0; i < rsa_size / sizeof(u32); i++) > I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]); > > + kfree(rsa); > + > /* Set the source address for the new blob */ > offset = i915_gem_obj_ggtt_offset(fw_obj); > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); > @@ -458,10 +479,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > { > struct drm_i915_gem_object *obj; > const struct firmware *fw; > - const u8 *css_header; > - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE; > - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE > - - 0x8000; /* 32k reserved (8K stack + 24k context) */ > + struct guc_css_header *css_header = &guc_fw->guc_fw_header; > + size_t size; > int err; > > DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n", > @@ -475,12 +494,31 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > > DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n", > guc_fw->guc_fw_path, fw); > - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum %zu)\n", > - fw->size, minsize, maxsize); > > - /* Check the size of the blob befoe examining buffer contents */ > - if (fw->size < minsize || fw->size > maxsize) > + /* Check the size of the blob before examining buffer contents */ > + if (fw->size < sizeof(struct guc_css_header)) { > + DRM_ERROR("Firmware header is missing\n"); > + goto fail; > + } > + > + memcpy(css_header, fw->data, sizeof(struct guc_css_header)); > + > + /* At least, it should have header, uCode and RSA. Size of all three. */ > + size = (css_header->size - css_header->modulus_size - > + css_header->exponent_size) * sizeof(u32); > + if (fw->size < size) { > + DRM_ERROR("Missing firmware components\n"); > goto fail; > + } > + > + /* Header and uCode will be loaded to WOPCM. Size of the two. */ > + size -= css_header->key_size * sizeof(u32); > + > + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ > + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { > + DRM_ERROR("Firmware is too large to fit in WOPCM\n"); > + goto fail; > + } > > /* > * The GuC firmware image has the version number embedded at a well-known > @@ -488,9 +526,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > * TWO bytes each (i.e. u16), although all pointers and offsets are defined > * in terms of bytes (u8). > */ > - css_header = fw->data + UOS_CSS_HEADER_OFFSET; > - guc_fw->guc_fw_major_found = *(u16 *)(css_header + UOS_VER_MAJOR_OFFSET); > - guc_fw->guc_fw_minor_found = *(u16 *)(css_header + UOS_VER_MINOR_OFFSET); > + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16; > + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF; > > if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || > guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 3/6] drm/i915/guc: Add host2guc notification for suspend and resume 2015-09-10 23:56 [PATCH 0/6] Several GuC related patches yu.dai 2015-09-10 23:56 ` [PATCH 1/6] drm/i915/guc: Fix a bug in GuC status check yu.dai 2015-09-10 23:56 ` [PATCH 2/6] drm/i915/guc: Add GuC css header parser yu.dai @ 2015-09-10 23:56 ` yu.dai 2015-09-15 23:30 ` [PATCH 03/15] " yu.dai 2015-09-10 23:56 ` [PATCH 4/6] drm/i915/guc: Don't send flips to GuC yu.dai ` (3 subsequent siblings) 6 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-10 23:56 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> Add host2guc interfaces to nofigy GuC power state changes when enter or resume from power saving state. Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 1 + drivers/gpu/drm/i915/i915_gem.c | 1 + drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_guc.h | 2 ++ drivers/gpu/drm/i915/intel_guc_fwif.h | 8 +++++ drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++- 6 files changed, 65 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index bdec64c..42a9752 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -735,6 +735,7 @@ static int i915_drm_resume(struct drm_device *dev) DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n"); atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter); } + intel_guc_resume(dev); mutex_unlock(&dev->struct_mutex); intel_modeset_init_hw(dev); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 41263cd..2e7fa12 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4462,6 +4462,7 @@ i915_gem_suspend(struct drm_device *dev) i915_gem_retire_requests(dev); i915_gem_stop_ringbuffers(dev); + intel_guc_suspend(dev); mutex_unlock(&dev->struct_mutex); cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 792d0b9..23c0a75 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -914,3 +914,53 @@ void i915_guc_submission_fini(struct drm_device *dev) gem_release_guc_obj(guc->ctx_pool_obj); guc->ctx_pool_obj = NULL; } + +/** + * intel_guc_suspend() - notify GuC entering suspend state + * @dev: drm device + */ +int intel_guc_suspend(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_guc *guc = &dev_priv->guc; + struct intel_context *ctx; + u32 data[3]; + + if (!i915.enable_guc_submission) + return 0; + + ctx = dev_priv->ring[RCS].default_context; + + data[0] = HOST2GUC_ACTION_ENTER_S_STATE; + /* any value greater than GUC_POWER_D0 */ + data[1] = GUC_POWER_D1; + /* first page is shared data with GuC */ + data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state); + + return host2guc_action(guc, data, 3); +} + + +/** + * intel_guc_resume() - notify GuC resuming from suspend state + * @dev: drm device + */ +int intel_guc_resume(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_guc *guc = &dev_priv->guc; + struct intel_context *ctx; + u32 data[3]; + + if (!i915.enable_guc_submission) + return 0; + + ctx = dev_priv->ring[RCS].default_context; + + data[0] = HOST2GUC_ACTION_EXIT_S_STATE; + data[1] = GUC_POWER_D0; + /* first page is shared data with GuC */ + data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state); + + return host2guc_action(guc, data, 3); +} diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index e1389fc..e90c156 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -110,6 +110,8 @@ extern void intel_guc_ucode_init(struct drm_device *dev); extern int intel_guc_ucode_load(struct drm_device *dev); extern void intel_guc_ucode_fini(struct drm_device *dev); extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); +extern int intel_guc_suspend(struct drm_device *dev); +extern int intel_guc_resume(struct drm_device *dev); /* i915_guc_submission.c */ int i915_guc_submission_init(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 006dc0d..f6d0aa4 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -254,12 +254,20 @@ struct guc_context_desc { u64 desc_private; } __packed; +#define GUC_POWER_UNSPECIFIED 0 +#define GUC_POWER_D0 1 +#define GUC_POWER_D1 2 +#define GUC_POWER_D2 3 +#define GUC_POWER_D3 4 + /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */ enum host2guc_action { HOST2GUC_ACTION_DEFAULT = 0x0, HOST2GUC_ACTION_SAMPLE_FORCEWAKE = 0x6, HOST2GUC_ACTION_ALLOCATE_DOORBELL = 0x10, HOST2GUC_ACTION_DEALLOCATE_DOORBELL = 0x20, + HOST2GUC_ACTION_ENTER_S_STATE = 0x501, + HOST2GUC_ACTION_EXIT_S_STATE = 0x502, HOST2GUC_ACTION_SLPC_REQUEST = 0x3003, HOST2GUC_ACTION_LIMIT }; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 0c3c4c9..4eadcd2 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -407,7 +407,6 @@ int intel_guc_ucode_load(struct drm_device *dev) intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); direct_interrupts_to_host(dev_priv); - i915_guc_submission_disable(dev); if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE) return 0; @@ -457,6 +456,9 @@ int intel_guc_ucode_load(struct drm_device *dev) intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); if (i915.enable_guc_submission) { + /* The execbuf_client will be recreated. Release it first. */ + i915_guc_submission_disable(dev); + err = i915_guc_submission_enable(dev); if (err) goto fail; -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 03/15] drm/i915/guc: Add host2guc notification for suspend and resume 2015-09-10 23:56 ` [PATCH 3/6] drm/i915/guc: Add host2guc notification for suspend and resume yu.dai @ 2015-09-15 23:30 ` yu.dai 0 siblings, 0 replies; 35+ messages in thread From: yu.dai @ 2015-09-15 23:30 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> Add host2guc interfaces to nofigy GuC power state changes when enter or resume from power saving state. v1: Change to a more flexible way when fill host to GuC scratch data in order to remove hard coding. Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 1 + drivers/gpu/drm/i915/i915_gem.c | 1 + drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_guc.h | 2 ++ drivers/gpu/drm/i915/intel_guc_fwif.h | 8 +++++ drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++- 6 files changed, 65 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index bdec64c..42a9752 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -735,6 +735,7 @@ static int i915_drm_resume(struct drm_device *dev) DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n"); atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter); } + intel_guc_resume(dev); mutex_unlock(&dev->struct_mutex); intel_modeset_init_hw(dev); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index cb0df7e..c98b3c6 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4458,6 +4458,7 @@ i915_gem_suspend(struct drm_device *dev) i915_gem_retire_requests(dev); i915_gem_stop_ringbuffers(dev); + intel_guc_suspend(dev); mutex_unlock(&dev->struct_mutex); cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 792d0b9..38b6ef4 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -914,3 +914,53 @@ void i915_guc_submission_fini(struct drm_device *dev) gem_release_guc_obj(guc->ctx_pool_obj); guc->ctx_pool_obj = NULL; } + +/** + * intel_guc_suspend() - notify GuC entering suspend state + * @dev: drm device + */ +int intel_guc_suspend(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_guc *guc = &dev_priv->guc; + struct intel_context *ctx; + u32 data[3]; + + if (!i915.enable_guc_submission) + return 0; + + ctx = dev_priv->ring[RCS].default_context; + + data[0] = HOST2GUC_ACTION_ENTER_S_STATE; + /* any value greater than GUC_POWER_D0 */ + data[1] = GUC_POWER_D1; + /* first page is shared data with GuC */ + data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state); + + return host2guc_action(guc, data, ARRAY_SIZE(data)); +} + + +/** + * intel_guc_resume() - notify GuC resuming from suspend state + * @dev: drm device + */ +int intel_guc_resume(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_guc *guc = &dev_priv->guc; + struct intel_context *ctx; + u32 data[3]; + + if (!i915.enable_guc_submission) + return 0; + + ctx = dev_priv->ring[RCS].default_context; + + data[0] = HOST2GUC_ACTION_EXIT_S_STATE; + data[1] = GUC_POWER_D0; + /* first page is shared data with GuC */ + data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state); + + return host2guc_action(guc, data, ARRAY_SIZE(data)); +} diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index e1389fc..e90c156 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -110,6 +110,8 @@ extern void intel_guc_ucode_init(struct drm_device *dev); extern int intel_guc_ucode_load(struct drm_device *dev); extern void intel_guc_ucode_fini(struct drm_device *dev); extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); +extern int intel_guc_suspend(struct drm_device *dev); +extern int intel_guc_resume(struct drm_device *dev); /* i915_guc_submission.c */ int i915_guc_submission_init(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 006dc0d..f6d0aa4 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -254,12 +254,20 @@ struct guc_context_desc { u64 desc_private; } __packed; +#define GUC_POWER_UNSPECIFIED 0 +#define GUC_POWER_D0 1 +#define GUC_POWER_D1 2 +#define GUC_POWER_D2 3 +#define GUC_POWER_D3 4 + /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */ enum host2guc_action { HOST2GUC_ACTION_DEFAULT = 0x0, HOST2GUC_ACTION_SAMPLE_FORCEWAKE = 0x6, HOST2GUC_ACTION_ALLOCATE_DOORBELL = 0x10, HOST2GUC_ACTION_DEALLOCATE_DOORBELL = 0x20, + HOST2GUC_ACTION_ENTER_S_STATE = 0x501, + HOST2GUC_ACTION_EXIT_S_STATE = 0x502, HOST2GUC_ACTION_SLPC_REQUEST = 0x3003, HOST2GUC_ACTION_LIMIT }; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index a6703b4..740bfb3 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -416,7 +416,6 @@ int intel_guc_ucode_load(struct drm_device *dev) intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); direct_interrupts_to_host(dev_priv); - i915_guc_submission_disable(dev); if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE) return 0; @@ -466,6 +465,9 @@ int intel_guc_ucode_load(struct drm_device *dev) intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); if (i915.enable_guc_submission) { + /* The execbuf_client will be recreated. Release it first. */ + i915_guc_submission_disable(dev); + err = i915_guc_submission_enable(dev); if (err) goto fail; -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 4/6] drm/i915/guc: Don't send flips to GuC 2015-09-10 23:56 [PATCH 0/6] Several GuC related patches yu.dai ` (2 preceding siblings ...) 2015-09-10 23:56 ` [PATCH 3/6] drm/i915/guc: Add host2guc notification for suspend and resume yu.dai @ 2015-09-10 23:56 ` yu.dai 2015-09-11 12:44 ` [PATCH v2 1/1] drm/i915: Direct all DE interrupts to host Sagar Arun Kamble 2015-09-10 23:56 ` [PATCH 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state yu.dai ` (2 subsequent siblings) 6 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-10 23:56 UTC (permalink / raw) To: intel-gfx From: Sagar Arun Kamble <sagar.a.kamble@intel.com> Due to flip interrupts GuC stays awake always and GT does not enter RC6. Do not route those interrupts to GuC for now. Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 4eadcd2..3dcc656 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -110,11 +110,11 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) for_each_ring(ring, dev_priv, i) I915_WRITE(RING_MODE_GEN7(ring), irqs); - /* tell DE to send (all) flip_done to GuC */ - irqs = DERRMR_PIPEA_PRI_FLIP_DONE | DERRMR_PIPEA_SPR_FLIP_DONE | - DERRMR_PIPEB_PRI_FLIP_DONE | DERRMR_PIPEB_SPR_FLIP_DONE | - DERRMR_PIPEC_PRI_FLIP_DONE | DERRMR_PIPEC_SPR_FLIP_DONE; - /* Unmasked bits will cause GuC response message to be sent */ + /* + * RC6 does not work if flips are directed to GuC as it is keeping + * GuC Awake + */ + irqs = 0; I915_WRITE(DE_GUCRMR, ~irqs); /* route USER_INTERRUPT to Host, all others are sent to GuC. */ -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 1/1] drm/i915: Direct all DE interrupts to host. 2015-09-10 23:56 ` [PATCH 4/6] drm/i915/guc: Don't send flips to GuC yu.dai @ 2015-09-11 12:44 ` Sagar Arun Kamble 2015-09-21 19:02 ` O'Rourke, Tom 0 siblings, 1 reply; 35+ messages in thread From: Sagar Arun Kamble @ 2015-09-11 12:44 UTC (permalink / raw) To: intel-gfx; +Cc: Akash Goel Due to flip interrupts routed to GuC, GuC stays awake always and GT does not enter RC6. GuC firmware should re-direct to GuC those interrupts that it can handle. v2: Commit message change and routing all interrupts to host. (Tom) Cc: Alex Dai <yu.dai@intel.com> Cc: Tom O'Rourke <Tom.O'Rourke@intel.com> Cc: Akash Goel <akash.goel@intel.com> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 5eafd31..0b047c4 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -110,12 +110,8 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) for_each_ring(ring, dev_priv, i) I915_WRITE(RING_MODE_GEN7(ring), irqs); - /* tell DE to send (all) flip_done to GuC */ - irqs = DERRMR_PIPEA_PRI_FLIP_DONE | DERRMR_PIPEA_SPR_FLIP_DONE | - DERRMR_PIPEB_PRI_FLIP_DONE | DERRMR_PIPEB_SPR_FLIP_DONE | - DERRMR_PIPEC_PRI_FLIP_DONE | DERRMR_PIPEC_SPR_FLIP_DONE; - /* Unmasked bits will cause GuC response message to be sent */ - I915_WRITE(DE_GUCRMR, ~irqs); + /* tell DE to send nothing to GuC */ + I915_WRITE(DE_GUCRMR, ~0); /* route USER_INTERRUPT to Host, all others are sent to GuC. */ irqs = GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT | -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Direct all DE interrupts to host. 2015-09-11 12:44 ` [PATCH v2 1/1] drm/i915: Direct all DE interrupts to host Sagar Arun Kamble @ 2015-09-21 19:02 ` O'Rourke, Tom 0 siblings, 0 replies; 35+ messages in thread From: O'Rourke, Tom @ 2015-09-21 19:02 UTC (permalink / raw) To: Kamble, Sagar A; +Cc: intel-gfx@lists.freedesktop.org, Goel, Akash On Fri, Sep 11, 2015 at 05:44:32AM -0700, Kamble, Sagar A wrote: > Due to flip interrupts routed to GuC, GuC stays awake always and GT does not enter RC6. > GuC firmware should re-direct to GuC those interrupts that it can handle. > > v2: Commit message change and routing all interrupts to host. (Tom) > > Cc: Alex Dai <yu.dai@intel.com> > Cc: Tom O'Rourke <Tom.O'Rourke@intel.com> > Cc: Akash Goel <akash.goel@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_loader.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 5eafd31..0b047c4 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -110,12 +110,8 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) > for_each_ring(ring, dev_priv, i) > I915_WRITE(RING_MODE_GEN7(ring), irqs); > > - /* tell DE to send (all) flip_done to GuC */ > - irqs = DERRMR_PIPEA_PRI_FLIP_DONE | DERRMR_PIPEA_SPR_FLIP_DONE | > - DERRMR_PIPEB_PRI_FLIP_DONE | DERRMR_PIPEB_SPR_FLIP_DONE | > - DERRMR_PIPEC_PRI_FLIP_DONE | DERRMR_PIPEC_SPR_FLIP_DONE; > - /* Unmasked bits will cause GuC response message to be sent */ > - I915_WRITE(DE_GUCRMR, ~irqs); > + /* tell DE to send nothing to GuC */ > + I915_WRITE(DE_GUCRMR, ~0); [TOR:] Should the host driver be writing these bits in DE_GUCRMR at all? An alternative approach would let GuC set/clear these bits based on whether or not GuC wants to handle the resulting interrupts; the host driver should not touch these bits since the host driver does not know what GuC wants to do (or may have already done with DE_GUCRMR register). Thanks, Tom > > /* route USER_INTERRUPT to Host, all others are sent to GuC. */ > irqs = GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT | > -- > 1.9.1 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state 2015-09-10 23:56 [PATCH 0/6] Several GuC related patches yu.dai ` (3 preceding siblings ...) 2015-09-10 23:56 ` [PATCH 4/6] drm/i915/guc: Don't send flips to GuC yu.dai @ 2015-09-10 23:56 ` yu.dai 2015-09-14 9:32 ` Daniel Vetter 2015-09-15 23:31 ` [PATCH 05/15] " yu.dai 2015-09-10 23:56 ` [PATCH 6/6] drm/i915/guc: Enable GuC submission, where supported yu.dai 2015-09-22 20:48 ` [PATCH v2 0/6] Several GuC related patches yu.dai 6 siblings, 2 replies; 35+ messages in thread From: yu.dai @ 2015-09-10 23:56 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> GuC expects two bits for Render and Media domain separately when driver sends data via host2guc SAMPLE_FORCEWAKE. Bit 0 is for Render and bit 1 is for Media domain. Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 23c0a75..69f9c5e 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -158,7 +158,8 @@ static int host2guc_sample_forcewake(struct intel_guc *guc, u32 data[2]; data[0] = HOST2GUC_ACTION_SAMPLE_FORCEWAKE; - data[1] = (intel_enable_rc6(dev_priv->dev)) ? 1 : 0; + /* bit 0 and 1 are for Render and Media domain separately */ + data[1] = (intel_enable_rc6(dev_priv->dev)) ? 3 : 0; return host2guc_action(guc, data, 2); } -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state 2015-09-10 23:56 ` [PATCH 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state yu.dai @ 2015-09-14 9:32 ` Daniel Vetter 2015-09-15 23:31 ` [PATCH 05/15] " yu.dai 1 sibling, 0 replies; 35+ messages in thread From: Daniel Vetter @ 2015-09-14 9:32 UTC (permalink / raw) To: yu.dai; +Cc: intel-gfx On Thu, Sep 10, 2015 at 04:56:11PM -0700, yu.dai@intel.com wrote: > From: Alex Dai <yu.dai@intel.com> > > GuC expects two bits for Render and Media domain separately when > driver sends data via host2guc SAMPLE_FORCEWAKE. Bit 0 is for > Render and bit 1 is for Media domain. > > Signed-off-by: Alex Dai <yu.dai@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 23c0a75..69f9c5e 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -158,7 +158,8 @@ static int host2guc_sample_forcewake(struct intel_guc *guc, > u32 data[2]; > > data[0] = HOST2GUC_ACTION_SAMPLE_FORCEWAKE; > - data[1] = (intel_enable_rc6(dev_priv->dev)) ? 1 : 0; > + /* bit 0 and 1 are for Render and Media domain separately */ > + data[1] = (intel_enable_rc6(dev_priv->dev)) ? 3 : 0; Please don't have magic values, please add proper defines for 3 (best to do 2 separate defines and | them together). > > return host2guc_action(guc, data, 2); Just aside: More magic values? Maybe replace with ARRAY_SIZE(date) to make it self-explanatory. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 05/15] drm/i915/guc: Media domain bit needed when notify GuC rc6 state 2015-09-10 23:56 ` [PATCH 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state yu.dai 2015-09-14 9:32 ` Daniel Vetter @ 2015-09-15 23:31 ` yu.dai 1 sibling, 0 replies; 35+ messages in thread From: yu.dai @ 2015-09-15 23:31 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> GuC expects two bits for Render and Media domain separately when driver sends data via host2guc SAMPLE_FORCEWAKE. Bit 0 is for Render and bit 1 is for Media domain. v1: Add parameters definition to avoid magic value Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++-- drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 38b6ef4..2bea858 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -158,9 +158,11 @@ static int host2guc_sample_forcewake(struct intel_guc *guc, u32 data[2]; data[0] = HOST2GUC_ACTION_SAMPLE_FORCEWAKE; - data[1] = (intel_enable_rc6(dev_priv->dev)) ? 1 : 0; + /* bit 0 and 1 are for Render and Media domain separately */ + data[1] = (intel_enable_rc6(dev_priv->dev)) ? + GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA : 0; - return host2guc_action(guc, data, 2); + return host2guc_action(guc, data, ARRAY_SIZE(data)); } /* diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index f6d0aa4..ecea053 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -260,6 +260,9 @@ struct guc_context_desc { #define GUC_POWER_D2 3 #define GUC_POWER_D3 4 +#define GUC_FORCEWAKE_RENDER (1 << 0) +#define GUC_FORCEWAKE_MEDIA (1 << 1) + /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */ enum host2guc_action { HOST2GUC_ACTION_DEFAULT = 0x0, -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 6/6] drm/i915/guc: Enable GuC submission, where supported 2015-09-10 23:56 [PATCH 0/6] Several GuC related patches yu.dai ` (4 preceding siblings ...) 2015-09-10 23:56 ` [PATCH 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state yu.dai @ 2015-09-10 23:56 ` yu.dai 2015-09-22 20:48 ` [PATCH v2 0/6] Several GuC related patches yu.dai 6 siblings, 0 replies; 35+ messages in thread From: yu.dai @ 2015-09-10 23:56 UTC (permalink / raw) To: intel-gfx From: Dave Gordon <david.s.gordon@intel.com> This is to enable command submission via GuC. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_params.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 05053e2..a50931c 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -53,7 +53,7 @@ struct i915_params i915 __read_mostly = { .verbose_state_checks = 1, .nuclear_pageflip = 0, .edp_vswing = 0, - .enable_guc_submission = false, + .enable_guc_submission = true, .guc_log_level = -1, }; @@ -190,7 +190,7 @@ MODULE_PARM_DESC(edp_vswing, "2=default swing(400mV))"); module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400); -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)"); +MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:true)"); module_param_named(guc_log_level, i915.guc_log_level, int, 0400); MODULE_PARM_DESC(guc_log_level, -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 0/6] Several GuC related patches 2015-09-10 23:56 [PATCH 0/6] Several GuC related patches yu.dai ` (5 preceding siblings ...) 2015-09-10 23:56 ` [PATCH 6/6] drm/i915/guc: Enable GuC submission, where supported yu.dai @ 2015-09-22 20:48 ` yu.dai 2015-09-22 20:48 ` [PATCH v2 1/6] drm/i915/guc: Fix a bug in GuC status check yu.dai ` (5 more replies) 6 siblings, 6 replies; 35+ messages in thread From: yu.dai @ 2015-09-22 20:48 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> Collection of some latest fixes for GuC. v2: This version changes the way how driver handle DE_GUCRMR (0x44054) in patch "Don't send flips to GuC". Now driver does not program it and leave it to GuC firmware or HW default value. Alex Dai (4): drm/i915/guc: Fix a bug in GuC status check drm/i915/guc: Add GuC css header parser drm/i915/guc: Add host2guc notification for suspend and resume drm/i915/guc: Media domain bit needed when notify GuC rc6 state Dave Gordon (1): drm/i915/guc: Enable GuC submission, where supported Sagar Arun Kamble (1): drm/i915/guc: Don't send flips to GuC Documentation/DocBook/drm.tmpl | 9 ++- drivers/gpu/drm/i915/i915_drv.c | 1 + drivers/gpu/drm/i915/i915_gem.c | 1 + drivers/gpu/drm/i915/i915_guc_reg.h | 1 + drivers/gpu/drm/i915/i915_guc_submission.c | 56 ++++++++++++- drivers/gpu/drm/i915/i915_params.c | 4 +- drivers/gpu/drm/i915/intel_guc.h | 4 +- drivers/gpu/drm/i915/intel_guc_fwif.h | 47 +++++++++++ drivers/gpu/drm/i915/intel_guc_loader.c | 126 ++++++++++++++++++----------- 9 files changed, 195 insertions(+), 54 deletions(-) -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 1/6] drm/i915/guc: Fix a bug in GuC status check 2015-09-22 20:48 ` [PATCH v2 0/6] Several GuC related patches yu.dai @ 2015-09-22 20:48 ` yu.dai 2015-09-24 4:59 ` Kamble, Sagar A 2015-09-22 20:48 ` [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser yu.dai ` (4 subsequent siblings) 5 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-22 20:48 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> Bit 16 of GuC status indicates resuming from RC6. The LAPIC_DONE status is a reliable readiness flag only when resuming from RC6. This fix a racing issue that allocation of doorbell fails whilst GuC init is not finished. Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_guc_reg.h | 1 + drivers/gpu/drm/i915/intel_guc_loader.c | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h index 8c8e574..dd0e1e8 100644 --- a/drivers/gpu/drm/i915/i915_guc_reg.h +++ b/drivers/gpu/drm/i915/i915_guc_reg.h @@ -37,6 +37,7 @@ #define GS_UKERNEL_READY (0xF0 << GS_UKERNEL_SHIFT) #define GS_MIA_SHIFT 16 #define GS_MIA_MASK (0x07 << GS_MIA_SHIFT) +#define GS_MIA_CORE_STATE (1 << GS_MIA_SHIFT) #define SOFT_SCRATCH(n) (0xc180 + ((n) * 4)) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index e0601cc..40241f3 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -209,9 +209,10 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, u32 *status) { u32 val = I915_READ(GUC_STATUS); + u32 uk_val = val & GS_UKERNEL_MASK; *status = val; - return ((val & GS_UKERNEL_MASK) == GS_UKERNEL_READY || - (val & GS_UKERNEL_MASK) == GS_UKERNEL_LAPIC_DONE); + return (uk_val == GS_UKERNEL_READY || + ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); } /* -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/6] drm/i915/guc: Fix a bug in GuC status check 2015-09-22 20:48 ` [PATCH v2 1/6] drm/i915/guc: Fix a bug in GuC status check yu.dai @ 2015-09-24 4:59 ` Kamble, Sagar A 2015-09-28 8:10 ` Daniel Vetter 0 siblings, 1 reply; 35+ messages in thread From: Kamble, Sagar A @ 2015-09-24 4:59 UTC (permalink / raw) To: yu.dai, intel-gfx; +Cc: Hiremath, Shashidhar Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> On 9/23/2015 2:18 AM, yu.dai@intel.com wrote: > From: Alex Dai <yu.dai@intel.com> > > Bit 16 of GuC status indicates resuming from RC6. The LAPIC_DONE > status is a reliable readiness flag only when resuming from RC6. > This fix a racing issue that allocation of doorbell fails whilst > GuC init is not finished. > > Signed-off-by: Alex Dai <yu.dai@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_reg.h | 1 + > drivers/gpu/drm/i915/intel_guc_loader.c | 5 +++-- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h > index 8c8e574..dd0e1e8 100644 > --- a/drivers/gpu/drm/i915/i915_guc_reg.h > +++ b/drivers/gpu/drm/i915/i915_guc_reg.h > @@ -37,6 +37,7 @@ > #define GS_UKERNEL_READY (0xF0 << GS_UKERNEL_SHIFT) > #define GS_MIA_SHIFT 16 > #define GS_MIA_MASK (0x07 << GS_MIA_SHIFT) > +#define GS_MIA_CORE_STATE (1 << GS_MIA_SHIFT) > > #define SOFT_SCRATCH(n) (0xc180 + ((n) * 4)) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index e0601cc..40241f3 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -209,9 +209,10 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, > u32 *status) > { > u32 val = I915_READ(GUC_STATUS); > + u32 uk_val = val & GS_UKERNEL_MASK; > *status = val; > - return ((val & GS_UKERNEL_MASK) == GS_UKERNEL_READY || > - (val & GS_UKERNEL_MASK) == GS_UKERNEL_LAPIC_DONE); > + return (uk_val == GS_UKERNEL_READY || > + ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); > } > > /* _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/6] drm/i915/guc: Fix a bug in GuC status check 2015-09-24 4:59 ` Kamble, Sagar A @ 2015-09-28 8:10 ` Daniel Vetter 0 siblings, 0 replies; 35+ messages in thread From: Daniel Vetter @ 2015-09-28 8:10 UTC (permalink / raw) To: Kamble, Sagar A; +Cc: intel-gfx, Hiremath, Shashidhar On Thu, Sep 24, 2015 at 10:29:18AM +0530, Kamble, Sagar A wrote: > Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > On 9/23/2015 2:18 AM, yu.dai@intel.com wrote: > >From: Alex Dai <yu.dai@intel.com> > > > >Bit 16 of GuC status indicates resuming from RC6. The LAPIC_DONE > >status is a reliable readiness flag only when resuming from RC6. > >This fix a racing issue that allocation of doorbell fails whilst > >GuC init is not finished. > > > >Signed-off-by: Alex Dai <yu.dai@intel.com> Queued for -next, thanks for the patch. -Daniel > >--- > > drivers/gpu/drm/i915/i915_guc_reg.h | 1 + > > drivers/gpu/drm/i915/intel_guc_loader.c | 5 +++-- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h > >index 8c8e574..dd0e1e8 100644 > >--- a/drivers/gpu/drm/i915/i915_guc_reg.h > >+++ b/drivers/gpu/drm/i915/i915_guc_reg.h > >@@ -37,6 +37,7 @@ > > #define GS_UKERNEL_READY (0xF0 << GS_UKERNEL_SHIFT) > > #define GS_MIA_SHIFT 16 > > #define GS_MIA_MASK (0x07 << GS_MIA_SHIFT) > >+#define GS_MIA_CORE_STATE (1 << GS_MIA_SHIFT) > > #define SOFT_SCRATCH(n) (0xc180 + ((n) * 4)) > >diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > >index e0601cc..40241f3 100644 > >--- a/drivers/gpu/drm/i915/intel_guc_loader.c > >+++ b/drivers/gpu/drm/i915/intel_guc_loader.c > >@@ -209,9 +209,10 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, > > u32 *status) > > { > > u32 val = I915_READ(GUC_STATUS); > >+ u32 uk_val = val & GS_UKERNEL_MASK; > > *status = val; > >- return ((val & GS_UKERNEL_MASK) == GS_UKERNEL_READY || > >- (val & GS_UKERNEL_MASK) == GS_UKERNEL_LAPIC_DONE); > >+ return (uk_val == GS_UKERNEL_READY || > >+ ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); > > } > > /* > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser 2015-09-22 20:48 ` [PATCH v2 0/6] Several GuC related patches yu.dai 2015-09-22 20:48 ` [PATCH v2 1/6] drm/i915/guc: Fix a bug in GuC status check yu.dai @ 2015-09-22 20:48 ` yu.dai 2015-09-24 14:23 ` Dave Gordon 2015-09-22 20:48 ` [PATCH v2 3/6] drm/i915/guc: Add host2guc notification for suspend and resume yu.dai ` (3 subsequent siblings) 5 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-22 20:48 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> By using information from GuC css header, we can eliminate some hard code w.r.t size of some components of firmware. v2: Add indent into DOC to make fixed-width format rather than change the tmpl. v1: 1) guc_css_header is defined as __packed now 2) Add and correct GuC related topics in kernel/Doc Signed-off-by: Alex Dai <yu.dai@intel.com> --- Documentation/DocBook/drm.tmpl | 9 ++- drivers/gpu/drm/i915/intel_guc.h | 2 +- drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++ drivers/gpu/drm/i915/intel_guc_loader.c | 107 ++++++++++++++++++++++---------- 4 files changed, 117 insertions(+), 37 deletions(-) diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 66bc646..116332f 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -4155,14 +4155,19 @@ int num_ioctls;</synopsis> <title>GuC-based Command Submission</title> <sect2> <title>GuC</title> -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC !Idrivers/gpu/drm/i915/intel_guc_loader.c </sect2> <sect2> <title>GuC Client</title> -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command submissison +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client !Idrivers/gpu/drm/i915/i915_guc_submission.c </sect2> + <sect2> + <title>GuC Firmware Layout</title> +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout +!Idrivers/gpu/drm/i915/intel_guc_loader.c + </sect2> </sect1> <sect1> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 4ec2d27..e1389fc 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -71,6 +71,7 @@ struct intel_guc_fw { struct drm_i915_gem_object * guc_fw_obj; enum intel_guc_fw_status guc_fw_fetch_status; enum intel_guc_fw_status guc_fw_load_status; + struct guc_css_header guc_fw_header; uint16_t guc_fw_major_wanted; uint16_t guc_fw_minor_wanted; @@ -80,7 +81,6 @@ struct intel_guc_fw { struct intel_guc { struct intel_guc_fw guc_fw; - uint32_t log_flags; struct drm_i915_gem_object *log_obj; diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index e1f47ba..006dc0d 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -122,6 +122,42 @@ #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1) +struct guc_css_header { + uint32_t module_type; + uint32_t header_len; /* header length plus size of all other keys */ + uint32_t header_version; + uint32_t module_id; + uint32_t module_vendor; + union { + struct { + uint8_t day; + uint8_t month; + uint16_t year; + }; + uint32_t date; + }; + uint32_t size; /* uCode size plus header_len */ + uint32_t key_size; + uint32_t modulus_size; + uint32_t exponent_size; + union { + struct { + uint8_t hour; + uint8_t min; + uint16_t sec; + }; + uint32_t time; + }; + + char username[8]; + char buildnumber[12]; + uint32_t device_id; + uint32_t guc_sw_version; + uint32_t prod_preprod_fw; + uint32_t reserved[12]; + uint32_t header_info; +} __packed; + struct guc_doorbell_info { u32 db_status; u32 cookie; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 40241f3..a6703b4 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -215,18 +215,29 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); } -/* - * Transfer the firmware image to RAM for execution by the microcontroller. +/** + * DOC: GuC Firmware Layout * - * GuC Firmware layout: - * +-------------------------------+ ---- - * | CSS header | 128B - * | contains major/minor version | - * +-------------------------------+ ---- - * | uCode | - * +-------------------------------+ ---- - * | RSA signature | 256B - * +-------------------------------+ ---- + * The GuC firmware layout looks like this: + * + * +-------------------------------+ + * | guc_css_header | + * | contains major/minor version | + * +-------------------------------+ + * | uCode | + * +-------------------------------+ + * | RSA signature | + * +-------------------------------+ + * | modulus key | + * +-------------------------------+ + * | exponent val | + * +-------------------------------+ + * + * The firmware may or may not have modulus key and exponent data. The header, + * uCode and RSA signature are must-have components that will be used by driver. + * Size of each components (in dwords) can be found in header. In the case that + * modulus and exponent are not present in fw, the size value still appears in + * header. * * Architecturally, the DMA engine is bidirectional, and can potentially even * transfer between GTT locations. This functionality is left out of the API @@ -236,30 +247,42 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, * DMA engine in one operation, whereas the RSA signature is loaded via MMIO. */ -#define UOS_CSS_HEADER_OFFSET 0 -#define UOS_VER_MINOR_OFFSET 0x44 -#define UOS_VER_MAJOR_OFFSET 0x46 -#define UOS_CSS_HEADER_SIZE 0x80 -#define UOS_RSA_SIG_SIZE 0x100 - +/* + * Transfer the firmware image to RAM for execution by the microcontroller. + */ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) { struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; + struct guc_css_header *header = &guc_fw->guc_fw_header; struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj; unsigned long offset; struct sg_table *sg = fw_obj->pages; - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)]; + u32 status, header_size, rsa_size, ucode_size, *rsa; int i, ret = 0; - /* uCode size, also is where RSA signature starts */ - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE; - I915_WRITE(DMA_COPY_SIZE, ucode_size); + /* The header plus uCode will be copied to WOPCM via DMA, excluding any + * other components */ + header_size = (header->header_len - header->key_size - + header->modulus_size - header->exponent_size) * sizeof(u32); + ucode_size = (header->size - header->header_len) * sizeof(u32); + + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size); + + /* where RSA signature starts */ + offset = header_size + ucode_size; + + rsa_size = header->key_size * sizeof(u32); + rsa = kmalloc(rsa_size, GFP_KERNEL); + if (!rsa) + return -ENOMEM; /* Copy RSA signature from the fw image to HW for verification */ - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, offset); - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++) + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset); + for (i = 0; i < rsa_size / sizeof(u32); i++) I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]); + kfree(rsa); + /* Set the source address for the new blob */ offset = i915_gem_obj_ggtt_offset(fw_obj); I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); @@ -465,10 +488,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) { struct drm_i915_gem_object *obj; const struct firmware *fw; - const u8 *css_header; - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE; - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE - - 0x8000; /* 32k reserved (8K stack + 24k context) */ + struct guc_css_header *css_header = &guc_fw->guc_fw_header; + size_t size; int err; DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n", @@ -482,12 +503,31 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n", guc_fw->guc_fw_path, fw); - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum %zu)\n", - fw->size, minsize, maxsize); - /* Check the size of the blob befoe examining buffer contents */ - if (fw->size < minsize || fw->size > maxsize) + /* Check the size of the blob before examining buffer contents */ + if (fw->size < sizeof(struct guc_css_header)) { + DRM_ERROR("Firmware header is missing\n"); + goto fail; + } + + memcpy(css_header, fw->data, sizeof(struct guc_css_header)); + + /* At least, it should have header, uCode and RSA. Size of all three. */ + size = (css_header->size - css_header->modulus_size - + css_header->exponent_size) * sizeof(u32); + if (fw->size < size) { + DRM_ERROR("Missing firmware components\n"); goto fail; + } + + /* Header and uCode will be loaded to WOPCM. Size of the two. */ + size -= css_header->key_size * sizeof(u32); + + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { + DRM_ERROR("Firmware is too large to fit in WOPCM\n"); + goto fail; + } /* * The GuC firmware image has the version number embedded at a well-known @@ -495,9 +535,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) * TWO bytes each (i.e. u16), although all pointers and offsets are defined * in terms of bytes (u8). */ - css_header = fw->data + UOS_CSS_HEADER_OFFSET; - guc_fw->guc_fw_major_found = *(u16 *)(css_header + UOS_VER_MAJOR_OFFSET); - guc_fw->guc_fw_minor_found = *(u16 *)(css_header + UOS_VER_MINOR_OFFSET); + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16; + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF; if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser 2015-09-22 20:48 ` [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser yu.dai @ 2015-09-24 14:23 ` Dave Gordon 2015-09-24 18:34 ` Yu Dai 0 siblings, 1 reply; 35+ messages in thread From: Dave Gordon @ 2015-09-24 14:23 UTC (permalink / raw) To: yu.dai, intel-gfx On 22/09/15 21:48, yu.dai@intel.com wrote: > From: Alex Dai <yu.dai@intel.com> > > By using information from GuC css header, we can eliminate some > hard code w.r.t size of some components of firmware. > > v2: Add indent into DOC to make fixed-width format rather than > change the tmpl. > > v1: 1) guc_css_header is defined as __packed now > 2) Add and correct GuC related topics in kernel/Doc > > Signed-off-by: Alex Dai <yu.dai@intel.com> > --- > Documentation/DocBook/drm.tmpl | 9 ++- > drivers/gpu/drm/i915/intel_guc.h | 2 +- > drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++ > drivers/gpu/drm/i915/intel_guc_loader.c | 107 ++++++++++++++++++++++---------- > 4 files changed, 117 insertions(+), 37 deletions(-) > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > index 66bc646..116332f 100644 > --- a/Documentation/DocBook/drm.tmpl > +++ b/Documentation/DocBook/drm.tmpl > @@ -4155,14 +4155,19 @@ int num_ioctls;</synopsis> > <title>GuC-based Command Submission</title> > <sect2> > <title>GuC</title> > -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC > !Idrivers/gpu/drm/i915/intel_guc_loader.c > </sect2> > <sect2> > <title>GuC Client</title> > -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command submissison > +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client > !Idrivers/gpu/drm/i915/i915_guc_submission.c > </sect2> > + <sect2> > + <title>GuC Firmware Layout</title> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout > +!Idrivers/gpu/drm/i915/intel_guc_loader.c > + </sect2> > </sect1> > > <sect1> > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index 4ec2d27..e1389fc 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -71,6 +71,7 @@ struct intel_guc_fw { > struct drm_i915_gem_object * guc_fw_obj; > enum intel_guc_fw_status guc_fw_fetch_status; > enum intel_guc_fw_status guc_fw_load_status; > + struct guc_css_header guc_fw_header; > > uint16_t guc_fw_major_wanted; > uint16_t guc_fw_minor_wanted; > @@ -80,7 +81,6 @@ struct intel_guc_fw { > > struct intel_guc { > struct intel_guc_fw guc_fw; > - > uint32_t log_flags; > struct drm_i915_gem_object *log_obj; > > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > index e1f47ba..006dc0d 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -122,6 +122,42 @@ > > #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1) > > +struct guc_css_header { > + uint32_t module_type; > + uint32_t header_len; /* header length plus size of all other keys */ > + uint32_t header_version; > + uint32_t module_id; > + uint32_t module_vendor; > + union { > + struct { > + uint8_t day; > + uint8_t month; > + uint16_t year; > + }; > + uint32_t date; > + }; > + uint32_t size; /* uCode size plus header_len */ > + uint32_t key_size; > + uint32_t modulus_size; > + uint32_t exponent_size; > + union { > + struct { > + uint8_t hour; > + uint8_t min; > + uint16_t sec; > + }; > + uint32_t time; > + }; > + > + char username[8]; > + char buildnumber[12]; > + uint32_t device_id; > + uint32_t guc_sw_version; > + uint32_t prod_preprod_fw; > + uint32_t reserved[12]; > + uint32_t header_info; > +} __packed; > + > struct guc_doorbell_info { > u32 db_status; > u32 cookie; > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 40241f3..a6703b4 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -215,18 +215,29 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, > ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); > } > > -/* > - * Transfer the firmware image to RAM for execution by the microcontroller. > +/** > + * DOC: GuC Firmware Layout > * > - * GuC Firmware layout: > - * +-------------------------------+ ---- > - * | CSS header | 128B > - * | contains major/minor version | > - * +-------------------------------+ ---- > - * | uCode | > - * +-------------------------------+ ---- > - * | RSA signature | 256B > - * +-------------------------------+ ---- > + * The GuC firmware layout looks like this: > + * > + * +-------------------------------+ > + * | guc_css_header | > + * | contains major/minor version | > + * +-------------------------------+ > + * | uCode | > + * +-------------------------------+ > + * | RSA signature | > + * +-------------------------------+ > + * | modulus key | > + * +-------------------------------+ > + * | exponent val | > + * +-------------------------------+ > + * > + * The firmware may or may not have modulus key and exponent data. The header, > + * uCode and RSA signature are must-have components that will be used by driver. > + * Size of each components (in dwords) can be found in header. In the case that > + * modulus and exponent are not present in fw, the size value still appears in > + * header. I think this picture & commentary belongs just ahead of the structure definition in intel_guc_fwif.h, rather than with the code here. Also, perhaps we nede a bit more explanation of the '*size' fields, since they have been defined in such a counter-intuitive way :( I suggest: * All the 'sizes' in the CSS header are expressed as numbers of * "dwords", and therefore have to be multiplied by sizeof (u32) to * get actual sizes (in the normal sense of byte counts). * * The 'size' field is the total size of the data in the picture * above, and should match the size of the file as provided by the * loader; the file is invalid if this size field is greater than * the actual filesize. * * The 'header_len' field contains the total size of the non-uCode * sections of the file (i.e. the sum of the sizes of the CSS header, * the RSA signature, the modulus key and the exponent). Thus, to find * the size of the uCode we subtract this total from 'size', but to * find the size of the CSS header (which also defines the start of * the uCode), we subtract the other three sizes from this total. The * size of the CSS header thus calculated should match sizeof(struct * guc_css_header) (or exceed it; allowing it to be larger permits * future expansion of the CSS header or insertion of extra sections * here). The file is invalid if this calculated size is less than * sizeof(struct guc_css_header). * * The size of the RSA signature must not exceed that expected by the * hardware. The file is invalid if the value of this field is more * than the size of the signature area in the GuC register space, * currently 0x100 bytes. * * Due to the requirements of the DMA engine, the total size of the * sections that are DMA'd into the GuC's memory (CSS header plus * uCode) must be a multiple of the cache line size. The file is * invalid if this requirement is not met. The rest of this comment can stay with the code ... > * > * Architecturally, the DMA engine is bidirectional, and can potentially even > * transfer between GTT locations. This functionality is left out of the API > @@ -236,30 +247,42 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, > * DMA engine in one operation, whereas the RSA signature is loaded via MMIO. > */ > > -#define UOS_CSS_HEADER_OFFSET 0 > -#define UOS_VER_MINOR_OFFSET 0x44 > -#define UOS_VER_MAJOR_OFFSET 0x46 > -#define UOS_CSS_HEADER_SIZE 0x80 > -#define UOS_RSA_SIG_SIZE 0x100 > - > +/* > + * Transfer the firmware image to RAM for execution by the microcontroller. > + */ > static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) > { > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > + struct guc_css_header *header = &guc_fw->guc_fw_header; > struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj; > unsigned long offset; > struct sg_table *sg = fw_obj->pages; > - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)]; > + u32 status, header_size, rsa_size, ucode_size, *rsa; > int i, ret = 0; I don't like doing the complicated size-based calculations in the DMA routine; I'd rather the important values (RSA start/size, CSS+uCode start/size) were precalculated during loading and saved in the struct intel_guc_fw or a member thereof so that this code already has the exact numbers it needs without any further computation. > - /* uCode size, also is where RSA signature starts */ > - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE; > - I915_WRITE(DMA_COPY_SIZE, ucode_size); > + /* The header plus uCode will be copied to WOPCM via DMA, excluding any > + * other components */ > + header_size = (header->header_len - header->key_size - > + header->modulus_size - header->exponent_size) * sizeof(u32); > + ucode_size = (header->size - header->header_len) * sizeof(u32); > + > + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size); > + > + /* where RSA signature starts */ > + offset = header_size + ucode_size; > + > + rsa_size = header->key_size * sizeof(u32); > + rsa = kmalloc(rsa_size, GFP_KERNEL); > + if (!rsa) > + return -ENOMEM; > > /* Copy RSA signature from the fw image to HW for verification */ > - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, offset); > - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++) > + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset); > + for (i = 0; i < rsa_size / sizeof(u32); i++) > I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]); > > + kfree(rsa); > + > /* Set the source address for the new blob */ > offset = i915_gem_obj_ggtt_offset(fw_obj); > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); > @@ -465,10 +488,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > { > struct drm_i915_gem_object *obj; > const struct firmware *fw; > - const u8 *css_header; > - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE; > - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE > - - 0x8000; /* 32k reserved (8K stack + 24k context) */ > + struct guc_css_header *css_header = &guc_fw->guc_fw_header; > + size_t size; > int err; > > DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n", > @@ -482,12 +503,31 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > > DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n", > guc_fw->guc_fw_path, fw); > - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum %zu)\n", > - fw->size, minsize, maxsize); > > - /* Check the size of the blob befoe examining buffer contents */ > - if (fw->size < minsize || fw->size > maxsize) > + /* Check the size of the blob before examining buffer contents */ > + if (fw->size < sizeof(struct guc_css_header)) { > + DRM_ERROR("Firmware header is missing\n"); > + goto fail; > + } > + > + memcpy(css_header, fw->data, sizeof(struct guc_css_header)); > + > + /* At least, it should have header, uCode and RSA. Size of all three. */ > + size = (css_header->size - css_header->modulus_size - > + css_header->exponent_size) * sizeof(u32); > + if (fw->size < size) { > + DRM_ERROR("Missing firmware components\n"); > goto fail; > + } > + > + /* Header and uCode will be loaded to WOPCM. Size of the two. */ > + size -= css_header->key_size * sizeof(u32); > + > + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ > + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { > + DRM_ERROR("Firmware is too large to fit in WOPCM\n"); > + goto fail; > + } > > /* > * The GuC firmware image has the version number embedded at a well-known > @@ -495,9 +535,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > * TWO bytes each (i.e. u16), although all pointers and offsets are defined > * in terms of bytes (u8). > */ > - css_header = fw->data + UOS_CSS_HEADER_OFFSET; > - guc_fw->guc_fw_major_found = *(u16 *)(css_header + UOS_VER_MAJOR_OFFSET); > - guc_fw->guc_fw_minor_found = *(u16 *)(css_header + UOS_VER_MINOR_OFFSET); > + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16; > + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF; > > if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || > guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { > We need to validate each of the sizes read from the binary blob before using them in calculations, and we then also need to validate the results of the calculations, to prevent anyone spoofing the loader into writing where it shouldn't. In particular, we need to check: fw->size >= sizeof(struct guc_css_header) css_header->size (*4) <= fw->size css_header->header_len <= css_header->size => (uCode_size = css_header->size - css_header_len) >= 0 css_header->key_size (*4) <= HW_SIG_SIZE (0x100) css_header->key_size <= css_header->header_len css_header->modulus_size <= css_header->header_len css_header->exponent_size <= css_header->header_len css_header->header_len >= css_header->key_size + css_header->modulus_size + css_header->exponent_size; => (css_header_size = css_header->header_len - css_header->key_size - css_header->modulus_size - css_header->exponent_size) >= 0 css_header_size + uCode_size == 0 mod cachelinesize (Since all these sizes are unsigned, we can't (and don't need to) check for negative results from the subtractions, but we can check that each value that's defined as including the sum of other values is actually large enough so that the subtractions give meaningful results). Some of these checks are already there, but I think we should complete all of them to catch any other invalid combinations of values. And then save the computed start/size values so the DMA code can just retrieve the precalculated values. .Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser 2015-09-24 14:23 ` Dave Gordon @ 2015-09-24 18:34 ` Yu Dai 2015-09-24 19:04 ` Dave Gordon 0 siblings, 1 reply; 35+ messages in thread From: Yu Dai @ 2015-09-24 18:34 UTC (permalink / raw) To: Dave Gordon, intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 17379 bytes --] On 09/24/2015 07:23 AM, Dave Gordon wrote: > On 22/09/15 21:48, yu.dai@intel.com wrote: > > From: Alex Dai <yu.dai@intel.com> > > > > By using information from GuC css header, we can eliminate some > > hard code w.r.t size of some components of firmware. > > > > v2: Add indent into DOC to make fixed-width format rather than > > change the tmpl. > > > > v1: 1) guc_css_header is defined as __packed now > > 2) Add and correct GuC related topics in kernel/Doc > > > > Signed-off-by: Alex Dai <yu.dai@intel.com> > > --- > > Documentation/DocBook/drm.tmpl | 9 ++- > > drivers/gpu/drm/i915/intel_guc.h | 2 +- > > drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++ > > drivers/gpu/drm/i915/intel_guc_loader.c | 107 ++++++++++++++++++++++---------- > > 4 files changed, 117 insertions(+), 37 deletions(-) > > > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > > index 66bc646..116332f 100644 > > --- a/Documentation/DocBook/drm.tmpl > > +++ b/Documentation/DocBook/drm.tmpl > > @@ -4155,14 +4155,19 @@ int num_ioctls;</synopsis> > > <title>GuC-based Command Submission</title> > > <sect2> > > <title>GuC</title> > > -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader > > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC > > !Idrivers/gpu/drm/i915/intel_guc_loader.c > > </sect2> > > <sect2> > > <title>GuC Client</title> > > -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command submissison > > +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client > > !Idrivers/gpu/drm/i915/i915_guc_submission.c > > </sect2> > > + <sect2> > > + <title>GuC Firmware Layout</title> > > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout > > +!Idrivers/gpu/drm/i915/intel_guc_loader.c > > + </sect2> > > </sect1> > > > > <sect1> > > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > > index 4ec2d27..e1389fc 100644 > > --- a/drivers/gpu/drm/i915/intel_guc.h > > +++ b/drivers/gpu/drm/i915/intel_guc.h > > @@ -71,6 +71,7 @@ struct intel_guc_fw { > > struct drm_i915_gem_object * guc_fw_obj; > > enum intel_guc_fw_status guc_fw_fetch_status; > > enum intel_guc_fw_status guc_fw_load_status; > > + struct guc_css_header guc_fw_header; > > > > uint16_t guc_fw_major_wanted; > > uint16_t guc_fw_minor_wanted; > > @@ -80,7 +81,6 @@ struct intel_guc_fw { > > > > struct intel_guc { > > struct intel_guc_fw guc_fw; > > - > > uint32_t log_flags; > > struct drm_i915_gem_object *log_obj; > > > > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > > index e1f47ba..006dc0d 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > > @@ -122,6 +122,42 @@ > > > > #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1) > > > > +struct guc_css_header { > > + uint32_t module_type; > > + uint32_t header_len; /* header length plus size of all other keys */ > > + uint32_t header_version; > > + uint32_t module_id; > > + uint32_t module_vendor; > > + union { > > + struct { > > + uint8_t day; > > + uint8_t month; > > + uint16_t year; > > + }; > > + uint32_t date; > > + }; > > + uint32_t size; /* uCode size plus header_len */ > > + uint32_t key_size; > > + uint32_t modulus_size; > > + uint32_t exponent_size; > > + union { > > + struct { > > + uint8_t hour; > > + uint8_t min; > > + uint16_t sec; > > + }; > > + uint32_t time; > > + }; > > + > > + char username[8]; > > + char buildnumber[12]; > > + uint32_t device_id; > > + uint32_t guc_sw_version; > > + uint32_t prod_preprod_fw; > > + uint32_t reserved[12]; > > + uint32_t header_info; > > +} __packed; > > + > > struct guc_doorbell_info { > > u32 db_status; > > u32 cookie; > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > > index 40241f3..a6703b4 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > > @@ -215,18 +215,29 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, > > ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE)); > > } > > > > -/* > > - * Transfer the firmware image to RAM for execution by the microcontroller. > > +/** > > + * DOC: GuC Firmware Layout > > * > > - * GuC Firmware layout: > > - * +-------------------------------+ ---- > > - * | CSS header | 128B > > - * | contains major/minor version | > > - * +-------------------------------+ ---- > > - * | uCode | > > - * +-------------------------------+ ---- > > - * | RSA signature | 256B > > - * +-------------------------------+ ---- > > + * The GuC firmware layout looks like this: > > + * > > + * +-------------------------------+ > > + * | guc_css_header | > > + * | contains major/minor version | > > + * +-------------------------------+ > > + * | uCode | > > + * +-------------------------------+ > > + * | RSA signature | > > + * +-------------------------------+ > > + * | modulus key | > > + * +-------------------------------+ > > + * | exponent val | > > + * +-------------------------------+ > > + * > > + * The firmware may or may not have modulus key and exponent data. The header, > > + * uCode and RSA signature are must-have components that will be used by driver. > > + * Size of each components (in dwords) can be found in header. In the case that > > + * modulus and exponent are not present in fw, the size value still appears in > > + * header. > > I think this picture & commentary belongs just ahead of the structure > definition in intel_guc_fwif.h, rather than with the code here. Also, Yes, this can be moved to intel_guc_fwif.h right before css_header structure definition. > perhaps we nede a bit more explanation of the '*size' fields, since they > have been defined in such a counter-intuitive way :( I suggest: > > * All the 'sizes' in the CSS header are expressed as numbers of > * "dwords", and therefore have to be multiplied by sizeof (u32) to > * get actual sizes (in the normal sense of byte counts). > * In comments, I clearly mention that all size of these guc components are in dwords. In next version, I can add surfix _dword to avoid confuse. > * The 'size' field is the total size of the data in the picture > * above, and should match the size of the file as provided by the > * loader; the file is invalid if this size field is greater than > * the actual filesize. This is not right assumption. And, that will change some expectation below related to size checking. I should have make my comments more clear. But "The firmware may or may not have modulus key and exponent data ... In the case that modulus and exponent are not present in fw, the size value still appears in header." We have discussed this with firmware team. Since the 'size' (actually the header) is used to generate RSA key, we can't change it after signing. So, we have information of modulus etc. but driver doesn't need them to load firmware. Likely they are not part of the release bin file. So the 'size' may be larger than the bin file size. Currently, fw check is based on the following rules (maybe I need add these to the comment too): 1. Header, uCode and RSA are must-have components. 2. All firmware components, if they present, are in the sequence illustrated in the layout table. 3. Size info of each component can be found in header, in dwords. 4. Modulus and exponent key are not required by driver. They may not appear in fw but their size info are recorded in header anyway. > * > * The 'header_len' field contains the total size of the non-uCode > * sections of the file (i.e. the sum of the sizes of the CSS header, > * the RSA signature, the modulus key and the exponent). Thus, to find > * the size of the uCode we subtract this total from 'size', but to > * find the size of the CSS header (which also defines the start of > * the uCode), we subtract the other three sizes from this total. The > * size of the CSS header thus calculated should match sizeof(struct > * guc_css_header) (or exceed it; allowing it to be larger permits > * future expansion of the CSS header or insertion of extra sections > * here). The file is invalid if this calculated size is less than > * sizeof(struct guc_css_header). > * > * The size of the RSA signature must not exceed that expected by the > * hardware. The file is invalid if the value of this field is more > * than the size of the signature area in the GuC register space, > * currently 0x100 bytes. > * > * Due to the requirements of the DMA engine, the total size of the > * sections that are DMA'd into the GuC's memory (CSS header plus > * uCode) must be a multiple of the cache line size. The file is > * invalid if this requirement is not met. > All suggestions above are valid at some points. However, be note that, the firmware size checking I put into code is actually to protect driver - just make sure we don't have a bug check due to memory access violation. No matter how much protection you put into driver, you still can't reject the case by driver if someone modifies one byte of uCode. So, as long as we have the bits of header, uCode and RSA, we will load it. HW will fail it anyway if anything goes wrong. > The rest of this comment can stay with the code ... > > > * > > * Architecturally, the DMA engine is bidirectional, and can potentially even > > * transfer between GTT locations. This functionality is left out of the API > > @@ -236,30 +247,42 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv, > > * DMA engine in one operation, whereas the RSA signature is loaded via MMIO. > > */ > > > > -#define UOS_CSS_HEADER_OFFSET 0 > > -#define UOS_VER_MINOR_OFFSET 0x44 > > -#define UOS_VER_MAJOR_OFFSET 0x46 > > -#define UOS_CSS_HEADER_SIZE 0x80 > > -#define UOS_RSA_SIG_SIZE 0x100 > > - > > +/* > > + * Transfer the firmware image to RAM for execution by the microcontroller. > > + */ > > static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) > > { > > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > + struct guc_css_header *header = &guc_fw->guc_fw_header; > > struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj; > > unsigned long offset; > > struct sg_table *sg = fw_obj->pages; > > - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)]; > > + u32 status, header_size, rsa_size, ucode_size, *rsa; > > int i, ret = 0; > > I don't like doing the complicated size-based calculations in the DMA > routine; I'd rather the important values (RSA start/size, CSS+uCode > start/size) were precalculated during loading and saved in the struct > intel_guc_fw or a member thereof so that this code already has the exact > numbers it needs without any further computation. This is a good point. > > - /* uCode size, also is where RSA signature starts */ > > - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE; > > - I915_WRITE(DMA_COPY_SIZE, ucode_size); > > + /* The header plus uCode will be copied to WOPCM via DMA, excluding any > > + * other components */ > > + header_size = (header->header_len - header->key_size - > > + header->modulus_size - header->exponent_size) * sizeof(u32); > > + ucode_size = (header->size - header->header_len) * sizeof(u32); > > + > > + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size); > > + > > + /* where RSA signature starts */ > > + offset = header_size + ucode_size; > > + > > + rsa_size = header->key_size * sizeof(u32); > > + rsa = kmalloc(rsa_size, GFP_KERNEL); > > + if (!rsa) > > + return -ENOMEM; > > > > /* Copy RSA signature from the fw image to HW for verification */ > > - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, offset); > > - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++) > > + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset); > > + for (i = 0; i < rsa_size / sizeof(u32); i++) > > I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]); > > > > + kfree(rsa); > > + > > /* Set the source address for the new blob */ > > offset = i915_gem_obj_ggtt_offset(fw_obj); > > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); > > @@ -465,10 +488,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > > { > > struct drm_i915_gem_object *obj; > > const struct firmware *fw; > > - const u8 *css_header; > > - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE; > > - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE > > - - 0x8000; /* 32k reserved (8K stack + 24k context) */ > > + struct guc_css_header *css_header = &guc_fw->guc_fw_header; > > + size_t size; > > int err; > > > > DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n", > > @@ -482,12 +503,31 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > > > > DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n", > > guc_fw->guc_fw_path, fw); > > - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum %zu)\n", > > - fw->size, minsize, maxsize); > > > > - /* Check the size of the blob befoe examining buffer contents */ > > - if (fw->size < minsize || fw->size > maxsize) > > + /* Check the size of the blob before examining buffer contents */ > > + if (fw->size < sizeof(struct guc_css_header)) { > > + DRM_ERROR("Firmware header is missing\n"); > > + goto fail; > > + } > > + > > + memcpy(css_header, fw->data, sizeof(struct guc_css_header)); > > + > > + /* At least, it should have header, uCode and RSA. Size of all three. */ > > + size = (css_header->size - css_header->modulus_size - > > + css_header->exponent_size) * sizeof(u32); > > + if (fw->size < size) { > > + DRM_ERROR("Missing firmware components\n"); > > goto fail; > > + } > > + > > + /* Header and uCode will be loaded to WOPCM. Size of the two. */ > > + size -= css_header->key_size * sizeof(u32); > > + > > + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ > > + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { > > + DRM_ERROR("Firmware is too large to fit in WOPCM\n"); > > + goto fail; > > + } > > > > /* > > * The GuC firmware image has the version number embedded at a well-known > > @@ -495,9 +535,8 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > > * TWO bytes each (i.e. u16), although all pointers and offsets are defined > > * in terms of bytes (u8). > > */ > > - css_header = fw->data + UOS_CSS_HEADER_OFFSET; > > - guc_fw->guc_fw_major_found = *(u16 *)(css_header + UOS_VER_MAJOR_OFFSET); > > - guc_fw->guc_fw_minor_found = *(u16 *)(css_header + UOS_VER_MINOR_OFFSET); > > + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16; > > + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF; > > > > if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || > > guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { > > > > We need to validate each of the sizes read from the binary blob before > using them in calculations, and we then also need to validate the > results of the calculations, to prevent anyone spoofing the loader into > writing where it shouldn't. > > In particular, we need to check: > > fw->size >= sizeof(struct guc_css_header) > css_header->size (*4) <= fw->size > css_header->header_len <= css_header->size > => (uCode_size = css_header->size - css_header_len) >= 0 > css_header->key_size (*4) <= HW_SIG_SIZE (0x100) > css_header->key_size <= css_header->header_len > css_header->modulus_size <= css_header->header_len > css_header->exponent_size <= css_header->header_len > css_header->header_len >= css_header->key_size + > css_header->modulus_size + > css_header->exponent_size; > => (css_header_size = css_header->header_len > - css_header->key_size > - css_header->modulus_size > - css_header->exponent_size) >= 0 > css_header_size + uCode_size == 0 mod cachelinesize > > (Since all these sizes are unsigned, we can't (and don't need to) check > for negative results from the subtractions, but we can check that each > value that's defined as including the sum of other values is actually > large enough so that the subtractions give meaningful results). > > Some of these checks are already there, but I think we should complete > all of them to catch any other invalid combinations of values. And then > save the computed start/size values so the DMA code can just retrieve > the precalculated values. > I only agree with check between header size and fw size. This allows driver keeps going without bug check. All others between members within css_header are not needed. The reason is simple. If you trust content of css_header, then you don't need to validate them. If you don't trust it, no need to check it anyway. Again, as long as we have enough bits to load to HW, we will let it go. Thanks, Alex [-- Attachment #1.2: Type: text/html, Size: 21227 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser 2015-09-24 18:34 ` Yu Dai @ 2015-09-24 19:04 ` Dave Gordon 2015-09-24 20:23 ` Yu Dai 0 siblings, 1 reply; 35+ messages in thread From: Dave Gordon @ 2015-09-24 19:04 UTC (permalink / raw) To: Yu Dai, intel-gfx On 24/09/15 19:34, Yu Dai wrote: > > > On 09/24/2015 07:23 AM, Dave Gordon wrote: >> On 22/09/15 21:48, yu.dai@intel.com wrote: >> > From: Alex Dai <yu.dai@intel.com> >> > >> > By using information from GuC css header, we can eliminate some >> > hard code w.r.t size of some components of firmware. >> > >> > v2: Add indent into DOC to make fixed-width format rather than >> > change the tmpl. >> > >> > v1: 1) guc_css_header is defined as __packed now >> > 2) Add and correct GuC related topics in kernel/Doc >> > >> > Signed-off-by: Alex Dai <yu.dai@intel.com> >> > --- >> > Documentation/DocBook/drm.tmpl | 9 ++- >> > drivers/gpu/drm/i915/intel_guc.h | 2 +- >> > drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++ >> > drivers/gpu/drm/i915/intel_guc_loader.c | 107 >> ++++++++++++++++++++++---------- >> > 4 files changed, 117 insertions(+), 37 deletions(-) >> > >> > diff --git a/Documentation/DocBook/drm.tmpl >> b/Documentation/DocBook/drm.tmpl >> > index 66bc646..116332f 100644 >> > --- a/Documentation/DocBook/drm.tmpl >> > +++ b/Documentation/DocBook/drm.tmpl >> > @@ -4155,14 +4155,19 @@ int num_ioctls;</synopsis> >> > <title>GuC-based Command Submission</title> >> > <sect2>../linux-image-4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly_4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly-7_amd64.deb >> > <title>GuC</title> >> > -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC >> > !Idrivers/gpu/drm/i915/intel_guc_loader.c >> > </sect2> >> > <sect2> >> > <title>GuC Client</title> >> > -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command >> submissison >> > +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client >> > !Idrivers/gpu/drm/i915/i915_guc_submission.c >> > </sect2> >> > + <sect2> >> > + <title>GuC Firmware Layout</title> >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout >> > +!Idrivers/gpu/drm/i915/intel_guc_loader.c >> > + </sect2> >> > </sect1> >> > >> > <sect1> >> > diff --git a/drivers/gpu/drm/i915/intel_guc.h >> b/drivers/gpu/drm/i915/intel_guc.h >> > index 4ec2d27..e1389fc 100644 >> > --- a/drivers/gpu/drm/i915/intel_guc.h >> > +++ b/drivers/gpu/drm/i915/intel_guc.h >> > @@ -71,6 +71,7 @@ struct intel_guc_fw { >> > struct drm_i915_gem_object * guc_fw_obj; >> > enum intel_guc_fw_status guc_fw_fetch_status; >> > enum intel_guc_fw_status guc_fw_load_status; >> > + struct guc_css_header guc_fw_header; >> > >> > uint16_t guc_fw_major_wanted; >> > uint16_t guc_fw_minor_wanted; >> > @@ -80,7 +81,6 @@ struct intel_guc_fw { >> > >> > struct intel_guc { >> > struct intel_guc_fw guc_fw; >> > - >> > uint32_t log_flags; >> > struct drm_i915_gem_object *log_obj; >> > >> > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >> b/drivers/gpu/drm/i915/intel_guc_fwif.h >> > index e1f47ba..006dc0d 100644 >> > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >> > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >> > @@ -122,6 +122,42 @@ >> > >> > #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1) >> > >> > +struct guc_css_header { >> > + uint32_t module_type; >> > + uint32_t header_len; /* header length plus size of all other >> keys */ >> > + uint32_t header_version; >> > + uint32_t module_id; >> > + uint32_t module_vendor; >> > + union { >> > + struct { >> > + uint8_t day; >> > + uint8_t month; >> > + uint16_t year; >> > + }; >> > + uint32_t date; >> > + }; >> > + uint32_t size; /* uCode size plus header_len */ >> > + uint32_t key_size; >> > + uint32_t modulus_size; >> > + uint32_t exponent_size; >> > + union { >> > + struct { >> > + uint8_t hour; >> > + uint8_t min; >> > + uint16_t sec; >> > + }; >> > + uint32_t time; >> > + }; >> > + >> > + char username[8]; >> > + char buildnumber[12]; >> > + uint32_t device_id; >> > + uint32_t guc_sw_version; >> > + uint32_t prod_preprod_fw; >> > + uint32_t reserved[12]; >> > + uint32_t header_info; >> > +} __packed; >> > + >> > struct guc_doorbell_info { >> > u32 db_status; >> > u32 cookie; >> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >> b/drivers/gpu/drm/i915/intel_guc_loader.c >> > index 40241f3..a6703b4 100644 >> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> > @@ -215,18 +215,29 @@ static inline bool guc_ucode_response(struct >> drm_i915_private *dev_priv, >> > ((val & GS_MIA_CORE_STATE) && uk_val == >> GS_UKERNEL_LAPIC_DONE)); >> > } >> > >> > -/* >> > - * Transfer the firmware image to RAM for execution by the >> microcontroller. >> > +/** >> > + * DOC: GuC Firmware Layout >> > * >> > - * GuC Firmware layout: >> > - * +-------------------------------+ ---- >> > - * | CSS header | 128B >> > - * | contains major/minor version | >> > - * +-------------------------------+ ---- >> > - * | uCode | >> > - * +-------------------------------+ ---- >> > - * | RSA signature | 256B >> > - * +-------------------------------+ ---- >> > + * The GuC firmware layout looks like this: >> > + * >> > + * +-------------------------------+ >> > + * | guc_css_header | >> > + * | contains major/minor version | >> > + * +-------------------------------+ >> > + * | uCode | >> > + * +-------------------------------+ >> > + * | RSA signature | >> > + * +-------------------------------+ >> > + * | modulus key | >> > + * +-------------------------------+ >> > + * | exponent val | >> > + * +-------------------------------+ >> > + * >> > + * The firmware may or may not have modulus key and exponent data. >> The header, >> > + * uCode and RSA signature are must-have components that will be >> used by driver. >> > + * Size of each components (in dwords) can be found in header. In >> the case that >> > + * modulus and exponent are not present in fw, the size value still >> appears in >> > + * header. >> >> I think this picture & commentary belongs just ahead of the structure >> definition in intel_guc_fwif.h, rather than with the code here. Also, > > Yes, this can be moved to intel_guc_fwif.h right before css_header > structure definition. > >> perhaps we nede a bit more explanation of the '*size' fields, since they >> have been defined in such a counter-intuitive way :( I suggest: >> >> * All the 'sizes' in the CSS header are expressed as numbers of >> * "dwords", and therefore have to be multiplied by sizeof (u32) to >> * get actual sizes (in the normal sense of byte counts). >> * > > In comments, I clearly mention that all size of these guc components are > in dwords. In next version, I can add surfix _dword to avoid confuse. The names are systematically (if not yet automatically) derived from the GuC header version, so best not to change them. The comment *in the header file* will suffice. >> * The 'size' field is the total size of the data in the picture >> * above, and should match the size of the file as provided by the >> * loader; the file is invalid if this size field is greater than >> * the actual filesize. > > This is not right assumption. And, that will change some expectation > below related to size checking. I should have make my comments more > clear. But OK, I see, we're going to allow a truncated image file, as long as only unused sections are missing ... > "The firmware may or may not have modulus key and exponent data ... In > the case that modulus and exponent are not present in fw, the size value > still appears in header." > > We have discussed this with firmware team. Since the 'size' (actually > the header) is used to generate RSA key, we can't change it after > signing. So, we have information of modulus etc. but driver doesn't need > them to load firmware. Likely they are not part of the release bin file. > So the 'size' may be larger than the bin file size. Currently, fw check > is based on the following rules (maybe I need add these to the comment > too): > > 1. Header, uCode and RSA are must-have components. > 2. All firmware components, if they present, are in the sequence > illustrated in the layout table. > 3. Size info of each component can be found in header, in dwords. > 4. Modulus and exponent key are not required by driver. They may not > appear in fw but their size info are recorded in header anyway. > >> * The 'header_len' field contains the total size of the non-uCode >> * sections of the file (i.e. the sum of the sizes of the CSS header, >> * the RSA signature, the modulus key and the exponent). Thus, to find >> * the size of the uCode we subtract this total from 'size', but to >> * find the size of the CSS header (which also defines the start of >> * the uCode), we subtract the other three sizes from this total. The >> * size of the CSS header thus calculated should match sizeof(struct >> * guc_css_header) (or exceed it; allowing it to be larger permits >> * future expansion of the CSS header or insertion of extra sections >> * here). The file is invalid if this calculated size is less than >> * sizeof(struct guc_css_header). >> * >> * The size of the RSA signature must not exceed that expected by the >> * hardware. The file is invalid if the value of this field is more >> * than the size of the signature area in the GuC register space, >> * currently 0x100 bytes. >> * >> * Due to the requirements of the DMA engine, the total size of the >> * sections that are DMA'd into the GuC's memory (CSS header plus >> * uCode) must be a multiple of the cache line size. The file is >> * invalid if this requirement is not met. > > All suggestions above are valid at some points. However, be note that, > the firmware size checking I put into code is actually to protect driver > - just make sure we don't have a bug check due to memory access > violation. No matter how much protection you put into driver, you still > can't reject the case by driver if someone modifies one byte of uCode. > So, as long as we have the bits of header, uCode and RSA, we will load > it. HW will fail it anyway if anything goes wrong. All the checks I suggested were of this type. For example, the change from using UOS_RSA_SIG_SIZE to the calculated RSA-signature-size would have allowed a malformed file to trigger writing beyond the 256 bytes of signature-register space, with unpredictable effects. Even if the trailing sections themselves have been removed from the image file, the assertions about the numerical relationships between the '*size' values in the CSS header remain valid and should be checked. >> The rest of this comment can stay with the code ... >> >> > * >> > * Architecturally, the DMA engine is bidirectional, and can >> potentially even >> > * transfer between GTT locations. This functionality is left out >> of the API >> > @@ -236,30 +247,42 @@ static inline bool guc_ucode_response(struct >> drm_i915_private *dev_priv, >> > * DMA engine in one operation, whereas the RSA signature is >> loaded via MMIO. >> > */ >> > >> > -#define UOS_CSS_HEADER_OFFSET 0 >> > -#define UOS_VER_MINOR_OFFSET 0x44 >> > -#define UOS_VER_MAJOR_OFFSET 0x46 >> > -#define UOS_CSS_HEADER_SIZE 0x80 >> > -#define UOS_RSA_SIG_SIZE 0x100 >> > - >> > +/* >> > + * Transfer the firmware image to RAM for execution by the >> microcontroller. >> > + */ >> > static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) >> > { >> > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; >> > + struct guc_css_header *header = &guc_fw->guc_fw_header; >> > struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj; >> > unsigned long offset; >> > struct sg_table *sg = fw_obj->pages; >> > - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)]; >> > + u32 status, header_size, rsa_size, ucode_size, *rsa; >> > int i, ret = 0; >> >> I don't like doing the complicated size-based calculations in the DMA >> routine; I'd rather the important values (RSA start/size, CSS+uCode >> start/size) were precalculated during loading and saved in the struct >> intel_guc_fw or a member thereof so that this code already has the exact >> numbers it needs without any further computation. > > This is a good point. > >> > - /* uCode size, also is where RSA signature starts */ >> > - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE; >> > - I915_WRITE(DMA_COPY_SIZE, ucode_size); >> > + /* The header plus uCode will be copied to WOPCM via DMA, >> excluding any >> > + * other components */ >> > + header_size = (header->header_len - header->key_size - >> > + header->modulus_size - header->exponent_size) * sizeof(u32); >> > + ucode_size = (header->size - header->header_len) * sizeof(u32); >> > + >> > + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size); >> > + >> > + /* where RSA signature starts */ >> > + offset = header_size + ucode_size; >> > + >> > + rsa_size = header->key_size * sizeof(u32); (header->key_size might be, say, 0x80) => rsa_size is 0x200 >> > + rsa = kmalloc(rsa_size, GFP_KERNEL); >> > + if (!rsa) >> > + return -ENOMEM; >> > >> > /* Copy RSA signature from the fw image to HW for verification */ >> > - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, >> offset); >> > - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++) >> > + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset); >> > + for (i = 0; i < rsa_size / sizeof(u32); i++) >> > I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]); i*sizeof(u32) had better not exceed 0x100! OOPS! >> > >> > + kfree(rsa); >> > + >> > /* Set the source address for the new blob */ >> > offset = i915_gem_obj_ggtt_offset(fw_obj); >> > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); >> > @@ -465,10 +488,8 @@ static void guc_fw_fetch(struct drm_device >> *dev, struct intel_guc_fw *guc_fw) >> > { >> > struct drm_i915_gem_object *obj; >> > const struct firmware *fw; >> > - const u8 *css_header; >> > - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE; >> > - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE >> > - - 0x8000; /* 32k reserved (8K stack + 24k context) */ >> > + struct guc_css_header *css_header = &guc_fw->guc_fw_header; >> > + size_t size; >> > int err; >> > >> > DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch >> status %s\n", >> > @@ -482,12 +503,31 @@ static void guc_fw_fetch(struct drm_device >> *dev, struct intel_guc_fw *guc_fw) >> > >> > DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n", >> > guc_fw->guc_fw_path, fw); >> > - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum >> %zu)\n", >> > - fw->size, minsize, maxsize); >> > >> > - /* Check the size of the blob befoe examining buffer contents */ >> > - if (fw->size < minsize || fw->size > maxsize) >> > + /* Check the size of the blob before examining buffer contents */ >> > + if (fw->size < sizeof(struct guc_css_header)) { >> > + DRM_ERROR("Firmware header is missing\n"); >> > + goto fail; >> > + } >> > + >> > + memcpy(css_header, fw->data, sizeof(struct guc_css_header)); >> > + >> > + /* At least, it should have header, uCode and RSA. Size of all >> three. */ >> > + size = (css_header->size - css_header->modulus_size - >> > + css_header->exponent_size) * sizeof(u32); >> > + if (fw->size < size) { >> > + DRM_ERROR("Missing firmware components\n"); >> > goto fail; >> > + } >> > + >> > + /* Header and uCode will be loaded to WOPCM. Size of the two. */ >> > + size -= css_header->key_size * sizeof(u32); >> > + >> > + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ >> > + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { >> > + DRM_ERROR("Firmware is too large to fit in WOPCM\n"); >> > + goto fail; >> > + } >> > >> > /* >> > * The GuC firmware image has the version number embedded at a >> well-known >> > @@ -495,9 +535,8 @@ static void guc_fw_fetch(struct drm_device *dev, >> struct intel_guc_fw *guc_fw) >> > * TWO bytes each (i.e. u16), although all pointers and >> offsets are defined >> > * in terms of bytes (u8). >> > */ >> > - css_header = fw->data + UOS_CSS_HEADER_OFFSET; >> > - guc_fw->guc_fw_major_found = *(u16 *)(css_header + >> UOS_VER_MAJOR_OFFSET); >> > - guc_fw->guc_fw_minor_found = *(u16 *)(css_header + >> UOS_VER_MINOR_OFFSET); >> > + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16; >> > + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF; >> > >> > if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || >> > guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { >> > >> >> We need to validate each of the sizes read from the binary blob before >> using them in calculations, and we then also need to validate the >> results of the calculations, to prevent anyone spoofing the loader into >> writing where it shouldn't. >> >> In particular, we need to check: >> >> fw->size >= sizeof(struct guc_css_header) >> css_header->size (*4) <= fw->size >> css_header->header_len <= css_header->size >> => (uCode_size = css_header->size - css_header_len) >= 0 >> css_header->key_size (*4) <= HW_SIG_SIZE (0x100) >> css_header->key_size <= css_header->header_len >> css_header->modulus_size <= css_header->header_len >> css_header->exponent_size <= css_header->header_len >> css_header->header_len >= css_header->key_size + >> css_header->modulus_size + >> css_header->exponent_size; >> => (css_header_size = css_header->header_len >> - css_header->key_size >> - css_header->modulus_size >> - css_header->exponent_size) >= 0 >> css_header_size + uCode_size == 0 mod cachelinesize >> >> (Since all these sizes are unsigned, we can't (and don't need to) check >> for negative results from the subtractions, but we can check that each >> value that's defined as including the sum of other values is actually >> large enough so that the subtractions give meaningful results). >> >> Some of these checks are already there, but I think we should complete >> all of them to catch any other invalid combinations of values. And then >> save the computed start/size values so the DMA code can just retrieve >> the precalculated values. > > I only agree with check between header size and fw size. This allows > driver keeps going without bug check. All others between members within > css_header are not needed. The reason is simple. If you trust content of > css_header, then you don't need to validate them. If you don't trust it, > no need to check it anyway. Again, as long as we have enough bits to > load to HW, we will let it go. > > Thanks, > Alex We can't trust the CSS header *unless* we validate it, in the sense of ensuring that bad values in it won't cause the driver to do anything it shouldn't (e.g. out of range accesses). For example, what happens if I set header_len to 0x100 and modulus_size to 0xfffffe00? Only if it passes all our checks should we put it into GuC memory; and then the GuC gets the final word on whether it's good or not. .Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser 2015-09-24 19:04 ` Dave Gordon @ 2015-09-24 20:23 ` Yu Dai 2015-09-25 14:45 ` Jani Nikula 0 siblings, 1 reply; 35+ messages in thread From: Yu Dai @ 2015-09-24 20:23 UTC (permalink / raw) To: Dave Gordon, intel-gfx On 09/24/2015 12:04 PM, Dave Gordon wrote: > On 24/09/15 19:34, Yu Dai wrote: > > > > > > On 09/24/2015 07:23 AM, Dave Gordon wrote: > >> On 22/09/15 21:48, yu.dai@intel.com wrote: > >> > From: Alex Dai <yu.dai@intel.com> > >> > > >> > By using information from GuC css header, we can eliminate some > >> > hard code w.r.t size of some components of firmware. > >> > > >> > v2: Add indent into DOC to make fixed-width format rather than > >> > change the tmpl. > >> > > >> > v1: 1) guc_css_header is defined as __packed now > >> > 2) Add and correct GuC related topics in kernel/Doc > >> > > >> > Signed-off-by: Alex Dai <yu.dai@intel.com> > >> > --- > >> > Documentation/DocBook/drm.tmpl | 9 ++- > >> > drivers/gpu/drm/i915/intel_guc.h | 2 +- > >> > drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++ > >> > drivers/gpu/drm/i915/intel_guc_loader.c | 107 > >> ++++++++++++++++++++++---------- > >> > 4 files changed, 117 insertions(+), 37 deletions(-) > >> > > >> > diff --git a/Documentation/DocBook/drm.tmpl > >> b/Documentation/DocBook/drm.tmpl > >> > index 66bc646..116332f 100644 > >> > --- a/Documentation/DocBook/drm.tmpl > >> > +++ b/Documentation/DocBook/drm.tmpl > >> > @@ -4155,14 +4155,19 @@ int num_ioctls;</synopsis> > >> > <title>GuC-based Command Submission</title> > >> > <sect2>../linux-image-4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly_4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly-7_amd64.deb > >> > <title>GuC</title> > >> > -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader > >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC > >> > !Idrivers/gpu/drm/i915/intel_guc_loader.c > >> > </sect2> > >> > <sect2> > >> > <title>GuC Client</title> > >> > -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command > >> submissison > >> > +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client > >> > !Idrivers/gpu/drm/i915/i915_guc_submission.c > >> > </sect2> > >> > + <sect2> > >> > + <title>GuC Firmware Layout</title> > >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout > >> > +!Idrivers/gpu/drm/i915/intel_guc_loader.c > >> > + </sect2> > >> > </sect1> > >> > > >> > <sect1> > >> > diff --git a/drivers/gpu/drm/i915/intel_guc.h > >> b/drivers/gpu/drm/i915/intel_guc.h > >> > index 4ec2d27..e1389fc 100644 > >> > --- a/drivers/gpu/drm/i915/intel_guc.h > >> > +++ b/drivers/gpu/drm/i915/intel_guc.h > >> > @@ -71,6 +71,7 @@ struct intel_guc_fw { > >> > struct drm_i915_gem_object * guc_fw_obj; > >> > enum intel_guc_fw_status guc_fw_fetch_status; > >> > enum intel_guc_fw_status guc_fw_load_status; > >> > + struct guc_css_header guc_fw_header; > >> > > >> > uint16_t guc_fw_major_wanted; > >> > uint16_t guc_fw_minor_wanted; > >> > @@ -80,7 +81,6 @@ struct intel_guc_fw { > >> > > >> > struct intel_guc { > >> > struct intel_guc_fw guc_fw; > >> > - > >> > uint32_t log_flags; > >> > struct drm_i915_gem_object *log_obj; > >> > > >> > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h > >> b/drivers/gpu/drm/i915/intel_guc_fwif.h > >> > index e1f47ba..006dc0d 100644 > >> > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > >> > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > >> > @@ -122,6 +122,42 @@ > >> > > >> > #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1) > >> > > >> > +struct guc_css_header { > >> > + uint32_t module_type; > >> > + uint32_t header_len; /* header length plus size of all other > >> keys */ > >> > + uint32_t header_version; > >> > + uint32_t module_id; > >> > + uint32_t module_vendor; > >> > + union { > >> > + struct { > >> > + uint8_t day; > >> > + uint8_t month; > >> > + uint16_t year; > >> > + }; > >> > + uint32_t date; > >> > + }; > >> > + uint32_t size; /* uCode size plus header_len */ > >> > + uint32_t key_size; > >> > + uint32_t modulus_size; > >> > + uint32_t exponent_size; > >> > + union { > >> > + struct { > >> > + uint8_t hour; > >> > + uint8_t min; > >> > + uint16_t sec; > >> > + }; > >> > + uint32_t time; > >> > + }; > >> > + > >> > + char username[8]; > >> > + char buildnumber[12]; > >> > + uint32_t device_id; > >> > + uint32_t guc_sw_version; > >> > + uint32_t prod_preprod_fw; > >> > + uint32_t reserved[12]; > >> > + uint32_t header_info; > >> > +} __packed; > >> > + > >> > struct guc_doorbell_info { > >> > u32 db_status; > >> > u32 cookie; > >> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > >> b/drivers/gpu/drm/i915/intel_guc_loader.c > >> > index 40241f3..a6703b4 100644 > >> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > >> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > >> > @@ -215,18 +215,29 @@ static inline bool guc_ucode_response(struct > >> drm_i915_private *dev_priv, > >> > ((val & GS_MIA_CORE_STATE) && uk_val == > >> GS_UKERNEL_LAPIC_DONE)); > >> > } > >> > > >> > -/* > >> > - * Transfer the firmware image to RAM for execution by the > >> microcontroller. > >> > +/** > >> > + * DOC: GuC Firmware Layout > >> > * > >> > - * GuC Firmware layout: > >> > - * +-------------------------------+ ---- > >> > - * | CSS header | 128B > >> > - * | contains major/minor version | > >> > - * +-------------------------------+ ---- > >> > - * | uCode | > >> > - * +-------------------------------+ ---- > >> > - * | RSA signature | 256B > >> > - * +-------------------------------+ ---- > >> > + * The GuC firmware layout looks like this: > >> > + * > >> > + * +-------------------------------+ > >> > + * | guc_css_header | > >> > + * | contains major/minor version | > >> > + * +-------------------------------+ > >> > + * | uCode | > >> > + * +-------------------------------+ > >> > + * | RSA signature | > >> > + * +-------------------------------+ > >> > + * | modulus key | > >> > + * +-------------------------------+ > >> > + * | exponent val | > >> > + * +-------------------------------+ > >> > + * > >> > + * The firmware may or may not have modulus key and exponent data. > >> The header, > >> > + * uCode and RSA signature are must-have components that will be > >> used by driver. > >> > + * Size of each components (in dwords) can be found in header. In > >> the case that > >> > + * modulus and exponent are not present in fw, the size value still > >> appears in > >> > + * header. > >> > >> I think this picture & commentary belongs just ahead of the structure > >> definition in intel_guc_fwif.h, rather than with the code here. Also, > > > > Yes, this can be moved to intel_guc_fwif.h right before css_header > > structure definition. > > > >> perhaps we nede a bit more explanation of the '*size' fields, since they > >> have been defined in such a counter-intuitive way :( I suggest: > >> > >> * All the 'sizes' in the CSS header are expressed as numbers of > >> * "dwords", and therefore have to be multiplied by sizeof (u32) to > >> * get actual sizes (in the normal sense of byte counts). > >> * > > > > In comments, I clearly mention that all size of these guc components are > > in dwords. In next version, I can add surfix _dword to avoid confuse. > > The names are systematically (if not yet automatically) derived from the > GuC header version, so best not to change them. The comment *in the > header file* will suffice. > > >> * The 'size' field is the total size of the data in the picture > >> * above, and should match the size of the file as provided by the > >> * loader; the file is invalid if this size field is greater than > >> * the actual filesize. > > > > This is not right assumption. And, that will change some expectation > > below related to size checking. I should have make my comments more > > clear. But > > OK, I see, we're going to allow a truncated image file, as long as only > unused sections are missing ... > > > "The firmware may or may not have modulus key and exponent data ... In > > the case that modulus and exponent are not present in fw, the size value > > still appears in header." > > > > We have discussed this with firmware team. Since the 'size' (actually > > the header) is used to generate RSA key, we can't change it after > > signing. So, we have information of modulus etc. but driver doesn't need > > them to load firmware. Likely they are not part of the release bin file. > > So the 'size' may be larger than the bin file size. Currently, fw check > > is based on the following rules (maybe I need add these to the comment > > too): > > > > 1. Header, uCode and RSA are must-have components. > > 2. All firmware components, if they present, are in the sequence > > illustrated in the layout table. > > 3. Size info of each component can be found in header, in dwords. > > 4. Modulus and exponent key are not required by driver. They may not > > appear in fw but their size info are recorded in header anyway. > > > >> * The 'header_len' field contains the total size of the non-uCode > >> * sections of the file (i.e. the sum of the sizes of the CSS header, > >> * the RSA signature, the modulus key and the exponent). Thus, to find > >> * the size of the uCode we subtract this total from 'size', but to > >> * find the size of the CSS header (which also defines the start of > >> * the uCode), we subtract the other three sizes from this total. The > >> * size of the CSS header thus calculated should match sizeof(struct > >> * guc_css_header) (or exceed it; allowing it to be larger permits > >> * future expansion of the CSS header or insertion of extra sections > >> * here). The file is invalid if this calculated size is less than > >> * sizeof(struct guc_css_header). > >> * > >> * The size of the RSA signature must not exceed that expected by the > >> * hardware. The file is invalid if the value of this field is more > >> * than the size of the signature area in the GuC register space, > >> * currently 0x100 bytes. > >> * > >> * Due to the requirements of the DMA engine, the total size of the > >> * sections that are DMA'd into the GuC's memory (CSS header plus > >> * uCode) must be a multiple of the cache line size. The file is > >> * invalid if this requirement is not met. > > > > All suggestions above are valid at some points. However, be note that, > > the firmware size checking I put into code is actually to protect driver > > - just make sure we don't have a bug check due to memory access > > violation. No matter how much protection you put into driver, you still > > can't reject the case by driver if someone modifies one byte of uCode. > > So, as long as we have the bits of header, uCode and RSA, we will load > > it. HW will fail it anyway if anything goes wrong. > > All the checks I suggested were of this type. For example, the change > from using UOS_RSA_SIG_SIZE to the calculated RSA-signature-size would > have allowed a malformed file to trigger writing beyond the 256 bytes of > signature-register space, with unpredictable effects. > > Even if the trailing sections themselves have been removed from the > image file, the assertions about the numerical relationships between the > '*size' values in the CSS header remain valid and should be checked. > > >> The rest of this comment can stay with the code ... > >> > >> > * > >> > * Architecturally, the DMA engine is bidirectional, and can > >> potentially even > >> > * transfer between GTT locations. This functionality is left out > >> of the API > >> > @@ -236,30 +247,42 @@ static inline bool guc_ucode_response(struct > >> drm_i915_private *dev_priv, > >> > * DMA engine in one operation, whereas the RSA signature is > >> loaded via MMIO. > >> > */ > >> > > >> > -#define UOS_CSS_HEADER_OFFSET 0 > >> > -#define UOS_VER_MINOR_OFFSET 0x44 > >> > -#define UOS_VER_MAJOR_OFFSET 0x46 > >> > -#define UOS_CSS_HEADER_SIZE 0x80 > >> > -#define UOS_RSA_SIG_SIZE 0x100 > >> > - > >> > +/* > >> > + * Transfer the firmware image to RAM for execution by the > >> microcontroller. > >> > + */ > >> > static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) > >> > { > >> > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > >> > + struct guc_css_header *header = &guc_fw->guc_fw_header; > >> > struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj; > >> > unsigned long offset; > >> > struct sg_table *sg = fw_obj->pages; > >> > - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)]; > >> > + u32 status, header_size, rsa_size, ucode_size, *rsa; > >> > int i, ret = 0; > >> > >> I don't like doing the complicated size-based calculations in the DMA > >> routine; I'd rather the important values (RSA start/size, CSS+uCode > >> start/size) were precalculated during loading and saved in the struct > >> intel_guc_fw or a member thereof so that this code already has the exact > >> numbers it needs without any further computation. > > > > This is a good point. > > > >> > - /* uCode size, also is where RSA signature starts */ > >> > - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE; > >> > - I915_WRITE(DMA_COPY_SIZE, ucode_size); > >> > + /* The header plus uCode will be copied to WOPCM via DMA, > >> excluding any > >> > + * other components */ > >> > + header_size = (header->header_len - header->key_size - > >> > + header->modulus_size - header->exponent_size) * sizeof(u32); > >> > + ucode_size = (header->size - header->header_len) * sizeof(u32); > >> > + > >> > + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size); > >> > + > >> > + /* where RSA signature starts */ > >> > + offset = header_size + ucode_size; > >> > + > >> > + rsa_size = header->key_size * sizeof(u32); > (header->key_size might be, say, 0x80) => rsa_size is 0x200 > >> > + rsa = kmalloc(rsa_size, GFP_KERNEL); > >> > + if (!rsa) > >> > + return -ENOMEM; > >> > > >> > /* Copy RSA signature from the fw image to HW for verification */ > >> > - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, > >> offset); > >> > - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++) > >> > + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset); > >> > + for (i = 0; i < rsa_size / sizeof(u32); i++) > >> > I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]); > i*sizeof(u32) had better not exceed 0x100! OOPS! > >> > > >> > + kfree(rsa); > >> > + > >> > /* Set the source address for the new blob */ > >> > offset = i915_gem_obj_ggtt_offset(fw_obj); > >> > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); > >> > @@ -465,10 +488,8 @@ static void guc_fw_fetch(struct drm_device > >> *dev, struct intel_guc_fw *guc_fw) > >> > { > >> > struct drm_i915_gem_object *obj; > >> > const struct firmware *fw; > >> > - const u8 *css_header; > >> > - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE; > >> > - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE > >> > - - 0x8000; /* 32k reserved (8K stack + 24k context) */ > >> > + struct guc_css_header *css_header = &guc_fw->guc_fw_header; > >> > + size_t size; > >> > int err; > >> > > >> > DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch > >> status %s\n", > >> > @@ -482,12 +503,31 @@ static void guc_fw_fetch(struct drm_device > >> *dev, struct intel_guc_fw *guc_fw) > >> > > >> > DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n", > >> > guc_fw->guc_fw_path, fw); > >> > - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum > >> %zu)\n", > >> > - fw->size, minsize, maxsize); > >> > > >> > - /* Check the size of the blob befoe examining buffer contents */ > >> > - if (fw->size < minsize || fw->size > maxsize) > >> > + /* Check the size of the blob before examining buffer contents */ > >> > + if (fw->size < sizeof(struct guc_css_header)) { > >> > + DRM_ERROR("Firmware header is missing\n"); > >> > + goto fail; > >> > + } > >> > + > >> > + memcpy(css_header, fw->data, sizeof(struct guc_css_header)); > >> > + > >> > + /* At least, it should have header, uCode and RSA. Size of all > >> three. */ > >> > + size = (css_header->size - css_header->modulus_size - > >> > + css_header->exponent_size) * sizeof(u32); > >> > + if (fw->size < size) { > >> > + DRM_ERROR("Missing firmware components\n"); > >> > goto fail; > >> > + } > >> > + > >> > + /* Header and uCode will be loaded to WOPCM. Size of the two. */ > >> > + size -= css_header->key_size * sizeof(u32); > >> > + > >> > + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ > >> > + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { > >> > + DRM_ERROR("Firmware is too large to fit in WOPCM\n"); > >> > + goto fail; > >> > + } > >> > > >> > /* > >> > * The GuC firmware image has the version number embedded at a > >> well-known > >> > @@ -495,9 +535,8 @@ static void guc_fw_fetch(struct drm_device *dev, > >> struct intel_guc_fw *guc_fw) > >> > * TWO bytes each (i.e. u16), although all pointers and > >> offsets are defined > >> > * in terms of bytes (u8). > >> > */ > >> > - css_header = fw->data + UOS_CSS_HEADER_OFFSET; > >> > - guc_fw->guc_fw_major_found = *(u16 *)(css_header + > >> UOS_VER_MAJOR_OFFSET); > >> > - guc_fw->guc_fw_minor_found = *(u16 *)(css_header + > >> UOS_VER_MINOR_OFFSET); > >> > + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16; > >> > + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF; > >> > > >> > if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || > >> > guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { > >> > > >> > >> We need to validate each of the sizes read from the binary blob before > >> using them in calculations, and we then also need to validate the > >> results of the calculations, to prevent anyone spoofing the loader into > >> writing where it shouldn't. > >> > >> In particular, we need to check: > >> > >> fw->size >= sizeof(struct guc_css_header) > >> css_header->size (*4) <= fw->size > >> css_header->header_len <= css_header->size > >> => (uCode_size = css_header->size - css_header_len) >= 0 > >> css_header->key_size (*4) <= HW_SIG_SIZE (0x100) > >> css_header->key_size <= css_header->header_len > >> css_header->modulus_size <= css_header->header_len > >> css_header->exponent_size <= css_header->header_len > >> css_header->header_len >= css_header->key_size + > >> css_header->modulus_size + > >> css_header->exponent_size; > >> => (css_header_size = css_header->header_len > >> - css_header->key_size > >> - css_header->modulus_size > >> - css_header->exponent_size) >= 0 > >> css_header_size + uCode_size == 0 mod cachelinesize > >> > >> (Since all these sizes are unsigned, we can't (and don't need to) check > >> for negative results from the subtractions, but we can check that each > >> value that's defined as including the sum of other values is actually > >> large enough so that the subtractions give meaningful results). > >> > >> Some of these checks are already there, but I think we should complete > >> all of them to catch any other invalid combinations of values. And then > >> save the computed start/size values so the DMA code can just retrieve > >> the precalculated values. > > > > I only agree with check between header size and fw size. This allows > > driver keeps going without bug check. All others between members within > > css_header are not needed. The reason is simple. If you trust content of > > css_header, then you don't need to validate them. If you don't trust it, > > no need to check it anyway. Again, as long as we have enough bits to > > load to HW, we will let it go. > > > > Thanks, > > Alex > > We can't trust the CSS header *unless* we validate it, in the sense of > ensuring that bad values in it won't cause the driver to do anything it > shouldn't (e.g. out of range accesses). For example, what happens if I > set header_len to 0x100 and modulus_size to 0xfffffe00? > We will use whatever provided from header to calculate uCode size and RSA offset etc. If all of them are within the bin file range, we will load it. Otherwise, reject it to avoid SW memory access violation. This is already implemented. My point is: if we can't trust the CSS header, then the check such as (css_header.A >= css_header.B - css_header.C) itself is insignificant. Yes, header data might be corrupted. HW will verify it as part of RSA key authentication anyway. -Alex _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser 2015-09-24 20:23 ` Yu Dai @ 2015-09-25 14:45 ` Jani Nikula 2015-09-25 16:31 ` Yu Dai 0 siblings, 1 reply; 35+ messages in thread From: Jani Nikula @ 2015-09-25 14:45 UTC (permalink / raw) To: Yu Dai, Dave Gordon, intel-gfx On Thu, 24 Sep 2015, Yu Dai <yu.dai@intel.com> wrote: > On 09/24/2015 12:04 PM, Dave Gordon wrote: >> On 24/09/15 19:34, Yu Dai wrote: >> > >> > >> > On 09/24/2015 07:23 AM, Dave Gordon wrote: >> >> On 22/09/15 21:48, yu.dai@intel.com wrote: >> >> > From: Alex Dai <yu.dai@intel.com> >> >> > >> >> > By using information from GuC css header, we can eliminate some >> >> > hard code w.r.t size of some components of firmware. >> >> > >> >> > v2: Add indent into DOC to make fixed-width format rather than >> >> > change the tmpl. >> >> > >> >> > v1: 1) guc_css_header is defined as __packed now >> >> > 2) Add and correct GuC related topics in kernel/Doc >> >> > >> >> > Signed-off-by: Alex Dai <yu.dai@intel.com> >> >> > --- >> >> > Documentation/DocBook/drm.tmpl | 9 ++- >> >> > drivers/gpu/drm/i915/intel_guc.h | 2 +- >> >> > drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++ >> >> > drivers/gpu/drm/i915/intel_guc_loader.c | 107 >> >> ++++++++++++++++++++++---------- >> >> > 4 files changed, 117 insertions(+), 37 deletions(-) >> >> > >> >> > diff --git a/Documentation/DocBook/drm.tmpl >> >> b/Documentation/DocBook/drm.tmpl >> >> > index 66bc646..116332f 100644 >> >> > --- a/Documentation/DocBook/drm.tmpl >> >> > +++ b/Documentation/DocBook/drm.tmpl >> >> > @@ -4155,14 +4155,19 @@ int num_ioctls;</synopsis> >> >> > <title>GuC-based Command Submission</title> >> >> > <sect2>../linux-image-4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly_4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly-7_amd64.deb >> >> > <title>GuC</title> >> >> > -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader >> >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC >> >> > !Idrivers/gpu/drm/i915/intel_guc_loader.c >> >> > </sect2> >> >> > <sect2> >> >> > <title>GuC Client</title> >> >> > -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command >> >> submissison >> >> > +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client >> >> > !Idrivers/gpu/drm/i915/i915_guc_submission.c >> >> > </sect2> >> >> > + <sect2> >> >> > + <title>GuC Firmware Layout</title> >> >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout >> >> > +!Idrivers/gpu/drm/i915/intel_guc_loader.c >> >> > + </sect2> >> >> > </sect1> >> >> > >> >> > <sect1> >> >> > diff --git a/drivers/gpu/drm/i915/intel_guc.h >> >> b/drivers/gpu/drm/i915/intel_guc.h >> >> > index 4ec2d27..e1389fc 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_guc.h >> >> > +++ b/drivers/gpu/drm/i915/intel_guc.h >> >> > @@ -71,6 +71,7 @@ struct intel_guc_fw { >> >> > struct drm_i915_gem_object * guc_fw_obj; >> >> > enum intel_guc_fw_status guc_fw_fetch_status; >> >> > enum intel_guc_fw_status guc_fw_load_status; >> >> > + struct guc_css_header guc_fw_header; >> >> > >> >> > uint16_t guc_fw_major_wanted; >> >> > uint16_t guc_fw_minor_wanted; >> >> > @@ -80,7 +81,6 @@ struct intel_guc_fw { >> >> > >> >> > struct intel_guc { >> >> > struct intel_guc_fw guc_fw; >> >> > - >> >> > uint32_t log_flags; >> >> > struct drm_i915_gem_object *log_obj; >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >> >> b/drivers/gpu/drm/i915/intel_guc_fwif.h >> >> > index e1f47ba..006dc0d 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >> >> > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >> >> > @@ -122,6 +122,42 @@ >> >> > >> >> > #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1) >> >> > >> >> > +struct guc_css_header { >> >> > + uint32_t module_type; >> >> > + uint32_t header_len; /* header length plus size of all other >> >> keys */ >> >> > + uint32_t header_version; >> >> > + uint32_t module_id; >> >> > + uint32_t module_vendor; >> >> > + union { >> >> > + struct { >> >> > + uint8_t day; >> >> > + uint8_t month; >> >> > + uint16_t year; >> >> > + }; >> >> > + uint32_t date; >> >> > + }; >> >> > + uint32_t size; /* uCode size plus header_len */ >> >> > + uint32_t key_size; >> >> > + uint32_t modulus_size; >> >> > + uint32_t exponent_size; >> >> > + union { >> >> > + struct { >> >> > + uint8_t hour; >> >> > + uint8_t min; >> >> > + uint16_t sec; >> >> > + }; >> >> > + uint32_t time; >> >> > + }; >> >> > + >> >> > + char username[8]; >> >> > + char buildnumber[12]; >> >> > + uint32_t device_id; >> >> > + uint32_t guc_sw_version; >> >> > + uint32_t prod_preprod_fw; >> >> > + uint32_t reserved[12]; >> >> > + uint32_t header_info; >> >> > +} __packed; >> >> > + >> >> > struct guc_doorbell_info { >> >> > u32 db_status; >> >> > u32 cookie; >> >> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >> >> b/drivers/gpu/drm/i915/intel_guc_loader.c >> >> > index 40241f3..a6703b4 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> >> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> >> > @@ -215,18 +215,29 @@ static inline bool guc_ucode_response(struct >> >> drm_i915_private *dev_priv, >> >> > ((val & GS_MIA_CORE_STATE) && uk_val == >> >> GS_UKERNEL_LAPIC_DONE)); >> >> > } >> >> > >> >> > -/* >> >> > - * Transfer the firmware image to RAM for execution by the >> >> microcontroller. >> >> > +/** >> >> > + * DOC: GuC Firmware Layout >> >> > * >> >> > - * GuC Firmware layout: >> >> > - * +-------------------------------+ ---- >> >> > - * | CSS header | 128B >> >> > - * | contains major/minor version | >> >> > - * +-------------------------------+ ---- >> >> > - * | uCode | >> >> > - * +-------------------------------+ ---- >> >> > - * | RSA signature | 256B >> >> > - * +-------------------------------+ ---- >> >> > + * The GuC firmware layout looks like this: >> >> > + * >> >> > + * +-------------------------------+ >> >> > + * | guc_css_header | >> >> > + * | contains major/minor version | >> >> > + * +-------------------------------+ >> >> > + * | uCode | >> >> > + * +-------------------------------+ >> >> > + * | RSA signature | >> >> > + * +-------------------------------+ >> >> > + * | modulus key | >> >> > + * +-------------------------------+ >> >> > + * | exponent val | >> >> > + * +-------------------------------+ >> >> > + * >> >> > + * The firmware may or may not have modulus key and exponent data. >> >> The header, >> >> > + * uCode and RSA signature are must-have components that will be >> >> used by driver. >> >> > + * Size of each components (in dwords) can be found in header. In >> >> the case that >> >> > + * modulus and exponent are not present in fw, the size value still >> >> appears in >> >> > + * header. >> >> >> >> I think this picture & commentary belongs just ahead of the structure >> >> definition in intel_guc_fwif.h, rather than with the code here. Also, >> > >> > Yes, this can be moved to intel_guc_fwif.h right before css_header >> > structure definition. >> > >> >> perhaps we nede a bit more explanation of the '*size' fields, since they >> >> have been defined in such a counter-intuitive way :( I suggest: >> >> >> >> * All the 'sizes' in the CSS header are expressed as numbers of >> >> * "dwords", and therefore have to be multiplied by sizeof (u32) to >> >> * get actual sizes (in the normal sense of byte counts). >> >> * >> > >> > In comments, I clearly mention that all size of these guc components are >> > in dwords. In next version, I can add surfix _dword to avoid confuse. >> >> The names are systematically (if not yet automatically) derived from the >> GuC header version, so best not to change them. The comment *in the >> header file* will suffice. >> >> >> * The 'size' field is the total size of the data in the picture >> >> * above, and should match the size of the file as provided by the >> >> * loader; the file is invalid if this size field is greater than >> >> * the actual filesize. >> > >> > This is not right assumption. And, that will change some expectation >> > below related to size checking. I should have make my comments more >> > clear. But >> >> OK, I see, we're going to allow a truncated image file, as long as only >> unused sections are missing ... >> >> > "The firmware may or may not have modulus key and exponent data ... In >> > the case that modulus and exponent are not present in fw, the size value >> > still appears in header." >> > >> > We have discussed this with firmware team. Since the 'size' (actually >> > the header) is used to generate RSA key, we can't change it after >> > signing. So, we have information of modulus etc. but driver doesn't need >> > them to load firmware. Likely they are not part of the release bin file. >> > So the 'size' may be larger than the bin file size. Currently, fw check >> > is based on the following rules (maybe I need add these to the comment >> > too): >> > >> > 1. Header, uCode and RSA are must-have components. >> > 2. All firmware components, if they present, are in the sequence >> > illustrated in the layout table. >> > 3. Size info of each component can be found in header, in dwords. >> > 4. Modulus and exponent key are not required by driver. They may not >> > appear in fw but their size info are recorded in header anyway. >> > >> >> * The 'header_len' field contains the total size of the non-uCode >> >> * sections of the file (i.e. the sum of the sizes of the CSS header, >> >> * the RSA signature, the modulus key and the exponent). Thus, to find >> >> * the size of the uCode we subtract this total from 'size', but to >> >> * find the size of the CSS header (which also defines the start of >> >> * the uCode), we subtract the other three sizes from this total. The >> >> * size of the CSS header thus calculated should match sizeof(struct >> >> * guc_css_header) (or exceed it; allowing it to be larger permits >> >> * future expansion of the CSS header or insertion of extra sections >> >> * here). The file is invalid if this calculated size is less than >> >> * sizeof(struct guc_css_header). >> >> * >> >> * The size of the RSA signature must not exceed that expected by the >> >> * hardware. The file is invalid if the value of this field is more >> >> * than the size of the signature area in the GuC register space, >> >> * currently 0x100 bytes. >> >> * >> >> * Due to the requirements of the DMA engine, the total size of the >> >> * sections that are DMA'd into the GuC's memory (CSS header plus >> >> * uCode) must be a multiple of the cache line size. The file is >> >> * invalid if this requirement is not met. >> > >> > All suggestions above are valid at some points. However, be note that, >> > the firmware size checking I put into code is actually to protect driver >> > - just make sure we don't have a bug check due to memory access >> > violation. No matter how much protection you put into driver, you still >> > can't reject the case by driver if someone modifies one byte of uCode. >> > So, as long as we have the bits of header, uCode and RSA, we will load >> > it. HW will fail it anyway if anything goes wrong. >> >> All the checks I suggested were of this type. For example, the change >> from using UOS_RSA_SIG_SIZE to the calculated RSA-signature-size would >> have allowed a malformed file to trigger writing beyond the 256 bytes of >> signature-register space, with unpredictable effects. >> >> Even if the trailing sections themselves have been removed from the >> image file, the assertions about the numerical relationships between the >> '*size' values in the CSS header remain valid and should be checked. >> >> >> The rest of this comment can stay with the code ... >> >> >> >> > * >> >> > * Architecturally, the DMA engine is bidirectional, and can >> >> potentially even >> >> > * transfer between GTT locations. This functionality is left out >> >> of the API >> >> > @@ -236,30 +247,42 @@ static inline bool guc_ucode_response(struct >> >> drm_i915_private *dev_priv, >> >> > * DMA engine in one operation, whereas the RSA signature is >> >> loaded via MMIO. >> >> > */ >> >> > >> >> > -#define UOS_CSS_HEADER_OFFSET 0 >> >> > -#define UOS_VER_MINOR_OFFSET 0x44 >> >> > -#define UOS_VER_MAJOR_OFFSET 0x46 >> >> > -#define UOS_CSS_HEADER_SIZE 0x80 >> >> > -#define UOS_RSA_SIG_SIZE 0x100 >> >> > - >> >> > +/* >> >> > + * Transfer the firmware image to RAM for execution by the >> >> microcontroller. >> >> > + */ >> >> > static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) >> >> > { >> >> > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; >> >> > + struct guc_css_header *header = &guc_fw->guc_fw_header; >> >> > struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj; >> >> > unsigned long offset; >> >> > struct sg_table *sg = fw_obj->pages; >> >> > - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)]; >> >> > + u32 status, header_size, rsa_size, ucode_size, *rsa; >> >> > int i, ret = 0; >> >> >> >> I don't like doing the complicated size-based calculations in the DMA >> >> routine; I'd rather the important values (RSA start/size, CSS+uCode >> >> start/size) were precalculated during loading and saved in the struct >> >> intel_guc_fw or a member thereof so that this code already has the exact >> >> numbers it needs without any further computation. >> > >> > This is a good point. >> > >> >> > - /* uCode size, also is where RSA signature starts */ >> >> > - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE; >> >> > - I915_WRITE(DMA_COPY_SIZE, ucode_size); >> >> > + /* The header plus uCode will be copied to WOPCM via DMA, >> >> excluding any >> >> > + * other components */ >> >> > + header_size = (header->header_len - header->key_size - >> >> > + header->modulus_size - header->exponent_size) * sizeof(u32); >> >> > + ucode_size = (header->size - header->header_len) * sizeof(u32); >> >> > + >> >> > + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size); >> >> > + >> >> > + /* where RSA signature starts */ >> >> > + offset = header_size + ucode_size; >> >> > + >> >> > + rsa_size = header->key_size * sizeof(u32); >> (header->key_size might be, say, 0x80) => rsa_size is 0x200 >> >> > + rsa = kmalloc(rsa_size, GFP_KERNEL); >> >> > + if (!rsa) >> >> > + return -ENOMEM; >> >> > >> >> > /* Copy RSA signature from the fw image to HW for verification */ >> >> > - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, >> >> offset); >> >> > - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++) >> >> > + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset); >> >> > + for (i = 0; i < rsa_size / sizeof(u32); i++) >> >> > I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]); >> i*sizeof(u32) had better not exceed 0x100! OOPS! >> >> > >> >> > + kfree(rsa); >> >> > + >> >> > /* Set the source address for the new blob */ >> >> > offset = i915_gem_obj_ggtt_offset(fw_obj); >> >> > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); >> >> > @@ -465,10 +488,8 @@ static void guc_fw_fetch(struct drm_device >> >> *dev, struct intel_guc_fw *guc_fw) >> >> > { >> >> > struct drm_i915_gem_object *obj; >> >> > const struct firmware *fw; >> >> > - const u8 *css_header; >> >> > - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE; >> >> > - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE >> >> > - - 0x8000; /* 32k reserved (8K stack + 24k context) */ >> >> > + struct guc_css_header *css_header = &guc_fw->guc_fw_header; >> >> > + size_t size; >> >> > int err; >> >> > >> >> > DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch >> >> status %s\n", >> >> > @@ -482,12 +503,31 @@ static void guc_fw_fetch(struct drm_device >> >> *dev, struct intel_guc_fw *guc_fw) >> >> > >> >> > DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n", >> >> > guc_fw->guc_fw_path, fw); >> >> > - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum >> >> %zu)\n", >> >> > - fw->size, minsize, maxsize); >> >> > >> >> > - /* Check the size of the blob befoe examining buffer contents */ >> >> > - if (fw->size < minsize || fw->size > maxsize) >> >> > + /* Check the size of the blob before examining buffer contents */ >> >> > + if (fw->size < sizeof(struct guc_css_header)) { >> >> > + DRM_ERROR("Firmware header is missing\n"); >> >> > + goto fail; >> >> > + } >> >> > + >> >> > + memcpy(css_header, fw->data, sizeof(struct guc_css_header)); >> >> > + >> >> > + /* At least, it should have header, uCode and RSA. Size of all >> >> three. */ >> >> > + size = (css_header->size - css_header->modulus_size - >> >> > + css_header->exponent_size) * sizeof(u32); >> >> > + if (fw->size < size) { >> >> > + DRM_ERROR("Missing firmware components\n"); >> >> > goto fail; >> >> > + } >> >> > + >> >> > + /* Header and uCode will be loaded to WOPCM. Size of the two. */ >> >> > + size -= css_header->key_size * sizeof(u32); >> >> > + >> >> > + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ >> >> > + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { >> >> > + DRM_ERROR("Firmware is too large to fit in WOPCM\n"); >> >> > + goto fail; >> >> > + } >> >> > >> >> > /* >> >> > * The GuC firmware image has the version number embedded at a >> >> well-known >> >> > @@ -495,9 +535,8 @@ static void guc_fw_fetch(struct drm_device *dev, >> >> struct intel_guc_fw *guc_fw) >> >> > * TWO bytes each (i.e. u16), although all pointers and >> >> offsets are defined >> >> > * in terms of bytes (u8). >> >> > */ >> >> > - css_header = fw->data + UOS_CSS_HEADER_OFFSET; >> >> > - guc_fw->guc_fw_major_found = *(u16 *)(css_header + >> >> UOS_VER_MAJOR_OFFSET); >> >> > - guc_fw->guc_fw_minor_found = *(u16 *)(css_header + >> >> UOS_VER_MINOR_OFFSET); >> >> > + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16; >> >> > + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF; >> >> > >> >> > if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || >> >> > guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { >> >> > >> >> >> >> We need to validate each of the sizes read from the binary blob before >> >> using them in calculations, and we then also need to validate the >> >> results of the calculations, to prevent anyone spoofing the loader into >> >> writing where it shouldn't. >> >> >> >> In particular, we need to check: >> >> >> >> fw->size >= sizeof(struct guc_css_header) >> >> css_header->size (*4) <= fw->size >> >> css_header->header_len <= css_header->size >> >> => (uCode_size = css_header->size - css_header_len) >= 0 >> >> css_header->key_size (*4) <= HW_SIG_SIZE (0x100) >> >> css_header->key_size <= css_header->header_len >> >> css_header->modulus_size <= css_header->header_len >> >> css_header->exponent_size <= css_header->header_len >> >> css_header->header_len >= css_header->key_size + >> >> css_header->modulus_size + >> >> css_header->exponent_size; >> >> => (css_header_size = css_header->header_len >> >> - css_header->key_size >> >> - css_header->modulus_size >> >> - css_header->exponent_size) >= 0 >> >> css_header_size + uCode_size == 0 mod cachelinesize >> >> >> >> (Since all these sizes are unsigned, we can't (and don't need to) check >> >> for negative results from the subtractions, but we can check that each >> >> value that's defined as including the sum of other values is actually >> >> large enough so that the subtractions give meaningful results). >> >> >> >> Some of these checks are already there, but I think we should complete >> >> all of them to catch any other invalid combinations of values. And then >> >> save the computed start/size values so the DMA code can just retrieve >> >> the precalculated values. >> > >> > I only agree with check between header size and fw size. This allows >> > driver keeps going without bug check. All others between members within >> > css_header are not needed. The reason is simple. If you trust content of >> > css_header, then you don't need to validate them. If you don't trust it, >> > no need to check it anyway. Again, as long as we have enough bits to >> > load to HW, we will let it go. >> > >> > Thanks, >> > Alex >> >> We can't trust the CSS header *unless* we validate it, in the sense of >> ensuring that bad values in it won't cause the driver to do anything it >> shouldn't (e.g. out of range accesses). For example, what happens if I >> set header_len to 0x100 and modulus_size to 0xfffffe00? >> > > We will use whatever provided from header to calculate uCode size and > RSA offset etc. If all of them are within the bin file range, we will > load it. Otherwise, reject it to avoid SW memory access violation. This > is already implemented. My point is: if we can't trust the CSS header, > then the check such as (css_header.A >= css_header.B - css_header.C) > itself is insignificant. Yes, header data might be corrupted. HW will > verify it as part of RSA key authentication anyway. I guess I was late to the party and commented on an old version [1]. But I'm with Dave here; you need to make sure nothing in kernel breaks with the data you read from the firmware blob. It's not immediately obvious that you're safe when you write malicious data to DMA_COPY_SIZE, or you write out of bounds of UOS_RSA_SCRATCH_0, for example. You *must* make sure they are within sensible limits. BR, Jani. [1] http://mid.gmane.org/87d1x6muq2.fsf@intel.com > > -Alex > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser 2015-09-25 14:45 ` Jani Nikula @ 2015-09-25 16:31 ` Yu Dai 2015-09-28 8:13 ` Daniel Vetter 0 siblings, 1 reply; 35+ messages in thread From: Yu Dai @ 2015-09-25 16:31 UTC (permalink / raw) To: Jani Nikula, Dave Gordon, intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 24025 bytes --] On 09/25/2015 07:45 AM, Jani Nikula wrote: > On Thu, 24 Sep 2015, Yu Dai <yu.dai@intel.com> wrote: > > On 09/24/2015 12:04 PM, Dave Gordon wrote: > >> On 24/09/15 19:34, Yu Dai wrote: > >> > > >> > > >> > On 09/24/2015 07:23 AM, Dave Gordon wrote: > >> >> On 22/09/15 21:48, yu.dai@intel.com wrote: > >> >> > From: Alex Dai <yu.dai@intel.com> > >> >> > > >> >> > By using information from GuC css header, we can eliminate some > >> >> > hard code w.r.t size of some components of firmware. > >> >> > > >> >> > v2: Add indent into DOC to make fixed-width format rather than > >> >> > change the tmpl. > >> >> > > >> >> > v1: 1) guc_css_header is defined as __packed now > >> >> > 2) Add and correct GuC related topics in kernel/Doc > >> >> > > >> >> > Signed-off-by: Alex Dai <yu.dai@intel.com> > >> >> > --- > >> >> > Documentation/DocBook/drm.tmpl | 9 ++- > >> >> > drivers/gpu/drm/i915/intel_guc.h | 2 +- > >> >> > drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++ > >> >> > drivers/gpu/drm/i915/intel_guc_loader.c | 107 > >> >> ++++++++++++++++++++++---------- > >> >> > 4 files changed, 117 insertions(+), 37 deletions(-) > >> >> > > >> >> > diff --git a/Documentation/DocBook/drm.tmpl > >> >> b/Documentation/DocBook/drm.tmpl > >> >> > index 66bc646..116332f 100644 > >> >> > --- a/Documentation/DocBook/drm.tmpl > >> >> > +++ b/Documentation/DocBook/drm.tmpl > >> >> > @@ -4155,14 +4155,19 @@ int num_ioctls;</synopsis> > >> >> > <title>GuC-based Command Submission</title> > >> >> > <sect2>../linux-image-4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly_4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly-7_amd64.deb > >> >> > <title>GuC</title> > >> >> > -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader > >> >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC > >> >> > !Idrivers/gpu/drm/i915/intel_guc_loader.c > >> >> > </sect2> > >> >> > <sect2> > >> >> > <title>GuC Client</title> > >> >> > -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command > >> >> submissison > >> >> > +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client > >> >> > !Idrivers/gpu/drm/i915/i915_guc_submission.c > >> >> > </sect2> > >> >> > + <sect2> > >> >> > + <title>GuC Firmware Layout</title> > >> >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout > >> >> > +!Idrivers/gpu/drm/i915/intel_guc_loader.c > >> >> > + </sect2> > >> >> > </sect1> > >> >> > > >> >> > <sect1> > >> >> > diff --git a/drivers/gpu/drm/i915/intel_guc.h > >> >> b/drivers/gpu/drm/i915/intel_guc.h > >> >> > index 4ec2d27..e1389fc 100644 > >> >> > --- a/drivers/gpu/drm/i915/intel_guc.h > >> >> > +++ b/drivers/gpu/drm/i915/intel_guc.h > >> >> > @@ -71,6 +71,7 @@ struct intel_guc_fw { > >> >> > struct drm_i915_gem_object * guc_fw_obj; > >> >> > enum intel_guc_fw_status guc_fw_fetch_status; > >> >> > enum intel_guc_fw_status guc_fw_load_status; > >> >> > + struct guc_css_header guc_fw_header; > >> >> > > >> >> > uint16_t guc_fw_major_wanted; > >> >> > uint16_t guc_fw_minor_wanted; > >> >> > @@ -80,7 +81,6 @@ struct intel_guc_fw { > >> >> > > >> >> > struct intel_guc { > >> >> > struct intel_guc_fw guc_fw; > >> >> > - > >> >> > uint32_t log_flags; > >> >> > struct drm_i915_gem_object *log_obj; > >> >> > > >> >> > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h > >> >> b/drivers/gpu/drm/i915/intel_guc_fwif.h > >> >> > index e1f47ba..006dc0d 100644 > >> >> > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > >> >> > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > >> >> > @@ -122,6 +122,42 @@ > >> >> > > >> >> > #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1) > >> >> > > >> >> > +struct guc_css_header { > >> >> > + uint32_t module_type; > >> >> > + uint32_t header_len; /* header length plus size of all other > >> >> keys */ > >> >> > + uint32_t header_version; > >> >> > + uint32_t module_id; > >> >> > + uint32_t module_vendor; > >> >> > + union { > >> >> > + struct { > >> >> > + uint8_t day; > >> >> > + uint8_t month; > >> >> > + uint16_t year; > >> >> > + }; > >> >> > + uint32_t date; > >> >> > + }; > >> >> > + uint32_t size; /* uCode size plus header_len */ > >> >> > + uint32_t key_size; > >> >> > + uint32_t modulus_size; > >> >> > + uint32_t exponent_size; > >> >> > + union { > >> >> > + struct { > >> >> > + uint8_t hour; > >> >> > + uint8_t min; > >> >> > + uint16_t sec; > >> >> > + }; > >> >> > + uint32_t time; > >> >> > + }; > >> >> > + > >> >> > + char username[8]; > >> >> > + char buildnumber[12]; > >> >> > + uint32_t device_id; > >> >> > + uint32_t guc_sw_version; > >> >> > + uint32_t prod_preprod_fw; > >> >> > + uint32_t reserved[12]; > >> >> > + uint32_t header_info; > >> >> > +} __packed; > >> >> > + > >> >> > struct guc_doorbell_info { > >> >> > u32 db_status; > >> >> > u32 cookie; > >> >> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > >> >> b/drivers/gpu/drm/i915/intel_guc_loader.c > >> >> > index 40241f3..a6703b4 100644 > >> >> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > >> >> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > >> >> > @@ -215,18 +215,29 @@ static inline bool guc_ucode_response(struct > >> >> drm_i915_private *dev_priv, > >> >> > ((val & GS_MIA_CORE_STATE) && uk_val == > >> >> GS_UKERNEL_LAPIC_DONE)); > >> >> > } > >> >> > > >> >> > -/* > >> >> > - * Transfer the firmware image to RAM for execution by the > >> >> microcontroller. > >> >> > +/** > >> >> > + * DOC: GuC Firmware Layout > >> >> > * > >> >> > - * GuC Firmware layout: > >> >> > - * +-------------------------------+ ---- > >> >> > - * | CSS header | 128B > >> >> > - * | contains major/minor version | > >> >> > - * +-------------------------------+ ---- > >> >> > - * | uCode | > >> >> > - * +-------------------------------+ ---- > >> >> > - * | RSA signature | 256B > >> >> > - * +-------------------------------+ ---- > >> >> > + * The GuC firmware layout looks like this: > >> >> > + * > >> >> > + * +-------------------------------+ > >> >> > + * | guc_css_header | > >> >> > + * | contains major/minor version | > >> >> > + * +-------------------------------+ > >> >> > + * | uCode | > >> >> > + * +-------------------------------+ > >> >> > + * | RSA signature | > >> >> > + * +-------------------------------+ > >> >> > + * | modulus key | > >> >> > + * +-------------------------------+ > >> >> > + * | exponent val | > >> >> > + * +-------------------------------+ > >> >> > + * > >> >> > + * The firmware may or may not have modulus key and exponent data. > >> >> The header, > >> >> > + * uCode and RSA signature are must-have components that will be > >> >> used by driver. > >> >> > + * Size of each components (in dwords) can be found in header. In > >> >> the case that > >> >> > + * modulus and exponent are not present in fw, the size value still > >> >> appears in > >> >> > + * header. > >> >> > >> >> I think this picture & commentary belongs just ahead of the structure > >> >> definition in intel_guc_fwif.h, rather than with the code here. Also, > >> > > >> > Yes, this can be moved to intel_guc_fwif.h right before css_header > >> > structure definition. > >> > > >> >> perhaps we nede a bit more explanation of the '*size' fields, since they > >> >> have been defined in such a counter-intuitive way :( I suggest: > >> >> > >> >> * All the 'sizes' in the CSS header are expressed as numbers of > >> >> * "dwords", and therefore have to be multiplied by sizeof (u32) to > >> >> * get actual sizes (in the normal sense of byte counts). > >> >> * > >> > > >> > In comments, I clearly mention that all size of these guc components are > >> > in dwords. In next version, I can add surfix _dword to avoid confuse. > >> > >> The names are systematically (if not yet automatically) derived from the > >> GuC header version, so best not to change them. The comment *in the > >> header file* will suffice. > >> > >> >> * The 'size' field is the total size of the data in the picture > >> >> * above, and should match the size of the file as provided by the > >> >> * loader; the file is invalid if this size field is greater than > >> >> * the actual filesize. > >> > > >> > This is not right assumption. And, that will change some expectation > >> > below related to size checking. I should have make my comments more > >> > clear. But > >> > >> OK, I see, we're going to allow a truncated image file, as long as only > >> unused sections are missing ... > >> > >> > "The firmware may or may not have modulus key and exponent data ... In > >> > the case that modulus and exponent are not present in fw, the size value > >> > still appears in header." > >> > > >> > We have discussed this with firmware team. Since the 'size' (actually > >> > the header) is used to generate RSA key, we can't change it after > >> > signing. So, we have information of modulus etc. but driver doesn't need > >> > them to load firmware. Likely they are not part of the release bin file. > >> > So the 'size' may be larger than the bin file size. Currently, fw check > >> > is based on the following rules (maybe I need add these to the comment > >> > too): > >> > > >> > 1. Header, uCode and RSA are must-have components. > >> > 2. All firmware components, if they present, are in the sequence > >> > illustrated in the layout table. > >> > 3. Size info of each component can be found in header, in dwords. > >> > 4. Modulus and exponent key are not required by driver. They may not > >> > appear in fw but their size info are recorded in header anyway. > >> > > >> >> * The 'header_len' field contains the total size of the non-uCode > >> >> * sections of the file (i.e. the sum of the sizes of the CSS header, > >> >> * the RSA signature, the modulus key and the exponent). Thus, to find > >> >> * the size of the uCode we subtract this total from 'size', but to > >> >> * find the size of the CSS header (which also defines the start of > >> >> * the uCode), we subtract the other three sizes from this total. The > >> >> * size of the CSS header thus calculated should match sizeof(struct > >> >> * guc_css_header) (or exceed it; allowing it to be larger permits > >> >> * future expansion of the CSS header or insertion of extra sections > >> >> * here). The file is invalid if this calculated size is less than > >> >> * sizeof(struct guc_css_header). > >> >> * > >> >> * The size of the RSA signature must not exceed that expected by the > >> >> * hardware. The file is invalid if the value of this field is more > >> >> * than the size of the signature area in the GuC register space, > >> >> * currently 0x100 bytes. > >> >> * > >> >> * Due to the requirements of the DMA engine, the total size of the > >> >> * sections that are DMA'd into the GuC's memory (CSS header plus > >> >> * uCode) must be a multiple of the cache line size. The file is > >> >> * invalid if this requirement is not met. > >> > > >> > All suggestions above are valid at some points. However, be note that, > >> > the firmware size checking I put into code is actually to protect driver > >> > - just make sure we don't have a bug check due to memory access > >> > violation. No matter how much protection you put into driver, you still > >> > can't reject the case by driver if someone modifies one byte of uCode. > >> > So, as long as we have the bits of header, uCode and RSA, we will load > >> > it. HW will fail it anyway if anything goes wrong. > >> > >> All the checks I suggested were of this type. For example, the change > >> from using UOS_RSA_SIG_SIZE to the calculated RSA-signature-size would > >> have allowed a malformed file to trigger writing beyond the 256 bytes of > >> signature-register space, with unpredictable effects. > >> > >> Even if the trailing sections themselves have been removed from the > >> image file, the assertions about the numerical relationships between the > >> '*size' values in the CSS header remain valid and should be checked. > >> > >> >> The rest of this comment can stay with the code ... > >> >> > >> >> > * > >> >> > * Architecturally, the DMA engine is bidirectional, and can > >> >> potentially even > >> >> > * transfer between GTT locations. This functionality is left out > >> >> of the API > >> >> > @@ -236,30 +247,42 @@ static inline bool guc_ucode_response(struct > >> >> drm_i915_private *dev_priv, > >> >> > * DMA engine in one operation, whereas the RSA signature is > >> >> loaded via MMIO. > >> >> > */ > >> >> > > >> >> > -#define UOS_CSS_HEADER_OFFSET 0 > >> >> > -#define UOS_VER_MINOR_OFFSET 0x44 > >> >> > -#define UOS_VER_MAJOR_OFFSET 0x46 > >> >> > -#define UOS_CSS_HEADER_SIZE 0x80 > >> >> > -#define UOS_RSA_SIG_SIZE 0x100 > >> >> > - > >> >> > +/* > >> >> > + * Transfer the firmware image to RAM for execution by the > >> >> microcontroller. > >> >> > + */ > >> >> > static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) > >> >> > { > >> >> > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > >> >> > + struct guc_css_header *header = &guc_fw->guc_fw_header; > >> >> > struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj; > >> >> > unsigned long offset; > >> >> > struct sg_table *sg = fw_obj->pages; > >> >> > - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)]; > >> >> > + u32 status, header_size, rsa_size, ucode_size, *rsa; > >> >> > int i, ret = 0; > >> >> > >> >> I don't like doing the complicated size-based calculations in the DMA > >> >> routine; I'd rather the important values (RSA start/size, CSS+uCode > >> >> start/size) were precalculated during loading and saved in the struct > >> >> intel_guc_fw or a member thereof so that this code already has the exact > >> >> numbers it needs without any further computation. > >> > > >> > This is a good point. > >> > > >> >> > - /* uCode size, also is where RSA signature starts */ > >> >> > - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE; > >> >> > - I915_WRITE(DMA_COPY_SIZE, ucode_size); > >> >> > + /* The header plus uCode will be copied to WOPCM via DMA, > >> >> excluding any > >> >> > + * other components */ > >> >> > + header_size = (header->header_len - header->key_size - > >> >> > + header->modulus_size - header->exponent_size) * sizeof(u32); > >> >> > + ucode_size = (header->size - header->header_len) * sizeof(u32); > >> >> > + > >> >> > + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size); > >> >> > + > >> >> > + /* where RSA signature starts */ > >> >> > + offset = header_size + ucode_size; > >> >> > + > >> >> > + rsa_size = header->key_size * sizeof(u32); > >> (header->key_size might be, say, 0x80) => rsa_size is 0x200 > >> >> > + rsa = kmalloc(rsa_size, GFP_KERNEL); > >> >> > + if (!rsa) > >> >> > + return -ENOMEM; > >> >> > > >> >> > /* Copy RSA signature from the fw image to HW for verification */ > >> >> > - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, > >> >> offset); > >> >> > - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++) > >> >> > + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset); > >> >> > + for (i = 0; i < rsa_size / sizeof(u32); i++) > >> >> > I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]); > >> i*sizeof(u32) had better not exceed 0x100! OOPS! > >> >> > > >> >> > + kfree(rsa); > >> >> > + > >> >> > /* Set the source address for the new blob */ > >> >> > offset = i915_gem_obj_ggtt_offset(fw_obj); > >> >> > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); > >> >> > @@ -465,10 +488,8 @@ static void guc_fw_fetch(struct drm_device > >> >> *dev, struct intel_guc_fw *guc_fw) > >> >> > { > >> >> > struct drm_i915_gem_object *obj; > >> >> > const struct firmware *fw; > >> >> > - const u8 *css_header; > >> >> > - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE; > >> >> > - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE > >> >> > - - 0x8000; /* 32k reserved (8K stack + 24k context) */ > >> >> > + struct guc_css_header *css_header = &guc_fw->guc_fw_header; > >> >> > + size_t size; > >> >> > int err; > >> >> > > >> >> > DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch > >> >> status %s\n", > >> >> > @@ -482,12 +503,31 @@ static void guc_fw_fetch(struct drm_device > >> >> *dev, struct intel_guc_fw *guc_fw) > >> >> > > >> >> > DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n", > >> >> > guc_fw->guc_fw_path, fw); > >> >> > - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum > >> >> %zu)\n", > >> >> > - fw->size, minsize, maxsize); > >> >> > > >> >> > - /* Check the size of the blob befoe examining buffer contents */ > >> >> > - if (fw->size < minsize || fw->size > maxsize) > >> >> > + /* Check the size of the blob before examining buffer contents */ > >> >> > + if (fw->size < sizeof(struct guc_css_header)) { > >> >> > + DRM_ERROR("Firmware header is missing\n"); > >> >> > + goto fail; > >> >> > + } > >> >> > + > >> >> > + memcpy(css_header, fw->data, sizeof(struct guc_css_header)); > >> >> > + > >> >> > + /* At least, it should have header, uCode and RSA. Size of all > >> >> three. */ > >> >> > + size = (css_header->size - css_header->modulus_size - > >> >> > + css_header->exponent_size) * sizeof(u32); > >> >> > + if (fw->size < size) { > >> >> > + DRM_ERROR("Missing firmware components\n"); > >> >> > goto fail; > >> >> > + } > >> >> > + > >> >> > + /* Header and uCode will be loaded to WOPCM. Size of the two. */ > >> >> > + size -= css_header->key_size * sizeof(u32); > >> >> > + > >> >> > + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ > >> >> > + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { > >> >> > + DRM_ERROR("Firmware is too large to fit in WOPCM\n"); > >> >> > + goto fail; > >> >> > + } > >> >> > > >> >> > /* > >> >> > * The GuC firmware image has the version number embedded at a > >> >> well-known > >> >> > @@ -495,9 +535,8 @@ static void guc_fw_fetch(struct drm_device *dev, > >> >> struct intel_guc_fw *guc_fw) > >> >> > * TWO bytes each (i.e. u16), although all pointers and > >> >> offsets are defined > >> >> > * in terms of bytes (u8). > >> >> > */ > >> >> > - css_header = fw->data + UOS_CSS_HEADER_OFFSET; > >> >> > - guc_fw->guc_fw_major_found = *(u16 *)(css_header + > >> >> UOS_VER_MAJOR_OFFSET); > >> >> > - guc_fw->guc_fw_minor_found = *(u16 *)(css_header + > >> >> UOS_VER_MINOR_OFFSET); > >> >> > + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16; > >> >> > + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF; > >> >> > > >> >> > if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || > >> >> > guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { > >> >> > > >> >> > >> >> We need to validate each of the sizes read from the binary blob before > >> >> using them in calculations, and we then also need to validate the > >> >> results of the calculations, to prevent anyone spoofing the loader into > >> >> writing where it shouldn't. > >> >> > >> >> In particular, we need to check: > >> >> > >> >> fw->size >= sizeof(struct guc_css_header) > >> >> css_header->size (*4) <= fw->size > >> >> css_header->header_len <= css_header->size > >> >> => (uCode_size = css_header->size - css_header_len) >= 0 > >> >> css_header->key_size (*4) <= HW_SIG_SIZE (0x100) > >> >> css_header->key_size <= css_header->header_len > >> >> css_header->modulus_size <= css_header->header_len > >> >> css_header->exponent_size <= css_header->header_len > >> >> css_header->header_len >= css_header->key_size + > >> >> css_header->modulus_size + > >> >> css_header->exponent_size; > >> >> => (css_header_size = css_header->header_len > >> >> - css_header->key_size > >> >> - css_header->modulus_size > >> >> - css_header->exponent_size) >= 0 > >> >> css_header_size + uCode_size == 0 mod cachelinesize > >> >> > >> >> (Since all these sizes are unsigned, we can't (and don't need to) check > >> >> for negative results from the subtractions, but we can check that each > >> >> value that's defined as including the sum of other values is actually > >> >> large enough so that the subtractions give meaningful results). > >> >> > >> >> Some of these checks are already there, but I think we should complete > >> >> all of them to catch any other invalid combinations of values. And then > >> >> save the computed start/size values so the DMA code can just retrieve > >> >> the precalculated values. > >> > > >> > I only agree with check between header size and fw size. This allows > >> > driver keeps going without bug check. All others between members within > >> > css_header are not needed. The reason is simple. If you trust content of > >> > css_header, then you don't need to validate them. If you don't trust it, > >> > no need to check it anyway. Again, as long as we have enough bits to > >> > load to HW, we will let it go. > >> > > >> > Thanks, > >> > Alex > >> > >> We can't trust the CSS header *unless* we validate it, in the sense of > >> ensuring that bad values in it won't cause the driver to do anything it > >> shouldn't (e.g. out of range accesses). For example, what happens if I > >> set header_len to 0x100 and modulus_size to 0xfffffe00? > >> > > > > We will use whatever provided from header to calculate uCode size and > > RSA offset etc. If all of them are within the bin file range, we will > > load it. Otherwise, reject it to avoid SW memory access violation. This > > is already implemented. My point is: if we can't trust the CSS header, > > then the check such as (css_header.A >= css_header.B - css_header.C) > > itself is insignificant. Yes, header data might be corrupted. HW will > > verify it as part of RSA key authentication anyway. > > I guess I was late to the party and commented on an old version [1]. But > I'm with Dave here; you need to make sure nothing in kernel breaks with > the data you read from the firmware blob. > > It's not immediately obvious that you're safe when you write malicious > data to DMA_COPY_SIZE, or you write out of bounds of UOS_RSA_SCRATCH_0, > for example. You *must* make sure they are within sensible limits. > Agree. I should have not used the word *trust*. What I mean is presume the data in header is correct, then calculate size / offset of all firmware ingredients based on that. If any of them is out of bound, driver will reject it. In the new version I am working on, the check related to sizes includes: 1. The fw blob should be larger than sizeof(css_header) 2. The header size should match sizeof(css_header) 3. The RSA length (in dwords) should match UOS_RSA_SCRATCH_MAX_COUNT (64). This is missing in previous patch. The number 64 (2048 bits) is from BSpec. 4. The fw blob should have header, ucode and rsa key. Be note that I will use term *length* or *len* in header definition, which is in dwords unit. This is to avoid the confusion with *size*. Thanks, Alex [-- Attachment #1.2: Type: text/html, Size: 30727 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser 2015-09-25 16:31 ` Yu Dai @ 2015-09-28 8:13 ` Daniel Vetter 0 siblings, 0 replies; 35+ messages in thread From: Daniel Vetter @ 2015-09-28 8:13 UTC (permalink / raw) To: Yu Dai; +Cc: intel-gfx On Fri, Sep 25, 2015 at 09:31:52AM -0700, Yu Dai wrote: > Be note that I will use term *length* or *len* in header definition, which > is in dwords unit. This is to avoid the confusion with *size*. lenght is also generally in bytes. If you want to use separate variables to avoid confusion I'd suggest num_dwords (treat it like an array of dwords) or size_dw (_dw as the unit suffix, like we do with e.g. _usec). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 3/6] drm/i915/guc: Add host2guc notification for suspend and resume 2015-09-22 20:48 ` [PATCH v2 0/6] Several GuC related patches yu.dai 2015-09-22 20:48 ` [PATCH v2 1/6] drm/i915/guc: Fix a bug in GuC status check yu.dai 2015-09-22 20:48 ` [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser yu.dai @ 2015-09-22 20:48 ` yu.dai 2015-09-24 8:27 ` Kamble, Sagar A 2015-09-22 20:48 ` [PATCH v2 4/6] drm/i915/guc: Don't send flips to GuC yu.dai ` (2 subsequent siblings) 5 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-22 20:48 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> Add host2guc interfaces to nofigy GuC power state changes when enter or resume from power saving state. v1: Change to a more flexible way when fill host to GuC scratch data in order to remove hard coding. Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 1 + drivers/gpu/drm/i915/i915_gem.c | 1 + drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_guc.h | 2 ++ drivers/gpu/drm/i915/intel_guc_fwif.h | 8 +++++ drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++- 6 files changed, 65 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e2bf9e2..8f2a139 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -735,6 +735,7 @@ static int i915_drm_resume(struct drm_device *dev) DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n"); atomic_or(I915_WEDGED, &dev_priv->gpu_error.reset_counter); } + intel_guc_resume(dev); mutex_unlock(&dev->struct_mutex); intel_modeset_init_hw(dev); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 46f0e83..92dd4ff 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4458,6 +4458,7 @@ i915_gem_suspend(struct drm_device *dev) i915_gem_retire_requests(dev); i915_gem_stop_ringbuffers(dev); + intel_guc_suspend(dev); mutex_unlock(&dev->struct_mutex); cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 792d0b9..38b6ef4 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -914,3 +914,53 @@ void i915_guc_submission_fini(struct drm_device *dev) gem_release_guc_obj(guc->ctx_pool_obj); guc->ctx_pool_obj = NULL; } + +/** + * intel_guc_suspend() - notify GuC entering suspend state + * @dev: drm device + */ +int intel_guc_suspend(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_guc *guc = &dev_priv->guc; + struct intel_context *ctx; + u32 data[3]; + + if (!i915.enable_guc_submission) + return 0; + + ctx = dev_priv->ring[RCS].default_context; + + data[0] = HOST2GUC_ACTION_ENTER_S_STATE; + /* any value greater than GUC_POWER_D0 */ + data[1] = GUC_POWER_D1; + /* first page is shared data with GuC */ + data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state); + + return host2guc_action(guc, data, ARRAY_SIZE(data)); +} + + +/** + * intel_guc_resume() - notify GuC resuming from suspend state + * @dev: drm device + */ +int intel_guc_resume(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_guc *guc = &dev_priv->guc; + struct intel_context *ctx; + u32 data[3]; + + if (!i915.enable_guc_submission) + return 0; + + ctx = dev_priv->ring[RCS].default_context; + + data[0] = HOST2GUC_ACTION_EXIT_S_STATE; + data[1] = GUC_POWER_D0; + /* first page is shared data with GuC */ + data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state); + + return host2guc_action(guc, data, ARRAY_SIZE(data)); +} diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index e1389fc..e90c156 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -110,6 +110,8 @@ extern void intel_guc_ucode_init(struct drm_device *dev); extern int intel_guc_ucode_load(struct drm_device *dev); extern void intel_guc_ucode_fini(struct drm_device *dev); extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); +extern int intel_guc_suspend(struct drm_device *dev); +extern int intel_guc_resume(struct drm_device *dev); /* i915_guc_submission.c */ int i915_guc_submission_init(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 006dc0d..f6d0aa4 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -254,12 +254,20 @@ struct guc_context_desc { u64 desc_private; } __packed; +#define GUC_POWER_UNSPECIFIED 0 +#define GUC_POWER_D0 1 +#define GUC_POWER_D1 2 +#define GUC_POWER_D2 3 +#define GUC_POWER_D3 4 + /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */ enum host2guc_action { HOST2GUC_ACTION_DEFAULT = 0x0, HOST2GUC_ACTION_SAMPLE_FORCEWAKE = 0x6, HOST2GUC_ACTION_ALLOCATE_DOORBELL = 0x10, HOST2GUC_ACTION_DEALLOCATE_DOORBELL = 0x20, + HOST2GUC_ACTION_ENTER_S_STATE = 0x501, + HOST2GUC_ACTION_EXIT_S_STATE = 0x502, HOST2GUC_ACTION_SLPC_REQUEST = 0x3003, HOST2GUC_ACTION_LIMIT }; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index a6703b4..740bfb3 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -416,7 +416,6 @@ int intel_guc_ucode_load(struct drm_device *dev) intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); direct_interrupts_to_host(dev_priv); - i915_guc_submission_disable(dev); if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE) return 0; @@ -466,6 +465,9 @@ int intel_guc_ucode_load(struct drm_device *dev) intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); if (i915.enable_guc_submission) { + /* The execbuf_client will be recreated. Release it first. */ + i915_guc_submission_disable(dev); + err = i915_guc_submission_enable(dev); if (err) goto fail; -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/6] drm/i915/guc: Add host2guc notification for suspend and resume 2015-09-22 20:48 ` [PATCH v2 3/6] drm/i915/guc: Add host2guc notification for suspend and resume yu.dai @ 2015-09-24 8:27 ` Kamble, Sagar A 0 siblings, 0 replies; 35+ messages in thread From: Kamble, Sagar A @ 2015-09-24 8:27 UTC (permalink / raw) To: yu.dai, intel-gfx; +Cc: Hiremath, Shashidhar On 9/23/2015 2:18 AM, yu.dai@intel.com wrote: > From: Alex Dai <yu.dai@intel.com> > > Add host2guc interfaces to nofigy GuC power state changes when *notify > enter or resume from power saving state. > > v1: Change to a more flexible way when fill host to GuC scratch > data in order to remove hard coding. > > Signed-off-by: Alex Dai <yu.dai@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 1 + > drivers/gpu/drm/i915/i915_gem.c | 1 + > drivers/gpu/drm/i915/i915_guc_submission.c | 50 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_guc.h | 2 ++ > drivers/gpu/drm/i915/intel_guc_fwif.h | 8 +++++ > drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++- > 6 files changed, 65 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index e2bf9e2..8f2a139 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -735,6 +735,7 @@ static int i915_drm_resume(struct drm_device *dev) > DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n"); > atomic_or(I915_WEDGED, &dev_priv->gpu_error.reset_counter); > } > + intel_guc_resume(dev); Need to call from intel_runtime_resume as well. > mutex_unlock(&dev->struct_mutex); > > intel_modeset_init_hw(dev); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 46f0e83..92dd4ff 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4458,6 +4458,7 @@ i915_gem_suspend(struct drm_device *dev) > i915_gem_retire_requests(dev); > > i915_gem_stop_ringbuffers(dev); > + intel_guc_suspend(dev); Should this be called as part of i915_drm_suspend for consistency? Need to call from intel_runtime_suspend as well. > mutex_unlock(&dev->struct_mutex); > > cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 792d0b9..38b6ef4 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -914,3 +914,53 @@ void i915_guc_submission_fini(struct drm_device *dev) > gem_release_guc_obj(guc->ctx_pool_obj); > guc->ctx_pool_obj = NULL; > } > + > +/** > + * intel_guc_suspend() - notify GuC entering suspend state > + * @dev: drm device > + */ > +int intel_guc_suspend(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_guc *guc = &dev_priv->guc; > + struct intel_context *ctx; > + u32 data[3]; > + > + if (!i915.enable_guc_submission) > + return 0; > + > + ctx = dev_priv->ring[RCS].default_context; > + > + data[0] = HOST2GUC_ACTION_ENTER_S_STATE; > + /* any value greater than GUC_POWER_D0 */ > + data[1] = GUC_POWER_D1; > + /* first page is shared data with GuC */ > + data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state); > + > + return host2guc_action(guc, data, ARRAY_SIZE(data)); > +} > + > + > +/** > + * intel_guc_resume() - notify GuC resuming from suspend state > + * @dev: drm device > + */ > +int intel_guc_resume(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_guc *guc = &dev_priv->guc; > + struct intel_context *ctx; > + u32 data[3]; > + > + if (!i915.enable_guc_submission) > + return 0; > + > + ctx = dev_priv->ring[RCS].default_context; > + > + data[0] = HOST2GUC_ACTION_EXIT_S_STATE; > + data[1] = GUC_POWER_D0; > + /* first page is shared data with GuC */ > + data[2] = i915_gem_obj_ggtt_offset(ctx->engine[RCS].state); > + > + return host2guc_action(guc, data, ARRAY_SIZE(data)); > +} > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index e1389fc..e90c156 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -110,6 +110,8 @@ extern void intel_guc_ucode_init(struct drm_device *dev); > extern int intel_guc_ucode_load(struct drm_device *dev); > extern void intel_guc_ucode_fini(struct drm_device *dev); > extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); > +extern int intel_guc_suspend(struct drm_device *dev); > +extern int intel_guc_resume(struct drm_device *dev); > > /* i915_guc_submission.c */ > int i915_guc_submission_init(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > index 006dc0d..f6d0aa4 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -254,12 +254,20 @@ struct guc_context_desc { > u64 desc_private; > } __packed; > > +#define GUC_POWER_UNSPECIFIED 0 > +#define GUC_POWER_D0 1 > +#define GUC_POWER_D1 2 > +#define GUC_POWER_D2 3 > +#define GUC_POWER_D3 4 > + > /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */ > enum host2guc_action { > HOST2GUC_ACTION_DEFAULT = 0x0, > HOST2GUC_ACTION_SAMPLE_FORCEWAKE = 0x6, > HOST2GUC_ACTION_ALLOCATE_DOORBELL = 0x10, > HOST2GUC_ACTION_DEALLOCATE_DOORBELL = 0x20, > + HOST2GUC_ACTION_ENTER_S_STATE = 0x501, > + HOST2GUC_ACTION_EXIT_S_STATE = 0x502, > HOST2GUC_ACTION_SLPC_REQUEST = 0x3003, > HOST2GUC_ACTION_LIMIT > }; > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index a6703b4..740bfb3 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -416,7 +416,6 @@ int intel_guc_ucode_load(struct drm_device *dev) > intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); > > direct_interrupts_to_host(dev_priv); > - i915_guc_submission_disable(dev); > > if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE) > return 0; > @@ -466,6 +465,9 @@ int intel_guc_ucode_load(struct drm_device *dev) > intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); > > if (i915.enable_guc_submission) { > + /* The execbuf_client will be recreated. Release it first. */ > + i915_guc_submission_disable(dev); > + > err = i915_guc_submission_enable(dev); > if (err) > goto fail; _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 4/6] drm/i915/guc: Don't send flips to GuC 2015-09-22 20:48 ` [PATCH v2 0/6] Several GuC related patches yu.dai ` (2 preceding siblings ...) 2015-09-22 20:48 ` [PATCH v2 3/6] drm/i915/guc: Add host2guc notification for suspend and resume yu.dai @ 2015-09-22 20:48 ` yu.dai 2015-09-22 23:11 ` [PATCH] " yu.dai 2015-09-22 20:48 ` [PATCH v2 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state yu.dai 2015-09-22 20:48 ` [PATCH v2 6/6] drm/i915/guc: Enable GuC submission, where supported yu.dai 5 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-22 20:48 UTC (permalink / raw) To: intel-gfx From: Sagar Arun Kamble <sagar.a.kamble@intel.com> Due to flip interrupts GuC stays awake always and GT does not enter RC6. Do not route those interrupts to GuC for now. Driver won't touch DE_GUCRMR register and leave it as what default value. Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 740bfb3..d513673 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -90,9 +90,6 @@ static void direct_interrupts_to_host(struct drm_i915_private *dev_priv) for_each_ring(ring, dev_priv, i) I915_WRITE(RING_MODE_GEN7(ring), irqs); - /* tell DE to send nothing to GuC */ - I915_WRITE(DE_GUCRMR, ~0); - /* route all GT interrupts to the host */ I915_WRITE(GUC_BCS_RCS_IER, 0); I915_WRITE(GUC_VCS2_VCS1_IER, 0); @@ -110,13 +107,6 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) for_each_ring(ring, dev_priv, i) I915_WRITE(RING_MODE_GEN7(ring), irqs); - /* tell DE to send (all) flip_done to GuC */ - irqs = DERRMR_PIPEA_PRI_FLIP_DONE | DERRMR_PIPEA_SPR_FLIP_DONE | - DERRMR_PIPEB_PRI_FLIP_DONE | DERRMR_PIPEB_SPR_FLIP_DONE | - DERRMR_PIPEC_PRI_FLIP_DONE | DERRMR_PIPEC_SPR_FLIP_DONE; - /* Unmasked bits will cause GuC response message to be sent */ - I915_WRITE(DE_GUCRMR, ~irqs); - /* route USER_INTERRUPT to Host, all others are sent to GuC. */ irqs = GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT | GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT; -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH] drm/i915/guc: Don't send flips to GuC 2015-09-22 20:48 ` [PATCH v2 4/6] drm/i915/guc: Don't send flips to GuC yu.dai @ 2015-09-22 23:11 ` yu.dai 2015-09-23 0:56 ` O'Rourke, Tom 0 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-22 23:11 UTC (permalink / raw) To: intel-gfx From: Sagar Arun Kamble <sagar.a.kamble@intel.com> Due to flip interrupts GuC stays awake always and GT does not enter RC6. Do not route those interrupts to GuC for now. Driver won't touch DE_GUCRMR register and leave it as what default value. Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 740bfb3..d513673 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -90,9 +90,6 @@ static void direct_interrupts_to_host(struct drm_i915_private *dev_priv) for_each_ring(ring, dev_priv, i) I915_WRITE(RING_MODE_GEN7(ring), irqs); - /* tell DE to send nothing to GuC */ - I915_WRITE(DE_GUCRMR, ~0); - /* route all GT interrupts to the host */ I915_WRITE(GUC_BCS_RCS_IER, 0); I915_WRITE(GUC_VCS2_VCS1_IER, 0); @@ -110,13 +107,6 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) for_each_ring(ring, dev_priv, i) I915_WRITE(RING_MODE_GEN7(ring), irqs); - /* tell DE to send (all) flip_done to GuC */ - irqs = DERRMR_PIPEA_PRI_FLIP_DONE | DERRMR_PIPEA_SPR_FLIP_DONE | - DERRMR_PIPEB_PRI_FLIP_DONE | DERRMR_PIPEB_SPR_FLIP_DONE | - DERRMR_PIPEC_PRI_FLIP_DONE | DERRMR_PIPEC_SPR_FLIP_DONE; - /* Unmasked bits will cause GuC response message to be sent */ - I915_WRITE(DE_GUCRMR, ~irqs); - /* route USER_INTERRUPT to Host, all others are sent to GuC. */ irqs = GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT | GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT; -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] drm/i915/guc: Don't send flips to GuC 2015-09-22 23:11 ` [PATCH] " yu.dai @ 2015-09-23 0:56 ` O'Rourke, Tom 0 siblings, 0 replies; 35+ messages in thread From: O'Rourke, Tom @ 2015-09-23 0:56 UTC (permalink / raw) To: yu.dai; +Cc: intel-gfx On Tue, Sep 22, 2015 at 04:11:48PM -0700, yu.dai@intel.com wrote: > From: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > Due to flip interrupts GuC stays awake always and GT does not enter > RC6. Do not route those interrupts to GuC for now. Driver won't touch > DE_GUCRMR register and leave it as what default value. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Signed-off-by: Alex Dai <yu.dai@intel.com> Looks good to me. Reviewed-by: Tom O'Rourke <Tom.O'Rourke@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_loader.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 740bfb3..d513673 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -90,9 +90,6 @@ static void direct_interrupts_to_host(struct drm_i915_private *dev_priv) > for_each_ring(ring, dev_priv, i) > I915_WRITE(RING_MODE_GEN7(ring), irqs); > > - /* tell DE to send nothing to GuC */ > - I915_WRITE(DE_GUCRMR, ~0); > - > /* route all GT interrupts to the host */ > I915_WRITE(GUC_BCS_RCS_IER, 0); > I915_WRITE(GUC_VCS2_VCS1_IER, 0); > @@ -110,13 +107,6 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv) > for_each_ring(ring, dev_priv, i) > I915_WRITE(RING_MODE_GEN7(ring), irqs); > > - /* tell DE to send (all) flip_done to GuC */ > - irqs = DERRMR_PIPEA_PRI_FLIP_DONE | DERRMR_PIPEA_SPR_FLIP_DONE | > - DERRMR_PIPEB_PRI_FLIP_DONE | DERRMR_PIPEB_SPR_FLIP_DONE | > - DERRMR_PIPEC_PRI_FLIP_DONE | DERRMR_PIPEC_SPR_FLIP_DONE; > - /* Unmasked bits will cause GuC response message to be sent */ > - I915_WRITE(DE_GUCRMR, ~irqs); > - > /* route USER_INTERRUPT to Host, all others are sent to GuC. */ > irqs = GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT | > GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state 2015-09-22 20:48 ` [PATCH v2 0/6] Several GuC related patches yu.dai ` (3 preceding siblings ...) 2015-09-22 20:48 ` [PATCH v2 4/6] drm/i915/guc: Don't send flips to GuC yu.dai @ 2015-09-22 20:48 ` yu.dai 2015-09-23 0:59 ` O'Rourke, Tom 2015-09-22 20:48 ` [PATCH v2 6/6] drm/i915/guc: Enable GuC submission, where supported yu.dai 5 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-22 20:48 UTC (permalink / raw) To: intel-gfx From: Alex Dai <yu.dai@intel.com> GuC expects two bits for Render and Media domain separately when driver sends data via host2guc SAMPLE_FORCEWAKE. Bit 0 is for Render and bit 1 is for Media domain. v1: Add parameters definition to avoid magic value Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++-- drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 38b6ef4..7dbc108 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -158,9 +158,11 @@ static int host2guc_sample_forcewake(struct intel_guc *guc, u32 data[2]; data[0] = HOST2GUC_ACTION_SAMPLE_FORCEWAKE; - data[1] = (intel_enable_rc6(dev_priv->dev)) ? 1 : 0; + /* bit 0 and 1 are for Render and Media domain separately */ + data[1] = intel_enable_rc6(dev_priv->dev) ? + GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA : 0; - return host2guc_action(guc, data, 2); + return host2guc_action(guc, data, ARRAY_SIZE(data)); } /* diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index f6d0aa4..ecea053 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -260,6 +260,9 @@ struct guc_context_desc { #define GUC_POWER_D2 3 #define GUC_POWER_D3 4 +#define GUC_FORCEWAKE_RENDER (1 << 0) +#define GUC_FORCEWAKE_MEDIA (1 << 1) + /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */ enum host2guc_action { HOST2GUC_ACTION_DEFAULT = 0x0, -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state 2015-09-22 20:48 ` [PATCH v2 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state yu.dai @ 2015-09-23 0:59 ` O'Rourke, Tom 2015-09-23 8:37 ` Daniel Vetter 0 siblings, 1 reply; 35+ messages in thread From: O'Rourke, Tom @ 2015-09-23 0:59 UTC (permalink / raw) To: yu.dai; +Cc: intel-gfx On Tue, Sep 22, 2015 at 01:48:44PM -0700, yu.dai@intel.com wrote: > From: Alex Dai <yu.dai@intel.com> > > GuC expects two bits for Render and Media domain separately when > driver sends data via host2guc SAMPLE_FORCEWAKE. Bit 0 is for > Render and bit 1 is for Media domain. > > v1: Add parameters definition to avoid magic value > > Signed-off-by: Alex Dai <yu.dai@intel.com> Reviewed-by: Tom O'Rourke <Tom.O'Rourke@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++-- > drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 38b6ef4..7dbc108 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -158,9 +158,11 @@ static int host2guc_sample_forcewake(struct intel_guc *guc, > u32 data[2]; > > data[0] = HOST2GUC_ACTION_SAMPLE_FORCEWAKE; > - data[1] = (intel_enable_rc6(dev_priv->dev)) ? 1 : 0; > + /* bit 0 and 1 are for Render and Media domain separately */ > + data[1] = intel_enable_rc6(dev_priv->dev) ? > + GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA : 0; > > - return host2guc_action(guc, data, 2); > + return host2guc_action(guc, data, ARRAY_SIZE(data)); > } > > /* > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > index f6d0aa4..ecea053 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -260,6 +260,9 @@ struct guc_context_desc { > #define GUC_POWER_D2 3 > #define GUC_POWER_D3 4 > > +#define GUC_FORCEWAKE_RENDER (1 << 0) > +#define GUC_FORCEWAKE_MEDIA (1 << 1) > + > /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */ > enum host2guc_action { > HOST2GUC_ACTION_DEFAULT = 0x0, > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state 2015-09-23 0:59 ` O'Rourke, Tom @ 2015-09-23 8:37 ` Daniel Vetter 0 siblings, 0 replies; 35+ messages in thread From: Daniel Vetter @ 2015-09-23 8:37 UTC (permalink / raw) To: O'Rourke, Tom; +Cc: intel-gfx On Tue, Sep 22, 2015 at 05:59:49PM -0700, O'Rourke, Tom wrote: > On Tue, Sep 22, 2015 at 01:48:44PM -0700, yu.dai@intel.com wrote: > > From: Alex Dai <yu.dai@intel.com> > > > > GuC expects two bits for Render and Media domain separately when > > driver sends data via host2guc SAMPLE_FORCEWAKE. Bit 0 is for > > Render and bit 1 is for Media domain. > > > > v1: Add parameters definition to avoid magic value > > > > Signed-off-by: Alex Dai <yu.dai@intel.com> > > Reviewed-by: Tom O'Rourke <Tom.O'Rourke@intel.com> I'm lacking r-b for all but 2 patches in this series ... is that still ongoing? -Daniel > > > --- > > drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++-- > > drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > > index 38b6ef4..7dbc108 100644 > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > > @@ -158,9 +158,11 @@ static int host2guc_sample_forcewake(struct intel_guc *guc, > > u32 data[2]; > > > > data[0] = HOST2GUC_ACTION_SAMPLE_FORCEWAKE; > > - data[1] = (intel_enable_rc6(dev_priv->dev)) ? 1 : 0; > > + /* bit 0 and 1 are for Render and Media domain separately */ > > + data[1] = intel_enable_rc6(dev_priv->dev) ? > > + GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA : 0; > > > > - return host2guc_action(guc, data, 2); > > + return host2guc_action(guc, data, ARRAY_SIZE(data)); > > } > > > > /* > > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > > index f6d0aa4..ecea053 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > > @@ -260,6 +260,9 @@ struct guc_context_desc { > > #define GUC_POWER_D2 3 > > #define GUC_POWER_D3 4 > > > > +#define GUC_FORCEWAKE_RENDER (1 << 0) > > +#define GUC_FORCEWAKE_MEDIA (1 << 1) > > + > > /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */ > > enum host2guc_action { > > HOST2GUC_ACTION_DEFAULT = 0x0, > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 6/6] drm/i915/guc: Enable GuC submission, where supported 2015-09-22 20:48 ` [PATCH v2 0/6] Several GuC related patches yu.dai ` (4 preceding siblings ...) 2015-09-22 20:48 ` [PATCH v2 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state yu.dai @ 2015-09-22 20:48 ` yu.dai 2015-09-22 23:13 ` [PATCH] " yu.dai 5 siblings, 1 reply; 35+ messages in thread From: yu.dai @ 2015-09-22 20:48 UTC (permalink / raw) To: intel-gfx From: Dave Gordon <david.s.gordon@intel.com> This is to enable command submission via GuC. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_params.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index ca060d5..4586eac 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -52,7 +52,7 @@ struct i915_params i915 __read_mostly = { .verbose_state_checks = 1, .nuclear_pageflip = 0, .edp_vswing = 0, - .enable_guc_submission = false, + .enable_guc_submission = true, .guc_log_level = -1, }; @@ -185,7 +185,7 @@ MODULE_PARM_DESC(edp_vswing, "2=default swing(400mV))"); module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400); -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)"); +MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:true)"); module_param_named(guc_log_level, i915.guc_log_level, int, 0400); MODULE_PARM_DESC(guc_log_level, -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH] drm/i915/guc: Enable GuC submission, where supported 2015-09-22 20:48 ` [PATCH v2 6/6] drm/i915/guc: Enable GuC submission, where supported yu.dai @ 2015-09-22 23:13 ` yu.dai 0 siblings, 0 replies; 35+ messages in thread From: yu.dai @ 2015-09-22 23:13 UTC (permalink / raw) To: intel-gfx From: Dave Gordon <david.s.gordon@intel.com> This is to enable command submission via GuC. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Signed-off-by: Alex Dai <yu.dai@intel.com> --- drivers/gpu/drm/i915/i915_params.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index ca060d5..4586eac 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -52,7 +52,7 @@ struct i915_params i915 __read_mostly = { .verbose_state_checks = 1, .nuclear_pageflip = 0, .edp_vswing = 0, - .enable_guc_submission = false, + .enable_guc_submission = true, .guc_log_level = -1, }; @@ -185,7 +185,7 @@ MODULE_PARM_DESC(edp_vswing, "2=default swing(400mV))"); module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400); -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)"); +MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:true)"); module_param_named(guc_log_level, i915.guc_log_level, int, 0400); MODULE_PARM_DESC(guc_log_level, -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 35+ messages in thread
end of thread, other threads:[~2015-09-28 8:10 UTC | newest] Thread overview: 35+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-10 23:56 [PATCH 0/6] Several GuC related patches yu.dai 2015-09-10 23:56 ` [PATCH 1/6] drm/i915/guc: Fix a bug in GuC status check yu.dai 2015-09-10 23:56 ` [PATCH 2/6] drm/i915/guc: Add GuC css header parser yu.dai 2015-09-14 9:28 ` Daniel Vetter 2015-09-10 23:56 ` [PATCH 3/6] drm/i915/guc: Add host2guc notification for suspend and resume yu.dai 2015-09-15 23:30 ` [PATCH 03/15] " yu.dai 2015-09-10 23:56 ` [PATCH 4/6] drm/i915/guc: Don't send flips to GuC yu.dai 2015-09-11 12:44 ` [PATCH v2 1/1] drm/i915: Direct all DE interrupts to host Sagar Arun Kamble 2015-09-21 19:02 ` O'Rourke, Tom 2015-09-10 23:56 ` [PATCH 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state yu.dai 2015-09-14 9:32 ` Daniel Vetter 2015-09-15 23:31 ` [PATCH 05/15] " yu.dai 2015-09-10 23:56 ` [PATCH 6/6] drm/i915/guc: Enable GuC submission, where supported yu.dai 2015-09-22 20:48 ` [PATCH v2 0/6] Several GuC related patches yu.dai 2015-09-22 20:48 ` [PATCH v2 1/6] drm/i915/guc: Fix a bug in GuC status check yu.dai 2015-09-24 4:59 ` Kamble, Sagar A 2015-09-28 8:10 ` Daniel Vetter 2015-09-22 20:48 ` [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser yu.dai 2015-09-24 14:23 ` Dave Gordon 2015-09-24 18:34 ` Yu Dai 2015-09-24 19:04 ` Dave Gordon 2015-09-24 20:23 ` Yu Dai 2015-09-25 14:45 ` Jani Nikula 2015-09-25 16:31 ` Yu Dai 2015-09-28 8:13 ` Daniel Vetter 2015-09-22 20:48 ` [PATCH v2 3/6] drm/i915/guc: Add host2guc notification for suspend and resume yu.dai 2015-09-24 8:27 ` Kamble, Sagar A 2015-09-22 20:48 ` [PATCH v2 4/6] drm/i915/guc: Don't send flips to GuC yu.dai 2015-09-22 23:11 ` [PATCH] " yu.dai 2015-09-23 0:56 ` O'Rourke, Tom 2015-09-22 20:48 ` [PATCH v2 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6 state yu.dai 2015-09-23 0:59 ` O'Rourke, Tom 2015-09-23 8:37 ` Daniel Vetter 2015-09-22 20:48 ` [PATCH v2 6/6] drm/i915/guc: Enable GuC submission, where supported yu.dai 2015-09-22 23:13 ` [PATCH] " yu.dai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).