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

[Sakari Ailus]

Moi,

On Tue, Mar 17, 2026 at 10:37:12AM +0200, Tomi Valkeinen wrote:
> 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@xxxxxxxxxxxxxxxx>
> >> ---
> >>  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

>From the name I'd expect otherwise. But...

> used at least on DRM drivers.

it maybe used in DRM for that but this isn't DRM. :-)

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

Or know what you're doing. You could add a lockdep annotation while at it.

-- 
Sakari Ailus