by Prabhakar Mahadev Lad

[permalink] [raw]

Subject: RE: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port

Hi Niklas,

Thank you for the review.

> -----Original Message-----
> From: Niklas <[email protected]>
> Sent: 10 March 2020 12:44
> To: Prabhakar Mahadev Lad <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]; Lad Prabhakar <[email protected]>
> Subject: Re: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode
> against remote or parent port
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-03-10 11:06:02 +0000, Lad Prabhakar wrote:
> > The rcar-csi2 driver uses the v4l2-async framework to do endpoint
> > matching instead of node matching. This is needed as it needs to work
> > with the adv748x driver which register it self in v4l2-async using
> > endpoints instead of nodes. The reason for this is that from a single
> > DT node it creates multiple subdevices, one for each endpoint.
> >
> > But when using subdevs which register itself in v4l2-async using
> > nodes, the rcar-csi2 driver failed to find the matching endpoint
> > because the match.fwnode was pointing to remote endpoint instead of
> > remote parent port.
> >
> > This commit adds support in rcar-csi2 driver to handle both the cases
> > where subdev registers in v4l2-async using endpoints/nodes, by using
> > match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the
> match()
> > callback to compare the fwnode of either remote/parent.
>
> This is a novel approach to the solution, and I won't object to it out right. But I
> think the proper solution is to move this logic into v4l2-async instead of
> adding a custom match handler in rcar-csi2.
>
> Think of the reveres use-case, a different CSI-2 receiver who wish to use the
> ADV748x would still have this node vs. endpoint issue.
>
Agreed.

> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> [email protected]>
> > ---
> > drivers/media/platform/rcar-vin/rcar-csi2.c | 41
> > ++++++++++++++++++++++++++---
> > 1 file changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index faa9fb2..39e1639 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -808,6 +808,41 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> > return 0;
> > }
> >
> > +static bool rcsi2_asd_match(struct device *dev,
> > + struct v4l2_async_subdev *async_sd) {
> > +struct rcar_csi2 *priv = (struct rcar_csi2 *)
> > + async_sd->match.custom.priv;
> > +struct fwnode_handle *endpoint;
> > +struct fwnode_handle *remote;
> > +struct fwnode_handle *parent;
> > +struct device_node *np;
> > +bool matched = false;
> > +
> > +np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > +if (!np) {
> > +dev_err(priv->dev, "Not connected to subdevice\n");
> > +return -EINVAL;
>
> You can't return -EINVAL here as it will be interpreted as a match by the caller
> ;-). You should not even register a device with v4l2-async if it's not
> connected to an endpoint.
>
My bad.

> > +}
> > +
> > +endpoint = of_fwnode_handle(np);
> > +remote = fwnode_graph_get_remote_endpoint(endpoint);
> > +parent = fwnode_graph_get_remote_port_parent(endpoint);
> > +if (parent) {
>
> This is wrong, we will always have a parent and will always take this code
> path. Hence reducing this to the equivalent of node only matching.
> I applied this patch and tried on M3-N with a ADv748x and the wrong
> endpoints of the ADV7482 is routed to the two CSI-2 receivers, breaking it.
>
I did try the media-tree on M3N but it has many issues for booting, could you please
point me to a tree which I can use for testing.

> I added some debug printouts to explain whats going on:
>
> * First call
> dev: rcar-csi2 fea80000.csi2
> endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
> remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
> parent: /soc/i2c@e66d8000/video-receiver@70
> dev->fwnode: /soc/csi2@fea80000
> dev->of_node: /soc/csi2@fea80000
> match: false
>
> * Second call
> dev: adv748x 4-0070
> endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
> remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
> parent: /soc/i2c@e66d8000/video-receiver@70
> dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
> dev->of_node: /soc/i2c@e66d8000/video-receiver@70
> match: true
>
> * Third call
> dev: adv748x 4-0070
> endpoint: /soc/csi2@fea80000/ports/port@0/endpoint
> remote: /soc/i2c@e66d8000/video-receiver@70/port@b/endpoint
> parent: /soc/i2c@e66d8000/video-receiver@70
> dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
> dev->of_node: /soc/i2c@e66d8000/video-receiver@70
> match: true
>
Thank you for the above.

> Now we have a media graph that is completely probed and video devices
> register in the system but you are not able to stream video as the wrong
> CSI-2 transmitter is described in the graph to be connected to the wrong
> receiver.
>
> This only strengthens my view that this should not be fixed with a custom
> matcher in rcar-csi2 but directly in v4l-async. Please see if you can't address
> the issue in the framework to allow node and endpoint matching to co-
> exists.
>
Ill do some more digging ????

> > +if (parent == dev->fwnode ||
> > + parent == &dev->of_node->fwnode)
> > +matched = true;
> > +} else if (remote && !matched) {
>
> No need to check !matched here ;-)
>
Agreed.

Cheers,
--Prabhakar

> > +if (remote == dev->fwnode ||
> > + remote == &dev->of_node->fwnode)
> > +matched = true;
> > +}
>
>
>
> > +
> > +of_node_put(np);
> > +
> > +return matched;
> > +}
> > +
> > static int rcsi2_parse_dt(struct rcar_csi2 *priv) {
> > struct device_node *ep;
> > @@ -833,9 +868,9 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> > return ret;
> > }
> >
> > -priv->asd.match.fwnode =
> > -
> fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > -priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +priv->asd.match.custom.match = &rcsi2_asd_match;
> > +priv->asd.match.custom.priv = priv;
> > +priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
> >
> > of_node_put(ep);
> >
> > --
> > 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

2020-03-19 15:06:05

by Niklas Söderlund

[permalink] [raw]

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 <[email protected]> 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 <[email protected]>
> > > > Sent: 10 March 2020 12:46
> > > > To: Prabhakar Mahadev Lad <[email protected]>
> > > > Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> > > > [email protected]; [email protected]; linux-
> > > > [email protected]; Lad Prabhakar <[email protected]>
> > > > 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-
> > > > [email protected]>
> > > > > ---
> > > > > 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

Attachments:

(No filename) (10.02 kB)
rggb2png.py (1.54 kB)
div1.png (67.54 kB)
div2.png (108.90 kB)
Download all attachments

2020-03-27 13:01:06

Hi Niklas,

On Tue, Apr 14, 2020 at 8:39 PM Niklas <[email protected]> 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