Re: [PATCH v8 04/36] media: subdev: pass also the active state to subdevs from ioctls

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

Hi,

On 15/09/2021 13:17, Jacopo Mondi wrote:
> Hi Tomi
> 
> On Mon, Aug 30, 2021 at 02:00:44PM +0300, Tomi Valkeinen wrote:
>> At the moment when a subdev op is called, the TRY subdev state
>> (subdev_fh->state) is passed as a parameter even for ACTIVE case, or
>> alternatively a NULL can be passed for ACTIVE case. This used to make
>> sense, as the ACTIVE state was handled internally by the subdev drivers.
>>
>> We now have a state for ACTIVE case in a standard place, and can pass
>> that alto to the drivers. This patch changes the subdev ioctls to either
>> pass the TRY or ACTIVE state to the subdev.
>>
>> Unfortunately many drivers call ops from other subdevs, and implicitly
>> pass NULL as the state, so this is just a partial solution. A coccinelle
>> spatch could perhaps be created which fixes the drivers' subdev calls.
>>
>> For all current upstream drivers this doesn't matter, as they do not
>> expect to get a valid state for ACTIVE case. But future drivers which
>> support multiplexed streaming and routing will depend on getting a state
>> for both active and try cases, and the simplest way to avoid this
>> problem is to introduce a helper function, used by the new drivers,
>> which makes sure the driver has either the TRY or ACTIVE state. This
>> helper will be introduced in a follow-up patch.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 73 +++++++++++++++++++++++----
>>   1 file changed, 63 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 04ad319fb150..b3637cddca58 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -353,6 +353,53 @@ const struct v4l2_subdev_ops v4l2_subdev_call_wrappers = {
>>   EXPORT_SYMBOL(v4l2_subdev_call_wrappers);
>>
>>   #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>> +
>> +static struct v4l2_subdev_state *
>> +subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
>> +		       unsigned int cmd, void *arg)
>> +{
>> +	u32 which;
>> +
>> +	switch (cmd) {
>> +	default:
>> +		return NULL;
>> +
>> +	case VIDIOC_SUBDEV_G_FMT:
>> +	case VIDIOC_SUBDEV_S_FMT: {
>> +		which = ((struct v4l2_subdev_format *)arg)->which;
>> +		break;
>> +	}
>> +	case VIDIOC_SUBDEV_G_CROP:
>> +	case VIDIOC_SUBDEV_S_CROP: {
>> +		which = ((struct v4l2_subdev_crop *)arg)->which;
>> +		break;
>> +	}
>> +	case VIDIOC_SUBDEV_ENUM_MBUS_CODE: {
>> +		which = ((struct v4l2_subdev_mbus_code_enum *)arg)->which;
>> +		break;
>> +	}
>> +	case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: {
>> +		which = ((struct v4l2_subdev_frame_size_enum *)arg)->which;
>> +		break;
>> +	}
>> +
>> +	case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: {
>> +		which = ((struct v4l2_subdev_frame_interval_enum *)arg)->which;
>> +		break;
>> +	}
>> +
>> +	case VIDIOC_SUBDEV_G_SELECTION:
>> +	case VIDIOC_SUBDEV_S_SELECTION: {
>> +		which = ((struct v4l2_subdev_selection *)arg)->which;
>> +		break;
>> +	}
>> +	}
>> +
>> +	return which == V4L2_SUBDEV_FORMAT_TRY ?
>> +			     subdev_fh->state :
>> +			     v4l2_subdev_get_active_state(sd);
> 
> Why this additional indirection layer ?
> 
> v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
> {
>      return sd->state;
> }

I wanted to hide all direct accesses to the state to make it easier to 
figure out how and where the state is accessed.

> I understand you want to have the core to fish the 'right' state for
> the drivers, but this then requires to protect against bridge drivers
> calling an op through v4l2_subdev_call() with a NULL state by using
> one more indirection like
> 
> 	state = v4l2_subdev_validate_state(sd, state);
> 
>          static inline struct v4l2_subdev_state *
>          v4l2_subdev_validate_state(struct v4l2_subdev *sd,
>                                     struct v4l2_subdev_state *state)
>          {
>                  return state ? state : sd->state;
>          }
> 
> Which I very much don't like as it implicitly changes what state the
> driver receives to work-around a design flaw (the fact that even if
> the core tries to, there's no gurantee state is valid).

I don't like it either. My idea was that in the future the subdevs would 
always get the correct state. In other words, all the subdev drivers 
calling ops in other subdevs would be changed to pass the state 
correctly. Thus the v4l2_subdev_validate_state() is a helper for the 
transition period, which can easily be dropped when the drivers work 
correctly.

> If feel like it would be much simpler if:
> 
> 1) The core passes in a state which always come from the fh (the
>     try_state) when it do_ioctl()
> 
> 2) Drivers use their 'active' states embedded in the subdev or the
>     'try' state passed in as parameter and decide
>     which one to use based on the context. It's a pattern we have
>     everywere already when using the per-fh try formats
> 
> 	switch (which) {
> 	case V4L2_SUBDEV_FORMAT_TRY:
> 		return v4l2_subdev_get_try_format(&sd, sd_state, pad);
> 	case V4L2_SUBDEV_FORMAT_ACTIVE:
> 		return &sd->fmt;
> 	default:
> 		return NULL;
> 	}

This is possible, of course. We could do this if we decide we don't want 
the subdev drivers to pass the state properly in the future.

However, if, in my series, I currently call this in a subdev driver:

state = v4l2_subdev_validate_state(sd, state);

With the change you suggest I'd just do (possibly with a helper):

state = which == V4L2_SUBDEV_FORMAT_TRY ? state : sd->state;

Is it any better?

> I liked the idea to have the core pass in a state without the driver
> having to care where it comes from, but it requires too many
> indirections and implicities like the above shown
> v4l2_subdev_validate_state().
> 
> One middle-ground could be to have the core pass in the 'correct' state as it
> does in your series, and default it to sd->state if a bridge driver
> calls an op through v4l2_subdev_call() with a NULL state, as the
> format is implicitly ACTIVE in that case.

If you mean changing all the bridge drivers so that they would give the 
state properly, yes, that was my plan (I think I mentioned it in a 
commit desc, perhaps). It's not a trivial change, though, as 
v4l2_subdev_call() cannot handle this at the moment.

I believe it should be doable with coccinelle. Maybe add a new macro, 
v4l2_subdev_call_state() or such, which gives the active state in the 
second parameter (looks like all the ops have the state as the second 
param). Then use coccinelle to find all the v4l2_subdev_call uses which 
call ops that get a state, verify that the current caller uses NULL as 
the state, and change v4l2_subdev_call to v4l2_subdev_call_state.

> This ofc requires the state to be embedded (ie it's always there) and
> that state-aware drivers to have properly initialized it, but that's a
> given.

Why does the state need to be embedded? If the subdev driver is not 
state-aware, it does not expect to get a state except for the TRY case. 
Passing NULL for those drivers should be fine.

> Nonetheless, this considerations do not defeat the purpose of having a
> 'state', as currently we have
> 
> struct v4l2_subdev_state {
>          struct v4l2_subdev_krouting; /* Use for TRY and ACTIVE */
>          struct v4l2_stream_configs; /* Use for ACTIVE */

stream_configs is used for TRY also.

>          struct v4l2_pad_configs; /* Used for TRY */

Probably no point in this, but this _could_ also be used for ACTIVE. We 
could have state aware drivers that don't use routing or streams, and 
use just a plain old pad_configs array. This would allow moving the 
ACTIVE pad_configs from the driver to the core.

But, as you suggest, probably a better direction is to try to get rid of 
pad_configs instead.

> };
> 
> and v4l2_stream_configs is a super-set of v4l2_pad_configs
> 
> If we could get to
> 
> struct v4l2_subdev_state {
>          struct v4l2_subdev_krouting; /* Use for TRY and ACTIVE */
>          struct v4l2_stream_configs; /* Use for TRY and ACTIVE */
> };
> 
> This could turn out to be pretty neat, as it allows 'new' drivers to
> maintain their current formats and routings in a subdev 'state'
> instead of scattering those information in the driver-wide structure
> as they currently do for formats, crops and whatnot. This can ofc go
> on top.

Yes, that's the long term plan, but it's a huge change. And when I say 
plan, I don't mean I'm planning to change all the current drivers, I'm 
just saying my series is designed so that it allows these to be done in 
the future.

  Tomi