| Message ID | 1436389139-16282-5-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > The doc is pretty clear that this register should be set to 0 on SNB. > We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines below. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Hm, do we have testcases where we have a sufficiently big y offset? We can just allocate 128 lines more and use that as the offset, that should be big enough everywhere. Actually make that 129 lines to check the tile-size rounding ;-) Ofc this means we need to have two sets of testcases for all the affected tests (i.e. everything that tries to test the gtt hw tracking). Another funny corner case (which we're getting wrong on skl even without fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold bigger values and then it wraps). I.e. I'd like this patch (and the others) to be augmented with a Testcase: tag. Cheers, Daniel > --- > drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 0373cbc..0a24e96 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc *crtc) > dpfc_ctl |= obj->fence_reg; > > y_offset = get_crtc_fence_y_offset(crtc); > - I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); > + > + if (IS_GEN5(dev_priv)) > + I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); > + else > + I915_WRITE(ILK_DPFC_FENCE_YOFF, 0); > + > I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID); > /* enable it... */ > I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN); > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-07-09 14:10 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> The doc is pretty clear that this register should be set to 0 on SNB. >> We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines below. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Hm, do we have testcases where we have a sufficiently big y offset? We can > just allocate 128 lines more and use that as the offset, that should be > big enough everywhere. Actually make that 129 lines to check the tile-size > rounding ;-) Yes, it's 500x500. See FBS_MULTI on kms_frontbuffer_tracking. Subtests with "sfb" on their names create a Single frontbuffer for each possible primary plane (primary, secondary, offscreen), while subtests with "mfb" have Multiple pipes pointing to the same frontbuffer. See the small drawing inside kms_frontbuffer_tracking, at the top of create_big_fb(). By the way, it's on my TODO list to change that arrangement a little bit in order to avoid super-huge strides. > > Ofc this means we need to have two sets of testcases for all the affected > tests (i.e. everything that tries to test the gtt hw tracking). We do. > > Another funny corner case (which we're getting wrong on skl even without > fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold > bigger values and then it wraps). Can you please clarify the sentence above? For the dual-screen subtests, if we use 2 screens of 1024 + the 500 pixel offset, we'll already be bigger than 2048 pixels. > > I.e. I'd like this patch (and the others) to be augmented with a Testcase: This one doesn't have a Testcase tag because I'm not testing SNB right now, so I can't confirm anything here. Patch 3 has the useful Testcases tags. > tag. > > Cheers, Daniel > >> --- >> drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >> index 0373cbc..0a24e96 100644 >> --- a/drivers/gpu/drm/i915/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/intel_fbc.c >> @@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc *crtc) >> dpfc_ctl |= obj->fence_reg; >> >> y_offset = get_crtc_fence_y_offset(crtc); >> - I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); >> + >> + if (IS_GEN5(dev_priv)) >> + I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); >> + else >> + I915_WRITE(ILK_DPFC_FENCE_YOFF, 0); >> + >> I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID); >> /* enable it... */ >> I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN); >> -- >> 2.1.4 >> >> _______________________________________________ >> 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
2015-07-09 14:15 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>: > 2015-07-09 14:10 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: >> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote: >>> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> >>> The doc is pretty clear that this register should be set to 0 on SNB. >>> We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines below. >>> >>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Hm, do we have testcases where we have a sufficiently big y offset? We can >> just allocate 128 lines more and use that as the offset, that should be >> big enough everywhere. Actually make that 129 lines to check the tile-size >> rounding ;-) > > Yes, it's 500x500. See FBS_MULTI on kms_frontbuffer_tracking. Subtests > with "sfb" on their names create a Single frontbuffer for each > possible primary plane (primary, secondary, offscreen), while subtests > with "mfb" have Multiple pipes pointing to the same frontbuffer. See > the small drawing inside kms_frontbuffer_tracking, at the top of > create_big_fb(). By the way, it's on my TODO list to change that > arrangement a little bit in order to avoid super-huge strides. Also notice that I tested values other than 500x500 while developing patch 3. 500x500 seems like a nice value since it's not aligned with any tiles and never part of the first tile. > >> >> Ofc this means we need to have two sets of testcases for all the affected >> tests (i.e. everything that tries to test the gtt hw tracking). > > We do. > >> >> Another funny corner case (which we're getting wrong on skl even without >> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold >> bigger values and then it wraps). > > Can you please clarify the sentence above? For the dual-screen > subtests, if we use 2 screens of 1024 + the 500 pixel offset, we'll > already be bigger than 2048 pixels. > >> >> I.e. I'd like this patch (and the others) to be augmented with a Testcase: > > This one doesn't have a Testcase tag because I'm not testing SNB right > now, so I can't confirm anything here. Patch 3 has the useful > Testcases tags. > >> tag. >> >> Cheers, Daniel >> >>> --- >>> drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >>> index 0373cbc..0a24e96 100644 >>> --- a/drivers/gpu/drm/i915/intel_fbc.c >>> +++ b/drivers/gpu/drm/i915/intel_fbc.c >>> @@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc *crtc) >>> dpfc_ctl |= obj->fence_reg; >>> >>> y_offset = get_crtc_fence_y_offset(crtc); >>> - I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); >>> + >>> + if (IS_GEN5(dev_priv)) >>> + I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); >>> + else >>> + I915_WRITE(ILK_DPFC_FENCE_YOFF, 0); >>> + >>> I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID); >>> /* enable it... */ >>> I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN); >>> -- >>> 2.1.4 >>> >>> _______________________________________________ >>> 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 > > > > -- > Paulo Zanoni
On Thu, Jul 09, 2015 at 07:10:04PM +0200, Daniel Vetter wrote: > On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > The doc is pretty clear that this register should be set to 0 on SNB. > > We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines below. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Hm, do we have testcases where we have a sufficiently big y offset? We can > just allocate 128 lines more and use that as the offset, that should be > big enough everywhere. Actually make that 129 lines to check the tile-size > rounding ;-) > > Ofc this means we need to have two sets of testcases for all the affected > tests (i.e. everything that tries to test the gtt hw tracking). > > Another funny corner case (which we're getting wrong on skl even without > fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold > bigger values and then it wraps). > > I.e. I'd like this patch (and the others) to be augmented with a Testcase: > tag. I think the entire Y offset thing is currently being misprogrammed. IIRC the offset is from the display base address but we program in the offset from the start of the FB. > > Cheers, Daniel > > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > index 0373cbc..0a24e96 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc *crtc) > > dpfc_ctl |= obj->fence_reg; > > > > y_offset = get_crtc_fence_y_offset(crtc); > > - I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); > > + > > + if (IS_GEN5(dev_priv)) > > + I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); > > + else > > + I915_WRITE(ILK_DPFC_FENCE_YOFF, 0); > > + > > I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID); > > /* enable it... */ > > I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN); > > -- > > 2.1.4 > > > > _______________________________________________ > > 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
2015-07-09 14:22 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > On Thu, Jul 09, 2015 at 07:10:04PM +0200, Daniel Vetter wrote: >> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote: >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > >> > The doc is pretty clear that this register should be set to 0 on SNB. >> > We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines below. >> > >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Hm, do we have testcases where we have a sufficiently big y offset? We can >> just allocate 128 lines more and use that as the offset, that should be >> big enough everywhere. Actually make that 129 lines to check the tile-size >> rounding ;-) >> >> Ofc this means we need to have two sets of testcases for all the affected >> tests (i.e. everything that tries to test the gtt hw tracking). >> >> Another funny corner case (which we're getting wrong on skl even without >> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold >> bigger values and then it wraps). >> >> I.e. I'd like this patch (and the others) to be augmented with a Testcase: >> tag. > > I think the entire Y offset thing is currently being misprogrammed. > IIRC the offset is from the display base address but we program in > the offset from the start of the FB. After patch 3, all the current tests pass on BDW. Can you suggest a different test that won't pass? > >> >> Cheers, Daniel >> >> > --- >> > drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++- >> > 1 file changed, 6 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >> > index 0373cbc..0a24e96 100644 >> > --- a/drivers/gpu/drm/i915/intel_fbc.c >> > +++ b/drivers/gpu/drm/i915/intel_fbc.c >> > @@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc *crtc) >> > dpfc_ctl |= obj->fence_reg; >> > >> > y_offset = get_crtc_fence_y_offset(crtc); >> > - I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); >> > + >> > + if (IS_GEN5(dev_priv)) >> > + I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); >> > + else >> > + I915_WRITE(ILK_DPFC_FENCE_YOFF, 0); >> > + >> > I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID); >> > /* enable it... */ >> > I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN); >> > -- >> > 2.1.4 >> > >> > _______________________________________________ >> > 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 > > -- > Ville Syrjälä > Intel OTC
On Thu, Jul 09, 2015 at 02:31:15PM -0300, Paulo Zanoni wrote: > 2015-07-09 14:22 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > On Thu, Jul 09, 2015 at 07:10:04PM +0200, Daniel Vetter wrote: > >> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote: > >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > > >> > The doc is pretty clear that this register should be set to 0 on SNB. > >> > We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines below. > >> > > >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> Hm, do we have testcases where we have a sufficiently big y offset? We can > >> just allocate 128 lines more and use that as the offset, that should be > >> big enough everywhere. Actually make that 129 lines to check the tile-size > >> rounding ;-) > >> > >> Ofc this means we need to have two sets of testcases for all the affected > >> tests (i.e. everything that tries to test the gtt hw tracking). > >> > >> Another funny corner case (which we're getting wrong on skl even without > >> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold > >> bigger values and then it wraps). > >> > >> I.e. I'd like this patch (and the others) to be augmented with a Testcase: > >> tag. > > > > I think the entire Y offset thing is currently being misprogrammed. > > IIRC the offset is from the display base address but we program in > > the offset from the start of the FB. > > After patch 3, all the current tests pass on BDW. Can you suggest a > different test that won't pass? Ah patch 3 tries to fix it. It's not entirely accurate though since it simply relies on an implementation detail of intel_gen4_compute_page_offset(). Well, assuming my recollection of the hardware details is correct. Also IIRC intel_gen4_compute_page_offset() isn't even used on SKL/BXT currently, so it should fail on those platforms.
2015-07-09 14:39 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > On Thu, Jul 09, 2015 at 02:31:15PM -0300, Paulo Zanoni wrote: >> 2015-07-09 14:22 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: >> > On Thu, Jul 09, 2015 at 07:10:04PM +0200, Daniel Vetter wrote: >> >> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote: >> >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> > >> >> > The doc is pretty clear that this register should be set to 0 on SNB. >> >> > We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines below. >> >> > >> >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> >> >> Hm, do we have testcases where we have a sufficiently big y offset? We can >> >> just allocate 128 lines more and use that as the offset, that should be >> >> big enough everywhere. Actually make that 129 lines to check the tile-size >> >> rounding ;-) >> >> >> >> Ofc this means we need to have two sets of testcases for all the affected >> >> tests (i.e. everything that tries to test the gtt hw tracking). >> >> >> >> Another funny corner case (which we're getting wrong on skl even without >> >> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold >> >> bigger values and then it wraps). >> >> >> >> I.e. I'd like this patch (and the others) to be augmented with a Testcase: >> >> tag. >> > >> > I think the entire Y offset thing is currently being misprogrammed. >> > IIRC the offset is from the display base address but we program in >> > the offset from the start of the FB. >> >> After patch 3, all the current tests pass on BDW. Can you suggest a >> different test that won't pass? > > Ah patch 3 tries to fix it. It's not entirely accurate though since it > simply relies on an implementation detail of intel_gen4_compute_page_offset(). > Well, assuming my recollection of the hardware details is correct. > > Also IIRC intel_gen4_compute_page_offset() isn't even used on SKL/BXT > currently, so it should fail on those platforms. Daniel clarified the problem to me, so I implemented the following test case: http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/commit/?id=04d1311fc3d2127d609b5c5e670bf9887652cb17 I hope this exercises the problem you're mentioning. So far I only tested BDW and it passes. > > -- > Ville Syrjälä > Intel OTC
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 0373cbc..0a24e96 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc *crtc) dpfc_ctl |= obj->fence_reg; y_offset = get_crtc_fence_y_offset(crtc); - I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); + + if (IS_GEN5(dev_priv)) + I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset); + else + I915_WRITE(ILK_DPFC_FENCE_YOFF, 0); + I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID); /* enable it... */ I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);