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@xxxxxxxxxx>
> >>> ---
> >>> [..]
> >>>
> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> >>> index 0b13b34d03e3..29d8c4a80c35 100644
> >>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> >>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> >>> @@ -217,7 +217,21 @@ struct rvin_dev {
> >>> v4l2_std_id std;
> >>> };
> >>> -#define vin_to_source(vin) ((vin)->parallel->subdev)
> >>> +static inline struct v4l2_subdev *
> >>> +vin_to_source(struct rvin_dev *vin)
> >>> +{
> >>> + if (vin->info->use_mc) {
> >>> + struct media_pad *pad;
> >>> +
> >>> + pad = media_entity_remote_pad(&vin->pad);
> >>> + if (!pad)
> >>> + return NULL;
> >>> +
> >>> + return media_entity_to_v4l2_subdev(pad->entity);
> >>> + }
> >>> +
> >>> + return vin->parallel->subdev;
> >>> +}
> >>> /* Debug */
> >>> #define vin_dbg(d, fmt, arg...) dev_dbg(d->dev, fmt, ##arg)
> >>>
--
Regards,
Laurent Pinchart
