LinuxTV Patchwork [v3,3/9] media: rcar-vin: Create a group notifier

login
register
mail settings
Submitter Eugeniu Rosca
Date April 5, 2019, 4:16 p.m.
Message ID <20190405161639.GA10433@vmlxhi-102.adit-jv.com>
Download mbox | patch
Permalink /patch/55488/
State New
Headers show

Comments

Eugeniu Rosca - April 5, 2019, 4:16 p.m.
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:

> Hi Jacopo,

> Thanks for your patch.
>
> On 2018-05-18 16:40:39 +0200, Jacopo Mondi wrote:
>> As CSI-2 subdevices are shared between several VIN instances, a shared
>> notifier to collect the CSI-2 async subdevices is required. So far, the
>> rcar-vin driver used the notifier of the last VIN instance to probe but
>> with the forth-coming introduction of parallel input subdevices support
>> in mc-compliant code path, each VIN may register its own notifier if any
>> parallel subdevice is connected there.
>> 
>> To avoid registering a notifier twice (once for parallel subdev and one
>> for the CSI-2 subdevs) create a group notifier, shared by all the VIN
>> instances.
>> 
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> ---
>>  drivers/media/platform/rcar-vin/rcar-core.c | 39 +++++++++++------------------
>>  drivers/media/platform/rcar-vin/rcar-vin.h  |  7 +++---
>>  2 files changed, 18 insertions(+), 28 deletions(-)

[..]

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

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


Best regards,
Eugeniu.
Steve Longerbeam - April 6, 2019, 12:04 a.m.
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.

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

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.

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.

There are some other miscellaneous patches that I can submit, but the 
above describes the bulk of the changes.

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@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)
>
> Best regards,
> Eugeniu.
Niklas Söderlund - April 8, 2019, 11:12 a.m.
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 ;-)

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

> 
> 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 :-)

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?

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

> 
> 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@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)
> > 
> > Best regards,
> > Eugeniu.
>
Steve Longerbeam - April 9, 2019, 3:58 a.m.
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?

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@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)
>>>
>>> Best regards,
>>> Eugeniu.
Kieran Bingham - April 9, 2019, 9:10 a.m.
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@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)
>>>>
>>>> Best regards,
>>>> Eugeniu.
>
Niklas Söderlund - April 9, 2019, 1:25 p.m.
Hi Steve, Kieran,

On 2019-04-09 10:10:45 +0100, Kieran Bingham wrote:
> 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.)?
> > 
> > 

The issue is backward compatibility. The original driver targeted 
Renesas Gen2 board which was relatively simple and might even pre-date 
the media controller api. When I extended the diver to support Gen3 
boards which are more complex I had to add media controller support to 
the driver. There is a huge overlap (rcar-dma.c) between Gen2 and Gen3 
and it seemed silly to have two drivers.

As a side note Jacopo added support for the parallel node in the media 
controller for Gen3 boards which have the same pipeline as Gen2. So 
everything needed to remove the none MC code paths are in the driver but 
I'm afraid that would upset current users on Gen2 ;-)

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

I agree V4L2_EVENT_SOURCE_CHANGE could be a nice way of doing this if 
the event routing is sorted out.

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

It might solve the event routing problem but will create others. For 
example each VIN instance can have a private notifier if it has a 
parallel subdevice and be part of VIN group for the ones coming from 
CSI-2.

> > 
> > 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 this is the right way to go.

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

I think you are correct Kieran, we now have the scratch buffer so it 
should be possible to add proper support for delivering ALTERNATING to 
userspace. I'm sure we find new issues on the way tho ;-)

> 
> 
> > 
> > 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 :)

My bad I should have said using instead of abusing, sorry about that :-) 
I did not know that this was a valid use-case for FIELD_NONE so yes I 
think it could be used if we can't solve the requirements for adding 
ALTERNATING.

> > 
> > "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@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)
> >>>>
> >>>> Best regards,
> >>>> Eugeniu.
> > 
> 
> -- 
> Regards
> --
> Kieran
Laurent Pinchart - April 9, 2019, 8:57 p.m.
Hi Steve,

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.

> (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)
> >>>
Steve Longerbeam - April 9, 2019, 10:22 p.m.
Hi Laurent,

On 4/9/19 1:57 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> 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. 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.


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)
>>>>>
Steve Longerbeam - April 9, 2019, 10:52 p.m.
On 4/9/19 6:25 AM, Niklas Söderlund wrote:
> Hi Steve, Kieran,
>
> On 2019-04-09 10:10:45 +0100, Kieran Bingham wrote:
>> 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.)?
>>>
>>>
> The issue is backward compatibility. The original driver targeted
> Renesas Gen2 board which was relatively simple and might even pre-date
> the media controller api. When I extended the diver to support Gen3
> boards which are more complex I had to add media controller support to
> the driver. There is a huge overlap (rcar-dma.c) between Gen2 and Gen3
> and it seemed silly to have two drivers.

Ok, I suspected it was a backward compatibility issue, makes sense.

> As a side note Jacopo added support for the parallel node in the media
> controller for Gen3 boards which have the same pipeline as Gen2. So
> everything needed to remove the none MC code paths are in the driver but
> I'm afraid that would upset current users on Gen2 ;-)
>
>>>>> 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.
>>>
> I agree V4L2_EVENT_SOURCE_CHANGE could be a nice way of doing this if
> the event routing is sorted out.

Ok.

But also, in my reply to Laurent, I brought the idea that there needs to 
be a mechanism in v4l2-core that will take care of this, e.g. 
automatically propagate formats from a sub-device source pad to a 
connected video device. I haven't looked into that idea in more detail 
so there are probably problems to overcome.

>>>
>>>>> 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.
> It might solve the event routing problem but will create others. For
> example each VIN instance can have a private notifier if it has a
> parallel subdevice and be part of VIN group for the ones coming from
> CSI-2.

Ah, ok, thanks for the warning. I would have run into that issue 
eventually, but now I won'r waste my time :)


>>> 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 this is the right way to go.

Yes, especially because events are not limited to video, they are 
applicable to other kinds of media devices.

>
>>>> 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?
> I think you are correct Kieran, we now have the scratch buffer so it
> should be possible to add proper support for delivering ALTERNATING to
> userspace. I'm sure we find new issues on the way tho ;-)

Ok, but still it seems alternate -> none is a valid use-case? It allows 
rcar-vin to send unmodified fields to userland, for possible 
de-interlace by FDP1 mem2mem. Or does FDP1 support de-interlacing an 
INTERLACED stream (in addition to de-interlacing a sequential field stream)?


>
>>
>>> 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 :)
> My bad I should have said using instead of abusing, sorry about that :-)
> I did not know that this was a valid use-case for FIELD_NONE so yes I
> think it could be used if we can't solve the requirements for adding
> ALTERNATING.

Ok, see above also.

Steve

>
>>> "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@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)
>>>>>>
>>>>>> Best regards,
>>>>>> Eugeniu.
>> -- 
>> Regards
>> --
>> Kieran
Laurent Pinchart - April 9, 2019, 10:59 p.m.
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 ?

> >> (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)
> >>>>>
Steve Longerbeam - April 9, 2019, 11:29 p.m.
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)
>>>>>>>
Eugeniu Rosca - April 11, 2019, 8:28 p.m.
Hi Niklas,

Two simple questions:
 - I can only clone git://git.ragnatech.se/linux, but can't access it on
   the web, does it have some web interface?
 - Browsing through your v4l2/next/csi2/upport branch, it looks to be
   targeted for upstream. Could we rely on those commits already or
   they are still in development/testing?

TIA,
Eugeniu.
Niklas Söderlund - April 11, 2019, 8:42 p.m.
Hi Eugenie,

On 2019-04-11 22:28:19 +0200, Eugeniu Rosca wrote:
> Hi Niklas,
> 
> Two simple questions:
>  - I can only clone git://git.ragnatech.se/linux, but can't access it on
>    the web, does it have some web interface?

There should be a gitweb instance running at https://git.ragnatech.se/ 
if not let me know since then something is wrong ;-)

>  - Browsing through your v4l2/next/csi2/upport branch, it looks to be
>    targeted for upstream. Could we rely on those commits already or
>    they are still in development/testing?

The patches in that branch are target for upstreaming but sometime 
changes due to review comments. Best check the media-tree of what have 
accepted and the patchwork instance if you want day fresh statuses. I 
also rebase the branch from time to time on-top of the media-tree so you 
should not depend on it much I'm afraid.

If you plan to do some work on the rcar-csi2 driver you can always reach 
out and check what the status is if you are unsure.
Eugeniu Rosca - April 12, 2019, 2:12 p.m.
Hi Niklas,

On Thu, Apr 11, 2019 at 10:42:37PM +0200, Niklas Söderlund wrote:
> Hi Eugenie,
> 
> On 2019-04-11 22:28:19 +0200, Eugeniu Rosca wrote:
> > Hi Niklas,
> > 
> > Two simple questions:
> >  - I can only clone git://git.ragnatech.se/linux, but can't access it on
> >    the web, does it have some web interface?
> 
> There should be a gitweb instance running at https://git.ragnatech.se/ 
> if not let me know since then something is wrong ;-)

Yes, I am able to access the gitweb instance too :)

> 
> >  - Browsing through your v4l2/next/csi2/upport branch, it looks to be
> >    targeted for upstream. Could we rely on those commits already or
> >    they are still in development/testing?
> 
> The patches in that branch are target for upstreaming but sometime 
> changes due to review comments. Best check the media-tree of what have 
> accepted and the patchwork instance if you want day fresh statuses. I 
> also rebase the branch from time to time on-top of the media-tree so you 
> should not depend on it much I'm afraid.
> 
> If you plan to do some work on the rcar-csi2 driver you can always reach 
> out and check what the status is if you are unsure.

Understood. Thanks. Since I've gone off-topic quite a bit already, may I
have your feedback on below questions sitting in the back of my mind for
some time:

 - There seem to be quite a number of non-mainlined rcar-vin patches
   sitting on top of v4.14.75-ltsi and part of rcar-3.9.x release. With
   some false positives, around 35-40 such commits can be identified as
   shown in [1]. Based on my checks, only a small subset of them are
   (actively) discussed in linux-media/linux-renesas-soc ML and for most
   of them I can't find any trace. I am just curious if there is any
   background force driving the upstreaming of those patches or the
   submissions/discussions happen rather by accident?

 - I am seeing two sets of rcar-vin tests [2-3] and I think there is a
   parent/fork relationship between them, but now they appear to be
   diverged. Which one would you recommend? What's the test coverage
   provided by these suites? Do you see room/chance for something like
   drivers/gpu/drm/selftests in rcar-vin in future (maybe using
   the upcoming kunit [4] approach?). BTW, we've been using the
   vsp-tests for quite some time and they've been extremely useful in
   pointing out regressions in our own patches. So, many thanks for
   sharing those too.

> 
> -- 
> Regards,
> Niklas Söderlund

[1] git log --oneline --cherry-pick --right-only \
    torvalds/master...rcar-3.9.4 -- drivers/media/platform/rcar-vin/
[2] https://git.ragnatech.se/vin-tests/
[3] http://jmondi.org/git/vin-tests/
[4] https://patchwork.kernel.org/cover/10704157/

Best regards,
Eugeniu.
Niklas Söderlund - April 12, 2019, 3:10 p.m.
Hi Eugeniu,

On 2019-04-12 16:12:05 +0200, Eugeniu Rosca wrote:
> Hi Niklas,
> 
> On Thu, Apr 11, 2019 at 10:42:37PM +0200, Niklas Söderlund wrote:
> > Hi Eugenie,
> > 
> > On 2019-04-11 22:28:19 +0200, Eugeniu Rosca wrote:
> > > Hi Niklas,
> > > 
> > > Two simple questions:
> > >  - I can only clone git://git.ragnatech.se/linux, but can't access it on
> > >    the web, does it have some web interface?
> > 
> > There should be a gitweb instance running at https://git.ragnatech.se/ 
> > if not let me know since then something is wrong ;-)
> 
> Yes, I am able to access the gitweb instance too :)
> 
> > 
> > >  - Browsing through your v4l2/next/csi2/upport branch, it looks to be
> > >    targeted for upstream. Could we rely on those commits already or
> > >    they are still in development/testing?
> > 
> > The patches in that branch are target for upstreaming but sometime 
> > changes due to review comments. Best check the media-tree of what have 
> > accepted and the patchwork instance if you want day fresh statuses. I 
> > also rebase the branch from time to time on-top of the media-tree so you 
> > should not depend on it much I'm afraid.
> > 
> > If you plan to do some work on the rcar-csi2 driver you can always reach 
> > out and check what the status is if you are unsure.
> 
> Understood. Thanks. Since I've gone off-topic quite a bit already, may I
> have your feedback on below questions sitting in the back of my mind for
> some time:

Off topic is good ;-)

> 
>  - There seem to be quite a number of non-mainlined rcar-vin patches
>    sitting on top of v4.14.75-ltsi and part of rcar-3.9.x release. With
>    some false positives, around 35-40 such commits can be identified as
>    shown in [1]. Based on my checks, only a small subset of them are
>    (actively) discussed in linux-media/linux-renesas-soc ML and for most
>    of them I can't find any trace. I am just curious if there is any
>    background force driving the upstreaming of those patches or the
>    submissions/discussions happen rather by accident?

Yes I'm driving the upstreaming of most of those out of tree patches.  
Currently a lot of them are blocked on the organic growth of the 
addition of Gen3 and my lack of understanding of v4l2 at the time. Now I 
know better and I'm in the process of cleaning up the driver to prepare 
upstreaming most of them. I will send out the first cleanup patch later 
today.

I have in the past tried to upstream all patches and then cleanup the 
feature complete driver but it's gotten to a point where that is more 
painful then doing the right thing (tm).

If there is any particular patch that would be helpful for you to have 
upstream earlier then others let me know and I can try to put it first 
in the queue.

> 
>  - I am seeing two sets of rcar-vin tests [2-3] and I think there is a
>    parent/fork relationship between them, but now they appear to be
>    diverged. Which one would you recommend? What's the test coverage
>    provided by these suites? Do you see room/chance for something like
>    drivers/gpu/drm/selftests in rcar-vin in future (maybe using
>    the upcoming kunit [4] approach?). BTW, we've been using the
>    vsp-tests for quite some time and they've been extremely useful in
>    pointing out regressions in our own patches. So, many thanks for
>    sharing those too.

The vin-tests at ragnatech.se is the parent one. It's not as nice as the 
vsp-tests and I would like to add more tests and rewrite it in python3.  
If you have additional tests pleas send me patches and I will be happy 
to include them.

One idea I would really like to do is a HDMI loopback test. Connecting 
the DU and VIN and running tests on that to allow automatic testing of 
multiple resolutions and in the future scaling. As it is now all 
vin-tests depends on external video sources. An alternate idea to this 
is to create a remote "video server" on a RPi as it can deliver both 
HDMI and CVBS video...

I see no issue with using a kernel test framework but I have little 
experience in that area and it's not something I'm planing on worknig on 
at the moment.

> 
> > 
> > -- 
> > Regards,
> > Niklas Söderlund
> 
> [1] git log --oneline --cherry-pick --right-only \
>     torvalds/master...rcar-3.9.4 -- drivers/media/platform/rcar-vin/
> [2] https://git.ragnatech.se/vin-tests/
> [3] http://jmondi.org/git/vin-tests/
> [4] https://patchwork.kernel.org/cover/10704157/
> 
> Best regards,
> Eugeniu.

Patch

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)

Privacy Policy