lkml.org 
[lkml]   [2020]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
    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

    \
     
     \ /
      Last update: 2020-04-06 19:21    [W:5.407 / U:0.276 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site