Message-ID: <20200319150329.GB3192108@oden.dyn.berto.se>
Date:   Thu, 19 Mar 2020 16:03:29 +0100
From:   Niklas <niklas.soderlund@...natech.se>
To:     "Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc:     Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
 MEDIA_BUS_FMT_SRGGB8_1X8 format

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@...natech.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@...natech.se>
> > > > Sent: 10 March 2020 12:46
> > > > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>
> > > > Cc: Mauro Carvalho Chehab <mchehab@...nel.org>; linux-
> > > > media@...r.kernel.org; linux-renesas-soc@...r.kernel.org; linux-
> > > > kernel@...r.kernel.org; Lad Prabhakar <prabhakar.csengg@...il.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@...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.

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.

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

View attachment "rggb2png.py" of type "text/plain" (1514 bytes)

Download attachment "div1.png" of type "image/png" (69165 bytes)

Download attachment "div2.png" of type "image/png" (111515 bytes)

Powered by blists - more mailing lists