Re: [PATCH] drm/i915: fix crash in error state readout on non-execlist platforms

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
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