Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

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

On Fri, Sep 30, 2022 at 05:00:40PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 30, 2022 at 02:47:30PM +0100, Matthew Auld wrote:
> > For these types of display buffers, we need to able to CPU access some
> > part of the backing memory in prepare_plane_clear_colors(). As a result
> > we need to ensure we always place in the mappable part of lmem, which
> > becomes necessary on small-bar systems.
> > 
> > Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support")
> > Reported-by: Jianshui Yu <jianshui.yu@intel.com>
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Nirmoy Das <nirmoy.das@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fb_pin.c   | 11 ++++--
> >  drivers/gpu/drm/i915/gem/i915_gem_object.c    | 37 ++++++++++++++++++-
> >  drivers/gpu/drm/i915/gem/i915_gem_object.h    |  4 ++
> >  .../gpu/drm/i915/gem/i915_gem_object_types.h  |  3 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_ttm.c       |  5 ++-
> >  5 files changed, 53 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > index c86e5d4ee016..f83cf41ddd63 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > @@ -139,9 +139,14 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> >  	ret = i915_gem_object_lock(obj, &ww);
> >  	if (!ret && phys_cursor)
> >  		ret = i915_gem_object_attach_phys(obj, alignment);
> > -	else if (!ret && HAS_LMEM(dev_priv))
> > -		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
> > -	/* TODO: Do we need to sync when migration becomes async? */
> 
> Why is the TODO being removed?
> 
> > +	else if (!ret && HAS_LMEM(dev_priv)) {
> > +		unsigned int flags = obj->flags;
> > +
> 
> It might not be super obvious what is going on here, so maybe add
> a comment stating the CPU needs to read the clear color from the bo?
> 
> > +		if (intel_fb_rc_ccs_cc_plane(fb) >= 0)
> > +			flags &= ~I915_BO_ALLOC_GPU_ONLY;
> 
> Hmm. Do we require the clear color plane to be in the same bo as the
> rest of the planes? I know we require the main and aux to be in the
> same bo, but dunno why we would require that also of the clear color
> plane (apart from being lazy perhaps).

I guess we must since we call this only once for the whole fb.

-- 
Ville Syrjälä
Intel