| Message ID | 1583838364-12932-3-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes | expand |
Hi Lad, Thanks for your work. On 2020-03-10 11:06:03 +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. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 1 + > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++- > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 7440c89..76daf2f 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: > 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 1a30cd0..1c1fafa 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,7 +588,6 @@ 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); > @@ -676,6 +676,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; > default: > break; > } > @@ -737,6 +740,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; > default: > vin_err(vin, "Invalid pixelformat (0x%x)\n", > vin->format.pixelformat); > @@ -1110,6 +1116,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; > break; > default: > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index 5151a3c..4698099 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 = 2, This does not look right, is not bytes-per-pixel 1 for a SRGGB8? > + }, > }; > > const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin, > @@ -102,6 +106,7 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin, > { > const struct rvin_video_format *fmt; > u32 align; > + u8 div; > > fmt = rvin_format_from_pixel(vin, pix->pixelformat); > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin, > case V4L2_PIX_FMT_NV12: > case V4L2_PIX_FMT_NV16: > align = 0x20; > + div = 1; > + break; > + case V4L2_PIX_FMT_SRGGB8: > + align = 0x10; > + div = 2; Yes this does not look right at all, I think you should set bpp to 1 and drop the div handling here. > break; > default: > align = 0x10; > + div = 1; > break; > } > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field)) > align = 0x80; > > - return ALIGN(pix->width, align) * fmt->bpp; > + return ALIGN(pix->width / div, align) * fmt->bpp; > } > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix) > -- > 2.7.4 >
Hi Niklas, Thank for the review. > -----Original Message----- > From: Niklas <niklas.soderlund@ragnatech.se> > Sent: 10 March 2020 12:46 > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux- > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux- > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com> > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for > MEDIA_BUS_FMT_SRGGB8_1X8 format > > Hi Lad, > > Thanks for your work. > > On 2020-03-10 11:06:03 +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. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev- > lad.rj@bp.renesas.com> > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 1 + > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++- > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++- > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c > > b/drivers/media/platform/rcar-vin/rcar-core.c > > index 7440c89..76daf2f 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: > > 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 1a30cd0..1c1fafa 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,7 +588,6 @@ 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); > > @@ -676,6 +676,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; > > default: > > break; > > } > > @@ -737,6 +740,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; > > default: > > vin_err(vin, "Invalid pixelformat (0x%x)\n", > > vin->format.pixelformat); > > @@ -1110,6 +1116,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; > > break; > > default: > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > index 5151a3c..4698099 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= 2, > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8? > I guessed the bpp's were picked from VnIS table as I result I did the same. > > +}, > > }; > > > > const struct rvin_video_format *rvin_format_from_pixel(struct > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32 > > rvin_format_bytesperline(struct rvin_dev *vin, { > > const struct rvin_video_format *fmt; > > u32 align; > > +u8 div; > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat); > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct > rvin_dev *vin, > > case V4L2_PIX_FMT_NV12: > > case V4L2_PIX_FMT_NV16: > > align = 0x20; > > +div = 1; > > +break; > > +case V4L2_PIX_FMT_SRGGB8: > > +align = 0x10; > > +div = 2; > > Yes this does not look right at all, I think you should set bpp to 1 and drop the > div handling here. > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in rvin_crop_scale_comp() and the image captured will be wrong. For example for 640x480: With the current patch bpp = 2: bytesperline = 640 image size = 307200 stride = 320 And with bpp = 1 and div removed bytesperline = 640 image size = 307200 stride = 640 Cheers, --Prabhakar > > break; > > default: > > align = 0x10; > > +div = 1; > > break; > > } > > > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field)) > > align = 0x80; > > > > -return ALIGN(pix->width, align) * fmt->bpp; > > +return ALIGN(pix->width / div, align) * fmt->bpp; > > } > > > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix) > > -- > > 2.7.4 > > > > -- > Regards, > Niklas Söderlund Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
Hi Lad, On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote: > Hi Niklas, > > Thank for the review. > > > -----Original Message----- > > From: Niklas <niklas.soderlund@ragnatech.se> > > Sent: 10 March 2020 12:46 > > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux- > > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux- > > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com> > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for > > MEDIA_BUS_FMT_SRGGB8_1X8 format > > > > Hi Lad, > > > > Thanks for your work. > > > > On 2020-03-10 11:06:03 +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. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev- > > lad.rj@bp.renesas.com> > > > --- > > > drivers/media/platform/rcar-vin/rcar-core.c | 1 + > > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++- > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++- > > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c > > > b/drivers/media/platform/rcar-vin/rcar-core.c > > > index 7440c89..76daf2f 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: > > > 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 1a30cd0..1c1fafa 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,7 +588,6 @@ 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); > > > @@ -676,6 +676,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; > > > default: > > > break; > > > } > > > @@ -737,6 +740,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; > > > default: > > > vin_err(vin, "Invalid pixelformat (0x%x)\n", > > > vin->format.pixelformat); > > > @@ -1110,6 +1116,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; > > > break; > > > default: > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > index 5151a3c..4698099 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= 2, > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8? > > > I guessed the bpp's were picked from VnIS table as I result I did the same. > > > > +}, > > > }; > > > > > > const struct rvin_video_format *rvin_format_from_pixel(struct > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32 > > > rvin_format_bytesperline(struct rvin_dev *vin, { > > > const struct rvin_video_format *fmt; > > > u32 align; > > > +u8 div; > > > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat); > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct > > rvin_dev *vin, > > > case V4L2_PIX_FMT_NV12: > > > case V4L2_PIX_FMT_NV16: > > > align = 0x20; > > > +div = 1; > > > +break; > > > +case V4L2_PIX_FMT_SRGGB8: > > > +align = 0x10; > > > +div = 2; > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the > > div handling here. > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in > rvin_crop_scale_comp() and the image captured will be wrong. > > For example for 640x480: > > With the current patch bpp = 2: > bytesperline = 640 This is wrong, if we have a line of 640 pixels and 2 bytes per pixel then bytesperline must be at least 1280 bytes right? > image size = 307200 > stride = 320 But this is incorrect, the VNIS_REG shall be at least the number of pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align it to the pixel unit (16, 32, 64, 128) depending on the output pixel format. This usually result in a stride that is larger then the line length. Thus you need a test application that knows the difference between width * bpp and bytesperline. I use qv4l2 without opengl support when I do quick tests and it does not support this hence I get a incorrect visual view of the stream when testing. How does the image capture fail with bpp = 1? > > And with bpp = 1 and div removed > bytesperline = 640 > image size = 307200 > stride = 640 > > Cheers, > --Prabhakar > > > > break; > > > default: > > > align = 0x10; > > > +div = 1; > > > break; > > > } > > > > > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field)) > > > align = 0x80; > > > > > > -return ALIGN(pix->width, align) * fmt->bpp; > > > +return ALIGN(pix->width / div, align) * fmt->bpp; > > > } > > > > > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix) > > > -- > > > 2.7.4 > > > > > > > -- > > Regards, > > Niklas Söderlund > > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
Hi Lad, Thanks for your reply. On 2020-03-27 12:59:52 +0000, Lad, Prabhakar wrote: > Hi Niklas, > > On Thu, Mar 19, 2020 at 3:03 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > > > Hi Prabhakar, > > > > Thanks for the sample files, sorry I did not have time before now to > > look at them. After doing so I believe I know what is wrong, see bellow. > > > > On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote: > > > Hi Niklas, > > > > > > On Tue, Mar 10, 2020 at 2:06 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > > > > > > > Hi Lad, > > > > > > > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote: > > > > > Hi Niklas, > > > > > > > > > > Thank for the review. > > > > > > > > > > > -----Original Message----- > > > > > > From: Niklas <niklas.soderlund@ragnatech.se> > > > > > > Sent: 10 March 2020 12:46 > > > > > > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux- > > > > > > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux- > > > > > > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com> > > > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for > > > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format > > > > > > > > > > > > Hi Lad, > > > > > > > > > > > > Thanks for your work. > > > > > > > > > > > > On 2020-03-10 11:06:03 +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. > > > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev- > > > > > > lad.rj@bp.renesas.com> > > > > > > > --- > > > > > > > drivers/media/platform/rcar-vin/rcar-core.c | 1 + > > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++- > > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++- > > > > > > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c > > > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c > > > > > > > index 7440c89..76daf2f 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: > > > > > > > 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 1a30cd0..1c1fafa 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,7 +588,6 @@ 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); > > > > > > > @@ -676,6 +676,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; > > > > > > > default: > > > > > > > break; > > > > > > > } > > > > > > > @@ -737,6 +740,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; > > > > > > > default: > > > > > > > vin_err(vin, "Invalid pixelformat (0x%x)\n", > > > > > > > vin->format.pixelformat); > > > > > > > @@ -1110,6 +1116,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; > > > > > > > break; > > > > > > > default: > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > > > index 5151a3c..4698099 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= 2, > > > > > > > > > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8? > > > > > > > > > > > I guessed the bpp's were picked from VnIS table as I result I did the same. > > > > > > > > > > > > +}, > > > > > > > }; > > > > > > > > > > > > > > const struct rvin_video_format *rvin_format_from_pixel(struct > > > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32 > > > > > > > rvin_format_bytesperline(struct rvin_dev *vin, { > > > > > > > const struct rvin_video_format *fmt; > > > > > > > u32 align; > > > > > > > +u8 div; > > > > > > > > > > > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat); > > > > > > > > > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct > > > > > > rvin_dev *vin, > > > > > > > case V4L2_PIX_FMT_NV12: > > > > > > > case V4L2_PIX_FMT_NV16: > > > > > > > align = 0x20; > > > > > > > +div = 1; > > > > > > > +break; > > > > > > > +case V4L2_PIX_FMT_SRGGB8: > > > > > > > +align = 0x10; > > > > > > > +div = 2; > > > > > > > > > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the > > > > > > div handling here. > > > > > > > > > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in > > > > > rvin_crop_scale_comp() and the image captured will be wrong. > > > > > > > > > > For example for 640x480: > > > > > > > > > > With the current patch bpp = 2: > > > > > bytesperline = 640 > > > > > > > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel > > > > then bytesperline must be at least 1280 bytes right? > > > > > > > > > image size = 307200 > > > > > stride = 320 > > > > > > > > But this is incorrect, the VNIS_REG shall be at least the number of > > > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align > > > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel > > > > format. > > > > > > > > This usually result in a stride that is larger then the line length. > > > > Thus you need a test application that knows the difference between width > > > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick > > > > tests and it does not support this hence I get a incorrect visual view > > > > of the stream when testing. > > > > > > > > How does the image capture fail with bpp = 1? > > > > > > > Attached is the captured bayer images 640x480 with bpp set to 1, for > > > file1bppstridediv1.raw > > > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw > > > VNIS_REF stride set to (640 * 1) / 2. > > > When the file1bppstridediv1.raw image is converted to png colors are incorrect > > > but whereas file1bppstridediv2.raw converted to png the picture is clear. > > > > > > Also while doing a loop-back to fbdevsink with the below pipeline: > > > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer, > > > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb ! > > > videoconvert > > > ! fbdevsink sync=false > > > > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320 > > > works correctly > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640 > > > image displayed is incorrect > > > > It's very unlogical to have a stride that is less then the width, which > > got me interested why the second one gave you better results. I wrote a > > small python hack which converts the raw SRGGB8 to PNG without any > > debyaer, just rows of RGRGRG and BGBGBG. > > > Finally I have some information from the hardware team, the VIN process RAW8 > in 2 pixel units as a result the stride for RAW8 needs to be > configured as bytesperline/2. Interesting, that is not how I have interpreted the datasheet. But rereading it now after our discussion I see how it could be so. I will dig into it during the week and see if I get make it all click in my head. Thanks for pointing this out. > > The python script which you attached doesn't seem to do the right > conversion. I discovered > that Shotwell Viewer on Ubuntu can open raw files. I also confirmed > this by bayer2rg [1] Oops, you are right. My bad sorry for sending you down that path. > > # ./bayer2rgb --input=file1bppstridediv1.raw --output=file1.tiff > --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff > # ./bayer2rgb --input=file1bppstridediv2.raw --output=file2.tiff > --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff > > # convert file1.tiff file1bppstridediv1.png > # convert file2.tiff file1bppstridediv2.png > > Attached are the png images for reference. > > > Looking at the output of that seems your sensor is not sending frames of > > 640x480 but 480x640. Both the raw files you sent have holes in them. > > The first line is always 480 pixels of data and then there are sections > > of no data, followed by good data. Some rows are chopped and some have > > their 480 bytes of good data on the "left" and some on the "right" side > > of the frame. > > > I can confirm the sensor is sending 640x480 as the support for same > was added recently in IMX219 driver > and was was tested on raspi by the maintainer. > > [1] https://github.com/jdthomas/bayer2rgb > > Cheers, > --Prabhakar > > > So for rcar-vin I think the following settings are what you want, > > > > width = 480 height = 640 bpp = 1, bytesperline = 480* stride = 480 > > * = I have not checked if this fits with alignment for VNIS > > > > I have attached the python hack and the two generated png files from > > your raw files so you can play with them yourself. > > > > > > > > Cheers, > > > --Prabhakar > > > > > > > > > > > > > And with bpp = 1 and div removed > > > > > bytesperline = 640 > > > > > image size = 307200 > > > > > stride = 640 > > > > > > > > > > > > > > > > > > Cheers, > > > > > --Prabhakar > > > > > > > > > > > > break; > > > > > > > default: > > > > > > > align = 0x10; > > > > > > > +div = 1; > > > > > > > break; > > > > > > > } > > > > > > > > > > > > > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field)) > > > > > > > align = 0x80; > > > > > > > > > > > > > > -return ALIGN(pix->width, align) * fmt->bpp; > > > > > > > +return ALIGN(pix->width / div, align) * fmt->bpp; > > > > > > > } > > > > > > > > > > > > > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix) > > > > > > > -- > > > > > > > 2.7.4 > > > > > > > > > > > > > > > > > > > -- > > > > > > Regards, > > > > > > Niklas Söderlund > > > > > > > > > > > > > > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647 > > > > > > > > -- > > > > Regards, > > > > Niklas Söderlund > > > > > > > > > > -- > > Regards, > > Niklas Söderlund
Hi Niklas, On Mon, Mar 30, 2020 at 1:07 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > Hi Lad, > > Thanks for your reply. > > On 2020-03-27 12:59:52 +0000, Lad, Prabhakar wrote: > > Hi Niklas, > > > > On Thu, Mar 19, 2020 at 3:03 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > > > > > Hi Prabhakar, > > > > > > Thanks for the sample files, sorry I did not have time before now to > > > look at them. After doing so I believe I know what is wrong, see bellow. > > > > > > On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote: > > > > Hi Niklas, > > > > > > > > On Tue, Mar 10, 2020 at 2:06 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > > > > > > > > > Hi Lad, > > > > > > > > > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote: > > > > > > Hi Niklas, > > > > > > > > > > > > Thank for the review. > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Niklas <niklas.soderlund@ragnatech.se> > > > > > > > Sent: 10 March 2020 12:46 > > > > > > > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux- > > > > > > > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux- > > > > > > > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com> > > > > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for > > > > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format > > > > > > > > > > > > > > Hi Lad, > > > > > > > > > > > > > > Thanks for your work. > > > > > > > > > > > > > > On 2020-03-10 11:06:03 +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. > > > > > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev- > > > > > > > lad.rj@bp.renesas.com> > > > > > > > > --- > > > > > > > > drivers/media/platform/rcar-vin/rcar-core.c | 1 + > > > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++- > > > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++- > > > > > > > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c > > > > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c > > > > > > > > index 7440c89..76daf2f 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: > > > > > > > > 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 1a30cd0..1c1fafa 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,7 +588,6 @@ 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); > > > > > > > > @@ -676,6 +676,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; > > > > > > > > default: > > > > > > > > break; > > > > > > > > } > > > > > > > > @@ -737,6 +740,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; > > > > > > > > default: > > > > > > > > vin_err(vin, "Invalid pixelformat (0x%x)\n", > > > > > > > > vin->format.pixelformat); > > > > > > > > @@ -1110,6 +1116,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; > > > > > > > > break; > > > > > > > > default: > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > > > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > > > > index 5151a3c..4698099 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= 2, > > > > > > > > > > > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8? > > > > > > > > > > > > > I guessed the bpp's were picked from VnIS table as I result I did the same. > > > > > > > > > > > > > > +}, > > > > > > > > }; > > > > > > > > > > > > > > > > const struct rvin_video_format *rvin_format_from_pixel(struct > > > > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32 > > > > > > > > rvin_format_bytesperline(struct rvin_dev *vin, { > > > > > > > > const struct rvin_video_format *fmt; > > > > > > > > u32 align; > > > > > > > > +u8 div; > > > > > > > > > > > > > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat); > > > > > > > > > > > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct > > > > > > > rvin_dev *vin, > > > > > > > > case V4L2_PIX_FMT_NV12: > > > > > > > > case V4L2_PIX_FMT_NV16: > > > > > > > > align = 0x20; > > > > > > > > +div = 1; > > > > > > > > +break; > > > > > > > > +case V4L2_PIX_FMT_SRGGB8: > > > > > > > > +align = 0x10; > > > > > > > > +div = 2; > > > > > > > > > > > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the > > > > > > > div handling here. > > > > > > > > > > > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in > > > > > > rvin_crop_scale_comp() and the image captured will be wrong. > > > > > > > > > > > > For example for 640x480: > > > > > > > > > > > > With the current patch bpp = 2: > > > > > > bytesperline = 640 > > > > > > > > > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel > > > > > then bytesperline must be at least 1280 bytes right? > > > > > > > > > > > image size = 307200 > > > > > > stride = 320 > > > > > > > > > > But this is incorrect, the VNIS_REG shall be at least the number of > > > > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align > > > > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel > > > > > format. > > > > > > > > > > This usually result in a stride that is larger then the line length. > > > > > Thus you need a test application that knows the difference between width > > > > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick > > > > > tests and it does not support this hence I get a incorrect visual view > > > > > of the stream when testing. > > > > > > > > > > How does the image capture fail with bpp = 1? > > > > > > > > > Attached is the captured bayer images 640x480 with bpp set to 1, for > > > > file1bppstridediv1.raw > > > > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw > > > > VNIS_REF stride set to (640 * 1) / 2. > > > > When the file1bppstridediv1.raw image is converted to png colors are incorrect > > > > but whereas file1bppstridediv2.raw converted to png the picture is clear. > > > > > > > > Also while doing a loop-back to fbdevsink with the below pipeline: > > > > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer, > > > > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb ! > > > > videoconvert > > > > ! fbdevsink sync=false > > > > > > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320 > > > > works correctly > > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640 > > > > image displayed is incorrect > > > > > > It's very unlogical to have a stride that is less then the width, which > > > got me interested why the second one gave you better results. I wrote a > > > small python hack which converts the raw SRGGB8 to PNG without any > > > debyaer, just rows of RGRGRG and BGBGBG. > > > > > Finally I have some information from the hardware team, the VIN process RAW8 > > in 2 pixel units as a result the stride for RAW8 needs to be > > configured as bytesperline/2. > > Interesting, that is not how I have interpreted the datasheet. But > rereading it now after our discussion I see how it could be so. I will > dig into it during the week and see if I get make it all click in my > head. Thanks for pointing this out. > Great, In that case Ill wait before I post a V4. > > > > The python script which you attached doesn't seem to do the right > > conversion. I discovered > > that Shotwell Viewer on Ubuntu can open raw files. I also confirmed > > this by bayer2rg [1] > > Oops, you are right. My bad sorry for sending you down that path. > That gave me an opportunity to explore a bit :) Cheers, --Prabhakar > > > > # ./bayer2rgb --input=file1bppstridediv1.raw --output=file1.tiff > > --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff > > # ./bayer2rgb --input=file1bppstridediv2.raw --output=file2.tiff > > --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff > > > > # convert file1.tiff file1bppstridediv1.png > > # convert file2.tiff file1bppstridediv2.png > > > > Attached are the png images for reference. > > > > > Looking at the output of that seems your sensor is not sending frames of > > > 640x480 but 480x640. Both the raw files you sent have holes in them. > > > The first line is always 480 pixels of data and then there are sections > > > of no data, followed by good data. Some rows are chopped and some have > > > their 480 bytes of good data on the "left" and some on the "right" side > > > of the frame. > > > > > I can confirm the sensor is sending 640x480 as the support for same > > was added recently in IMX219 driver > > and was was tested on raspi by the maintainer. > > > > [1] https://github.com/jdthomas/bayer2rgb > > > > Cheers, > > --Prabhakar > > > > > So for rcar-vin I think the following settings are what you want, > > > > > > width = 480 height = 640 bpp = 1, bytesperline = 480* stride = 480 > > > * = I have not checked if this fits with alignment for VNIS > > > > > > I have attached the python hack and the two generated png files from > > > your raw files so you can play with them yourself. > > > > > > > > > > > Cheers, > > > > --Prabhakar > > > > > > > > > > > > > > > > And with bpp = 1 and div removed > > > > > > bytesperline = 640 > > > > > > image size = 307200 > > > > > > stride = 640 > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > --Prabhakar > > > > > > > > > > > > > > break; > > > > > > > > default: > > > > > > > > align = 0x10; > > > > > > > > +div = 1; > > > > > > > > break; > > > > > > > > } > > > > > > > > > > > > > > > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field)) > > > > > > > > align = 0x80; > > > > > > > > > > > > > > > > -return ALIGN(pix->width, align) * fmt->bpp; > > > > > > > > +return ALIGN(pix->width / div, align) * fmt->bpp; > > > > > > > > } > > > > > > > > > > > > > > > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix) > > > > > > > > -- > > > > > > > > 2.7.4 > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Regards, > > > > > > > Niklas Söderlund > > > > > > > > > > > > > > > > > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647 > > > > > > > > > > -- > > > > > Regards, > > > > > Niklas Söderlund > > > > > > > > > > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > > > -- > Regards, > Niklas Söderlund
HI Niklas, On Mon, Mar 30, 2020 at 1:07 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > Hi Lad, > > Thanks for your reply. > > On 2020-03-27 12:59:52 +0000, Lad, Prabhakar wrote: > > Hi Niklas, > > > > On Thu, Mar 19, 2020 at 3:03 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > > > > > Hi Prabhakar, > > > > > > Thanks for the sample files, sorry I did not have time before now to > > > look at them. After doing so I believe I know what is wrong, see bellow. > > > > > > On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote: > > > > Hi Niklas, > > > > > > > > On Tue, Mar 10, 2020 at 2:06 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > > > > > > > > > Hi Lad, > > > > > > > > > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote: > > > > > > Hi Niklas, > > > > > > > > > > > > Thank for the review. > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Niklas <niklas.soderlund@ragnatech.se> > > > > > > > Sent: 10 March 2020 12:46 > > > > > > > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux- > > > > > > > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux- > > > > > > > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com> > > > > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for > > > > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format > > > > > > > > > > > > > > Hi Lad, > > > > > > > > > > > > > > Thanks for your work. > > > > > > > > > > > > > > On 2020-03-10 11:06:03 +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. > > > > > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev- > > > > > > > lad.rj@bp.renesas.com> > > > > > > > > --- > > > > > > > > drivers/media/platform/rcar-vin/rcar-core.c | 1 + > > > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++- > > > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++- > > > > > > > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c > > > > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c > > > > > > > > index 7440c89..76daf2f 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: > > > > > > > > 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 1a30cd0..1c1fafa 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,7 +588,6 @@ 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); > > > > > > > > @@ -676,6 +676,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; > > > > > > > > default: > > > > > > > > break; > > > > > > > > } > > > > > > > > @@ -737,6 +740,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; > > > > > > > > default: > > > > > > > > vin_err(vin, "Invalid pixelformat (0x%x)\n", > > > > > > > > vin->format.pixelformat); > > > > > > > > @@ -1110,6 +1116,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; > > > > > > > > break; > > > > > > > > default: > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > > > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > > > > index 5151a3c..4698099 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= 2, > > > > > > > > > > > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8? > > > > > > > > > > > > > I guessed the bpp's were picked from VnIS table as I result I did the same. > > > > > > > > > > > > > > +}, > > > > > > > > }; > > > > > > > > > > > > > > > > const struct rvin_video_format *rvin_format_from_pixel(struct > > > > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32 > > > > > > > > rvin_format_bytesperline(struct rvin_dev *vin, { > > > > > > > > const struct rvin_video_format *fmt; > > > > > > > > u32 align; > > > > > > > > +u8 div; > > > > > > > > > > > > > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat); > > > > > > > > > > > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct > > > > > > > rvin_dev *vin, > > > > > > > > case V4L2_PIX_FMT_NV12: > > > > > > > > case V4L2_PIX_FMT_NV16: > > > > > > > > align = 0x20; > > > > > > > > +div = 1; > > > > > > > > +break; > > > > > > > > +case V4L2_PIX_FMT_SRGGB8: > > > > > > > > +align = 0x10; > > > > > > > > +div = 2; > > > > > > > > > > > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the > > > > > > > div handling here. > > > > > > > > > > > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in > > > > > > rvin_crop_scale_comp() and the image captured will be wrong. > > > > > > > > > > > > For example for 640x480: > > > > > > > > > > > > With the current patch bpp = 2: > > > > > > bytesperline = 640 > > > > > > > > > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel > > > > > then bytesperline must be at least 1280 bytes right? > > > > > > > > > > > image size = 307200 > > > > > > stride = 320 > > > > > > > > > > But this is incorrect, the VNIS_REG shall be at least the number of > > > > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align > > > > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel > > > > > format. > > > > > > > > > > This usually result in a stride that is larger then the line length. > > > > > Thus you need a test application that knows the difference between width > > > > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick > > > > > tests and it does not support this hence I get a incorrect visual view > > > > > of the stream when testing. > > > > > > > > > > How does the image capture fail with bpp = 1? > > > > > > > > > Attached is the captured bayer images 640x480 with bpp set to 1, for > > > > file1bppstridediv1.raw > > > > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw > > > > VNIS_REF stride set to (640 * 1) / 2. > > > > When the file1bppstridediv1.raw image is converted to png colors are incorrect > > > > but whereas file1bppstridediv2.raw converted to png the picture is clear. > > > > > > > > Also while doing a loop-back to fbdevsink with the below pipeline: > > > > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer, > > > > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb ! > > > > videoconvert > > > > ! fbdevsink sync=false > > > > > > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320 > > > > works correctly > > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640 > > > > image displayed is incorrect > > > > > > It's very unlogical to have a stride that is less then the width, which > > > got me interested why the second one gave you better results. I wrote a > > > small python hack which converts the raw SRGGB8 to PNG without any > > > debyaer, just rows of RGRGRG and BGBGBG. > > > > > Finally I have some information from the hardware team, the VIN process RAW8 > > in 2 pixel units as a result the stride for RAW8 needs to be > > configured as bytesperline/2. > > Interesting, that is not how I have interpreted the datasheet. But > rereading it now after our discussion I see how it could be so. I will > dig into it during the week and see if I get make it all click in my > head. Thanks for pointing this out. > Did you manage to get the required information on this ? Cheers, --Prabhakar
Hi Lad,
On 2020-04-06 18:20:33 +0100, Lad, Prabhakar wrote:
> Did you manage to get the required information on this ?
I'm still working on it, sorry for not completing it last week. I will
let you know as soon as I can.
Hi Lad,
I spent all day playing with different solutions to how to move forward
with this. My main problem is I have no setup where I can produce RAW
image formats to test. But reading the datasheet I see the problem you
are trying to solve.
I think for now the best solution might be to in rvin_crop_scale_comp()
add a check for if the pixelformat is RAW and cut the value written to
VNIS_REG in half. The bpp for the format shall still be set to 1.
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;
rvin_write(vin, stride, VNIS_REG);
I would also add a nice big comment above the if () that explains why
the stride is cut in half for raw.
On 2020-04-07 11:56:23 +0200, Niklas wrote:
> Hi Lad,
>
> On 2020-04-06 18:20:33 +0100, Lad, Prabhakar wrote:
> > Did you manage to get the required information on this ?
>
> I'm still working on it, sorry for not completing it last week. I will
> let you know as soon as I can.
>
> --
> Regards,
> Niklas Söderlund
Hi Niklas, On Tue, Apr 14, 2020 at 8:39 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > Hi Lad, > > I spent all day playing with different solutions to how to move forward > with this. My main problem is I have no setup where I can produce RAW > image formats to test. But reading the datasheet I see the problem you > are trying to solve. > Thank you for looking into this. > I think for now the best solution might be to in rvin_crop_scale_comp() > add a check for if the pixelformat is RAW and cut the value written to > VNIS_REG in half. The bpp for the format shall still be set to 1. > > > 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; > > rvin_write(vin, stride, VNIS_REG); > > I would also add a nice big comment above the if () that explains why > the stride is cut in half for raw. > Agreed shall do that as above. Cheers, --Prabhakar > On 2020-04-07 11:56:23 +0200, Niklas wrote: > > Hi Lad, > > > > On 2020-04-06 18:20:33 +0100, Lad, Prabhakar wrote: > > > Did you manage to get the required information on this ? > > > > I'm still working on it, sorry for not completing it last week. I will > > let you know as soon as I can. > > > > -- > > Regards, > > Niklas Söderlund > > -- > Regards, > Niklas Söderlund
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index 7440c89..76daf2f 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: 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 1a30cd0..1c1fafa 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,7 +588,6 @@ 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); @@ -676,6 +676,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; default: break; } @@ -737,6 +740,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; default: vin_err(vin, "Invalid pixelformat (0x%x)\n", vin->format.pixelformat); @@ -1110,6 +1116,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; break; default: diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 5151a3c..4698099 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 = 2, + }, }; const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin, @@ -102,6 +106,7 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin, { const struct rvin_video_format *fmt; u32 align; + u8 div; fmt = rvin_format_from_pixel(vin, pix->pixelformat); @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin, case V4L2_PIX_FMT_NV12: case V4L2_PIX_FMT_NV16: align = 0x20; + div = 1; + break; + case V4L2_PIX_FMT_SRGGB8: + align = 0x10; + div = 2; break; default: align = 0x10; + div = 1; break; } if (V4L2_FIELD_IS_SEQUENTIAL(pix->field)) align = 0x80; - return ALIGN(pix->width, align) * fmt->bpp; + return ALIGN(pix->width / div, align) * fmt->bpp; } static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
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. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/media/platform/rcar-vin/rcar-core.c | 1 + drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++- drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++- 3 files changed, 21 insertions(+), 2 deletions(-)