Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Steve Longerbeam <slongerbeam@gmail.com>]

On 4/9/19 3:59 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> On Tue, Apr 09, 2019 at 03:22:47PM -0700, Steve Longerbeam wrote:
>> On 4/9/19 1:57 PM, Laurent Pinchart wrote:
>>> On Mon, Apr 08, 2019 at 08:58:25PM -0700, Steve Longerbeam wrote:
>>>> On 4/8/19 4:12 AM, Niklas Söderlund wrote:
>>>>> On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
>>>>>> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
>>>>>>> Hi Niklas, Jacopo cc: Steve
>>>>>>>
>>>>>>> Apologize for reviving this old thread. Just one question below.
>>>>>>>
>>>>>>> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
>>>>>>>
>>>>>>> [..]
>>>>>>>
>>>>>>>>> +
>>>>>>>>>      #define vin_to_source(vin)		((vin)->parallel->subdev)
>>>>>>>> This in particular I hate and at some point I hope to remove it or
>>>>>>>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
>>>>>>>> related to  your patch-set.
>>>>>>> What about below patch excerpt (courtesy of Steve) which is currently
>>>>>>> under review in our tree? If we are on the same page here, we would
>>>>>>> happily contribute a patch to you based on below.
>>>>>> I can submit this patch to media-tree ML for formal review.
>>>>> I see no problem with the patch itself but as you point out bellow there
>>>>> are other patches which makes of the change I assume? The change to
>>>>> vin_to_source() on it's own would not add much value as vin_to_source()
>>>>> is currently only used in the none-mc parts of the driver where the
>>>>> change bellow would never be used.
>>>>>
>>>>> My dream would be to one day drop the none-mc mode of this driver, or
>>>>> split it out to a rcar-vin-legacy module or something ;-)
>>>> Yes, that's something that has been confusing me. Why does there need to
>>>> be a distinction between a media control mode non-mc mode in the
>>>> rcar-vin driver? I understand the "digital"/"parallel" mode is to handle
>>>> sensors with a parallel interface (as opposed to MIPI CSI-2), but why
>>>> not make the parallel sensors also require media-control
>>>> (enabling/disable media links, etc.)?
>>>>
>>>>>> But there are also other patches to rcar-vin applied to the tree mentioned
>>>>>> by Eugeniu, which can broadly be described as:
>>>>>>
>>>>>> 1. If the rcar-csi2 sub-device's source pad format is changed via media-ctl,
>>>>>> the connected VIN's crop bounds (vin->source rectangle) will be stale, since
>>>>>> vin->source must always be equal to the source pad rectangle. So we have a
>>>>>> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event sent
>>>>>> from the rcar-csi2 sub-device when its source pad format is changed. In
>>>>>> response, the VIN connected to that rcar-csi2 sub-device will reset the crop
>>>>>> bounds to the new source pad format rectangle. In order to make this work
>>>>>> however...
>>>>> I'm no expert on when the crop/selection rectangles shall be reset and
>>>>> have wrestled with this in the past for this driver. I have partial
>>>>> rework of how formats especially crop/selection works planed. My goal of
>>>>> that is to try and make it simpler and more straight forward taking
>>>>> ideas from the vivid driver. There are some limitations in my
>>>>> implementation of this in the current driver which prevents me from
>>>>> adding sequential formats and enable the UDS scaler.
>>>> Ok. What I did for imx-media driver is to export a function from the
>>>> video capture interface that allows the source sub-device connected to
>>>> the capture interface to call it when the source pad format changes,
>>>> which gives the capture interface the chance to adjust compose window
>>> That sounds like a big hack, the two devices should be independent.
>>> Let's not replicate this, and it should be fixed in the imx driver.
>> I admit it's a bit of a hack, but it is there to get around a limitation
>> of media/v4l2 framework. And the two devices are not independent, they
>> are connected the same way other media entities are connected. The
>> media-ctl tool will take care of propagating formats from connected
>> source -> sink sub-devices, but it stops at the sub-device ->
>> video_device interface. First, this propagation should happen in the
>> kernel at the time a source pad format is modified, rather than by a
>> user tool, because it is a media/v4l2 core requirement that source ->
>> sink formats must be equal before streaming starts.
> That's not right. It's a requirement that the formats are compatible to
> start streaming, but it's not a requirement *before* starting streaming.
> That's why kernel drivers must not propagate formats between entities,
> only within an entity. Doing in-kernel propagation between entities is
> an API violation.
>
>> Second, a source pad format change should also be propagated to
>> connected video devices. Userland is then free to make modifications
>> to the video_device format if the latter supports cropping/scaling
>> etc., but the default should be to reset the format at the time the
>> connected sub-device format has changed.
> For the same reason above, the format on video nodes must be set by
> applications. Could you please fix it in the imx driver ?

Well, if it's an API violation then I'll remove it. But removing the 
propagation will put (more) burden on userland.  In imx the sub-device 
does the cropping and scaling, but the video_device connected  to it 
does not. Which means the compose rectangle in the video_device must 
always be the same as the sub-device compose rectangle.

Does sending the V4L2_EVENT_SOURCE_CHANGE event to automatically reset 
the video_device format when the sub-device format changes also violate 
the API? I don't see why it would, it's just making use of the event 
mechanism. If you agree then I will do the same in imx, and offer it as 
a patch to rcar-vin (to ensure the rcar video_device's crop bounds are 
always correct).

If you don't agree then perhaps imx and rcar-vin can at least do the 
equivalent of v4l2-core, which is to ensure the video_device formats are 
compatible with the connected sub-device before streaming can start.


Steve



>
>>>> (unlike rcar-vin, imx capture interface doesn't crop or scale, the
>>>> connected source subdev does that, but it does pad the compose window so
>>>> it needs to know the original compose window, sorry hope I haven't lost
>>>> you). Anyway I think catching the V4L2_EVENT_SOURCE_CHANGE event is
>>>> maybe a cleaner way to update the crop bounds in rcar-vin than using an
>>>> exported function.
>>>>
>>>>>> 2. Sub-device events need to be forwarded to the VIN's that have enabled
>>>>>> media links to the reporting sub-device. Currently, sub-device events are
>>>>>> only forwarded to VIN7, because the notifier is registered only from VIN7,
>>>>>> and so the sub-devices are registered only to the v4l2_dev owned by VIN7. So
>>>>>> we have a set of patches that fix this: sub-device events get forwarded to
>>>>>> the VIN's that have connected media paths to that sub-device. Besides
>>>>>> allowing to reset the VIN crop bounds rectangle asynchronously, this also
>>>>>> seems to be logically the correct behavior. It also makes the user interface
>>>>>> a little more intuitive: userland knows which VIN it is capturing on, and
>>>>>> that is the same VIN that will be receiving sub-device events in the active
>>>>>> pipeline.
>>>>> This is a problem I ponder from time to time, happy to hear I'm not the
>>>>> only one :-) My view is that events are somewhat not functional in the
>>>>> media controller world and should be fixed at the framework level, maybe
>>>>> by moving them from the vdev to the mdev :-)
>>>> How about embedding the 'struct v4l2_device' into 'struct rvin_group'
>>>> instead of 'struct rvin_dev'? That probably makes more sense, the
>>>> v4l2_dev keeps track of media-device-wide info such as the list of
>>>> sub-devices in the graph, so it seems more appropriate to create only
>>>> one instance of v4l2_dev. That will force rcar-vin to lookup the correct
>>>> VINs to forward the event to, since v4l2_dev is no longer attached to
>>>> the VINs.
>>>>
>>>> But moving events from v4l2 to media (e.g. media entities send events to
>>>> the mdev, rather than v4l2 sub-devices sending events to the v4l2_dev)
>>>> is also an interesting idea.
>>>>
>>>>> I think it's great that you worked on the problem but I'm a bit
>>>>> concerned with addressing this on a driver basis. Maybe this is
>>>>> something to bring up at the next v4l2 summit? I might be wrong here and
>>>>> there already is a consensus to address this as you describe in each
>>>>> driver. Do you have any insights on the topic?
>>>> I haven't been tuned into that topic, but yes I agree that events need a
>>>> better framework.
>>>>
>>>>>> 3. We have some patches under review that allow alternate -> none field
>>>>>> mode. That is, source sub-device is sending alternate fields (top, bottom,
>>>>>> top, ...), and userland is configured to receive those fields unmodified by
>>>>>> setting field type to none. rcar-dma then detects and reports the field type
>>>>>> of the captured field (top or bottom) in (struct v4l2_buffer)->field. Doing
>>>>>> this allows for de-interlacing the captured fields using the FDP1 mem2mem
>>>>>> device.
>>>>> Interesting, maybe I'm missing something but would it not be a better
>>>>> solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of
>>>>> abusing V4L2_FIELD_NONE?
>>>> Well, from 6c51f646f6 ("media: rcar-vin: fix handling of single field
>>>> frames (top, bottom and alternate fields)"),
>>>>
>>>> "There was never proper support in the VIN driver to deliver ALTERNATING
>>>> field format to user-space, remove this field option. The problem is
>>>> that ALTERNATING field order requires the sequence numbers of buffers
>>>> returned to userspace to reflect if fields were dropped or not,
>>>> something which is not possible with the VIN drivers capture logic."
>>>>
>>>> Is that still a concern, or can alternate mode be brought back but with
>>>> possibly the above limitation?
>>>>
>>>> Also, there is this paragraph in [1] regarding V4L2_FIELD_NONE, so it
>>>> sounds like using this field type in this way may not be too much abuse :)
>>>>
>>>> "Images are in progressive format, not interlaced. The driver may also
>>>> indicate this order when it cannot distinguish
>>>> between|V4L2_FIELD_TOP|and|V4L2_FIELD_BOTTOM|."
>>>>
>>>> [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html
>>>>
>>>>>> There are some other miscellaneous patches that I can submit, but the above
>>>>>> describes the bulk of the changes.
>>>>> I'm happy to see work being done on the driver and would of course like
>>>>> to help you get all changes upstream so that all your use-cases would
>>>>> work out-of-the-box.
>>>>>
>>>>>> Before I start on porting these patches to the media-tree, do the above
>>>>>> changes make general sense, or are there fundamental problems with those
>>>>>> ideas?
>>>>>>
>>>>>>> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
>>>>>>>
>>>>>>> Change the vin_to_source() macro to an inline function that will
>>>>>>> retrieve the source subdevice for both media-control and non
>>>>>>> media-control mode.
>>>>>>>
>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>> ---
>>>>>>> [..]
>>>>>>>
>>>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>>> index 0b13b34d03e3..29d8c4a80c35 100644
>>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>>> @@ -217,7 +217,21 @@ struct rvin_dev {
>>>>>>>      	v4l2_std_id std;
>>>>>>>      };
>>>>>>> -#define vin_to_source(vin)		((vin)->parallel->subdev)
>>>>>>> +static inline struct v4l2_subdev *
>>>>>>> +vin_to_source(struct rvin_dev *vin)
>>>>>>> +{
>>>>>>> +	if (vin->info->use_mc) {
>>>>>>> +		struct media_pad *pad;
>>>>>>> +
>>>>>>> +		pad = media_entity_remote_pad(&vin->pad);
>>>>>>> +		if (!pad)
>>>>>>> +			return NULL;
>>>>>>> +
>>>>>>> +		return media_entity_to_v4l2_subdev(pad->entity);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return vin->parallel->subdev;
>>>>>>> +}
>>>>>>>      /* Debug */
>>>>>>>      #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
>>>>>>>