| Message ID | 20200429152921.10596-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Wed, Apr 29, 2020 at 06:29:21PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Consult the actual plane stride instead of the fb stride. The two > will disagree when we remap the gtt. The plane stride is what the > hw will be fed so that's what we should look at for the FBC > retrictions/cfb allocation. > > Since we no longer require a fence we are going to attempt using > FBC with remapping, and so we should look at correct stride. > > With 90/270 degree rotation the plane stride is stored in units > of pixels, so we need to conver it to bytes for the purposes > of calculating the cfb stride. Not entirely sure if this matches > the hw behaviour though. Need to reverse engineer that at some > point... > > We also need to reorder the pixel format check vs. stride check > to avoid triggering a spurious WARN(stride & 63) with cpp==1 and > plane stride==32. > > v2: Try to deal with rotated stride and related WARN > > Cc: José Roberto de Souza <jose.souza@intel.com> > Fixes: 691f7ba58d52 ("drm/i915/display/fbc: Make fences a nice-to-have for GEN9+") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index 7194f9bc62c5..7f2b2382b813 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -707,9 +707,13 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode; > > cache->fb.format = fb->format; > - cache->fb.stride = fb->pitches[0]; > cache->fb.modifier = fb->modifier; > > + /* FIXME is this correct? */ > + cache->fb.stride = plane_state->color_plane[0].stride; We still have a comment in intel_fbc_calculate_cfb_size() that indicates that we need to use the framebuffer stride instead of the plane stride (explicitly added in commit 850bfaab7120a). The bspec (page 49227) uses terminology "Stride of plane uncompressed surface" which sounds like framebuffer size to me; I'm not sure if switching it to the plane's size will cause problems if the plane is only scanning out a subregion of the framebuffer? If it really is safe to use the plane size instead of the framebuffer size, then I think we at least need to remove or change that comment too. Matt > + if (drm_rotation_90_or_270(plane_state->hw.rotation)) > + cache->fb.stride *= fb->format->cpp[0]; > + > drm_WARN_ON(&dev_priv->drm, plane_state->flags & PLANE_HAS_FENCE && > !plane_state->vma->fence); > > @@ -804,6 +808,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > return false; > } > > + if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) { > + fbc->no_fbc_reason = "pixel format is invalid"; > + return false; > + } > + > if (!rotation_is_valid(dev_priv, cache->fb.format->format, > cache->plane.rotation)) { > fbc->no_fbc_reason = "rotation unsupported"; > @@ -820,11 +829,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > return false; > } > > - if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) { > - fbc->no_fbc_reason = "pixel format is invalid"; > - return false; > - } > - > if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE && > cache->fb.format->has_alpha) { > fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC"; > -- > 2.24.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 01, 2020 at 05:16:13PM -0700, Matt Roper wrote: > On Wed, Apr 29, 2020 at 06:29:21PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Consult the actual plane stride instead of the fb stride. The two > > will disagree when we remap the gtt. The plane stride is what the > > hw will be fed so that's what we should look at for the FBC > > retrictions/cfb allocation. > > > > Since we no longer require a fence we are going to attempt using > > FBC with remapping, and so we should look at correct stride. > > > > With 90/270 degree rotation the plane stride is stored in units > > of pixels, so we need to conver it to bytes for the purposes > > of calculating the cfb stride. Not entirely sure if this matches > > the hw behaviour though. Need to reverse engineer that at some > > point... > > > > We also need to reorder the pixel format check vs. stride check > > to avoid triggering a spurious WARN(stride & 63) with cpp==1 and > > plane stride==32. > > > > v2: Try to deal with rotated stride and related WARN > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Fixes: 691f7ba58d52 ("drm/i915/display/fbc: Make fences a nice-to-have for GEN9+") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > index 7194f9bc62c5..7f2b2382b813 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -707,9 +707,13 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > > cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode; > > > > cache->fb.format = fb->format; > > - cache->fb.stride = fb->pitches[0]; > > cache->fb.modifier = fb->modifier; > > > > + /* FIXME is this correct? */ > > + cache->fb.stride = plane_state->color_plane[0].stride; > > We still have a comment in intel_fbc_calculate_cfb_size() that indicates > that we need to use the framebuffer stride instead of the plane stride > (explicitly added in commit 850bfaab7120a). That's not really what it's saying. full buffer stride == plane stride, vs. active area == plane width > The bspec (page 49227) uses > terminology "Stride of plane uncompressed surface" which sounds like > framebuffer size to me; I'm not sure if switching it to the plane's size > will cause problems if the plane is only scanning out a subregion of the > framebuffer? There is no framebuffer stride as far as the hardware is concerned. There is only plane width and plane stride. > > If it really is safe to use the plane size instead of the framebuffer > size, then I think we at least need to remove or change that comment > too. > > > Matt > > > + if (drm_rotation_90_or_270(plane_state->hw.rotation)) > > + cache->fb.stride *= fb->format->cpp[0]; > > + > > drm_WARN_ON(&dev_priv->drm, plane_state->flags & PLANE_HAS_FENCE && > > !plane_state->vma->fence); > > > > @@ -804,6 +808,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > > return false; > > } > > > > + if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) { > > + fbc->no_fbc_reason = "pixel format is invalid"; > > + return false; > > + } > > + > > if (!rotation_is_valid(dev_priv, cache->fb.format->format, > > cache->plane.rotation)) { > > fbc->no_fbc_reason = "rotation unsupported"; > > @@ -820,11 +829,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > > return false; > > } > > > > - if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) { > > - fbc->no_fbc_reason = "pixel format is invalid"; > > - return false; > > - } > > - > > if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE && > > cache->fb.format->has_alpha) { > > fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC"; > > -- > > 2.24.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation > (916) 356-2795
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index 7194f9bc62c5..7f2b2382b813 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -707,9 +707,13 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode; cache->fb.format = fb->format; - cache->fb.stride = fb->pitches[0]; cache->fb.modifier = fb->modifier; + /* FIXME is this correct? */ + cache->fb.stride = plane_state->color_plane[0].stride; + if (drm_rotation_90_or_270(plane_state->hw.rotation)) + cache->fb.stride *= fb->format->cpp[0]; + drm_WARN_ON(&dev_priv->drm, plane_state->flags & PLANE_HAS_FENCE && !plane_state->vma->fence); @@ -804,6 +808,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) return false; } + if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) { + fbc->no_fbc_reason = "pixel format is invalid"; + return false; + } + if (!rotation_is_valid(dev_priv, cache->fb.format->format, cache->plane.rotation)) { fbc->no_fbc_reason = "rotation unsupported"; @@ -820,11 +829,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) return false; } - if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) { - fbc->no_fbc_reason = "pixel format is invalid"; - return false; - } - if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE && cache->fb.format->has_alpha) { fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC";