Re: [PATCH v6 18/18] drm: rcar-du: Add writeback support for R-Car Gen3

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

On Wed, Mar 13, 2019 at 12:06:40PM +0000, Kieran Bingham wrote:
> On 13/03/2019 00:05, Laurent Pinchart wrote:
> > Implement writeback support for R-Car Gen3 by exposing writeback
> > connectors. Behind the scene the calls are forwarded to the VSP
> > backend.
> > 
> > Using writeback connectors will allow implemented writeback support for
> > R-Car Gen2 with a consistent API if desired.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> An extra blank line, and I was a bit concerned about a function naming
> below - but other than that:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> > Changes since v5:
> > 
> > - Skip writeback connector when configuring output routing
> > - Implement writeback connector atomic state operations
> > ---
> >  drivers/gpu/drm/rcar-du/Kconfig             |   4 +
> >  drivers/gpu/drm/rcar-du/Makefile            |   3 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c      |   7 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      |   7 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c       |  12 +
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c       |   5 +
> >  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 243 ++++++++++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_du_writeback.h |  39 ++++
> >  8 files changed, 317 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_writeback.h

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > index 0806a69c4679..99ae03a1713a 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > @@ -25,6 +25,7 @@
> >  #include "rcar_du_drv.h"
> >  #include "rcar_du_kms.h"
> >  #include "rcar_du_vsp.h"
> > +#include "rcar_du_writeback.h"
> >  
> >  static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
> >  {
> > @@ -35,6 +36,8 @@ static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
> >  
> >  	if (status & VSP1_DU_STATUS_COMPLETE)
> >  		rcar_du_crtc_finish_page_flip(crtc);
> > +	if (status & VSP1_DU_STATUS_WRITEBACK)
> > +		rcar_du_writeback_complete(crtc);
> >  
> >  	drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
> >  }
> > @@ -106,6 +109,8 @@ void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
> >  	state = to_rcar_crtc_state(crtc->crtc.state);
> >  	cfg.crc = state->crc;
> >  
> > +	rcar_du_writeback_atomic_flush(crtc, &cfg.writeback);
> 
> Hrm ...the naming here worries me a bit. This doesn't do the actual
> flushing (execution?) of the writeback operation, it just configures the
> writeback into the VSP cfg structure. The 'flush' to hardware takes
> place in vsp1_du_atomic_flush().
> 
> Or maybe it is ok becuase it calls drm_writeback_queue_job() as well as
> setting up the cfg...

You've got a point. I've renamed the function to
rcar_du_writeback_setup().

> > +
> >  	vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
> >  }
> >  

[snip]

-- 
Regards,

Laurent Pinchart