diff mbox

[4/6] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

Message ID 1436389139-16282-5-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni July 8, 2015, 8:58 p.m. UTC
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>
---
 drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Daniel Vetter July 9, 2015, 5:10 p.m. UTC | #1
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
Paulo Zanoni July 9, 2015, 5:15 p.m. UTC | #2
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
Paulo Zanoni July 9, 2015, 5:18 p.m. UTC | #3
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
Ville Syrjälä July 9, 2015, 5:22 p.m. UTC | #4
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
Paulo Zanoni July 9, 2015, 5:31 p.m. UTC | #5
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
Ville Syrjälä July 9, 2015, 5:39 p.m. UTC | #6
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.
Paulo Zanoni July 14, 2015, 7:01 p.m. UTC | #7
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 mbox

Patch

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