| Message ID | 20250212131244.1397722-1-y-abhilashchandra@ti.com |
|---|---|
| Headers | show |
| Series | Enable support for error detection in CSI2RX | expand |
On Wed, Feb 12, 2025 at 06:42:41PM +0530, Yemike Abhilash Chandra wrote: > This patch series enables the csi2rx_err_irq interrupt to record any errors > that occur during streaming. It also adds support for the VIDIOC_LOG_STATUS > ioctl, which outputs the current device status to the kernel log. > > The IRQ handler records any errors encountered during streaming. > Additionally, VIDIOC_LOG_STATUS can be invoked from user space to retrieve > the latest status. > > Logs with interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/58ced4df0158efad6f453b4d96463723 > Logs without interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/d807230b2a624b7a38effed89efdd148 What is actually RFC about this patchset, rather than just being v1?
On 12/02/2025 14:12, Yemike Abhilash Chandra wrote: > The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem. > Enabling these interrupts will provide additional information about a CSI > packet or an individual frame. So, add support for optional interrupts > and interrupt-names properties. > > [0]: http://www.ti.com/lit/pdf/spruil1 Why is this RFC? > > Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> > --- > .../devicetree/bindings/media/cdns,csi2rx.yaml | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > index 2008a47c0580..a3acf4f861c2 100644 > --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > @@ -24,6 +24,17 @@ properties: > reg: > maxItems: 1 > > + interrupts: > + minItems: 1 > + maxItems: 3 I understand interrupts might be unused by driver, but are you sure they are optionally connected one-by-one? IOW, why is this flexible? Best regards, Krzysztof
On 13/02/25 00:46, Conor Dooley wrote: > On Wed, Feb 12, 2025 at 06:42:41PM +0530, Yemike Abhilash Chandra wrote: >> This patch series enables the csi2rx_err_irq interrupt to record any errors >> that occur during streaming. It also adds support for the VIDIOC_LOG_STATUS >> ioctl, which outputs the current device status to the kernel log. >> >> The IRQ handler records any errors encountered during streaming. >> Additionally, VIDIOC_LOG_STATUS can be invoked from user space to retrieve >> the latest status. >> >> Logs with interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/58ced4df0158efad6f453b4d96463723 >> Logs without interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/d807230b2a624b7a38effed89efdd148 > > What is actually RFC about this patchset, rather than just being v1? I sent this as an RFC to gather input from different vendors using the cdns,csi2rx driver and its device tree bindings. so I just wanted to get their feedback as well. If there are no concerns from any of the them, I will proceed with sending this as v1. Thanks and Regards, Yemike Abhilash Chandra
On 13/02/25 00:58, Krzysztof Kozlowski wrote: > On 12/02/2025 14:12, Yemike Abhilash Chandra wrote: >> The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem. >> Enabling these interrupts will provide additional information about a CSI >> packet or an individual frame. So, add support for optional interrupts >> and interrupt-names properties. >> >> [0]: http://www.ti.com/lit/pdf/spruil1 > > > Why is this RFC? > I sent this as an RFC to gather input from different vendors using the cdns,csi2rx driver and its device tree bindings. so I just wanted to get their feedback as well. If there are no concerns from any of the them, I will proceed with sending this as v1. >> >> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> >> --- >> .../devicetree/bindings/media/cdns,csi2rx.yaml | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml >> index 2008a47c0580..a3acf4f861c2 100644 >> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml >> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml >> @@ -24,6 +24,17 @@ properties: >> reg: >> maxItems: 1 >> >> + interrupts: >> + minItems: 1 >> + maxItems: 3 > > I understand interrupts might be unused by driver, but are you sure they > are optionally connected one-by-one? IOW, why is this flexible? > I understand that this flexibility is not needed, and I will correct that while sending v1. Thanks and Regards, Yemike Abhilash Chandra > > Best regards, > Krzysztof
On 13/02/2025 08:16, Yemike Abhilash Chandra wrote: > > On 13/02/25 00:58, Krzysztof Kozlowski wrote: >> On 12/02/2025 14:12, Yemike Abhilash Chandra wrote: >>> The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem. >>> Enabling these interrupts will provide additional information about a CSI >>> packet or an individual frame. So, add support for optional interrupts >>> and interrupt-names properties. >>> >>> [0]: http://www.ti.com/lit/pdf/spruil1 >> >> >> Why is this RFC? >> > > I sent this as an RFC to gather input from different vendors using the > cdns,csi2rx driver > and its device tree bindings. so I just wanted to get their feedback as > well. Then document it clearly that you do not expect review. > If there are no concerns from any of the them, I will proceed with > sending this as v1. No, this was v1. Best regards, Krzysztof
Hi Abhilash, On Wed, Feb 12, 2025 at 06:42:44PM +0530, Yemike Abhilash Chandra wrote: > The VIDIOC_LOG_STATUS ioctl outputs the current status of a device to the > kernel log. When this ioctl is called on a video device, the current > implementation queries the log status for all connected subdevices in the > media pipeline. > What is the benefit of doing this for a video node? The user can directly check the status on the cdns-csi2rx subdev for CSI errors. As far as I understand, the video node corresponds to a particular stream, but the cdns-csi2rx source pad is shared for all video nodes, so it will report the total errors seen for all video nodes in multi-camera scenarios. This approach will also give you v4l2 control handler status from a few sensors (like OV5640) that implement the ioctl using v4l2_ctrl_subdev_log_status(), which is probably just noise for the case where a user wants to check for stream errors. > Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> > --- > drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > index 6412a00be8ea..946704458fee 100644 > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > @@ -377,6 +377,15 @@ static int ti_csi2rx_enum_framesizes(struct file *file, void *fh, > return 0; > } > > +static int ti_csi2rx_log_status(struct file *file, void *fh) > +{ > + struct ti_csi2rx_dev *csi = video_drvdata(file); > + > + v4l2_device_call_all(&csi->v4l2_dev, 0, core, log_status); > + > + return 0; > +} > + > static const struct v4l2_ioctl_ops csi_ioctl_ops = { > .vidioc_querycap = ti_csi2rx_querycap, > .vidioc_enum_fmt_vid_cap = ti_csi2rx_enum_fmt_vid_cap, > @@ -393,6 +402,7 @@ static const struct v4l2_ioctl_ops csi_ioctl_ops = { > .vidioc_expbuf = vb2_ioctl_expbuf, > .vidioc_streamon = vb2_ioctl_streamon, > .vidioc_streamoff = vb2_ioctl_streamoff, > + .vidioc_log_status = ti_csi2rx_log_status, > }; > > static const struct v4l2_file_operations csi_fops = { > -- > 2.34.1 > Thanks, Jai
This patch series enables the csi2rx_err_irq interrupt to record any errors that occur during streaming. It also adds support for the VIDIOC_LOG_STATUS ioctl, which outputs the current device status to the kernel log. The IRQ handler records any errors encountered during streaming. Additionally, VIDIOC_LOG_STATUS can be invoked from user space to retrieve the latest status. Logs with interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/58ced4df0158efad6f453b4d96463723 Logs without interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/d807230b2a624b7a38effed89efdd148 Yemike Abhilash Chandra (3): dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS media: ti: j721e-csi2rx: Add support for VIDIOC_LOG_STATUS .../bindings/media/cdns,csi2rx.yaml | 11 ++ drivers/media/platform/cadence/cdns-csi2rx.c | 104 +++++++++++++++++- .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 10 ++ 3 files changed, 124 insertions(+), 1 deletion(-)