| Message ID | 1526654445-10702-4-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 1aadd90..c6e603f 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -583,7 +583,7 @@ static int rvin_parallel_graph_init(struct rvin_dev *vin) > > static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier) > { > - struct rvin_dev *vin = notifier_to_vin(notifier); > + struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev); > const struct rvin_group_route *route; > unsigned int i; > int ret; > @@ -649,7 +649,7 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > struct v4l2_async_subdev *asd) > { > - struct rvin_dev *vin = notifier_to_vin(notifier); > + struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev); > unsigned int i; > > for (i = 0; i < RCAR_VIN_NUM; i++) > @@ -673,7 +673,7 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > struct v4l2_async_subdev *asd) > { > - struct rvin_dev *vin = notifier_to_vin(notifier); > + struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev); > unsigned int i; > > mutex_lock(&vin->group->lock); > @@ -734,12 +734,6 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) > > mutex_lock(&vin->group->lock); > > - /* If there already is a notifier something has gone wrong, bail out. */ > - if (WARN_ON(vin->group->notifier)) { > - mutex_unlock(&vin->group->lock); > - return -EINVAL; > - } > - > /* If not all VIN's are registered don't register the notifier. */ > for (i = 0; i < RCAR_VIN_NUM; i++) > if (vin->group->vin[i]) > @@ -751,19 +745,16 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) > } > > /* > - * Have all VIN's look for subdevices. Some subdevices will overlap > - * but the parser function can handle it, so each subdevice will > - * only be registered once with the notifier. > + * Have all VIN's look for CSI-2 subdevices. Some subdevices will > + * overlap but the parser function can handle it, so each subdevice > + * will only be registered once with the group notifier. > */ > - > - vin->group->notifier = &vin->notifier; > - > for (i = 0; i < RCAR_VIN_NUM; i++) { > if (!vin->group->vin[i]) > continue; > > ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( > - vin->group->vin[i]->dev, vin->group->notifier, > + vin->group->vin[i]->dev, &vin->group->notifier, > sizeof(struct v4l2_async_subdev), 1, > rvin_mc_parse_of_endpoint); > if (ret) { > @@ -774,9 +765,12 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) > > mutex_unlock(&vin->group->lock); > > - vin->group->notifier->ops = &rvin_group_notify_ops; > + if (!vin->group->notifier.num_subdevs) > + return 0; > > - ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier); > + vin->group->notifier.ops = &rvin_group_notify_ops; > + ret = v4l2_async_notifier_register(&vin->v4l2_dev, > + &vin->group->notifier); > if (ret < 0) { > vin_err(vin, "Notifier registration failed\n"); > return ret; > @@ -1112,15 +1106,10 @@ static int rcar_vin_remove(struct platform_device *pdev) > v4l2_async_notifier_unregister(&vin->notifier); > v4l2_async_notifier_cleanup(&vin->notifier); If the vin being removed is the one which v4l2_dev is used to register the group you should unregister the group notifier here. You got this for 'free' with the above code before this change so it's easy to miss :-) > > - if (vin->info->use_mc) { > - mutex_lock(&vin->group->lock); > - if (vin->group->notifier == &vin->notifier) > - vin->group->notifier = NULL; > - mutex_unlock(&vin->group->lock); > + if (vin->info->use_mc) > rvin_group_put(vin); > - } else { > + else > v4l2_ctrl_handler_free(&vin->ctrl_handler); > - } > > rvin_dma_unregister(vin); > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h > index 755ac3c..7d0ffe08 100644 > --- a/drivers/media/platform/rcar-vin/rcar-vin.h > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h > @@ -209,6 +209,8 @@ struct rvin_dev { > v4l2_std_id std; > }; > > +#define v4l2_dev_to_vin(d) container_of(d, struct rvin_dev, v4l2_dev) I think you should either define this in rcar-core.c (see notifier_to_vin()) or use the container_of() directly to limit it's scope. Over all I'm not happy about my initial usage these defines in the driver, I was young and dumb :-) I won't object if you wish to keep it but then please move it to the .c file. > + > #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. > > /* Debug */ > @@ -225,8 +227,7 @@ struct rvin_dev { > * > * @lock: protects the count, notifier, vin and csi members > * @count: number of enabled VIN instances found in DT > - * @notifier: pointer to the notifier of a VIN which handles the > - * groups async sub-devices. > + * @notifier: group notifier for CSI-2 async subdevices > * @vin: VIN instances which are part of the group > * @csi: array of pairs of fwnode and subdev pointers > * to all CSI-2 subdevices. > @@ -238,7 +239,7 @@ struct rvin_group { > > struct mutex lock; > unsigned int count; > - struct v4l2_async_notifier *notifier; > + struct v4l2_async_notifier notifier; > struct rvin_dev *vin[RCAR_VIN_NUM]; > > struct { > -- > 2.7.4 >
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> --- [..] 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.
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.
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. >
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.
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. >
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
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) > >>>
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) >>>>>
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
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) > >>>>>
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) >>>>>>>
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.
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.
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.
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.
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index 1aadd90..c6e603f 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -583,7 +583,7 @@ static int rvin_parallel_graph_init(struct rvin_dev *vin) static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier) { - struct rvin_dev *vin = notifier_to_vin(notifier); + struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev); const struct rvin_group_route *route; unsigned int i; int ret; @@ -649,7 +649,7 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier, struct v4l2_subdev *subdev, struct v4l2_async_subdev *asd) { - struct rvin_dev *vin = notifier_to_vin(notifier); + struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev); unsigned int i; for (i = 0; i < RCAR_VIN_NUM; i++) @@ -673,7 +673,7 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier, struct v4l2_subdev *subdev, struct v4l2_async_subdev *asd) { - struct rvin_dev *vin = notifier_to_vin(notifier); + struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev); unsigned int i; mutex_lock(&vin->group->lock); @@ -734,12 +734,6 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) mutex_lock(&vin->group->lock); - /* If there already is a notifier something has gone wrong, bail out. */ - if (WARN_ON(vin->group->notifier)) { - mutex_unlock(&vin->group->lock); - return -EINVAL; - } - /* If not all VIN's are registered don't register the notifier. */ for (i = 0; i < RCAR_VIN_NUM; i++) if (vin->group->vin[i]) @@ -751,19 +745,16 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) } /* - * Have all VIN's look for subdevices. Some subdevices will overlap - * but the parser function can handle it, so each subdevice will - * only be registered once with the notifier. + * Have all VIN's look for CSI-2 subdevices. Some subdevices will + * overlap but the parser function can handle it, so each subdevice + * will only be registered once with the group notifier. */ - - vin->group->notifier = &vin->notifier; - for (i = 0; i < RCAR_VIN_NUM; i++) { if (!vin->group->vin[i]) continue; ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( - vin->group->vin[i]->dev, vin->group->notifier, + vin->group->vin[i]->dev, &vin->group->notifier, sizeof(struct v4l2_async_subdev), 1, rvin_mc_parse_of_endpoint); if (ret) { @@ -774,9 +765,12 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) mutex_unlock(&vin->group->lock); - vin->group->notifier->ops = &rvin_group_notify_ops; + if (!vin->group->notifier.num_subdevs) + return 0; - ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier); + vin->group->notifier.ops = &rvin_group_notify_ops; + ret = v4l2_async_notifier_register(&vin->v4l2_dev, + &vin->group->notifier); if (ret < 0) { vin_err(vin, "Notifier registration failed\n"); return ret; @@ -1112,15 +1106,10 @@ static int rcar_vin_remove(struct platform_device *pdev) v4l2_async_notifier_unregister(&vin->notifier); v4l2_async_notifier_cleanup(&vin->notifier); - if (vin->info->use_mc) { - mutex_lock(&vin->group->lock); - if (vin->group->notifier == &vin->notifier) - vin->group->notifier = NULL; - mutex_unlock(&vin->group->lock); + if (vin->info->use_mc) rvin_group_put(vin); - } else { + else v4l2_ctrl_handler_free(&vin->ctrl_handler); - } rvin_dma_unregister(vin); diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h index 755ac3c..7d0ffe08 100644 --- a/drivers/media/platform/rcar-vin/rcar-vin.h +++ b/drivers/media/platform/rcar-vin/rcar-vin.h @@ -209,6 +209,8 @@ struct rvin_dev { v4l2_std_id std; }; +#define v4l2_dev_to_vin(d) container_of(d, struct rvin_dev, v4l2_dev) + #define vin_to_source(vin) ((vin)->parallel->subdev) /* Debug */ @@ -225,8 +227,7 @@ struct rvin_dev { * * @lock: protects the count, notifier, vin and csi members * @count: number of enabled VIN instances found in DT - * @notifier: pointer to the notifier of a VIN which handles the - * groups async sub-devices. + * @notifier: group notifier for CSI-2 async subdevices * @vin: VIN instances which are part of the group * @csi: array of pairs of fwnode and subdev pointers * to all CSI-2 subdevices. @@ -238,7 +239,7 @@ struct rvin_group { struct mutex lock; unsigned int count; - struct v4l2_async_notifier *notifier; + struct v4l2_async_notifier notifier; struct rvin_dev *vin[RCAR_VIN_NUM]; struct {
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(-)