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@xxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
>
> > ---
> > 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
