LinuxTV Patchwork [v6,10/18] media: vsp1: drm: Extend frame completion API to the DU driver

login
register
mail settings
Submitter Laurent Pinchart
Date March 13, 2019, 12:05 a.m.
Message ID <20190313000532.7087-11-laurent.pinchart+renesas@ideasonboard.com>
Download mbox | patch
Permalink /patch/55031/
State New
Headers show

Comments

Laurent Pinchart - March 13, 2019, 12:05 a.m.
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@ideasonboard.com>
---
 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(-)
Kieran Bingham - March 13, 2019, 11:26 a.m.
Hi Laurent,

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@ideasonboard.com>
> ---
>  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.

However you resolve, the rest looks fine:

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



>  		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;
>  };
>  
>
Laurent Pinchart - March 13, 2019, 3:50 p.m.
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@ideasonboard.com>
> > ---
> >  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@ideasonboard.com>
> 
> >  		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;
> >  };
> >

Patch

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;
 		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;
 };
 

Privacy Policy