Re: [PATCH] drm/tilcdc: Use standard drm_atomic_helper_commit

[Jyri Sarha <jsarha@ti.com>]

Thank you Daniel,
Now this works perfectly, all while I was on vacation.

On 08/07/2020 17:20, Daniel Vetter wrote:
> Gives us proper nonblocking support for free, and a pile of other
> things. The tilcdc code is simply old enough that it was never
> converted over, but was stuck forever with the copypasta from when it
> was initially merged.
> 
> The riskiest thing with this conversion is maybe that there's an issue
> with the vblank handling or vblank event handling, which will upset
> the modern commit support in atomic helpers. But from a cursory review
> drm_crtc_vblank_on/off is called in the right places, and the event
> handling also seems to exist (albeit with much hand-rolling and
> probably some races, could perhaps be converted over to
> drm_crtc_arm_vblank_event without any real loss).
> 
> Motivated by me not having to hand-roll the dma-fence annotations for
> this.
> 
> v2: Clear out crtc_state->event when we're handling the event, to
> avoid upsetting the helpers (reported by Jyri).
> 
> v3: Also send out even whent the crtc is getting disabled. Tilcdc looks a
> bit like conversion to simple display helpers would work out really
> nice.
> 

Probably. Should take a closer looks some day when I have time.

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>

Tested-by: Jyri Sarha <jsarha@ti.com>
Reviewed-by: Jyri Sarha <jsarha@ti.com>

> --
> From logs looks like we're not stuck when disabling the display, so I
> hacked in a bit of code for that too. Like mentioned above, tilcdc
> looks like a perfect candidate for simple display helpers, I think
> that would simplify a _lot_ of code here.
> -Daniel
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c  | 13 ++++++++
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c   | 47 +--------------------------
>  drivers/gpu/drm/tilcdc/tilcdc_plane.c |  8 +++--
>  3 files changed, 19 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index e9dd5e5cb4e7..1856962411c7 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -537,6 +537,18 @@ static void tilcdc_crtc_atomic_disable(struct drm_crtc *crtc,
>  	tilcdc_crtc_disable(crtc);
>  }
>  
> +static void tilcdc_crtc_atomic_flush(struct drm_crtc *crtc,
> +				     struct drm_crtc_state *old_state)
> +{
> +	if (!crtc->state->event)
> +		return;
> +
> +	spin_lock_irq(&crtc->dev->event_lock);
> +	drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +	crtc->state->event = NULL;
> +	spin_unlock_irq(&crtc->dev->event_lock);
> +}
> +
>  void tilcdc_crtc_shutdown(struct drm_crtc *crtc)
>  {
>  	tilcdc_crtc_off(crtc, true);
> @@ -822,6 +834,7 @@ static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
>  	.atomic_check	= tilcdc_crtc_atomic_check,
>  	.atomic_enable	= tilcdc_crtc_atomic_enable,
>  	.atomic_disable	= tilcdc_crtc_atomic_disable,
> +	.atomic_flush	= tilcdc_crtc_atomic_flush,
>  };
>  
>  void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 0d74a6443263..4f5fc3e87383 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -87,55 +87,10 @@ static int tilcdc_atomic_check(struct drm_device *dev,
>  	return ret;
>  }
>  
> -static int tilcdc_commit(struct drm_device *dev,
> -		  struct drm_atomic_state *state,
> -		  bool async)
> -{
> -	int ret;
> -
> -	ret = drm_atomic_helper_prepare_planes(dev, state);
> -	if (ret)
> -		return ret;
> -
> -	ret = drm_atomic_helper_swap_state(state, true);
> -	if (ret) {
> -		drm_atomic_helper_cleanup_planes(dev, state);
> -		return ret;
> -	}
> -
> -	/*
> -	 * Everything below can be run asynchronously without the need to grab
> -	 * any modeset locks at all under one condition: It must be guaranteed
> -	 * that the asynchronous work has either been cancelled (if the driver
> -	 * supports it, which at least requires that the framebuffers get
> -	 * cleaned up with drm_atomic_helper_cleanup_planes()) or completed
> -	 * before the new state gets committed on the software side with
> -	 * drm_atomic_helper_swap_state().
> -	 *
> -	 * This scheme allows new atomic state updates to be prepared and
> -	 * checked in parallel to the asynchronous completion of the previous
> -	 * update. Which is important since compositors need to figure out the
> -	 * composition of the next frame right after having submitted the
> -	 * current layout.
> -	 */
> -
> -	drm_atomic_helper_commit_modeset_disables(dev, state);
> -
> -	drm_atomic_helper_commit_planes(dev, state, 0);
> -
> -	drm_atomic_helper_commit_modeset_enables(dev, state);
> -
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> -
> -	drm_atomic_helper_cleanup_planes(dev, state);
> -
> -	return 0;
> -}
> -
>  static const struct drm_mode_config_funcs mode_config_funcs = {
>  	.fb_create = drm_gem_fb_create,
>  	.atomic_check = tilcdc_atomic_check,
> -	.atomic_commit = tilcdc_commit,
> +	.atomic_commit = drm_atomic_helper_commit,
>  };
>  
>  static void modeset_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> index 0d09b31ae759..2f681a713815 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> @@ -83,9 +83,11 @@ static void tilcdc_plane_atomic_update(struct drm_plane *plane,
>  	if (WARN_ON(!state->fb || !state->crtc->state))
>  		return;
>  
> -	tilcdc_crtc_update_fb(state->crtc,
> -			      state->fb,
> -			      state->crtc->state->event);
> +	if (tilcdc_crtc_update_fb(state->crtc,
> +				  state->fb,
> +				  state->crtc->state->event) == 0) {
> +		state->crtc->state->event = NULL;
> +	}
>  }
>  
>  static const struct drm_plane_helper_funcs plane_helper_funcs = {
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel