Re: [PATCH v2 13/15] drm/xe/display: Use async flip for flipping initial fb.

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

On Mon, Sep 30, 2024 at 09:57:47PM +0200, Maarten Lankhorst wrote:
> Since memirq requires a ggtt allocation, irqs will be initialised
> later. This means we cannot rely on drm_wait_for_vblank any more as
> it requires interrupts.
> 
> Fortunately there's still SURFLIVE
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/display/xe_plane_initial.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> index 4660db0aecb68..3fd0fe2dbc27a 100644
> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> @@ -17,6 +17,8 @@
>  #include "intel_fb_pin.h"
>  #include "intel_frontbuffer.h"
>  #include "intel_plane_initial.h"
> +#include "intel_uncore.h"
> +#include "skl_universal_plane_regs.h"
>  #include "xe_bo.h"
>  #include "xe_wa.h"
>  
> @@ -187,6 +189,7 @@ static void
>  intel_find_initial_plane_obj(struct intel_crtc *crtc,
>  			     struct intel_initial_plane_config plane_configs[])
>  {
> +	struct xe_device *xe = to_xe_device(crtc->base.dev);
>  	struct intel_initial_plane_config *plane_config =
>  		&plane_configs[crtc->pipe];
>  	struct intel_plane *plane =
> @@ -235,6 +238,14 @@ intel_find_initial_plane_obj(struct intel_crtc *crtc,
>  	plane_state->uapi.crtc = &crtc->base;
>  	intel_plane_copy_uapi_to_hw_state(plane_state, plane_state, crtc);
>  
> +	/* Flip async, we don't have interrupts yet */
> +	plane_state->ctl = intel_uncore_read(&xe->uncore, PLANE_CTL(plane->pipe, PLANE_1));
> +	plane->async_flip(plane, to_intel_crtc_state(crtc->base.state), plane_state, true);
> +
> +	/* Wait 40 ms (1 frame at 25 fps) for async flip to complete */
> +	if (intel_wait_for_register_fw(&xe->uncore, PLANE_SURFLIVE(plane->pipe, PLANE_1), XE_PAGE_SIZE - 1, vma->node->base.start, 40) < 0)
> +		drm_warn(&xe->drm, "async flip timed out\n");

Please wrap that in a function...

> +
>  	atomic_or(plane->frontbuffer_bit, &to_intel_frontbuffer(fb)->bits);
>  
>  	plane_config->vma = vma;
> @@ -292,9 +303,6 @@ void intel_initial_plane_config(struct drm_i915_private *i915)
>  		 */
>  		intel_find_initial_plane_obj(crtc, plane_configs);
>  
> -		if (i915->display.funcs.display->fixup_initial_plane_config(crtc, plane_config))
> -			intel_crtc_wait_for_next_vblank(crtc);

... and use it here instead of abusing async flips.

But perhaps it would actually be best to suck that vblank wait into
.fixup_plane_config(). That would keep the SURF write and SURFLIVE
read close by at least.

Although it would require some kind of "are interrupts enabled yet?"
check which will need some kind of abstract interface in the end,
but for now I guess for now we could go with a IS_ENABLED() check
since we already have one of those in skl_universal_plane.c.
Hmm, or I guess we could just pass in a parameter to
.fixup_initial_plane_config() indicating what to do, since we
currently call this from driver specific code anyway.

-- 
Ville Syrjälä
Intel