Hi Laurent,
On Thu, Dec 01, 2011 at 12:26:07AM +0100, Laurent Pinchart wrote:
> On Wednesday 30 November 2011 09:35:38 Sakari Ailus wrote:
> > Laurent Pinchart wrote:
> > > On Monday 28 November 2011 17:01:12 Sakari Ailus wrote:
> > >> On Mon, Nov 28, 2011 at 12:37:34PM +0100, Laurent Pinchart wrote:
> > >>> When mapping from a V4L2 pixel format to a media bus format in the
> > >>> VIDIOC_TRY_FMT and VIDIOC_S_FMT handlers, the requested format may be
> > >>> unsupported by the driver. Return a hardcoded format instead of
> > >>> WARN()ing in that case.
> > >>>
> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > >>> ---
> > >>>
> > >>> drivers/media/video/omap3isp/ispvideo.c | 8 ++++----
> > >>> 1 files changed, 4 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/drivers/media/video/omap3isp/ispvideo.c
> > >>> b/drivers/media/video/omap3isp/ispvideo.c index d100072..ffe7ce9 100644
> > >>> --- a/drivers/media/video/omap3isp/ispvideo.c
> > >>> +++ b/drivers/media/video/omap3isp/ispvideo.c
> > >>> @@ -210,14 +210,14 @@ static void isp_video_pix_to_mbus(const struct
> > >>> v4l2_pix_format *pix,
> > >>>
> > >>> mbus->width = pix->width;
> > >>> mbus->height = pix->height;
> > >>>
> > >>> - for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> > >>> + /* Skip the last format in the loop so that it will be selected if
> no
> > >>> + * match is found.
> > >>> + */
> > >>> + for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
> > >>>
> > >>> if (formats[i].pixelformat == pix->pixelformat)
> > >>>
> > >>> break;
> > >>>
> > >>> }
> > >>>
> > >>> - if (WARN_ON(i == ARRAY_SIZE(formats)))
> > >>> - return;
> > >>> -
> > >>>
> > >>> mbus->code = formats[i].code;
> > >>> mbus->colorspace = pix->colorspace;
> > >>> mbus->field = pix->field;
> > >>
> > >> In case of setting or trying an invalid format, instead of selecting a
> > >> default format, shouldn't we leave the format unchanced --- the current
> > >> setting is valid after all.
> > >
> > > TRY/SET operations must succeed. The format we select when an invalid
> > > format is requested isn't specified. We could keep the current format,
> > > but wouldn't that be more confusing for applications ? The format they
> > > would get in response to a TRY/SET operation would then potentially
> > > depend on the previous SET operations.
> >
> > I don't think a change to something that has nothing to do what was
> > requested is better than not changing it. The application has requested
> > a particular format; changing it to something else isn't useful for the
> > application. And if the application would try more than invalid format
> > in a row, they both would yield to the same default format.
> >
> > I would personally not change it.
>
> I can agree with you for S_FMT, but I have more doubts about TRY_FMT. Making
> TRY_FMT return the current format if the requested format is not supported
> seems confusing to me. And if we make TRY_FMT return a fixed format in that
> case, why not making S_FMT do the same ? :-)
I'd rather have it the other way around. :-)
Hans; what do you think? (Cc Hans.)
> > What I can find in the spec is this:
> >
> > "When the application calls the VIDIOC_S_FMT ioctl with a pointer to a
> > v4l2_format structure the driver checks and adjusts the parameters
> > against hardware abilities."
> >
> > I wonder how other drivers behave.
>
> uvcvideo returns -EINVAL, which I think should be fixed.
>
> The sensor drivers I wrote return a fixed format (this isn't strictly
> S_FMT/TRY_FMT, but I think it's related).
For the mbus format it's a little bit different: if the format is something
else than what the user asked for, chances are high there's no use for it.
Cheers,
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx
--
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
