diff mbox series

[v2,10/18] media: cadence: csi2rx: Populate subdev devnode

Message ID 20210526152308.16525-11-p.yadav@ti.com
State Changes Requested
Headers show
Series CSI2RX support on J721E | expand

Commit Message

Pratyush Yadav May 26, 2021, 3:23 p.m. UTC
The devnode can be used by media-ctl and other userspace tools to
perform configurations on the subdev. Without it, media-ctl returns
ENOENT when setting format on the sensor subdev.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---

(no changes since v1)

 drivers/media/platform/cadence/cdns-csi2rx.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Tomi Valkeinen May 28, 2021, 7:16 a.m. UTC | #1
On 26/05/2021 18:23, Pratyush Yadav wrote:
> The devnode can be used by media-ctl and other userspace tools to
> perform configurations on the subdev. Without it, media-ctl returns
> ENOENT when setting format on the sensor subdev.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
> 
> (no changes since v1)
> 
>   drivers/media/platform/cadence/cdns-csi2rx.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 1df21f462f3c..49bed63d5faa 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -613,6 +613,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>   	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>   	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++)
>   		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>   
>   	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
>   				     csi2rx->pads);
> 

I don't understand this one. There's nothing to configure in cdns-csi2rx 
from userspace, as far as I can see, so why is the dev node needed? And 
why would the lack of csi2rx dev node cause sensor subdev config to fail?

  Tomi
Pratyush Yadav May 28, 2021, 7:24 a.m. UTC | #2
On 28/05/21 10:16AM, Tomi Valkeinen wrote:
> On 26/05/2021 18:23, Pratyush Yadav wrote:
> > The devnode can be used by media-ctl and other userspace tools to
> > perform configurations on the subdev. Without it, media-ctl returns
> > ENOENT when setting format on the sensor subdev.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> > 
> > (no changes since v1)
> > 
> >   drivers/media/platform/cadence/cdns-csi2rx.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 1df21f462f3c..49bed63d5faa 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -613,6 +613,7 @@ static int csi2rx_probe(struct platform_device *pdev)
> >   	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> >   	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++)
> >   		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > +	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >   	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
> >   				     csi2rx->pads);
> > 
> 
> I don't understand this one. There's nothing to configure in cdns-csi2rx
> from userspace, as far as I can see, so why is the dev node needed? And why
> would the lack of csi2rx dev node cause sensor subdev config to fail?

Sensor config does not fail. But when I run media-ctl to set the format 
on /dev/media0, I get an error message that comes because the devnode 
for the bridge does not exist. I was not 100% sure about this patch but 
I figured if media-ctl expects it then it should be exposed.

I don't mind dropping this patch. Just want to make sure what the right 
thing to do here is. Should every element of the pipeline have a devnode 
or not?
Tomi Valkeinen May 28, 2021, 7:35 a.m. UTC | #3
On 28/05/2021 10:24, Pratyush Yadav wrote:
> On 28/05/21 10:16AM, Tomi Valkeinen wrote:
>> On 26/05/2021 18:23, Pratyush Yadav wrote:
>>> The devnode can be used by media-ctl and other userspace tools to
>>> perform configurations on the subdev. Without it, media-ctl returns
>>> ENOENT when setting format on the sensor subdev.
>>>
>>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>>    drivers/media/platform/cadence/cdns-csi2rx.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
>>> index 1df21f462f3c..49bed63d5faa 100644
>>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
>>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
>>> @@ -613,6 +613,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>>>    	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>>    	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++)
>>>    		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
>>> +	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>>    	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
>>>    				     csi2rx->pads);
>>>
>>
>> I don't understand this one. There's nothing to configure in cdns-csi2rx
>> from userspace, as far as I can see, so why is the dev node needed? And why
>> would the lack of csi2rx dev node cause sensor subdev config to fail?
> 
> Sensor config does not fail. But when I run media-ctl to set the format
> on /dev/media0, I get an error message that comes because the devnode
> for the bridge does not exist. I was not 100% sure about this patch but
> I figured if media-ctl expects it then it should be exposed.
> 
> I don't mind dropping this patch. Just want to make sure what the right
> thing to do here is. Should every element of the pipeline have a devnode
> or not?

Tbh, I don't know. But I don't see why they should have. Also, my test 
works fine if I remove the devnode here.

What media-ctl parameters did you use which fails?

  Tomi
Pratyush Yadav June 3, 2021, 12:42 p.m. UTC | #4
On 28/05/21 10:35AM, Tomi Valkeinen wrote:
> On 28/05/2021 10:24, Pratyush Yadav wrote:
> > On 28/05/21 10:16AM, Tomi Valkeinen wrote:
> > > On 26/05/2021 18:23, Pratyush Yadav wrote:
> > > > The devnode can be used by media-ctl and other userspace tools to
> > > > perform configurations on the subdev. Without it, media-ctl returns
> > > > ENOENT when setting format on the sensor subdev.
> > > > 
> > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > > > ---
> > > > 
> > > > (no changes since v1)
> > > > 
> > > >    drivers/media/platform/cadence/cdns-csi2rx.c | 1 +
> > > >    1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > > > index 1df21f462f3c..49bed63d5faa 100644
> > > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > > > @@ -613,6 +613,7 @@ static int csi2rx_probe(struct platform_device *pdev)
> > > >    	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > > >    	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++)
> > > >    		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > > > +	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > >    	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
> > > >    				     csi2rx->pads);
> > > > 
> > > 
> > > I don't understand this one. There's nothing to configure in cdns-csi2rx
> > > from userspace, as far as I can see, so why is the dev node needed? And why
> > > would the lack of csi2rx dev node cause sensor subdev config to fail?
> > 
> > Sensor config does not fail. But when I run media-ctl to set the format
> > on /dev/media0, I get an error message that comes because the devnode
> > for the bridge does not exist. I was not 100% sure about this patch but
> > I figured if media-ctl expects it then it should be exposed.
> > 
> > I don't mind dropping this patch. Just want to make sure what the right
> > thing to do here is. Should every element of the pipeline have a devnode
> > or not?
> 
> Tbh, I don't know. But I don't see why they should have. Also, my test works
> fine if I remove the devnode here.
> 
> What media-ctl parameters did you use which fails?

Media controller topology:

  root@j7-evm:~# media-ctl -p
  Media controller API version 5.13.0
  
  Media device information
  ------------------------
  driver         j721e-csi2rx
  model          TI-CSI2RX
  serial
  bus info       platform:4500000.ticsi2rx
  hw revision    0x1
  driver version 5.13.0
  
  Device topology
  - entity 1: cdns_csi2rx.4504000.csi-bridge (5 pads, 2 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev0
         pad0: Sink
                 <- "ov5640 9-003c":0 [ENABLED,IMMUTABLE]
         pad1: Source
                 -> "j721e-csi2rx":0 [ENABLED,IMMUTABLE]
         pad2: Source
         pad3: Source
         pad4: Source
  
  - entity 7: ov5640 9-003c (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev1
         pad0: Source
                 [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range]
                 -> "cdns_csi2rx.4504000.csi-bridge":0 [ENABLED,IMMUTABLE]
  
  - entity 11: j721e-csi2rx (1 pad, 1 link)
              type Node subtype V4L flags 0
              device node name /dev/video0
         pad0: Sink
                 <- "cdns_csi2rx.4504000.csi-bridge":1 [ENABLED,IMMUTABLE]


Setting resolution on the sensor (with cdns-csi2rx devnode present):

  root@j7-evm:~# media-ctl --set-v4l2 '"ov5640 9-003c":0 [fmt:UYVY8_2X8/1920x1080@1/30]'

The above command works. Setting resolution on the sensor (without the 
devnode present):

  root@j7-evm:~# media-ctl --set-v4l2 '"ov5640 9-003c":0 [fmt:UYVY8_2X8/1920x1080@1/30]'
  Unable to setup formats: No such file or directory (2)

This sets the format correctly on the sensor but fails when it tried to 
set it on the bridge.
diff mbox series

Patch

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 1df21f462f3c..49bed63d5faa 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -613,6 +613,7 @@  static int csi2rx_probe(struct platform_device *pdev)
 	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
 	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++)
 		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
+	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
 				     csi2rx->pads);