[Intel-gfx] [RFC 2/7] drm/i915: Add support for async flips in I915

[B S, Karthik]

> -----Original Message-----
> From: Zanoni, Paulo R <paulo.r.zanoni at intel.com>
> Sent: Tuesday, March 10, 2020 4:48 AM
> To: B S, Karthik <karthik.b.s at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: ville.syrjala at linux.intel.com; Kulkarni, Vandita
> <vandita.kulkarni at intel.com>; Shankar, Uma <uma.shankar at intel.com>
> Subject: Re: [RFC 2/7] drm/i915: Add support for async flips in I915
> 
> Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> > Enable support for async flips in I915.
> > Set the Async Address Update Enable bit in plane ctl when async flip
> > is requested.
> >
> > Signed-off-by: Karthik B S <karthik.b.s at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 4 ++++
> >  drivers/gpu/drm/i915/i915_reg.h              | 1 +
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index dd47eb65b563..4ce9897f5c58 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4756,6 +4756,9 @@ u32 skl_plane_ctl(const struct intel_crtc_state
> *crtc_state,
> >  			plane_ctl |=
> PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE;
> >  	}
> >
> > +	if (crtc_state->uapi.async_flip)
> > +		plane_ctl |= PLANE_CTL_ASYNC_FLIP;
> > +
> >  	plane_ctl |= skl_plane_ctl_format(fb->format->format);
> >  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> >  	plane_ctl |= skl_plane_ctl_rotate(rotation &
> DRM_MODE_ROTATE_MASK);
> > @@ -17738,6 +17741,7 @@ static void intel_mode_config_init(struct
> > drm_i915_private *i915)
> >
> >  	mode_config->funcs = &intel_mode_funcs;
> >
> > +	mode_config->async_page_flip = true;
> 
> We should only enable the feature to user space after it has been fully
> implemented inside the Kernel. Think about the case where git-bisect
> decides to land at exactly this commit when someone is debugging a failure
> unrelated to async vblanks.
> 
> Also, when features have trivial on/off switches like the line above, it's
> better if the patch that enables the feature only contains the line that toggles
> the on/off switch. This way, if a revert is needed, we can just switch it to off
> without removing more code. Also, it enables us to land the rest of the code
> while keeping the feature off for stabilization.
> 
> Also, the line above is enabling the feature for every platform, which is
> probably not a goal of this series.

Agreed. Will make the on/off part a separate patch and also add a gen check for it.
> 
> 
> >  	/*
> >  	 * Maximum framebuffer dimensions, chosen to match
> >  	 * the maximum render engine surface size on gen4+.
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 80cf02a6eec1..42037aee9b78
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6794,6 +6794,7 @@ enum {
> >  #define   PLANE_CTL_TILED_X			(1 << 10)
> >  #define   PLANE_CTL_TILED_Y			(4 << 10)
> >  #define   PLANE_CTL_TILED_YF			(5 << 10)
> > +#define   PLANE_CTL_ASYNC_FLIP			(1 << 9)
> >  #define   PLANE_CTL_FLIP_HORIZONTAL		(1 << 8)
> >  #define   PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE	(1 << 4) /*
> TGL+ */
> >  #define   PLANE_CTL_ALPHA_MASK			(0x3 << 4) /* Pre-GLK
> */