| Message ID | 20200219014230.145601-3-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter | expand |
On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza wrote: > dGFX have local memory so it do not have aperture and do not support > CPU fences but even for iGFX it have a small number of fences. > > As replacement for fences to track frontbuffer modifications by CPU > we have a software tracking that is already in used by FBC and PSR. > PSR don't support fences so it shows that this tracking is reliable. > > So lets make fences a nice-to-have to activate FBC for GEN9+, this > will allow us to enable FBC for dGFXs and iGFXs even when there is no > available fence. > > We do not set fences to rotated planes but FBC only have restrictions > against 16bpp, so adding it here. > > Also adding a new check for the tiling format, fences are only set > to X and Y tiled planes but again FBC don't have any restrictions > against tiling so adding linear as supported as well, other formats > should be added after tested but IGT only supports drawing in thse > 3 formats. > > intel_fbc_hw_tracking_covers_screen() maybe can also have the same > treatment as fences but BSpec is not clear if the size limitation is > for hardware tracking or general use of FBC and I don't have a 5K > display to test it, so keeping as is for safety. > > v2: > - Added tiling and pixel format rotation checks > - Changed the GEN version not requiring fences to 11 from 9, DDX > needs some changes but it don't have support for GEN11+ > > v3: > - Changed back to GEN9+ > - Moved GEN test to inside of tiling_is_valid() > > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 45 ++++++++++++++++++++---- > drivers/gpu/drm/i915/i915_drv.h | 1 + > 2 files changed, 39 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index 1d76e3646a25..a0d1d661a006 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct drm_i915_private *dev_priv, > } > > static bool pixel_format_is_valid(struct drm_i915_private *dev_priv, > - u32 pixel_format) > + u32 pixel_format, unsigned int rotation) > { > switch (pixel_format) { > case DRM_FORMAT_XRGB8888: > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct drm_i915_private *dev_priv, > /* WaFbcOnly1to1Ratio:ctg */ > if (IS_G4X(dev_priv)) > return false; > + if ((rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) && > + INTEL_GEN(dev_priv) >= 9) > + return false; Would still would prefer a rotations_is_valid() or some such thing. > return true; > default: > return false; > @@ -639,6 +642,22 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > return effective_w <= max_w && effective_h <= max_h; > } > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv, > + uint64_t modifier) > +{ > + switch (modifier) { > + case DRM_FORMAT_MOD_LINEAR: > + if (INTEL_GEN(dev_priv) >= 9) > + return true; Have we checked that eg. fbcon cursor still blinks correctly with FBC active and all? > + return false; > + case I915_FORMAT_MOD_X_TILED: > + case I915_FORMAT_MOD_Y_TILED: > + return true; > + default: > + return false; > + } > +} > + > static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > const struct intel_crtc_state *crtc_state, > const struct intel_plane_state *plane_state) > @@ -672,6 +691,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > > cache->fb.format = fb->format; > cache->fb.stride = fb->pitches[0]; > + cache->fb.modifier = fb->modifier; > > drm_WARN_ON(&dev_priv->drm, plane_state->flags & PLANE_HAS_FENCE && > !plane_state->vma->fence); > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > return false; > } > > - /* The use of a CPU fence is mandatory in order to detect writes > - * by the CPU to the scanout and trigger updates to the FBC. > + /* The use of a CPU fence is one of two ways to detect writes by the > + * CPU to the scanout and trigger updates to the FBC. > + * > + * The other method is by software tracking(see > + * intel_fbc_invalidate/flush()), it will manually notify FBC and nuke > + * the current compressed buffer and recompress it. > * > * Note that is possible for a tiled surface to be unmappable (and > - * so have no fence associated with it) due to aperture constaints > + * so have no fence associated with it) due to aperture constraints > * at the time of pinning. > * > * FIXME with 90/270 degree rotation we should use the fence on > * the normal GTT view (the rotated view doesn't even have a > * fence). Would need changes to the FBC fence Y offset as well. > - * For now this will effecively disable FBC with 90/270 degree > + * For now this will effectively disable FBC with 90/270 degree > * rotation. > */ > - if (cache->fence_id < 0) { > + if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) { > fbc->no_fbc_reason = "framebuffer not tiled or fenced"; > return false; > } > + > + if (!tiling_is_valid(dev_priv, cache->fb.modifier)) { > + fbc->no_fbc_reason = "tiling unsupported"; > + return false; > + } > + > if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) && > cache->plane.rotation != DRM_MODE_ROTATE_0) { > fbc->no_fbc_reason = "rotation unsupported"; > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > return false; > } > > - if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) { > + if (!pixel_format_is_valid(dev_priv, cache->fb.format->format, > + cache->plane.rotation)) { > fbc->no_fbc_reason = "pixel format is invalid"; > return false; > } > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 3330b538d379..bf88663d8217 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -413,6 +413,7 @@ struct intel_fbc { > struct { > const struct drm_format_info *format; > unsigned int stride; > + u64 modifier; > } fb; > u16 gen9_wa_cfb_stride; > s8 fence_id; > -- > 2.25.1
On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote: > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza > wrote: > > dGFX have local memory so it do not have aperture and do not > > support > > CPU fences but even for iGFX it have a small number of fences. > > > > As replacement for fences to track frontbuffer modifications by CPU > > we have a software tracking that is already in used by FBC and PSR. > > PSR don't support fences so it shows that this tracking is > > reliable. > > > > So lets make fences a nice-to-have to activate FBC for GEN9+, this > > will allow us to enable FBC for dGFXs and iGFXs even when there is > > no > > available fence. > > > > We do not set fences to rotated planes but FBC only have > > restrictions > > against 16bpp, so adding it here. > > > > Also adding a new check for the tiling format, fences are only set > > to X and Y tiled planes but again FBC don't have any restrictions > > against tiling so adding linear as supported as well, other formats > > should be added after tested but IGT only supports drawing in thse > > 3 formats. > > > > intel_fbc_hw_tracking_covers_screen() maybe can also have the same > > treatment as fences but BSpec is not clear if the size limitation > > is > > for hardware tracking or general use of FBC and I don't have a 5K > > display to test it, so keeping as is for safety. > > > > v2: > > - Added tiling and pixel format rotation checks > > - Changed the GEN version not requiring fences to 11 from 9, DDX > > needs some changes but it don't have support for GEN11+ > > > > v3: > > - Changed back to GEN9+ > > - Moved GEN test to inside of tiling_is_valid() > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 45 > > ++++++++++++++++++++---- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > 2 files changed, 39 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > index 1d76e3646a25..a0d1d661a006 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct > > drm_i915_private *dev_priv, > > } > > > > static bool pixel_format_is_valid(struct drm_i915_private > > *dev_priv, > > - u32 pixel_format) > > + u32 pixel_format, unsigned int > > rotation) > > { > > switch (pixel_format) { > > case DRM_FORMAT_XRGB8888: > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct > > drm_i915_private *dev_priv, > > /* WaFbcOnly1to1Ratio:ctg */ > > if (IS_G4X(dev_priv)) > > return false; > > + if ((rotation & (DRM_MODE_ROTATE_90 | > > DRM_MODE_ROTATE_270)) && > > + INTEL_GEN(dev_priv) >= 9) > > + return false; > > Would still would prefer a rotations_is_valid() or some such thing. Like this? diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index 5e35c894bdf9..692edd45b769 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct drm_i915_private *dev_priv, /* WaFbcOnly1to1Ratio:ctg */ if (IS_G4X(dev_priv)) return false; - if ((rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) && - INTEL_GEN(dev_priv) >= 9) - return false; return true; default: return false; } } +static bool rotations_is_valid(struct drm_i915_private *dev_priv, + u32 pixel_format, unsigned int rotation) +{ + if (INTEL_GEN(dev_priv) >= 9 && pixel_format == DRM_FORMAT_RGB565 && + rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) + return false; + return true; +} + /* * For some reason, the hardware tracking starts looking at whatever we * programmed as the display plane base address register. It does not look at @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) return false; } + if (!rotations_is_valid(dev_priv, cache->fb.format->format, + cache->plane.rotation)) { + fbc->no_fbc_reason = "plane rotation 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"; > > > return true; > > default: > > return false; > > @@ -639,6 +642,22 @@ static bool > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > return effective_w <= max_w && effective_h <= max_h; > > } > > > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv, > > + uint64_t modifier) > > +{ > > + switch (modifier) { > > + case DRM_FORMAT_MOD_LINEAR: > > + if (INTEL_GEN(dev_priv) >= 9) > > + return true; > > Have we checked that eg. fbcon cursor still blinks correctly > with FBC active and all? Hum on fbcon FBC is enabled but it never compress, IGT tests with fences complete disables are working fine, screen is updated when tests asks to with FBC enabled and compressing. I will debug fbcon a little more to understand why it is never compressing. > > > + return false; > > + case I915_FORMAT_MOD_X_TILED: > > + case I915_FORMAT_MOD_Y_TILED: > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > > const struct intel_crtc_state > > *crtc_state, > > const struct intel_plane_state > > *plane_state) > > @@ -672,6 +691,7 @@ static void intel_fbc_update_state_cache(struct > > intel_crtc *crtc, > > > > cache->fb.format = fb->format; > > cache->fb.stride = fb->pitches[0]; > > + cache->fb.modifier = fb->modifier; > > > > drm_WARN_ON(&dev_priv->drm, plane_state->flags & > > PLANE_HAS_FENCE && > > !plane_state->vma->fence); > > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct > > intel_crtc *crtc) > > return false; > > } > > > > - /* The use of a CPU fence is mandatory in order to detect > > writes > > - * by the CPU to the scanout and trigger updates to the FBC. > > + /* The use of a CPU fence is one of two ways to detect writes > > by the > > + * CPU to the scanout and trigger updates to the FBC. > > + * > > + * The other method is by software tracking(see > > + * intel_fbc_invalidate/flush()), it will manually notify FBC > > and nuke > > + * the current compressed buffer and recompress it. > > * > > * Note that is possible for a tiled surface to be unmappable > > (and > > - * so have no fence associated with it) due to aperture > > constaints > > + * so have no fence associated with it) due to aperture > > constraints > > * at the time of pinning. > > * > > * FIXME with 90/270 degree rotation we should use the fence on > > * the normal GTT view (the rotated view doesn't even have a > > * fence). Would need changes to the FBC fence Y offset as > > well. > > - * For now this will effecively disable FBC with 90/270 degree > > + * For now this will effectively disable FBC with 90/270 degree > > * rotation. > > */ > > - if (cache->fence_id < 0) { > > + if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) { > > fbc->no_fbc_reason = "framebuffer not tiled or fenced"; > > return false; > > } > > + > > + if (!tiling_is_valid(dev_priv, cache->fb.modifier)) { > > + fbc->no_fbc_reason = "tiling unsupported"; > > + return false; > > + } > > + > > if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) && > > cache->plane.rotation != DRM_MODE_ROTATE_0) { > > fbc->no_fbc_reason = "rotation unsupported"; > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct > > intel_crtc *crtc) > > return false; > > } > > > > - if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) > > { > > + if (!pixel_format_is_valid(dev_priv, cache->fb.format->format, > > + cache->plane.rotation)) { > > fbc->no_fbc_reason = "pixel format is invalid"; > > return false; > > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 3330b538d379..bf88663d8217 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -413,6 +413,7 @@ struct intel_fbc { > > struct { > > const struct drm_format_info *format; > > unsigned int stride; > > + u64 modifier; > > } fb; > > u16 gen9_wa_cfb_stride; > > s8 fence_id; > > -- > > 2.25.1
On Thu, 2020-03-05 at 17:33 -0800, José Roberto de Souza wrote: > On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote: > > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza > > wrote: > > > dGFX have local memory so it do not have aperture and do not > > > support > > > CPU fences but even for iGFX it have a small number of fences. > > > > > > As replacement for fences to track frontbuffer modifications by > > > CPU > > > we have a software tracking that is already in used by FBC and > > > PSR. > > > PSR don't support fences so it shows that this tracking is > > > reliable. > > > > > > So lets make fences a nice-to-have to activate FBC for GEN9+, > > > this > > > will allow us to enable FBC for dGFXs and iGFXs even when there > > > is > > > no > > > available fence. > > > > > > We do not set fences to rotated planes but FBC only have > > > restrictions > > > against 16bpp, so adding it here. > > > > > > Also adding a new check for the tiling format, fences are only > > > set > > > to X and Y tiled planes but again FBC don't have any restrictions > > > against tiling so adding linear as supported as well, other > > > formats > > > should be added after tested but IGT only supports drawing in > > > thse > > > 3 formats. > > > > > > intel_fbc_hw_tracking_covers_screen() maybe can also have the > > > same > > > treatment as fences but BSpec is not clear if the size limitation > > > is > > > for hardware tracking or general use of FBC and I don't have a 5K > > > display to test it, so keeping as is for safety. > > > > > > v2: > > > - Added tiling and pixel format rotation checks > > > - Changed the GEN version not requiring fences to 11 from 9, DDX > > > needs some changes but it don't have support for GEN11+ > > > > > > v3: > > > - Changed back to GEN9+ > > > - Moved GEN test to inside of tiling_is_valid() > > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_fbc.c | 45 > > > ++++++++++++++++++++---- > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > 2 files changed, 39 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index 1d76e3646a25..a0d1d661a006 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct > > > drm_i915_private *dev_priv, > > > } > > > > > > static bool pixel_format_is_valid(struct drm_i915_private > > > *dev_priv, > > > - u32 pixel_format) > > > + u32 pixel_format, unsigned int > > > rotation) > > > { > > > switch (pixel_format) { > > > case DRM_FORMAT_XRGB8888: > > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct > > > drm_i915_private *dev_priv, > > > /* WaFbcOnly1to1Ratio:ctg */ > > > if (IS_G4X(dev_priv)) > > > return false; > > > + if ((rotation & (DRM_MODE_ROTATE_90 | > > > DRM_MODE_ROTATE_270)) && > > > + INTEL_GEN(dev_priv) >= 9) > > > + return false; > > > > Would still would prefer a rotations_is_valid() or some such thing. > > Like this? > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > b/drivers/gpu/drm/i915/display/intel_fbc.c > index 5e35c894bdf9..692edd45b769 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct > drm_i915_private *dev_priv, > /* WaFbcOnly1to1Ratio:ctg */ > if (IS_G4X(dev_priv)) > return false; > - if ((rotation & (DRM_MODE_ROTATE_90 | > DRM_MODE_ROTATE_270)) && > - INTEL_GEN(dev_priv) >= 9) > - return false; > return true; > default: > return false; > } > } > > +static bool rotations_is_valid(struct drm_i915_private *dev_priv, > + u32 pixel_format, unsigned int > rotation) > +{ > + if (INTEL_GEN(dev_priv) >= 9 && pixel_format == > DRM_FORMAT_RGB565 && > + rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) > + return false; > + return true; > +} > + > /* > * For some reason, the hardware tracking starts looking at whatever > we > * programmed as the display plane base address register. It does > not > look at > @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct > intel_crtc *crtc) > return false; > } > > + if (!rotations_is_valid(dev_priv, cache->fb.format->format, > + cache->plane.rotation)) { > + fbc->no_fbc_reason = "plane rotation 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"; > > > > > return true; > > > default: > > > return false; > > > @@ -639,6 +642,22 @@ static bool > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > > return effective_w <= max_w && effective_h <= max_h; > > > } > > > > > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv, > > > + uint64_t modifier) > > > +{ > > > + switch (modifier) { > > > + case DRM_FORMAT_MOD_LINEAR: > > > + if (INTEL_GEN(dev_priv) >= 9) > > > + return true; > > > > Have we checked that eg. fbcon cursor still blinks correctly > > with FBC active and all? > > Hum on fbcon FBC is enabled but it never compress, IGT tests with > fences complete disables are working fine, screen is updated when > tests > asks to with FBC enabled and compressing. > > I will debug fbcon a little more to understand why it is never > compressing. Actually is is compressing but is very rare, I even reduced the blink rate to 1hz but it did not helped. I have compared the plane registers between fbcon and IGT both in tiling linear but the only difference is the plane surface address: # fbcon [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A [drm:skl_update_plane [i915]] plane_stride=78 [drm:skl_update_plane [i915]] plane_pos=0 [drm:skl_update_plane [i915]] plane_size=437077f [drm:skl_update_plane [i915]] plane_aux_dist=0 [drm:skl_update_plane [i915]] plane_color_ctl=2000 [drm:skl_update_plane [i915]] plane_keyval=0 [drm:skl_update_plane [i915]] plane_keymsk=0 [drm:skl_update_plane [i915]] plane_keymax=ff000000 [drm:skl_update_plane [i915]] plane_offset=0 [drm:skl_update_plane [i915]] plane_ctl=84000000 [drm:skl_update_plane [i915]] plane_surf=40000 # IGT test with tiling linear framebuffer [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A [drm:skl_update_plane [i915]] plane_stride=78 [drm:skl_update_plane [i915]] plane_pos=0 [drm:skl_update_plane [i915]] plane_size=437077f [drm:skl_update_plane [i915]] plane_aux_dist=0 [drm:skl_update_plane [i915]] plane_color_ctl=2000 [drm:skl_update_plane [i915]] plane_keyval=0 [drm:skl_update_plane [i915]] plane_keymsk=0 [drm:skl_update_plane [i915]] plane_keymax=ff000000 [drm:skl_update_plane [i915]] plane_offset=0 [drm:skl_update_plane [i915]] plane_ctl=84000000 [drm:skl_update_plane [i915]] plane_surf=1040000 Do you have any tips of what could be causing this? > > > > + return false; > > > + case I915_FORMAT_MOD_X_TILED: > > > + case I915_FORMAT_MOD_Y_TILED: > > > + return true; > > > + default: > > > + return false; > > > + } > > > +} > > > + > > > static void intel_fbc_update_state_cache(struct intel_crtc > > > *crtc, > > > const struct intel_crtc_state > > > *crtc_state, > > > const struct intel_plane_state > > > *plane_state) > > > @@ -672,6 +691,7 @@ static void > > > intel_fbc_update_state_cache(struct > > > intel_crtc *crtc, > > > > > > cache->fb.format = fb->format; > > > cache->fb.stride = fb->pitches[0]; > > > + cache->fb.modifier = fb->modifier; > > > > > > drm_WARN_ON(&dev_priv->drm, plane_state->flags & > > > PLANE_HAS_FENCE && > > > !plane_state->vma->fence); > > > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct > > > intel_crtc *crtc) > > > return false; > > > } > > > > > > - /* The use of a CPU fence is mandatory in order to detect > > > writes > > > - * by the CPU to the scanout and trigger updates to the FBC. > > > + /* The use of a CPU fence is one of two ways to detect writes > > > by the > > > + * CPU to the scanout and trigger updates to the FBC. > > > + * > > > + * The other method is by software tracking(see > > > + * intel_fbc_invalidate/flush()), it will manually notify FBC > > > and nuke > > > + * the current compressed buffer and recompress it. > > > * > > > * Note that is possible for a tiled surface to be unmappable > > > (and > > > - * so have no fence associated with it) due to aperture > > > constaints > > > + * so have no fence associated with it) due to aperture > > > constraints > > > * at the time of pinning. > > > * > > > * FIXME with 90/270 degree rotation we should use the fence on > > > * the normal GTT view (the rotated view doesn't even have a > > > * fence). Would need changes to the FBC fence Y offset as > > > well. > > > - * For now this will effecively disable FBC with 90/270 degree > > > + * For now this will effectively disable FBC with 90/270 degree > > > * rotation. > > > */ > > > - if (cache->fence_id < 0) { > > > + if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) { > > > fbc->no_fbc_reason = "framebuffer not tiled or fenced"; > > > return false; > > > } > > > + > > > + if (!tiling_is_valid(dev_priv, cache->fb.modifier)) { > > > + fbc->no_fbc_reason = "tiling unsupported"; > > > + return false; > > > + } > > > + > > > if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) && > > > cache->plane.rotation != DRM_MODE_ROTATE_0) { > > > fbc->no_fbc_reason = "rotation unsupported"; > > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct > > > intel_crtc *crtc) > > > return false; > > > } > > > > > > - if (!pixel_format_is_valid(dev_priv, cache->fb.format- > > > >format)) > > > { > > > + if (!pixel_format_is_valid(dev_priv, cache->fb.format->format, > > > + cache->plane.rotation)) { > > > fbc->no_fbc_reason = "pixel format is invalid"; > > > return false; > > > } > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index 3330b538d379..bf88663d8217 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -413,6 +413,7 @@ struct intel_fbc { > > > struct { > > > const struct drm_format_info *format; > > > unsigned int stride; > > > + u64 modifier; > > > } fb; > > > u16 gen9_wa_cfb_stride; > > > s8 fence_id; > > > -- > > > 2.25.1
On Thu, Mar 12, 2020 at 12:35:56AM +0000, Souza, Jose wrote: > On Thu, 2020-03-05 at 17:33 -0800, José Roberto de Souza wrote: > > On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote: > > > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza > > > wrote: > > > > dGFX have local memory so it do not have aperture and do not > > > > support > > > > CPU fences but even for iGFX it have a small number of fences. > > > > > > > > As replacement for fences to track frontbuffer modifications by > > > > CPU > > > > we have a software tracking that is already in used by FBC and > > > > PSR. > > > > PSR don't support fences so it shows that this tracking is > > > > reliable. > > > > > > > > So lets make fences a nice-to-have to activate FBC for GEN9+, > > > > this > > > > will allow us to enable FBC for dGFXs and iGFXs even when there > > > > is > > > > no > > > > available fence. > > > > > > > > We do not set fences to rotated planes but FBC only have > > > > restrictions > > > > against 16bpp, so adding it here. > > > > > > > > Also adding a new check for the tiling format, fences are only > > > > set > > > > to X and Y tiled planes but again FBC don't have any restrictions > > > > against tiling so adding linear as supported as well, other > > > > formats > > > > should be added after tested but IGT only supports drawing in > > > > thse > > > > 3 formats. > > > > > > > > intel_fbc_hw_tracking_covers_screen() maybe can also have the > > > > same > > > > treatment as fences but BSpec is not clear if the size limitation > > > > is > > > > for hardware tracking or general use of FBC and I don't have a 5K > > > > display to test it, so keeping as is for safety. > > > > > > > > v2: > > > > - Added tiling and pixel format rotation checks > > > > - Changed the GEN version not requiring fences to 11 from 9, DDX > > > > needs some changes but it don't have support for GEN11+ > > > > > > > > v3: > > > > - Changed back to GEN9+ > > > > - Moved GEN test to inside of tiling_is_valid() > > > > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 45 > > > > ++++++++++++++++++++---- > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > 2 files changed, 39 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > index 1d76e3646a25..a0d1d661a006 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct > > > > drm_i915_private *dev_priv, > > > > } > > > > > > > > static bool pixel_format_is_valid(struct drm_i915_private > > > > *dev_priv, > > > > - u32 pixel_format) > > > > + u32 pixel_format, unsigned int > > > > rotation) > > > > { > > > > switch (pixel_format) { > > > > case DRM_FORMAT_XRGB8888: > > > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct > > > > drm_i915_private *dev_priv, > > > > /* WaFbcOnly1to1Ratio:ctg */ > > > > if (IS_G4X(dev_priv)) > > > > return false; > > > > + if ((rotation & (DRM_MODE_ROTATE_90 | > > > > DRM_MODE_ROTATE_270)) && > > > > + INTEL_GEN(dev_priv) >= 9) > > > > + return false; > > > > > > Would still would prefer a rotations_is_valid() or some such thing. > > > > Like this? > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > index 5e35c894bdf9..692edd45b769 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct > > drm_i915_private *dev_priv, > > /* WaFbcOnly1to1Ratio:ctg */ > > if (IS_G4X(dev_priv)) > > return false; > > - if ((rotation & (DRM_MODE_ROTATE_90 | > > DRM_MODE_ROTATE_270)) && > > - INTEL_GEN(dev_priv) >= 9) > > - return false; > > return true; > > default: > > return false; > > } > > } > > > > +static bool rotations_is_valid(struct drm_i915_private *dev_priv, > > + u32 pixel_format, unsigned int > > rotation) > > +{ > > + if (INTEL_GEN(dev_priv) >= 9 && pixel_format == > > DRM_FORMAT_RGB565 && > > + rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) > > + return false; > > + return true; > > +} > > + > > /* > > * For some reason, the hardware tracking starts looking at whatever > > we > > * programmed as the display plane base address register. It does > > not > > look at > > @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct > > intel_crtc *crtc) > > return false; > > } > > > > + if (!rotations_is_valid(dev_priv, cache->fb.format->format, > > + cache->plane.rotation)) { > > + fbc->no_fbc_reason = "plane rotation 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"; > > > > > > > > return true; > > > > default: > > > > return false; > > > > @@ -639,6 +642,22 @@ static bool > > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > > > return effective_w <= max_w && effective_h <= max_h; > > > > } > > > > > > > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv, > > > > + uint64_t modifier) > > > > +{ > > > > + switch (modifier) { > > > > + case DRM_FORMAT_MOD_LINEAR: > > > > + if (INTEL_GEN(dev_priv) >= 9) > > > > + return true; > > > > > > Have we checked that eg. fbcon cursor still blinks correctly > > > with FBC active and all? > > > > Hum on fbcon FBC is enabled but it never compress, IGT tests with > > fences complete disables are working fine, screen is updated when > > tests > > asks to with FBC enabled and compressing. > > > > I will debug fbcon a little more to understand why it is never > > compressing. > > Actually is is compressing but is very rare, I even reduced the blink > rate to 1hz but it did not helped. Is FBC actually enabled all the time though? By "compressing" I take you mean the debugfs thing we have? I don't actually remember what that looks at. Number of compressed segments? Or something else? You're polling it fast enough to not simply suffer from poor sampling? > > I have compared the plane registers > between fbcon and IGT both in tiling linear but the only difference is > the plane surface address: > > # fbcon > [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A > [drm:skl_update_plane [i915]] plane_stride=78 > [drm:skl_update_plane [i915]] plane_pos=0 > [drm:skl_update_plane [i915]] plane_size=437077f > [drm:skl_update_plane [i915]] plane_aux_dist=0 > [drm:skl_update_plane [i915]] plane_color_ctl=2000 > [drm:skl_update_plane [i915]] plane_keyval=0 > [drm:skl_update_plane [i915]] plane_keymsk=0 > [drm:skl_update_plane [i915]] plane_keymax=ff000000 > [drm:skl_update_plane [i915]] plane_offset=0 > [drm:skl_update_plane [i915]] plane_ctl=84000000 > [drm:skl_update_plane [i915]] plane_surf=40000 > > # IGT test with tiling linear framebuffer > [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A > [drm:skl_update_plane [i915]] plane_stride=78 > [drm:skl_update_plane [i915]] plane_pos=0 > [drm:skl_update_plane [i915]] plane_size=437077f > [drm:skl_update_plane [i915]] plane_aux_dist=0 > [drm:skl_update_plane [i915]] plane_color_ctl=2000 > [drm:skl_update_plane [i915]] plane_keyval=0 > [drm:skl_update_plane [i915]] plane_keymsk=0 > [drm:skl_update_plane [i915]] plane_keymax=ff000000 > [drm:skl_update_plane [i915]] plane_offset=0 > [drm:skl_update_plane [i915]] plane_ctl=84000000 > [drm:skl_update_plane [i915]] plane_surf=1040000 > > Do you have any tips of what could be causing this? > > > > > > > + return false; > > > > + case I915_FORMAT_MOD_X_TILED: > > > > + case I915_FORMAT_MOD_Y_TILED: > > > > + return true; > > > > + default: > > > > + return false; > > > > + } > > > > +} > > > > + > > > > static void intel_fbc_update_state_cache(struct intel_crtc > > > > *crtc, > > > > const struct intel_crtc_state > > > > *crtc_state, > > > > const struct intel_plane_state > > > > *plane_state) > > > > @@ -672,6 +691,7 @@ static void > > > > intel_fbc_update_state_cache(struct > > > > intel_crtc *crtc, > > > > > > > > cache->fb.format = fb->format; > > > > cache->fb.stride = fb->pitches[0]; > > > > + cache->fb.modifier = fb->modifier; > > > > > > > > drm_WARN_ON(&dev_priv->drm, plane_state->flags & > > > > PLANE_HAS_FENCE && > > > > !plane_state->vma->fence); > > > > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct > > > > intel_crtc *crtc) > > > > return false; > > > > } > > > > > > > > - /* The use of a CPU fence is mandatory in order to detect > > > > writes > > > > - * by the CPU to the scanout and trigger updates to the FBC. > > > > + /* The use of a CPU fence is one of two ways to detect writes > > > > by the > > > > + * CPU to the scanout and trigger updates to the FBC. > > > > + * > > > > + * The other method is by software tracking(see > > > > + * intel_fbc_invalidate/flush()), it will manually notify FBC > > > > and nuke > > > > + * the current compressed buffer and recompress it. > > > > * > > > > * Note that is possible for a tiled surface to be unmappable > > > > (and > > > > - * so have no fence associated with it) due to aperture > > > > constaints > > > > + * so have no fence associated with it) due to aperture > > > > constraints > > > > * at the time of pinning. > > > > * > > > > * FIXME with 90/270 degree rotation we should use the fence on > > > > * the normal GTT view (the rotated view doesn't even have a > > > > * fence). Would need changes to the FBC fence Y offset as > > > > well. > > > > - * For now this will effecively disable FBC with 90/270 degree > > > > + * For now this will effectively disable FBC with 90/270 degree > > > > * rotation. > > > > */ > > > > - if (cache->fence_id < 0) { > > > > + if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) { > > > > fbc->no_fbc_reason = "framebuffer not tiled or fenced"; > > > > return false; > > > > } > > > > + > > > > + if (!tiling_is_valid(dev_priv, cache->fb.modifier)) { > > > > + fbc->no_fbc_reason = "tiling unsupported"; > > > > + return false; > > > > + } > > > > + > > > > if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) && > > > > cache->plane.rotation != DRM_MODE_ROTATE_0) { > > > > fbc->no_fbc_reason = "rotation unsupported"; > > > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct > > > > intel_crtc *crtc) > > > > return false; > > > > } > > > > > > > > - if (!pixel_format_is_valid(dev_priv, cache->fb.format- > > > > >format)) > > > > { > > > > + if (!pixel_format_is_valid(dev_priv, cache->fb.format->format, > > > > + cache->plane.rotation)) { > > > > fbc->no_fbc_reason = "pixel format is invalid"; > > > > return false; > > > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > index 3330b538d379..bf88663d8217 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -413,6 +413,7 @@ struct intel_fbc { > > > > struct { > > > > const struct drm_format_info *format; > > > > unsigned int stride; > > > > + u64 modifier; > > > > } fb; > > > > u16 gen9_wa_cfb_stride; > > > > s8 fence_id; > > > > -- > > > > 2.25.1
On Fri, Mar 06, 2020 at 01:32:12AM +0000, Souza, Jose wrote: > On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote: > > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza > > wrote: > > > dGFX have local memory so it do not have aperture and do not > > > support > > > CPU fences but even for iGFX it have a small number of fences. > > > > > > As replacement for fences to track frontbuffer modifications by CPU > > > we have a software tracking that is already in used by FBC and PSR. > > > PSR don't support fences so it shows that this tracking is > > > reliable. > > > > > > So lets make fences a nice-to-have to activate FBC for GEN9+, this > > > will allow us to enable FBC for dGFXs and iGFXs even when there is > > > no > > > available fence. > > > > > > We do not set fences to rotated planes but FBC only have > > > restrictions > > > against 16bpp, so adding it here. > > > > > > Also adding a new check for the tiling format, fences are only set > > > to X and Y tiled planes but again FBC don't have any restrictions > > > against tiling so adding linear as supported as well, other formats > > > should be added after tested but IGT only supports drawing in thse > > > 3 formats. > > > > > > intel_fbc_hw_tracking_covers_screen() maybe can also have the same > > > treatment as fences but BSpec is not clear if the size limitation > > > is > > > for hardware tracking or general use of FBC and I don't have a 5K > > > display to test it, so keeping as is for safety. > > > > > > v2: > > > - Added tiling and pixel format rotation checks > > > - Changed the GEN version not requiring fences to 11 from 9, DDX > > > needs some changes but it don't have support for GEN11+ > > > > > > v3: > > > - Changed back to GEN9+ > > > - Moved GEN test to inside of tiling_is_valid() > > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_fbc.c | 45 > > > ++++++++++++++++++++---- > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > 2 files changed, 39 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index 1d76e3646a25..a0d1d661a006 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct > > > drm_i915_private *dev_priv, > > > } > > > > > > static bool pixel_format_is_valid(struct drm_i915_private > > > *dev_priv, > > > - u32 pixel_format) > > > + u32 pixel_format, unsigned int > > > rotation) > > > { > > > switch (pixel_format) { > > > case DRM_FORMAT_XRGB8888: > > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct > > > drm_i915_private *dev_priv, > > > /* WaFbcOnly1to1Ratio:ctg */ > > > if (IS_G4X(dev_priv)) > > > return false; > > > + if ((rotation & (DRM_MODE_ROTATE_90 | > > > DRM_MODE_ROTATE_270)) && > > > + INTEL_GEN(dev_priv) >= 9) > > > + return false; > > > > Would still would prefer a rotations_is_valid() or some such thing. > > Like this? > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > b/drivers/gpu/drm/i915/display/intel_fbc.c > index 5e35c894bdf9..692edd45b769 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct > drm_i915_private *dev_priv, > /* WaFbcOnly1to1Ratio:ctg */ > if (IS_G4X(dev_priv)) > return false; > - if ((rotation & (DRM_MODE_ROTATE_90 | > DRM_MODE_ROTATE_270)) && > - INTEL_GEN(dev_priv) >= 9) > - return false; > return true; > default: > return false; > } > } > > +static bool rotations_is_valid(struct drm_i915_private *dev_priv, > + u32 pixel_format, unsigned int rotation) > +{ > + if (INTEL_GEN(dev_priv) >= 9 && pixel_format == > DRM_FORMAT_RGB565 && > + rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) drm_rotation_90_or_270() > + return false; > + return true; > +} > + > /* > * For some reason, the hardware tracking starts looking at whatever > we > * programmed as the display plane base address register. It does not > look at > @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct > intel_crtc *crtc) > return false; > } > > + if (!rotations_is_valid(dev_priv, cache->fb.format->format, > + cache->plane.rotation)) { > + fbc->no_fbc_reason = "plane rotation is invalid"; > + return false; > + } s/rotations/rotation/ + I'd move the pre-g4x rotation check into that function as well. With that Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > 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"; > > > > > > > return true; > > > default: > > > return false; > > > @@ -639,6 +642,22 @@ static bool > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > > return effective_w <= max_w && effective_h <= max_h; > > > } > > > > > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv, > > > + uint64_t modifier) > > > +{ > > > + switch (modifier) { > > > + case DRM_FORMAT_MOD_LINEAR: > > > + if (INTEL_GEN(dev_priv) >= 9) > > > + return true; > > > > Have we checked that eg. fbcon cursor still blinks correctly > > with FBC active and all? > > Hum on fbcon FBC is enabled but it never compress, IGT tests with > fences complete disables are working fine, screen is updated when tests > asks to with FBC enabled and compressing. > > I will debug fbcon a little more to understand why it is never > compressing. > > > > > > + return false; > > > + case I915_FORMAT_MOD_X_TILED: > > > + case I915_FORMAT_MOD_Y_TILED: > > > + return true; > > > + default: > > > + return false; > > > + } > > > +} > > > + > > > static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > > > const struct intel_crtc_state > > > *crtc_state, > > > const struct intel_plane_state > > > *plane_state) > > > @@ -672,6 +691,7 @@ static void intel_fbc_update_state_cache(struct > > > intel_crtc *crtc, > > > > > > cache->fb.format = fb->format; > > > cache->fb.stride = fb->pitches[0]; > > > + cache->fb.modifier = fb->modifier; > > > > > > drm_WARN_ON(&dev_priv->drm, plane_state->flags & > > > PLANE_HAS_FENCE && > > > !plane_state->vma->fence); > > > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct > > > intel_crtc *crtc) > > > return false; > > > } > > > > > > - /* The use of a CPU fence is mandatory in order to detect > > > writes > > > - * by the CPU to the scanout and trigger updates to the FBC. > > > + /* The use of a CPU fence is one of two ways to detect writes > > > by the > > > + * CPU to the scanout and trigger updates to the FBC. > > > + * > > > + * The other method is by software tracking(see > > > + * intel_fbc_invalidate/flush()), it will manually notify FBC > > > and nuke > > > + * the current compressed buffer and recompress it. > > > * > > > * Note that is possible for a tiled surface to be unmappable > > > (and > > > - * so have no fence associated with it) due to aperture > > > constaints > > > + * so have no fence associated with it) due to aperture > > > constraints > > > * at the time of pinning. > > > * > > > * FIXME with 90/270 degree rotation we should use the fence on > > > * the normal GTT view (the rotated view doesn't even have a > > > * fence). Would need changes to the FBC fence Y offset as > > > well. > > > - * For now this will effecively disable FBC with 90/270 degree > > > + * For now this will effectively disable FBC with 90/270 degree > > > * rotation. > > > */ > > > - if (cache->fence_id < 0) { > > > + if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) { > > > fbc->no_fbc_reason = "framebuffer not tiled or fenced"; > > > return false; > > > } > > > + > > > + if (!tiling_is_valid(dev_priv, cache->fb.modifier)) { > > > + fbc->no_fbc_reason = "tiling unsupported"; > > > + return false; > > > + } > > > + > > > if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) && > > > cache->plane.rotation != DRM_MODE_ROTATE_0) { > > > fbc->no_fbc_reason = "rotation unsupported"; > > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct > > > intel_crtc *crtc) > > > return false; > > > } > > > > > > - if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) > > > { > > > + if (!pixel_format_is_valid(dev_priv, cache->fb.format->format, > > > + cache->plane.rotation)) { > > > fbc->no_fbc_reason = "pixel format is invalid"; > > > return false; > > > } > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index 3330b538d379..bf88663d8217 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -413,6 +413,7 @@ struct intel_fbc { > > > struct { > > > const struct drm_format_info *format; > > > unsigned int stride; > > > + u64 modifier; > > > } fb; > > > u16 gen9_wa_cfb_stride; > > > s8 fence_id; > > > -- > > > 2.25.1
On Wed, 2020-03-18 at 14:44 +0200, Ville Syrjälä wrote: > On Thu, Mar 12, 2020 at 12:35:56AM +0000, Souza, Jose wrote: > > On Thu, 2020-03-05 at 17:33 -0800, José Roberto de Souza wrote: > > > On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote: > > > > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza > > > > wrote: > > > > > dGFX have local memory so it do not have aperture and do not > > > > > support > > > > > CPU fences but even for iGFX it have a small number of > > > > > fences. > > > > > > > > > > As replacement for fences to track frontbuffer modifications > > > > > by > > > > > CPU > > > > > we have a software tracking that is already in used by FBC > > > > > and > > > > > PSR. > > > > > PSR don't support fences so it shows that this tracking is > > > > > reliable. > > > > > > > > > > So lets make fences a nice-to-have to activate FBC for GEN9+, > > > > > this > > > > > will allow us to enable FBC for dGFXs and iGFXs even when > > > > > there > > > > > is > > > > > no > > > > > available fence. > > > > > > > > > > We do not set fences to rotated planes but FBC only have > > > > > restrictions > > > > > against 16bpp, so adding it here. > > > > > > > > > > Also adding a new check for the tiling format, fences are > > > > > only > > > > > set > > > > > to X and Y tiled planes but again FBC don't have any > > > > > restrictions > > > > > against tiling so adding linear as supported as well, other > > > > > formats > > > > > should be added after tested but IGT only supports drawing in > > > > > thse > > > > > 3 formats. > > > > > > > > > > intel_fbc_hw_tracking_covers_screen() maybe can also have the > > > > > same > > > > > treatment as fences but BSpec is not clear if the size > > > > > limitation > > > > > is > > > > > for hardware tracking or general use of FBC and I don't have > > > > > a 5K > > > > > display to test it, so keeping as is for safety. > > > > > > > > > > v2: > > > > > - Added tiling and pixel format rotation checks > > > > > - Changed the GEN version not requiring fences to 11 from 9, > > > > > DDX > > > > > needs some changes but it don't have support for GEN11+ > > > > > > > > > > v3: > > > > > - Changed back to GEN9+ > > > > > - Moved GEN test to inside of tiling_is_valid() > > > > > > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 45 > > > > > ++++++++++++++++++++---- > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > > 2 files changed, 39 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > index 1d76e3646a25..a0d1d661a006 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct > > > > > drm_i915_private *dev_priv, > > > > > } > > > > > > > > > > static bool pixel_format_is_valid(struct drm_i915_private > > > > > *dev_priv, > > > > > - u32 pixel_format) > > > > > + u32 pixel_format, unsigned > > > > > int > > > > > rotation) > > > > > { > > > > > switch (pixel_format) { > > > > > case DRM_FORMAT_XRGB8888: > > > > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct > > > > > drm_i915_private *dev_priv, > > > > > /* WaFbcOnly1to1Ratio:ctg */ > > > > > if (IS_G4X(dev_priv)) > > > > > return false; > > > > > + if ((rotation & (DRM_MODE_ROTATE_90 | > > > > > DRM_MODE_ROTATE_270)) && > > > > > + INTEL_GEN(dev_priv) >= 9) > > > > > + return false; > > > > > > > > Would still would prefer a rotations_is_valid() or some such > > > > thing. > > > > > > Like this? > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index 5e35c894bdf9..692edd45b769 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct > > > drm_i915_private *dev_priv, > > > /* WaFbcOnly1to1Ratio:ctg */ > > > if (IS_G4X(dev_priv)) > > > return false; > > > - if ((rotation & (DRM_MODE_ROTATE_90 | > > > DRM_MODE_ROTATE_270)) && > > > - INTEL_GEN(dev_priv) >= 9) > > > - return false; > > > return true; > > > default: > > > return false; > > > } > > > } > > > > > > +static bool rotations_is_valid(struct drm_i915_private > > > *dev_priv, > > > + u32 pixel_format, unsigned int > > > rotation) > > > +{ > > > + if (INTEL_GEN(dev_priv) >= 9 && pixel_format == > > > DRM_FORMAT_RGB565 && > > > + rotation & (DRM_MODE_ROTATE_90 | > > > DRM_MODE_ROTATE_270)) > > > + return false; > > > + return true; > > > +} > > > + > > > /* > > > * For some reason, the hardware tracking starts looking at > > > whatever > > > we > > > * programmed as the display plane base address register. It > > > does > > > not > > > look at > > > @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct > > > intel_crtc *crtc) > > > return false; > > > } > > > > > > + if (!rotations_is_valid(dev_priv, cache->fb.format- > > > >format, > > > + cache->plane.rotation)) { > > > + fbc->no_fbc_reason = "plane rotation 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"; > > > > > > > > > > > return true; > > > > > default: > > > > > return false; > > > > > @@ -639,6 +642,22 @@ static bool > > > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > > > > return effective_w <= max_w && effective_h <= max_h; > > > > > } > > > > > > > > > > +static bool tiling_is_valid(struct drm_i915_private > > > > > *dev_priv, > > > > > + uint64_t modifier) > > > > > +{ > > > > > + switch (modifier) { > > > > > + case DRM_FORMAT_MOD_LINEAR: > > > > > + if (INTEL_GEN(dev_priv) >= 9) > > > > > + return true; > > > > > > > > Have we checked that eg. fbcon cursor still blinks correctly > > > > with FBC active and all? > > > > > > Hum on fbcon FBC is enabled but it never compress, IGT tests with > > > fences complete disables are working fine, screen is updated when > > > tests > > > asks to with FBC enabled and compressing. > > > > > > I will debug fbcon a little more to understand why it is never > > > compressing. > > > > Actually is is compressing but is very rare, I even reduced the > > blink > > rate to 1hz but it did not helped. > > Is FBC actually enabled all the time though? By "compressing" I take > you mean the debugfs thing we have? I don't actually remember what > that looks at. Number of compressed segments? Or something else? Exacly that. > You're polling it fast enough to not simply suffer from poor > sampling? Hum in other platforms the compressing is more consistent. > > > I have compared the plane registers > > between fbcon and IGT both in tiling linear but the only difference > > is > > the plane surface address: > > > > # fbcon > > [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A > > [drm:skl_update_plane [i915]] plane_stride=78 > > [drm:skl_update_plane [i915]] plane_pos=0 > > [drm:skl_update_plane [i915]] plane_size=437077f > > [drm:skl_update_plane [i915]] plane_aux_dist=0 > > [drm:skl_update_plane [i915]] plane_color_ctl=2000 > > [drm:skl_update_plane [i915]] plane_keyval=0 > > [drm:skl_update_plane [i915]] plane_keymsk=0 > > [drm:skl_update_plane [i915]] plane_keymax=ff000000 > > [drm:skl_update_plane [i915]] plane_offset=0 > > [drm:skl_update_plane [i915]] plane_ctl=84000000 > > [drm:skl_update_plane [i915]] plane_surf=40000 > > > > # IGT test with tiling linear framebuffer > > [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A > > [drm:skl_update_plane [i915]] plane_stride=78 > > [drm:skl_update_plane [i915]] plane_pos=0 > > [drm:skl_update_plane [i915]] plane_size=437077f > > [drm:skl_update_plane [i915]] plane_aux_dist=0 > > [drm:skl_update_plane [i915]] plane_color_ctl=2000 > > [drm:skl_update_plane [i915]] plane_keyval=0 > > [drm:skl_update_plane [i915]] plane_keymsk=0 > > [drm:skl_update_plane [i915]] plane_keymax=ff000000 > > [drm:skl_update_plane [i915]] plane_offset=0 > > [drm:skl_update_plane [i915]] plane_ctl=84000000 > > [drm:skl_update_plane [i915]] plane_surf=1040000 > > > > Do you have any tips of what could be causing this? > > > > > > > + return false; > > > > > + case I915_FORMAT_MOD_X_TILED: > > > > > + case I915_FORMAT_MOD_Y_TILED: > > > > > + return true; > > > > > + default: > > > > > + return false; > > > > > + } > > > > > +} > > > > > + > > > > > static void intel_fbc_update_state_cache(struct intel_crtc > > > > > *crtc, > > > > > const struct > > > > > intel_crtc_state > > > > > *crtc_state, > > > > > const struct > > > > > intel_plane_state > > > > > *plane_state) > > > > > @@ -672,6 +691,7 @@ static void > > > > > intel_fbc_update_state_cache(struct > > > > > intel_crtc *crtc, > > > > > > > > > > cache->fb.format = fb->format; > > > > > cache->fb.stride = fb->pitches[0]; > > > > > + cache->fb.modifier = fb->modifier; > > > > > > > > > > drm_WARN_ON(&dev_priv->drm, plane_state->flags & > > > > > PLANE_HAS_FENCE && > > > > > !plane_state->vma->fence); > > > > > @@ -720,23 +740,33 @@ static bool > > > > > intel_fbc_can_activate(struct > > > > > intel_crtc *crtc) > > > > > return false; > > > > > } > > > > > > > > > > - /* The use of a CPU fence is mandatory in order to > > > > > detect > > > > > writes > > > > > - * by the CPU to the scanout and trigger updates to the > > > > > FBC. > > > > > + /* The use of a CPU fence is one of two ways to detect > > > > > writes > > > > > by the > > > > > + * CPU to the scanout and trigger updates to the FBC. > > > > > + * > > > > > + * The other method is by software tracking(see > > > > > + * intel_fbc_invalidate/flush()), it will manually > > > > > notify FBC > > > > > and nuke > > > > > + * the current compressed buffer and recompress it. > > > > > * > > > > > * Note that is possible for a tiled surface to be > > > > > unmappable > > > > > (and > > > > > - * so have no fence associated with it) due to aperture > > > > > constaints > > > > > + * so have no fence associated with it) due to aperture > > > > > constraints > > > > > * at the time of pinning. > > > > > * > > > > > * FIXME with 90/270 degree rotation we should use the > > > > > fence on > > > > > * the normal GTT view (the rotated view doesn't even > > > > > have a > > > > > * fence). Would need changes to the FBC fence Y offset > > > > > as > > > > > well. > > > > > - * For now this will effecively disable FBC with 90/270 > > > > > degree > > > > > + * For now this will effectively disable FBC with > > > > > 90/270 degree > > > > > * rotation. > > > > > */ > > > > > - if (cache->fence_id < 0) { > > > > > + if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) { > > > > > fbc->no_fbc_reason = "framebuffer not tiled or > > > > > fenced"; > > > > > return false; > > > > > } > > > > > + > > > > > + if (!tiling_is_valid(dev_priv, cache->fb.modifier)) { > > > > > + fbc->no_fbc_reason = "tiling unsupported"; > > > > > + return false; > > > > > + } > > > > > + > > > > > if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) && > > > > > cache->plane.rotation != DRM_MODE_ROTATE_0) { > > > > > fbc->no_fbc_reason = "rotation unsupported"; > > > > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct > > > > > intel_crtc *crtc) > > > > > return false; > > > > > } > > > > > > > > > > - if (!pixel_format_is_valid(dev_priv, cache->fb.format- > > > > > > format)) > > > > > { > > > > > + if (!pixel_format_is_valid(dev_priv, cache->fb.format- > > > > > >format, > > > > > + cache->plane.rotation)) { > > > > > fbc->no_fbc_reason = "pixel format is invalid"; > > > > > return false; > > > > > } > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > > index 3330b538d379..bf88663d8217 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > > @@ -413,6 +413,7 @@ struct intel_fbc { > > > > > struct { > > > > > const struct drm_format_info *format; > > > > > unsigned int stride; > > > > > + u64 modifier; > > > > > } fb; > > > > > u16 gen9_wa_cfb_stride; > > > > > s8 fence_id; > > > > > -- > > > > > 2.25.1
On Wed, 2020-03-18 at 15:30 +0200, Ville Syrjälä wrote: > On Fri, Mar 06, 2020 at 01:32:12AM +0000, Souza, Jose wrote: > > On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote: > > > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza > > > wrote: > > > > dGFX have local memory so it do not have aperture and do not > > > > support > > > > CPU fences but even for iGFX it have a small number of fences. > > > > > > > > As replacement for fences to track frontbuffer modifications by > > > > CPU > > > > we have a software tracking that is already in used by FBC and > > > > PSR. > > > > PSR don't support fences so it shows that this tracking is > > > > reliable. > > > > > > > > So lets make fences a nice-to-have to activate FBC for GEN9+, > > > > this > > > > will allow us to enable FBC for dGFXs and iGFXs even when there > > > > is > > > > no > > > > available fence. > > > > > > > > We do not set fences to rotated planes but FBC only have > > > > restrictions > > > > against 16bpp, so adding it here. > > > > > > > > Also adding a new check for the tiling format, fences are only > > > > set > > > > to X and Y tiled planes but again FBC don't have any > > > > restrictions > > > > against tiling so adding linear as supported as well, other > > > > formats > > > > should be added after tested but IGT only supports drawing in > > > > thse > > > > 3 formats. > > > > > > > > intel_fbc_hw_tracking_covers_screen() maybe can also have the > > > > same > > > > treatment as fences but BSpec is not clear if the size > > > > limitation > > > > is > > > > for hardware tracking or general use of FBC and I don't have a > > > > 5K > > > > display to test it, so keeping as is for safety. > > > > > > > > v2: > > > > - Added tiling and pixel format rotation checks > > > > - Changed the GEN version not requiring fences to 11 from 9, > > > > DDX > > > > needs some changes but it don't have support for GEN11+ > > > > > > > > v3: > > > > - Changed back to GEN9+ > > > > - Moved GEN test to inside of tiling_is_valid() > > > > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 45 > > > > ++++++++++++++++++++---- > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > 2 files changed, 39 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > index 1d76e3646a25..a0d1d661a006 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct > > > > drm_i915_private *dev_priv, > > > > } > > > > > > > > static bool pixel_format_is_valid(struct drm_i915_private > > > > *dev_priv, > > > > - u32 pixel_format) > > > > + u32 pixel_format, unsigned > > > > int > > > > rotation) > > > > { > > > > switch (pixel_format) { > > > > case DRM_FORMAT_XRGB8888: > > > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct > > > > drm_i915_private *dev_priv, > > > > /* WaFbcOnly1to1Ratio:ctg */ > > > > if (IS_G4X(dev_priv)) > > > > return false; > > > > + if ((rotation & (DRM_MODE_ROTATE_90 | > > > > DRM_MODE_ROTATE_270)) && > > > > + INTEL_GEN(dev_priv) >= 9) > > > > + return false; > > > > > > Would still would prefer a rotations_is_valid() or some such > > > thing. > > > > Like this? > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > index 5e35c894bdf9..692edd45b769 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct > > drm_i915_private *dev_priv, > > /* WaFbcOnly1to1Ratio:ctg */ > > if (IS_G4X(dev_priv)) > > return false; > > - if ((rotation & (DRM_MODE_ROTATE_90 | > > DRM_MODE_ROTATE_270)) && > > - INTEL_GEN(dev_priv) >= 9) > > - return false; > > return true; > > default: > > return false; > > } > > } > > > > +static bool rotations_is_valid(struct drm_i915_private *dev_priv, > > + u32 pixel_format, unsigned int > > rotation) > > +{ > > + if (INTEL_GEN(dev_priv) >= 9 && pixel_format == > > DRM_FORMAT_RGB565 && > > + rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) > > drm_rotation_90_or_270() Done > > > + return false; > > + return true; > > +} > > + > > /* > > * For some reason, the hardware tracking starts looking at > > whatever > > we > > * programmed as the display plane base address register. It does > > not > > look at > > @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct > > intel_crtc *crtc) > > return false; > > } > > > > + if (!rotations_is_valid(dev_priv, cache->fb.format->format, > > + cache->plane.rotation)) { > > + fbc->no_fbc_reason = "plane rotation is invalid"; > > + return false; > > + } > > s/rotations/rotation/ + I'd move the pre-g4x rotation check into > that function as well. With that Done and done > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks > > > + > > 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"; > > > > > > > > return true; > > > > default: > > > > return false; > > > > @@ -639,6 +642,22 @@ static bool > > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > > > return effective_w <= max_w && effective_h <= max_h; > > > > } > > > > > > > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv, > > > > + uint64_t modifier) > > > > +{ > > > > + switch (modifier) { > > > > + case DRM_FORMAT_MOD_LINEAR: > > > > + if (INTEL_GEN(dev_priv) >= 9) > > > > + return true; > > > > > > Have we checked that eg. fbcon cursor still blinks correctly > > > with FBC active and all? > > > > Hum on fbcon FBC is enabled but it never compress, IGT tests with > > fences complete disables are working fine, screen is updated when > > tests > > asks to with FBC enabled and compressing. > > > > I will debug fbcon a little more to understand why it is never > > compressing. > > > > > > + return false; > > > > + case I915_FORMAT_MOD_X_TILED: > > > > + case I915_FORMAT_MOD_Y_TILED: > > > > + return true; > > > > + default: > > > > + return false; > > > > + } > > > > +} > > > > + > > > > static void intel_fbc_update_state_cache(struct intel_crtc > > > > *crtc, > > > > const struct > > > > intel_crtc_state > > > > *crtc_state, > > > > const struct > > > > intel_plane_state > > > > *plane_state) > > > > @@ -672,6 +691,7 @@ static void > > > > intel_fbc_update_state_cache(struct > > > > intel_crtc *crtc, > > > > > > > > cache->fb.format = fb->format; > > > > cache->fb.stride = fb->pitches[0]; > > > > + cache->fb.modifier = fb->modifier; > > > > > > > > drm_WARN_ON(&dev_priv->drm, plane_state->flags & > > > > PLANE_HAS_FENCE && > > > > !plane_state->vma->fence); > > > > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct > > > > intel_crtc *crtc) > > > > return false; > > > > } > > > > > > > > - /* The use of a CPU fence is mandatory in order to > > > > detect > > > > writes > > > > - * by the CPU to the scanout and trigger updates to the > > > > FBC. > > > > + /* The use of a CPU fence is one of two ways to detect > > > > writes > > > > by the > > > > + * CPU to the scanout and trigger updates to the FBC. > > > > + * > > > > + * The other method is by software tracking(see > > > > + * intel_fbc_invalidate/flush()), it will manually > > > > notify FBC > > > > and nuke > > > > + * the current compressed buffer and recompress it. > > > > * > > > > * Note that is possible for a tiled surface to be > > > > unmappable > > > > (and > > > > - * so have no fence associated with it) due to aperture > > > > constaints > > > > + * so have no fence associated with it) due to aperture > > > > constraints > > > > * at the time of pinning. > > > > * > > > > * FIXME with 90/270 degree rotation we should use the > > > > fence on > > > > * the normal GTT view (the rotated view doesn't even > > > > have a > > > > * fence). Would need changes to the FBC fence Y offset > > > > as > > > > well. > > > > - * For now this will effecively disable FBC with 90/270 > > > > degree > > > > + * For now this will effectively disable FBC with > > > > 90/270 degree > > > > * rotation. > > > > */ > > > > - if (cache->fence_id < 0) { > > > > + if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) { > > > > fbc->no_fbc_reason = "framebuffer not tiled or > > > > fenced"; > > > > return false; > > > > } > > > > + > > > > + if (!tiling_is_valid(dev_priv, cache->fb.modifier)) { > > > > + fbc->no_fbc_reason = "tiling unsupported"; > > > > + return false; > > > > + } > > > > + > > > > if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) && > > > > cache->plane.rotation != DRM_MODE_ROTATE_0) { > > > > fbc->no_fbc_reason = "rotation unsupported"; > > > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct > > > > intel_crtc *crtc) > > > > return false; > > > > } > > > > > > > > - if (!pixel_format_is_valid(dev_priv, cache->fb.format- > > > > >format)) > > > > { > > > > + if (!pixel_format_is_valid(dev_priv, cache->fb.format- > > > > >format, > > > > + cache->plane.rotation)) { > > > > fbc->no_fbc_reason = "pixel format is invalid"; > > > > return false; > > > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > index 3330b538d379..bf88663d8217 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -413,6 +413,7 @@ struct intel_fbc { > > > > struct { > > > > const struct drm_format_info *format; > > > > unsigned int stride; > > > > + u64 modifier; > > > > } fb; > > > > u16 gen9_wa_cfb_stride; > > > > s8 fence_id; > > > > -- > > > > 2.25.1
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index 1d76e3646a25..a0d1d661a006 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -585,7 +585,7 @@ static bool stride_is_valid(struct drm_i915_private *dev_priv, } static bool pixel_format_is_valid(struct drm_i915_private *dev_priv, - u32 pixel_format) + u32 pixel_format, unsigned int rotation) { switch (pixel_format) { case DRM_FORMAT_XRGB8888: @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct drm_i915_private *dev_priv, /* WaFbcOnly1to1Ratio:ctg */ if (IS_G4X(dev_priv)) return false; + if ((rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) && + INTEL_GEN(dev_priv) >= 9) + return false; return true; default: return false; @@ -639,6 +642,22 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) return effective_w <= max_w && effective_h <= max_h; } +static bool tiling_is_valid(struct drm_i915_private *dev_priv, + uint64_t modifier) +{ + switch (modifier) { + case DRM_FORMAT_MOD_LINEAR: + if (INTEL_GEN(dev_priv) >= 9) + return true; + return false; + case I915_FORMAT_MOD_X_TILED: + case I915_FORMAT_MOD_Y_TILED: + return true; + default: + return false; + } +} + static void intel_fbc_update_state_cache(struct intel_crtc *crtc, const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state) @@ -672,6 +691,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, cache->fb.format = fb->format; cache->fb.stride = fb->pitches[0]; + cache->fb.modifier = fb->modifier; drm_WARN_ON(&dev_priv->drm, plane_state->flags & PLANE_HAS_FENCE && !plane_state->vma->fence); @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) return false; } - /* The use of a CPU fence is mandatory in order to detect writes - * by the CPU to the scanout and trigger updates to the FBC. + /* The use of a CPU fence is one of two ways to detect writes by the + * CPU to the scanout and trigger updates to the FBC. + * + * The other method is by software tracking(see + * intel_fbc_invalidate/flush()), it will manually notify FBC and nuke + * the current compressed buffer and recompress it. * * Note that is possible for a tiled surface to be unmappable (and - * so have no fence associated with it) due to aperture constaints + * so have no fence associated with it) due to aperture constraints * at the time of pinning. * * FIXME with 90/270 degree rotation we should use the fence on * the normal GTT view (the rotated view doesn't even have a * fence). Would need changes to the FBC fence Y offset as well. - * For now this will effecively disable FBC with 90/270 degree + * For now this will effectively disable FBC with 90/270 degree * rotation. */ - if (cache->fence_id < 0) { + if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) { fbc->no_fbc_reason = "framebuffer not tiled or fenced"; return false; } + + if (!tiling_is_valid(dev_priv, cache->fb.modifier)) { + fbc->no_fbc_reason = "tiling unsupported"; + return false; + } + if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) && cache->plane.rotation != DRM_MODE_ROTATE_0) { fbc->no_fbc_reason = "rotation unsupported"; @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) return false; } - if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) { + if (!pixel_format_is_valid(dev_priv, cache->fb.format->format, + cache->plane.rotation)) { fbc->no_fbc_reason = "pixel format is invalid"; return false; } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3330b538d379..bf88663d8217 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -413,6 +413,7 @@ struct intel_fbc { struct { const struct drm_format_info *format; unsigned int stride; + u64 modifier; } fb; u16 gen9_wa_cfb_stride; s8 fence_id;
dGFX have local memory so it do not have aperture and do not support CPU fences but even for iGFX it have a small number of fences. As replacement for fences to track frontbuffer modifications by CPU we have a software tracking that is already in used by FBC and PSR. PSR don't support fences so it shows that this tracking is reliable. So lets make fences a nice-to-have to activate FBC for GEN9+, this will allow us to enable FBC for dGFXs and iGFXs even when there is no available fence. We do not set fences to rotated planes but FBC only have restrictions against 16bpp, so adding it here. Also adding a new check for the tiling format, fences are only set to X and Y tiled planes but again FBC don't have any restrictions against tiling so adding linear as supported as well, other formats should be added after tested but IGT only supports drawing in thse 3 formats. intel_fbc_hw_tracking_covers_screen() maybe can also have the same treatment as fences but BSpec is not clear if the size limitation is for hardware tracking or general use of FBC and I don't have a 5K display to test it, so keeping as is for safety. v2: - Added tiling and pixel format rotation checks - Changed the GEN version not requiring fences to 11 from 9, DDX needs some changes but it don't have support for GEN11+ v3: - Changed back to GEN9+ - Moved GEN test to inside of tiling_is_valid() Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_fbc.c | 45 ++++++++++++++++++++---- drivers/gpu/drm/i915/i915_drv.h | 1 + 2 files changed, 39 insertions(+), 7 deletions(-)