Re: [PATCH v2 5/7] media: renesas: vsp1: Add new formats (2-10-10-10 ARGB, Y210)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Tomi,

Thank you for the patch.

On Mon, Dec 19, 2022 at 04:01:37PM +0200, Tomi Valkeinen wrote:
> Add new pixel formats: XBGR2101010, ABGR2101010, BGRA1010102 and Y210.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
> ---
>  .../media/platform/renesas/vsp1/vsp1_pipe.c   | 15 ++++++
>  .../media/platform/renesas/vsp1/vsp1_regs.h   | 22 +++++++++
>  .../media/platform/renesas/vsp1/vsp1_rpf.c    | 49 +++++++++++++++++++
>  3 files changed, 86 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> index f72ac01c21ea..2867b3de06fa 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> @@ -146,6 +146,18 @@ static const struct vsp1_format_info vsp1_video_formats[] = {
>  	  VI6_FMT_ARGB_8888, VI6_RPF_DSWAP_P_LLS | VI6_RPF_DSWAP_P_LWS |
>  	  VI6_RPF_DSWAP_P_WDS | VI6_RPF_DSWAP_P_BTS,
>  	  1, { 32, 0, 0 }, false, false, 1, 1, false },
> +	{ V4L2_PIX_FMT_XBGR2101010, MEDIA_BUS_FMT_ARGB8888_1X32,
> +	  VI6_FMT_RGB10_RGB10A2_A2RGB10,
> +	  VI6_RPF_DSWAP_P_LLS | VI6_RPF_DSWAP_P_LWS,
> +	  1, { 32, 0, 0 }, false, false, 1, 1, false },
> +	{ V4L2_PIX_FMT_ABGR2101010, MEDIA_BUS_FMT_ARGB8888_1X32,
> +	  VI6_FMT_RGB10_RGB10A2_A2RGB10,
> +	  VI6_RPF_DSWAP_P_LLS | VI6_RPF_DSWAP_P_LWS,
> +	  1, { 32, 0, 0 }, false, false, 1, 1, false },
> +	{ V4L2_PIX_FMT_BGRA1010102, MEDIA_BUS_FMT_ARGB8888_1X32,
> +	  VI6_FMT_RGB10_RGB10A2_A2RGB10,
> +	  VI6_RPF_DSWAP_P_LLS | VI6_RPF_DSWAP_P_LWS,
> +	  1, { 32, 0, 0 }, false, false, 1, 1, false },
>  	{ V4L2_PIX_FMT_UYVY, MEDIA_BUS_FMT_AYUV8_1X32,
>  	  VI6_FMT_YUYV_422, VI6_RPF_DSWAP_P_LLS | VI6_RPF_DSWAP_P_LWS |
>  	  VI6_RPF_DSWAP_P_WDS | VI6_RPF_DSWAP_P_BTS,
> @@ -202,6 +214,9 @@ static const struct vsp1_format_info vsp1_video_formats[] = {
>  	  VI6_FMT_Y_U_V_444, VI6_RPF_DSWAP_P_LLS | VI6_RPF_DSWAP_P_LWS |
>  	  VI6_RPF_DSWAP_P_WDS | VI6_RPF_DSWAP_P_BTS,
>  	  3, { 8, 8, 8 }, false, true, 1, 1, false },
> +	{ V4L2_PIX_FMT_Y210, MEDIA_BUS_FMT_AYUV8_1X32,
> +	  VI6_FMT_YUYV_422, VI6_RPF_DSWAP_P_LLS | VI6_RPF_DSWAP_P_LWS,
> +	  1, { 32, 0, 0 }, false, false, 2, 1, false },
>  };
>  
>  /**
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> index c61e8dafeecf..8947ea05f95e 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> @@ -228,6 +228,27 @@
>  #define VI6_RPF_MULT_ALPHA_RATIO_MASK	(0xff << 0)
>  #define VI6_RPF_MULT_ALPHA_RATIO_SHIFT	0
>  
> +#define VI6_RPF_EXT_INFMT0		0x0370
> +#define VI6_RPF_EXT_INFMT0_F2B_LSB		(0 << 12)
> +#define VI6_RPF_EXT_INFMT0_F2B_MSB		(1 << 12)

We don't normally define two macros for each bit. You can drop the
F2B_LSB macro, rename F2B_MSB to F2B, and use BIT(12).

> +#define VI6_RPF_EXT_INFMT0_IPBD_Y_8		(0 << 8)
> +#define VI6_RPF_EXT_INFMT0_IPBD_Y_10		(1 << 8)
> +#define VI6_RPF_EXT_INFMT0_IPBD_Y_12		(2 << 8)
> +#define VI6_RPF_EXT_INFMT0_IPBD_C_8		(0 << 4)
> +#define VI6_RPF_EXT_INFMT0_IPBD_C_10		(1 << 4)
> +#define VI6_RPF_EXT_INFMT0_IPBD_C_12		(2 << 4)
> +#define VI6_RPF_EXT_INFMT0_BYPP_M1_RGB10	(3 << 0)
> +#define VI6_RPF_EXT_INFMT0_BYPP_M1_N_RGB10	(0 << 0)

I would drop the last one as you don't use it.

> +
> +#define VI6_RPF_EXT_INFMT1		0x0374
> +#define VI6_RPF_EXT_INFMT2		0x0378
> +
> +#define VI6_RPF_BRDITH_CTRL		0x03e0
> +#define VI6_RPF_BRDITH_CTRL_ODE_EN	(1 << 8)
> +#define VI6_RPF_BRDITH_CTRL_ODE_DIS	(0 << 8)
> +#define VI6_RPF_BRDITH_CTRL_CBRM_RO	(1 << 0)
> +#define VI6_RPF_BRDITH_CTRL_CBRM_TR	(0 << 0)

Same here, one macro per bit.

The BRDITH_CTRL registers doesn't seem to be used. I don't mind adding
it though.

> +
>  /* -----------------------------------------------------------------------------
>   * WPF Control Registers
>   */
> @@ -846,6 +867,7 @@
>  #define VI6_FMT_XBXGXR_262626		0x21
>  #define VI6_FMT_ABGR_8888		0x22
>  #define VI6_FMT_XXRGB_88565		0x23
> +#define VI6_FMT_RGB10_RGB10A2_A2RGB10	0x30
>  
>  #define VI6_FMT_Y_UV_444		0x40
>  #define VI6_FMT_Y_UV_422		0x41
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> index 045aa54f7998..60ba3c62e86c 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> @@ -55,6 +55,11 @@ static const struct v4l2_subdev_ops rpf_ops = {
>   * VSP1 Entity Operations
>   */
>  
> +#define PACK_CPOS(a, b, c, d) \
> +	(((a) << 24) | ((b) << 16) | ((c) << 8) | ((d) << 0))
> +#define PACK_CLEN(a, b, c, d) \
> +	(((a) << 24) | ((b) << 16) | ((c) << 8) | ((d) << 0))
> +

Please move this to vsp1_regs.h, just below the corresponding registers.
I would also prefer giving the macros names that related to the
registers.

>  static void rpf_configure_stream(struct vsp1_entity *entity,
>  				 struct vsp1_pipeline *pipe,
>  				 struct vsp1_dl_list *dl,
> @@ -109,6 +114,50 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
>  	vsp1_rpf_write(rpf, dlb, VI6_RPF_INFMT, infmt);
>  	vsp1_rpf_write(rpf, dlb, VI6_RPF_DSWAP, fmtinfo->swap);
>  
> +	if ((entity->vsp1->version & VI6_IP_VERSION_MODEL_MASK) == VI6_IP_VERSION_MODEL_VSPD_GEN4) {

Please wrap this as

	if ((entity->vsp1->version & VI6_IP_VERSION_MODEL_MASK) ==
	    VI6_IP_VERSION_MODEL_VSPD_GEN4) {

You could also test entity->vsp1->gen == 4, up to you.

> +		u32 ext_infmt0;
> +		u32 ext_infmt1;
> +		u32 ext_infmt2;
> +
> +		switch (fmtinfo->fourcc) {
> +		case V4L2_PIX_FMT_XBGR2101010:
> +			ext_infmt0 = VI6_RPF_EXT_INFMT0_BYPP_M1_RGB10;
> +			ext_infmt1 = PACK_CPOS(0, 10, 20, 0);
> +			ext_infmt2 = PACK_CLEN(10, 10, 10, 0);

While this matches the detailed bit order of the V4L2 format as defined
in patch 1/7, it doesn't match the XBGR2101010 name, at least if
interpreted the same way as DRM_FORMAT_XBGR2101010. I think this should
be DRM_FORMAT_RGBX1010102 instead.

Same for the nex two formats, they should be RGBA1010102 and
ARGB2101010.

> +			break;
> +
> +		case V4L2_PIX_FMT_ABGR2101010:
> +			ext_infmt0 = VI6_RPF_EXT_INFMT0_BYPP_M1_RGB10;
> +			ext_infmt1 = PACK_CPOS(0, 10, 20, 30);
> +			ext_infmt2 = PACK_CLEN(10, 10, 10, 2);
> +			break;
> +
> +		case V4L2_PIX_FMT_BGRA1010102:
> +			ext_infmt0 = VI6_RPF_EXT_INFMT0_BYPP_M1_RGB10;
> +			ext_infmt1 = PACK_CPOS(2, 12, 22, 0);
> +			ext_infmt2 = PACK_CLEN(10, 10, 10, 2);
> +			break;
> +
> +		case V4L2_PIX_FMT_Y210:
> +			ext_infmt0 = VI6_RPF_EXT_INFMT0_F2B_MSB |
> +				     VI6_RPF_EXT_INFMT0_IPBD_Y_10 |
> +				     VI6_RPF_EXT_INFMT0_IPBD_C_10;
> +			ext_infmt1 = 0x0;
> +			ext_infmt2 = 0x0;
> +			break;
> +
> +		default:
> +			ext_infmt0 = 0;
> +			ext_infmt1 = 0;
> +			ext_infmt2 = 0;
> +			break;
> +		}
> +
> +		vsp1_rpf_write(rpf, dlb, VI6_RPF_EXT_INFMT0, ext_infmt0);
> +		vsp1_rpf_write(rpf, dlb, VI6_RPF_EXT_INFMT1, ext_infmt1);
> +		vsp1_rpf_write(rpf, dlb, VI6_RPF_EXT_INFMT2, ext_infmt2);
> +	}
> +
>  	/* Output location. */
>  	if (pipe->brx) {
>  		const struct v4l2_rect *compose;

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux