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

From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
To: Steve Longerbeam <slongerbeam@gmail.com>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
	Eugeniu Rosca <erosca@de.adit-jv.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	Steve Longerbeam <steve_longerbeam@mentor.com>,
	"George G. Davis" <george_davis@mentor.com>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Michael Rodin <mrodin@de.adit-jv.com>,
	Eugen Friedrich <efriedrich@de.adit-jv.com>,
	harsha.manjulamallikarjun@in.bosch.com,
	Eugeniu Rosca <roscaeugeniu@gmail.com>
Subject: Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier
Date: Mon, 8 Apr 2019 13:12:41 +0200	[thread overview]
Message-ID: <20190408111241.GF15350@bigcity.dyn.berto.se> (raw)
In-Reply-To: <25dc2668-d270-ebf8-b014-88bbb7a6f243@gmail.com>

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

-- 
Regards,
Niklas Söderlund