Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled
On 6/12/20 11:00 AM, Daniel Vetter wrote: The atomic helpers try really hard to not lose track of things, duplicating enabled tracking in the driver is at best confusing. Double-enabling or disabling is a bug in atomic helpers. In the fb_dirty function we can just assume that the fb always exists, simple display pipe helpers guarantee that the crtc is only enabled together with the output, so we always have a primary plane around. Now in the update function we need to be a notch more careful, since that can also get called when the crtc is off. And we don't want to upload frames when that's the case, so filter that out too. Signed-off-by: Daniel Vetter Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter Cc: David Lechner --- Acked-by: David Lechner ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 8/8] drm/tiny/repaper: Drop edp->enabled
Den 12.06.2020 18.00, skrev Daniel Vetter: > Same patch as the mipi-dbi one, atomic tracks this for us already, we > just have to check the right thing. > > Signed-off-by: Daniel Vetter > Cc: "Noralf Trønnes" > --- Reviewed-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled
Den 12.06.2020 18.00, skrev Daniel Vetter: > The atomic helpers try really hard to not lose track of things, > duplicating enabled tracking in the driver is at best confusing. > Double-enabling or disabling is a bug in atomic helpers. > > In the fb_dirty function we can just assume that the fb always exists, > simple display pipe helpers guarantee that the crtc is only enabled > together with the output, so we always have a primary plane around. > > Now in the update function we need to be a notch more careful, since > that can also get called when the crtc is off. And we don't want to > upload frames when that's the case, so filter that out too. > > Signed-off-by: Daniel Vetter > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: David Lechner > --- Thanks for fixing this. Reviewed-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: Flush gen3 relocs harder, again
Quoting Mika Kuoppala (2020-06-13 09:44:39)
> Chris Wilson writes:
>
> > Quoting Mika Kuoppala (2020-06-12 23:05:18)
> >> Chris Wilson writes:
> >>
> >> > gen3 does not fully flush MI stores to memory on MI_FLUSH, such that a
> >> > subsequent read from e.g. the sampler can bypass the store and read the
> >> > stale value from memory. This is a serious issue when we are using MI
> >> > stores to rewrite the batches for relocation, as it means that the batch
> >> > is reading from random user/kernel memory. While it is particularly
> >> > sensitive [and detectable] for relocations, reading stale data at any
> >> > time is a worry.
> >> >
> >> > Having started with a small number of delaying stores and doubling until
> >> > no more incoherency was seen over a few hours (with and without
> >> > background memory pressure), 32 was the magic number.
> >> >
> >> > v2: Follow more closer with the gen5 w/a and include some
> >> > post-invalidate flushes as well.
> >> >
> >> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2018
> >> > References: a889580c087a ("drm/i915: Flush GPU relocs harder for gen3")
> >> > Signed-off-by: Chris Wilson
> >> > Cc: Mika Kuoppala
> >> > Cc: Joonas Lahtinen
> >> > ---
> >> > drivers/gpu/drm/i915/gt/gen2_engine_cs.c | 61 ++--
> >> > 1 file changed, 25 insertions(+), 36 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c
> >> > b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c
> >> > index 3fb0dc1fb910..5400d657f334 100644
> >> > --- a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c
> >> > +++ b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c
> >> > @@ -13,28 +13,25 @@
> >> >
> >> > int gen2_emit_flush(struct i915_request *rq, u32 mode)
> >> > {
> >> > - unsigned int num_store_dw;
> >> > + unsigned int num_store_dw = 12;
> >> > u32 cmd, *cs;
> >> >
> >> > cmd = MI_FLUSH;
> >> > - num_store_dw = 0;
> >> > if (mode & EMIT_INVALIDATE)
> >> > cmd |= MI_READ_FLUSH;
> >> > - if (mode & EMIT_FLUSH)
> >> > - num_store_dw = 4;
> >> >
> >> > - cs = intel_ring_begin(rq, 2 + 3 * num_store_dw);
> >> > + cs = intel_ring_begin(rq, 2 + 4 * num_store_dw);
> >> > if (IS_ERR(cs))
> >> > return PTR_ERR(cs);
> >> >
> >> > *cs++ = cmd;
> >> > while (num_store_dw--) {
> >> > - *cs++ = MI_STORE_DWORD_IMM | MI_MEM_VIRTUAL;
> >> > - *cs++ = intel_gt_scratch_offset(rq->engine->gt,
> >> > -
> >> > INTEL_GT_SCRATCH_FIELD_DEFAULT);
> >> > - *cs++ = 0;
> >> > + *cs++ = MI_STORE_DWORD_INDEX;
> >> > + *cs++ = I915_GEM_HWS_SCRATCH * sizeof(u32);
> >> > + *cs++ = rq->fence.seqno - 1;
> >> > + *cs++ = MI_FLUSH | MI_NO_WRITE_FLUSH;
> >> > }
> >> > - *cs++ = MI_FLUSH | MI_NO_WRITE_FLUSH;
> >> > + *cs++ = cmd;
> >> >
> >> > intel_ring_advance(rq, cs);
> >> >
> >> > @@ -142,38 +139,21 @@ int gen4_emit_flush_vcs(struct i915_request *rq,
> >> > u32 mode)
> >> > return 0;
> >> > }
> >> >
> >> > -u32 *gen3_emit_breadcrumb(struct i915_request *rq, u32 *cs)
> >> > +static u32 *__gen2_emit_breadcrumb(struct i915_request *rq, u32 *cs,
> >> > +int flush, int post)
> >> > {
> >> > GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt !=
> >> > rq->engine->status_page.vma);
> >> >
> >> > GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset)
> >> > != I915_GEM_HWS_SEQNO_ADDR);
> >> >
> >> > *cs++ = MI_FLUSH;
> >> >
> >> > - *cs++ = MI_STORE_DWORD_INDEX;
> >> > - *cs++ = I915_GEM_HWS_SEQNO_ADDR;
> >> > - *cs++ = rq->fence.seqno;
> >> > -
> >> > - *cs++ = MI_USER_INTERRUPT;
> >>
> >> How can you throw the interrupt part out?
> >
> > Diff being confusing. gen3_emit_breadcrumb and gen5_emit_breadcrumb
> > merged together.
>
> Reviewed-by: Mika Kuoppala
It failed eventually, so unfortunately it's still just paper. However
increasing the MTBF by a few orders of magnitude should be enough to
stop CI complaining on every idle run.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: Flush gen3 relocs harder, again
Chris Wilson writes:
> Quoting Mika Kuoppala (2020-06-12 23:05:18)
>> Chris Wilson writes:
>>
>> > gen3 does not fully flush MI stores to memory on MI_FLUSH, such that a
>> > subsequent read from e.g. the sampler can bypass the store and read the
>> > stale value from memory. This is a serious issue when we are using MI
>> > stores to rewrite the batches for relocation, as it means that the batch
>> > is reading from random user/kernel memory. While it is particularly
>> > sensitive [and detectable] for relocations, reading stale data at any
>> > time is a worry.
>> >
>> > Having started with a small number of delaying stores and doubling until
>> > no more incoherency was seen over a few hours (with and without
>> > background memory pressure), 32 was the magic number.
>> >
>> > v2: Follow more closer with the gen5 w/a and include some
>> > post-invalidate flushes as well.
>> >
>> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2018
>> > References: a889580c087a ("drm/i915: Flush GPU relocs harder for gen3")
>> > Signed-off-by: Chris Wilson
>> > Cc: Mika Kuoppala
>> > Cc: Joonas Lahtinen
>> > ---
>> > drivers/gpu/drm/i915/gt/gen2_engine_cs.c | 61 ++--
>> > 1 file changed, 25 insertions(+), 36 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c
>> > b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c
>> > index 3fb0dc1fb910..5400d657f334 100644
>> > --- a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c
>> > +++ b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c
>> > @@ -13,28 +13,25 @@
>> >
>> > int gen2_emit_flush(struct i915_request *rq, u32 mode)
>> > {
>> > - unsigned int num_store_dw;
>> > + unsigned int num_store_dw = 12;
>> > u32 cmd, *cs;
>> >
>> > cmd = MI_FLUSH;
>> > - num_store_dw = 0;
>> > if (mode & EMIT_INVALIDATE)
>> > cmd |= MI_READ_FLUSH;
>> > - if (mode & EMIT_FLUSH)
>> > - num_store_dw = 4;
>> >
>> > - cs = intel_ring_begin(rq, 2 + 3 * num_store_dw);
>> > + cs = intel_ring_begin(rq, 2 + 4 * num_store_dw);
>> > if (IS_ERR(cs))
>> > return PTR_ERR(cs);
>> >
>> > *cs++ = cmd;
>> > while (num_store_dw--) {
>> > - *cs++ = MI_STORE_DWORD_IMM | MI_MEM_VIRTUAL;
>> > - *cs++ = intel_gt_scratch_offset(rq->engine->gt,
>> > -
>> > INTEL_GT_SCRATCH_FIELD_DEFAULT);
>> > - *cs++ = 0;
>> > + *cs++ = MI_STORE_DWORD_INDEX;
>> > + *cs++ = I915_GEM_HWS_SCRATCH * sizeof(u32);
>> > + *cs++ = rq->fence.seqno - 1;
>> > + *cs++ = MI_FLUSH | MI_NO_WRITE_FLUSH;
>> > }
>> > - *cs++ = MI_FLUSH | MI_NO_WRITE_FLUSH;
>> > + *cs++ = cmd;
>> >
>> > intel_ring_advance(rq, cs);
>> >
>> > @@ -142,38 +139,21 @@ int gen4_emit_flush_vcs(struct i915_request *rq, u32
>> > mode)
>> > return 0;
>> > }
>> >
>> > -u32 *gen3_emit_breadcrumb(struct i915_request *rq, u32 *cs)
>> > +static u32 *__gen2_emit_breadcrumb(struct i915_request *rq, u32 *cs,
>> > +int flush, int post)
>> > {
>> > GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt !=
>> > rq->engine->status_page.vma);
>> >
>> > GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset)
>> > != I915_GEM_HWS_SEQNO_ADDR);
>> >
>> > *cs++ = MI_FLUSH;
>> >
>> > - *cs++ = MI_STORE_DWORD_INDEX;
>> > - *cs++ = I915_GEM_HWS_SEQNO_ADDR;
>> > - *cs++ = rq->fence.seqno;
>> > -
>> > - *cs++ = MI_USER_INTERRUPT;
>>
>> How can you throw the interrupt part out?
>
> Diff being confusing. gen3_emit_breadcrumb and gen5_emit_breadcrumb
> merged together.
Reviewed-by: Mika Kuoppala
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
