Re: [PATCH 3/3] media: subdev: Split v4l2_subdev_get_frame_desc_passthrough() into locked and unlocked

[Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>]

Hi,

On 17/03/2026 10:02, Sakari Ailus wrote:
> Moi,
> 
> On Thu, Mar 12, 2026 at 02:15:30PM +0200, Tomi Valkeinen wrote:
>> The recently added v4l2_subdev_get_frame_desc_passthrough() can be used
>> directly as an implementation for .get_frame_desc subdev op. However, in
>> some cases the drivers may want to add some customizations, while the
>> bulk of the work is still identical to what
>> v4l2_subdev_get_frame_desc_passthrough() does. Current locking scheme
>> makes this impossible to do properly.
>>
>> Split v4l2_subdev_get_frame_desc_passthrough() into two functions:
>>
>> v4l2_subdev_get_frame_desc_passthrough_locked(), which takes a locked
>> subdev state as a parameter, instead of locking and getting the active
>> state internally. Other than that, it does the same as
>> v4l2_subdev_get_frame_desc_passthrough() used to do.
>>
>> v4l2_subdev_get_frame_desc_passthrough(), which locks the active state
>> and calls v4l2_subdev_get_frame_desc_passthrough_locked().
>>
>> In other words, v4l2_subdev_get_frame_desc_passthrough() works as
>> before, but drivers can now alternatively add custom .get_frame_desc
>> code and call v4l2_subdev_get_frame_desc_passthrough().
>>
>> An example use case is with DS90UB953 serializer: in normal use the
>> serializer passes through everything, but when test-pattern-generator
>> (TPG) is used, an internal TPG source is used. After this commit, the
>> UB953 get_frame_desc() can lock the state, look at the routing table to
>> see if we're in normal or TPG mode, then either call
>> v4l2_subdev_get_frame_desc_passthrough_locked() if in normal mode, or
>> construct a TPG frame desc if in TPG mode.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-subdev.c | 46 ++++++++++++++++++++---------------
>>  include/media/v4l2-subdev.h           | 38 +++++++++++++++++++++++++----
>>  2 files changed, 59 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 2757378c628a..8b1a7f00c86b 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -2545,21 +2545,19 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_subdev_s_stream_helper);
>>  
>> -int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
>> -					   unsigned int pad,
>> -					   struct v4l2_mbus_frame_desc *fd)
>> +int v4l2_subdev_get_frame_desc_passthrough_locked(struct v4l2_subdev *sd,
>> +						  struct v4l2_subdev_state *state,
>> +						  unsigned int pad,
>> +						  struct v4l2_mbus_frame_desc *fd)
>>  {
>>  	struct media_pad *local_sink_pad;
>>  	struct v4l2_subdev_route *route;
>> -	struct v4l2_subdev_state *state;
>>  	struct device *dev = sd->dev;
>>  	int ret = 0;
>>  
>>  	if (WARN_ON(!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)))
>>  		return -EINVAL;
>>  
>> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> 
> This variant appears to be unlocked rather than locked.

The variant expects locked parameters, thus "locked". This is widely
used at least on DRM drivers.

> Could you instead use two underscores as a prefix to the same name? That's
> an established practice.
I can do that, although I don't personally like it. Double underscore
hints at an internal function, something that shouldn't be called
normally, whereas this is not internal or anything to avoid.

 Tomi