cron job: media_tree daily build: OK
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Fri Aug 17 05:00:14 CEST 2018 media-tree git hash:da2048b7348a0be92f706ac019e022139e29495e media_build git hash: baf45935ffad914f33faf751ad9f4d0dd276c021 v4l-utils git hash: 02a260d3b307c61798a97c0c04ce2e7d862b1347 edid-decode git hash: b2da1516df3cc2756bfe8d1fa06d7bf2562ba1f4 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.16.0-1-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-2.6.36.4-i686: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-i686: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-i686: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-i686: OK linux-2.6.39.4-x86_64: OK linux-3.0.101-i686: OK linux-3.0.101-x86_64: OK linux-3.1.10-i686: OK linux-3.1.10-x86_64: OK linux-3.2.102-i686: OK linux-3.2.102-x86_64: OK linux-3.3.8-i686: OK linux-3.3.8-x86_64: OK linux-3.4.113-i686: OK linux-3.4.113-x86_64: OK linux-3.5.7-i686: OK linux-3.5.7-x86_64: OK linux-3.6.11-i686: OK linux-3.6.11-x86_64: OK linux-3.7.10-i686: OK linux-3.7.10-x86_64: OK linux-3.8.13-i686: OK linux-3.8.13-x86_64: OK linux-3.9.11-i686: OK linux-3.9.11-x86_64: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.57-i686: OK linux-3.16.57-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.115-i686: OK linux-3.18.115-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.18-i686: OK linux-4.18-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [PATCH v2 5/6] media: Add controls for jpeg quantization tables
Hi Ezequiel,
On Fri, Aug 3, 2018 at 5:00 AM Ezequiel Garcia wrote:
>
> From: Shunqian Zheng
>
> Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace
> configure the JPEG quantization tables.
>
> Signed-off-by: Shunqian Zheng
> Signed-off-by: Ezequiel Garcia
> ---
> Documentation/media/uapi/v4l/extended-controls.rst | 9 +
> drivers/media/v4l2-core/v4l2-ctrls.c | 4
> include/uapi/linux/v4l2-controls.h | 3 +++
> 3 files changed, 16 insertions(+)
Thanks for this series and sorry for being late with review. Please
see my comments inline.
>
> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst
> b/Documentation/media/uapi/v4l/extended-controls.rst
> index 9f7312bf3365..80e26f81900b 100644
> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> @@ -3354,6 +3354,15 @@ JPEG Control IDs
> Specify which JPEG markers are included in compressed stream. This
> control is valid only for encoders.
>
> +.. _jpeg-quant-tables-control:
> +
> +``V4L2_CID_JPEG_LUMA_QUANTIZATION (__u8 matrix)``
> +Sets the luma quantization table to be used for encoding
> +or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. This table is
> +expected to be in JPEG zigzag order, as per the JPEG specification.
Should we also specify this to be 8x8?
> +
> +``V4L2_CID_JPEG_CHROMA_QUANTIZATION (__u8 matrix)``
> +Sets the chroma quantization table.
>
nit: I guess we aff something like
"See also V4L2_CID_JPEG_LUMA_QUANTIZATION for details."
to avoid repeating the V4L2_PIX_FMT_JPEG_RAW and zigzag order bits? Or
maybe just repeating is better?
>
> .. flat-table::
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 599c1cbff3b9..5c62c3101851 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -999,6 +999,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_JPEG_RESTART_INTERVAL:return "Restart Interval";
> case V4L2_CID_JPEG_COMPRESSION_QUALITY: return "Compression Quality";
> case V4L2_CID_JPEG_ACTIVE_MARKER: return "Active Markers";
> + case V4L2_CID_JPEG_LUMA_QUANTIZATION: return "Luminance
> Quantization Matrix";
> + case V4L2_CID_JPEG_CHROMA_QUANTIZATION: return "Chrominance
> Quantization Matrix";
>
> /* Image source controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1284,6 +1286,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type,
> *flags |= V4L2_CTRL_FLAG_READ_ONLY;
> break;
> case V4L2_CID_DETECT_MD_REGION_GRID:
> + case V4L2_CID_JPEG_LUMA_QUANTIZATION:
> + case V4L2_CID_JPEG_CHROMA_QUANTIZATION:
It looks like with this setup, the driver has to explicitly set dims
to { 8, 8 } and min/max to 0/255.
At least for min and max, we could set them here. For dims, i don't
see it handled in generic code, so I guess we can leave it to the
driver now and add move into generic code, if another driver shows up.
Hans, what do you think?
Best regards,
Tomasz
[no subject]
-- Greetings, It's my pleasure to introduce my self via this means. I am Moise Kabila the son of the late Democratic Republic of Congo President Laurent Desire Kabila of the blessed memory. I know this letter mightcome to you as a surprise but I honestly do not intend to I write This Letter in respect of my intention to invest the sum of US$12 M (Twelve Million United State Dollars)with you. Once I get your responds, I will flourish you with details on how to go about this transaction. Best Regards, Moise Kabila (for the family)
Re: [PATCH v2 5/5] media: ov5640: fix restore of last mode set
Hi Hugues,
On Thu, Aug 16, 2018 at 03:07:54PM +, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> On 08/16/2018 12:10 PM, jacopo mondi wrote:
> > Hi Hugues,
> > thanks for the patch
> >
> > On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
> >> Mode setting depends on last mode set, in particular
> >> because of exposure calculation when downscale mode
> >> change between subsampling and scaling.
> >> At stream on the last mode was wrongly set to current mode,
> >> so no change was detected and exposure calculation
> >> was not made, fix this.
> >
> > I actually see a different issue here...
>
> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA
> YUYV ?
>
Not a matter of image format but sizes. I printed out the format
applied and it seems to me it was always the initial one.
On a second thought, a pipeline with a mis-configuration would not
have ever started streaming, so I should have investigated better.
> >
> > The issue I see here depends on the format programmed through
> > set_fmt() never being applied when using the sensor with a media
> > controller equipped device (in this case an i.MX6 board) through
> > capture sessions, and the not properly calculated exposure you see may
> > be a consequence of this.
> >
> > I'll try to write down what I see, with the help of some debug output.
> >
> > - At probe time mode 640x460@30 is programmed:
> >[1.651216] ov5640_probe: Initial mode with id: 2
> >
> > - I set the format on the sensor's pad and it gets not applied but
> >marked as pending as the sensor is powered off:
> >
> >#media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]"
> > [ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
>
> So here sensor->current_mode is set to <1>;//QVGA
> and sensor->pending_mode_change is set to true;
>
> >
> > - I start streaming with yavta, and the sensor receives a power on;
> >this causes the 'initial' format to be re-programmed and the pending
> >change to be ignored:
> >
> >#yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4
> > [ 69.395018] ov5640_set_power:1805 - on
> > [ 69.431342] ov5640_restore_mode:1711
> > [ 69.996882] ov5640_set_mode: Apply mode with id: 0
> >
> >The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
> >sensor->pending flag, discarding the newly requested format, for
> >this reason, at s_stream() time, the pending flag is not set
> >anymore.
>
> OK but before clearing sensor->pending_mode_change, set_mode() is
> loading registers corresponding to sensor->current_mode:
> static int ov5640_set_mode(struct ov5640_dev *sensor,
> const struct ov5640_mode_info *orig_mode)
> {
> ==> const struct ov5640_mode_info *mode = sensor->current_mode;
> ...
> ret = ov5640_set_mode_direct(sensor, mode, exposure);
>
> => so mode <1> is expected to be set now, so I don't understand your trace:
> "> [ 69.996882] ov5640_set_mode: Apply mode with id: 0"
> Which variable do you trace that shows "0" ?
>
>
> >
> > Are you using a media-controller system? I suspect in non-mc cases,
> > the set_fmt is applied through a single power_on/power_off session, not
> > causing the 'restore_mode()' issue. Is this the case for you or your
> > issue is differnt?
> >
> > Edit:
> > Mita-san tried to address the issue of the output pixel format not
> > being restored when the image format was restored in
> > 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
> >
> > I understand the issue he tried to fix, but shouldn't the pending
> > format (if any) be applied instead of the initial one unconditionally?
>
> This is what does the ov5640_restore_mode(), set the current mode
> (sensor->current_mode), that is done through this line:
> /* now restore the last capture mode */
> ret = ov5640_set_mode(sensor, _mode_init_data);
> => note that the comment above is weird, in fact it is the "current"
> mode that is set.
> => note also that the 2nd parameter is not the mode to be set but the
> previously applied mode ! (ie loaded in ov5640 registers). This is used
> to decide if we have to go to the "set_mode_exposure_calc" or
> "set_mode_direct".
This is where I got lost... Sorry for the sloppy comment, now I get
what your patch was for :)
>
> the ov5640_restore_mode() also set the current pixel format
> (sensor->fmt), that is done through this line:
> return ov5640_set_framefmt(sensor, >fmt);
> ==> This is what have fixed Mita-san, this line was missing previously,
> leading to "mode registers" being loaded but not the "pixel format
> registers".
>
>
> PS: There are two other "set mode" related changes that are related to this:
> 1) 6949d864776e ("media: ov5640: do not change mode if format or frame
> interval is unchanged")
> => this is merged in media master, unfortunately I've introduced a
> regression on "pixel format" side that I've fixed in this
Re: [PATCH] uvcvideo: add quirk to force Phytec CAM 004H to GBRG
Hi Christoph,
(Philipp, there's a question for you at the end)
On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote:
> > On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote:
> >> On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote:
> > drivers/media/usb/uvc/uvc_driver.c | 16
> > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > 2 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device
> > *dev,
> > width_multiplier = 2;
> > }
> > }
> > + if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
> > + if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
> > + strlcpy(format->name, "GBRG Bayer
> > (GBRG)",
> > + sizeof(format->name));
> > + format->fcc = V4L2_PIX_FMT_SGBRG8;
> > + }
> > + }
> >
> > if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
> > ftype = UVC_VS_FRAME_UNCOMPRESSED;
> > @@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
> > .bInterfaceClass = USB_CLASS_VENDOR_SPEC,
> > .bInterfaceSubClass = 1,
> > .bInterfaceProtocol = 0 },
> > + /* PHYTEC CAM 004H cameras */
> > + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > + | USB_DEVICE_ID_MATCH_INT_INFO,
> > + .idVendor = 0x199e,
> > + .idProduct= 0x8302,
> > + .bInterfaceClass = USB_CLASS_VIDEO,
> > + .bInterfaceSubClass = 1,
> > + .bInterfaceProtocol = 0,
> > + .driver_info = UVC_QUIRK_FORCE_GBRG },
> > /* Bodelin ProScopeHR */
> > { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > | USB_DEVICE_ID_MATCH_DEV_HI
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h
> > b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -164,6 +164,7 @@
> > #define UVC_QUIRK_RESTRICT_FRAME_RATE 0x0200
> > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT0x0400
> > #define UVC_QUIRK_FORCE_Y8 0x0800
> > +#define UVC_QUIRK_FORCE_GBRG 0x1000
>
> I don't think we should add a quirk flag for every format that needs
> to be forced. Instead, now that we have a new way to store per-device
> parameters since commit 3bc85817d798 ("media: uvcvideo: Add
> extensible device information"), how about making use of it and adding
> a field to the uvc_device_info structure to store the forced format ?
>
> you mean something like:
>
> struct uvc_device_info {
> u32 quirks;
> + u32 forced_color_format;
> u32 meta_format;
> };
>
> and
>
> +static const struct uvc_device_info uvc_forced_color_sgbrg8 = {
> + .forced_color_format = V4L2_PIX_FMT_SGBRG8,
> +};
>
> and
>
> @@ -2817,7 +2820,7 @@ static const struct usb_device_id uvc_ids[] = {
> .bInterfaceClass = USB_CLASS_VENDOR_SPEC,
> .bInterfaceSubClass = 1,
> .bInterfaceProtocol = 0,
> - .driver_info = (kernel_ulong_t)_quirk_force_y8 },
> + .driver_info = (kernel_ulong_t)_forced_color_y8 },
>
> ?
With an additional
static const struct uvc_device_info uvc_forced_color_y8 = {
.forced_color_format = V4L2_PIX_FMT_GREY,
};
> If yes:
>
> - there would be a need for forced_color_format in struct uvc_device
Why so ?
> - module-parameter quirk would not test force color format any more
> - the actual force/quirk changes not only format->fcc:
>
> if (dev->forced_color_format == V4L2_PIX_FMT_SGBRG8) {
The test should be if (dev->forced_color_format) to cover both the Y8 and
SGBRG8 cases.
> strlcpy(format->name, "Greyscale 8-bit (Y8 )",
> sizeof(format->name));
You can get the name from the uvc_fmts entry corresponding to dev-
>forced_color_format.
> format->fcc = dev->forced_color_format;
> format->bpp = 8;
> width_multiplier = 2;
bpp and multiplier are more annoying. bpp is a property of the format, which
we could add to the uvc_fmts array.
I believe the multiplier could be computed by
Re: [PATCH v2 5/5] media: ov5640: fix restore of last mode set
Hi Jacopo,
On 08/16/2018 12:10 PM, jacopo mondi wrote:
> Hi Hugues,
> thanks for the patch
>
> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
>> Mode setting depends on last mode set, in particular
>> because of exposure calculation when downscale mode
>> change between subsampling and scaling.
>> At stream on the last mode was wrongly set to current mode,
>> so no change was detected and exposure calculation
>> was not made, fix this.
>
> I actually see a different issue here...
Which problem do you have exactly, you got a VGA JPEG instead of a QVGA
YUYV ?
>
> The issue I see here depends on the format programmed through
> set_fmt() never being applied when using the sensor with a media
> controller equipped device (in this case an i.MX6 board) through
> capture sessions, and the not properly calculated exposure you see may
> be a consequence of this.
>
> I'll try to write down what I see, with the help of some debug output.
>
> - At probe time mode 640x460@30 is programmed:
>[1.651216] ov5640_probe: Initial mode with id: 2
>
> - I set the format on the sensor's pad and it gets not applied but
>marked as pending as the sensor is powered off:
>
>#media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]"
> [ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
So here sensor->current_mode is set to <1>;//QVGA
and sensor->pending_mode_change is set to true;
>
> - I start streaming with yavta, and the sensor receives a power on;
>this causes the 'initial' format to be re-programmed and the pending
>change to be ignored:
>
>#yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4
> [ 69.395018] ov5640_set_power:1805 - on
> [ 69.431342] ov5640_restore_mode:1711
> [ 69.996882] ov5640_set_mode: Apply mode with id: 0
>
>The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
>sensor->pending flag, discarding the newly requested format, for
>this reason, at s_stream() time, the pending flag is not set
>anymore.
OK but before clearing sensor->pending_mode_change, set_mode() is
loading registers corresponding to sensor->current_mode:
static int ov5640_set_mode(struct ov5640_dev *sensor,
const struct ov5640_mode_info *orig_mode)
{
==> const struct ov5640_mode_info *mode = sensor->current_mode;
...
ret = ov5640_set_mode_direct(sensor, mode, exposure);
=> so mode <1> is expected to be set now, so I don't understand your trace:
"> [ 69.996882] ov5640_set_mode: Apply mode with id: 0"
Which variable do you trace that shows "0" ?
>
> Are you using a media-controller system? I suspect in non-mc cases,
> the set_fmt is applied through a single power_on/power_off session, not
> causing the 'restore_mode()' issue. Is this the case for you or your
> issue is differnt?
>
> Edit:
> Mita-san tried to address the issue of the output pixel format not
> being restored when the image format was restored in
> 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
>
> I understand the issue he tried to fix, but shouldn't the pending
> format (if any) be applied instead of the initial one unconditionally?
This is what does the ov5640_restore_mode(), set the current mode
(sensor->current_mode), that is done through this line:
/* now restore the last capture mode */
ret = ov5640_set_mode(sensor, _mode_init_data);
=> note that the comment above is weird, in fact it is the "current"
mode that is set.
=> note also that the 2nd parameter is not the mode to be set but the
previously applied mode ! (ie loaded in ov5640 registers). This is used
to decide if we have to go to the "set_mode_exposure_calc" or
"set_mode_direct".
the ov5640_restore_mode() also set the current pixel format
(sensor->fmt), that is done through this line:
return ov5640_set_framefmt(sensor, >fmt);
==> This is what have fixed Mita-san, this line was missing previously,
leading to "mode registers" being loaded but not the "pixel format
registers".
PS: There are two other "set mode" related changes that are related to this:
1) 6949d864776e ("media: ov5640: do not change mode if format or frame
interval is unchanged")
=> this is merged in media master, unfortunately I've introduced a
regression on "pixel format" side that I've fixed in this patchset :
2) https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html
Symptom was a noisy image when capturing QVGA YUV (in fact captured as
JPEG data).
Best regards,
Hugues.
>
> Thanks
> j
>
>>
>> Signed-off-by: Hugues Fruchet
>> ---
>> drivers/media/i2c/ov5640.c | 8 +++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index c110a6a..923cc30 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -225,6 +225,7 @@ struct ov5640_dev {
>> struct
Re: [PATCH 05/21] dt-bindings: media: Specify bus type for MIPI D-PHY, others, explicitly
Hi Rob, On Thu, Aug 16, 2018 at 07:48:24AM -0600, Rob Herring wrote: > On Thu, Aug 16, 2018 at 3:17 AM Sakari Ailus > wrote: > > > > Ping? > > > > On Wed, Aug 01, 2018 at 02:16:27PM +0300, Sakari Ailus wrote: > > > Hi Rob, > > > > > > Thanks for the review. > > > > > > On Tue, Jul 31, 2018 at 03:32:10PM -0600, Rob Herring wrote: > > > > On Mon, Jul 23, 2018 at 04:46:50PM +0300, Sakari Ailus wrote: > > > > > Allow specifying the bus type explicitly for MIPI D-PHY, parallel and > > > > > Bt.656 busses. This is useful for devices that can make use of > > > > > different > > > > > bus types. There are CSI-2 transmitters and receivers but the PHY > > > > > selection needs to be made between C-PHY and D-PHY; many devices also > > > > > support parallel and Bt.656 interfaces but the means to pass that > > > > > information to software wasn't there. > > > > > > > > > > Autodetection (value 0) is removed as an option as the property could > > > > > be > > > > > simply omitted in that case. > > > > > > > > Presumably there are users, so you can't remove it. But documenting > > > > behavior when absent would be good. > > > > > > Well, it's effectively the same as having no such property at all: the > > > type > > > is not specified. Generally there are two possibilities: the hardware > > > supports just a single bus or it supports more than one. If there's just > > > one, the type can be known by the driver. In that case there's no use for > > > autodetection. > > > > > > The second case is a bit more complicated: the bus type detection is > > > solely > > > based on properties available in the endpoint, and I think that may have > > > been feasible approach when there were just parallel and Bt.656 busses > > > that > > > were supported, but with the additional busses, the V4L2 fwnode framework > > > may no longer guess the bus in any meaningful way from the available > > > properties. I'd think the only known-good option here is to specify the > > > type explicitly in that case: there's no room for guessing. (This patchset > > > makes it possible for drivers to explicitly define the bus type, but the > > > autodetection support is maintained for backwards compatibility.) > > > > > > One of the existing issues is that there are combined parallel/Bt.656 > > > receivers that need to know the type of the bus. This is based on the > > > existence parallel interface only properties: if any of these exist, then > > > the interface is parallel, otherwise it is Bt.656. The DT bindings for the > > > same devices also define the defaults for the parallel interface. This > > > leaves the end result ambiguous: is it the parallel interface with the > > > default configuration or is it Bt.656? > > > > > > There will likely be similar issues for CSI-2 D-PHY and CSI-2 C-PHY. The > > > question there would be: is this CSI-2 C-PHY or CSI-2 D-PHY with default > > > clock lane configuration? > > > > > > In either case the autodetection option for the bus type provides no > > > useful > > > information. If it exists in DT source, that's fine, there's just no use > > > for it. > > > > > > Let me know if you still think it should be maintained in binding > > > documentation. > > > > If you prefer to keep it, I'd propose to mark it as deprecated or something > > as it provides no information to software. > > Looks like there's only one user in tree: > > arch/arm/boot/dts/omap3-n900.dts: > bus-type = <3>; /* CCP2 */ > arch/arm/boot/dts/omap3-n900.dts: > bus-type = <3>; /* CCP2 */ > > So I guess removing is okay. Note that we're only discussing the value 0 --- autodetection. The rest of the values are good as they are, and indeed, necessary for unambiguous parsing of the endpoint configuration in cases where the hardware supports multiple bus types. -- Sakari Ailus sakari.ai...@linux.intel.com
[RFP] Media Summit: Complex cameras
I expect that we could have something to discuss there about complex cameras. So, I'd reserve a 50 mins slot for it. The idea is to discuss about the undergoing work with complex camera development is happening. As we're working to merge request API, another topic for discussion is how to add support for requests on it (or on a separate but related library). Thanks, Mauro
Re: [PATCH 05/21] dt-bindings: media: Specify bus type for MIPI D-PHY, others, explicitly
On Thu, Aug 16, 2018 at 3:17 AM Sakari Ailus wrote: > > Ping? > > On Wed, Aug 01, 2018 at 02:16:27PM +0300, Sakari Ailus wrote: > > Hi Rob, > > > > Thanks for the review. > > > > On Tue, Jul 31, 2018 at 03:32:10PM -0600, Rob Herring wrote: > > > On Mon, Jul 23, 2018 at 04:46:50PM +0300, Sakari Ailus wrote: > > > > Allow specifying the bus type explicitly for MIPI D-PHY, parallel and > > > > Bt.656 busses. This is useful for devices that can make use of different > > > > bus types. There are CSI-2 transmitters and receivers but the PHY > > > > selection needs to be made between C-PHY and D-PHY; many devices also > > > > support parallel and Bt.656 interfaces but the means to pass that > > > > information to software wasn't there. > > > > > > > > Autodetection (value 0) is removed as an option as the property could be > > > > simply omitted in that case. > > > > > > Presumably there are users, so you can't remove it. But documenting > > > behavior when absent would be good. > > > > Well, it's effectively the same as having no such property at all: the type > > is not specified. Generally there are two possibilities: the hardware > > supports just a single bus or it supports more than one. If there's just > > one, the type can be known by the driver. In that case there's no use for > > autodetection. > > > > The second case is a bit more complicated: the bus type detection is solely > > based on properties available in the endpoint, and I think that may have > > been feasible approach when there were just parallel and Bt.656 busses that > > were supported, but with the additional busses, the V4L2 fwnode framework > > may no longer guess the bus in any meaningful way from the available > > properties. I'd think the only known-good option here is to specify the > > type explicitly in that case: there's no room for guessing. (This patchset > > makes it possible for drivers to explicitly define the bus type, but the > > autodetection support is maintained for backwards compatibility.) > > > > One of the existing issues is that there are combined parallel/Bt.656 > > receivers that need to know the type of the bus. This is based on the > > existence parallel interface only properties: if any of these exist, then > > the interface is parallel, otherwise it is Bt.656. The DT bindings for the > > same devices also define the defaults for the parallel interface. This > > leaves the end result ambiguous: is it the parallel interface with the > > default configuration or is it Bt.656? > > > > There will likely be similar issues for CSI-2 D-PHY and CSI-2 C-PHY. The > > question there would be: is this CSI-2 C-PHY or CSI-2 D-PHY with default > > clock lane configuration? > > > > In either case the autodetection option for the bus type provides no useful > > information. If it exists in DT source, that's fine, there's just no use > > for it. > > > > Let me know if you still think it should be maintained in binding > > documentation. > > If you prefer to keep it, I'd propose to mark it as deprecated or something > as it provides no information to software. Looks like there's only one user in tree: arch/arm/boot/dts/omap3-n900.dts: bus-type = <3>; /* CCP2 */ arch/arm/boot/dts/omap3-n900.dts: bus-type = <3>; /* CCP2 */ So I guess removing is okay. Rob
[RFP] Media Summit: CEC Status Update
A 15 minute (max) update of the HDMI CEC subsystem status. Regards, Hans
Re: [PATCH] uvcvideo: add quirk to force Phytec CAM 004H to GBRG
Hi Laurent
> On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote:
> > On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote:
> > >>> drivers/media/usb/uvc/uvc_driver.c | 16
> > >>> drivers/media/usb/uvc/uvcvideo.h | 1 +
> > >>> 2 files changed, 17 insertions(+)
> > >>>
> > >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > >>> b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644
> > >>> --- a/drivers/media/usb/uvc/uvc_driver.c
> > >>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> > >>> @@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device
> > >>> *dev,
> > >>> width_multiplier = 2;
> > >>> }
> > >>> }
> > >>> + if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
> > >>> + if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
> > >>> + strlcpy(format->name, "GBRG Bayer
> > >>> (GBRG)",
> > >>> + sizeof(format->name));
> > >>> + format->fcc = V4L2_PIX_FMT_SGBRG8;
> > >>> + }
> > >>> + }
> > >>>
> > >>> if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
> > >>> ftype = UVC_VS_FRAME_UNCOMPRESSED;
> > >>> @@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
> > >>> .bInterfaceClass = USB_CLASS_VENDOR_SPEC,
> > >>> .bInterfaceSubClass = 1,
> > >>> .bInterfaceProtocol = 0 },
> > >>> + /* PHYTEC CAM 004H cameras */
> > >>> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > >>> + | USB_DEVICE_ID_MATCH_INT_INFO,
> > >>> + .idVendor = 0x199e,
> > >>> + .idProduct= 0x8302,
> > >>> + .bInterfaceClass = USB_CLASS_VIDEO,
> > >>> + .bInterfaceSubClass = 1,
> > >>> + .bInterfaceProtocol = 0,
> > >>> + .driver_info = UVC_QUIRK_FORCE_GBRG },
> > >>> /* Bodelin ProScopeHR */
> > >>> { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > >>> | USB_DEVICE_ID_MATCH_DEV_HI
> > >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> > >>> b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644
> > >>> --- a/drivers/media/usb/uvc/uvcvideo.h
> > >>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> > >>> @@ -164,6 +164,7 @@
> > >>> #define UVC_QUIRK_RESTRICT_FRAME_RATE 0x0200
> > >>> #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT0x0400
> > >>> #define UVC_QUIRK_FORCE_Y8 0x0800
> > >>> +#define UVC_QUIRK_FORCE_GBRG 0x1000
> > >>
> > >> I don't think we should add a quirk flag for every format that needs to
> > >> be forced. Instead, now that we have a new way to store per-device
> > >> parameters since commit 3bc85817d798 ("media: uvcvideo: Add extensible
> > >> device information"), how about making use of it and adding a field to
> > >> the uvc_device_info structure to store the forced format ?
you mean something like:
struct uvc_device_info {
u32 quirks;
+ u32 forced_color_format;
u32 meta_format;
};
and
+static const struct uvc_device_info uvc_forced_color_sgbrg8 = {
+ .forced_color_format = V4L2_PIX_FMT_SGBRG8,
+};
and
@@ -2817,7 +2820,7 @@ static const struct usb_device_id uvc_ids[] = {
.bInterfaceClass = USB_CLASS_VENDOR_SPEC,
.bInterfaceSubClass = 1,
.bInterfaceProtocol = 0,
- .driver_info = (kernel_ulong_t)_quirk_force_y8 },
+ .driver_info = (kernel_ulong_t)_forced_color_y8 },
?
If yes:
- there would be a need for forced_color_format in struct uvc_device
- module-parameter quirk would not test force color format any more
- the actual force/quirk changes not only format->fcc:
if (dev->forced_color_format == V4L2_PIX_FMT_SGBRG8) {
strlcpy(format->name, "Greyscale 8-bit (Y8 )",
sizeof(format->name));
format->fcc = dev->forced_color_format;
format->bpp = 8;
width_multiplier = 2;
}
Is this the way you want me to go?
Thanks
-- Christoph
Re: [RFC] Request API questions
Em Thu, 16 Aug 2018 12:25:25 +0200
Hans Verkuil escreveu:
> Laurent raised a few API issues/questions in his review of the documentation.
>
> I've consolidated those in this RFC. I would like to know what others think
> and if I should make changes.
>
> 1) Should you be allowed to set controls directly if they are also used in
>requests? Right now this is allowed, although we warn in the spec that
>this can lead to undefined behavior.
>
>In my experience being able to do this is very useful while testing,
>and restricting this is not all that easy to implement. I also think it is
>not our job. It is not as if something will break when you do this.
>
>If there really is a good reason why you can't mix this for a specific
>control, then the driver can check this and return -EBUSY.
IMHO, there's not much sense on preventing it. Just having a warning
at the spec is enough.
+.. caution::
+
+ Setting the same control through a request and also directly can lead to
+ undefined behavior!
It is already warned with a caution. Anyone that decides to ignore a
warning like that will deserve his faith if things stop work.
>
> 2) If request_fd in QBUF or the control ioctls is not a request fd, then we
>now return ENOENT. Laurent suggests using EBADR ('Invalid request
> descriptor')
>instead. This seems like a good idea to me. Should I change this?
I don't have a strong opinion, but EBADR value seems to be arch-dependent:
arch/alpha/include/uapi/asm/errno.h:#define EBADR 98 /*
Invalid request descriptor */
arch/mips/include/uapi/asm/errno.h:#define EBADR51 /*
Invalid request descriptor */
arch/parisc/include/uapi/asm/errno.h:#defineEBADR 161 /*
Invalid request descriptor */
arch/sparc/include/uapi/asm/errno.h:#define EBADR 103 /*
Invalid request descriptor */
include/uapi/asm-generic/errno.h:#defineEBADR 53 /*
Invalid request descriptor */
Also, just because its name says "invalid request", it doesn't mean that it
is the right error code. In this specific case, we're talking about a file
descriptor. Invalid file descriptors is something that the FS subsystem
has already a defined set of return codes. We should stick with whatever
FS uses when a file descriptor is invalid.
Where the VFS code returns EBADR? Does it make sense for our use cases?
>
> 3) Calling VIDIOC_G_EXT_CTRLS for a request that has not been queued yet will
>return either the value of the control you set earlier in the request, or
>the current HW control value if it was never set in the request.
>
>I believe it makes sense to return what was set in the request previously
>(if you set it, you should be able to get it), but it is an idea to return
>ENOENT when calling this for controls that are NOT in the request.
>
>I'm inclined to implement that. Opinions?
Return the request "cached" value, IMO, doesn't make sense. If the
application needs such cache, it can implement itself.
Return an error code if the request has not yet completed makes
sense. Not sure what would be the best error code here... if the
request is queued already (but not processed), EBUSY seems to be the
better choice, but, if it was not queued yet, I'm not sure. I guess
ENOENT would work.
>
> 4) When queueing a buffer to a request with VIDIOC_QBUF you set
> V4L2_BUF_FLAG_REQUEST_FD
>to indicate a valid request_fd. For other queue ioctls that take a struct
> v4l2_buffer
>this flag and the request_fd field are just ignored. Should we return
> EINVAL
>instead if the flag is set for those ioctls?
>
>The argument for just ignoring it is that older kernels that do not know
> about
>this flag will ignore it as well. There is no check against unknown flags.
As I answered before, I don't see any need to add extra code for checking
invalid
flags.
It might make sense to ask users to clean the flag if not QBUF, just at the
eventual remote case we might want to use it on other ioctls.
Thanks,
Mauro
Re: [PATCH v2 1/4] media: soc_camera: ov9640: move ov9640 out of soc_camera
Dne 16.8.2018 v 12:23 jacopo mondi napsal(a): > Hi Petr, > > On Wed, Aug 15, 2018 at 03:35:39PM +0200, Petr Cvek wrote: >> BTW from the v1 discussion: >> >> Dne 10.8.2018 v 09:32 jacopo mondi napsal(a): >>> When I've been recently doing the same for ov772x and other sensor >>> driver I've been suggested to first copy the driver into >>> drivers/media/i2c/ and leave the original soc_camera one there, so >>> they can be bulk removed or moved to staging. I'll let Hans confirm >>> this, as he's about to take care of this process. >> >> I would rather used git mv for preserve the git history, but if a simple >> copy is fine then I'm fine too ;-). > > Well, 'git mv' removes the soc_camera version of this driver, and my > understanding was that when those driver will be obsoleted they will > be bulk removed or moved to staging by Hans. > Nevermind I've probably used the wrong command :-/ Petr
Re: [PATCHv18 01/35] Documentation: v4l: document request API
Em Thu, 16 Aug 2018 11:58:09 +0200 Hans Verkuil escreveu: > >> This is set by the user when > >> calling > >> + :ref:`ioctl VIDIOC_QBUF ` and ignored by other ioctls. > > > > Shouldn't other ioctls return an error when V4L2_BUF_FLAG_REQUEST_FD is set > > ? > > Should they? I'd like to know what others think. I don't see any reason why to add an extra validation logic checking if V4L2_BUF_FLAG_REQUEST_FD is set just to return an error. It might make sense to ask apps to not set it on other ioctls, just in eventual case we might want to use this flag there (although I don't foresee any such usage). Thanks, Mauro
[RFC] Request API questions
Laurent raised a few API issues/questions in his review of the documentation.
I've consolidated those in this RFC. I would like to know what others think
and if I should make changes.
1) Should you be allowed to set controls directly if they are also used in
requests? Right now this is allowed, although we warn in the spec that
this can lead to undefined behavior.
In my experience being able to do this is very useful while testing,
and restricting this is not all that easy to implement. I also think it is
not our job. It is not as if something will break when you do this.
If there really is a good reason why you can't mix this for a specific
control, then the driver can check this and return -EBUSY.
2) If request_fd in QBUF or the control ioctls is not a request fd, then we
now return ENOENT. Laurent suggests using EBADR ('Invalid request
descriptor')
instead. This seems like a good idea to me. Should I change this?
3) Calling VIDIOC_G_EXT_CTRLS for a request that has not been queued yet will
return either the value of the control you set earlier in the request, or
the current HW control value if it was never set in the request.
I believe it makes sense to return what was set in the request previously
(if you set it, you should be able to get it), but it is an idea to return
ENOENT when calling this for controls that are NOT in the request.
I'm inclined to implement that. Opinions?
4) When queueing a buffer to a request with VIDIOC_QBUF you set
V4L2_BUF_FLAG_REQUEST_FD
to indicate a valid request_fd. For other queue ioctls that take a struct
v4l2_buffer
this flag and the request_fd field are just ignored. Should we return EINVAL
instead if the flag is set for those ioctls?
The argument for just ignoring it is that older kernels that do not know
about
this flag will ignore it as well. There is no check against unknown flags.
Regards,
Hans
Re: [PATCH v2 1/4] media: soc_camera: ov9640: move ov9640 out of soc_camera
Hi Petr, On Wed, Aug 15, 2018 at 03:35:39PM +0200, Petr Cvek wrote: > BTW from the v1 discussion: > > Dne 10.8.2018 v 09:32 jacopo mondi napsal(a): > > When I've been recently doing the same for ov772x and other sensor > > driver I've been suggested to first copy the driver into > > drivers/media/i2c/ and leave the original soc_camera one there, so > > they can be bulk removed or moved to staging. I'll let Hans confirm > > this, as he's about to take care of this process. > > I would rather used git mv for preserve the git history, but if a simple > copy is fine then I'm fine too ;-). Well, 'git mv' removes the soc_camera version of this driver, and my understanding was that when those driver will be obsoleted they will be bulk removed or moved to staging by Hans. Also, I feel this is trivial and I'm not getting something but: $ git mv drivers/media/i2c/soc_camera/ov9640.c drivers/media/i2c/ $ git commit -s -m "test" $ git log --oneline drivers/media/i2c/ov9640.c | wc -l 1 Hence, I don't see history be preserved (which makes sense, as git mv is actually a shortcut for 'mv $old $new; git add $new; git add $old') Am I missing something maybe? Thanks j > > Petr signature.asc Description: PGP signature
Re: [PATCHv18 01/35] Documentation: v4l: document request API
On 16/08/18 11:58, Hans Verkuil wrote: >>> +On success :c:func:`poll() ` returns the number of file >>> +descriptors that have been selected (that is, file descriptors for which >>> the +``revents`` field of the respective struct :c:type:`pollfd` >>> +is non-zero). Request file descriptor set the ``POLLPRI`` flag in >>> ``revents`` +when the request was completed. When the function times out >>> it returns +a value of zero, on failure it returns -1 and the ``errno`` >>> variable is +set appropriately. >>> + >>> +Attempting to poll for a request that is completed or not yet queued will >>> +set the ``POLLERR`` flag in ``revents``. >> >> Why should a completed request set POLLERR, given that the purpose of poll() >> is to poll for completion ? > > I think you are right. We should just always set POLLPRI for completed > requests. > I'll change that. I checked the code, and this is actually what happens already. So the text should read: "Attempting to poll for a request that is not yet queued will set the ``POLLERR`` flag in ``revents``." I'll update this. Regards, Hans
Re: [PATCH v2 5/5] media: ov5640: fix restore of last mode set
Hi Hugues,
thanks for the patch
On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
> Mode setting depends on last mode set, in particular
> because of exposure calculation when downscale mode
> change between subsampling and scaling.
> At stream on the last mode was wrongly set to current mode,
> so no change was detected and exposure calculation
> was not made, fix this.
I actually see a different issue here...
The issue I see here depends on the format programmed through
set_fmt() never being applied when using the sensor with a media
controller equipped device (in this case an i.MX6 board) through
capture sessions, and the not properly calculated exposure you see may
be a consequence of this.
I'll try to write down what I see, with the help of some debug output.
- At probe time mode 640x460@30 is programmed:
[1.651216] ov5640_probe: Initial mode with id: 2
- I set the format on the sensor's pad and it gets not applied but
marked as pending as the sensor is powered off:
#media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]"
[ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
- I start streaming with yavta, and the sensor receives a power on;
this causes the 'initial' format to be re-programmed and the pending
change to be ignored:
#yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4
[ 69.395018] ov5640_set_power:1805 - on
[ 69.431342] ov5640_restore_mode:1711
[ 69.996882] ov5640_set_mode: Apply mode with id: 0
The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
sensor->pending flag, discarding the newly requested format, for
this reason, at s_stream() time, the pending flag is not set
anymore.
Are you using a media-controller system? I suspect in non-mc cases,
the set_fmt is applied through a single power_on/power_off session, not
causing the 'restore_mode()' issue. Is this the case for you or your
issue is differnt?
Edit:
Mita-san tried to address the issue of the output pixel format not
being restored when the image format was restored in
19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
I understand the issue he tried to fix, but shouldn't the pending
format (if any) be applied instead of the initial one unconditionally?
Thanks
j
>
> Signed-off-by: Hugues Fruchet
> ---
> drivers/media/i2c/ov5640.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index c110a6a..923cc30 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -225,6 +225,7 @@ struct ov5640_dev {
> struct v4l2_mbus_framefmt fmt;
>
> const struct ov5640_mode_info *current_mode;
> + const struct ov5640_mode_info *last_mode;
> enum ov5640_frame_rate current_fr;
> struct v4l2_fract frame_interval;
>
> @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
> int ret;
>
> + if (!orig_mode)
> + orig_mode = mode;
> +
> dn_mode = mode->dn_mode;
> orig_dn_mode = orig_mode->dn_mode;
>
> @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> return ret;
>
> sensor->pending_mode_change = false;
> + sensor->last_mode = mode;
>
> return 0;
>
> @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int
> enable)
>
> if (sensor->streaming == !enable) {
> if (enable && sensor->pending_mode_change) {
> - ret = ov5640_set_mode(sensor, sensor->current_mode);
> + ret = ov5640_set_mode(sensor, sensor->last_mode);
> +
> if (ret)
> goto out;
>
> --
> 2.7.4
>
signature.asc
Description: PGP signature
Re: [PATCHv18 01/35] Documentation: v4l: document request API
Hi Laurent, Thank you very much for your extensive review! On 15/08/18 18:14, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Tuesday, 14 August 2018 17:20:13 EEST Hans Verkuil wrote: >> +Description >> +=== >> + >> +If the media device supports :ref:`requests `, then >> +this ioctl can be used to allocate a request. If it is not supported, then >> +``errno`` is set to ``ENOTTY``. A request is accessed through a file >> descriptor +that is returned in ``*argp``. >> + >> +If the request was successfully allocated, then the request file descriptor >> +can be passed to the :ref:`VIDIOC_QBUF `, >> +:ref:`VIDIOC_G_EXT_CTRLS `, >> +:ref:`VIDIOC_S_EXT_CTRLS ` and >> +:ref:`VIDIOC_TRY_EXT_CTRLS ` ioctls. > > As with the media controller API, the request API isn't V4L2-specific, but > can > be used by subsystems that use the media controller API, such as V4L2. The > above paragraph is correct, but I wouldn't list V4L2-specific ioctls in the > MC > API documentation. Instead, you could state that the request file descriptor > can be passed to subsystem APIs that are request-aware, without detailing > which ones. The documentation of the above V4L2 ioctls will contain detailed > information. There has to a list of ioctls (V4L2 or otherwise) that support requests. I believe it makes sense to keep that in a single place, and since this ioctl is the starting point for using requests I think it makes sense to list them here. It's not V4L2 specific, it's just that only V4L2 ioctls support this at the moment. >> +In addition, the request can be queued by calling >> +:ref:`MEDIA_REQUEST_IOC_QUEUE` and re-initialized by calling >> +:ref:`MEDIA_REQUEST_IOC_REINIT`. >> + >> +Finally, the file descriptor can be :ref:`polled ` to >> wait >> +for the request to complete. >> + >> +The request will remain allocated until all the file descriptors associated >> +with it are closed by :ref:`close() ` and the driver >> no >> +longer uses the request internally. > > I think it would be useful here to refer to the appropriate section of > Documentation/media/uapi/mediactl/request-api.rst for more information about > request life time management. OK. > >> +Return Value >> + >> + >> +On success 0 is returned, on error -1 and the ``errno`` variable is set >> +appropriately. The generic error codes are described at the >> +:ref:`Generic Error Codes ` chapter. >> + >> +ENOTTY >> +The driver has no support for requests. >> diff --git a/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst >> b/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst new file >> mode 100644 >> index ..d4f8119e0643 >> --- /dev/null >> +++ b/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst >> @@ -0,0 +1,82 @@ >> +.. SPDX-License-Identifier: GPL-2.0 OR GFDL-1.1-or-later WITH >> no-invariant-sections >> + >> +.. _media_request_ioc_queue: >> + >> +* >> +ioctl MEDIA_REQUEST_IOC_QUEUE >> +* >> + >> +Name >> + >> + >> +MEDIA_REQUEST_IOC_QUEUE - Queue a request >> + >> + >> +Synopsis >> + >> + >> +.. c:function:: int ioctl( int request_fd, MEDIA_REQUEST_IOC_QUEUE ) >> +:name: MEDIA_REQUEST_IOC_QUEUE >> + >> + >> +Arguments >> += >> + >> +``request_fd`` >> +File descriptor returned by :ref:`MEDIA_IOC_REQUEST_ALLOC`. >> + >> + >> +Description >> +=== >> + >> +If the media device supports :ref:`requests `, then >> +this request ioctl can be used to queue a previously allocated request. >> + >> +If the request was successfully queued, then the file descriptor can be >> +:ref:`polled ` to wait for the request to complete. >> + >> +If the request was already queued before, then ``EBUSY`` is returned. >> +Other errors can be returned if the contents of the request contained >> +invalid or inconsistent data, see the next section for a list of >> +common error codes. On error both the request and driver state are >> unchanged. >> + >> +Typically if you get an error here, then that means that the application >> +did something wrong and you have to fix the application. > > Isn't that always the case ? :-) Hmm, I'm not entirely sure what I intended to say. I'll just drop this. > >> +Once a request is queued, then the driver is required to gracefully handle >> +errors that occur when the request is applied to the hardware. The >> +exception is the ``EIO`` error which signals a fatal error that requires >> +the application to stop streaming to reset the hardware state. > > Queuing a request will validate it synchronously (possibly returning -EINVAL) > and execute it asynchronously. The -EIO error signals a fatal error during > execution of the request, how can it thus be returned from the queue ioctl ? > I > think the API definition is a bit vague in this area, we lack a detailed > documentation of how errors at request execution time are handled. This is the case where
Re: [PATCH v2] media: ov5640: do not change mode if format or frame interval is unchanged
Hi all,
Please ignore this v2, the v1 was merged.
I've just pushed a new patch which fixes the regression observed, see:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html
Sorry for inconvenience.
Best regards,
Hugues.
On 08/13/2018 11:21 AM, Hugues Fruchet wrote:
> Save load of mode registers array when V4L2 client sets a format or a
> frame interval which selects the same mode than the current one.
>
> Signed-off-by: Hugues Fruchet
> ---
> Version 2:
>- Fix fuzzy image because of JPEG default format not being
> changed according to new format selected, fix this.
>- Init sequence initialises format to YUV422 UYVY but
> sensor->fmt initial value was set to JPEG, fix this.
>
>
> drivers/media/i2c/ov5640.c | 33 -
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 1ecbb7a..2ddd86d 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -223,6 +223,7 @@ struct ov5640_dev {
> int power_count;
>
> struct v4l2_mbus_framefmt fmt;
> + bool pending_fmt_change;
>
> const struct ov5640_mode_info *current_mode;
> enum ov5640_frame_rate current_fr;
> @@ -255,7 +256,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct
> v4l2_ctrl *ctrl)
>* should be identified and removed to speed register load time
>* over i2c.
>*/
> -
> +/* YUV422 UYVY VGA@30fps */
> static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> @@ -1966,9 +1967,14 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> goto out;
> }
>
> - sensor->current_mode = new_mode;
> - sensor->fmt = *mbus_fmt;
> - sensor->pending_mode_change = true;
> + if (new_mode != sensor->current_mode) {
> + sensor->current_mode = new_mode;
> + sensor->pending_mode_change = true;
> + }
> + if (mbus_fmt->code != sensor->fmt.code) {
> + sensor->fmt = *mbus_fmt;
> + sensor->pending_fmt_change = true;
> + }
> out:
> mutex_unlock(>lock);
> return ret;
> @@ -2508,8 +2514,10 @@ static int ov5640_s_frame_interval(struct v4l2_subdev
> *sd,
> goto out;
> }
>
> - sensor->current_mode = mode;
> - sensor->pending_mode_change = true;
> + if (mode != sensor->current_mode) {
> + sensor->current_mode = mode;
> + sensor->pending_mode_change = true;
> + }
> out:
> mutex_unlock(>lock);
> return ret;
> @@ -2540,10 +2548,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd,
> int enable)
> ret = ov5640_set_mode(sensor, sensor->current_mode);
> if (ret)
> goto out;
> + }
>
> + if (enable && sensor->pending_fmt_change) {
> ret = ov5640_set_framefmt(sensor, >fmt);
> if (ret)
> goto out;
> + sensor->pending_fmt_change = false;
> }
>
> if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
> @@ -2638,9 +2649,14 @@ static int ov5640_probe(struct i2c_client *client,
> return -ENOMEM;
>
> sensor->i2c_client = client;
> +
> + /*
> + * default init sequence initialize sensor to
> + * YUV422 UYVY VGA@30fps
> + */
> fmt = >fmt;
> - fmt->code = ov5640_formats[0].code;
> - fmt->colorspace = ov5640_formats[0].colorspace;
> + fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
> + fmt->colorspace = V4L2_COLORSPACE_SRGB;
> fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> @@ -2652,7 +2668,6 @@ static int ov5640_probe(struct i2c_client *client,
> sensor->current_fr = OV5640_30_FPS;
> sensor->current_mode =
> _mode_data[OV5640_30_FPS][OV5640_MODE_VGA_640_480];
> - sensor->pending_mode_change = true;
>
> sensor->ae_target = 52;
>
>
[PATCH] media: ov5640: fix mode change regression
fixes: 6949d864776e ("media: ov5640: do not change mode if format or frame
interval is unchanged").
Symptom was fuzzy image because of JPEG default format
not being changed according to new format selected, fix this.
Init sequence initialises format to YUV422 UYVY but
sensor->fmt initial value was set to JPEG, fix this.
Signed-off-by: Hugues Fruchet
---
drivers/media/i2c/ov5640.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 071f4bc..2ddd86d 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -223,6 +223,7 @@ struct ov5640_dev {
int power_count;
struct v4l2_mbus_framefmt fmt;
+ bool pending_fmt_change;
const struct ov5640_mode_info *current_mode;
enum ov5640_frame_rate current_fr;
@@ -255,7 +256,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct
v4l2_ctrl *ctrl)
* should be identified and removed to speed register load time
* over i2c.
*/
-
+/* YUV422 UYVY VGA@30fps */
static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
@@ -1968,9 +1969,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
if (new_mode != sensor->current_mode) {
sensor->current_mode = new_mode;
- sensor->fmt = *mbus_fmt;
sensor->pending_mode_change = true;
}
+ if (mbus_fmt->code != sensor->fmt.code) {
+ sensor->fmt = *mbus_fmt;
+ sensor->pending_fmt_change = true;
+ }
out:
mutex_unlock(>lock);
return ret;
@@ -2544,10 +2548,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int
enable)
ret = ov5640_set_mode(sensor, sensor->current_mode);
if (ret)
goto out;
+ }
+ if (enable && sensor->pending_fmt_change) {
ret = ov5640_set_framefmt(sensor, >fmt);
if (ret)
goto out;
+ sensor->pending_fmt_change = false;
}
if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
@@ -2642,9 +2649,14 @@ static int ov5640_probe(struct i2c_client *client,
return -ENOMEM;
sensor->i2c_client = client;
+
+ /*
+* default init sequence initialize sensor to
+* YUV422 UYVY VGA@30fps
+*/
fmt = >fmt;
- fmt->code = ov5640_formats[0].code;
- fmt->colorspace = ov5640_formats[0].colorspace;
+ fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
+ fmt->colorspace = V4L2_COLORSPACE_SRGB;
fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
@@ -2656,7 +2668,6 @@ static int ov5640_probe(struct i2c_client *client,
sensor->current_fr = OV5640_30_FPS;
sensor->current_mode =
_mode_data[OV5640_30_FPS][OV5640_MODE_VGA_640_480];
- sensor->pending_mode_change = true;
sensor->ae_target = 52;
--
2.7.4
Re: [PATCH 05/21] dt-bindings: media: Specify bus type for MIPI D-PHY, others, explicitly
Ping? On Wed, Aug 01, 2018 at 02:16:27PM +0300, Sakari Ailus wrote: > Hi Rob, > > Thanks for the review. > > On Tue, Jul 31, 2018 at 03:32:10PM -0600, Rob Herring wrote: > > On Mon, Jul 23, 2018 at 04:46:50PM +0300, Sakari Ailus wrote: > > > Allow specifying the bus type explicitly for MIPI D-PHY, parallel and > > > Bt.656 busses. This is useful for devices that can make use of different > > > bus types. There are CSI-2 transmitters and receivers but the PHY > > > selection needs to be made between C-PHY and D-PHY; many devices also > > > support parallel and Bt.656 interfaces but the means to pass that > > > information to software wasn't there. > > > > > > Autodetection (value 0) is removed as an option as the property could be > > > simply omitted in that case. > > > > Presumably there are users, so you can't remove it. But documenting > > behavior when absent would be good. > > Well, it's effectively the same as having no such property at all: the type > is not specified. Generally there are two possibilities: the hardware > supports just a single bus or it supports more than one. If there's just > one, the type can be known by the driver. In that case there's no use for > autodetection. > > The second case is a bit more complicated: the bus type detection is solely > based on properties available in the endpoint, and I think that may have > been feasible approach when there were just parallel and Bt.656 busses that > were supported, but with the additional busses, the V4L2 fwnode framework > may no longer guess the bus in any meaningful way from the available > properties. I'd think the only known-good option here is to specify the > type explicitly in that case: there's no room for guessing. (This patchset > makes it possible for drivers to explicitly define the bus type, but the > autodetection support is maintained for backwards compatibility.) > > One of the existing issues is that there are combined parallel/Bt.656 > receivers that need to know the type of the bus. This is based on the > existence parallel interface only properties: if any of these exist, then > the interface is parallel, otherwise it is Bt.656. The DT bindings for the > same devices also define the defaults for the parallel interface. This > leaves the end result ambiguous: is it the parallel interface with the > default configuration or is it Bt.656? > > There will likely be similar issues for CSI-2 D-PHY and CSI-2 C-PHY. The > question there would be: is this CSI-2 C-PHY or CSI-2 D-PHY with default > clock lane configuration? > > In either case the autodetection option for the bus type provides no useful > information. If it exists in DT source, that's fine, there's just no use > for it. > > Let me know if you still think it should be maintained in binding > documentation. If you prefer to keep it, I'd propose to mark it as deprecated or something as it provides no information to software. > > > > > > > > > Signed-off-by: Sakari Ailus > > > --- > > > Documentation/devicetree/bindings/media/video-interfaces.txt | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt > > > b/Documentation/devicetree/bindings/media/video-interfaces.txt > > > index baf9d9756b3c..f884ada0bffc 100644 > > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt > > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt > > > @@ -100,10 +100,12 @@ Optional endpoint properties > > >slave device (data source) by the master device (data sink). In the > > > master > > >mode the data source device is also the source of the synchronization > > > signals. > > > - bus-type: data bus type. Possible values are: > > > - 0 - autodetect based on other properties (MIPI CSI-2 D-PHY, parallel > > > or Bt656) > > >1 - MIPI CSI-2 C-PHY > > >2 - MIPI CSI1 > > >3 - CCP2 > > > + 4 - MIPI CSI-2 D-PHY > > > + 5 - Parallel > > > > Is that really specific enough to be useful? > > Yes; see above. > > > > > > + 6 - Bt.656 > > > - bus-width: number of data lines actively used, valid for the parallel > > > busses. > > > - data-shift: on the parallel data busses, if bus-width is used to > > > specify the > > >number of data lines, data-shift can be used to specify which data > > > lines are > -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCHv2 5/5] adv7842: enable reduced fps detection
On 15/08/18 15:40, Hans Verkuil wrote:
> From: Hans Verkuil
>
> The pixelclock detection of the adv7842 is precise enough to detect
> if the framerate is 60 Hz or 59.94 Hz (aka "reduced fps").
>
> Implement this detection.
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/i2c/adv7842.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
> index 4f8fbdd00e35..999d621f5667 100644
> --- a/drivers/media/i2c/adv7842.c
> +++ b/drivers/media/i2c/adv7842.c
> @@ -1525,6 +1525,7 @@ static void
> adv7842_fill_optional_dv_timings_fields(struct v4l2_subdev *sd,
> v4l2_find_dv_timings_cap(timings, adv7842_get_dv_timings_cap(sd),
> is_digital_input(sd) ? 25 : 100,
> adv7842_check_dv_timings, NULL);
> + timings->bt.flags |= V4L2_DV_FL_CAN_DETECT_REDUCED_FPS;
> }
>
> static int adv7842_query_dv_timings(struct v4l2_subdev *sd,
> @@ -1596,6 +1597,14 @@ static int adv7842_query_dv_timings(struct v4l2_subdev
> *sd,
> bt->il_vbackporch = 0;
> }
> adv7842_fill_optional_dv_timings_fields(sd, timings);
> + if ((timings->bt.flags & V4L2_DV_FL_CAN_REDUCE_FPS) &&
> + freq < bt->pixelclock) {
> + u32 reduced_freq = (bt->pixelclock / 1001) * 1000;
bt->pixelclock needs to be cast to u32 to avoid a 64 bit division.
> + u32 delta_freq = abs(freq - reduced_freq);
> +
> + if (delta_freq < (bt->pixelclock - reduced_freq) / 2)
Ditto.
(kbuild robot complained about this.)
Hans
> + timings->bt.flags |= V4L2_DV_FL_REDUCED_FPS;
> + }
> } else {
> /* find format
>* Since LCVS values are inaccurate [REF_03, p. 339-340],
>
