[PATCH v6 17/18] drm: rcar-du: vsp: Extract framebuffer (un)mapping to separate functions

[Kieran Bingham]

Hi Laurent,

On 13/03/2019 00:05, Laurent Pinchart wrote:
> The rcar_du_vsp_plane_prepare_fb() and rcar_du_vsp_plane_cleanup_fb()
> functions implement the DRM plane .prepare_fb() and .cleanup_fb()
> operations. They map and unmap the framebuffer to/from the VSP
> internally, which will be useful to implement writeback support. Split
> the mapping and unmapping out to separate functions.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>

Looks good to me, Just a refactor and nothing controversial.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas at ideasonboard.com>



> ---
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 68 ++++++++++++++++-----------
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 17 +++++++
>  2 files changed, 58 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 29a08f7b0761..0806a69c4679 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -173,26 +173,16 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
>  			      plane->index, &cfg);
>  }
>  
> -static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> -					struct drm_plane_state *state)
> +int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
> +		       struct sg_table sg_tables[3])
>  {
> -	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> -	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
>  	struct rcar_du_device *rcdu = vsp->dev;
>  	unsigned int i;
>  	int ret;
>  
> -	/*
> -	 * There's no need to prepare (and unprepare) the framebuffer when the
> -	 * plane is not visible, as it will not be displayed.
> -	 */
> -	if (!state->visible)
> -		return 0;
> -
> -	for (i = 0; i < rstate->format->planes; ++i) {
> -		struct drm_gem_cma_object *gem =
> -			drm_fb_cma_get_gem_obj(state->fb, i);
> -		struct sg_table *sgt = &rstate->sg_tables[i];
> +	for (i = 0; i < fb->format->num_planes; ++i) {
> +		struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(fb, i);
> +		struct sg_table *sgt = &sg_tables[i];
>  
>  		ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr, gem->paddr,
>  				      gem->base.size);
> @@ -207,15 +197,11 @@ static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
>  		}
>  	}
>  
> -	ret = drm_gem_fb_prepare_fb(plane, state);
> -	if (ret)
> -		goto fail;
> -
>  	return 0;
>  
>  fail:
>  	while (i--) {
> -		struct sg_table *sgt = &rstate->sg_tables[i];
> +		struct sg_table *sgt = &sg_tables[i];
>  
>  		vsp1_du_unmap_sg(vsp->vsp, sgt);
>  		sg_free_table(sgt);
> @@ -224,22 +210,50 @@ static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
>  	return ret;
>  }
>  
> +static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> +					struct drm_plane_state *state)
> +{
> +	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> +	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> +	int ret;
> +
> +	/*
> +	 * There's no need to prepare (and unprepare) the framebuffer when the
> +	 * plane is not visible, as it will not be displayed.
> +	 */
> +	if (!state->visible)
> +		return 0;

What about writeback?

- Never mind - that doesn't call through here.

> +
> +	ret = rcar_du_vsp_map_fb(vsp, state->fb, rstate->sg_tables);
> +	if (ret < 0)
> +		return ret;
> +
> +	return drm_gem_fb_prepare_fb(plane, state);
> +}
> +
> +void rcar_du_vsp_unmap_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
> +			  struct sg_table sg_tables[3])
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < fb->format->num_planes; ++i) {
> +		struct sg_table *sgt = &sg_tables[i];
> +
> +		vsp1_du_unmap_sg(vsp->vsp, sgt);
> +		sg_free_table(sgt);
> +	}
> +}
> +
>  static void rcar_du_vsp_plane_cleanup_fb(struct drm_plane *plane,
>  					 struct drm_plane_state *state)
>  {
>  	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
>  	struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> -	unsigned int i;
>  
>  	if (!state->visible)
>  		return;
>  
> -	for (i = 0; i < rstate->format->planes; ++i) {
> -		struct sg_table *sgt = &rstate->sg_tables[i];
> -
> -		vsp1_du_unmap_sg(vsp->vsp, sgt);
> -		sg_free_table(sgt);
> -	}
> +	rcar_du_vsp_unmap_fb(vsp, state->fb, rstate->sg_tables);
>  }
>  
>  static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> index db232037f24a..9b4724159378 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> @@ -12,8 +12,10 @@
>  
>  #include <drm/drm_plane.h>
>  
> +struct drm_framebuffer;
>  struct rcar_du_format_info;
>  struct rcar_du_vsp;
> +struct sg_table;
>  
>  struct rcar_du_vsp_plane {
>  	struct drm_plane plane;
> @@ -60,6 +62,10 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc);
>  void rcar_du_vsp_disable(struct rcar_du_crtc *crtc);
>  void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc);
> +int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
> +		       struct sg_table sg_tables[3]);
> +void rcar_du_vsp_unmap_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
> +			  struct sg_table sg_tables[3]);
>  #else
>  static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp,
>  				   struct device_node *np,
> @@ -71,6 +77,17 @@ static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
>  static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
>  static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };
>  static inline void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc) { };
> +static inline int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp,
> +				     struct drm_framebuffer *fb,
> +				     struct sg_table sg_tables[3])
> +{
> +	return -ENXIO;
> +}
> +static inline void rcar_du_vsp_unmap_fb(struct rcar_du_vsp *vsp,
> +					struct drm_framebuffer *fb,
> +					struct sg_table sg_tables[3])
> +{
> +}
>  #endif
>  
>  #endif /* __RCAR_DU_VSP_H__ */
> 

-- 
Regards
--
Kieran