linux-kernel - Re: [PATCH] mipi-csis: Emit V4L2_EVENT_FRAME

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <axrx5ynlyoyjgkhj3j4r5b6xfaxwetx4c7u5ngq5wunin5ynv6@qmvav4y3xczq>
Date: Wed, 13 Mar 2024 17:49:12 +0100
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Stefan Klug <stefan.klug@...asonboard.com>
Cc: libcamera-devel@...ts.libcamera.org, linux-media@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Rui Miguel Silva <rmfrfs@...il.com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Martin Kepplinger <martink@...teo.de>, Purism Kernel Team <kernel@...i.sm>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Shawn Guo <shawnguo@...nel.org>, 
	Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>, 
	Fabio Estevam <festevam@...il.com>, NXP Linux Team <linux-imx@....com>
Subject: Re: [PATCH] mipi-csis: Emit V4L2_EVENT_FRAME_SYNC events

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@...asonboard.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@...asonboard.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
>
>