Re: [PATCH 2/9] drm/exynos: add atomic asynchronous commit

[Joonyoung Shim <jy0922.shim@samsung.com>]

On 06/03/2015 11:30 PM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> The atomic modesetting interfaces supports async commits that should be
> implemented by the drivers. If drm core requests an async commit
> exynos_atomic_commit() will schedule a work task to run the update later.
> It also waits to an update to finishes to schedule a new one.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c |  6 ++++
>  drivers/gpu/drm/exynos/exynos_drm_drv.h |  6 ++++
>  drivers/gpu/drm/exynos/exynos_drm_fb.c  | 51 ++++++++++++++++++++++++++-------
>  3 files changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 08b9a8c..8ccff36 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -60,6 +60,9 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
>  	if (!private)
>  		return -ENOMEM;
>  
> +	INIT_WORK(&private->work, exynos_drm_atomic_work);
> +	private->dev = dev;
> +
>  	dev_set_drvdata(dev->dev, dev);
>  	dev->dev_private = (void *)private;
>  
> @@ -140,6 +143,8 @@ err_free_private:
>  
>  static int exynos_drm_unload(struct drm_device *dev)
>  {
> +	struct exynos_drm_private *private = dev->dev_private;
> +
>  	exynos_drm_device_subdrv_remove(dev);
>  
>  	exynos_drm_fbdev_fini(dev);
> @@ -150,6 +155,7 @@ static int exynos_drm_unload(struct drm_device *dev)
>  	drm_mode_config_cleanup(dev);
>  	drm_release_iommu_mapping(dev);
>  
> +	flush_work(&private->work);
>  	kfree(dev->dev_private);
>  	dev->dev_private = NULL;
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 1c66f65..552ca4a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -255,6 +255,10 @@ struct exynos_drm_private {
>  	unsigned long da_space_size;
>  
>  	unsigned int pipe;
> +
> +	struct drm_device *dev;
> +	struct work_struct work;
> +	struct drm_atomic_state *state;
>  };
>  
>  /*
> @@ -324,6 +328,8 @@ static inline int exynos_drm_probe_vidi(void) { return 0; }
>  static inline void exynos_drm_remove_vidi(void) {}
>  #endif
>  
> +void exynos_drm_atomic_work(struct work_struct *work);
> +
>  /* This function creates a encoder and a connector, and initializes them. */
>  int exynos_drm_create_enc_conn(struct drm_device *dev,
>  				struct exynos_drm_display *display);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index 789db6f..28626db 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -267,20 +267,18 @@ static void exynos_drm_output_poll_changed(struct drm_device *dev)
>  		exynos_drm_fbdev_init(dev);
>  }
>  
> -static int exynos_atomic_commit(struct drm_device *dev,
> -				struct drm_atomic_state *state,
> -				bool async)
> +static void exynos_atomic_commit_schedule(struct drm_device *dev,
> +				struct drm_atomic_state *state)
>  {
> -	int ret;
> -
> -	ret = drm_atomic_helper_prepare_planes(dev, state);
> -	if (ret)
> -		return ret;
> -
> -	/* This is the point of no return */
> +	struct exynos_drm_private *private = dev->dev_private;
>  
> -	drm_atomic_helper_swap_state(dev, state);
> +	private->state = state;
> +	schedule_work(&private->work);
> +}
>  
> +static void exynos_atomic_commit_complete(struct drm_device *dev,
> +					  struct drm_atomic_state *state)
> +{
>  	drm_atomic_helper_commit_modeset_disables(dev, state);
>  
>  	drm_atomic_helper_commit_modeset_enables(dev, state);
> @@ -300,6 +298,37 @@ static int exynos_atomic_commit(struct drm_device *dev,
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  
>  	drm_atomic_state_free(state);
> +}
> +
> +void exynos_drm_atomic_work(struct work_struct *work)
> +{
> +	struct exynos_drm_private *private = container_of(work,
> +				struct exynos_drm_private, work);
> +
> +	exynos_atomic_commit_complete(private->dev, private->state);
> +}
> +
> +static int exynos_atomic_commit(struct drm_device *dev,
> +				struct drm_atomic_state *state,
> +				bool async)
> +{
> +	struct exynos_drm_private *private = dev->dev_private;
> +	int ret;
> +
> +	ret = drm_atomic_helper_prepare_planes(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	/* This is the point of no return */
> +
> +	drm_atomic_helper_swap_state(dev, state);
> +
> +	flush_work(&private->work);

It seems be wrong to swap state before flush work.

I'm not sure whether need locking but other drivers changed to atomic
modeset feature use locking here.

> +
> +	if (async)
> +		exynos_atomic_commit_schedule(dev, state);
> +	else
> +		exynos_atomic_commit_complete(dev, state);
>  
>  	return 0;
>  }
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel