From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 461F91C5D5E for ; Thu, 16 Apr 2026 16:16:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776356220; cv=none; b=GKT9hXsaL2sD/fSwFf48w03zmVDesCBr67j6nU6SYoI1XQ89SeSQrrnvut+/Hj04XMWjaX0TRf1PRo3W5/QFFAtiGayXHGAjiFQdMGyTQDxTpTv+kM/EqzgNav24J7Dq3XogkZyrXwAqEZOQ9ASQG9VjfSjWlCP/IJsTNrk6IVs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776356220; c=relaxed/simple; bh=uYt/iK/S0wPX2Nui5TwDN5hgNZQPoBWV2cF1IxjusMQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=mcBbRvJ6KPhJoxtqbnAFl7PZkueRJu0/V+6PHHwbHj7lc/HTfY4x3Bu7oBhffKdDm6Y93UBef0FLD99WOwTR6ozyDedh9/0pebvi4CxjYn+jHqBMDlTzz95X5meKVBS/X5Kr8qS7x8Op1gYll/N1IoX6WcQJgp/bPj++M41oKT4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=HrBNaTsb; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="HrBNaTsb" Received: from killaraus.ideasonboard.com (2001-14ba-703d-e500--2a1.rev.dnainternet.fi [IPv6:2001:14ba:703d:e500::2a1]) by perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 8F723132; Thu, 16 Apr 2026 18:15:21 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1776356121; bh=uYt/iK/S0wPX2Nui5TwDN5hgNZQPoBWV2cF1IxjusMQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HrBNaTsbKl61o6zC1wuqiiOLasofXobI5wVvhSR+GRp5HfhBeFCNZ1bSEoGRB5S42 opq9I5IgKW9bA51Ng+hG4c/JNZYz0Fge0+oFkKAJiFqOszEZ3VCBpuo2tYO/1J9hq4 s9Pawhi7f2R2vSjrs17PJFvO/XT5iYn1hjFr6Bxw= Date: Thu, 16 Apr 2026 19:16:54 +0300 From: Laurent Pinchart To: Sakari Ailus Cc: Jacopo Mondi , linux-media@vger.kernel.org, hans@jjverkuil.nl, Prabhakar , Kate Hsuan , Dave Stevenson , Tommaso Merciai , Benjamin Mugnier , Sylvain Petinot , Christophe JAILLET , Julien Massot , Naushir Patuck , "Yan, Dongcheng" , "Cao, Bingbu" , "Qiu, Tian Shu" , Stefan Klug , Mirela Rabulea , =?utf-8?B?QW5kcsOp?= Apitzsch , Heimir Thor Sverrisson , Kieran Bingham , Mehdi Djait , Ricardo Ribalda Delgado , Hans de Goede , Tomi Valkeinen , David Plowman , "Yu, Ong Hock" , "Ng, Khai Wen" , Jai Luthra , Rishikesh Donadkar Subject: Re: [PATCH v4 24/29] media: v4l2-subdev: Introduce v4l2_subdev_get_frame_desc() Message-ID: <20260416161654.GC1823068@killaraus.ideasonboard.com> References: <20260408153939.969381-1-sakari.ailus@linux.intel.com> <20260408153939.969381-25-sakari.ailus@linux.intel.com> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: On Mon, Apr 13, 2026 at 11:07:07AM +0300, Sakari Ailus wrote: > On Fri, Apr 10, 2026 at 12:53:42PM +0200, Jacopo Mondi wrote: > > On Wed, Apr 08, 2026 at 06:39:33PM +0300, Sakari Ailus wrote: > > > Introduce v4l2_subdev_get_frame_desc() in order to facilitate implementing > > > drivers that need frame descriptors. If the remote sub-device does not > > > support frame descriptors, v4l2_subdev_get_frame_desc() creates one (with > > > a single entry) opportunistically, thus avoiding the need to add frame > > > descriptor support to sensor drivers the device for which only generates a s/the device for which/whose device/ I think all sensor drivers should implement .get_frame_desc(), possibly with a helper. > > > single stream, or managing the situation on the caller side. That part I agree with, we currently need to manage the situation on the caller side because not all subdevs implement the operation. That will still be the case for a while, so this helper is useful. Can you also replace manual calls to .get_frame_desc() with the helper ? Some of the existing callers implement fallbacks, I fonud them at least in drivers/media/platform/broadcom/bcm2835-unicam.c drivers/media/platform/raspberrypi/rp1-cfe/cfe.c drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c drivers/media/platform/ti/cal/cal.c (directly in the function that calls .get_frame_desc(), or in its caller) Replacing those will ensure the helper works as expected. > > > Signed-off-by: Sakari Ailus > > > --- > > > drivers/media/v4l2-core/v4l2-subdev.c | 96 +++++++++++++++++++++++++++ > > > include/media/v4l2-subdev.h | 20 ++++++ > > > 2 files changed, 116 insertions(+) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index 647587c0499a..40b28e070726 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -20,6 +20,7 @@ > > > #include > > > #include > > > > > > +#include > > > #include > > > #include > > > #include > > > @@ -2758,3 +2759,98 @@ void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd) > > > #endif > > > } > > > EXPORT_SYMBOL_GPL(v4l2_subdev_put_privacy_led); > > > + > > > +static int get_mipi_dt_for_mbus(u32 code) v4l2_subdev_get_mipi_dt_for_mbus() Or move the function to v4l2-common.c, name it mipi_csi2_dt_for_mbus() and declare it in include/media/mipi-csi2.h. > > > +{ > > > + switch (code) { > > > + case MEDIA_BUS_FMT_BGR888_1X24: > > > + return MIPI_CSI2_DT_RGB888; > > > + case MEDIA_BUS_FMT_Y8_1X8: > > > + case MEDIA_BUS_FMT_SBGGR8_1X8: > > > + case MEDIA_BUS_FMT_SGBRG8_1X8: > > > + case MEDIA_BUS_FMT_SGRBG8_1X8: > > > + case MEDIA_BUS_FMT_SRGGB8_1X8: > > > + return MIPI_CSI2_DT_RAW8; > > > + case MEDIA_BUS_FMT_Y10_1X10: > > > + case MEDIA_BUS_FMT_SBGGR10_1X10: > > > + case MEDIA_BUS_FMT_SGBRG10_1X10: > > > + case MEDIA_BUS_FMT_SGRBG10_1X10: > > > + case MEDIA_BUS_FMT_SRGGB10_1X10: > > > + return MIPI_CSI2_DT_RAW10; > > > + case MEDIA_BUS_FMT_Y12_1X12: > > > + case MEDIA_BUS_FMT_SBGGR12_1X12: > > > + case MEDIA_BUS_FMT_SGBRG12_1X12: > > > + case MEDIA_BUS_FMT_SGRBG12_1X12: > > > + case MEDIA_BUS_FMT_SRGGB12_1X12: > > > + return MIPI_CSI2_DT_RAW12; > > > + case MEDIA_BUS_FMT_Y14_1X14: > > > + case MEDIA_BUS_FMT_SBGGR14_1X14: > > > + case MEDIA_BUS_FMT_SGBRG14_1X14: > > > + case MEDIA_BUS_FMT_SGRBG14_1X14: > > > + case MEDIA_BUS_FMT_SRGGB14_1X14: > > > + return MIPI_CSI2_DT_RAW14; > > > + case MEDIA_BUS_FMT_Y16_1X16: > > > + case MEDIA_BUS_FMT_SBGGR16_1X16: > > > + case MEDIA_BUS_FMT_SGBRG16_1X16: > > > + case MEDIA_BUS_FMT_SGRBG16_1X16: > > > + case MEDIA_BUS_FMT_SRGGB16_1X16: > > > + return MIPI_CSI2_DT_RAW16; > > > + case MEDIA_BUS_FMT_SBGGR20_1X20: > > > + case MEDIA_BUS_FMT_SGBRG20_1X20: > > > + case MEDIA_BUS_FMT_SGRBG20_1X20: > > > + case MEDIA_BUS_FMT_SRGGB20_1X20: > > > + return MIPI_CSI2_DT_RAW20; > > > + default: > > > + return -EINVAL; > > > + } > > > +} > > > + > > > +int v4l2_subdev_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > > + struct v4l2_mbus_frame_desc *desc) > > > +{ > > > + struct v4l2_subdev_format subdev_fmt = { > > > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > > + .pad = pad, > > > + }; > > > + int ret; > > > + > > > + if (v4l2_subdev_has_op(sd, pad, get_frame_desc)) { > > > + unsigned int type = desc->type; > > > + > > > + ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, desc); > > > + > > > + if (desc->type != type) > > > + return -EINVAL; I'd add a dev_err() here. There are .get_frame_desc() callers that check if the returned type matches what they expect and log an error otherwise. When using this helper the check can't be performed in the callera any more, leading to possibly hard to debug issues if no message is printed. > > > + > > > + return ret; > > > + } > > > + > > > + if (desc->type != V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL && > > > + desc->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > > > + return -EINVAL; > > > + > > > + ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, NULL, > > > + &subdev_fmt); > > > + if (ret) > > > + return ret; > > > + > > > + struct v4l2_mbus_frame_desc_entry entry = { > > > + .pixelcode = subdev_fmt.format.code, > > > + }; > > > + > > > + if (desc->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > > > + int dt; > > > + > > > + dt = get_mipi_dt_for_mbus(subdev_fmt.format.code); > > > + if (dt < 0) > > > + return dt; > > > + > > > + entry.bus.csi2.dt = dt; > > > + } > > > + > > > + desc->entry[0] = entry; > > > + desc->num_entries = 1; > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(v4l2_subdev_get_frame_desc); > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > > index 4588992b4417..93b672edd08e 100644 > > > --- a/include/media/v4l2-subdev.h > > > +++ b/include/media/v4l2-subdev.h > > > @@ -2058,4 +2058,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd, > > > */ > > > bool v4l2_subdev_is_streaming(struct v4l2_subdev *sd); > > > > > > +/** > > > + * v4l2_subdev_get_frame_desc() - Get a pad's frame descriptor > > > + * @sd: The sub-device > > > + * @pad: The number of the pad in @sd from which to obtain the frame descriptor > > > + * @desc: A pointer to a frame descriptor, with its type field set > > > + * > > > + * Obtain a frame descriptor from a sub-device. If the sub-device supports the > > > + * get_frame_desc pad operation, its result is returned, just like calling it > > > + * directly using v4l2_subdev_call(). If the sub-device driver does not support > > > + * it, then one containing a single entry is created using the information from s/one /a frame descriptor/ > > > + * the sub-device active state, which this function locks for the duration of > > > + * the call to obtain it. > > > > This doesn't seem to apply anymore... > > I'll rephrase it. > > > > + * > > > + * The caller is required to set @desc->type to the expected bus type. > > > > Is it worth mentioning that only V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL > > and V4L2_MBUS_FRAME_DESC_TYPE_CSI2 are supported ? > > I'll add that. > > > > + * > > > + * Return: %0 on success or negative error code on failure. > > > + */ > > > +int v4l2_subdev_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > > + struct v4l2_mbus_frame_desc *desc); > > > > With the documentation addressed > > Reviewed-by: Jacopo Mondi -- Regards, Laurent Pinchart