| Message ID | 1443643545-3603-2-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote: > We were considering the whole framebuffer height, but the spec says we > should only consider the active display height size. There were still > some unclear questions based on the spec, but the hardware guys > clarified them for us. According to them: > > - CFB size = CFB stride * Number of lines FBC writes to CFB > - CFB stride = plane stride / compression limit > - Number of lines FBC writes to CFB = MIN(plane source height, maximum > number of lines FBC writes to CFB) > - Plane source height = > - pipe source height (PIPE_SRCSZ register) (before SKL) > - plane size register height (PLANE_SIZE register) (SKL+) > - Maximum number of lines FBC writes to CFB = > - plane source height (before HSW) > - 2048 (HSW+) > > For the plane source height, I could just have made our code do > I915_READ() in order to be more future proof, but since it's not cool > to do register reads I decided to just recalculate the values we use > when we actually write to those registers. > > With this patch, depending on your machine configuration, a lot of the > kms_frontbuffer_tracking subtests that used to result in a SKIP due to > not enough stolen memory still start resulting in a PASS. > > v2: Use the clipped src size instead of pipe_src_h (Ville). > v3: Use the appropriate information provided by the hardware guys. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_fbc.c | 58 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 54 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 1b2ebb2..d53f73f 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv) > mutex_unlock(&dev_priv->fbc.lock); > } > > -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size, > - int fb_cpp) > +/* > + * For SKL+, the plane source size used by the hardware is based on the value we > + * write to the PIPE_SIZE register. For BDW-, the hardware looks at the value we > + * wrote to PIPESRC. > + */ > +static void intel_fbc_get_plane_source_sizes(struct intel_crtc *crtc, > + int *width, int *height) size in my mind already includes width and height, so plural _sizes doesn't make much sense to me. > { > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > + struct intel_plane_state *plane_state = > + to_intel_plane_state(crtc->base.primary->state); > + int w, h; > + > + if (INTEL_INFO(dev_priv)->gen >= 9) { > + if (intel_rotation_90_or_270(plane_state->base.rotation)) { > + w = drm_rect_height(&plane_state->src) >> 16; > + h = drm_rect_width(&plane_state->src) >> 16; > + } else { > + w = drm_rect_width(&plane_state->src) >> 16; > + h = drm_rect_height(&plane_state->src) >> 16; > + } You can just use this same code for all platforms. Actually I'm not sure what we should do wrt. rotation. Do we support FBC with 90/270 degree rotation? The scanout happens in a rotated fashion, so swapping the dimensions like you do would seem like the right thing. But not sure. > + } else { > + w = crtc->config->pipe_src_w; > + h = crtc->config->pipe_src_h; > + } > + > + if (width) > + *width = w; > + if (height) > + *height = h; > +} > + > +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc) > +{ > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > + struct drm_framebuffer *fb = crtc->base.primary->fb; > + int lines; > + > + intel_fbc_get_plane_source_sizes(crtc, NULL, &lines); > + if (INTEL_INFO(dev_priv)->gen >= 7) > + lines = min(lines, 2048); > + > + return lines * fb->pitches[0]; > +} > + > +static int intel_fbc_setup_cfb(struct intel_crtc *crtc) > +{ > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > + struct drm_framebuffer *fb = crtc->base.primary->fb; > + int size, fb_cpp; > + > + size = intel_fbc_calculate_cfb_size(crtc); > + fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0); Can we just all it 'cpp' please. We already have too many names for the same thing. Someone could also do a small search&replace to unify the whatever we have currently. > + > if (size <= dev_priv->fbc.uncompressed_size) > return 0; > > @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) > goto out_disable; > } > > - if (intel_fbc_setup_cfb(dev_priv, obj->base.size, > - drm_format_plane_cpp(fb->pixel_format, 0))) { > + if (intel_fbc_setup_cfb(intel_crtc)) { > set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL); > goto out_disable; > } > -- > 2.5.3
On Thu, Oct 01, 2015 at 03:14:13PM +0300, Ville Syrjälä wrote: > On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote: > > We were considering the whole framebuffer height, but the spec says we > > should only consider the active display height size. There were still > > some unclear questions based on the spec, but the hardware guys > > clarified them for us. According to them: > > > > - CFB size = CFB stride * Number of lines FBC writes to CFB > > - CFB stride = plane stride / compression limit > > - Number of lines FBC writes to CFB = MIN(plane source height, maximum > > number of lines FBC writes to CFB) > > - Plane source height = > > - pipe source height (PIPE_SRCSZ register) (before SKL) > > - plane size register height (PLANE_SIZE register) (SKL+) > > - Maximum number of lines FBC writes to CFB = > > - plane source height (before HSW) > > - 2048 (HSW+) > > > > For the plane source height, I could just have made our code do > > I915_READ() in order to be more future proof, but since it's not cool > > to do register reads I decided to just recalculate the values we use > > when we actually write to those registers. > > > > With this patch, depending on your machine configuration, a lot of the > > kms_frontbuffer_tracking subtests that used to result in a SKIP due to > > not enough stolen memory still start resulting in a PASS. > > > > v2: Use the clipped src size instead of pipe_src_h (Ville). > > v3: Use the appropriate information provided by the hardware guys. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 58 +++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 54 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > index 1b2ebb2..d53f73f 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv) > > mutex_unlock(&dev_priv->fbc.lock); > > } > > > > -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size, > > - int fb_cpp) > > +/* > > + * For SKL+, the plane source size used by the hardware is based on the value we > > + * write to the PIPE_SIZE register. For BDW-, the hardware looks at the value we > > + * wrote to PIPESRC. > > + */ > > +static void intel_fbc_get_plane_source_sizes(struct intel_crtc *crtc, > > + int *width, int *height) > > size in my mind already includes width and height, so plural _sizes > doesn't make much sense to me. > > > { > > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > > + struct intel_plane_state *plane_state = > > + to_intel_plane_state(crtc->base.primary->state); > > + int w, h; > > + > > + if (INTEL_INFO(dev_priv)->gen >= 9) { > > + if (intel_rotation_90_or_270(plane_state->base.rotation)) { > > + w = drm_rect_height(&plane_state->src) >> 16; > > + h = drm_rect_width(&plane_state->src) >> 16; > > + } else { > > + w = drm_rect_width(&plane_state->src) >> 16; > > + h = drm_rect_height(&plane_state->src) >> 16; > > + } > > You can just use this same code for all platforms. > > Actually I'm not sure what we should do wrt. rotation. Do we support > FBC with 90/270 degree rotation? The scanout happens in a rotated > fashion, so swapping the dimensions like you do would seem like the > right thing. But not sure. While writing my reply tothe GTT tracking offset patch, I realized that 90/270 degree rotation requires Y-tiling, so since we're currently limiting FBC to X-tiling we can never get here with 90/270 degree rotation. > > > + } else { > > + w = crtc->config->pipe_src_w; > > + h = crtc->config->pipe_src_h; > > + } > > + > > + if (width) > > + *width = w; > > + if (height) > > + *height = h; > > +} > > + > > +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc) > > +{ > > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > > + struct drm_framebuffer *fb = crtc->base.primary->fb; > > + int lines; > > + > > + intel_fbc_get_plane_source_sizes(crtc, NULL, &lines); > > + if (INTEL_INFO(dev_priv)->gen >= 7) > > + lines = min(lines, 2048); > > + > > + return lines * fb->pitches[0]; > > +} > > + > > +static int intel_fbc_setup_cfb(struct intel_crtc *crtc) > > +{ > > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > > + struct drm_framebuffer *fb = crtc->base.primary->fb; > > + int size, fb_cpp; > > + > > + size = intel_fbc_calculate_cfb_size(crtc); > > + fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0); > > Can we just all it 'cpp' please. We already have too many names for the > same thing. Someone could also do a small search&replace to unify the > whatever we have currently. > > > + > > if (size <= dev_priv->fbc.uncompressed_size) > > return 0; > > > > @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) > > goto out_disable; > > } > > > > - if (intel_fbc_setup_cfb(dev_priv, obj->base.size, > > - drm_format_plane_cpp(fb->pixel_format, 0))) { > > + if (intel_fbc_setup_cfb(intel_crtc)) { > > set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL); > > goto out_disable; > > } > > -- > > 2.5.3 > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qui, 2015-10-01 às 15:23 +0300, Ville Syrjälä escreveu: > On Thu, Oct 01, 2015 at 03:14:13PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote: > > > We were considering the whole framebuffer height, but the spec > > > says we > > > should only consider the active display height size. There were > > > still > > > some unclear questions based on the spec, but the hardware guys > > > clarified them for us. According to them: > > > > > > - CFB size = CFB stride * Number of lines FBC writes to CFB > > > - CFB stride = plane stride / compression limit > > > - Number of lines FBC writes to CFB = MIN(plane source height, > > > maximum > > > number of lines FBC writes to CFB) > > > - Plane source height = > > > - pipe source height (PIPE_SRCSZ register) (before SKL) > > > - plane size register height (PLANE_SIZE register) (SKL+) > > > - Maximum number of lines FBC writes to CFB = > > > - plane source height (before HSW) > > > - 2048 (HSW+) > > > > > > For the plane source height, I could just have made our code do > > > I915_READ() in order to be more future proof, but since it's not > > > cool > > > to do register reads I decided to just recalculate the values we > > > use > > > when we actually write to those registers. > > > > > > With this patch, depending on your machine configuration, a lot > > > of the > > > kms_frontbuffer_tracking subtests that used to result in a SKIP > > > due to > > > not enough stolen memory still start resulting in a PASS. > > > > > > v2: Use the clipped src size instead of pipe_src_h (Ville). > > > v3: Use the appropriate information provided by the hardware > > > guys. > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_fbc.c | 58 > > > +++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 54 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > index 1b2ebb2..d53f73f 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct > > > drm_i915_private *dev_priv) > > > mutex_unlock(&dev_priv->fbc.lock); > > > } > > > > > > -static int intel_fbc_setup_cfb(struct drm_i915_private > > > *dev_priv, int size, > > > - int fb_cpp) > > > +/* > > > + * For SKL+, the plane source size used by the hardware is based > > > on the value we > > > + * write to the PIPE_SIZE register. For BDW-, the hardware looks > > > at the value we > > > + * wrote to PIPESRC. > > > + */ > > > +static void intel_fbc_get_plane_source_sizes(struct intel_crtc > > > *crtc, > > > + int *width, int > > > *height) > > > > size in my mind already includes width and height, so plural _sizes > > doesn't make much sense to me. > > > > > { > > > + struct drm_i915_private *dev_priv = crtc->base.dev > > > ->dev_private; > > > + struct intel_plane_state *plane_state = > > > + to_intel_plane_state(crtc->base.primary > > > ->state); > > > + int w, h; > > > + > > > + if (INTEL_INFO(dev_priv)->gen >= 9) { > > > + if (intel_rotation_90_or_270(plane_state > > > ->base.rotation)) { > > > + w = drm_rect_height(&plane_state->src) > > > >> 16; > > > + h = drm_rect_width(&plane_state->src) >> > > > 16; > > > + } else { > > > + w = drm_rect_width(&plane_state->src) >> > > > 16; > > > + h = drm_rect_height(&plane_state->src) > > > >> 16; > > > + } > > > > You can just use this same code for all platforms. This code is trying to recalculate the values we use to write to PIPESRC and PIPE_SIZE because that's what the HW is using. Since on the older platforms we use crtc->pipe_src_x for PIPESRC, I think the correct thing to do is to check those values. > > > > Actually I'm not sure what we should do wrt. rotation. Do we > > support > > FBC with 90/270 degree rotation? The scanout happens in a rotated > > fashion, so swapping the dimensions like you do would seem like the > > right thing. But not sure. > > While writing my reply tothe GTT tracking offset patch, I realized > that > 90/270 degree rotation requires Y-tiling, so since we're currently > limiting FBC to X-tiling we can never get here with 90/270 degree > rotation. But SKL FBC does support Y tiling (even though our code doesn't), so keeping the checks as they are would help preventing future problems. > > > > > > + } else { > > > + w = crtc->config->pipe_src_w; > > > + h = crtc->config->pipe_src_h; > > > + } > > > + > > > + if (width) > > > + *width = w; > > > + if (height) > > > + *height = h; > > > +} > > > + > > > +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc) > > > +{ > > > + struct drm_i915_private *dev_priv = crtc->base.dev > > > ->dev_private; > > > + struct drm_framebuffer *fb = crtc->base.primary->fb; > > > + int lines; > > > + > > > + intel_fbc_get_plane_source_sizes(crtc, NULL, &lines); > > > + if (INTEL_INFO(dev_priv)->gen >= 7) > > > + lines = min(lines, 2048); > > > + > > > + return lines * fb->pitches[0]; > > > +} > > > + > > > +static int intel_fbc_setup_cfb(struct intel_crtc *crtc) > > > +{ > > > + struct drm_i915_private *dev_priv = crtc->base.dev > > > ->dev_private; > > > + struct drm_framebuffer *fb = crtc->base.primary->fb; > > > + int size, fb_cpp; > > > + > > > + size = intel_fbc_calculate_cfb_size(crtc); > > > + fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0); > > > > Can we just all it 'cpp' please. We already have too many names for > > the I was just moving things around... > > same thing. Someone could also do a small search&replace to unify > > the > > whatever we have currently. Someone could patch the drm_format_plane_cpp() function so that it at least explains what CPP is supposed to mean in this context (Colors Per Pixel? Countofbytes Per Pixel?). Or rename the function to drm_format_pane_bpp() to make the name match the description. > > > > > + > > > if (size <= dev_priv->fbc.uncompressed_size) > > > return 0; > > > > > > @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct > > > drm_i915_private *dev_priv) > > > goto out_disable; > > > } > > > > > > - if (intel_fbc_setup_cfb(dev_priv, obj->base.size, > > > - drm_format_plane_cpp(fb > > > ->pixel_format, 0))) { > > > + if (intel_fbc_setup_cfb(intel_crtc)) { > > > set_no_fbc_reason(dev_priv, > > > FBC_STOLEN_TOO_SMALL); > > > goto out_disable; > > > } > > > -- > > > 2.5.3 > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Oct 01, 2015 at 05:47:11PM +0000, Zanoni, Paulo R wrote: > Em Qui, 2015-10-01 às 15:23 +0300, Ville Syrjälä escreveu: > > On Thu, Oct 01, 2015 at 03:14:13PM +0300, Ville Syrjälä wrote: > > > On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote: > > > > We were considering the whole framebuffer height, but the spec > > > > says we > > > > should only consider the active display height size. There were > > > > still > > > > some unclear questions based on the spec, but the hardware guys > > > > clarified them for us. According to them: > > > > > > > > - CFB size = CFB stride * Number of lines FBC writes to CFB > > > > - CFB stride = plane stride / compression limit > > > > - Number of lines FBC writes to CFB = MIN(plane source height, > > > > maximum > > > > number of lines FBC writes to CFB) > > > > - Plane source height = > > > > - pipe source height (PIPE_SRCSZ register) (before SKL) > > > > - plane size register height (PLANE_SIZE register) (SKL+) > > > > - Maximum number of lines FBC writes to CFB = > > > > - plane source height (before HSW) > > > > - 2048 (HSW+) > > > > > > > > For the plane source height, I could just have made our code do > > > > I915_READ() in order to be more future proof, but since it's not > > > > cool > > > > to do register reads I decided to just recalculate the values we > > > > use > > > > when we actually write to those registers. > > > > > > > > With this patch, depending on your machine configuration, a lot > > > > of the > > > > kms_frontbuffer_tracking subtests that used to result in a SKIP > > > > due to > > > > not enough stolen memory still start resulting in a PASS. > > > > > > > > v2: Use the clipped src size instead of pipe_src_h (Ville). > > > > v3: Use the appropriate information provided by the hardware > > > > guys. > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_fbc.c | 58 > > > > +++++++++++++++++++++++++++++++++++++--- > > > > 1 file changed, 54 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > > index 1b2ebb2..d53f73f 100644 > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > > @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct > > > > drm_i915_private *dev_priv) > > > > mutex_unlock(&dev_priv->fbc.lock); > > > > } > > > > > > > > -static int intel_fbc_setup_cfb(struct drm_i915_private > > > > *dev_priv, int size, > > > > - int fb_cpp) > > > > +/* > > > > + * For SKL+, the plane source size used by the hardware is based > > > > on the value we > > > > + * write to the PIPE_SIZE register. For BDW-, the hardware looks > > > > at the value we > > > > + * wrote to PIPESRC. > > > > + */ > > > > +static void intel_fbc_get_plane_source_sizes(struct intel_crtc > > > > *crtc, > > > > + int *width, int > > > > *height) > > > > > > size in my mind already includes width and height, so plural _sizes > > > doesn't make much sense to me. > > > > > > > { > > > > + struct drm_i915_private *dev_priv = crtc->base.dev > > > > ->dev_private; > > > > + struct intel_plane_state *plane_state = > > > > + to_intel_plane_state(crtc->base.primary > > > > ->state); > > > > + int w, h; > > > > + > > > > + if (INTEL_INFO(dev_priv)->gen >= 9) { > > > > + if (intel_rotation_90_or_270(plane_state > > > > ->base.rotation)) { > > > > + w = drm_rect_height(&plane_state->src) > > > > >> 16; > > > > + h = drm_rect_width(&plane_state->src) >> > > > > 16; > > > > + } else { > > > > + w = drm_rect_width(&plane_state->src) >> > > > > 16; > > > > + h = drm_rect_height(&plane_state->src) > > > > >> 16; > > > > + } > > > > > > You can just use this same code for all platforms. > > This code is trying to recalculate the values we use to write to > PIPESRC and PIPE_SIZE because that's what the HW is using. Since on the > older platforms we use crtc->pipe_src_x for PIPESRC, I think the > correct thing to do is to check those values. The plane src size is still what matters. Older platforms just didn't support windowing or scaling on the primary plane. And so there was no need to have a dedicated register for plane size since PIPESRC would anyway contain the same numbers. I can't recall off the top of my head whether gen2/3 supported FBC on the planes that had windowing (plane != A). Perhaps not. > > > > > > Actually I'm not sure what we should do wrt. rotation. Do we > > > support > > > FBC with 90/270 degree rotation? The scanout happens in a rotated > > > fashion, so swapping the dimensions like you do would seem like the > > > right thing. But not sure. > > > > While writing my reply tothe GTT tracking offset patch, I realized > > that > > 90/270 degree rotation requires Y-tiling, so since we're currently > > limiting FBC to X-tiling we can never get here with 90/270 degree > > rotation. > > But SKL FBC does support Y tiling (even though our code doesn't), so > keeping the checks as they are would help preventing future problems. I suppose. Although removing the dependency on hw tracking would be required in any case for Y tiling. > > > > > > > > > > + } else { > > > > + w = crtc->config->pipe_src_w; > > > > + h = crtc->config->pipe_src_h; > > > > + } > > > > + > > > > + if (width) > > > > + *width = w; > > > > + if (height) > > > > + *height = h; > > > > +} > > > > + > > > > +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc) > > > > +{ > > > > + struct drm_i915_private *dev_priv = crtc->base.dev > > > > ->dev_private; > > > > + struct drm_framebuffer *fb = crtc->base.primary->fb; > > > > + int lines; > > > > + > > > > + intel_fbc_get_plane_source_sizes(crtc, NULL, &lines); > > > > + if (INTEL_INFO(dev_priv)->gen >= 7) > > > > + lines = min(lines, 2048); > > > > + > > > > + return lines * fb->pitches[0]; > > > > +} > > > > + > > > > +static int intel_fbc_setup_cfb(struct intel_crtc *crtc) > > > > +{ > > > > + struct drm_i915_private *dev_priv = crtc->base.dev > > > > ->dev_private; > > > > + struct drm_framebuffer *fb = crtc->base.primary->fb; > > > > + int size, fb_cpp; > > > > + > > > > + size = intel_fbc_calculate_cfb_size(crtc); > > > > + fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0); > > > > > > Can we just all it 'cpp' please. We already have too many names for > > > the > > I was just moving things around... > > > > same thing. Someone could also do a small search&replace to unify > > > the > > > whatever we have currently. > > > Someone could patch the drm_format_plane_cpp() function so that it at > least explains what CPP is supposed to mean in this context (Colors Per > Pixel? Countofbytes Per Pixel?). Or rename the function to > drm_format_pane_bpp() to make the name match the description. "chars per pixel" I suppose. Or maybe "bpp" was already taken and 'c' was just the next letter from 'b'. No one knows for sure perhaps ;) But in any case, still a fairly standard way to call it. > > > > > > > > + > > > > if (size <= dev_priv->fbc.uncompressed_size) > > > > return 0; > > > > > > > > @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct > > > > drm_i915_private *dev_priv) > > > > goto out_disable; > > > > } > > > > > > > > - if (intel_fbc_setup_cfb(dev_priv, obj->base.size, > > > > - drm_format_plane_cpp(fb > > > > ->pixel_format, 0))) { > > > > + if (intel_fbc_setup_cfb(intel_crtc)) { > > > > set_no_fbc_reason(dev_priv, > > > > FBC_STOLEN_TOO_SMALL); > > > > goto out_disable; > > > > } > > > > -- > > > > 2.5.3 > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
Em Qui, 2015-10-01 às 21:11 +0300, Ville Syrjälä escreveu: > On Thu, Oct 01, 2015 at 05:47:11PM +0000, Zanoni, Paulo R wrote: > > Em Qui, 2015-10-01 às 15:23 +0300, Ville Syrjälä escreveu: > > > On Thu, Oct 01, 2015 at 03:14:13PM +0300, Ville Syrjälä wrote: > > > > On Wed, Sep 30, 2015 at 05:05:44PM -0300, Paulo Zanoni wrote: > > > > > We were considering the whole framebuffer height, but the > > > > > spec > > > > > says we > > > > > should only consider the active display height size. There > > > > > were > > > > > still > > > > > some unclear questions based on the spec, but the hardware > > > > > guys > > > > > clarified them for us. According to them: > > > > > > > > > > - CFB size = CFB stride * Number of lines FBC writes to CFB > > > > > - CFB stride = plane stride / compression limit > > > > > - Number of lines FBC writes to CFB = MIN(plane source > > > > > height, > > > > > maximum > > > > > number of lines FBC writes to CFB) > > > > > - Plane source height = > > > > > - pipe source height (PIPE_SRCSZ register) (before SKL) > > > > > - plane size register height (PLANE_SIZE register) (SKL+) > > > > > - Maximum number of lines FBC writes to CFB = > > > > > - plane source height (before HSW) > > > > > - 2048 (HSW+) > > > > > > > > > > For the plane source height, I could just have made our code > > > > > do > > > > > I915_READ() in order to be more future proof, but since it's > > > > > not > > > > > cool > > > > > to do register reads I decided to just recalculate the values > > > > > we > > > > > use > > > > > when we actually write to those registers. > > > > > > > > > > With this patch, depending on your machine configuration, a > > > > > lot > > > > > of the > > > > > kms_frontbuffer_tracking subtests that used to result in a > > > > > SKIP > > > > > due to > > > > > not enough stolen memory still start resulting in a PASS. > > > > > > > > > > v2: Use the clipped src size instead of pipe_src_h (Ville). > > > > > v3: Use the appropriate information provided by the hardware > > > > > guys. > > > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_fbc.c | 58 > > > > > +++++++++++++++++++++++++++++++++++++--- > > > > > 1 file changed, 54 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > > > index 1b2ebb2..d53f73f 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > > > @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct > > > > > drm_i915_private *dev_priv) > > > > > mutex_unlock(&dev_priv->fbc.lock); > > > > > } > > > > > > > > > > -static int intel_fbc_setup_cfb(struct drm_i915_private > > > > > *dev_priv, int size, > > > > > - int fb_cpp) > > > > > +/* > > > > > + * For SKL+, the plane source size used by the hardware is > > > > > based > > > > > on the value we > > > > > + * write to the PIPE_SIZE register. For BDW-, the hardware > > > > > looks > > > > > at the value we > > > > > + * wrote to PIPESRC. > > > > > + */ > > > > > +static void intel_fbc_get_plane_source_sizes(struct > > > > > intel_crtc > > > > > *crtc, > > > > > + int *width, int > > > > > *height) > > > > > > > > size in my mind already includes width and height, so plural > > > > _sizes > > > > doesn't make much sense to me. > > > > > > > > > { > > > > > + struct drm_i915_private *dev_priv = crtc->base.dev > > > > > ->dev_private; > > > > > + struct intel_plane_state *plane_state = > > > > > + to_intel_plane_state(crtc > > > > > ->base.primary > > > > > ->state); > > > > > + int w, h; > > > > > + > > > > > + if (INTEL_INFO(dev_priv)->gen >= 9) { > > > > > + if (intel_rotation_90_or_270(plane_state > > > > > ->base.rotation)) { > > > > > + w = drm_rect_height(&plane_state > > > > > ->src) > > > > > > > 16; > > > > > + h = drm_rect_width(&plane_state > > > > > ->src) >> > > > > > 16; > > > > > + } else { > > > > > + w = drm_rect_width(&plane_state > > > > > ->src) >> > > > > > 16; > > > > > + h = drm_rect_height(&plane_state > > > > > ->src) > > > > > > > 16; > > > > > + } > > > > > > > > You can just use this same code for all platforms. > > > > This code is trying to recalculate the values we use to write to > > PIPESRC and PIPE_SIZE because that's what the HW is using. Since on > > the > > older platforms we use crtc->pipe_src_x for PIPESRC, I think the > > correct thing to do is to check those values. > > The plane src size is still what matters. Older platforms just didn't > support windowing or scaling on the primary plane. And so there was > no > need to have a dedicated register for plane size since PIPESRC would > anyway contain the same numbers. I still think the fact that we we use pipe_src_x to set PIPESRC is the most relevant argument. Anyway, since you're sure changing the value won't cause problems, I'll do it: we have way too many plane size fields in our structs and I'm always confused by their undocumented differences. > > I can't recall off the top of my head whether gen2/3 supported FBC on > the planes that had windowing (plane != A). Perhaps not. > > > > > > > > > Actually I'm not sure what we should do wrt. rotation. Do we > > > > support > > > > FBC with 90/270 degree rotation? The scanout happens in a > > > > rotated > > > > fashion, so swapping the dimensions like you do would seem like > > > > the > > > > right thing. But not sure. > > > > > > While writing my reply tothe GTT tracking offset patch, I > > > realized > > > that > > > 90/270 degree rotation requires Y-tiling, so since we're > > > currently > > > limiting FBC to X-tiling we can never get here with 90/270 degree > > > rotation. > > > > But SKL FBC does support Y tiling (even though our code doesn't), > > so > > keeping the checks as they are would help preventing future > > problems. > > I suppose. Although removing the dependency on hw tracking would be > required in any case for Y tiling. > > > > > > > > > > > > > > > + } else { > > > > > + w = crtc->config->pipe_src_w; > > > > > + h = crtc->config->pipe_src_h; > > > > > + } > > > > > + > > > > > + if (width) > > > > > + *width = w; > > > > > + if (height) > > > > > + *height = h; > > > > > +} > > > > > + > > > > > +static int intel_fbc_calculate_cfb_size(struct intel_crtc > > > > > *crtc) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = crtc->base.dev > > > > > ->dev_private; > > > > > + struct drm_framebuffer *fb = crtc->base.primary->fb; > > > > > + int lines; > > > > > + > > > > > + intel_fbc_get_plane_source_sizes(crtc, NULL, > > > > > &lines); > > > > > + if (INTEL_INFO(dev_priv)->gen >= 7) > > > > > + lines = min(lines, 2048); > > > > > + > > > > > + return lines * fb->pitches[0]; > > > > > +} > > > > > + > > > > > +static int intel_fbc_setup_cfb(struct intel_crtc *crtc) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = crtc->base.dev > > > > > ->dev_private; > > > > > + struct drm_framebuffer *fb = crtc->base.primary->fb; > > > > > + int size, fb_cpp; > > > > > + > > > > > + size = intel_fbc_calculate_cfb_size(crtc); > > > > > + fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0); > > > > > > > > Can we just all it 'cpp' please. We already have too many names > > > > for > > > > the > > > > I was just moving things around... > > > > > > same thing. Someone could also do a small search&replace to > > > > unify > > > > the > > > > whatever we have currently. > > > > > > Someone could patch the drm_format_plane_cpp() function so that it > > at > > least explains what CPP is supposed to mean in this context (Colors > > Per > > Pixel? Countofbytes Per Pixel?). Or rename the function to > > drm_format_pane_bpp() to make the name match the description. > > "chars per pixel" I suppose. Or maybe "bpp" was already taken > and 'c' was just the next letter from 'b'. No one knows for sure > perhaps ;) > > But in any case, still a fairly standard way to call it. > > > > > > > > > > > > + > > > > > if (size <= dev_priv->fbc.uncompressed_size) > > > > > return 0; > > > > > > > > > > @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct > > > > > drm_i915_private *dev_priv) > > > > > goto out_disable; > > > > > } > > > > > > > > > > - if (intel_fbc_setup_cfb(dev_priv, obj->base.size, > > > > > - drm_format_plane_cpp(fb > > > > > ->pixel_format, 0))) { > > > > > + if (intel_fbc_setup_cfb(intel_crtc)) { > > > > > set_no_fbc_reason(dev_priv, > > > > > FBC_STOLEN_TOO_SMALL); > > > > > goto out_disable; > > > > > } > > > > > -- > > > > > 2.5.3 > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel OTC > > > > _______________________________________________ > > > > 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 1b2ebb2..d53f73f 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -698,9 +698,60 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv) mutex_unlock(&dev_priv->fbc.lock); } -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size, - int fb_cpp) +/* + * For SKL+, the plane source size used by the hardware is based on the value we + * write to the PIPE_SIZE register. For BDW-, the hardware looks at the value we + * wrote to PIPESRC. + */ +static void intel_fbc_get_plane_source_sizes(struct intel_crtc *crtc, + int *width, int *height) { + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + struct intel_plane_state *plane_state = + to_intel_plane_state(crtc->base.primary->state); + int w, h; + + if (INTEL_INFO(dev_priv)->gen >= 9) { + if (intel_rotation_90_or_270(plane_state->base.rotation)) { + w = drm_rect_height(&plane_state->src) >> 16; + h = drm_rect_width(&plane_state->src) >> 16; + } else { + w = drm_rect_width(&plane_state->src) >> 16; + h = drm_rect_height(&plane_state->src) >> 16; + } + } else { + w = crtc->config->pipe_src_w; + h = crtc->config->pipe_src_h; + } + + if (width) + *width = w; + if (height) + *height = h; +} + +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc) +{ + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + struct drm_framebuffer *fb = crtc->base.primary->fb; + int lines; + + intel_fbc_get_plane_source_sizes(crtc, NULL, &lines); + if (INTEL_INFO(dev_priv)->gen >= 7) + lines = min(lines, 2048); + + return lines * fb->pitches[0]; +} + +static int intel_fbc_setup_cfb(struct intel_crtc *crtc) +{ + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + struct drm_framebuffer *fb = crtc->base.primary->fb; + int size, fb_cpp; + + size = intel_fbc_calculate_cfb_size(crtc); + fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0); + if (size <= dev_priv->fbc.uncompressed_size) return 0; @@ -897,8 +948,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) goto out_disable; } - if (intel_fbc_setup_cfb(dev_priv, obj->base.size, - drm_format_plane_cpp(fb->pixel_format, 0))) { + if (intel_fbc_setup_cfb(intel_crtc)) { set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL); goto out_disable; }
We were considering the whole framebuffer height, but the spec says we should only consider the active display height size. There were still some unclear questions based on the spec, but the hardware guys clarified them for us. According to them: - CFB size = CFB stride * Number of lines FBC writes to CFB - CFB stride = plane stride / compression limit - Number of lines FBC writes to CFB = MIN(plane source height, maximum number of lines FBC writes to CFB) - Plane source height = - pipe source height (PIPE_SRCSZ register) (before SKL) - plane size register height (PLANE_SIZE register) (SKL+) - Maximum number of lines FBC writes to CFB = - plane source height (before HSW) - 2048 (HSW+) For the plane source height, I could just have made our code do I915_READ() in order to be more future proof, but since it's not cool to do register reads I decided to just recalculate the values we use when we actually write to those registers. With this patch, depending on your machine configuration, a lot of the kms_frontbuffer_tracking subtests that used to result in a SKIP due to not enough stolen memory still start resulting in a PASS. v2: Use the clipped src size instead of pipe_src_h (Ville). v3: Use the appropriate information provided by the hardware guys. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 58 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 4 deletions(-)