Re: [PATCH 1/2] [media] v4l: tegra: Add NVIDIA Tegra VI driver

[Bryan Wu <pengw-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 08/24/2015 11:30 PM, Hans Verkuil wrote:
> A quick follow-up to Thierry's excellent review:
>
> On 08/25/2015 02:26 AM, Bryan Wu wrote:
>> On 08/21/2015 06:03 AM, Thierry Reding wrote:
>>> On Thu, Aug 20, 2015 at 05:51:39PM -0700, Bryan Wu wrote:
> <snip>
>
>>>> +static void
>>>> +__tegra_channel_try_format(struct tegra_channel *chan, struct v4l2_pix_format *pix,
>>>> +		      const struct tegra_video_format **fmtinfo)
>>>> +{
>>>> +	const struct tegra_video_format *info;
>>>> +	unsigned int min_width;
>>>> +	unsigned int max_width;
>>>> +	unsigned int min_bpl;
>>>> +	unsigned int max_bpl;
>>>> +	unsigned int width;
>>>> +	unsigned int align;
>>>> +	unsigned int bpl;
>>>> +
>>>> +	/* Retrieve format information and select the default format if the
>>>> +	 * requested format isn't supported.
>>>> +	 */
>>>> +	info = tegra_core_get_format_by_fourcc(pix->pixelformat);
>>>> +	if (!info)
>>>> +		info = tegra_core_get_format_by_fourcc(TEGRA_VF_DEF_FOURCC);
>>> Should this not be an error? As far as I can tell this is silently
>>> substituting the default format for the requested one if the requested
>>> one isn't supported. Isn't the whole point of this to find out if some
>>> format is supported?
>>>
>> I think it should return some error and escape following code. I will
>> fix that.
> Actually, this code is according to the V4L2 spec: if the given format is
> not supported, then VIDIOC_TRY_FMT should replace it with a valid default
> format.
>
> The reality is a bit more complex: in many drivers this was never reviewed
> correctly and we ended up with some drivers that return an error for this
> case and some drivers that follow the spec. Historically TV capture drivers
> return an error, webcam drivers don't. Most unfortunate.
>
> Since this driver is much more likely to be used with sensors I would
> follow the spec here and substitute an invalid format with a default
> format.
>
>

Thanks for letting me know this. It's actually quite confusing since I 
looked at several drivers, some of them return error some of them use 
default format.

-Bryan