Re: [PATCH v9 13/28] rcar-vin: fix handling of single field frames (top, bottom and alternate fields)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

Thanks for your comments.

On 2017-12-08 21:30:20 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Friday, 8 December 2017 16:06:58 EET Niklas Söderlund wrote:
> > On 2017-12-08 11:35:18 +0200, Laurent Pinchart wrote:
> > > On Friday, 8 December 2017 03:08:27 EET Niklas Söderlund wrote:
> > >> There was never proper support in the VIN driver to deliver ALTERNATING
> > >> field format to user-space, remove this field option. For sources using
> > >> this field format instead use the VIN hardware feature of combining the
> > >> fields to an interlaced format. This mode of operation was previously
> > >> the default behavior and ALTERNATING was only delivered to user-space if
> > >> explicitly requested. Allowing this to be explicitly requested was a
> > >> mistake and was never properly tested and never worked due to the
> > >> constraints put on the field format when it comes to sequence numbers
> > >> and timestamps etc.
> > > 
> > > I'm puzzled, why can't we support V4L2_FIELD_ALTERNATE if we can support
> > > V4L2_FIELD_TOP and V4L2_FIELD_BOTTOM ? I don't dispute the fact that the
> > > currently implemented logic might be wrong (although I haven't
> > > double-checked that), but what prevents us from implementing it correctly
> > > ?
> > 
> > Maybe my commit message is fuzzy. We can support V4L2_FIELD_ALTERNATE as
> > a source to the VIN but we can't (yet) support delivering it to
> > user-space in a good way. So if we have a video source which outputs
> > V4L2_FIELD_ALTERNATE we are fine as we can use the hardware to interlace
> > that or only capture the TOP or BOTTOM fields.
> > 
> > But the driver logic to capture frames (the whole dance with single and
> > continues capture modes) to be able to deal with situations where
> > buffers are not queued fast enough currently prevents us from delivering
> > V4L2_FIELD_ALTERNATE to user-space. The problem is we can only capture
> > (correctly) ALTERNATE if we run in continues mode, if the driver is feed
> > buffers to slow and switches to single capture mode we can't live up to
> > the specification of the field order from the documentation:
> > 
> > "If fields are successive, without any dropped fields between them
> > (fields can drop individually), can be determined from the struct
> > v4l2_buffer sequence field."
> > 
> > So even if in single capture mode we switch between TOP and BOTTOM for
> > each capture the sequence number would always be sequential but the
> > fields would in temporal time potentially be far apparat (depending on
> > how fast user-space queues buffers + the time it takes to shutdown and
> > startup the VIN capture).
> > 
> > So instead of badly supporting this field order now I feel it's better
> > to not support it and once we tackle the issue of trying to remove
> > single capture mode (if at all possible) add support for it. But this is
> > a task for a different patch-set as this one is quiet large already and
> > it's focus is to add Gen3 support.
> 
> OK, so we could support capturing alternating fields, but in that case we 
> wouldn't be able to provide accurate sequence numbers. I'm fine with dropping 
> support for ALTERNATE, but I would capture that information in the commit 
> message, and probably as well in a comment in the code.

Agree, I tried to capture this in the commit message. But as You did not 
understand it I need to make a better job. I will expand on this in the 
commit message and add a comment in the code. Thanks.

> 
> > >> The height should not be cut in half for the format for TOP or BOTTOM
> > >> fields settings. This was a mistake and it was made visible by the
> > >> scaling refactoring. Correct behavior is that the user should request a
> > >> frame size that fits the half height frame reflected in the field
> > >> setting. If not the VIN will do its best to scale the top or bottom to
> > >> the requested format and cropping and scaling do not work as expected.
> > >> 
> > >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > >> Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> > >> ---
> > >> 
> > >>  drivers/media/platform/rcar-vin/rcar-dma.c  | 15 +--------
> > >>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 48 +++++++++--------------
> > >>  2 files changed, 19 insertions(+), 44 deletions(-)
> 
> [snip]
> 
> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > >> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > >> 9cf9ff48ac1e2f4f..37fe1f6c646b0ea3 100644
> > >> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > >> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > >> @@ -102,6 +102,24 @@ static int rvin_get_sd_format(struct rvin_dev *vin,
> > >> struct v4l2_pix_format *pix)
> > >> 	if (ret)
> > >>  		return ret;
> > >> 
> > >> +	switch (fmt.format.field) {
> > >> +	case V4L2_FIELD_TOP:
> > >> +	case V4L2_FIELD_BOTTOM:
> > >> +	case V4L2_FIELD_NONE:
> > >> +	case V4L2_FIELD_INTERLACED_TB:
> > >> +	case V4L2_FIELD_INTERLACED_BT:
> > >> +	case V4L2_FIELD_INTERLACED:
> > >> +		break;
> > >> +	case V4L2_FIELD_ALTERNATE:
> > >> +		/* Use VIN hardware to combine the two fields */
> > >> +		fmt.format.field = V4L2_FIELD_INTERLACED;
> > >> +		fmt.format.height *= 2;
> > >> +		break;
> > > 
> > > I don't think this is right. If V4L2_FIELD_ALTERNATE isn't supported it
> > > should be rejected in the set format handler, or rather this logic should
> > > be moved there. It doesn't belong here, rvin_get_sd_format() should only
> > > be called with a validated and supported field.
> > 
> > I might misunderstand you here, fmt.format.field comes from a the
> > subdevice, just above this:
> > 
> >     struct v4l2_subdev_format fmt = {
> > 	    .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > 	    .pad = vin->digital->source_pad,
> >     };
> >     int ret;
> > 
> >     ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
> >     if (ret)
> > 	    return ret;
> > 
> >     switch (fmt.format.field) {
> >         ...
> >     }
> > 
> > So the format acted on here is the one from the subdevice, and if it is
> > V4L2_FIELD_ALTERNATE it is supported as a source format, just not for
> > output to user-space.
> > 
> > > Furthermore treating the pix parameter of this function as both input and
> > > output seems very confusing to me. If you want to extend
> > > rvin_get_sd_format() beyond just getting the format from the subdev then
> > > please document the function with kerneldoc, and let's try to make its
> > > API clear.
> > 
> > This comment confuses me, are we looking at the same change? The only
> > reference I have to the pix parameter in rvin_get_sd_format() is just
> > before the function returns and it's:
> > 
> >    v4l2_fill_pix_format(pix, &fmt.format);
> > 
> > So it's only used as an output for this function.
> 
> I had mistakenly read the switch statement as operating on the pix function 
> parameter. My bad, sorry about the noise.

No problem :-)

> 
> However, V4L2_FIELD_ALTERNATE should still be rejected in the set format 
> handler, and I don't think you do so in this patch.
> 
> It looks like the field handling logic needs a rewrite :-)

The V4L2_FIELD_ALTERNATE is rejected in the set format handler, or 
rather if the user tries to request it will get V4L2_FIELD_NONE, see the 
latest change of this patch ;-)

But I will add a comment there to explain why ALTERNATE is rejected.

> 
> > >> +	default:
> > >> +		vin->format.field = V4L2_FIELD_NONE;
> > >> +		break;
> > >> +	}
> > >> +
> > >>  	v4l2_fill_pix_format(pix, &fmt.format);
> > >>  	
> > >>  	return 0;
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas Söderlund



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux