* [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes @ 2020-03-10 11:06 Lad Prabhakar 2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Lad Prabhakar @ 2020-03-10 11:06 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Cc: linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar, Lad Prabhakar First patch of the series adds support for matching fwnode against endpoints/nodes which are registered in v4l2-async and the later adds supports for MEDIA_BUS_FMT_SRGGB8_1X8 format. Changes for v2: 1: Added support for matching fwnode against endpoints/nodes. 2: Seperated patch for rcar-vin and rcar-csi2.c which added MEDIA_BUS_FMT_SRGGB8_1X8. Lad Prabhakar (3): media: rcar-csi2: Add support to match fwnode against remote or parent port media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format drivers/media/platform/rcar-vin/rcar-core.c | 1 + drivers/media/platform/rcar-vin/rcar-csi2.c | 42 ++++++++++++++++++++++++++--- drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++- drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++- 4 files changed, 60 insertions(+), 5 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port 2020-03-10 11:06 [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes Lad Prabhakar @ 2020-03-10 11:06 ` Lad Prabhakar 2020-03-10 12:43 ` Niklas 2020-03-10 11:06 ` [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format Lad Prabhakar 2020-03-10 11:06 ` [PATCH v2 3/3] media: rcar-vin: rcar-csi2: " Lad Prabhakar 2 siblings, 1 reply; 17+ messages in thread From: Lad Prabhakar @ 2020-03-10 11:06 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Cc: linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar, Lad Prabhakar 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. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- 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; + } + + endpoint = of_fwnode_handle(np); + remote = fwnode_graph_get_remote_endpoint(endpoint); + parent = fwnode_graph_get_remote_port_parent(endpoint); + if (parent) { + if (parent == dev->fwnode || + parent == &dev->of_node->fwnode) + matched = true; + } else if (remote && !matched) { + 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port 2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar @ 2020-03-10 12:43 ` Niklas 2020-03-10 14:54 ` Prabhakar Mahadev Lad 0 siblings, 1 reply; 17+ messages in thread From: Niklas @ 2020-03-10 12:43 UTC (permalink / raw) To: Lad Prabhakar Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar 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. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > 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. > + } > + > + 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 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 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. > + if (parent == dev->fwnode || > + parent == &dev->of_node->fwnode) > + matched = true; > + } else if (remote && !matched) { No need to check !matched here ;-) > + 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port 2020-03-10 12:43 ` Niklas @ 2020-03-10 14:54 ` Prabhakar Mahadev Lad 0 siblings, 0 replies; 17+ messages in thread From: Prabhakar Mahadev Lad @ 2020-03-10 14:54 UTC (permalink / raw) To: Niklas Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar Hi Niklas, Thank you for the review. > -----Original Message----- > From: Niklas <niklas.soderlund@ragnatech.se> > Sent: 10 March 2020 12:44 > 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 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- > lad.rj@bp.renesas.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-03-10 11:06 [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes Lad Prabhakar 2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar @ 2020-03-10 11:06 ` Lad Prabhakar 2020-03-10 12:46 ` Niklas 2020-03-10 11:06 ` [PATCH v2 3/3] media: rcar-vin: rcar-csi2: " Lad Prabhakar 2 siblings, 1 reply; 17+ messages in thread From: Lad Prabhakar @ 2020-03-10 11:06 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Cc: linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar, Lad Prabhakar 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, + }, }; 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) -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-03-10 11:06 ` [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format Lad Prabhakar @ 2020-03-10 12:46 ` Niklas 2020-03-10 13:42 ` Prabhakar Mahadev Lad 0 siblings, 1 reply; 17+ messages in thread From: Niklas @ 2020-03-10 12:46 UTC (permalink / raw) To: Lad Prabhakar Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar 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 > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-03-10 12:46 ` Niklas @ 2020-03-10 13:42 ` Prabhakar Mahadev Lad 2020-03-10 14:06 ` Niklas 0 siblings, 1 reply; 17+ messages in thread From: Prabhakar Mahadev Lad @ 2020-03-10 13:42 UTC (permalink / raw) To: Niklas Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-03-10 13:42 ` Prabhakar Mahadev Lad @ 2020-03-10 14:06 ` Niklas [not found] ` <CA+V-a8vsYGdx6AtgqwS0LXREn4hu-EjVh2D5Dp_rHmpazBYG5A@mail.gmail.com> 0 siblings, 1 reply; 17+ messages in thread From: Niklas @ 2020-03-10 14:06 UTC (permalink / raw) To: Prabhakar Mahadev Lad Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar 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 -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CA+V-a8vsYGdx6AtgqwS0LXREn4hu-EjVh2D5Dp_rHmpazBYG5A@mail.gmail.com>]
[parent not found: <20200319150329.GB3192108@oden.dyn.berto.se>]
[parent not found: <CA+V-a8u8=H-6WfaYMLWH73zo5ehP8cu9D-tdGULk=Hkvq4KuAQ@mail.gmail.com>]
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format [not found] ` <CA+V-a8u8=H-6WfaYMLWH73zo5ehP8cu9D-tdGULk=Hkvq4KuAQ@mail.gmail.com> @ 2020-03-30 12:07 ` Niklas 2020-03-30 13:13 ` Lad, Prabhakar 2020-04-06 17:20 ` Lad, Prabhakar 0 siblings, 2 replies; 17+ messages in thread From: Niklas @ 2020-03-30 12:07 UTC (permalink / raw) To: Lad, Prabhakar Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel 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 -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-03-30 12:07 ` Niklas @ 2020-03-30 13:13 ` Lad, Prabhakar 2020-04-06 17:20 ` Lad, Prabhakar 1 sibling, 0 replies; 17+ messages in thread From: Lad, Prabhakar @ 2020-03-30 13:13 UTC (permalink / raw) To: Niklas Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-03-30 12:07 ` Niklas 2020-03-30 13:13 ` Lad, Prabhakar @ 2020-04-06 17:20 ` Lad, Prabhakar 2020-04-07 9:56 ` Niklas 1 sibling, 1 reply; 17+ messages in thread From: Lad, Prabhakar @ 2020-04-06 17:20 UTC (permalink / raw) To: Niklas Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-04-06 17:20 ` Lad, Prabhakar @ 2020-04-07 9:56 ` Niklas 2020-04-14 19:39 ` Niklas 0 siblings, 1 reply; 17+ messages in thread From: Niklas @ 2020-04-07 9:56 UTC (permalink / raw) To: Lad, Prabhakar Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-04-07 9:56 ` Niklas @ 2020-04-14 19:39 ` Niklas 2020-04-15 8:21 ` Lad, Prabhakar 0 siblings, 1 reply; 17+ messages in thread From: Niklas @ 2020-04-14 19:39 UTC (permalink / raw) To: Lad, Prabhakar Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel 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 -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-04-14 19:39 ` Niklas @ 2020-04-15 8:21 ` Lad, Prabhakar 0 siblings, 0 replies; 17+ messages in thread From: Lad, Prabhakar @ 2020-04-15 8:21 UTC (permalink / raw) To: Niklas Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-03-10 11:06 [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes Lad Prabhakar 2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar 2020-03-10 11:06 ` [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format Lad Prabhakar @ 2020-03-10 11:06 ` Lad Prabhakar 2020-03-10 12:48 ` Niklas 2 siblings, 1 reply; 17+ messages in thread From: Lad Prabhakar @ 2020-03-10 11:06 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Cc: linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar, Lad Prabhakar This patch adds support for MEDIA_BUS_FMT_SRGGB8_1X8 format for CSI2 input. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/media/platform/rcar-vin/rcar-csi2.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c index 39e1639..b030ef6 100644 --- a/drivers/media/platform/rcar-vin/rcar-csi2.c +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c @@ -320,6 +320,7 @@ static const struct rcar_csi2_format rcar_csi2_formats[] = { { .code = MEDIA_BUS_FMT_YUYV8_1X16, .datatype = 0x1e, .bpp = 16 }, { .code = MEDIA_BUS_FMT_UYVY8_2X8, .datatype = 0x1e, .bpp = 16 }, { .code = MEDIA_BUS_FMT_YUYV10_2X10, .datatype = 0x1e, .bpp = 20 }, + { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = 0x2a, .bpp = 8 }, }; static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code) -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-03-10 11:06 ` [PATCH v2 3/3] media: rcar-vin: rcar-csi2: " Lad Prabhakar @ 2020-03-10 12:48 ` Niklas 2020-03-10 13:32 ` Prabhakar Mahadev Lad 0 siblings, 1 reply; 17+ messages in thread From: Niklas @ 2020-03-10 12:48 UTC (permalink / raw) To: Lad Prabhakar Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar Hi Lad, Thanks for your work. On 2020-03-10 11:06:04 +0000, Lad Prabhakar wrote: > This patch adds support for MEDIA_BUS_FMT_SRGGB8_1X8 format for CSI2 > input. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Small nit, you can drop rcar-vin from the subject as this patch is for the rcar-csi2 driver. With this fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-csi2.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > index 39e1639..b030ef6 100644 > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -320,6 +320,7 @@ static const struct rcar_csi2_format rcar_csi2_formats[] = { > { .code = MEDIA_BUS_FMT_YUYV8_1X16, .datatype = 0x1e, .bpp = 16 }, > { .code = MEDIA_BUS_FMT_UYVY8_2X8, .datatype = 0x1e, .bpp = 16 }, > { .code = MEDIA_BUS_FMT_YUYV10_2X10, .datatype = 0x1e, .bpp = 20 }, > + { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = 0x2a, .bpp = 8 }, > }; > > static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code) > -- > 2.7.4 > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format 2020-03-10 12:48 ` Niklas @ 2020-03-10 13:32 ` Prabhakar Mahadev Lad 0 siblings, 0 replies; 17+ messages in thread From: Prabhakar Mahadev Lad @ 2020-03-10 13:32 UTC (permalink / raw) To: Niklas Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar Hi Niklas, Thank you for the review. > -----Original Message----- > From: Niklas <niklas.soderlund@ragnatech.se> > Sent: 10 March 2020 12:49 > 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 3/3] media: rcar-vin: rcar-csi2: Add support for > MEDIA_BUS_FMT_SRGGB8_1X8 format > > Hi Lad, > > Thanks for your work. > > On 2020-03-10 11:06:04 +0000, Lad Prabhakar wrote: > > This patch adds support for MEDIA_BUS_FMT_SRGGB8_1X8 format for > CSI2 > > input. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev- > lad.rj@bp.renesas.com> > > Small nit, you can drop rcar-vin from the subject as this patch is for the rcar- > csi2 driver. With this fixed, > Sure will update it for next iteration. Cheers, --Prabhakar > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > drivers/media/platform/rcar-vin/rcar-csi2.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > > b/drivers/media/platform/rcar-vin/rcar-csi2.c > > index 39e1639..b030ef6 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > @@ -320,6 +320,7 @@ static const struct rcar_csi2_format > rcar_csi2_formats[] = { > > { .code = MEDIA_BUS_FMT_YUYV8_1X16,.datatype = 0x1e, > .bpp = 16 }, > > { .code = MEDIA_BUS_FMT_UYVY8_2X8,.datatype = 0x1e, > .bpp = 16 }, > > { .code = MEDIA_BUS_FMT_YUYV10_2X10,.datatype = 0x1e, > .bpp = 20 }, > > +{ .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = 0x2a, .bpp = > 8 }, > > }; > > > > static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int > > code) > > -- > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-04-15 8:22 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-10 11:06 [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes Lad Prabhakar 2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar 2020-03-10 12:43 ` Niklas 2020-03-10 14:54 ` Prabhakar Mahadev Lad 2020-03-10 11:06 ` [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format Lad Prabhakar 2020-03-10 12:46 ` Niklas 2020-03-10 13:42 ` Prabhakar Mahadev Lad 2020-03-10 14:06 ` Niklas [not found] ` <CA+V-a8vsYGdx6AtgqwS0LXREn4hu-EjVh2D5Dp_rHmpazBYG5A@mail.gmail.com> [not found] ` <20200319150329.GB3192108@oden.dyn.berto.se> [not found] ` <CA+V-a8u8=H-6WfaYMLWH73zo5ehP8cu9D-tdGULk=Hkvq4KuAQ@mail.gmail.com> 2020-03-30 12:07 ` Niklas 2020-03-30 13:13 ` Lad, Prabhakar 2020-04-06 17:20 ` Lad, Prabhakar 2020-04-07 9:56 ` Niklas 2020-04-14 19:39 ` Niklas 2020-04-15 8:21 ` Lad, Prabhakar 2020-03-10 11:06 ` [PATCH v2 3/3] media: rcar-vin: rcar-csi2: " Lad Prabhakar 2020-03-10 12:48 ` Niklas 2020-03-10 13:32 ` Prabhakar Mahadev Lad
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).