| Message ID | 20240123090244.30025-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/fbc: Allow FBC with CCS modifiers on SKL+ | expand |
On Tue, Jan 23, 2024 at 11:02:44AM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Only display workarounds 0391 and 0475 call for disabling > FBC with render compression, and those are listed only for > pre-prod SKL steppings. So it should be safe to enable > FB+CCS on production hardware. > > AFAIK CCS is limited to 50% bandwidth reduction (perhaps > clear color can do better?). FBC can exceed that number > by quite a bit, given the right kind of framebuffer > contents. So piling on both kinds of compressions could > still make sense. yeap, I think so. The risk is to hit a workaround that is not ducumented in the BSpec cases after gen11... Uma, do you recall having seen lately any workaround with FBC and render compression? > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10125 > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index f17a1afb4929..b453fcbd67da 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -1087,18 +1087,7 @@ static bool i8xx_fbc_tiling_valid(const struct intel_plane_state *plane_state) > > static bool skl_fbc_tiling_valid(const struct intel_plane_state *plane_state) > { > - const struct drm_framebuffer *fb = plane_state->hw.fb; > - > - switch (fb->modifier) { > - case DRM_FORMAT_MOD_LINEAR: > - case I915_FORMAT_MOD_Y_TILED: > - case I915_FORMAT_MOD_Yf_TILED: > - case I915_FORMAT_MOD_4_TILED: > - case I915_FORMAT_MOD_X_TILED: > - return true; > - default: > - return false; > - } > + return true; we could also simply kill this function... the compiler does the right thing, but users navigating on the code needs to do an extra ctag/cscope inspections to see that it is a simple return. But well, the code do gets prettier with the function :) So, up to you: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > } > > static bool tiling_is_valid(const struct intel_plane_state *plane_state) > -- > 2.43.0 >
On Tue, Jan 23, 2024 at 05:44:06PM -0500, Rodrigo Vivi wrote: > On Tue, Jan 23, 2024 at 11:02:44AM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Only display workarounds 0391 and 0475 call for disabling > > FBC with render compression, and those are listed only for > > pre-prod SKL steppings. So it should be safe to enable > > FB+CCS on production hardware. > > > > AFAIK CCS is limited to 50% bandwidth reduction (perhaps > > clear color can do better?). FBC can exceed that number > > by quite a bit, given the right kind of framebuffer > > contents. So piling on both kinds of compressions could > > still make sense. > > yeap, I think so. > The risk is to hit a workaround that is not ducumented in the BSpec > cases after gen11... > > Uma, do you recall having seen lately any workaround with FBC > and render compression? > > > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10125 > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 13 +------------ > > 1 file changed, 1 insertion(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > index f17a1afb4929..b453fcbd67da 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -1087,18 +1087,7 @@ static bool i8xx_fbc_tiling_valid(const struct intel_plane_state *plane_state) > > > > static bool skl_fbc_tiling_valid(const struct intel_plane_state *plane_state) > > { > > - const struct drm_framebuffer *fb = plane_state->hw.fb; > > - > > - switch (fb->modifier) { > > - case DRM_FORMAT_MOD_LINEAR: > > - case I915_FORMAT_MOD_Y_TILED: > > - case I915_FORMAT_MOD_Yf_TILED: > > - case I915_FORMAT_MOD_4_TILED: > > - case I915_FORMAT_MOD_X_TILED: > > - return true; > > - default: > > - return false; > > - } > > + return true; > > we could also simply kill this function... the compiler does the right thing, > but users navigating on the code needs to do an extra ctag/cscope inspections > to see that it is a simple return. But well, the code do gets prettier with > the function :) I've been thinking of converting a bunch of this stuff to vfuncs, so keeping the function around in anticipation of that seemed semi-reasonable. > So, up to you: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Thanks. > > > } > > > > static bool tiling_is_valid(const struct intel_plane_state *plane_state) > > -- > > 2.43.0 > >
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Wednesday, January 24, 2024 9:13 AM > To: Vivi, Rodrigo <rodrigo.vivi@intel.com> > Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/i915/fbc: Allow FBC with CCS modifiers on SKL+ > > On Tue, Jan 23, 2024 at 05:44:06PM -0500, Rodrigo Vivi wrote: > > On Tue, Jan 23, 2024 at 11:02:44AM +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Only display workarounds 0391 and 0475 call for disabling FBC with > > > render compression, and those are listed only for pre-prod SKL > > > steppings. So it should be safe to enable > > > FB+CCS on production hardware. > > > > > > AFAIK CCS is limited to 50% bandwidth reduction (perhaps clear color > > > can do better?). FBC can exceed that number by quite a bit, given > > > the right kind of framebuffer contents. So piling on both kinds of > > > compressions could still make sense. > > > > yeap, I think so. > > The risk is to hit a workaround that is not ducumented in the BSpec > > cases after gen11... > > > > Uma, do you recall having seen lately any workaround with FBC and > > render compression? Sorry missed to reply earlier. I don't see any restriction called out on this, so we should be good enabling them both. Let's enable and uncover bugs if any
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index f17a1afb4929..b453fcbd67da 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -1087,18 +1087,7 @@ static bool i8xx_fbc_tiling_valid(const struct intel_plane_state *plane_state) static bool skl_fbc_tiling_valid(const struct intel_plane_state *plane_state) { - const struct drm_framebuffer *fb = plane_state->hw.fb; - - switch (fb->modifier) { - case DRM_FORMAT_MOD_LINEAR: - case I915_FORMAT_MOD_Y_TILED: - case I915_FORMAT_MOD_Yf_TILED: - case I915_FORMAT_MOD_4_TILED: - case I915_FORMAT_MOD_X_TILED: - return true; - default: - return false; - } + return true; } static bool tiling_is_valid(const struct intel_plane_state *plane_state)