Re: [PATCH 3/4] drm/xe/xe2: Limit ccs framebuffers to tile4 only

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 31, 2024 at 01:40:30PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 30, 2024 at 09:36:51PM +0200, Juha-Pekka Heikkila wrote:
> > Display engine support ccs only with tile4, prevent other modifiers
> > from using compressed memory.
> > 
> > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > ---
> >  drivers/gpu/drm/xe/display/xe_fb_pin.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > index 722c84a56607..fab0871f0cdf 100644
> > --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > @@ -10,9 +10,18 @@
> >  #include "intel_fb_pin.h"
> >  #include "xe_ggtt.h"
> >  #include "xe_gt.h"
> > +#include "xe_pat.h"
> >  
> >  #include <drm/ttm/ttm_bo.h>
> >  
> > +static bool is_compressed(const struct drm_framebuffer *fb)
> > +{
> > +	struct xe_bo *bo = intel_fb_obj(fb);
> > +	struct xe_device *xe = to_xe_device(to_intel_framebuffer(fb)->base.dev);
> > +
> > +	return xe_pat_index_has_compression(xe, bo->pat_index);
> > +}
> > +
> >  static void
> >  write_dpt_rotated(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs, u32 bo_ofs,
> >  		  u32 width, u32 height, u32 src_stride, u32 dst_stride)
> > @@ -283,6 +292,17 @@ static struct i915_vma *__xe_pin_fb_vma(struct intel_framebuffer *fb,
> >  	if (ret)
> >  		goto err;
> >  
> > +	if (GRAPHICS_VER(xe) >= 20) {
> > +		if (fb->base.modifier != I915_FORMAT_MOD_4_TILED &&
> > +		    is_compressed(&fb->base)) {
> > +			drm_warn(&xe->drm, "Cannot create ccs framebuffer with other than tile4 mofifier\n");
> > +			ttm_bo_unreserve(&bo->ttm);
> > +			ret = -EINVAL;
> > +			goto err;
> > +		}
> > +		bo->has_sealed_pat_index = true;
> 
> That needs to happen when the FB is created, otherwise 
> someone can come in afterwards and still frob the PAT
> and then you're left with a FB that can not be used.

Hmm, and there's already some kind of XE_BO_SCANOUT flag 
that seems to maybe do similar things? But that one seems
to be intended to be set when creating the bo, but then it
also gets abused internally by intel_fb_bo_framebuffer_init().
Seems to be a total mess.

> 
> > +	}
> > +
> >  	if (IS_DGFX(xe))
> >  		ret = xe_bo_migrate(bo, XE_PL_VRAM0);
> >  	else
> > @@ -308,6 +328,7 @@ static struct i915_vma *__xe_pin_fb_vma(struct intel_framebuffer *fb,
> >  	ttm_bo_unpin(&bo->ttm);
> >  	ttm_bo_unreserve(&bo->ttm);
> >  err:
> > +	bo->has_sealed_pat_index = false;
> >  	kfree(vma);
> >  	return ERR_PTR(ret);
> >  }
> > @@ -323,6 +344,8 @@ static void __xe_unpin_fb_vma(struct i915_vma *vma)
> >  		 vma->bo->ggtt_node.start != vma->node.start)
> >  		xe_ggtt_remove_node(ggtt, &vma->node);
> >  
> > +	vma->bo->has_sealed_pat_index = false;
> > +
> >  	ttm_bo_reserve(&vma->bo->ttm, false, false, NULL);
> >  	ttm_bo_unpin(&vma->bo->ttm);
> >  	ttm_bo_unreserve(&vma->bo->ttm);
> > -- 
> > 2.25.1
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel