| Message ID | 55F1FC83.80807@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
That looks like it would, but I think it's still confusing to reference LRC state when we haven't initialized execlists at all... Jesse On 09/10/2015 02:56 PM, Yu Dai wrote: > Jesse, > > Will the patch here fix the issue? It should help other cases where LRC_PPHWSP_PN is referenced on non-execlist / guc platforms. > > diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h > index 4cc54b3..233a930 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.h > +++ b/drivers/gpu/drm/i915/intel_lrc.h > @@ -71,7 +71,7 @@ static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf, > > /* One extra page is added before LRC for GuC as shared data */ > #define LRC_GUCSHR_PN (0) > -#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + 1) > +#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + i915.enable_guc_submission ? 1 : 0) > #define LRC_STATE_PN (LRC_PPHWSP_PN + 1) > > void intel_lr_context_free(struct intel_context *ctx); > > Thanks, > Alex > > On 09/10/2015 02:40 PM, Jesse Barnes wrote: >> Looks like this was introduced in: >> commit d1675198ed1f21aec6e036336e4340c40b726497 >> Author: Alex Dai <yu.dai@intel.com> >> Date: Wed Aug 12 15:43:43 2015 +0100 >> >> drm/i915: Integrate GuC-based command submission >> >> This patch assumed LRC contexts and HWS layout, which is incorrect on >> platforms without execlists. This can lead to a crash in GPU error >> state readout on those platforms. >> >> I don't see a bug filed for this, but there may be one that I haven't >> found. >> >> Cc: Alex Dai <yu.dai@intel.com> >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> --- >> drivers/gpu/drm/i915/i915_gpu_error.c | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c >> index 3379f9c..d0822f8 100644 >> --- a/drivers/gpu/drm/i915/i915_gpu_error.c >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c >> @@ -457,17 +457,24 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m, >> } >> if ((obj = error->ring[i].hws_page)) { >> + u64 hws_offset = lower_32_bits(obj->gtt_offset); >> + u32 *hws_page = &obj->pages[0][0]; >> + >> + if (i915.enable_execlists) { >> + hws_offset = obj->gtt_offset + LRC_PPHWSP_PN * >> + PAGE_SIZE; >> + hws_page = &obj->pages[LRC_PPHWSP_PN][0]; >> + } >> err_printf(m, "%s --- HW Status = 0x%08llx\n", >> - dev_priv->ring[i].name, >> - obj->gtt_offset + LRC_PPHWSP_PN * PAGE_SIZE); >> + dev_priv->ring[i].name, hws_offset); >> offset = 0; >> for (elt = 0; elt < PAGE_SIZE/16; elt += 4) { >> err_printf(m, "[%04x] %08x %08x %08x %08x\n", >> offset, >> - obj->pages[LRC_PPHWSP_PN][elt], >> - obj->pages[LRC_PPHWSP_PN][elt+1], >> - obj->pages[LRC_PPHWSP_PN][elt+2], >> - obj->pages[LRC_PPHWSP_PN][elt+3]); >> + hws_page[elt], >> + hws_page[elt+1], >> + hws_page[elt+2], >> + hws_page[elt+3]); >> offset += 16; >> } >> } > >
Agree. The LRC prefix is confusing. Thanks for the patch. -Alex On 09/10/2015 02:58 PM, Jesse Barnes wrote: > That looks like it would, but I think it's still confusing to reference LRC state when we haven't initialized execlists at all... > > Jesse > > On 09/10/2015 02:56 PM, Yu Dai wrote: > > Jesse, > > > > Will the patch here fix the issue? It should help other cases where LRC_PPHWSP_PN is referenced on non-execlist / guc platforms. > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h > > index 4cc54b3..233a930 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.h > > +++ b/drivers/gpu/drm/i915/intel_lrc.h > > @@ -71,7 +71,7 @@ static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf, > > > > /* One extra page is added before LRC for GuC as shared data */ > > #define LRC_GUCSHR_PN (0) > > -#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + 1) > > +#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + i915.enable_guc_submission ? 1 : 0) > > #define LRC_STATE_PN (LRC_PPHWSP_PN + 1) > > > > void intel_lr_context_free(struct intel_context *ctx); > > > > Thanks, > > Alex > > > > On 09/10/2015 02:40 PM, Jesse Barnes wrote: > >> Looks like this was introduced in: > >> commit d1675198ed1f21aec6e036336e4340c40b726497 > >> Author: Alex Dai <yu.dai@intel.com> > >> Date: Wed Aug 12 15:43:43 2015 +0100 > >> > >> drm/i915: Integrate GuC-based command submission > >> > >> This patch assumed LRC contexts and HWS layout, which is incorrect on > >> platforms without execlists. This can lead to a crash in GPU error > >> state readout on those platforms. > >> > >> I don't see a bug filed for this, but there may be one that I haven't > >> found. > >> > >> Cc: Alex Dai <yu.dai@intel.com> > >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > >> --- > >> drivers/gpu/drm/i915/i915_gpu_error.c | 19 +++++++++++++------ > >> 1 file changed, 13 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > >> index 3379f9c..d0822f8 100644 > >> --- a/drivers/gpu/drm/i915/i915_gpu_error.c > >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > >> @@ -457,17 +457,24 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m, > >> } > >> if ((obj = error->ring[i].hws_page)) { > >> + u64 hws_offset = lower_32_bits(obj->gtt_offset); > >> + u32 *hws_page = &obj->pages[0][0]; > >> + > >> + if (i915.enable_execlists) { > >> + hws_offset = obj->gtt_offset + LRC_PPHWSP_PN * > >> + PAGE_SIZE; > >> + hws_page = &obj->pages[LRC_PPHWSP_PN][0]; > >> + } > >> err_printf(m, "%s --- HW Status = 0x%08llx\n", > >> - dev_priv->ring[i].name, > >> - obj->gtt_offset + LRC_PPHWSP_PN * PAGE_SIZE); > >> + dev_priv->ring[i].name, hws_offset); > >> offset = 0; > >> for (elt = 0; elt < PAGE_SIZE/16; elt += 4) { > >> err_printf(m, "[%04x] %08x %08x %08x %08x\n", > >> offset, > >> - obj->pages[LRC_PPHWSP_PN][elt], > >> - obj->pages[LRC_PPHWSP_PN][elt+1], > >> - obj->pages[LRC_PPHWSP_PN][elt+2], > >> - obj->pages[LRC_PPHWSP_PN][elt+3]); > >> + hws_page[elt], > >> + hws_page[elt+1], > >> + hws_page[elt+2], > >> + hws_page[elt+3]); > >> offset += 16; > >> } > >> } > > > > >
On Thu, Sep 10, 2015 at 03:07:00PM -0700, Yu Dai wrote: > Agree. The LRC prefix is confusing. Thanks for the patch. -Alex Care to do an official r-b? Thanks, Daniel > > On 09/10/2015 02:58 PM, Jesse Barnes wrote: > >That looks like it would, but I think it's still confusing to reference LRC state when we haven't initialized execlists at all... > > > >Jesse > > > >On 09/10/2015 02:56 PM, Yu Dai wrote: > >> Jesse, > >> > >> Will the patch here fix the issue? It should help other cases where LRC_PPHWSP_PN is referenced on non-execlist / guc platforms. > >> > >> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h > >> index 4cc54b3..233a930 100644 > >> --- a/drivers/gpu/drm/i915/intel_lrc.h > >> +++ b/drivers/gpu/drm/i915/intel_lrc.h > >> @@ -71,7 +71,7 @@ static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf, > >> > >> /* One extra page is added before LRC for GuC as shared data */ > >> #define LRC_GUCSHR_PN (0) > >> -#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + 1) > >> +#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + i915.enable_guc_submission ? 1 : 0) > >> #define LRC_STATE_PN (LRC_PPHWSP_PN + 1) > >> > >> void intel_lr_context_free(struct intel_context *ctx); > >> > >> Thanks, > >> Alex > >> > >> On 09/10/2015 02:40 PM, Jesse Barnes wrote: > >>> Looks like this was introduced in: > >>> commit d1675198ed1f21aec6e036336e4340c40b726497 > >>> Author: Alex Dai <yu.dai@intel.com> > >>> Date: Wed Aug 12 15:43:43 2015 +0100 > >>> > >>> drm/i915: Integrate GuC-based command submission > >>> > >>> This patch assumed LRC contexts and HWS layout, which is incorrect on > >>> platforms without execlists. This can lead to a crash in GPU error > >>> state readout on those platforms. > >>> > >>> I don't see a bug filed for this, but there may be one that I haven't > >>> found. > >>> > >>> Cc: Alex Dai <yu.dai@intel.com> > >>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > >>> --- > >>> drivers/gpu/drm/i915/i915_gpu_error.c | 19 +++++++++++++------ > >>> 1 file changed, 13 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > >>> index 3379f9c..d0822f8 100644 > >>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c > >>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > >>> @@ -457,17 +457,24 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m, > >>> } > >>> if ((obj = error->ring[i].hws_page)) { > >>> + u64 hws_offset = lower_32_bits(obj->gtt_offset); > >>> + u32 *hws_page = &obj->pages[0][0]; > >>> + > >>> + if (i915.enable_execlists) { > >>> + hws_offset = obj->gtt_offset + LRC_PPHWSP_PN * > >>> + PAGE_SIZE; > >>> + hws_page = &obj->pages[LRC_PPHWSP_PN][0]; > >>> + } > >>> err_printf(m, "%s --- HW Status = 0x%08llx\n", > >>> - dev_priv->ring[i].name, > >>> - obj->gtt_offset + LRC_PPHWSP_PN * PAGE_SIZE); > >>> + dev_priv->ring[i].name, hws_offset); > >>> offset = 0; > >>> for (elt = 0; elt < PAGE_SIZE/16; elt += 4) { > >>> err_printf(m, "[%04x] %08x %08x %08x %08x\n", > >>> offset, > >>> - obj->pages[LRC_PPHWSP_PN][elt], > >>> - obj->pages[LRC_PPHWSP_PN][elt+1], > >>> - obj->pages[LRC_PPHWSP_PN][elt+2], > >>> - obj->pages[LRC_PPHWSP_PN][elt+3]); > >>> + hws_page[elt], > >>> + hws_page[elt+1], > >>> + hws_page[elt+2], > >>> + hws_page[elt+3]); > >>> offset += 16; > >>> } > >>> } > >> > >> > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 14/09/15 10:21, Daniel Vetter wrote: > On Thu, Sep 10, 2015 at 03:07:00PM -0700, Yu Dai wrote: >> Agree. The LRC prefix is confusing. Thanks for the patch. -Alex > > Care to do an official r-b? > > Thanks, Daniel > >> >> On 09/10/2015 02:58 PM, Jesse Barnes wrote: >>> That looks like it would, but I think it's still confusing to reference LRC state when we haven't initialized execlists at all... >>> >>> Jesse >>> >>> On 09/10/2015 02:56 PM, Yu Dai wrote: >>>> Jesse, >>>> >>>> Will the patch here fix the issue? It should help other cases where LRC_PPHWSP_PN is referenced on non-execlist / guc platforms. >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h >>>> index 4cc54b3..233a930 100644 >>>> --- a/drivers/gpu/drm/i915/intel_lrc.h >>>> +++ b/drivers/gpu/drm/i915/intel_lrc.h >>>> @@ -71,7 +71,7 @@ static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf, >>>> >>>> /* One extra page is added before LRC for GuC as shared data */ >>>> #define LRC_GUCSHR_PN (0) >>>> -#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + 1) >>>> +#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + i915.enable_guc_submission ? 1 : 0) >>>> #define LRC_STATE_PN (LRC_PPHWSP_PN + 1) I don't like this approach of hiding the runtime conditional offset in the macro. I think it would be better to leave this alone and use the technique in the code below where the (const) macros named 'LRC_*' are used only inside clauses guarded by an "if (i915.enable_execlists)". That way the code won't mention the LRC_* symbols except where they're known tp be meaningful. Of these symbols, LRC_GUCSHR_PN is not used except in GuC-specific code, and LRC_STATE_PN is used only in GuC and/or LRC (execlist-mode) specific code. So it's only LRC_PPHWSP_PN that needs to be used selectively (specifically, added to the base of a context object when using execlist mode). And the code below already accomplishes that without changing the macro. >>>> void intel_lr_context_free(struct intel_context *ctx); >>>> >>>> Thanks, >>>> Alex >>>> >>>> On 09/10/2015 02:40 PM, Jesse Barnes wrote: >>>>> Looks like this was introduced in: >>>>> commit d1675198ed1f21aec6e036336e4340c40b726497 >>>>> Author: Alex Dai <yu.dai@intel.com> >>>>> Date: Wed Aug 12 15:43:43 2015 +0100 >>>>> >>>>> drm/i915: Integrate GuC-based command submission >>>>> >>>>> This patch assumed LRC contexts and HWS layout, which is incorrect on >>>>> platforms without execlists. This can lead to a crash in GPU error >>>>> state readout on those platforms. >>>>> >>>>> I don't see a bug filed for this, but there may be one that I haven't >>>>> found. >>>>> >>>>> Cc: Alex Dai <yu.dai@intel.com> >>>>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >>>>> --- >>>>> drivers/gpu/drm/i915/i915_gpu_error.c | 19 +++++++++++++------ >>>>> 1 file changed, 13 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c >>>>> index 3379f9c..d0822f8 100644 >>>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c >>>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c >>>>> @@ -457,17 +457,24 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m, >>>>> } >>>>> if ((obj = error->ring[i].hws_page)) { >>>>> + u64 hws_offset = lower_32_bits(obj->gtt_offset); No need to take only lower 32 bits here, the variable is already u64 and the format below is 0x%08llx, so we'll get a 32-bit value in the output (unless it actually is bigger than 32 bits, which shouldn't happen and which we'd want to know about!). >>>>> + u32 *hws_page = &obj->pages[0][0]; >>>>> + >>>>> + if (i915.enable_execlists) { >>>>> + hws_offset = obj->gtt_offset + LRC_PPHWSP_PN * >>>>> + PAGE_SIZE; If we keep the full 64-bit value (or even if we don't), this should simplify to "hws_offset += LRC_PPHWSP_PN*PAGE_SIZE;" >>>>> + hws_page = &obj->pages[LRC_PPHWSP_PN][0]; >>>>> + } >>>>> err_printf(m, "%s --- HW Status = 0x%08llx\n", >>>>> - dev_priv->ring[i].name, >>>>> - obj->gtt_offset + LRC_PPHWSP_PN * PAGE_SIZE); >>>>> + dev_priv->ring[i].name, hws_offset); >>>>> offset = 0; >>>>> for (elt = 0; elt < PAGE_SIZE/16; elt += 4) { >>>>> err_printf(m, "[%04x] %08x %08x %08x %08x\n", >>>>> offset, >>>>> - obj->pages[LRC_PPHWSP_PN][elt], >>>>> - obj->pages[LRC_PPHWSP_PN][elt+1], >>>>> - obj->pages[LRC_PPHWSP_PN][elt+2], >>>>> - obj->pages[LRC_PPHWSP_PN][elt+3]); >>>>> + hws_page[elt], >>>>> + hws_page[elt+1], >>>>> + hws_page[elt+2], >>>>> + hws_page[elt+3]); >>>>> offset += 16; >>>>> } >>>>> } With the 64-bit handling changed, I'd be happy to r-b Jesse's original patch, but not Alex's redefinition of LRC_PPHWSP_PN. Cheers, .Dave.
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index 4cc54b3..233a930 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -71,7 +71,7 @@ static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf, /* One extra page is added before LRC for GuC as shared data */ #define LRC_GUCSHR_PN (0) -#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + 1) +#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + i915.enable_guc_submission ? 1 : 0) #define LRC_STATE_PN (LRC_PPHWSP_PN + 1) void intel_lr_context_free(struct intel_context *ctx);