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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
