From: "Souza, Jose" <jose.souza@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+
Date: Thu, 12 Mar 2020 00:35:56 +0000 [thread overview]
Message-ID: <baca2cdbd379190de3c3545b43a8be1a7310da90.camel@intel.com> (raw)
In-Reply-To: <647f33f4e86eb4edecd856b08d505e248afca1b9.camel@intel.com>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-03-12 0:36 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-19 1:42 [Intel-gfx] [PATCH v3 1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter José Roberto de Souza
2020-02-19 1:42 ` [Intel-gfx] [PATCH v3 2/3] drm/i915/display: Do not write in removed FBC fence registers José Roberto de Souza
2020-02-19 1:42 ` [Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+ José Roberto de Souza
2020-02-20 17:26 ` Ville Syrjälä
2020-03-06 1:32 ` Souza, Jose
2020-03-12 0:35 ` Souza, Jose [this message]
2020-03-18 12:44 ` Ville Syrjälä
2020-03-19 21:12 ` Souza, Jose
2020-03-18 13:30 ` Ville Syrjälä
2020-03-19 21:12 ` Souza, Jose
2020-02-19 5:38 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v3,1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter Patchwork
2020-02-19 6:02 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2020-02-19 13:37 ` [Intel-gfx] [PATCH v3 1/3] " Ville Syrjälä
2020-02-19 18:37 ` Souza, Jose
2020-02-19 18:52 ` Ville Syrjälä
2020-03-06 0:22 ` Souza, Jose
2020-03-06 14:22 ` Ville Syrjälä
2020-03-06 8:15 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [v3,1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter (rev2) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=baca2cdbd379190de3c3545b43a8be1a7310da90.camel@intel.com \
--to=jose.souza@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).