Hi Steve, Niklas,
Just a small 2-cents below:
On 09/04/2019 04:58, Steve Longerbeam wrote:
>
>
> On 4/8/19 4:12 AM, Niklas Söderlund wrote:
>> Hi Steve, Eugeniu,
>>
>> On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
>>> Hi Niklas, all,
>>>
>>>
>>> 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
> (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?
I think that might have been before we added the scratch buffer to
support continuous capture?
Now that we have that - I think we should be able to correctly track
dropped frames right?
--
Kieran
>
> 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
>
>
> Steve
>
>>
>>> 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?
>>>
>>> TIA,
>>> Steve
>>>
>>>
>>>> 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@xxxxxxxxxx>
>>>> ---
>>>> [..]
>>>>
>>>> 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)
>>>>
>>>> Best regards,
>>>> Eugeniu.
>
--
Regards
--
Kieran
