mipi-csis: Emit V4L2_EVENT_FRAME_SYNC events

[PATCH] mipi-csis: Emit V4L2_EVENT_FRAME_SYNC events

Posted by Stefan Klug 1 year, 11 months ago

The Samsung CSIS Mipi receiver provides a start-of-frame interrupt and
a framecount register. As the CSI receiver is the hardware unit
that lies closest to the sensor, the frame counter is the best we can
get on these devices.
In case of the ISI available on the i.MX8 M Plus it is also the only
native start-of-frame signal available.

This patch exposes the sof interrupt and the framecount as
V4L2_EVENT_FRAME_SYNC event on the subdevice.

It was tested on a Debix-Som-A with a 6.8-rc4 kernel.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 34 +++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index db8ff5f5c4d3..caeb1622f741 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -30,6 +30,7 @@
 
 #include <media/v4l2-common.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-mc.h>
 #include <media/v4l2-subdev.h>
@@ -742,6 +743,18 @@ static void mipi_csis_stop_stream(struct mipi_csis_device *csis)
 	mipi_csis_system_enable(csis, false);
 }
 
+static void mipi_csis_queue_event_sof(struct mipi_csis_device *csis)
+{
+	struct v4l2_event event = {
+		.type = V4L2_EVENT_FRAME_SYNC,
+	};
+
+	u32 frame = mipi_csis_read(csis, MIPI_CSIS_FRAME_COUNTER_CH(0));
+
+	event.u.frame_sync.frame_sequence = frame;
+	v4l2_event_queue(csis->sd.devnode, &event);
+}
+
 static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
 {
 	struct mipi_csis_device *csis = dev_id;
@@ -765,6 +778,10 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
 				event->counter++;
 		}
 	}
+
+	if (status & MIPI_CSIS_INT_SRC_FRAME_START)
+		mipi_csis_queue_event_sof(csis);
+
 	spin_unlock_irqrestore(&csis->slock, flags);
 
 	mipi_csis_write(csis, MIPI_CSIS_INT_SRC, status);
@@ -1154,8 +1171,23 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
 	return 0;
 }
 
+static int mipi_csis_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
+			       struct v4l2_event_subscription *sub)
+{
+	if (sub->type != V4L2_EVENT_FRAME_SYNC)
+		return -EINVAL;
+
+	/* V4L2_EVENT_FRAME_SYNC doesn't require an id, so zero should be set */
+	if (sub->id != 0)
+		return -EINVAL;
+
+	return v4l2_event_subscribe(fh, sub, 0, NULL);
+}
+
 static const struct v4l2_subdev_core_ops mipi_csis_core_ops = {
 	.log_status	= mipi_csis_log_status,
+	.subscribe_event =  mipi_csis_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 };
 
 static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
@@ -1358,7 +1390,7 @@ static int mipi_csis_subdev_init(struct mipi_csis_device *csis)
 	snprintf(sd->name, sizeof(sd->name), "csis-%s",
 		 dev_name(csis->dev));
 
-	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	sd->ctrl_handler = NULL;
 
 	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
-- 
2.40.1

Re: [PATCH] mipi-csis: Emit V4L2_EVENT_FRAME_SYNC events

Posted by Umang Jain 1 year, 11 months ago

Hi Stefan,

Thank you for the patch.

On 13/03/24 9:00 pm, Stefan Klug wrote:
> The Samsung CSIS Mipi receiver provides a start-of-frame interrupt and
> a framecount register. As the CSI receiver is the hardware unit
> that lies closest to the sensor, the frame counter is the best we can
> get on these devices.
> In case of the ISI available on the i.MX8 M Plus it is also the only
> native start-of-frame signal available.
>
> This patch exposes the sof interrupt and the framecount as
> V4L2_EVENT_FRAME_SYNC event on the subdevice.
>
> It was tested on a Debix-Som-A with a 6.8-rc4 kernel.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   drivers/media/platform/nxp/imx-mipi-csis.c | 34 +++++++++++++++++++++-
>   1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index db8ff5f5c4d3..caeb1622f741 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -30,6 +30,7 @@
>   
>   #include <media/v4l2-common.h>
>   #include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
>   #include <media/v4l2-fwnode.h>
>   #include <media/v4l2-mc.h>
>   #include <media/v4l2-subdev.h>
> @@ -742,6 +743,18 @@ static void mipi_csis_stop_stream(struct mipi_csis_device *csis)
>   	mipi_csis_system_enable(csis, false);
>   }
>   
> +static void mipi_csis_queue_event_sof(struct mipi_csis_device *csis)
> +{
> +	struct v4l2_event event = {
> +		.type = V4L2_EVENT_FRAME_SYNC,
> +	};
> +
> +	u32 frame = mipi_csis_read(csis, MIPI_CSIS_FRAME_COUNTER_CH(0));

nit: variables declaration go at the start of function and then need to 
be assigned.

rest is pointed already on the thread. Otherwise LGTM,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>



> +
> +	event.u.frame_sync.frame_sequence = frame;
> +	v4l2_event_queue(csis->sd.devnode, &event);
> +}
> +
>   static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
>   {
>   	struct mipi_csis_device *csis = dev_id;
> @@ -765,6 +778,10 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
>   				event->counter++;
>   		}
>   	}
> +
> +	if (status & MIPI_CSIS_INT_SRC_FRAME_START)
> +		mipi_csis_queue_event_sof(csis);
> +
>   	spin_unlock_irqrestore(&csis->slock, flags);
>   
>   	mipi_csis_write(csis, MIPI_CSIS_INT_SRC, status);
> @@ -1154,8 +1171,23 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
>   	return 0;
>   }
>   
> +static int mipi_csis_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> +			       struct v4l2_event_subscription *sub)
> +{
> +	if (sub->type != V4L2_EVENT_FRAME_SYNC)
> +		return -EINVAL;
> +
> +	/* V4L2_EVENT_FRAME_SYNC doesn't require an id, so zero should be set */
> +	if (sub->id != 0)
> +		return -EINVAL;
> +
> +	return v4l2_event_subscribe(fh, sub, 0, NULL);
> +}
> +
>   static const struct v4l2_subdev_core_ops mipi_csis_core_ops = {
>   	.log_status	= mipi_csis_log_status,
> +	.subscribe_event =  mipi_csis_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>   };
>   
>   static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
> @@ -1358,7 +1390,7 @@ static int mipi_csis_subdev_init(struct mipi_csis_device *csis)
>   	snprintf(sd->name, sizeof(sd->name), "csis-%s",
>   		 dev_name(csis->dev));
>   
> -	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>   	sd->ctrl_handler = NULL;
>   
>   	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;

Re: [PATCH] mipi-csis: Emit V4L2_EVENT_FRAME_SYNC events

Posted by Jacopo Mondi 1 year, 11 months ago

Hi Stefan

In Subject: missing the 'media:' prefix

On Wed, Mar 13, 2024 at 04:30:58PM +0100, Stefan Klug wrote:
> The Samsung CSIS Mipi receiver provides a start-of-frame interrupt and

s/Mipi/MIPI/

> a framecount register. As the CSI receiver is the hardware unit
> that lies closest to the sensor, the frame counter is the best we can
> get on these devices.
> In case of the ISI available on the i.MX8 M Plus it is also the only
> native start-of-frame signal available.
>
> This patch exposes the sof interrupt and the framecount as
> V4L2_EVENT_FRAME_SYNC event on the subdevice.
>
> It was tested on a Debix-Som-A with a 6.8-rc4 kernel.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 34 +++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index db8ff5f5c4d3..caeb1622f741 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -30,6 +30,7 @@
>
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-mc.h>
>  #include <media/v4l2-subdev.h>
> @@ -742,6 +743,18 @@ static void mipi_csis_stop_stream(struct mipi_csis_device *csis)
>  	mipi_csis_system_enable(csis, false);
>  }
>
> +static void mipi_csis_queue_event_sof(struct mipi_csis_device *csis)
> +{
> +	struct v4l2_event event = {
> +		.type = V4L2_EVENT_FRAME_SYNC,
> +	};
> +
> +	u32 frame = mipi_csis_read(csis, MIPI_CSIS_FRAME_COUNTER_CH(0));
> +
> +	event.u.frame_sync.frame_sequence = frame;
> +	v4l2_event_queue(csis->sd.devnode, &event);
> +}
> +
>  static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
>  {
>  	struct mipi_csis_device *csis = dev_id;
> @@ -765,6 +778,10 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
>  				event->counter++;
>  		}
>  	}
> +
> +	if (status & MIPI_CSIS_INT_SRC_FRAME_START)
> +		mipi_csis_queue_event_sof(csis);
> +
>  	spin_unlock_irqrestore(&csis->slock, flags);
>
>  	mipi_csis_write(csis, MIPI_CSIS_INT_SRC, status);
> @@ -1154,8 +1171,23 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
>  	return 0;
>  }
>
> +static int mipi_csis_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> +			       struct v4l2_event_subscription *sub)

Please align to open ( on the previous line

All minors, with the above fixed
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> +{
> +	if (sub->type != V4L2_EVENT_FRAME_SYNC)
> +		return -EINVAL;
> +
> +	/* V4L2_EVENT_FRAME_SYNC doesn't require an id, so zero should be set */
> +	if (sub->id != 0)
> +		return -EINVAL;
> +
> +	return v4l2_event_subscribe(fh, sub, 0, NULL);
> +}
> +
>  static const struct v4l2_subdev_core_ops mipi_csis_core_ops = {
>  	.log_status	= mipi_csis_log_status,
> +	.subscribe_event =  mipi_csis_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  };
>
>  static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
> @@ -1358,7 +1390,7 @@ static int mipi_csis_subdev_init(struct mipi_csis_device *csis)
>  	snprintf(sd->name, sizeof(sd->name), "csis-%s",
>  		 dev_name(csis->dev));
>
> -	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>  	sd->ctrl_handler = NULL;
>
>  	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> --
> 2.40.1
>
>

Re: [PATCH] mipi-csis: Emit V4L2_EVENT_FRAME_SYNC events

Posted by Laurent Pinchart 1 year, 11 months ago

On Wed, Mar 13, 2024 at 05:49:12PM +0100, Jacopo Mondi wrote:
> Hi Stefan
> 
> In Subject: missing the 'media:' prefix
> 
> On Wed, Mar 13, 2024 at 04:30:58PM +0100, Stefan Klug wrote:
> > The Samsung CSIS Mipi receiver provides a start-of-frame interrupt and
> 
> s/Mipi/MIPI/
> 
> > a framecount register. As the CSI receiver is the hardware unit
> > that lies closest to the sensor, the frame counter is the best we can
> > get on these devices.

Nitpicking: if you intended to start a new paragraph, add a blank line.
If not, don't add a line break between sentences.

> > In case of the ISI available on the i.MX8 M Plus it is also the only
> > native start-of-frame signal available.
> >
> > This patch exposes the sof interrupt and the framecount as
> > V4L2_EVENT_FRAME_SYNC event on the subdevice.
> >
> > It was tested on a Debix-Som-A with a 6.8-rc4 kernel.
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 34 +++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index db8ff5f5c4d3..caeb1622f741 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -30,6 +30,7 @@
> >
> >  #include <media/v4l2-common.h>
> >  #include <media/v4l2-device.h>
> > +#include <media/v4l2-event.h>
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-mc.h>
> >  #include <media/v4l2-subdev.h>
> > @@ -742,6 +743,18 @@ static void mipi_csis_stop_stream(struct mipi_csis_device *csis)
> >  	mipi_csis_system_enable(csis, false);
> >  }
> >
> > +static void mipi_csis_queue_event_sof(struct mipi_csis_device *csis)
> > +{
> > +	struct v4l2_event event = {
> > +		.type = V4L2_EVENT_FRAME_SYNC,
> > +	};
> > +

No need for a blank line.

> > +	u32 frame = mipi_csis_read(csis, MIPI_CSIS_FRAME_COUNTER_CH(0));
> > +
> > +	event.u.frame_sync.frame_sequence = frame;
> > +	v4l2_event_queue(csis->sd.devnode, &event);
> > +}
> > +
> >  static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> >  {
> >  	struct mipi_csis_device *csis = dev_id;
> > @@ -765,6 +778,10 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> >  				event->counter++;
> >  		}
> >  	}
> > +
> > +	if (status & MIPI_CSIS_INT_SRC_FRAME_START)
> > +		mipi_csis_queue_event_sof(csis);
> > +
> >  	spin_unlock_irqrestore(&csis->slock, flags);
> >
> >  	mipi_csis_write(csis, MIPI_CSIS_INT_SRC, status);
> > @@ -1154,8 +1171,23 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
> >  	return 0;
> >  }
> >
> > +static int mipi_csis_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> > +			       struct v4l2_event_subscription *sub)
> 
> Please align to open ( on the previous line
> 
> All minors, with the above fixed
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If you don't want to send a v2, I can address the minor comments when
applying this to my tree. Please let me know what you prefer.

> > +{
> > +	if (sub->type != V4L2_EVENT_FRAME_SYNC)
> > +		return -EINVAL;
> > +
> > +	/* V4L2_EVENT_FRAME_SYNC doesn't require an id, so zero should be set */
> > +	if (sub->id != 0)
> > +		return -EINVAL;
> > +
> > +	return v4l2_event_subscribe(fh, sub, 0, NULL);
> > +}
> > +
> >  static const struct v4l2_subdev_core_ops mipi_csis_core_ops = {
> >  	.log_status	= mipi_csis_log_status,
> > +	.subscribe_event =  mipi_csis_subscribe_event,
> > +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> >  };
> >
> >  static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
> > @@ -1358,7 +1390,7 @@ static int mipi_csis_subdev_init(struct mipi_csis_device *csis)
> >  	snprintf(sd->name, sizeof(sd->name), "csis-%s",
> >  		 dev_name(csis->dev));
> >
> > -	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> >  	sd->ctrl_handler = NULL;
> >
> >  	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;

-- 
Regards,

Laurent Pinchart