| Message ID | 1443643545-3603-3-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 30, 2015 at 05:05:45PM -0300, Paulo Zanoni wrote: > According to my experiments (and later confirmation from the hardware > developers), the maximum sizes mentioned in the specification delimit > how far in the buffer the hardware tracking can go. And the hardware > calculates the size based on the plane address we provide - and the > provided plane address might not be the real x:0,y:0 point due to the > compute_page_offset() function. > > On platforms that do the x/y offset adjustment trick it will be really > hard to reproduce a bug, but on the current SKL we can reproduce the > bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this > patch, we'll go from "CRC assertion failure" to "FBC unexpectedly > disabled", which is still a failure on the test suite but is not a > perceived user bug - you will just not save as much power as you could > if FBC is disabled. > > v2, rewrite patch after clarification from the Hadware guys: > - Rename function so it's clear what the check is for. > - Use the new intel_fbc_get_plane_source_sizes() function in order > to get the proper sizes as seen by FBC. > > Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL) > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_fbc.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index d53f73f..313ef91 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -805,10 +805,16 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb) > } > } > > -static bool pipe_size_is_valid(struct intel_crtc *crtc) > +/* > + * For some reason, the hardware tracking starts looking at whatever we > + * programmed as the display plane base address register. It does not look at > + * the X and Y offset registers. That's why we look at the crtc->adjusted{x,y} > + * variables instead of just looking at the pipe size. "plane size" or something > + */ > +static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > - unsigned int max_w, max_h; > + unsigned int used_w, used_h, max_w, max_h; > > if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) { > max_w = 4096; > @@ -821,8 +827,11 @@ static bool pipe_size_is_valid(struct intel_crtc *crtc) > max_h = 1536; > } > > - return crtc->config->pipe_src_w <= max_w && > - crtc->config->pipe_src_h <= max_h; > + intel_fbc_get_plane_source_sizes(crtc, &used_w, &used_h); > + used_w += crtc->adjusted_x; > + used_h += crtc->adjusted_y; > + > + return used_w <= max_w && used_h <= max_h; Makes sense. Not sure used_ is the best prefix. Maybe effective_ ? But I don't mind if you think used_ is better. So with the comment fixed a bit this is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> You know, we could avoid these issues if we stopped using hw gtt tracking too. And then we could use FBC without a fence, and hence wouldn't need X-tiling. IIRC that's now allowed on SKL+. > } > > /** > @@ -899,7 +908,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) > goto out_disable; > } > > - if (!pipe_size_is_valid(intel_crtc)) { > + if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) { > set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE); > goto out_disable; > } > -- > 2.5.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qui, 2015-10-01 às 15:22 +0300, Ville Syrjälä escreveu: > On Wed, Sep 30, 2015 at 05:05:45PM -0300, Paulo Zanoni wrote: > > According to my experiments (and later confirmation from the > > hardware > > developers), the maximum sizes mentioned in the specification > > delimit > > how far in the buffer the hardware tracking can go. And the > > hardware > > calculates the size based on the plane address we provide - and the > > provided plane address might not be the real x:0,y:0 point due to > > the > > compute_page_offset() function. > > > > On platforms that do the x/y offset adjustment trick it will be > > really > > hard to reproduce a bug, but on the current SKL we can reproduce > > the > > bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this > > patch, we'll go from "CRC assertion failure" to "FBC unexpectedly > > disabled", which is still a failure on the test suite but is not a > > perceived user bug - you will just not save as much power as you > > could > > if FBC is disabled. > > > > v2, rewrite patch after clarification from the Hadware guys: > > - Rename function so it's clear what the check is for. > > - Use the new intel_fbc_get_plane_source_sizes() function in > > order > > to get the proper sizes as seen by FBC. > > > > Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL) > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 19 ++++++++++++++----- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > b/drivers/gpu/drm/i915/intel_fbc.c > > index d53f73f..313ef91 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -805,10 +805,16 @@ static bool pixel_format_is_valid(struct > > drm_framebuffer *fb) > > } > > } > > > > -static bool pipe_size_is_valid(struct intel_crtc *crtc) > > +/* > > + * For some reason, the hardware tracking starts looking at > > whatever we > > + * programmed as the display plane base address register. It does > > not look at > > + * the X and Y offset registers. That's why we look at the crtc > > ->adjusted{x,y} > > + * variables instead of just looking at the pipe size. > > "plane size" or something I guess we can say "pipe/plane size" because we're not looking at any of these. > > > + */ > > +static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc > > *crtc) > > { > > struct drm_i915_private *dev_priv = crtc->base.dev > > ->dev_private; > > - unsigned int max_w, max_h; > > + unsigned int used_w, used_h, max_w, max_h; > > > > if (INTEL_INFO(dev_priv)->gen >= 8 || > > IS_HASWELL(dev_priv)) { > > max_w = 4096; > > @@ -821,8 +827,11 @@ static bool pipe_size_is_valid(struct > > intel_crtc *crtc) > > max_h = 1536; > > } > > > > - return crtc->config->pipe_src_w <= max_w && > > - crtc->config->pipe_src_h <= max_h; > > + intel_fbc_get_plane_source_sizes(crtc, &used_w, &used_h); > > + used_w += crtc->adjusted_x; > > + used_h += crtc->adjusted_y; > > + > > + return used_w <= max_w && used_h <= max_h; > > Makes sense. Not sure used_ is the best prefix. Maybe effective_ ? > But I > don't mind if you think used_ is better. I agree used_ is not very good. I changed my mind a few times on these names already. > > So with the comment fixed a bit this is > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > You know, we could avoid these issues if we stopped using hw gtt > tracking too. And then we could use FBC without a fence, and hence > wouldn't need X-tiling. IIRC that's now allowed on SKL+. The problem with stopping the GTT tracking is that then we'd have to completely disable FBC when GTT mmaps are being used, just like we do for PSR. I didn't stop to check how common this is or how much power we'd stop saving if we actually did this. Maybe I should at some point. The other plan is to make our code support FBC both with and without GTT tracking (so we can disable just the tracking when we conclude it won't work). Maybe we can implement this for Kernel 5.23 :) Thanks for the reviews. I'll send the updated versions soon. > > > } > > > > /** > > @@ -899,7 +908,7 @@ static void __intel_fbc_update(struct > > drm_i915_private *dev_priv) > > goto out_disable; > > } > > > > - if (!pipe_size_is_valid(intel_crtc)) { > > + if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) { > > set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE); > > goto out_disable; > > } > > -- > > 2.5.3 > > > > _______________________________________________ > > 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_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index d53f73f..313ef91 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -805,10 +805,16 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb) } } -static bool pipe_size_is_valid(struct intel_crtc *crtc) +/* + * For some reason, the hardware tracking starts looking at whatever we + * programmed as the display plane base address register. It does not look at + * the X and Y offset registers. That's why we look at the crtc->adjusted{x,y} + * variables instead of just looking at the pipe size. + */ +static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) { struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; - unsigned int max_w, max_h; + unsigned int used_w, used_h, max_w, max_h; if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) { max_w = 4096; @@ -821,8 +827,11 @@ static bool pipe_size_is_valid(struct intel_crtc *crtc) max_h = 1536; } - return crtc->config->pipe_src_w <= max_w && - crtc->config->pipe_src_h <= max_h; + intel_fbc_get_plane_source_sizes(crtc, &used_w, &used_h); + used_w += crtc->adjusted_x; + used_h += crtc->adjusted_y; + + return used_w <= max_w && used_h <= max_h; } /** @@ -899,7 +908,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) goto out_disable; } - if (!pipe_size_is_valid(intel_crtc)) { + if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) { set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE); goto out_disable; }
According to my experiments (and later confirmation from the hardware developers), the maximum sizes mentioned in the specification delimit how far in the buffer the hardware tracking can go. And the hardware calculates the size based on the plane address we provide - and the provided plane address might not be the real x:0,y:0 point due to the compute_page_offset() function. On platforms that do the x/y offset adjustment trick it will be really hard to reproduce a bug, but on the current SKL we can reproduce the bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this patch, we'll go from "CRC assertion failure" to "FBC unexpectedly disabled", which is still a failure on the test suite but is not a perceived user bug - you will just not save as much power as you could if FBC is disabled. v2, rewrite patch after clarification from the Hadware guys: - Rename function so it's clear what the check is for. - Use the new intel_fbc_get_plane_source_sizes() function in order to get the proper sizes as seen by FBC. Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL) Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)