[{"id":28955,"web_url":"https://patchwork.libcamera.org/comment/28955/","msgid":"<20240314101049.GC4372@pendragon.ideasonboard.com>","date":"2024-03-14T10:10:49","subject":"Re: [PATCH v2] media: mipi-csis: Emit V4L2_EVENT_FRAME_SYNC events","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Thu, Mar 14, 2024 at 10:36:50AM +0100, Stefan Klug wrote:\n> The Samsung CSIS MIPI receiver provides a start-of-frame interrupt and\n> a framecount register. As the CSI receiver is the hardware unit that lies\n> closest to the sensor, the frame counter is the best we can get on these\n> devices. In case of the ISI available on the i.MX8 M Plus it is also the\n> only native start-of-frame signal available.\n\nYou still have either an extra line break or a missing blank line :-)\n\n> This patch exposes the sof interrupt and the framecount as\n> V4L2_EVENT_FRAME_SYNC event on the subdevice.\n> \n> It was tested on a Debix-Som-A with a 6.8-rc4 kernel.\n> \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Umang Jain \n> Signed-off-by: Stefan Klug \n> ---\n> Changes v1 -> v2:\n> - fixed formatting issues from review\n> - moved frame variable declaration to top of subscribe_event()\n> \n> Thanks all for the review!\n> \n> drivers/media/platform/nxp/imx-mipi-csis.c | 34 +++++++++++++++++++++-\n> 1 file changed, 33 insertions(+), 1 deletion(-)\n> \n> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c\n> index db8ff5f5c4d3..664be27c4224 100644\n> --- a/drivers/media/platform/nxp/imx-mipi-csis.c\n> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c\n> @@ -30,6 +30,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -742,6 +743,18 @@ static void mipi_csis_stop_stream(struct mipi_csis_device *csis)\n> \tmipi_csis_system_enable(csis, false);\n> }\n> \n> +static void mipi_csis_queue_event_sof(struct mipi_csis_device *csis)\n> +{\n> +\tu32 frame;\n> +\tstruct v4l2_event event = {\n> +\t\t.type = V4L2_EVENT_FRAME_SYNC,\n> +\t};\n\nNitpicking, we usually sort declarations by decreasing length. No need\nfor a v3.\n\n> +\n> +\tframe = mipi_csis_read(csis, MIPI_CSIS_FRAME_COUNTER_CH(0));\n> +\tevent.u.frame_sync.frame_sequence = frame;\n> +\tv4l2_event_queue(csis->sd.devnode, &event);\n> +}\n> +\n> static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)\n> {\n> \tstruct mipi_csis_device *csis = dev_id;\n> @@ -765,6 +778,10 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)\n> \t\t\t\tevent->counter++;\n> \t\t}\n> \t}\n> +\n> +\tif (status & MIPI_CSIS_INT_SRC_FRAME_START)\n> +\t\tmipi_csis_queue_event_sof(csis);\n> +\n> \tspin_unlock_irqrestore(&csis->slock, flags);\n> \n> \tmipi_csis_write(csis, MIPI_CSIS_INT_SRC, status);\n> @@ -1154,8 +1171,23 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)\n> \treturn 0;\n> }\n> \n> +static int mipi_csis_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,\n> +\t\t\t\t struct v4l2_event_subscription *sub)\n> +{\n> +\tif (sub->type != V4L2_EVENT_FRAME_SYNC)\n> +\t\treturn -EINVAL;\n> +\n> +\t/* V4L2_EVENT_FRAME_SYNC doesn't require an id, so zero should be set */\n> +\tif (sub->id != 0)\n> +\t\treturn -EINVAL;\n> +\n> +\treturn v4l2_event_subscribe(fh, sub, 0, NULL);\n> +}\n> +\n> static const struct v4l2_subdev_core_ops mipi_csis_core_ops = {\n> \t.log_status\t= mipi_csis_log_status,\n> +\t.subscribe_event = mipi_csis_subscribe_event,\n> +\t.unsubscribe_event = v4l2_event_subdev_unsubscribe,\n> };\n> \n> static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {\n> @@ -1358,7 +1390,7 @@ static int mipi_csis_subdev_init(struct mipi_csis_device *csis)\n> \tsnprintf(sd->name, sizeof(sd->name), \"csis-%s\",\n> \t\t dev_name(csis->dev));\n> \n> -\tsd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;\n> +\tsd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;\n> \tsd->ctrl_handler = NULL;\n> \n> \tsd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3D0D1BD808\n\tfor ;\n\tThu, 14 Mar 2024 10:10:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F36B62C97;\n\tThu, 14 Mar 2024 11:10:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A671062C95\n\tfor ;\n\tThu, 14 Mar 2024 11:10:51 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D8F67675;\n\tThu, 14 Mar 2024 11:10:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tGbbHdKF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710411028;\n\tbh=NHtfGxhYFnBpkY/V0r9ZDvAGH7uhabLvklqAYXuLw/E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tGbbHdKFkvq56hha5BqkMYN0VaMpIa9uUJrn6y6GIsj6aHuxwTjv1HttRVoL5iuMi\n\tIJUGiqMmd+4rbohBKegGxujb8qV9rIPtVdLGQmstIWbd+DaRqUV20mAHdFXg6W1NDd\n\toRM0iHIfSPA8dUbbYPLJKddPO1nad8ND4ENdMD/I=","Date":"Thu, 14 Mar 2024 12:10:49 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Subject":"Re: [PATCH v2] media: mipi-csis: Emit V4L2_EVENT_FRAME_SYNC events","Message-ID":"<20240314101049.GC4372@pendragon.ideasonboard.com>","References":"<20240314093652.56923-1-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240314093652.56923-1-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Martin Kepplinger , Fabio Estevam ,\n\tSascha Hauer ,\n\tPurism Kernel Team , \n\tlinux-kernel@vger.kernel.org, Rui Miguel Silva ,\n\tlibcamera-devel@lists.libcamera.org, NXP Linux Team , \n\tPengutronix Kernel Team ,\n\tJacopo Mondi ,\n\tMauro Carvalho Chehab ,\n\tShawn Guo , \n\tlinux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]