[PATCH 0/3] rcar-vin: Add support for RGB formats with alpha component

[PATCH 0/3] rcar-vin: Add support for RGB formats with alpha component

[Niklas Söderlund]

Hi,

This small series adds support for two new pixel formats for the 
rcar-vin driver; V4L2_PIX_FMT_ARGB555 and V4L2_PIX_FMT_ABGR32. Both 
formats have an alpha component so a new standard control is also added
to control its value, V4L2_CID_ALPHA_COMPONENT.

The series is based on the latest media-tree and is tested on both 
Renesas Gen2 and Gen3 hardware without any regressions found.

Patch 1/3 fixes a badly named register name, 2/3 adds the new control 
and finally 3/3 adds the two new pixel formats.

Niklas Söderlund (3):
  rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB
  rcar-vin: Add control for alpha component
  rcar-vin: Add support for RGB formats with alpha component

 drivers/media/platform/rcar-vin/rcar-core.c | 53 ++++++++++++++++++++-
 drivers/media/platform/rcar-vin/rcar-dma.c  | 39 ++++++++++++++-
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  8 ++++
 drivers/media/platform/rcar-vin/rcar-vin.h  |  5 ++
 4 files changed, 101 insertions(+), 4 deletions(-)

-- 
2.21.0

[PATCH 1/3] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB

[Niklas Söderlund]

The value have nothing to do with ARGB1555, it controls if the alpha
component should be filled in for ARGB1555 or ARGB888. Rename it to
reflect this.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 91ab064404a185af..2d146ecf93d66ad5 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -114,7 +114,7 @@
 #define VNDMR_EXRGB		(1 << 8)
 #define VNDMR_BPSM		(1 << 4)
 #define VNDMR_DTMD_YCSEP	(1 << 1)
-#define VNDMR_DTMD_ARGB1555	(1 << 0)
+#define VNDMR_DTMD_ARGB		(1 << 0)
 
 /* Video n Data Mode Register 2 bits */
 #define VNDMR2_VPS		(1 << 30)
@@ -721,7 +721,7 @@ static int rvin_setup(struct rvin_dev *vin)
 		output_is_yuv = true;
 		break;
 	case V4L2_PIX_FMT_XRGB555:
-		dmr = VNDMR_DTMD_ARGB1555;
+		dmr = VNDMR_DTMD_ARGB;
 		break;
 	case V4L2_PIX_FMT_RGB565:
 		dmr = 0;
-- 
2.21.0

Re: [PATCH 1/3] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB

[Ulrich Hecht]

> On May 16, 2019 at 2:47 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> The value have nothing to do with ARGB1555, it controls if the alpha
> component should be filled in for ARGB1555 or ARGB888. Rename it to
> reflect this.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 91ab064404a185af..2d146ecf93d66ad5 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -114,7 +114,7 @@
>  #define VNDMR_EXRGB		(1 << 8)
>  #define VNDMR_BPSM		(1 << 4)
>  #define VNDMR_DTMD_YCSEP	(1 << 1)
> -#define VNDMR_DTMD_ARGB1555	(1 << 0)
> +#define VNDMR_DTMD_ARGB		(1 << 0)
>  
>  /* Video n Data Mode Register 2 bits */
>  #define VNDMR2_VPS		(1 << 30)
> @@ -721,7 +721,7 @@ static int rvin_setup(struct rvin_dev *vin)
>  		output_is_yuv = true;
>  		break;
>  	case V4L2_PIX_FMT_XRGB555:
> -		dmr = VNDMR_DTMD_ARGB1555;
> +		dmr = VNDMR_DTMD_ARGB;
>  		break;
>  	case V4L2_PIX_FMT_RGB565:
>  		dmr = 0;
> -- 
> 2.21.0
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH 1/3] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 02:47:44AM +0200, Niklas Söderlund wrote:
> The value have nothing to do with ARGB1555, it controls if the alpha
> component should be filled in for ARGB1555 or ARGB888. Rename it to
> reflect this.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 91ab064404a185af..2d146ecf93d66ad5 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -114,7 +114,7 @@
>  #define VNDMR_EXRGB		(1 << 8)
>  #define VNDMR_BPSM		(1 << 4)
>  #define VNDMR_DTMD_YCSEP	(1 << 1)
> -#define VNDMR_DTMD_ARGB1555	(1 << 0)
> +#define VNDMR_DTMD_ARGB		(1 << 0)
>  
>  /* Video n Data Mode Register 2 bits */
>  #define VNDMR2_VPS		(1 << 30)
> @@ -721,7 +721,7 @@ static int rvin_setup(struct rvin_dev *vin)
>  		output_is_yuv = true;
>  		break;
>  	case V4L2_PIX_FMT_XRGB555:
> -		dmr = VNDMR_DTMD_ARGB1555;
> +		dmr = VNDMR_DTMD_ARGB;
>  		break;
>  	case V4L2_PIX_FMT_RGB565:
>  		dmr = 0;

-- 
Regards,

Laurent Pinchart

[PATCH 2/3] rcar-vin: Add control for alpha component

[Niklas Söderlund]

In preparation to adding support for RGB pixel formats with an alpha
component add a control to allow the user to control which alpha value
should be used.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 53 ++++++++++++++++++++-
 drivers/media/platform/rcar-vin/rcar-dma.c  |  5 ++
 drivers/media/platform/rcar-vin/rcar-vin.h  |  5 ++
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 64f9cf790445d14e..ee6e6cb39c749675 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -389,6 +389,28 @@ static void rvin_group_put(struct rvin_dev *vin)
 	kref_put(&group->refcount, rvin_group_release);
 }
 
+/* -----------------------------------------------------------------------------
+ * Controls
+ */
+
+static int rvin_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct rvin_dev *vin =
+		container_of(ctrl->handler, struct rvin_dev, ctrl_handler);
+
+	switch (ctrl->id) {
+	case V4L2_CID_ALPHA_COMPONENT:
+		rvin_set_alpha(vin, ctrl->val);
+		break;
+	}
+
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
+	.s_ctrl = rvin_s_ctrl,
+};
+
 /* -----------------------------------------------------------------------------
  * Async notifier
  */
@@ -478,6 +500,15 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 	if (ret < 0)
 		return ret;
 
+	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
+			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
+
+	if (vin->ctrl_handler.error) {
+		ret = vin->ctrl_handler.error;
+		v4l2_ctrl_handler_free(&vin->ctrl_handler);
+		return ret;
+	}
+
 	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
 				    NULL, true);
 	if (ret < 0) {
@@ -870,6 +901,21 @@ static int rvin_mc_init(struct rvin_dev *vin)
 	if (ret)
 		rvin_group_put(vin);
 
+	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
+	if (ret < 0)
+		return ret;
+
+	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
+			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
+
+	if (vin->ctrl_handler.error) {
+		ret = vin->ctrl_handler.error;
+		v4l2_ctrl_handler_free(&vin->ctrl_handler);
+		return ret;
+	}
+
+	vin->vdev.ctrl_handler = &vin->ctrl_handler;
+
 	return ret;
 }
 
@@ -1245,6 +1291,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
 
 	vin->dev = &pdev->dev;
 	vin->info = of_device_get_match_data(&pdev->dev);
+	vin->alpha = 0xff;
 
 	/*
 	 * Special care is needed on r8a7795 ES1.x since it
@@ -1288,6 +1335,8 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	return 0;
 
 error_group_unregister:
+	v4l2_ctrl_handler_free(&vin->ctrl_handler);
+
 	if (vin->info->use_mc) {
 		mutex_lock(&vin->group->lock);
 		if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
@@ -1323,10 +1372,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
 		}
 		mutex_unlock(&vin->group->lock);
 		rvin_group_put(vin);
-	} else {
-		v4l2_ctrl_handler_free(&vin->ctrl_handler);
 	}
 
+	v4l2_ctrl_handler_free(&vin->ctrl_handler);
+
 	rvin_dma_unregister(vin);
 
 	return 0;
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 2d146ecf93d66ad5..4e991cce5fb56a90 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1343,3 +1343,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
 
 	return 0;
 }
+
+void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha)
+{
+	vin->alpha = alpha;
+}
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 0b13b34d03e3dce4..365dfde06ec25add 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -178,6 +178,8 @@ struct rvin_info {
  * @compose:		active composing
  * @source:		active size of the video source
  * @std:		active video standard of the video source
+ *
+ * @alpha:		Alpha component to fill in for supported pixel formats
  */
 struct rvin_dev {
 	struct device *dev;
@@ -215,6 +217,8 @@ struct rvin_dev {
 	struct v4l2_rect compose;
 	struct v4l2_rect source;
 	v4l2_std_id std;
+
+	unsigned int alpha;
 };
 
 #define vin_to_source(vin)		((vin)->parallel->subdev)
@@ -266,5 +270,6 @@ const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat);
 void rvin_crop_scale_comp(struct rvin_dev *vin);
 
 int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
+void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha);
 
 #endif
-- 
2.21.0

Re: [PATCH 2/3] rcar-vin: Add control for alpha component

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 02:47:45AM +0200, Niklas Söderlund wrote:
> In preparation to adding support for RGB pixel formats with an alpha
> component add a control to allow the user to control which alpha value
> should be used.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 53 ++++++++++++++++++++-
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  5 ++
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  5 ++
>  3 files changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 64f9cf790445d14e..ee6e6cb39c749675 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -389,6 +389,28 @@ static void rvin_group_put(struct rvin_dev *vin)
>  	kref_put(&group->refcount, rvin_group_release);
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Controls
> + */
> +
> +static int rvin_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct rvin_dev *vin =
> +		container_of(ctrl->handler, struct rvin_dev, ctrl_handler);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_ALPHA_COMPONENT:
> +		rvin_set_alpha(vin, ctrl->val);

You can set vin->alpha here directly, no need for a rvin_set_alpha()
function.

> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
> +	.s_ctrl = rvin_s_ctrl,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * Async notifier
>   */
> @@ -478,6 +500,15 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  	if (ret < 0)
>  		return ret;
>  
> +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> +
> +	if (vin->ctrl_handler.error) {
> +		ret = vin->ctrl_handler.error;
> +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +		return ret;
> +	}
> +
>  	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
>  				    NULL, true);
>  	if (ret < 0) {
> @@ -870,6 +901,21 @@ static int rvin_mc_init(struct rvin_dev *vin)
>  	if (ret)
>  		rvin_group_put(vin);
>  
> +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> +
> +	if (vin->ctrl_handler.error) {
> +		ret = vin->ctrl_handler.error;
> +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +		return ret;
> +	}
> +
> +	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> +
>  	return ret;
>  }
>  
> @@ -1245,6 +1291,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  
>  	vin->dev = &pdev->dev;
>  	vin->info = of_device_get_match_data(&pdev->dev);
> +	vin->alpha = 0xff;
>  
>  	/*
>  	 * Special care is needed on r8a7795 ES1.x since it
> @@ -1288,6 +1335,8 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  	return 0;
>  
>  error_group_unregister:
> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +
>  	if (vin->info->use_mc) {
>  		mutex_lock(&vin->group->lock);
>  		if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
> @@ -1323,10 +1372,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
>  		}
>  		mutex_unlock(&vin->group->lock);
>  		rvin_group_put(vin);
> -	} else {
> -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
>  	}
>  
> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +
>  	rvin_dma_unregister(vin);
>  
>  	return 0;
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 2d146ecf93d66ad5..4e991cce5fb56a90 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1343,3 +1343,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
>  
>  	return 0;
>  }
> +
> +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha)
> +{
> +	vin->alpha = alpha;
> +}
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 0b13b34d03e3dce4..365dfde06ec25add 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -178,6 +178,8 @@ struct rvin_info {
>   * @compose:		active composing
>   * @source:		active size of the video source
>   * @std:		active video standard of the video source
> + *

Do you need a blank line here ?

Apart from that the patch looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + * @alpha:		Alpha component to fill in for supported pixel formats
>   */
>  struct rvin_dev {
>  	struct device *dev;
> @@ -215,6 +217,8 @@ struct rvin_dev {
>  	struct v4l2_rect compose;
>  	struct v4l2_rect source;
>  	v4l2_std_id std;
> +
> +	unsigned int alpha;
>  };
>  
>  #define vin_to_source(vin)		((vin)->parallel->subdev)
> @@ -266,5 +270,6 @@ const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat);
>  void rvin_crop_scale_comp(struct rvin_dev *vin);
>  
>  int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha);
>  
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] rcar-vin: Add control for alpha component

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-05-16 13:01:38 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, May 16, 2019 at 02:47:45AM +0200, Niklas Söderlund wrote:
> > In preparation to adding support for RGB pixel formats with an alpha
> > component add a control to allow the user to control which alpha value
> > should be used.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 53 ++++++++++++++++++++-
> >  drivers/media/platform/rcar-vin/rcar-dma.c  |  5 ++
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  5 ++
> >  3 files changed, 61 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 64f9cf790445d14e..ee6e6cb39c749675 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -389,6 +389,28 @@ static void rvin_group_put(struct rvin_dev *vin)
> >  	kref_put(&group->refcount, rvin_group_release);
> >  }
> >  
> > +/* -----------------------------------------------------------------------------
> > + * Controls
> > + */
> > +
> > +static int rvin_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct rvin_dev *vin =
> > +		container_of(ctrl->handler, struct rvin_dev, ctrl_handler);
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_ALPHA_COMPONENT:
> > +		rvin_set_alpha(vin, ctrl->val);
> 
> You can set vin->alpha here directly, no need for a rvin_set_alpha()
> function.

I think you discovers the reason for this in 3/3. I could set vin->alpha 
directly here and introduce rvin_set_alpha() in 3/3. I opted for this 
approach as the reason form not margin 2/3 and 3/3 into a single patch 
is that I only want the register writes in 3/3.

> 
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
> > +	.s_ctrl = rvin_s_ctrl,
> > +};
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Async notifier
> >   */
> > @@ -478,6 +500,15 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > +
> > +	if (vin->ctrl_handler.error) {
> > +		ret = vin->ctrl_handler.error;
> > +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +		return ret;
> > +	}
> > +
> >  	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
> >  				    NULL, true);
> >  	if (ret < 0) {
> > @@ -870,6 +901,21 @@ static int rvin_mc_init(struct rvin_dev *vin)
> >  	if (ret)
> >  		rvin_group_put(vin);
> >  
> > +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > +
> > +	if (vin->ctrl_handler.error) {
> > +		ret = vin->ctrl_handler.error;
> > +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +		return ret;
> > +	}
> > +
> > +	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > +
> >  	return ret;
> >  }
> >  
> > @@ -1245,6 +1291,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  
> >  	vin->dev = &pdev->dev;
> >  	vin->info = of_device_get_match_data(&pdev->dev);
> > +	vin->alpha = 0xff;
> >  
> >  	/*
> >  	 * Special care is needed on r8a7795 ES1.x since it
> > @@ -1288,6 +1335,8 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  	return 0;
> >  
> >  error_group_unregister:
> > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +
> >  	if (vin->info->use_mc) {
> >  		mutex_lock(&vin->group->lock);
> >  		if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
> > @@ -1323,10 +1372,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
> >  		}
> >  		mutex_unlock(&vin->group->lock);
> >  		rvin_group_put(vin);
> > -	} else {
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> >  	}
> >  
> > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +
> >  	rvin_dma_unregister(vin);
> >  
> >  	return 0;
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 2d146ecf93d66ad5..4e991cce5fb56a90 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -1343,3 +1343,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> >  
> >  	return 0;
> >  }
> > +
> > +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha)
> > +{
> > +	vin->alpha = alpha;
> > +}
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index 0b13b34d03e3dce4..365dfde06ec25add 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -178,6 +178,8 @@ struct rvin_info {
> >   * @compose:		active composing
> >   * @source:		active size of the video source
> >   * @std:		active video standard of the video source
> > + *
> 
> Do you need a blank line here ?

I don't need it, but it make sens in my head on how I group variables in 
sturct rvin_info. There already exists blank lines in the structure for 
this grouping of variables thinking. If you are OK with it I would 
prefer to keep it, but I do not feel strongly about it.

> 
> Apart from that the patch looks good to me.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

> 
> > + * @alpha:		Alpha component to fill in for supported pixel formats
> >   */
> >  struct rvin_dev {
> >  	struct device *dev;
> > @@ -215,6 +217,8 @@ struct rvin_dev {
> >  	struct v4l2_rect compose;
> >  	struct v4l2_rect source;
> >  	v4l2_std_id std;
> > +
> > +	unsigned int alpha;
> >  };
> >  
> >  #define vin_to_source(vin)		((vin)->parallel->subdev)
> > @@ -266,5 +270,6 @@ const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat);
> >  void rvin_crop_scale_comp(struct rvin_dev *vin);
> >  
> >  int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> > +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha);
> >  
> >  #endif
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund

[PATCH 3/3] rcar-vin: Add support for RGB formats with alpha component

[Niklas Söderlund]

The R-Car VIN module supports V4L2_PIX_FMT_ARGB555 and
V4L2_PIX_FMT_ABGR32 pixel formats. Add the hardware register setup and
allow the alpha component to be changed while streaming using the
V4L2_CID_ALPHA_COMPONENT control.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c  | 30 +++++++++++++++++++++
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  8 ++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 4e991cce5fb56a90..5c0ed27c5d05dd45 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -111,8 +111,11 @@
 #define VNIE_EFE		(1 << 1)
 
 /* Video n Data Mode Register bits */
+#define VNDMR_A8BIT(n)		((n & 0xff) << 24)
+#define VNDMR_A8BIT_MASK	(0xff << 24)
 #define VNDMR_EXRGB		(1 << 8)
 #define VNDMR_BPSM		(1 << 4)
+#define VNDMR_ABIT		(1 << 2)
 #define VNDMR_DTMD_YCSEP	(1 << 1)
 #define VNDMR_DTMD_ARGB		(1 << 0)
 
@@ -730,6 +733,12 @@ static int rvin_setup(struct rvin_dev *vin)
 		/* Note: not supported on M1 */
 		dmr = VNDMR_EXRGB;
 		break;
+	case V4L2_PIX_FMT_ARGB555:
+		dmr = (vin->alpha ? VNDMR_ABIT : 0) | VNDMR_DTMD_ARGB;
+		break;
+	case V4L2_PIX_FMT_ABGR32:
+		dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB;
+		break;
 	default:
 		vin_err(vin, "Invalid pixelformat (0x%x)\n",
 			vin->format.pixelformat);
@@ -1346,5 +1355,26 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
 
 void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha)
 {
+	u32 dmr;
+
 	vin->alpha = alpha;
+
+	if (vin->state == STOPPED)
+		return;
+
+	switch (vin->format.pixelformat) {
+	case V4L2_PIX_FMT_ARGB555:
+		dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_ABIT;
+		if (vin->alpha)
+			dmr |= VNDMR_ABIT;
+		break;
+	case V4L2_PIX_FMT_ABGR32:
+		dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_A8BIT_MASK;
+		dmr |= VNDMR_A8BIT(vin->alpha);
+		break;
+	default:
+		return;
+	}
+
+	rvin_write(vin, dmr,  VNDMR_REG);
 }
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 7cbdcbf9b090c638..bb2900f5d000f9a6 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -54,6 +54,14 @@ static const struct rvin_video_format rvin_formats[] = {
 		.fourcc			= V4L2_PIX_FMT_XBGR32,
 		.bpp			= 4,
 	},
+	{
+		.fourcc			= V4L2_PIX_FMT_ARGB555,
+		.bpp			= 2,
+	},
+	{
+		.fourcc			= V4L2_PIX_FMT_ABGR32,
+		.bpp			= 4,
+	},
 };
 
 const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat)
-- 
2.21.0

Re: [PATCH 3/3] rcar-vin: Add support for RGB formats with alpha component

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 02:47:46AM +0200, Niklas Söderlund wrote:
> The R-Car VIN module supports V4L2_PIX_FMT_ARGB555 and
> V4L2_PIX_FMT_ABGR32 pixel formats. Add the hardware register setup and
> allow the alpha component to be changed while streaming using the
> V4L2_CID_ALPHA_COMPONENT control.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 30 +++++++++++++++++++++
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  8 ++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 4e991cce5fb56a90..5c0ed27c5d05dd45 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -111,8 +111,11 @@
>  #define VNIE_EFE		(1 << 1)
>  
>  /* Video n Data Mode Register bits */
> +#define VNDMR_A8BIT(n)		((n & 0xff) << 24)
> +#define VNDMR_A8BIT_MASK	(0xff << 24)
>  #define VNDMR_EXRGB		(1 << 8)
>  #define VNDMR_BPSM		(1 << 4)
> +#define VNDMR_ABIT		(1 << 2)
>  #define VNDMR_DTMD_YCSEP	(1 << 1)
>  #define VNDMR_DTMD_ARGB		(1 << 0)
>  
> @@ -730,6 +733,12 @@ static int rvin_setup(struct rvin_dev *vin)
>  		/* Note: not supported on M1 */
>  		dmr = VNDMR_EXRGB;
>  		break;
> +	case V4L2_PIX_FMT_ARGB555:
> +		dmr = (vin->alpha ? VNDMR_ABIT : 0) | VNDMR_DTMD_ARGB;
> +		break;
> +	case V4L2_PIX_FMT_ABGR32:
> +		dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB;
> +		break;
>  	default:
>  		vin_err(vin, "Invalid pixelformat (0x%x)\n",
>  			vin->format.pixelformat);
> @@ -1346,5 +1355,26 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
>  
>  void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha)

OK, I now see why you added a rvin_set_alpha() function. It makes sense,
but I think you need to protect this with a lock to avoid races between
stream start and control set. Or are we already protected by a lock the
serialises all V4L2 ioctls for the VIN video node ?

>  {
> +	u32 dmr;
> +
>  	vin->alpha = alpha;
> +
> +	if (vin->state == STOPPED)
> +		return;
> +
> +	switch (vin->format.pixelformat) {
> +	case V4L2_PIX_FMT_ARGB555:
> +		dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_ABIT;
> +		if (vin->alpha)
> +			dmr |= VNDMR_ABIT;
> +		break;

Should you cache the DNDMR valid to avoid a hardware read ?

> +	case V4L2_PIX_FMT_ABGR32:
> +		dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_A8BIT_MASK;
> +		dmr |= VNDMR_A8BIT(vin->alpha);
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	rvin_write(vin, dmr,  VNDMR_REG);
>  }
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 7cbdcbf9b090c638..bb2900f5d000f9a6 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -54,6 +54,14 @@ static const struct rvin_video_format rvin_formats[] = {
>  		.fourcc			= V4L2_PIX_FMT_XBGR32,
>  		.bpp			= 4,
>  	},
> +	{
> +		.fourcc			= V4L2_PIX_FMT_ARGB555,
> +		.bpp			= 2,
> +	},
> +	{
> +		.fourcc			= V4L2_PIX_FMT_ABGR32,
> +		.bpp			= 4,
> +	},
>  };
>  
>  const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] rcar-vin: Add support for RGB formats with alpha component

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-05-16 13:08:22 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, May 16, 2019 at 02:47:46AM +0200, Niklas Söderlund wrote:
> > The R-Car VIN module supports V4L2_PIX_FMT_ARGB555 and
> > V4L2_PIX_FMT_ABGR32 pixel formats. Add the hardware register setup and
> > allow the alpha component to be changed while streaming using the
> > V4L2_CID_ALPHA_COMPONENT control.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 30 +++++++++++++++++++++
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c |  8 ++++++
> >  2 files changed, 38 insertions(+)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 4e991cce5fb56a90..5c0ed27c5d05dd45 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -111,8 +111,11 @@
> >  #define VNIE_EFE		(1 << 1)
> >  
> >  /* Video n Data Mode Register bits */
> > +#define VNDMR_A8BIT(n)		((n & 0xff) << 24)
> > +#define VNDMR_A8BIT_MASK	(0xff << 24)
> >  #define VNDMR_EXRGB		(1 << 8)
> >  #define VNDMR_BPSM		(1 << 4)
> > +#define VNDMR_ABIT		(1 << 2)
> >  #define VNDMR_DTMD_YCSEP	(1 << 1)
> >  #define VNDMR_DTMD_ARGB		(1 << 0)
> >  
> > @@ -730,6 +733,12 @@ static int rvin_setup(struct rvin_dev *vin)
> >  		/* Note: not supported on M1 */
> >  		dmr = VNDMR_EXRGB;
> >  		break;
> > +	case V4L2_PIX_FMT_ARGB555:
> > +		dmr = (vin->alpha ? VNDMR_ABIT : 0) | VNDMR_DTMD_ARGB;
> > +		break;
> > +	case V4L2_PIX_FMT_ABGR32:
> > +		dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB;
> > +		break;
> >  	default:
> >  		vin_err(vin, "Invalid pixelformat (0x%x)\n",
> >  			vin->format.pixelformat);
> > @@ -1346,5 +1355,26 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> >  
> >  void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha)
> 
> OK, I now see why you added a rvin_set_alpha() function. It makes sense,
> but I think you need to protect this with a lock to avoid races between
> stream start and control set. Or are we already protected by a lock the
> serialises all V4L2 ioctls for the VIN video node ?

Yes we are protected by the ioctls lock in __video_do_ioctl(), at least 
that is my interpretation of the code I have not tried to force a race.

> 
> >  {
> > +	u32 dmr;
> > +
> >  	vin->alpha = alpha;
> > +
> > +	if (vin->state == STOPPED)
> > +		return;
> > +
> > +	switch (vin->format.pixelformat) {
> > +	case V4L2_PIX_FMT_ARGB555:
> > +		dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_ABIT;
> > +		if (vin->alpha)
> > +			dmr |= VNDMR_ABIT;
> > +		break;
> 
> Should you cache the DNDMR valid to avoid a hardware read ?

It is one possibility, as VNDMR is written to at other locations I would 
feel better to to this at a later point in time.

I'm currently trying to clean up the rcar-vin driver with regard to how 
it handles formats a bit different in the devnode and media centric code 
paths. Once that work is complete a generic way to cache register values 
could be added on top. I suspect there are other registers then VNDMR 
which could benefit from such a solution.

> 
> > +	case V4L2_PIX_FMT_ABGR32:
> > +		dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_A8BIT_MASK;
> > +		dmr |= VNDMR_A8BIT(vin->alpha);
> > +		break;
> > +	default:
> > +		return;
> > +	}
> > +
> > +	rvin_write(vin, dmr,  VNDMR_REG);
> >  }
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index 7cbdcbf9b090c638..bb2900f5d000f9a6 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -54,6 +54,14 @@ static const struct rvin_video_format rvin_formats[] = {
> >  		.fourcc			= V4L2_PIX_FMT_XBGR32,
> >  		.bpp			= 4,
> >  	},
> > +	{
> > +		.fourcc			= V4L2_PIX_FMT_ARGB555,
> > +		.bpp			= 2,
> > +	},
> > +	{
> > +		.fourcc			= V4L2_PIX_FMT_ABGR32,
> > +		.bpp			= 4,
> > +	},
> >  };
> >  
> >  const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund