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@xxxxxxxxxxxxxxxx>
Looks good to me, Just a refactor and nothing controversial.
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> ---
> 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
