Hi Kieran,
On Wed, Mar 13, 2019 at 11:26:14AM +0000, Kieran Bingham wrote:
> On 13/03/2019 00:05, Laurent Pinchart wrote:
> > The VSP1 driver will need to pass extra flags to the DU through the
> > frame completion API. Replace the completed bool flag by a bitmask to
> > support this.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 ++--
> > drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
> > drivers/media/platform/vsp1/vsp1_drm.h | 2 +-
> > include/media/vsp1.h | 4 +++-
> > 4 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > index 76a39eee7c9c..28bfeb8c24fb 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > @@ -26,14 +26,14 @@
> > #include "rcar_du_kms.h"
> > #include "rcar_du_vsp.h"
> >
> > -static void rcar_du_vsp_complete(void *private, bool completed, u32 crc)
> > +static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
> > {
> > struct rcar_du_crtc *crtc = private;
> >
> > if (crtc->vblank_enable)
> > drm_crtc_handle_vblank(&crtc->crtc);
> >
> > - if (completed)
> > + if (status & VSP1_DU_STATUS_COMPLETE)
> > rcar_du_crtc_finish_page_flip(crtc);
> >
> > drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> > index 5601a787688b..0367f88135bf 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -34,14 +34,14 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> > unsigned int completion)
> > {
> > struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
> > - bool complete = completion & VSP1_DL_FRAME_END_COMPLETED;
> >
> > if (drm_pipe->du_complete) {
> > struct vsp1_entity *uif = drm_pipe->uif;
> > + unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
>
>
> Why do you mask the bit(s) here?
>
> Can't we just pass completion into
> drm_pipe->du_complete(p, completion, c); ?
>
> [(edit: no, because completion contains things such as
> VSP1_DL_FRAME_END_INTERNAL)]
>
> Or are you relying on the fact that,
> VSP1_DL_FRAME_END_COMPLETED == VSP1_DU_STATUS_COMPLETE
>
> in which case perhaps we should be more explicit somehow, either in code
> or with a comment that this section is converting between the two
> bitfield types.
Good point. I'll add the following comment above the definition of
VSP1_DL_FRAME_END_COMPLETED:
/* Keep these flags in sync with VSP1_DU_STATUS_* in include/media/vsp1.h. */
> However you resolve, the rest looks fine:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> > u32 crc;
> >
> > crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> > - drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
> > + drm_pipe->du_complete(drm_pipe->du_private, status, crc);
> > }
> >
> > if (completion & VSP1_DL_FRAME_END_INTERNAL) {
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
> > index 8dfd274a59e2..e85ad4366fbb 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.h
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> > @@ -42,7 +42,7 @@ struct vsp1_drm_pipeline {
> > struct vsp1_du_crc_config crc;
> >
> > /* Frame synchronisation */
> > - void (*du_complete)(void *data, bool completed, u32 crc);
> > + void (*du_complete)(void *data, unsigned int status, u32 crc);
> > void *du_private;
> > };
> >
> > diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> > index 1cf868360701..877496936487 100644
> > --- a/include/media/vsp1.h
> > +++ b/include/media/vsp1.h
> > @@ -17,6 +17,8 @@ struct device;
> >
> > int vsp1_du_init(struct device *dev);
> >
> > +#define VSP1_DU_STATUS_COMPLETE BIT(0)
> > +
> > /**
> > * struct vsp1_du_lif_config - VSP LIF configuration
> > * @width: output frame width
> > @@ -32,7 +34,7 @@ struct vsp1_du_lif_config {
> > unsigned int height;
> > bool interlaced;
> >
> > - void (*callback)(void *data, bool completed, u32 crc);
> > + void (*callback)(void *data, unsigned int status, u32 crc);
> > void *callback_data;
> > };
> >
--
Regards,
Laurent Pinchart
