| Message ID | 1450374597-7021-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote: > It is unclear if this is even required on BXT. I'm not sure either, I only added it on the premise that it was marked as SKL+ originally in BSpec. The revision log entry in BSpec has this much to say: """ The workaround that requires an empty PIPE_CONTROL before a PIPE_CONTROL with a VF Cache Invalidation Enable is only for SKL and not SKL+. The bug was fixed in CNL with the following HSD:... """ Which doesn't make this clear either imo. > v2: Make sure to set the default value to false. Uncertain how my compiler > doesn't complain with v1. > > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 519cea32..af1e001 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1695,7 +1695,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request, > struct intel_ringbuffer *ringbuf = request->ringbuf; > struct intel_engine_cs *ring = ringbuf->ring; > u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES; > - bool vf_flush_wa; > + bool vf_flush_wa = false; > u32 flags = 0; > int ret; > > @@ -1716,14 +1716,14 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request, > flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE; > flags |= PIPE_CONTROL_QW_WRITE; > flags |= PIPE_CONTROL_GLOBAL_GTT_IVB; > - } > > - /* > - * On GEN9+ Before VF_CACHE_INVALIDATE we need to emit a NULL pipe > - * control. > - */ > - vf_flush_wa = INTEL_INFO(ring->dev)->gen >= 9 && > - flags & PIPE_CONTROL_VF_CACHE_INVALIDATE; > + /* > + * On GEN9: before VF_CACHE_INVALIDATE we need to emit a NULL > + * pipe control. > + */ > +> if (IS_GEN9(ring->dev)) Nitpick: INTEL_INFO()->gen == 9 is the preferred way. Either way: Reviewed-by: Imre Deak <imre.deak@intel.com> > + vf_flush_wa = true; > + } > > ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6); > if (ret)
On Thu, Dec 17, 2015 at 10:49:24PM +0200, Imre Deak wrote: > On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote: > > It is unclear if this is even required on BXT. > > I'm not sure either, I only added it on the premise that it was marked > as SKL+ originally in BSpec. The revision log entry in BSpec has this > much to say: > """ > The workaround that requires an empty PIPE_CONTROL before a > PIPE_CONTROL with a VF Cache Invalidation Enable is only for SKL and > not SKL+. The bug was fixed in CNL with the following HSD:... > """ > Which doesn't make this clear either imo. > > > v2: Make sure to set the default value to false. Uncertain how my compiler > > doesn't complain with v1. > > > > Cc: Imre Deak <imre.deak@intel.com> > > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> > > --- > > drivers/gpu/drm/i915/intel_lrc.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index 519cea32..af1e001 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -1695,7 +1695,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request, > > struct intel_ringbuffer *ringbuf = request->ringbuf; > > struct intel_engine_cs *ring = ringbuf->ring; > > u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES; > > - bool vf_flush_wa; > > + bool vf_flush_wa = false; > > u32 flags = 0; > > int ret; > > > > @@ -1716,14 +1716,14 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request, > > flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE; > > flags |= PIPE_CONTROL_QW_WRITE; > > flags |= PIPE_CONTROL_GLOBAL_GTT_IVB; > > - } > > > > - /* > > - * On GEN9+ Before VF_CACHE_INVALIDATE we need to emit a NULL pipe > > - * control. > > - */ > > - vf_flush_wa = INTEL_INFO(ring->dev)->gen >= 9 && > > - flags & PIPE_CONTROL_VF_CACHE_INVALIDATE; > > + /* > > + * On GEN9: before VF_CACHE_INVALIDATE we need to emit a NULL > > + * pipe control. > > + */ > > +> if (IS_GEN9(ring->dev)) > > Nitpick: INTEL_INFO()->gen == 9 is the preferred way. Either way: > Reviewed-by: Imre Deak <imre.deak@intel.com> > Sounds good to me. Does someone mind fixing this up when they push? This patch is a pre-requisite to another internal patch I have at the moment. > > + vf_flush_wa = true; > > + } > > > > ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6); > > if (ret)
On Thu, 17 Dec 2015, Imre Deak <imre.deak@intel.com> wrote: > On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote: >> +> if (IS_GEN9(ring->dev)) > > Nitpick: INTEL_INFO()->gen == 9 is the preferred way. Oh? Since when and says who? BR, Jani.
On pe, 2015-12-18 at 11:59 +0200, Jani Nikula wrote: > On Thu, 17 Dec 2015, Imre Deak <imre.deak@intel.com> wrote: > > On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote: > > > +> if (IS_GEN9(ring->dev)) > > > > Nitpick: INTEL_INFO()->gen == 9 is the preferred way. > > Oh? Since when and says who? Haven't found it after some digging, but I do remember someone having this comment earlier. My opinion is that we should only use one form, and since INTEL_INFO()- gen can be used in a more generic manner I would prefer that. Currently we have it both ways in the code. --Imre
On Fri, Dec 18, 2015 at 12:34:16PM +0200, Imre Deak wrote: > On pe, 2015-12-18 at 11:59 +0200, Jani Nikula wrote: > > On Thu, 17 Dec 2015, Imre Deak <imre.deak@intel.com> wrote: > > > On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote: > > > > +> if (IS_GEN9(ring->dev)) > > > > > > Nitpick: INTEL_INFO()->gen == 9 is the preferred way. > > > > Oh? Since when and says who? > > Haven't found it after some digging, but I do remember someone having > this comment earlier. > > My opinion is that we should only use one form, and since INTEL_INFO()- > gen can be used in a more generic manner I would prefer that. Currently > we have it both ways in the code. We use IS_GENx for individual gen checks and INTEL_INFO()->gen when checking for ranges. At least that's the usage I inferred ... -Daniel
On Thu, Dec 17, 2015 at 10:49:24PM +0200, Imre Deak wrote: > On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote: > > It is unclear if this is even required on BXT. > > I'm not sure either, I only added it on the premise that it was marked > as SKL+ originally in BSpec. The revision log entry in BSpec has this > much to say: > """ > The workaround that requires an empty PIPE_CONTROL before a > PIPE_CONTROL with a VF Cache Invalidation Enable is only for SKL and > not SKL+. The bug was fixed in CNL with the following HSD:... > """ > Which doesn't make this clear either imo. > > > v2: Make sure to set the default value to false. Uncertain how my compiler > > doesn't complain with v1. > > > > Cc: Imre Deak <imre.deak@intel.com> > > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> > > --- > > drivers/gpu/drm/i915/intel_lrc.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index 519cea32..af1e001 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -1695,7 +1695,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request, > > struct intel_ringbuffer *ringbuf = request->ringbuf; > > struct intel_engine_cs *ring = ringbuf->ring; > > u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES; > > - bool vf_flush_wa; > > + bool vf_flush_wa = false; > > u32 flags = 0; > > int ret; > > > > @@ -1716,14 +1716,14 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request, > > flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE; > > flags |= PIPE_CONTROL_QW_WRITE; > > flags |= PIPE_CONTROL_GLOBAL_GTT_IVB; > > - } > > > > - /* > > - * On GEN9+ Before VF_CACHE_INVALIDATE we need to emit a NULL pipe > > - * control. > > - */ > > - vf_flush_wa = INTEL_INFO(ring->dev)->gen >= 9 && > > - flags & PIPE_CONTROL_VF_CACHE_INVALIDATE; > > + /* > > + * On GEN9: before VF_CACHE_INVALIDATE we need to emit a NULL > > + * pipe control. > > + */ > > +> if (IS_GEN9(ring->dev)) > > Nitpick: INTEL_INFO()->gen == 9 is the preferred way. Either way: > Reviewed-by: Imre Deak <imre.deak@intel.com> Queued for -next, thanks for the patch. -Daniel > > > + vf_flush_wa = true; > > + } > > > > ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6); > > if (ret) > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 519cea32..af1e001 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1695,7 +1695,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request, struct intel_ringbuffer *ringbuf = request->ringbuf; struct intel_engine_cs *ring = ringbuf->ring; u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES; - bool vf_flush_wa; + bool vf_flush_wa = false; u32 flags = 0; int ret; @@ -1716,14 +1716,14 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request, flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE; flags |= PIPE_CONTROL_QW_WRITE; flags |= PIPE_CONTROL_GLOBAL_GTT_IVB; - } - /* - * On GEN9+ Before VF_CACHE_INVALIDATE we need to emit a NULL pipe - * control. - */ - vf_flush_wa = INTEL_INFO(ring->dev)->gen >= 9 && - flags & PIPE_CONTROL_VF_CACHE_INVALIDATE; + /* + * On GEN9: before VF_CACHE_INVALIDATE we need to emit a NULL + * pipe control. + */ + if (IS_GEN9(ring->dev)) + vf_flush_wa = true; + } ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6); if (ret)
It is unclear if this is even required on BXT. v2: Make sure to set the default value to false. Uncertain how my compiler doesn't complain with v1. Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)