On Tue, Aug 25, 2015 at 08:30:41AM +0200, 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.
Okay, sounds good to me.
Thierry
Attachment:
signature.asc
Description: PGP signature
