Re: [PATCH v3 1/2] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Niklas]

Hi Prabhakar,

Thanks for your work.

On 2020-03-18 17:06:37 +0000, Lad Prabhakar wrote:
> Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by setting
> format type to RAW8 in VNMC register and appropriately setting the
> bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.

> For RAW8 format data is transferred by 4-Byte unit so VnIS register is
> configured accordingly.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c |  1 +
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 11 ++++++++++-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  4 ++++
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 7440c8965d27..76daf2fe5bcd 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  		case MEDIA_BUS_FMT_UYVY8_2X8:
>  		case MEDIA_BUS_FMT_UYVY10_2X10:
>  		case MEDIA_BUS_FMT_RGB888_1X24:
> +		case MEDIA_BUS_FMT_SRGGB8_1X8:

This is wrong RAW formats are only supported on the CSI-2 interface and 
not the parallel one. This line shall be dropped.

>  			vin->mbus_code = code.code;
>  			vin_dbg(vin, "Found media bus format for %s: %d\n",
>  				subdev->name, vin->mbus_code);
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 1a30cd036371..ec7b49c0b281 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -85,6 +85,7 @@
>  #define VNMC_INF_YUV8_BT601	(1 << 16)
>  #define VNMC_INF_YUV10_BT656	(2 << 16)
>  #define VNMC_INF_YUV10_BT601	(3 << 16)
> +#define VNMC_INF_RAW8		(4 << 16)
>  #define VNMC_INF_YUV16		(5 << 16)
>  #define VNMC_INF_RGB888		(6 << 16)
>  #define VNMC_VUP		(1 << 10)
> @@ -587,13 +588,14 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  	rvin_write(vin, vin->crop.top, VNSLPRC_REG);
>  	rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
>  
> -
>  	/* TODO: Add support for the UDS scaler. */
>  	if (vin->info->model != RCAR_GEN3)
>  		rvin_crop_scale_comp_gen2(vin);
>  
>  	fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
>  	stride = vin->format.bytesperline / fmt->bpp;
> +	if (vin->format.pixelformat == V4L2_PIX_FMT_SRGGB8)
> +		stride /= 2;

I'm sorry this makes no sens to me.

- The width of the image is number of pixels in the raw format.
- In memory each row is either is either RGRGRG... or GBGBGB...
- The pixel size is 1 byte per pixel.
- We calculate bytesperline as ALIGN(width, align) * bpp, where align is 
  how much we need to "adjust" the width to match the VNIS_REG reg 
  value.  We do this in rvin_format_bytesperline().
- We then remove bpp from bytesperline and we have a unit in pixels 
  which is our stride.

I can't see why you need to cut the stride in half. In my view you 
should add a check for V4L2_PIX_FMT_SRGGB8 in rvin_format_bytesperline() 
and pick an alignment value that matches the restrictions.

I might miss something, but then I wish to learn.

>  	rvin_write(vin, stride, VNIS_REG);
>  }
>  
> @@ -676,6 +678,9 @@ static int rvin_setup(struct rvin_dev *vin)
>  
>  		input_is_yuv = true;
>  		break;
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +		vnmc |= VNMC_INF_RAW8;
> +		break;

Here and ...

>  	default:
>  		break;
>  	}
> @@ -737,6 +742,9 @@ static int rvin_setup(struct rvin_dev *vin)
>  	case V4L2_PIX_FMT_ABGR32:
>  		dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB;
>  		break;
> +	case V4L2_PIX_FMT_SRGGB8:
> +		dmr = 0;
> +		break;

... here we have a new problem, sorry for not thinking of it before.

Up until now the VIN was capable to convert any of its supported input 
mbus formats to any of it's supported output pixel formats. With the 
addition of RAW formats this is no longer true. This new restriction 
needs to be added to the driver.

Luck has it we can fix ...

>  	default:
>  		vin_err(vin, "Invalid pixelformat (0x%x)\n",
>  			vin->format.pixelformat);
> @@ -1110,6 +1118,7 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>  	case MEDIA_BUS_FMT_UYVY10_2X10:
>  	case MEDIA_BUS_FMT_RGB888_1X24:
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
>  		vin->mbus_code = fmt.format.code;

... this here by changes this to

        switch (fmt.format.code) {
        case MEDIA_BUS_FMT_YUYV8_1X16:
        case MEDIA_BUS_FMT_UYVY8_1X16:
        case MEDIA_BUS_FMT_UYVY8_2X8:
        case MEDIA_BUS_FMT_UYVY10_2X10:
                break;
        case MEDIA_BUS_FMT_RGB888_1X24:
                if (vin->format.pixelformat != V4L2_PIX_FMT_SRGGB8)
                    return -EPIPE:
                break;
        default:
                return -EPIPE;
	}

        vin->mbus_code = fmt.format.code;

>  		break;
>  	default:
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 5151a3cd8a6e..ca542219e8ae 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[] = {
>  		.fourcc			= V4L2_PIX_FMT_ABGR32,
>  		.bpp			= 4,
>  	},
> +	{
> +		.fourcc			= V4L2_PIX_FMT_SRGGB8,
> +		.bpp			= 1,
> +	},
>  };
>  
>  const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin,
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund