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

From: Steve Longerbeam <slongerbeam@gmail.com>
To: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Jacopo Mondi" <jacopo+renesas@jmondi.org>
Cc: 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: Fri, 5 Apr 2019 17:04:50 -0700	[thread overview]
Message-ID: <25dc2668-d270-ebf8-b014-88bbb7a6f243@gmail.com> (raw)
In-Reply-To: <20190405161639.GA10433@vmlxhi-102.adit-jv.com>

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.