Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

On 11/07/2017 07:04 AM, Tim Harvey wrote:
> On Fri, Oct 20, 2017 at 7:00 AM, Tim Harvey  wrote:
>> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
>> 

 Regarding video standard detection where this chip provides me with
 vertical-period, horizontal-period, and horizontal-pulse-width I
 should be able to detect the standard simply based off of
 vertical-period (framerate) and horizontal-period (line width
 including blanking) right? I wasn't sure if my method of matching
 these within 14% tolerance made sense. I will be removing the hsmatch
 logic from that as it seems the horizontal-pulse-width should be
 irrelevant.
>>>
>>> For proper video detection you ideally need:
>>>
>>> h/v sync size
>>> h/v back/front porch size
>>> h/v polarity
>>> pixelclock (usually an approximation)
>>>
>>> The v4l2_find_dv_timings_cap() helper can help you find the corresponding
>>> timings, allowing for pixelclock variation.
>>>
>>> That function assumes that the sync/back/frontporch values are all known.
>>> But not all devices can actually discover those values. What can your
>>> hardware detect? Can it tell front and backporch apart? Can it determine
>>> the sync size?
>>>
>>> I've been considering for some time to improve that helper function to be
>>> able to handle hardware that isn't able separate sync/back/frontporch 
>>> values.
>>>
>>
>> The TDA1997x provides only the vertical/horizontal periods and the
>> horizontal pulse
>> width (section 8.3.4 of datasheet [1]).
>>
>> Can you point me to a good primer on the relationship between these
>> values and the h/v back/front porch?
>>
>> Currently I iterate over the list of known formats calculating hper
>> (bt->pixelclock / V4L2_DV_BT_FRAME_WIDTH(bt)) and vper (hper /
>> V4L2_DV_BT_FRAME_HEIGHT(bt)) (framerate) and find the closest match
>> within +/- 7% tolerance. The list of supported formats is sorted by
>> framerate then width.
>>
>> /* look for matching timings */
>> for (i = 0; i < ARRAY_SIZE(tda1997x_hdmi_modes); i++) {
>> const struct tda1997x_video_std *std = 
>> _hdmi_modes[i];
>> const struct v4l2_bt_timings *bt = >timings.bt;
>> int _hper, _vper, _hsper;
>> int vmin, vmax, hmin, hmax, hsmin, hsmax;
>> int vmatch, hsmatch;
>>
>> width = V4L2_DV_BT_FRAME_WIDTH(bt);
>> lines = V4L2_DV_BT_FRAME_HEIGHT(bt);
>>
>> _hper = (int)bt->pixelclock / (int)width;
>> _vper = _hper / lines;
>> _hsper = (int)bt->pixelclock / (int)bt->hsync;
>> if (bt->interlaced)
>> _vper *= 2;
>> /* vper +/- 0.7% */
>> vmin = 993 * (2700 / _vper) / 1000;
>> vmax = 1007 * (2700 / _vper) / 1000;
>> _hsper = (int)bt->pixelclock / (int)bt->hsync;
>> if (bt->interlaced)
>> _vper *= 2;
>> /* vper +/- 0.7% */
>> vmin = 993 * (2700 / _vper) / 1000;
>> vmax = 1007 * (2700 / _vper) / 1000;
>> /* hper +/- 0.7% */
>> hmin = 993 * (2700 / _hper) / 1000;
>> hmax = 1007 * (2700 / _hper) / 1000;
>>
>> /* vmatch matches the framerate */
>> vmatch = ((vper <= vmax) && (vper >= vmin)) ? 1 : 0;
>> /* hmatch matches the width */
>> hmatch = ((hper <= hmax) && (hper >= hmin)) ? 1 : 0;
>> if (vmatch && hsmatch) {
>> v4l_info(state->client,
>>  "resolution: %dx%d%c@%d (%d/%d/%d) %dMHz 
>> %d\n",
>>  bt->width, bt->height, 
>> bt->interlaced?'i':'p',
>>  _vper, vper, hper, hsper, pixrate, hsmatch);
>> state->fps = (int)bt->pixelclock / (width * lines);
>> state->std = std;
>> return 0;
>> }
>> }
>>
>> Note that I've thrown out any comparisons based on horizontal pulse
>> width from my first patch as that didn't appear to fit well. So far
>> the above works well however I do fail to recognize the following
>> modes (using a Marshall SG4K HDMI test generator):
>>
> 
> Hans,
> 
> I've found that I do indeed need to look at the 'hsper' that the TDA
> provides above along with the vper/hper as there are several timings
> that match a given vper/hper. However I haven't figured out how to
> make sense of the hsper value that is returned.
> 
> Here are some example timings and the vper/hper/hsper returned from the TDA:
> V4L2_DV_BT_DMT_1280X960P60 449981/448/55
> V4L2_DV_BT_DMT_1280X1024P60 449833/420/27
> V4L2_DV_BT_DMT_1280X768P60 450021/568/11
> V4L2_DV_BT_DMT_1360X768P60 449867/564/34
> 
> Do you know what the 

lening bieden nu aanvragen

[Scotwest Credit Union Limited]

Groetjes aan jou,


GELD BESCHIKBAAR VOOR LENING. Krijg het geld / de lening die je nodig hebt bij 
Scotwest Credit Union Limited. Wij zijn particuliere kredietverstrekkers / 
investeerders en bieden zowel persoonlijke leningen, startleningen, educatieve 
/ agrarische leningen, onroerend goed / bouwlening, onroerendgoedlening, 
schuldenvrije leningen, verhuur van mobiele woningen, lening met een hard geld, 
beveiligde / ongedekte lening, investeringsfinanciering, uitbreiding lening, Jv 
Capital / Rehab-leningen, aandelen / herfinancieringsleningen enz. aan 
genteresseerde personen uit welk land dan ook. We bieden leningen aan 
personen zowel nationaal als internationaal tegen een lage rente van 3%. Bent u 
geweigerd door banken of andere kredietverstrekkers, Scotwest Credit Union 
Limited is hier om u te helpen uw doel te bereiken. Als je een lening van welk 
type dan ook nodig hebt, neem dan contact met ons op via het onderstaande 
e-mailadres en we zijn er om je te helpen krijgen wat je nodig hebt: 
scot.wes...@gmail.com

Uw volledige namen:

Adres:

Telefoonnummer:

Lening vereist:

Looptijd:

Bezetting:

Maandelijks inkomensniveau:

Geslacht:

Geboortedatum:

Staat:

land:

Postcode:

Doel:

Zodra u deze informatie verstrekt, kunnen wij u de terugbetaling van de lening 
op basis van een maandelijkse basis aanbieden


We wachten op je snelle reactie.

Dank je.

Mr. R. Ashley

Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Fri, Oct 20, 2017 at 7:00 AM, Tim Harvey  wrote:
> On Thu, Oct 19, 2017 at 12:39 AM, Hans Verkuil  wrote:
> 
>>>
>>> Regarding video standard detection where this chip provides me with
>>> vertical-period, horizontal-period, and horizontal-pulse-width I
>>> should be able to detect the standard simply based off of
>>> vertical-period (framerate) and horizontal-period (line width
>>> including blanking) right? I wasn't sure if my method of matching
>>> these within 14% tolerance made sense. I will be removing the hsmatch
>>> logic from that as it seems the horizontal-pulse-width should be
>>> irrelevant.
>>
>> For proper video detection you ideally need:
>>
>> h/v sync size
>> h/v back/front porch size
>> h/v polarity
>> pixelclock (usually an approximation)
>>
>> The v4l2_find_dv_timings_cap() helper can help you find the corresponding
>> timings, allowing for pixelclock variation.
>>
>> That function assumes that the sync/back/frontporch values are all known.
>> But not all devices can actually discover those values. What can your
>> hardware detect? Can it tell front and backporch apart? Can it determine
>> the sync size?
>>
>> I've been considering for some time to improve that helper function to be
>> able to handle hardware that isn't able separate sync/back/frontporch values.
>>
>
> The TDA1997x provides only the vertical/horizontal periods and the
> horizontal pulse
> width (section 8.3.4 of datasheet [1]).
>
> Can you point me to a good primer on the relationship between these
> values and the h/v back/front porch?
>
> Currently I iterate over the list of known formats calculating hper
> (bt->pixelclock / V4L2_DV_BT_FRAME_WIDTH(bt)) and vper (hper /
> V4L2_DV_BT_FRAME_HEIGHT(bt)) (framerate) and find the closest match
> within +/- 7% tolerance. The list of supported formats is sorted by
> framerate then width.
>
> /* look for matching timings */
> for (i = 0; i < ARRAY_SIZE(tda1997x_hdmi_modes); i++) {
> const struct tda1997x_video_std *std = 
> _hdmi_modes[i];
> const struct v4l2_bt_timings *bt = >timings.bt;
> int _hper, _vper, _hsper;
> int vmin, vmax, hmin, hmax, hsmin, hsmax;
> int vmatch, hsmatch;
>
> width = V4L2_DV_BT_FRAME_WIDTH(bt);
> lines = V4L2_DV_BT_FRAME_HEIGHT(bt);
>
> _hper = (int)bt->pixelclock / (int)width;
> _vper = _hper / lines;
> _hsper = (int)bt->pixelclock / (int)bt->hsync;
> if (bt->interlaced)
> _vper *= 2;
> /* vper +/- 0.7% */
> vmin = 993 * (2700 / _vper) / 1000;
> vmax = 1007 * (2700 / _vper) / 1000;
> _hsper = (int)bt->pixelclock / (int)bt->hsync;
> if (bt->interlaced)
> _vper *= 2;
> /* vper +/- 0.7% */
> vmin = 993 * (2700 / _vper) / 1000;
> vmax = 1007 * (2700 / _vper) / 1000;
> /* hper +/- 0.7% */
> hmin = 993 * (2700 / _hper) / 1000;
> hmax = 1007 * (2700 / _hper) / 1000;
>
> /* vmatch matches the framerate */
> vmatch = ((vper <= vmax) && (vper >= vmin)) ? 1 : 0;
> /* hmatch matches the width */
> hmatch = ((hper <= hmax) && (hper >= hmin)) ? 1 : 0;
> if (vmatch && hsmatch) {
> v4l_info(state->client,
>  "resolution: %dx%d%c@%d (%d/%d/%d) %dMHz 
> %d\n",
>  bt->width, bt->height, 
> bt->interlaced?'i':'p',
>  _vper, vper, hper, hsper, pixrate, hsmatch);
> state->fps = (int)bt->pixelclock / (width * lines);
> state->std = std;
> return 0;
> }
> }
>
> Note that I've thrown out any comparisons based on horizontal pulse
> width from my first patch as that didn't appear to fit well. So far
> the above works well however I do fail to recognize the following
> modes (using a Marshall SG4K HDMI test generator):
>

Hans,

I've found that I do indeed need to look at the 'hsper' that the TDA
provides above along with the vper/hper as there are several timings
that match a given vper/hper. However I haven't figured out how to
make sense of the hsper value that is returned.

Here are some example timings and the vper/hper/hsper returned from the TDA:
V4L2_DV_BT_DMT_1280X960P60 449981/448/55
V4L2_DV_BT_DMT_1280X1024P60 449833/420/27
V4L2_DV_BT_DMT_1280X768P60 450021/568/11
V4L2_DV_BT_DMT_1360X768P60 449867/564/34

Do you know what the hsper could be here? It doesn't appear to match
v4l2_bt_timings hsync ((27MHz/bt->pixelclock)*bt->hsync).

Thanks,

Tim

cron job: media_tree daily build: ERRORS

[Hans Verkuil]

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:   Tue Nov  7 05:00:17 CET 2017
media-tree git hash:9917fbcfa20ab987d6381fd0365665e5c1402d75
media_build git hash:   106c5880df126bdd61f8d1c1f59570524a95d77c
v4l-utils git hash: f28ea7e869751e284b739fb226c3d26e9f8c2b1a
gcc version:i686-linux-gcc (GCC) 7.1.0
sparse version: v0.5.0
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.12.0-164

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-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.2.37-i686: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: ERRORS
linux-3.12.67-i686: ERRORS
linux-3.13.11-i686: ERRORS
linux-3.14.9-i686: ERRORS
linux-3.15.2-i686: ERRORS
linux-3.16.7-i686: ERRORS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: OK
linux-4.9.26-i686: OK
linux-4.10.14-i686: OK
linux-4.11-i686: OK
linux-4.12.1-i686: OK
linux-4.13-i686: OK
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.37-x86_64: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-x86_64: WARNINGS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: ERRORS
linux-3.12.67-x86_64: ERRORS
linux-3.13.11-x86_64: ERRORS
linux-3.14.9-x86_64: ERRORS
linux-3.15.2-x86_64: ERRORS
linux-3.16.7-x86_64: ERRORS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: WARNINGS
linux-4.9.26-x86_64: WARNINGS
linux-4.10.14-x86_64: WARNINGS
linux-4.11-x86_64: WARNINGS
linux-4.12.1-x86_64: WARNINGS
linux-4.13-x86_64: OK
apps: OK
spec-git: OK

Detailed results are available here:

http://www.xs4all.nl/~hverkuil/logs/Tuesday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/index.html

Re: [PATCH v2] uvc: Add D3DFMT_L8 support

[Laurent Pinchart]

Hi Nicolas,

Thank you for the patch.

On Monday, 6 November 2017 22:13:28 EET Nicolas Dufresne wrote:
> Microsoft HoloLense UVC sensor uses D3DFMT instead of FOURCC when
> exposing formats. This adds support for D3DFMT_L8 as exposed from
> the Acer Windows Mixed Reality Headset.
> 
> Signed-off-by: Nicolas Dufresne 

Reviewed-by: Laurent Pinchart 

and applied to my tree with "uvc" replaced by "uvcvideo" in the subject to 
match other commits.

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 5 +
>  drivers/media/usb/uvc/uvcvideo.h   | 5 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 6d22b22cb35b..113130b6b2d6
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -94,6 +94,11 @@ static struct uvc_format_desc uvc_fmts[] = {
>   .fcc= V4L2_PIX_FMT_GREY,
>   },
>   {
> + .name   = "Greyscale 8-bit (D3DFMT_L8)",
> + .guid   = UVC_GUID_FORMAT_D3DFMT_L8,
> + .fcc= V4L2_PIX_FMT_GREY,
> + },
> + {
>   .name   = "Greyscale 10-bit (Y10 )",
>   .guid   = UVC_GUID_FORMAT_Y10,
>   .fcc= V4L2_PIX_FMT_Y10,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 34c7ee6cc9e5..fbc1f433ff05 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -153,6 +153,11 @@
>   { 'I',  'N',  'V',  'I', 0xdb, 0x57, 0x49, 0x5e, \
>0x8e, 0x3f, 0xf4, 0x79, 0x53, 0x2b, 0x94, 0x6f}
> 
> +#define UVC_GUID_FORMAT_D3DFMT_L8 \
> + {0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, \
> +  0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71}
> +
> +
>  /* 
> * Driver specific constants.
>   */


-- 
Regards,

Laurent Pinchart

[PATCH 0/3] introduce get_user_pages_longterm()

[Dan Williams]

Andrew,

Here is a new get_user_pages api for cases where a driver intends to
keep an elevated page count indefinitely. This is distinct from usages
like iov_iter_get_pages where the elevated page counts are transient.
The iov_iter_get_pages cases immediately turn around and submit the
pages to a device driver which will put_page when the i/o operation
completes (under kernel control).

In the longterm case userspace is responsible for dropping the page
reference at some undefined point in the future. This is untenable for
filesystem-dax case where the filesystem is in control of the lifetime
of the block / page and needs reasonable limits on how long it can wait
for pages in a mapping to become idle.

Fixing filesystems to actually wait for dax pages to be idle before
blocks from a truncate/hole-punch operation are repurposed is saved for
a later patch series.

Also, allowing longterm registration of dax mappings is a future patch
series that introduces a "map with lease" semantic where the kernel can
revoke a lease and force userspace to drop its page references.

I have also tagged these for -stable to purposely break cases that might
assume that longterm memory registrations for filesystem-dax mappings
were supported by the kernel. The behavior regression this policy change
implies is one of the reasons we maintain the "dax enabled. Warning:
EXPERIMENTAL, use at your own risk" notification when mounting a
filesystem in dax mode.

It is worth noting the device-dax interface does not suffer the same
constraints since it does not support file space management operations
like hole-punch.

---

Dan Williams (3):
  mm: introduce get_user_pages_longterm
  IB/core: disable memory registration of fileystem-dax vmas
  [media] v4l2: disable filesystem-dax mapping support


 drivers/infiniband/core/umem.c|2 -
 drivers/media/v4l2-core/videobuf-dma-sg.c |5 +-
 include/linux/mm.h|3 +
 mm/gup.c  |   75 +
 4 files changed, 82 insertions(+), 3 deletions(-)

[PATCH 3/3] [media] v4l2: disable filesystem-dax mapping support

[Dan Williams]

V4L2 memory registrations are incompatible with filesystem-dax that
needs the ability to revoke dma access to a mapping at will, or
otherwise allow the kernel to wait for completion of DMA. The
filesystem-dax implementation breaks the traditional solution of
truncate of active file backed mappings since there is no page-cache
page we can orphan to sustain ongoing DMA.

If v4l2 wants to support long lived DMA mappings it needs to arrange to
hold a file lease or use some other mechanism so that the kernel can
coordinate revoking DMA access when the filesystem needs to truncate
mappings.

Reported-by: Jan Kara 
Cc: Mauro Carvalho Chehab 
Cc: linux-media@vger.kernel.org
Cc: 
Signed-off-by: Dan Williams 
---
 drivers/media/v4l2-core/videobuf-dma-sg.c |5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c 
b/drivers/media/v4l2-core/videobuf-dma-sg.c
index 0b5c43f7e020..f412429cf5ba 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -185,12 +185,13 @@ static int videobuf_dma_init_user_locked(struct 
videobuf_dmabuf *dma,
dprintk(1, "init user [0x%lx+0x%lx => %d pages]\n",
data, size, dma->nr_pages);
 
-   err = get_user_pages(data & PAGE_MASK, dma->nr_pages,
+   err = get_user_pages_longterm(data & PAGE_MASK, dma->nr_pages,
 flags, dma->pages, NULL);
 
if (err != dma->nr_pages) {
dma->nr_pages = (err >= 0) ? err : 0;
-   dprintk(1, "get_user_pages: err=%d [%d]\n", err, dma->nr_pages);
+   dprintk(1, "get_user_pages_longterm: err=%d [%d]\n", err,
+   dma->nr_pages);
return err < 0 ? err : -EINVAL;
}
return 0;

RE: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Monday, November 06, 2017 3:38 PM
> To: Mani, Rajmohan 
> Cc: Zhi, Yong ; linux-media@vger.kernel.org; Zheng, Jian
> Xu ; tf...@chromium.org; Toivonen, Tuukka
> ; Yang, Hyungwoo
> ; Rapolu, Chiranjeevi
> ; Hu, Jerry W ;
> Vijaykumar, Ramya 
> Subject: Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
> 
> Hi Rajmohan,
> 
> On Mon, Nov 06, 2017 at 07:32:52PM +, Mani, Rajmohan wrote:
> > Hi Sakari, Yong,
> >
> > > Subject: Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2
> > > driver
> > >
> > > Hi Yong,
> > >
> > > Thanks for the update!
> > >
> > > I took a final glance, there are a few matters that still need to be
> > > addressed but then I think we're done. Please see below.
> > >
> > > Then we'll need an entry in the MAINTAINERS file in the kernel root
> directory.
> > > Could you add an entry for this driver in v8?
> > >
> > > On Thu, Nov 02, 2017 at 03:00:01PM -0500, Yong Zhi wrote:
> > > ...
> > > > +static int cio2_fbpt_rearrange(struct cio2_device *cio2, struct
> > > > +cio2_queue *q) {
> > > > +   unsigned int i, j;
> > > > +
> > > > +   for (i = 0, j = q->bufs_first; i < CIO2_MAX_BUFFERS;
> > > > +   i++, j = (j + 1) % CIO2_MAX_BUFFERS)
> > > > +   if (q->bufs[j])
> > > > +   break;
> > > > +
> > > > +   if (i == CIO2_MAX_BUFFERS)
> > > > +   return 0;
> > >
> > > You always return 0. The return type could be void.
> > >
> > > You could also move this function just above the function using it
> > > (it's a single location).
> > >
> > > > +
> > > > +   if (j) {
> > > > +   arrange(q->fbpt, sizeof(struct cio2_fbpt_entry) *
> > > CIO2_MAX_LOPS,
> > > > +   CIO2_MAX_BUFFERS, j);
> > > > +   arrange(q->bufs, sizeof(struct cio2_buffer *),
> > > > +   CIO2_MAX_BUFFERS, j);
> > > > +   }
> > > > +
> > > > +   /*
> > > > +* DMA clears the valid bit when accessing the buffer.
> > > > +* When stopping stream in suspend callback, some of the buffers
> > > > +* may be in invalid state. After resume, when DMA meets the 
> > > > invalid
> > > > +* buffer, it will halt and stop receiving new data.
> > > > +* To avoid DMA halting, set the valid bit for all buffers in 
> > > > FBPT.
> > > > +*/
> > > > +   for (i = 0; i < CIO2_MAX_BUFFERS; i++)
> > > > +   cio2_fbpt_entry_enable(cio2, q->fbpt + i * 
> > > > CIO2_MAX_LOPS);
> > > > +
> > > > +   return 0;
> > > > +}
> > >
> > > ...
> > >
> > > > +static int cio2_vb2_start_streaming(struct vb2_queue *vq,
> > > > +unsigned int count) {
> > > > +   struct cio2_queue *q = vb2q_to_cio2_queue(vq);
> > > > +   struct cio2_device *cio2 = vb2_get_drv_priv(vq);
> > > > +   int r;
> > > > +
> > > > +   cio2->cur_queue = q;
> > > > +   atomic_set(>frame_sequence, 0);
> > > > +
> > > > +   r = pm_runtime_get_sync(>pci_dev->dev);
> > > > +   if (r < 0) {
> > > > +   dev_info(>pci_dev->dev, "failed to set power 
> > > > %d\n", r);
> > > > +   pm_runtime_put(>pci_dev->dev);
> >
> > Shouldn't we use pm_runtime_put_noidle() here, so the usage_count gets
> decremented?
> 
> Yeah, pm_runtime_put_noidle() would be perhaps more appropriate. The
> difference is that the noidle variant won't suspend the device (but it was
> already suspended to begin with as resuming failed).
> 

Thanks for confirming.

> >
> > > > +   return r;
> > > > +   }
> > > > +
> > > > +   r = media_pipeline_start(>vdev.entity, >pipe);
> > > > +   if (r)
> > > > +   goto fail_pipeline;
> > > > +
> > > > +   r = cio2_hw_init(cio2, q);
> > > > +   if (r)
> > > > +   goto fail_hw;
> > > > +
> > > > +   /* Start streaming on sensor */
> > > > +   r = v4l2_subdev_call(q->sensor, video, s_stream, 1);
> > > > +   if (r)
> > > > +   goto fail_csi2_subdev;
> > > > +
> > > > +   cio2->streaming = true;
> > > > +
> > > > +   return 0;
> > > > +
> > > > +fail_csi2_subdev:
> > > > +   cio2_hw_exit(cio2, q);
> > > > +fail_hw:
> > > > +   media_pipeline_stop(>vdev.entity);
> > > > +fail_pipeline:
> > > > +   dev_dbg(>pci_dev->dev, "failed to start streaming 
> > > > (%d)\n", r);
> > > > +   cio2_vb2_return_all_buffers(q);
> > >
> > > I believe there should be
> > >
> > >   pm_runtime_put(>pci_dev->dev);
> > >
> > > here. You should also add a label and use goto from where you do
> > > pm_runtime_put() in error handling in this function in order to make
> > > this cleaner.
> > >
> > > > +
> > > > +   return r;
> > > > +}
> > >
> > > --
> 

Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Sakari Ailus]

Hi Rajmohan,

On Mon, Nov 06, 2017 at 07:32:52PM +, Mani, Rajmohan wrote:
> Hi Sakari, Yong,
> 
> > Subject: Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
> > 
> > Hi Yong,
> > 
> > Thanks for the update!
> > 
> > I took a final glance, there are a few matters that still need to be 
> > addressed but
> > then I think we're done. Please see below.
> > 
> > Then we'll need an entry in the MAINTAINERS file in the kernel root 
> > directory.
> > Could you add an entry for this driver in v8?
> > 
> > On Thu, Nov 02, 2017 at 03:00:01PM -0500, Yong Zhi wrote:
> > ...
> > > +static int cio2_fbpt_rearrange(struct cio2_device *cio2, struct
> > > +cio2_queue *q) {
> > > + unsigned int i, j;
> > > +
> > > + for (i = 0, j = q->bufs_first; i < CIO2_MAX_BUFFERS;
> > > + i++, j = (j + 1) % CIO2_MAX_BUFFERS)
> > > + if (q->bufs[j])
> > > + break;
> > > +
> > > + if (i == CIO2_MAX_BUFFERS)
> > > + return 0;
> > 
> > You always return 0. The return type could be void.
> > 
> > You could also move this function just above the function using it (it's a 
> > single
> > location).
> > 
> > > +
> > > + if (j) {
> > > + arrange(q->fbpt, sizeof(struct cio2_fbpt_entry) *
> > CIO2_MAX_LOPS,
> > > + CIO2_MAX_BUFFERS, j);
> > > + arrange(q->bufs, sizeof(struct cio2_buffer *),
> > > + CIO2_MAX_BUFFERS, j);
> > > + }
> > > +
> > > + /*
> > > +  * DMA clears the valid bit when accessing the buffer.
> > > +  * When stopping stream in suspend callback, some of the buffers
> > > +  * may be in invalid state. After resume, when DMA meets the invalid
> > > +  * buffer, it will halt and stop receiving new data.
> > > +  * To avoid DMA halting, set the valid bit for all buffers in FBPT.
> > > +  */
> > > + for (i = 0; i < CIO2_MAX_BUFFERS; i++)
> > > + cio2_fbpt_entry_enable(cio2, q->fbpt + i * CIO2_MAX_LOPS);
> > > +
> > > + return 0;
> > > +}
> > 
> > ...
> > 
> > > +static int cio2_vb2_start_streaming(struct vb2_queue *vq, unsigned
> > > +int count) {
> > > + struct cio2_queue *q = vb2q_to_cio2_queue(vq);
> > > + struct cio2_device *cio2 = vb2_get_drv_priv(vq);
> > > + int r;
> > > +
> > > + cio2->cur_queue = q;
> > > + atomic_set(>frame_sequence, 0);
> > > +
> > > + r = pm_runtime_get_sync(>pci_dev->dev);
> > > + if (r < 0) {
> > > + dev_info(>pci_dev->dev, "failed to set power %d\n", r);
> > > + pm_runtime_put(>pci_dev->dev);
> 
> Shouldn't we use pm_runtime_put_noidle() here, so the usage_count gets 
> decremented?

Yeah, pm_runtime_put_noidle() would be perhaps more appropriate. The
difference is that the noidle variant won't suspend the device (but it was
already suspended to begin with as resuming failed).

> 
> > > + return r;
> > > + }
> > > +
> > > + r = media_pipeline_start(>vdev.entity, >pipe);
> > > + if (r)
> > > + goto fail_pipeline;
> > > +
> > > + r = cio2_hw_init(cio2, q);
> > > + if (r)
> > > + goto fail_hw;
> > > +
> > > + /* Start streaming on sensor */
> > > + r = v4l2_subdev_call(q->sensor, video, s_stream, 1);
> > > + if (r)
> > > + goto fail_csi2_subdev;
> > > +
> > > + cio2->streaming = true;
> > > +
> > > + return 0;
> > > +
> > > +fail_csi2_subdev:
> > > + cio2_hw_exit(cio2, q);
> > > +fail_hw:
> > > + media_pipeline_stop(>vdev.entity);
> > > +fail_pipeline:
> > > + dev_dbg(>pci_dev->dev, "failed to start streaming (%d)\n", r);
> > > + cio2_vb2_return_all_buffers(q);
> > 
> > I believe there should be
> > 
> > pm_runtime_put(>pci_dev->dev);
> > 
> > here. You should also add a label and use goto from where you do
> > pm_runtime_put() in error handling in this function in order to make this
> > cleaner.
> > 
> > > +
> > > + return r;
> > > +}
> > 
> > --
> > Kind regards,
> > 
> > Sakari Ailus
> > sakari.ai...@linux.intel.com

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

[PATCH 2/3] atomisp: fix vfree of bogus data on unload

[Alan]

We load the firmware once, set pointers to it and then at some point release
it. We should not be doing a vfree() on the pointers into the firmware.

Signed-off-by: Alan Cox 
---
 .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c |2 --
 1 file changed, 2 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
index 8158ea40d069..f181bd8fcee2 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c
@@ -288,8 +288,6 @@ void sh_css_unload_firmware(void)
for (i = 0; i < sh_css_num_binaries; i++) {
if (fw_minibuffer[i].name)
kfree((void *)fw_minibuffer[i].name);
-   if (fw_minibuffer[i].buffer)
-   vfree((void *)fw_minibuffer[i].buffer);
}
kfree(fw_minibuffer);
fw_minibuffer = NULL;

[PATCH 1/3] atomisp: Fix up the open v load race

[Alan]

This isn't the ideal final solution but it stops the main problem for now
where an open (often from udev) races the device initialization and we try
and load the firmware twice at the same time. This needless to say doesn't
usually end well.

Signed-off-by: Alan Cox 
---
 .../media/atomisp/pci/atomisp2/atomisp_fops.c  |   12 
 .../media/atomisp/pci/atomisp2/atomisp_internal.h  |5 +
 .../media/atomisp/pci/atomisp2/atomisp_v4l2.c  |6 ++
 3 files changed, 23 insertions(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
index dd7596d8763d..b82c53cee32c 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
@@ -771,6 +771,18 @@ static int atomisp_open(struct file *file)
 
dev_dbg(isp->dev, "open device %s\n", vdev->name);
 
+   /* Ensure that if we are still loading we block. Once the loading
+  is over we can proceed. We can't blindly hold the lock until
+  that occurs as if the load fails we'll deadlock the unload */
+   rt_mutex_lock(>loading);
+   /* Revisit this with a better check once the code structure is
+  cleaned up a bit more FIXME */
+   if (!isp->ready) {
+   rt_mutex_unlock(>loading);
+   return -ENXIO;
+   }
+   rt_mutex_unlock(>loading);
+
rt_mutex_lock(>mutex);
 
acc_node = !strncmp(vdev->name, "ATOMISP ISP ACC",
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
index 52a6f8002048..808d79c840d4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
@@ -252,6 +252,11 @@ struct atomisp_device {
/* Purpose of mutex is to protect and serialize use of isp data
 * structures and css API calls. */
struct rt_mutex mutex;
+   /* This mutex ensures that we don't allow an open to succeed while
+* the initialization process is incomplete */
+   struct rt_mutex loading;
+   /* Set once the ISP is ready to allow opens */
+   bool ready;
/*
 * Serialise streamoff: mutex is dropped during streamoff to
 * cancel the watchdog queue. MUST be acquired BEFORE
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
index 3c260f8b52e2..350e298bc3a6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
@@ -1220,6 +1220,7 @@ static int atomisp_pci_probe(struct pci_dev *dev,
isp->saved_regs.ispmmadr = start;
 
rt_mutex_init(>mutex);
+   rt_mutex_init(>loading);
mutex_init(>streamoff_mutex);
spin_lock_init(>lock);
 
@@ -1393,6 +1394,8 @@ static int atomisp_pci_probe(struct pci_dev *dev,
  csi_afe_trim);
}
 
+   rt_mutex_lock(>loading);
+
err = atomisp_initialize_modules(isp);
if (err < 0) {
dev_err(>dev, "atomisp_initialize_modules (%d)\n", err);
@@ -1450,6 +1453,8 @@ static int atomisp_pci_probe(struct pci_dev *dev,
release_firmware(isp->firmware);
isp->firmware = NULL;
isp->css_env.isp_css_fw.data = NULL;
+   isp->ready = true;
+   rt_mutex_unlock(>loading);
 
atomisp_drvfs_init(_pci_driver, isp);
 
@@ -1468,6 +1473,7 @@ static int atomisp_pci_probe(struct pci_dev *dev,
 register_entities_fail:
atomisp_uninitialize_modules(isp);
 initialize_modules_fail:
+   rt_mutex_unlock(>loading);
pm_qos_remove_request(>pm_qos);
atomisp_msi_irq_uninit(isp, dev);
 enable_msi_fail:

[PATCH 3/3] atomisp: hmm gives a bogus warning on unload

[Alan]

The problem is that we allocated a dummy page to ensure that 0 aka NULL
isn't returned as an ISP pointer into the hmm space. The free routine rather
sensibly checks for bogus NULL frees but is tripped by hmm_cleanup freeing
the dummy.

Split the routine so that we can keep the protection check and avoid the
bogus warning on a cleanup

Signed-off-by: Alan Cox 
---
 .../staging/media/atomisp/pci/atomisp2/hmm/hmm.c   |   21 +---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c 
b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
index a1c81c12718c..dfb9bf54b5d2 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
@@ -42,6 +42,8 @@ static ia_css_ptr dummy_ptr;
 static bool hmm_initialized;
 struct _hmm_mem_stat hmm_mem_stat;
 
+static void hmm_do_free(ia_css_ptr virt);
+
 /*
  * p: private
  * s: shared
@@ -211,7 +213,7 @@ void hmm_cleanup(void)
sysfs_remove_group(_dev->kobj, atomisp_attribute_group);
 
/* free dummy memory first */
-   hmm_free(dummy_ptr);
+   hmm_do_free(dummy_ptr);
dummy_ptr = 0;
 
hmm_bo_device_exit(_device);
@@ -268,13 +270,10 @@ ia_css_ptr hmm_alloc(size_t bytes, enum hmm_bo_type type,
return 0;
 }
 
-void hmm_free(ia_css_ptr virt)
+static void hmm_do_free(ia_css_ptr virt)
 {
-   struct hmm_buffer_object *bo;
-
-   WARN_ON(!virt);
-
-   bo = hmm_bo_device_search_start(_device, (unsigned int)virt);
+   struct hmm_buffer_object *bo =
+   hmm_bo_device_search_start(_device, (unsigned int)virt);
 
if (!bo) {
dev_err(atomisp_dev,
@@ -290,6 +289,14 @@ void hmm_free(ia_css_ptr virt)
hmm_bo_unref(bo);
 }
 
+
+void hmm_free(ia_css_ptr virt)
+{
+   WARN_ON(!virt);
+
+   hmm_do_free(virt);
+}
+
 static inline int hmm_check_bo(struct hmm_buffer_object *bo, unsigned int ptr)
 {
if (!bo) {

Re: [RESEND PATCH 1/1] of: Make return types of to_of_node and of_fwnode_handle macros consistent

[Rob Herring]

On Thu, Nov 02, 2017 at 11:59:18AM +0200, Sakari Ailus wrote:
> (Fixed Mauro's e-mail.)
> 
> to_of_node() macro checks whether the fwnode_handle passed to it is not an
> OF node, and if so, returns NULL in order to be NULL-safe. Otherwise it
> returns the pointer to the OF node which the fwnode_handle contains.
> 
> The problem with returning NULL is that its type was void *, which
> sometimes matters. Explicitly return struct device_node * instead.
> 
> Make a similar change to of_fwnode_handle() as well.
> 
> Fixes: d20dc1493db4 ("of: Support const and non-const use for to_of_node()")
> Fixes: debd3a3b27c7 ("of: Make of_fwnode_handle() safer")
> Signed-off-by: Sakari Ailus 
> ---
> Hi Mauro,
> 
> Could you check whether this addresses the smatch issue with the xilinx
> driver?
> 
> Thanks.
> 
>  include/linux/of.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Acked-by: Rob Herring 

[PATCH v2] uvc: Add D3DFMT_L8 support

[Nicolas Dufresne]

Microsoft HoloLense UVC sensor uses D3DFMT instead of FOURCC when
exposing formats. This adds support for D3DFMT_L8 as exposed from
the Acer Windows Mixed Reality Headset.

Signed-off-by: Nicolas Dufresne 
---
 drivers/media/usb/uvc/uvc_driver.c | 5 +
 drivers/media/usb/uvc/uvcvideo.h   | 5 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c 
b/drivers/media/usb/uvc/uvc_driver.c
index 6d22b22cb35b..113130b6b2d6 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -94,6 +94,11 @@ static struct uvc_format_desc uvc_fmts[] = {
.fcc= V4L2_PIX_FMT_GREY,
},
{
+   .name   = "Greyscale 8-bit (D3DFMT_L8)",
+   .guid   = UVC_GUID_FORMAT_D3DFMT_L8,
+   .fcc= V4L2_PIX_FMT_GREY,
+   },
+   {
.name   = "Greyscale 10-bit (Y10 )",
.guid   = UVC_GUID_FORMAT_Y10,
.fcc= V4L2_PIX_FMT_Y10,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 34c7ee6cc9e5..fbc1f433ff05 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -153,6 +153,11 @@
{ 'I',  'N',  'V',  'I', 0xdb, 0x57, 0x49, 0x5e, \
 0x8e, 0x3f, 0xf4, 0x79, 0x53, 0x2b, 0x94, 0x6f}
 
+#define UVC_GUID_FORMAT_D3DFMT_L8 \
+   {0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, \
+0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71}
+
+
 /* 
  * Driver specific constants.
  */
-- 
2.13.6

RE: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Mani, Rajmohan]

Hi Sakari, Yong,

> Subject: Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
> 
> Hi Yong,
> 
> Thanks for the update!
> 
> I took a final glance, there are a few matters that still need to be 
> addressed but
> then I think we're done. Please see below.
> 
> Then we'll need an entry in the MAINTAINERS file in the kernel root directory.
> Could you add an entry for this driver in v8?
> 
> On Thu, Nov 02, 2017 at 03:00:01PM -0500, Yong Zhi wrote:
> ...
> > +static int cio2_fbpt_rearrange(struct cio2_device *cio2, struct
> > +cio2_queue *q) {
> > +   unsigned int i, j;
> > +
> > +   for (i = 0, j = q->bufs_first; i < CIO2_MAX_BUFFERS;
> > +   i++, j = (j + 1) % CIO2_MAX_BUFFERS)
> > +   if (q->bufs[j])
> > +   break;
> > +
> > +   if (i == CIO2_MAX_BUFFERS)
> > +   return 0;
> 
> You always return 0. The return type could be void.
> 
> You could also move this function just above the function using it (it's a 
> single
> location).
> 
> > +
> > +   if (j) {
> > +   arrange(q->fbpt, sizeof(struct cio2_fbpt_entry) *
> CIO2_MAX_LOPS,
> > +   CIO2_MAX_BUFFERS, j);
> > +   arrange(q->bufs, sizeof(struct cio2_buffer *),
> > +   CIO2_MAX_BUFFERS, j);
> > +   }
> > +
> > +   /*
> > +* DMA clears the valid bit when accessing the buffer.
> > +* When stopping stream in suspend callback, some of the buffers
> > +* may be in invalid state. After resume, when DMA meets the invalid
> > +* buffer, it will halt and stop receiving new data.
> > +* To avoid DMA halting, set the valid bit for all buffers in FBPT.
> > +*/
> > +   for (i = 0; i < CIO2_MAX_BUFFERS; i++)
> > +   cio2_fbpt_entry_enable(cio2, q->fbpt + i * CIO2_MAX_LOPS);
> > +
> > +   return 0;
> > +}
> 
> ...
> 
> > +static int cio2_vb2_start_streaming(struct vb2_queue *vq, unsigned
> > +int count) {
> > +   struct cio2_queue *q = vb2q_to_cio2_queue(vq);
> > +   struct cio2_device *cio2 = vb2_get_drv_priv(vq);
> > +   int r;
> > +
> > +   cio2->cur_queue = q;
> > +   atomic_set(>frame_sequence, 0);
> > +
> > +   r = pm_runtime_get_sync(>pci_dev->dev);
> > +   if (r < 0) {
> > +   dev_info(>pci_dev->dev, "failed to set power %d\n", r);
> > +   pm_runtime_put(>pci_dev->dev);

Shouldn't we use pm_runtime_put_noidle() here, so the usage_count gets 
decremented?

> > +   return r;
> > +   }
> > +
> > +   r = media_pipeline_start(>vdev.entity, >pipe);
> > +   if (r)
> > +   goto fail_pipeline;
> > +
> > +   r = cio2_hw_init(cio2, q);
> > +   if (r)
> > +   goto fail_hw;
> > +
> > +   /* Start streaming on sensor */
> > +   r = v4l2_subdev_call(q->sensor, video, s_stream, 1);
> > +   if (r)
> > +   goto fail_csi2_subdev;
> > +
> > +   cio2->streaming = true;
> > +
> > +   return 0;
> > +
> > +fail_csi2_subdev:
> > +   cio2_hw_exit(cio2, q);
> > +fail_hw:
> > +   media_pipeline_stop(>vdev.entity);
> > +fail_pipeline:
> > +   dev_dbg(>pci_dev->dev, "failed to start streaming (%d)\n", r);
> > +   cio2_vb2_return_all_buffers(q);
> 
> I believe there should be
> 
>   pm_runtime_put(>pci_dev->dev);
> 
> here. You should also add a label and use goto from where you do
> pm_runtime_put() in error handling in this function in order to make this
> cleaner.
> 
> > +
> > +   return r;
> > +}
> 
> --
> Kind regards,
> 
> Sakari Ailus
> sakari.ai...@linux.intel.com

Re: [PATCH v2] media: s5p-mfc: Add support for V4L2_MEMORY_DMABUF type

[Nicolas Dufresne]

Le lundi 06 novembre 2017 à 10:28 +0100, Marek Szyprowski a écrit :
> Hi Nicolas,
> 
> On 2017-11-03 14:45, Nicolas Dufresne wrote:
> > Le vendredi 03 novembre 2017 à 09:11 +0100, Marek Szyprowski a écrit :
> > > MFC driver supports only MMAP operation mode mainly due to the hardware
> > > restrictions of the addresses of the DMA buffers (MFC v5 hardware can
> > > access buffers only in 128MiB memory region starting from the base address
> > > of its firmware). When IOMMU is available, this requirement is easily
> > > fulfilled even for the buffers located anywhere in the memory - typically
> > > by mapping them in the DMA address space as close as possible to the
> > > firmware. Later hardware revisions don't have this limitations at all.
> > > 
> > > The second limitation of the MFC hardware related to the memory buffers
> > > is constant buffer address. Once the hardware has been initialized for
> > > operation on given buffer set, the addresses of the buffers cannot be
> > > changed.
> > > 
> > > With the above assumptions, a limited support for USERPTR and DMABUF
> > > operation modes can be added. The main requirement is to have all buffers
> > > known when starting hardware. This has been achieved by postponing
> > > hardware initialization once all the DMABUF or USERPTR buffers have been
> > > queued for the first time. Once then, buffers cannot be modified to point
> > > to other memory area.
> > 
> > I am concerned about enabling this support with existing userspace.
> > Userspace application will be left with some driver with this
> > limitation and other drivers without. I think it is harmful to enable
> > that feature without providing userspace the ability to know.
> > 
> > This is specially conflicting with let's say UVC driver providing
> > buffers, since UVC driver implementing CREATE_BUFS ioctl. So even if
> > userspace start making an effort to maintain the same DMABuf for the
> > same buffer index, if a new buffer is created, we won't be able to use
> > it.
> 
> Sorry, but I don't get this as an 'issue'. The typical use scenario is to
> configure buffer queue and start streaming. Basically ReqBufs, stream on and
> a sequence of bufq/bufdq. This is handled without any problem by MFC driver
> with this patch. After releasing a queue with reqbufs(0), one can use
> different set of buffers.

In real life, you often have capture code decorelated from the encoder
code. At least, it's the case in GStreamer. The encoder have no
information about how many buffers were pre-allocated by let's say the
capture driver. As a side effect, we cannot make sure the importation
queue is of the same size (amount of buffer). Specially if you have UVC
driver that allow allocating more buffers at run-time. This is used in
real-life to compensate additional latency that get's added when a
pipeline topology is changed (at run-time). Only workaround I had in
mind, would be to always prepare the queue with the maximum (32), and
use this as a cache size, but for now, this is not how the deployed
userspace works unfortunately.

Your reqbuf(0) technique cause a break in the stream (probably a new
keyframe), as you are forced to STREAMOFF. This is often unwanted, and
it may create a time discontinuity in case the allocation took time.

> 
> What is the point of changing the buffers during the streaming? IMHO it was
> one of the biggest pathology of the V4L2 USERPTR API that the buffers 
> were in
> fact 'created' on the first queue operation. By creating I mean creating all
> the kernel all needed structures and mappings between the real memory (user
> ptr value) and the buffer index. The result of this was userspace, which 
> don't
> use buffer indices and always queues buffers with index = 0, what means that
> kernel has to reacquire direct access to each buffer every single frame. 
> That
> is highly inefficient approach. DMABUF operation mode inherited this 
> drawback.

This in fact is an implementation detail of the caching in the kernel
framework. There is nothing that prevent the framework to maintain a
validation cache that isn't bound to the queue size. DMABuf simply
makes the buffer identification easier and safer. I bet it is difficult
and it will stay like this, so it should at least be documented.

I am completely aware of the inefficiency of the GStreamer behaviour,
though it remains much faster in many case then copying raw frames
using the CPU. You can complain as much as you can about what this
userspace doing, but it as has been working better then nothing for
many users. It might not be like this forever, someone could optimize
this by signalling the missing information or with the 32 buffer hack I
just mention, but I don't see anyone standing up for that work atm,
which I have assumed to be good enough (for now). We see a lot more
overhead from other component when piling up with a wayland compositor,
a GL stack and more.

> 
> When we have a pipeline for processing video data it should use N 

[PATCH] media: ddbridge: fix build warnings

[Randy Dunlap]

From: Randy Dunlap 

Fix 2 build warnings.
These functions are void, so drop the "return"s.

./drivers/media/pci/ddbridge/ddbridge-io.h: warning: 'return' with a value, in 
function returning void [enabled by default]:  => 50:2, 55:2

Signed-off-by: Randy Dunlap 
Cc: Daniel Scheller 
Cc: Mauro Carvalho Chehab 
Cc: Mauro Carvalho Chehab 
Reported-by: Geert Uytterhoeven 
---
 drivers/media/pci/ddbridge/ddbridge-io.h |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- lnx-414-rc8.orig/drivers/media/pci/ddbridge/ddbridge-io.h
+++ lnx-414-rc8/drivers/media/pci/ddbridge/ddbridge-io.h
@@ -47,12 +47,12 @@ static inline void ddbwritel(struct ddb
 
 static inline void ddbcpyto(struct ddb *dev, u32 adr, void *src, long count)
 {
-   return memcpy_toio(dev->regs + adr, src, count);
+   memcpy_toio(dev->regs + adr, src, count);
 }
 
 static inline void ddbcpyfrom(struct ddb *dev, void *dst, u32 adr, long count)
 {
-   return memcpy_fromio(dst, dev->regs + adr, count);
+   memcpy_fromio(dst, dev->regs + adr, count);
 }
 
 static inline u32 safe_ddbreadl(struct ddb *dev, u32 adr)

[PATCH] usb: dvb-usb-v2: dvb_usb_core: remove redundant code in dvb_usb_fe_sleep

[Gustavo A. R. Silva]

Check on return value and goto instruction is redundant as the code
that follows is the goto label err.

Addresses-Coverity-ID: 1268783
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c 
b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
index 096bb75..2bf3bd8 100644
--- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
+++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
@@ -628,8 +628,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
}
 
ret = dvb_usbv2_device_power_ctrl(d, 0);
-   if (ret < 0)
-   goto err;
+
 err:
if (!adap->suspend_resume_active) {
adap->active_fe = -1;
-- 
2.7.4

Re: [PATCH 2/6 v5] V4L: Add a UVC Metadata format

[Guennadi Liakhovetski]

Hi Hans,

Thanks for the comments.

On Mon, 30 Oct 2017, Hans Verkuil wrote:

> Hi Guennadi,
> 
> On 07/28/2017 02:46 PM, Hans Verkuil wrote:
> > On 07/28/2017 02:33 PM, Guennadi Liakhovetski wrote:
> >> Add a pixel format, used by the UVC driver to stream metadata.
> >>
> >> Signed-off-by: Guennadi Liakhovetski 
> >> ---
> >>  Documentation/media/uapi/v4l/meta-formats.rst|  1 +
> >>  Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39 
> >> 
> >>  include/uapi/linux/videodev2.h   |  1 +
> >>  3 files changed, 41 insertions(+)
> >>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst

[snip]

> >> diff --git a/include/uapi/linux/videodev2.h 
> >> b/include/uapi/linux/videodev2.h
> >> index 45cf735..0aad91c 100644
> >> --- a/include/uapi/linux/videodev2.h
> >> +++ b/include/uapi/linux/videodev2.h
> >> @@ -682,6 +682,7 @@ struct v4l2_pix_format {
> >>  /* Meta-data formats */
> >>  #define V4L2_META_FMT_VSP1_HGOv4l2_fourcc('V', 'S', 'P', 'H') /* 
> >> R-Car VSP1 1-D Histogram */
> >>  #define V4L2_META_FMT_VSP1_HGTv4l2_fourcc('V', 'S', 'P', 'T') /* 
> >> R-Car VSP1 2-D Histogram */
> >> +#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC 
> >> Payload Header metadata */
> 
> I discussed this with Laurent last week and since the metadata for UVC starts
> with a standard header followed by vendor-specific data it makes sense to
> use V4L2_META_FMT_UVC for just the standard header. Any vendor specific 
> formats
> should have their own fourcc which starts with the standard header followed by
> the custom data. The UVC driver would enumerate both the standard and the 
> vendor
> specific fourcc. This would allow generic UVC applications to use the standard
> header. Applications that know about the vendor specific data can select the
> vendor specific format.
> 
> This change would make this much more convenient to use.

Then the driver should be able to decide, which private fourcc code to use 
for each of those devices. A natural way to do that seems to be to put 
that in the .driver_info field of struct usb_device_id. For that I'd 
replace the current use of that field for quirks with a pointer to a 
struct in a separate patch. Laurent, would that be acceptable? Then add a 
field to that struct for a private metadata fourcc code.

Thanks
Guennadi

> Regards,
> 
>   Hans
> 
> >>  /* priv field value to indicates that subsequent fields are valid. */
> >>  #define V4L2_PIX_FMT_PRIV_MAGIC   0xfeedcafe

[PATCH] media: av7110: avoid 2038 overflow in debug print

[Arnd Bergmann]

Using the deprecated do_gettimeofday() in print_time() will overflow
in 2038 on 32-bit architectures. It'sbetter to use a structure that
is safe everywhere. While we're at it, fix the missing leading zeroes
on the sub-second portion.

Signed-off-by: Arnd Bergmann 
---
 drivers/media/pci/ttpci/av7110.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/ttpci/av7110.c b/drivers/media/pci/ttpci/av7110.c
index f89fb23f6c57..6d415bdeef18 100644
--- a/drivers/media/pci/ttpci/av7110.c
+++ b/drivers/media/pci/ttpci/av7110.c
@@ -347,9 +347,9 @@ static int DvbDmxFilterCallback(u8 *buffer1, size_t 
buffer1_len,
 static inline void print_time(char *s)
 {
 #ifdef DEBUG_TIMING
-   struct timeval tv;
-   do_gettimeofday();
-   printk("%s: %d.%d\n", s, (int)tv.tv_sec, (int)tv.tv_usec);
+   struct timespec64 ts;
+   ktime_get_real_ts64();
+   printk("%s: %lld.%09ld\n", s, (s64)ts.tv_sec, ts.tv_nsec);
 #endif
 }
 
-- 
2.9.0

[PATCH] media: rc: Replace timeval with ktime_t in imon.c

[Arnd Bergmann]

From: Chunyan Zhang 

This patch changes the 32-bit time type (timeval) to the 64-bit one
(ktime_t), since 32-bit time types will break in the year 2038.

I use ktime_t instead of all uses of timeval in imon.c

This patch also changes do_gettimeofday() to ktime_get() accordingly,
since ktime_get returns a ktime_t, but do_gettimeofday returns a
struct timeval, and the other reason is that ktime_get() uses
the monotonic clock.

Signed-off-by: Chunyan Zhang 
Acked-by: Mauro Carvalho Chehab 
Signed-off-by: Arnd Bergmann 
---
This patch was part of an old three-patch series, but did not
get merged at the time after I dropped the ball on it.
---
 drivers/media/rc/imon.c | 49 +
 1 file changed, 13 insertions(+), 36 deletions(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index b25b35b3f6da..6c3ca1fff16b 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -37,7 +38,6 @@
 #include 
 #include 
 
-#include 
 #include 
 
 #define MOD_AUTHOR "Jarod Wilson "
@@ -1168,29 +1168,6 @@ static int imon_ir_change_protocol(struct rc_dev *rc, 
u64 *rc_proto)
return retval;
 }
 
-static inline int tv2int(const struct timeval *a, const struct timeval *b)
-{
-   int usecs = 0;
-   int sec   = 0;
-
-   if (b->tv_usec > a->tv_usec) {
-   usecs = 100;
-   sec--;
-   }
-
-   usecs += a->tv_usec - b->tv_usec;
-
-   sec += a->tv_sec - b->tv_sec;
-   sec *= 1000;
-   usecs /= 1000;
-   sec += usecs;
-
-   if (sec < 0)
-   sec = 1000;
-
-   return sec;
-}
-
 /**
  * The directional pad behaves a bit differently, depending on whether this is
  * one of the older ffdc devices or a newer device. Newer devices appear to
@@ -1201,16 +1178,16 @@ static inline int tv2int(const struct timeval *a, const 
struct timeval *b)
  */
 static int stabilize(int a, int b, u16 timeout, u16 threshold)
 {
-   struct timeval ct;
-   static struct timeval prev_time = {0, 0};
-   static struct timeval hit_time  = {0, 0};
+   ktime_t ct;
+   static ktime_t prev_time;
+   static ktime_t hit_time;
static int x, y, prev_result, hits;
int result = 0;
-   int msec, msec_hit;
+   long msec, msec_hit;
 
-   do_gettimeofday();
-   msec = tv2int(, _time);
-   msec_hit = tv2int(, _time);
+   ct = ktime_get();
+   msec = ktime_ms_delta(ct, prev_time);
+   msec_hit = ktime_ms_delta(ct, hit_time);
 
if (msec > 100) {
x = 0;
@@ -1688,9 +1665,9 @@ static void imon_incoming_scancode(struct imon_context 
*ictx,
u32 kc;
u64 scancode;
int press_type = 0;
-   int msec;
-   struct timeval t;
-   static struct timeval prev_time = { 0, 0 };
+   long msec;
+   ktime_t t;
+   static ktime_t prev_time;
u8 ktype;
 
/* filter out junk data on the older 0xffdc imon devices */
@@ -1783,10 +1760,10 @@ static void imon_incoming_scancode(struct imon_context 
*ictx,
/* Only panel type events left to process now */
spin_lock_irqsave(>kc_lock, flags);
 
-   do_gettimeofday();
+   t = ktime_get();
/* KEY_MUTE repeats from knob need to be suppressed */
if (ictx->kc == KEY_MUTE && ictx->kc == ictx->last_keycode) {
-   msec = tv2int(, _time);
+   msec = ktime_ms_delta(t, prev_time);
if (msec < ictx->idev->rep[REP_DELAY]) {
spin_unlock_irqrestore(>kc_lock, flags);
return;
-- 
2.9.0

[PATCH] [media] Don't do DMA on stack for firmware upload in the AS102 driver

[Michele Baldessari]

Firmware load on AS102 is using the stack which is not allowed any
longer. We currently fail with:

kernel: transfer buffer not dma capable
kernel: [ cut here ]
kernel: WARNING: CPU: 0 PID: 598 at drivers/usb/core/hcd.c:1595 
usb_hcd_map_urb_for_dma+0x41d/0x620
kernel: Modules linked in: amd64_edac_mod(-) edac_mce_amd as102_fe dvb_as102(+) 
kvm_amd kvm snd_hda_codec_realtek dvb_core snd_hda_codec_generic 
snd_hda_codec_hdmi snd_hda_intel snd_hda_codec irqbypass crct10dif_pclmul 
crc32_pclmul snd_hda_core snd_hwdep snd_seq ghash_clmulni_intel sp5100_tco 
fam15h_power wmi k10temp i2c_piix4 snd_seq_device snd_pcm snd_timer parport_pc 
parport tpm_infineon snd tpm_tis soundcore tpm_tis_core tpm shpchp acpi_cpufreq 
xfs libcrc32c amdgpu amdkfd amd_iommu_v2 radeon hid_logitech_hidpp i2c_algo_bit 
drm_kms_helper crc32c_intel ttm drm r8169 mii hid_logitech_dj
kernel: CPU: 0 PID: 598 Comm: systemd-udevd Not tainted 4.13.10-200.fc26.x86_64 
#1
kernel: Hardware name: ASUS All Series/AM1I-A, BIOS 0505 03/13/2014
kernel: task: 979933b24c80 task.stack: af83413a4000
kernel: RIP: 0010:usb_hcd_map_urb_for_dma+0x41d/0x620
systemd-fsck[659]: /dev/sda2: clean, 49/128016 files, 268609/512000 blocks
kernel: RSP: 0018:af83413a7728 EFLAGS: 00010282
systemd-udevd[604]: link_config: autonegotiation is unset or enabled, the speed 
and duplex are not writable.
kernel: RAX: 001f RBX: 979930bce780 RCX: 
kernel: RDX:  RSI: 97993ec0e118 RDI: 97993ec0e118
kernel: RBP: af83413a7768 R08: 039a R09: 
kernel: R10: 0001 R11:  R12: fff5
kernel: R13: 0140 R14: 0001 R15: 979930806800
kernel: FS:  7effaca5c8c0() GS:97993ec0() 
knlGS:
kernel: CS:  0010 DS:  ES:  CR0: 80050033
kernel: CR2: 7effa9fca962 CR3: 000233089000 CR4: 000406f0
kernel: Call Trace:
kernel:  usb_hcd_submit_urb+0x493/0xb40
kernel:  ? page_cache_tree_insert+0x100/0x100
kernel:  ? xfs_iunlock+0xd5/0x100 [xfs]
kernel:  ? xfs_file_buffered_aio_read+0x57/0xc0 [xfs]
kernel:  usb_submit_urb+0x22d/0x560
kernel:  usb_start_wait_urb+0x6e/0x180
kernel:  usb_bulk_msg+0xb8/0x160
kernel:  as102_send_ep1+0x49/0xe0 [dvb_as102]
kernel:  ? devres_add+0x3f/0x50
kernel:  as102_firmware_upload.isra.0+0x1dc/0x210 [dvb_as102]
kernel:  as102_fw_upload+0xb6/0x1f0 [dvb_as102]
kernel:  as102_dvb_register+0x2af/0x2d0 [dvb_as102]
kernel:  as102_usb_probe+0x1f3/0x260 [dvb_as102]
kernel:  usb_probe_interface+0x124/0x300
kernel:  driver_probe_device+0x2ff/0x450
kernel:  __driver_attach+0xa4/0xe0
kernel:  ? driver_probe_device+0x450/0x450
kernel:  bus_for_each_dev+0x6e/0xb0
kernel:  driver_attach+0x1e/0x20
kernel:  bus_add_driver+0x1c7/0x270
kernel:  driver_register+0x60/0xe0
kernel:  usb_register_driver+0x81/0x150
kernel:  ? 0xc0807000
kernel:  as102_usb_driver_init+0x1e/0x1000 [dvb_as102]
kernel:  do_one_initcall+0x50/0x190
kernel:  ? __vunmap+0x81/0xb0
kernel:  ? kfree+0x154/0x170
kernel:  ? kmem_cache_alloc_trace+0x15f/0x1c0
kernel:  ? do_init_module+0x27/0x1e9
kernel:  do_init_module+0x5f/0x1e9
kernel:  load_module+0x2602/0x2c30
kernel:  SYSC_init_module+0x170/0x1a0
kernel:  ? SYSC_init_module+0x170/0x1a0
kernel:  SyS_init_module+0xe/0x10
kernel:  do_syscall_64+0x67/0x140
kernel:  entry_SYSCALL64_slow_path+0x25/0x25
kernel: RIP: 0033:0x7effab6cf3ea
kernel: RSP: 002b:7fff5cfcbbc8 EFLAGS: 0246 ORIG_RAX: 00af
kernel: RAX: ffda RBX: 5569e0b83760 RCX: 7effab6cf3ea
kernel: RDX: 7effac2099c5 RSI: 9a13 RDI: 5569e0b98c50
kernel: RBP: 7effac2099c5 R08: 5569e0b83ed0 R09: 1d80
kernel: R10: 7effab98db00 R11: 0246 R12: 5569e0b98c50
kernel: R13: 5569e0b81c60 R14: 0002 R15: 5569dfadfdf7
kernel: Code: 48 39 c8 73 30 80 3d 59 60 9d 00 00 41 bc f5 ff ff ff 0f 85 26 ff 
ff ff 48 c7 c7 b8 6b d0 92 c6 05 3f 60 9d 00 01 e8 24 3d ad ff <0f> ff 8b 53 64 
e9 09 ff ff ff 65 48 8b 0c 25 00 d3 00 00 48 8b
kernel: ---[ end trace c4cae366180e70ec ]---
kernel: as10x_usb: error during firmware upload part1

Let's allocate the the structure dynamically so we can get the firmware
loaded correctly:
[   14.243057] as10x_usb: firmware: as102_data1_st.hex loaded with success
[   14.500777] as10x_usb: firmware: as102_data2_st.hex loaded with success

Signed-off-by: Michele Baldessari 
---
 drivers/media/usb/as102/as102_fw.c | 28 +---
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/media/usb/as102/as102_fw.c 
b/drivers/media/usb/as102/as102_fw.c
index 5a28ce3a1d49..38dbc128340d 100644
--- a/drivers/media/usb/as102/as102_fw.c
+++ b/drivers/media/usb/as102/as102_fw.c
@@ -101,18 +101,23 @@ static int as102_firmware_upload(struct 
as10x_bus_adapter_t *bus_adap,
 unsigned 

usb/media/uvc: slab-out-of-bounds in uvc_probe

[Andrey Konovalov]

Hi!

I've got the following report while fuzzing the kernel with syzkaller.

On commit 39dae59d66acd86d1de24294bd2f343fd5e7a625 (4.14-rc8).

It seems that type == UVC_ITT_CAMERA | 0x8000, that's why the (type ==
UVC_ITT_CAMERA) check fails and (UVC_ENTITY_TYPE(term) ==
UVC_ITT_CAMERA) passes, so len ends up being 8 instead of 15.

==
BUG: KASAN: slab-out-of-bounds in uvc_probe+0x6469/0x6dd0
Read of size 2 at addr 88006975864e by task kworker/1:1/33

CPU: 1 PID: 33 Comm: kworker/1:1 Not tainted 4.14.0-rc8-44453-g1fdc1a82c34f #56
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
 __dump_stack lib/dump_stack.c:17
 dump_stack+0xe1/0x157 lib/dump_stack.c:53
 print_address_description+0x71/0x234 mm/kasan/report.c:252
 kasan_report_error mm/kasan/report.c:351
 kasan_report+0x173/0x270 mm/kasan/report.c:409
 __asan_report_load2_noabort+0x19/0x20 mm/kasan/report.c:428
 __le16_to_cpup ./include/uapi/linux/byteorder/little_endian.h:66
 get_unaligned_le16 ./include/linux/unaligned/access_ok.h:10
 uvc_parse_standard_control drivers/media/usb/uvc/uvc_driver.c:1104
 uvc_parse_control drivers/media/usb/uvc/uvc_driver.c:1281
 uvc_probe+0x6469/0x6dd0 drivers/media/usb/uvc/uvc_driver.c:2064
 usb_probe_interface+0x324/0x940 drivers/usb/core/driver.c:361
 really_probe drivers/base/dd.c:413
 driver_probe_device+0x522/0x740 drivers/base/dd.c:557
 __device_attach_driver+0x25d/0x2d0 drivers/base/dd.c:653
 bus_for_each_drv+0xff/0x160 drivers/base/bus.c:463
 __device_attach+0x1a8/0x2a0 drivers/base/dd.c:710
 device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
 bus_probe_device+0x1fc/0x2a0 drivers/base/bus.c:523
 device_add+0xc27/0x15a0 drivers/base/core.c:1835
 usb_set_configuration+0xd4f/0x17a0 drivers/usb/core/message.c:1932
 generic_probe+0xbb/0x120 drivers/usb/core/generic.c:174
 usb_probe_device+0xab/0x100 drivers/usb/core/driver.c:266
 really_probe drivers/base/dd.c:413
 driver_probe_device+0x522/0x740 drivers/base/dd.c:557
 __device_attach_driver+0x25d/0x2d0 drivers/base/dd.c:653
 bus_for_each_drv+0xff/0x160 drivers/base/bus.c:463
 __device_attach+0x1a8/0x2a0 drivers/base/dd.c:710
 device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
 bus_probe_device+0x1fc/0x2a0 drivers/base/bus.c:523
 device_add+0xc27/0x15a0 drivers/base/core.c:1835
 usb_new_device+0x7fa/0x1090 drivers/usb/core/hub.c:2538
 hub_port_connect drivers/usb/core/hub.c:4987
 hub_port_connect_change drivers/usb/core/hub.c:5093
 port_event drivers/usb/core/hub.c:5199
 hub_event_impl+0x17b8/0x3440 drivers/usb/core/hub.c:5311
 hub_event+0x38/0x50 drivers/usb/core/hub.c:5209
 process_one_work+0x925/0x15d0 kernel/workqueue.c:2113
 worker_thread+0xef/0x10d0 kernel/workqueue.c:2247
 kthread+0x346/0x410 kernel/kthread.c:231
 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:432

Allocated by task 33:
 save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
 save_stack+0x43/0xd0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459
 kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:551
 __kmalloc+0x1bc/0x300 mm/slub.c:3783
 kmalloc ./include/linux/slab.h:499
 usb_get_configuration+0x299/0x4e60 drivers/usb/core/config.c:856
 usb_enumerate_device drivers/usb/core/hub.c:2371
 usb_new_device+0xab1/0x1090 drivers/usb/core/hub.c:2507
 hub_port_connect drivers/usb/core/hub.c:4987
 hub_port_connect_change drivers/usb/core/hub.c:5093
 port_event drivers/usb/core/hub.c:5199
 hub_event_impl+0x17b8/0x3440 drivers/usb/core/hub.c:5311
 hub_event+0x38/0x50 drivers/usb/core/hub.c:5209
 process_one_work+0x925/0x15d0 kernel/workqueue.c:2113
 worker_thread+0xef/0x10d0 kernel/workqueue.c:2247
 kthread+0x346/0x410 kernel/kthread.c:231
 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:432

Freed by task 1:
 save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
 save_stack+0x43/0xd0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459
 kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:524
 slab_free_hook mm/slub.c:1391
 slab_free_freelist_hook mm/slub.c:1413
 slab_free mm/slub.c:2989
 kfree+0xf2/0x2e0 mm/slub.c:3920
 kobject_uevent_env+0x249/0xd40 lib/kobject_uevent.c:533
 kobject_uevent+0x1f/0x30 lib/kobject_uevent.c:550
 tty_register_device_attr+0x505/0x650 drivers/tty/tty_io.c:2976
 tty_register_device drivers/tty/tty_io.c:2889
 tty_register_driver+0x3ed/0x770 drivers/tty/tty_io.c:3160
 vty_init+0x337/0x374 drivers/tty/vt/vt.c:3100
 tty_init+0x192/0x197 drivers/tty/tty_io.c:3318
 chr_dev_init+0x14b/0x15d drivers/char/mem.c:921
 do_one_initcall+0x6d/0x177 init/main.c:826
 do_initcall_level init/main.c:892
 do_initcalls init/main.c:900
 do_basic_setup init/main.c:918
 kernel_init_freeable+0x3b5/0x49e init/main.c:1066
 kernel_init+0x16/0x1b7 init/main.c:993
 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:432

The buggy address belongs to the object at 880069758630
 which belongs to the cache kmalloc-32 of size 32
The buggy address is located 30 bytes inside 

usb/media/tm6000: use-after-free in tm6000_read_write_usb

[Andrey Konovalov]

Hi!

I've got the following report while fuzzing the kernel with syzkaller.

On commit 39dae59d66acd86d1de24294bd2f343fd5e7a625 (4.14-rc8).

usb 1-1: USB disconnect, device number 11
tm6000: disconnecting tm6000 #0
xc2028 0-0061: destroying instance
==
BUG: KASAN: use-after-free in tm6000_read_write_usb+0x3cd/0x3f0
Read of size 4 at addr 8800697c4c80 by task v4l_id/5544

CPU: 1 PID: 5544 Comm: v4l_id Not tainted 4.14.0-rc8-44453-g1fdc1a82c34f #56
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:17
 dump_stack+0xe1/0x157 lib/dump_stack.c:53
 print_address_description+0x71/0x234 mm/kasan/report.c:252
 kasan_report_error mm/kasan/report.c:351
 kasan_report+0x173/0x270 mm/kasan/report.c:409
 __asan_report_load4_noabort+0x19/0x20 mm/kasan/report.c:429
 tm6000_read_write_usb+0x3cd/0x3f0 drivers/media/usb/tm6000/tm6000-core.c:48
 tm6000_set_reg+0x3d/0x50 drivers/media/usb/tm6000/tm6000-core.c:113
 tm6000_set_standard+0x7f1/0x13dc drivers/media/usb/tm6000/tm6000-stds.c:574
 tm6000_init_analog_mode+0x232/0x990 drivers/media/usb/tm6000/tm6000-core.c:340
 __tm6000_open drivers/media/usb/tm6000/tm6000-video.c:1373
 tm6000_open+0x409/0x830 drivers/media/usb/tm6000/tm6000-video.c:1406
 v4l2_open+0x1b7/0x380 drivers/media/v4l2-core/v4l2-dev.c:425
 chrdev_open+0x1db/0x520 fs/char_dev.c:417
 do_dentry_open+0x735/0xe20 fs/open.c:752
 vfs_open+0x13e/0x230 fs/open.c:866
 do_last fs/namei.c:3388
 path_openat+0x722/0x2860 fs/namei.c:3528
 do_filp_open+0x13f/0x1d0 fs/namei.c:3563
 do_sys_open+0x362/0x4c0 fs/open.c:1059
 SYSC_open fs/open.c:1077
 SyS_open+0x32/0x40 fs/open.c:1072
 entry_SYSCALL_64_fastpath+0x23/0xc2 arch/x86/entry/entry_64.S:203
RIP: 0033:0x7f10089a9120
RSP: 002b:7ffd20f92098 EFLAGS: 0246 ORIG_RAX: 0002
RAX: ffda RBX: 0046 RCX: 7f10089a9120
RDX: 7f1008c5e138 RSI:  RDI: 7ffd20f93f27
RBP:  R08:  R09: 
R10:  R11: 0246 R12: 00400884
R13: 7ffd20f921f0 R14:  R15: 

Allocated by task 2263:
 save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
 save_stack+0x43/0xd0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459
 kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:551
 kmem_cache_alloc_trace+0x11a/0x290 mm/slub.c:2773
 kmalloc ./include/linux/slab.h:494
 kzalloc ./include/linux/slab.h:667
 usb_alloc_dev+0x3a/0xd86 drivers/usb/core/usb.c:561
 hub_port_connect drivers/usb/core/hub.c:4893
 hub_port_connect_change drivers/usb/core/hub.c:5093
 port_event drivers/usb/core/hub.c:5199
 hub_event_impl+0x124b/0x3440 drivers/usb/core/hub.c:5311
 hub_event+0x38/0x50 drivers/usb/core/hub.c:5209
 process_one_work+0x925/0x15d0 kernel/workqueue.c:2113
 worker_thread+0xef/0x10d0 kernel/workqueue.c:2247
 kthread+0x346/0x410 kernel/kthread.c:231
 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:432

Freed by task 2263:
 save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
 save_stack+0x43/0xd0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459
 kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:524
 slab_free_hook mm/slub.c:1391
 slab_free_freelist_hook mm/slub.c:1413
 slab_free mm/slub.c:2989
 kfree+0xf2/0x2e0 mm/slub.c:3920
 usb_release_dev+0xe3/0x110 drivers/usb/core/usb.c:424
 device_release+0xfc/0x1b0 drivers/base/core.c:812
 kobject_cleanup lib/kobject.c:648
 kobject_release lib/kobject.c:677
 kref_put ./include/linux/kref.h:70
 kobject_put+0x18f/0x240 lib/kobject.c:694
 put_device+0x25/0x30 drivers/base/core.c:1931
 usb_disconnect+0x5de/0x7f0 drivers/usb/core/hub.c:2248
 hub_port_connect drivers/usb/core/hub.c:4838
 hub_port_connect_change drivers/usb/core/hub.c:5093
 port_event drivers/usb/core/hub.c:5199
 hub_event_impl+0x10ec/0x3440 drivers/usb/core/hub.c:5311
 hub_event+0x38/0x50 drivers/usb/core/hub.c:5209
 process_one_work+0x925/0x15d0 kernel/workqueue.c:2113
 worker_thread+0xef/0x10d0 kernel/workqueue.c:2247
 kthread+0x346/0x410 kernel/kthread.c:231
 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:432

The buggy address belongs to the object at 8800697c4c80
 which belongs to the cache kmalloc-2048 of size 2048
The buggy address is located 0 bytes inside of
 2048-byte region [8800697c4c80, 8800697c5480)
The buggy address belongs to the page:
page:ea0001a5f000 count:1 mapcount:0 mapping:  (null)
index:0x0 compound_mapcount: 0
flags: 0x1008100(slab|head)
raw: 01008100   0001000f000f
raw: dead0100 dead0200 88006c402d80 
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 8800697c4b80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 8800697c4c00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>8800697c4c80: fb fb fb fb fb fb fb fb fb fb fb fb 

usb/media/technisat: slab-out-of-bounds in technisat_usb2_rc_query

[Andrey Konovalov]

Hi!

I've got the following report while fuzzing the kernel with syzkaller.

On commit 39dae59d66acd86d1de24294bd2f343fd5e7a625 (4.14-rc8).

It seems that there's no check of the received buffer length in
technisat_usb2_get_ir().

==
BUG: KASAN: slab-out-of-bounds in technisat_usb2_rc_query+0x5a2/0x5c0
Read of size 1 at addr 880064457230 by task kworker/1:2/2650

CPU: 1 PID: 2650 Comm: kworker/1:2 Not tainted
4.14.0-rc8-44453-g1fdc1a82c34f #56
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: events dvb_usb_read_remote_control
Call Trace:
 __dump_stack lib/dump_stack.c:17
 dump_stack+0xe1/0x157 lib/dump_stack.c:53
 print_address_description+0x71/0x234 mm/kasan/report.c:252
 kasan_report_error mm/kasan/report.c:351
 kasan_report+0x173/0x270 mm/kasan/report.c:409
 __asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
 technisat_usb2_get_ir drivers/media/usb/dvb-usb/technisat-usb2.c:663
 technisat_usb2_rc_query+0x5a2/0x5c0
drivers/media/usb/dvb-usb/technisat-usb2.c:678
 dvb_usb_read_remote_control+0xb6/0x150
drivers/media/usb/dvb-usb/dvb-usb-remote.c:261
 process_one_work+0x925/0x15d0 kernel/workqueue.c:2113
 worker_thread+0xef/0x10d0 kernel/workqueue.c:2247
 kthread+0x346/0x410 kernel/kthread.c:231
 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:432

Allocated by task 40:
 save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
 save_stack+0x43/0xd0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459
 kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:551
 __kmalloc+0x1bc/0x300 mm/slub.c:3783
 kmalloc ./include/linux/slab.h:499
 kzalloc ./include/linux/slab.h:667
 dvb_usb_init drivers/media/usb/dvb-usb/dvb-usb-init.c:152
 dvb_usb_device_init.cold.7+0x2d7/0x1029
drivers/media/usb/dvb-usb/dvb-usb-init.c:277
 technisat_usb2_probe+0x36/0x270 drivers/media/usb/dvb-usb/technisat-usb2.c:762
 usb_probe_interface+0x324/0x940 drivers/usb/core/driver.c:361
 really_probe drivers/base/dd.c:413
 driver_probe_device+0x522/0x740 drivers/base/dd.c:557
 __device_attach_driver+0x25d/0x2d0 drivers/base/dd.c:653
 bus_for_each_drv+0xff/0x160 drivers/base/bus.c:463
 __device_attach+0x1a8/0x2a0 drivers/base/dd.c:710
 device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
 bus_probe_device+0x1fc/0x2a0 drivers/base/bus.c:523
 device_add+0xc27/0x15a0 drivers/base/core.c:1835
 usb_set_configuration+0xd4f/0x17a0 drivers/usb/core/message.c:1932
 generic_probe+0xbb/0x120 drivers/usb/core/generic.c:174
 usb_probe_device+0xab/0x100 drivers/usb/core/driver.c:266
 really_probe drivers/base/dd.c:413
 driver_probe_device+0x522/0x740 drivers/base/dd.c:557
 __device_attach_driver+0x25d/0x2d0 drivers/base/dd.c:653
 bus_for_each_drv+0xff/0x160 drivers/base/bus.c:463
 __device_attach+0x1a8/0x2a0 drivers/base/dd.c:710
 device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
 bus_probe_device+0x1fc/0x2a0 drivers/base/bus.c:523
 device_add+0xc27/0x15a0 drivers/base/core.c:1835
 usb_new_device+0x7fa/0x1090 drivers/usb/core/hub.c:2538
 hub_port_connect drivers/usb/core/hub.c:4987
 hub_port_connect_change drivers/usb/core/hub.c:5093
 port_event drivers/usb/core/hub.c:5199
 hub_event_impl+0x17b8/0x3440 drivers/usb/core/hub.c:5311
 hub_event+0x38/0x50 drivers/usb/core/hub.c:5209
 process_one_work+0x925/0x15d0 kernel/workqueue.c:2113
 worker_thread+0xef/0x10d0 kernel/workqueue.c:2247
 kthread+0x346/0x410 kernel/kthread.c:231
 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:432

Freed by task 5251:
 save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
 save_stack+0x43/0xd0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459
 kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:524
 slab_free_hook mm/slub.c:1391
 slab_free_freelist_hook mm/slub.c:1413
 slab_free mm/slub.c:2989
 kfree+0xf2/0x2e0 mm/slub.c:3920
 seq_release fs/seq_file.c:366
 single_release+0x85/0xb0 fs/seq_file.c:602
 close_pdeo.part.1+0xe6/0x2e0 fs/proc/inode.c:165
 close_pdeo+0xd9/0x100 fs/proc/inode.c:173
 proc_reg_release+0x130/0x170 fs/proc/inode.c:376
 __fput+0x2b6/0x730 fs/file_table.c:210
 fput+0x1a/0x20 fs/file_table.c:244
 task_work_run+0x13d/0x1b0 kernel/task_work.c:113
 tracehook_notify_resume ./include/linux/tracehook.h:191
 exit_to_usermode_loop+0xb9/0x190 arch/x86/entry/common.c:162
 prepare_exit_to_usermode arch/x86/entry/common.c:197
 syscall_return_slowpath+0x21a/0x260 arch/x86/entry/common.c:266
 entry_SYSCALL_64_fastpath+0xc0/0xc2 arch/x86/entry/entry_64.S:239

The buggy address belongs to the object at 880064457140
 which belongs to the cache kmalloc-256 of size 256
The buggy address is located 240 bytes inside of
 256-byte region [880064457140, 880064457240)
The buggy address belongs to the page:
page:ea00019115c0 count:1 mapcount:0 mapping:  (null) index:0x0
flags: 0x1000100(slab)
raw: 01000100   0001000c000c
raw: ea000187d640 00060006 88006c403200 
page 

Re: [PATCH] vimc: add test_pattern and h/vflip controls to the sensor

[Helen Koike]

Hi Hans,

On 2017-11-06 08:19 AM, Hans Verkuil wrote:
> Hi Helen,
> 
> On 09/27/2017 08:30 PM, Helen Koike wrote:
>> Hi Hans,
>>
>> Thanks for your patch and sorry for my late reply.
> 
> Sorry for my late reply to your reply :-)
> 
>> Please see my comments and questions below
>>
>> On 2017-07-28 07:23 AM, Hans Verkuil wrote:
>>> Add support for the test_pattern control and the h/vflip controls.
>>>
>>> This makes it possible to switch to more interesting test patterns and to
>>> test control handling in v4l-subdevs.
>>>
>>> There are more tpg-related controls that can be added, but this is a good
>>> start.
>>>
>>> Signed-off-by: Hans Verkuil 
>>> ---
>>> diff --git a/drivers/media/platform/vimc/vimc-common.h 
>>> b/drivers/media/platform/vimc/vimc-common.h
>>> index dca528a316e7..2e9981b18166 100644
>>> --- a/drivers/media/platform/vimc/vimc-common.h
>>> +++ b/drivers/media/platform/vimc/vimc-common.h
>>> @@ -22,6 +22,11 @@
>>>  #include 
>>>  #include 
>>>
>>> +/* VIMC-specific controls */
>>> +#define VIMC_CID_VIMC_BASE (0x00f0 | 0xf000)
>>> +#define VIMC_CID_VIMC_CLASS(0x00f0 | 1)
>>
>> Why this values, shouldn't we use a derivative from
>> V4L2_CID_PRIVATE_BASE for custom controls? Or can we use random values?
> 
> The values are taken from vivid which uses the same scheme. These controls
> deal with how the virtual driver emulates things, and I prefer not to make
> these control IDs part of the public API so we can be a bit more flexible in
> the future. It's a design choice which worked well for vivid.
> 
>>
>>> +#define VIMC_CID_TEST_PATTERN  (VIMC_CID_VIMC_BASE + 0)
>>> +
>>>  #define VIMC_FRAME_MAX_WIDTH 4096
>>>  #define VIMC_FRAME_MAX_HEIGHT 2160
>>>  #define VIMC_FRAME_MIN_WIDTH 16
>>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c 
>>> b/drivers/media/platform/vimc/vimc-sensor.c
>>> index 615c2b18dcfc..532097566b27 100644
>>> --- a/drivers/media/platform/vimc/vimc-sensor.c
>>> +++ b/drivers/media/platform/vimc/vimc-sensor.c
>>> @@ -22,6 +22,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>>
>>> @@ -38,6 +39,7 @@ struct vimc_sen_device {
>>> u8 *frame;
>>> /* The active format */
>>> struct v4l2_mbus_framefmt mbus_format;
>>> +   struct v4l2_ctrl_handler hdl;
>>>  };
>>>
>>>  static const struct v4l2_mbus_framefmt fmt_default = {
>>> @@ -291,6 +293,31 @@ static const struct v4l2_subdev_ops vimc_sen_ops = {
>>> .video = _sen_video_ops,
>>>  };
>>>
>>> +static int vimc_sen_s_ctrl(struct v4l2_ctrl *ctrl)
>>> +{
>>> +   struct vimc_sen_device *vsen =
>>> +   container_of(ctrl->handler, struct vimc_sen_device, hdl);
>>> +
>>> +   switch (ctrl->id) {
>>> +   case VIMC_CID_TEST_PATTERN:
>>> +   tpg_s_pattern(>tpg, ctrl->val);
>>> +   break;
>>> +   case V4L2_CID_HFLIP:
>>> +   tpg_s_hflip(>tpg, ctrl->val);
>>> +   break;
>>> +   case V4L2_CID_VFLIP:
>>> +   tpg_s_vflip(>tpg, ctrl->val);
>>> +   break;
>>> +   default:
>>> +   return -EINVAL;
>>> +   }
>>> +   return 0;
>>> +}
>>> +
>>> +static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
>>> +   .s_ctrl = vimc_sen_s_ctrl,
>>> +};
>>> +
>>>  static void vimc_sen_comp_unbind(struct device *comp, struct device 
>>> *master,
>>>  void *master_data)
>>>  {
>>> @@ -299,10 +326,29 @@ static void vimc_sen_comp_unbind(struct device *comp, 
>>> struct device *master,
>>> container_of(ved, struct vimc_sen_device, ved);
>>>
>>> vimc_ent_sd_unregister(ved, >sd);
>>> +   v4l2_ctrl_handler_free(>hdl);
>>> tpg_free(>tpg);
>>> kfree(vsen);
>>>  }
>>>
>>> +/* Image Processing Controls */
>>> +static const struct v4l2_ctrl_config vimc_sen_ctrl_class = {
>>> +   .ops = _sen_ctrl_ops,
>>> +   .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY,
>>
>> I was wondering if it is really necessary to specify the ops and flags
>> in the class, as this is seems to me a meta control for the other
>> controls to be grouped in a class.
> 
> ops can be dropped, but flags needs to be there.
> 
>>
>>> +   .id = VIMC_CID_VIMC_CLASS,
>>> +   .name = "VIMC Controls",
>>> +   .type = V4L2_CTRL_TYPE_CTRL_CLASS,
>>> +};
>>> +
>>> +static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = {
>>> +   .ops = _sen_ctrl_ops,
>>> +   .id = VIMC_CID_TEST_PATTERN,
>>> +   .name = "Test Pattern",
>>> +   .type = V4L2_CTRL_TYPE_MENU,
>>> +   .max = TPG_PAT_NOISE,
>>> +   .qmenu = tpg_pattern_strings,
>>> +};
>>> +
>>>  static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>>   void *master_data)
>>>  {
>>> @@ -316,6 +362,20 @@ static int vimc_sen_comp_bind(struct device *comp, 
>>> struct device *master,
>>> if (!vsen)
>>> return -ENOMEM;
>>>
>>> +   v4l2_ctrl_handler_init(>hdl, 4);
>>> +
>>> +   v4l2_ctrl_new_custom(>hdl, _sen_ctrl_class, NULL);
>>> 

Re: broken build against 4.4.0

[Vincent McIntyre]

On Sun, Nov 05, 2017 at 01:55:22PM +0100, Jasmin J. wrote:
> Hello Vincent!
> 
> > can someone take a look please?
> Well, Matthias may have fixed that (I didn't try).
> 
> See:
>   https://www.mail-archive.com/linux-media@vger.kernel.org/msg121610.html
> 
> Maybe you need that also:
>   https://www.mail-archive.com/linux-media@vger.kernel.org/msg121625.html
> 

It seems Hans applied this a couple of hours ago - thank you Hans!
The build completes just fine now.

Cheers
Vince

[PATCH 2/5] media: rc: include rather than

[Sean Young]

This removes the need for include/media/lirc.h, which just includes
the uapi file.

Signed-off-by: Sean Young 
---
 drivers/media/rc/lirc_dev.c | 2 +-
 include/media/lirc.h| 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)
 delete mode 100644 include/media/lirc.h

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 5c0e6a3ea3d4..24e0c56c9892 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -26,7 +26,7 @@
 #include 
 
 #include "rc-core-priv.h"
-#include 
+#include 
 
 #define LOGHEAD"lirc_dev (%s[%d]): "
 #define LIRCBUF_SIZE   256
diff --git a/include/media/lirc.h b/include/media/lirc.h
deleted file mode 100644
index 554988c860c1..
--- a/include/media/lirc.h
+++ /dev/null
@@ -1 +0,0 @@
-#include 
-- 
2.13.6

[PATCH 0/5] lirc improvements

[Sean Young]

This patch series depend on the lirc scancode and lirc kapi removal
series.

This implements correct locking for lirc and allows the chardev to be
opened more than once.

Sean Young (5):
  media: rc: move ir-lirc-codec.c contents into lirc_dev.c
  media: rc: include  rather than 
  media: lirc: allow lirc device to opened more than once
  media: lirc: improve locking
  media: rc: iguanair: remove unnecessary locking

 drivers/media/rc/Makefile|   2 +-
 drivers/media/rc/iguanair.c  |  28 --
 drivers/media/rc/ir-lirc-codec.c | 637 
 drivers/media/rc/lirc_dev.c  | 771 ---
 drivers/media/rc/rc-core-priv.h  |   2 -
 include/media/lirc.h |   1 -
 include/media/rc-core.h  |  56 +--
 7 files changed, 760 insertions(+), 737 deletions(-)
 delete mode 100644 drivers/media/rc/ir-lirc-codec.c
 delete mode 100644 include/media/lirc.h

-- 
2.13.6

[PATCH 1/5] media: rc: move ir-lirc-codec.c contents into lirc_dev.c

[Sean Young]

Since removing the lirc kapi, ir-lirc-codec.c only contains lirc fops
so the file name is no longer correct. By moving its content into
lirc_dev.c the ugly extern struct lirc_fops is not longer needed,
and everything lirc related is in one file.

Signed-off-by: Sean Young 
---
 drivers/media/rc/Makefile|   2 +-
 drivers/media/rc/ir-lirc-codec.c | 637 ---
 drivers/media/rc/lirc_dev.c  | 618 +
 drivers/media/rc/rc-core-priv.h  |   2 -
 4 files changed, 619 insertions(+), 640 deletions(-)
 delete mode 100644 drivers/media/rc/ir-lirc-codec.c

diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index a1ef86767aef..9d487988b78d 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -3,7 +3,7 @@ obj-y += keymaps/
 
 obj-$(CONFIG_RC_CORE) += rc-core.o
 rc-core-y := rc-main.o rc-ir-raw.o
-rc-core-$(CONFIG_LIRC) += lirc_dev.o ir-lirc-codec.o
+rc-core-$(CONFIG_LIRC) += lirc_dev.o
 obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
 obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
 obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
deleted file mode 100644
index 78934479bcff..
--- a/drivers/media/rc/ir-lirc-codec.c
+++ /dev/null
@@ -1,637 +0,0 @@
-/* ir-lirc-codec.c - rc-core to classic lirc interface bridge
- *
- * Copyright (C) 2010 by Jarod Wilson 
- *
- * This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation version 2 of the License.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include "rc-core-priv.h"
-
-#define LIRCBUF_SIZE 256
-
-/**
- * ir_lirc_raw_event() - Send raw IR data to lirc to be relayed to userspace
- *
- * @dev:   the struct rc_dev descriptor of the device
- * @ev:the struct ir_raw_event descriptor of the pulse/space
- */
-void ir_lirc_raw_event(struct rc_dev *dev, struct ir_raw_event ev)
-{
-   int sample;
-
-   /* Packet start */
-   if (ev.reset) {
-   /* Userspace expects a long space event before the start of
-* the signal to use as a sync.  This may be done with repeat
-* packets and normal samples.  But if a reset has been sent
-* then we assume that a long time has passed, so we send a
-* space with the maximum time value. */
-   sample = LIRC_SPACE(LIRC_VALUE_MASK);
-   IR_dprintk(2, "delivering reset sync space to lirc_dev\n");
-
-   /* Carrier reports */
-   } else if (ev.carrier_report) {
-   sample = LIRC_FREQUENCY(ev.carrier);
-   IR_dprintk(2, "carrier report (freq: %d)\n", sample);
-
-   /* Packet end */
-   } else if (ev.timeout) {
-
-   if (dev->gap)
-   return;
-
-   dev->gap_start = ktime_get();
-   dev->gap = true;
-   dev->gap_duration = ev.duration;
-
-   if (!dev->send_timeout_reports)
-   return;
-
-   sample = LIRC_TIMEOUT(ev.duration / 1000);
-   IR_dprintk(2, "timeout report (duration: %d)\n", sample);
-
-   /* Normal sample */
-   } else {
-
-   if (dev->gap) {
-   dev->gap_duration += ktime_to_ns(ktime_sub(ktime_get(),
-dev->gap_start));
-
-   /* Convert to ms and cap by LIRC_VALUE_MASK */
-   do_div(dev->gap_duration, 1000);
-   dev->gap_duration = min_t(u64, dev->gap_duration,
- LIRC_VALUE_MASK);
-
-   kfifo_put(>rawir, LIRC_SPACE(dev->gap_duration));
-   dev->gap = false;
-   }
-
-   sample = ev.pulse ? LIRC_PULSE(ev.duration / 1000) :
-   LIRC_SPACE(ev.duration / 1000);
-   IR_dprintk(2, "delivering %uus %s to lirc_dev\n",
-  TO_US(ev.duration), TO_STR(ev.pulse));
-   }
-
-   kfifo_put(>rawir, sample);
-   wake_up_poll(>wait_poll, POLLIN | POLLRDNORM);
-}
-
-/**
- * ir_lirc_scancode_event() - Send scancode data to lirc to be relayed to
- * userspace
- * @dev:   the struct rc_dev descriptor of the device
- * @lscthe struct lirc_scancode describing the decoded scancode
- */
-void ir_lirc_scancode_event(struct rc_dev *dev, struct lirc_scancode *lsc)
-{
-   

[PATCH 3/5] media: lirc: allow lirc device to opened more than once

[Sean Young]

This makes it possible for lircd to read from a lirc chardev, and not
keep it busy.

Note that this changes the default for timeout reports to on.

Signed-off-by: Sean Young 
---
 drivers/media/rc/lirc_dev.c | 280 
 include/media/rc-core.h |  56 +
 2 files changed, 185 insertions(+), 151 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 24e0c56c9892..32beecf103cc 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -28,7 +28,6 @@
 #include "rc-core-priv.h"
 #include 
 
-#define LOGHEAD"lirc_dev (%s[%d]): "
 #define LIRCBUF_SIZE   256
 
 static dev_t lirc_base_dev;
@@ -47,6 +46,9 @@ static struct class *lirc_class;
  */
 void ir_lirc_raw_event(struct rc_dev *dev, struct ir_raw_event ev)
 {
+   unsigned long flags;
+   struct list_head *l;
+   struct lirc_fh *fh;
int sample;
 
/* Packet start */
@@ -75,9 +77,6 @@ void ir_lirc_raw_event(struct rc_dev *dev, struct 
ir_raw_event ev)
dev->gap = true;
dev->gap_duration = ev.duration;
 
-   if (!dev->send_timeout_reports)
-   return;
-
sample = LIRC_TIMEOUT(ev.duration / 1000);
IR_dprintk(2, "timeout report (duration: %d)\n", sample);
 
@@ -92,7 +91,13 @@ void ir_lirc_raw_event(struct rc_dev *dev, struct 
ir_raw_event ev)
dev->gap_duration = min_t(u64, dev->gap_duration,
  LIRC_VALUE_MASK);
 
-   kfifo_put(>rawir, LIRC_SPACE(dev->gap_duration));
+   spin_lock_irqsave(>lirc_fh_lock, flags);
+   list_for_each(l, >lirc_fh) {
+   fh = list_entry(l, struct lirc_fh, list);
+   kfifo_put(>rawir,
+ LIRC_SPACE(dev->gap_duration));
+   }
+   spin_unlock_irqrestore(>lirc_fh_lock, flags);
dev->gap = false;
}
 
@@ -102,22 +107,38 @@ void ir_lirc_raw_event(struct rc_dev *dev, struct 
ir_raw_event ev)
   TO_US(ev.duration), TO_STR(ev.pulse));
}
 
-   kfifo_put(>rawir, sample);
-   wake_up_poll(>wait_poll, POLLIN | POLLRDNORM);
+   spin_lock_irqsave(>lirc_fh_lock, flags);
+   list_for_each(l, >lirc_fh) {
+   fh = list_entry(l, struct lirc_fh, list);
+   if (LIRC_IS_TIMEOUT(sample) && !fh->send_timeout_reports)
+   continue;
+   if (kfifo_put(>rawir, sample))
+   wake_up_poll(>wait_poll, POLLIN | POLLRDNORM);
+   }
+   spin_unlock_irqrestore(>lirc_fh_lock, flags);
 }
 
 /**
  * ir_lirc_scancode_event() - Send scancode data to lirc to be relayed to
- * userspace
+ * userspace. This can be called in atomic context.
  * @dev:   the struct rc_dev descriptor of the device
  * @lscthe struct lirc_scancode describing the decoded scancode
  */
 void ir_lirc_scancode_event(struct rc_dev *dev, struct lirc_scancode *lsc)
 {
+   unsigned long flags;
+   struct list_head *l;
+   struct lirc_fh *fh;
+
lsc->timestamp = ktime_get_ns();
 
-   if (kfifo_put(>scancodes, *lsc))
-   wake_up_poll(>wait_poll, POLLIN | POLLRDNORM);
+   spin_lock_irqsave(>lirc_fh_lock, flags);
+   list_for_each(l, >lirc_fh) {
+   fh = list_entry(l, struct lirc_fh, list);
+   if (kfifo_put(>scancodes, *lsc))
+   wake_up_poll(>wait_poll, POLLIN | POLLRDNORM);
+   }
+   spin_unlock_irqrestore(>lirc_fh_lock, flags);
 }
 EXPORT_SYMBOL_GPL(ir_lirc_scancode_event);
 
@@ -125,55 +146,89 @@ static int ir_lirc_open(struct inode *inode, struct file 
*file)
 {
struct rc_dev *dev = container_of(inode->i_cdev, struct rc_dev,
  lirc_cdev);
+   struct lirc_fh *fh = kzalloc(sizeof(*fh), GFP_KERNEL);
+   unsigned long flags;
int retval;
 
-   retval = rc_open(dev);
-   if (retval)
-   return retval;
-
-   retval = mutex_lock_interruptible(>lock);
-   if (retval)
-   goto out_rc;
+   if (!fh)
+   return -ENOMEM;
 
if (!dev->registered) {
retval = -ENODEV;
-   goto out_unlock;
+   goto out_fh;
}
 
-   if (dev->lirc_open) {
-   retval = -EBUSY;
-   goto out_unlock;
+   if (dev->driver_type == RC_DRIVER_IR_RAW) {
+   if (kfifo_alloc(>rawir, MAX_IR_EVENT_SIZE, GFP_KERNEL)) {
+   retval = -ENOMEM;
+   goto out_fh;
+   }
}
 
-   if (dev->driver_type == RC_DRIVER_IR_RAW)
-   kfifo_reset_out(>rawir);
-   if 

[PATCH 5/5] media: rc: iguanair: remove unnecessary locking

[Sean Young]

Since lirc now correctly locks the rcdev, this locking is no longer
needed.

Signed-off-by: Sean Young 
---
 drivers/media/rc/iguanair.c | 28 
 1 file changed, 28 deletions(-)

diff --git a/drivers/media/rc/iguanair.c b/drivers/media/rc/iguanair.c
index 30e24da67226..64231efcc47a 100644
--- a/drivers/media/rc/iguanair.c
+++ b/drivers/media/rc/iguanair.c
@@ -36,8 +36,6 @@ struct iguanair {
uint8_t bufsize;
uint8_t cycle_overhead;
 
-   struct mutex lock;
-
/* receiver support */
bool receiver_on;
dma_addr_t dma_in, dma_out;
@@ -295,8 +293,6 @@ static int iguanair_set_tx_carrier(struct rc_dev *dev, 
uint32_t carrier)
if (carrier < 25000 || carrier > 15)
return -EINVAL;
 
-   mutex_lock(>lock);
-
if (carrier != ir->carrier) {
uint32_t cycles, fours, sevens;
 
@@ -325,8 +321,6 @@ static int iguanair_set_tx_carrier(struct rc_dev *dev, 
uint32_t carrier)
ir->packet->busy4 = 110 - fours;
}
 
-   mutex_unlock(>lock);
-
return 0;
 }
 
@@ -337,9 +331,7 @@ static int iguanair_set_tx_mask(struct rc_dev *dev, 
uint32_t mask)
if (mask > 15)
return 4;
 
-   mutex_lock(>lock);
ir->packet->channels = mask << 4;
-   mutex_unlock(>lock);
 
return 0;
 }
@@ -351,7 +343,6 @@ static int iguanair_tx(struct rc_dev *dev, unsigned *txbuf, 
unsigned count)
unsigned i, size, periods, bytes;
int rc;
 
-   mutex_lock(>lock);
 
/* convert from us to carrier periods */
for (i = space = size = 0; i < count; i++) {
@@ -382,8 +373,6 @@ static int iguanair_tx(struct rc_dev *dev, unsigned *txbuf, 
unsigned count)
rc = -EOVERFLOW;
 
 out:
-   mutex_unlock(>lock);
-
return rc ? rc : count;
 }
 
@@ -392,14 +381,10 @@ static int iguanair_open(struct rc_dev *rdev)
struct iguanair *ir = rdev->priv;
int rc;
 
-   mutex_lock(>lock);
-
rc = iguanair_receiver(ir, true);
if (rc == 0)
ir->receiver_on = true;
 
-   mutex_unlock(>lock);
-
return rc;
 }
 
@@ -408,14 +393,10 @@ static void iguanair_close(struct rc_dev *rdev)
struct iguanair *ir = rdev->priv;
int rc;
 
-   mutex_lock(>lock);
-
rc = iguanair_receiver(ir, false);
ir->receiver_on = false;
if (rc && rc != -ENODEV)
dev_warn(ir->dev, "failed to disable receiver: %d\n", rc);
-
-   mutex_unlock(>lock);
 }
 
 static int iguanair_probe(struct usb_interface *intf,
@@ -456,7 +437,6 @@ static int iguanair_probe(struct usb_interface *intf,
ir->rc = rc;
ir->dev = >dev;
ir->udev = udev;
-   mutex_init(>lock);
 
init_completion(>completion);
pipeout = usb_sndintpipe(udev,
@@ -553,8 +533,6 @@ static int iguanair_suspend(struct usb_interface *intf, 
pm_message_t message)
struct iguanair *ir = usb_get_intfdata(intf);
int rc = 0;
 
-   mutex_lock(>lock);
-
if (ir->receiver_on) {
rc = iguanair_receiver(ir, false);
if (rc)
@@ -564,8 +542,6 @@ static int iguanair_suspend(struct usb_interface *intf, 
pm_message_t message)
usb_kill_urb(ir->urb_in);
usb_kill_urb(ir->urb_out);
 
-   mutex_unlock(>lock);
-
return rc;
 }
 
@@ -574,8 +550,6 @@ static int iguanair_resume(struct usb_interface *intf)
struct iguanair *ir = usb_get_intfdata(intf);
int rc = 0;
 
-   mutex_lock(>lock);
-
rc = usb_submit_urb(ir->urb_in, GFP_KERNEL);
if (rc)
dev_warn(>dev, "failed to submit urb: %d\n", rc);
@@ -586,8 +560,6 @@ static int iguanair_resume(struct usb_interface *intf)
dev_warn(ir->dev, "failed to enable receiver after 
resume\n");
}
 
-   mutex_unlock(>lock);
-
return rc;
 }
 
-- 
2.13.6

[PATCH 4/5] media: lirc: improve locking

[Sean Young]

Once rc_unregister_device() has been called, no driver function
should be called.

Signed-off-by: Sean Young 
---
 drivers/media/rc/lirc_dev.c | 255 +---
 1 file changed, 147 insertions(+), 108 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 32beecf103cc..6b0053d4f041 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -240,15 +240,21 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
const char __user *buf,
struct rc_dev *dev = fh->rc;
unsigned int *txbuf = NULL;
struct ir_raw_event *raw = NULL;
-   ssize_t ret = -EINVAL;
+   ssize_t ret;
size_t count;
ktime_t start;
s64 towait;
unsigned int duration = 0; /* signal duration in us */
int i;
 
-   if (!dev->registered)
-   return -ENODEV;
+   ret = mutex_lock_interruptible(>lock);
+   if (ret)
+   return ret;
+
+   if (!dev->registered) {
+   ret = -ENODEV;
+   goto out;
+   }
 
start = ktime_get();
 
@@ -260,14 +266,20 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
const char __user *buf,
if (fh->send_mode == LIRC_MODE_SCANCODE) {
struct lirc_scancode scan;
 
-   if (n != sizeof(scan))
-   return -EINVAL;
+   if (n != sizeof(scan)) {
+   ret = -EINVAL;
+   goto out;
+   }
 
-   if (copy_from_user(, buf, sizeof(scan)))
-   return -EFAULT;
+   if (copy_from_user(, buf, sizeof(scan))) {
+   ret = -EFAULT;
+   goto out;
+   }
 
-   if (scan.flags || scan.keycode || scan.timestamp)
-   return -EINVAL;
+   if (scan.flags || scan.keycode || scan.timestamp) {
+   ret = -EINVAL;
+   goto out;
+   }
 
/*
 * The scancode field in lirc_scancode is 64-bit simply
@@ -276,12 +288,16 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
const char __user *buf,
 * are supported.
 */
if (scan.scancode > U32_MAX ||
-   !rc_validate_scancode(scan.rc_proto, scan.scancode))
-   return -EINVAL;
+   !rc_validate_scancode(scan.rc_proto, scan.scancode)) {
+   ret = -EINVAL;
+   goto out;
+   }
 
raw = kmalloc_array(LIRCBUF_SIZE, sizeof(*raw), GFP_KERNEL);
-   if (!raw)
-   return -ENOMEM;
+   if (!raw) {
+   ret = -ENOMEM;
+   goto out;
+   }
 
ret = ir_raw_encode_scancode(scan.rc_proto, scan.scancode,
 raw, LIRCBUF_SIZE);
@@ -307,16 +323,22 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
const char __user *buf,
dev->s_tx_carrier(dev, carrier);
}
} else {
-   if (n < sizeof(unsigned int) || n % sizeof(unsigned int))
-   return -EINVAL;
+   if (n < sizeof(unsigned int) || n % sizeof(unsigned int)) {
+   ret = -EINVAL;
+   goto out;
+   }
 
count = n / sizeof(unsigned int);
-   if (count > LIRCBUF_SIZE || count % 2 == 0)
-   return -EINVAL;
+   if (count > LIRCBUF_SIZE || count % 2 == 0) {
+   ret = -EINVAL;
+   goto out;
+   }
 
txbuf = memdup_user(buf, n);
-   if (IS_ERR(txbuf))
-   return PTR_ERR(txbuf);
+   if (IS_ERR(txbuf)) {
+   ret = PTR_ERR(txbuf);
+   goto out;
+   }
}
 
for (i = 0; i < count; i++) {
@@ -354,6 +376,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const 
char __user *buf,
}
 
 out:
+   mutex_unlock(>lock);
kfree(txbuf);
kfree(raw);
return ret;
@@ -365,8 +388,8 @@ static long ir_lirc_ioctl(struct file *file, unsigned int 
cmd,
struct lirc_fh *fh = file->private_data;
struct rc_dev *dev = fh->rc;
u32 __user *argp = (u32 __user *)(arg);
-   int ret = 0;
-   __u32 val = 0, tmp;
+   u32 val = 0;
+   int ret;
 
if (_IOC_DIR(cmd) & _IOC_WRITE) {
ret = get_user(val, argp);
@@ -374,8 +397,14 @@ static long ir_lirc_ioctl(struct file *file, unsigned int 
cmd,
return ret;
}
 
-   if (!dev->registered)
-   return -ENODEV;
+   ret = mutex_lock_interruptible(>lock);
+   

[PATCHv2] vimc: add test_pattern and h/vflip controls to the sensor

[Hans Verkuil]

Add support for the test_pattern control and the h/vflip controls.

This makes it possible to switch to more interesting test patterns and to
test control handling in v4l-subdevs.

There are more tpg-related controls that can be added, but this is a good
start.

Signed-off-by: Hans Verkuil 
---
Changes since v2:
- dropped ops field for the custom vimc control class, it's not needed.
---
 drivers/media/platform/vimc/vimc-common.h |  5 +++
 drivers/media/platform/vimc/vimc-sensor.c | 63 ++-
 2 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vimc/vimc-common.h 
b/drivers/media/platform/vimc/vimc-common.h
index dca528a316e7..2e9981b18166 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -22,6 +22,11 @@
 #include 
 #include 

+/* VIMC-specific controls */
+#define VIMC_CID_VIMC_BASE (0x00f0 | 0xf000)
+#define VIMC_CID_VIMC_CLASS(0x00f0 | 1)
+#define VIMC_CID_TEST_PATTERN  (VIMC_CID_VIMC_BASE + 0)
+
 #define VIMC_FRAME_MAX_WIDTH 4096
 #define VIMC_FRAME_MAX_HEIGHT 2160
 #define VIMC_FRAME_MIN_WIDTH 16
diff --git a/drivers/media/platform/vimc/vimc-sensor.c 
b/drivers/media/platform/vimc/vimc-sensor.c
index 02e68c8fc02b..1ab52a821b8e 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 

@@ -38,6 +39,7 @@ struct vimc_sen_device {
u8 *frame;
/* The active format */
struct v4l2_mbus_framefmt mbus_format;
+   struct v4l2_ctrl_handler hdl;
 };

 static const struct v4l2_mbus_framefmt fmt_default = {
@@ -291,6 +293,31 @@ static const struct v4l2_subdev_ops vimc_sen_ops = {
.video = _sen_video_ops,
 };

+static int vimc_sen_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct vimc_sen_device *vsen =
+   container_of(ctrl->handler, struct vimc_sen_device, hdl);
+
+   switch (ctrl->id) {
+   case VIMC_CID_TEST_PATTERN:
+   tpg_s_pattern(>tpg, ctrl->val);
+   break;
+   case V4L2_CID_HFLIP:
+   tpg_s_hflip(>tpg, ctrl->val);
+   break;
+   case V4L2_CID_VFLIP:
+   tpg_s_vflip(>tpg, ctrl->val);
+   break;
+   default:
+   return -EINVAL;
+   }
+   return 0;
+}
+
+static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
+   .s_ctrl = vimc_sen_s_ctrl,
+};
+
 static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
 void *master_data)
 {
@@ -299,10 +326,28 @@ static void vimc_sen_comp_unbind(struct device *comp, 
struct device *master,
container_of(ved, struct vimc_sen_device, ved);

vimc_ent_sd_unregister(ved, >sd);
+   v4l2_ctrl_handler_free(>hdl);
tpg_free(>tpg);
kfree(vsen);
 }

+/* Image Processing Controls */
+static const struct v4l2_ctrl_config vimc_sen_ctrl_class = {
+   .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY,
+   .id = VIMC_CID_VIMC_CLASS,
+   .name = "VIMC Controls",
+   .type = V4L2_CTRL_TYPE_CTRL_CLASS,
+};
+
+static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = {
+   .ops = _sen_ctrl_ops,
+   .id = VIMC_CID_TEST_PATTERN,
+   .name = "Test Pattern",
+   .type = V4L2_CTRL_TYPE_MENU,
+   .max = TPG_PAT_NOISE,
+   .qmenu = tpg_pattern_strings,
+};
+
 static int vimc_sen_comp_bind(struct device *comp, struct device *master,
  void *master_data)
 {
@@ -316,6 +361,20 @@ static int vimc_sen_comp_bind(struct device *comp, struct 
device *master,
if (!vsen)
return -ENOMEM;

+   v4l2_ctrl_handler_init(>hdl, 4);
+
+   v4l2_ctrl_new_custom(>hdl, _sen_ctrl_class, NULL);
+   v4l2_ctrl_new_custom(>hdl, _sen_ctrl_test_pattern, NULL);
+   v4l2_ctrl_new_std(>hdl, _sen_ctrl_ops,
+   V4L2_CID_VFLIP, 0, 1, 1, 0);
+   v4l2_ctrl_new_std(>hdl, _sen_ctrl_ops,
+   V4L2_CID_HFLIP, 0, 1, 1, 0);
+   vsen->sd.ctrl_handler = >hdl;
+   if (vsen->hdl.error) {
+   ret = vsen->hdl.error;
+   goto err_free_vsen;
+   }
+
/* Initialize ved and sd */
ret = vimc_ent_sd_register(>ved, >sd, v4l2_dev,
   pdata->entity_name,
@@ -323,7 +382,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct 
device *master,
   (const unsigned long[1]) 
{MEDIA_PAD_FL_SOURCE},
   _sen_ops);
if (ret)
-   goto err_free_vsen;
+   goto err_free_hdl;

dev_set_drvdata(comp, >ved);
vsen->dev = comp;
@@ -342,6 +401,8 @@ static int vimc_sen_comp_bind(struct device *comp, struct 
device *master,

 err_unregister_ent_sd:
  

Re: [PATCH] vimc: add test_pattern and h/vflip controls to the sensor

[Hans Verkuil]

Hi Helen,

On 09/27/2017 08:30 PM, Helen Koike wrote:
> Hi Hans,
> 
> Thanks for your patch and sorry for my late reply.

Sorry for my late reply to your reply :-)

> Please see my comments and questions below
> 
> On 2017-07-28 07:23 AM, Hans Verkuil wrote:
>> Add support for the test_pattern control and the h/vflip controls.
>>
>> This makes it possible to switch to more interesting test patterns and to
>> test control handling in v4l-subdevs.
>>
>> There are more tpg-related controls that can be added, but this is a good
>> start.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>> diff --git a/drivers/media/platform/vimc/vimc-common.h 
>> b/drivers/media/platform/vimc/vimc-common.h
>> index dca528a316e7..2e9981b18166 100644
>> --- a/drivers/media/platform/vimc/vimc-common.h
>> +++ b/drivers/media/platform/vimc/vimc-common.h
>> @@ -22,6 +22,11 @@
>>  #include 
>>  #include 
>>
>> +/* VIMC-specific controls */
>> +#define VIMC_CID_VIMC_BASE  (0x00f0 | 0xf000)
>> +#define VIMC_CID_VIMC_CLASS (0x00f0 | 1)
> 
> Why this values, shouldn't we use a derivative from
> V4L2_CID_PRIVATE_BASE for custom controls? Or can we use random values?

The values are taken from vivid which uses the same scheme. These controls
deal with how the virtual driver emulates things, and I prefer not to make
these control IDs part of the public API so we can be a bit more flexible in
the future. It's a design choice which worked well for vivid.

> 
>> +#define VIMC_CID_TEST_PATTERN   (VIMC_CID_VIMC_BASE + 0)
>> +
>>  #define VIMC_FRAME_MAX_WIDTH 4096
>>  #define VIMC_FRAME_MAX_HEIGHT 2160
>>  #define VIMC_FRAME_MIN_WIDTH 16
>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c 
>> b/drivers/media/platform/vimc/vimc-sensor.c
>> index 615c2b18dcfc..532097566b27 100644
>> --- a/drivers/media/platform/vimc/vimc-sensor.c
>> +++ b/drivers/media/platform/vimc/vimc-sensor.c
>> @@ -22,6 +22,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>
>> @@ -38,6 +39,7 @@ struct vimc_sen_device {
>>  u8 *frame;
>>  /* The active format */
>>  struct v4l2_mbus_framefmt mbus_format;
>> +struct v4l2_ctrl_handler hdl;
>>  };
>>
>>  static const struct v4l2_mbus_framefmt fmt_default = {
>> @@ -291,6 +293,31 @@ static const struct v4l2_subdev_ops vimc_sen_ops = {
>>  .video = _sen_video_ops,
>>  };
>>
>> +static int vimc_sen_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +struct vimc_sen_device *vsen =
>> +container_of(ctrl->handler, struct vimc_sen_device, hdl);
>> +
>> +switch (ctrl->id) {
>> +case VIMC_CID_TEST_PATTERN:
>> +tpg_s_pattern(>tpg, ctrl->val);
>> +break;
>> +case V4L2_CID_HFLIP:
>> +tpg_s_hflip(>tpg, ctrl->val);
>> +break;
>> +case V4L2_CID_VFLIP:
>> +tpg_s_vflip(>tpg, ctrl->val);
>> +break;
>> +default:
>> +return -EINVAL;
>> +}
>> +return 0;
>> +}
>> +
>> +static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
>> +.s_ctrl = vimc_sen_s_ctrl,
>> +};
>> +
>>  static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
>>   void *master_data)
>>  {
>> @@ -299,10 +326,29 @@ static void vimc_sen_comp_unbind(struct device *comp, 
>> struct device *master,
>>  container_of(ved, struct vimc_sen_device, ved);
>>
>>  vimc_ent_sd_unregister(ved, >sd);
>> +v4l2_ctrl_handler_free(>hdl);
>>  tpg_free(>tpg);
>>  kfree(vsen);
>>  }
>>
>> +/* Image Processing Controls */
>> +static const struct v4l2_ctrl_config vimc_sen_ctrl_class = {
>> +.ops = _sen_ctrl_ops,
>> +.flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY,
> 
> I was wondering if it is really necessary to specify the ops and flags
> in the class, as this is seems to me a meta control for the other
> controls to be grouped in a class.

ops can be dropped, but flags needs to be there.

> 
>> +.id = VIMC_CID_VIMC_CLASS,
>> +.name = "VIMC Controls",
>> +.type = V4L2_CTRL_TYPE_CTRL_CLASS,
>> +};
>> +
>> +static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = {
>> +.ops = _sen_ctrl_ops,
>> +.id = VIMC_CID_TEST_PATTERN,
>> +.name = "Test Pattern",
>> +.type = V4L2_CTRL_TYPE_MENU,
>> +.max = TPG_PAT_NOISE,
>> +.qmenu = tpg_pattern_strings,
>> +};
>> +
>>  static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>void *master_data)
>>  {
>> @@ -316,6 +362,20 @@ static int vimc_sen_comp_bind(struct device *comp, 
>> struct device *master,
>>  if (!vsen)
>>  return -ENOMEM;
>>
>> +v4l2_ctrl_handler_init(>hdl, 4);
>> +
>> +v4l2_ctrl_new_custom(>hdl, _sen_ctrl_class, NULL);
>> +v4l2_ctrl_new_custom(>hdl, _sen_ctrl_test_pattern, NULL);
>> +v4l2_ctrl_new_std(>hdl, _sen_ctrl_ops,
>> +V4L2_CID_VFLIP, 0, 1, 1, 0);
>> +

Re: [PATCH v2] media: s5p-mfc: Add support for V4L2_MEMORY_DMABUF type

[Marek Szyprowski]

Hi Nicolas,

On 2017-11-03 14:45, Nicolas Dufresne wrote:

Le vendredi 03 novembre 2017 à 09:11 +0100, Marek Szyprowski a écrit :

MFC driver supports only MMAP operation mode mainly due to the hardware
restrictions of the addresses of the DMA buffers (MFC v5 hardware can
access buffers only in 128MiB memory region starting from the base address
of its firmware). When IOMMU is available, this requirement is easily
fulfilled even for the buffers located anywhere in the memory - typically
by mapping them in the DMA address space as close as possible to the
firmware. Later hardware revisions don't have this limitations at all.

The second limitation of the MFC hardware related to the memory buffers
is constant buffer address. Once the hardware has been initialized for
operation on given buffer set, the addresses of the buffers cannot be
changed.

With the above assumptions, a limited support for USERPTR and DMABUF
operation modes can be added. The main requirement is to have all buffers
known when starting hardware. This has been achieved by postponing
hardware initialization once all the DMABUF or USERPTR buffers have been
queued for the first time. Once then, buffers cannot be modified to point
to other memory area.

I am concerned about enabling this support with existing userspace.
Userspace application will be left with some driver with this
limitation and other drivers without. I think it is harmful to enable
that feature without providing userspace the ability to know.

This is specially conflicting with let's say UVC driver providing
buffers, since UVC driver implementing CREATE_BUFS ioctl. So even if
userspace start making an effort to maintain the same DMABuf for the
same buffer index, if a new buffer is created, we won't be able to use
it.


Sorry, but I don't get this as an 'issue'. The typical use scenario is to
configure buffer queue and start streaming. Basically ReqBufs, stream on and
a sequence of bufq/bufdq. This is handled without any problem by MFC driver
with this patch. After releasing a queue with reqbufs(0), one can use
different set of buffers.

What is the point of changing the buffers during the streaming? IMHO it was
one of the biggest pathology of the V4L2 USERPTR API that the buffers 
were in

fact 'created' on the first queue operation. By creating I mean creating all
the kernel all needed structures and mappings between the real memory (user
ptr value) and the buffer index. The result of this was userspace, which 
don't

use buffer indices and always queues buffers with index = 0, what means that
kernel has to reacquire direct access to each buffer every single frame. 
That
is highly inefficient approach. DMABUF operation mode inherited this 
drawback.


When we have a pipeline for processing video data it should use N buffers on
both input and output of each element when DMAbuf is used. Once we 
allocate N
buffers, using N dmabuf-imported buffers which maps 1-1 is trivial. Only 
this

way you will have true zero-copy processing without any additional overhead.


This patch also removes unconditional USERPTR operation mode from encoder
video node, because it doesn't work with v5 MFC hardware without IOMMU
being enabled.

In case of MFC v5 a bidirectional queue flag has to be enabled as a
workaround of the strange hardware behavior - MFC performs a few writes
to source data during the operation.

Do you have more information about this ? It is quite terrible, since
if you enable buffer importation, the buffer might be multi-plex across
multiple encoder instance. That is another way this feature can be
harmful to existing userspace.


I also don't like this behavior of the hardware. I will probably investigate
it further once I have some time. The other workaround would be to 
always use

driver allocated buffers and simply copy input stream in case of USERPTR or
DMABUF to avoid modifying source data. It would mean copying the compressed
stream, what should not hurt us that much.


Signed-off-by: Seung-Woo Kim 
[mszyprow: adapted to v4.14 code base, rewrote and extended commit message,
  added checks for changing buffer addresses, added bidirectional queue
  flags and comments]
Signed-off-by: Marek Szyprowski 
---
v2:
- fixed copy/paste bug, which broke encoding support (thanks to Marian
   Mihailescu for reporting it)
- added checks for changing buffers DMA addresses
- added bidirectional queue flags

v1:
- inital version
---
  drivers/media/platform/s5p-mfc/s5p_mfc.c |  23 +-
  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 111 +++
  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c |  64 +++
  3 files changed, 147 insertions(+), 51 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 1839a86cc2a5..f1ab8d198158 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -754,6 

Re: [PATCH 2/4] dma-buf/fence: Sparse wants __rcu on the object itself

[Daniel Vetter]

On Thu, Nov 02, 2017 at 10:03:34PM +0200, Ville Syrjala wrote:
> From: Chris Wilson 
> 
> In order to silent sparse in dma_fence_get_rcu_safe(), we need to mark

s/silent/silence/

On the series (assuming sparse is indeed happy now, I didn't check that):

Reviewed-by: Daniel Vetter 

> the incoming fence object as being RCU protected and not the pointer to
> the object.
> 
> Cc: Dave Airlie 
> Cc: Jason Ekstrand 
> Cc: linaro-mm-...@lists.linaro.org
> Cc: linux-media@vger.kernel.org
> Cc: Alex Deucher 
> Cc: Christian König 
> Cc: Sumit Semwal 
> Signed-off-by: Chris Wilson 
> ---
>  include/linux/dma-fence.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index efdabbb64e3c..4c008170fe65 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -242,7 +242,7 @@ static inline struct dma_fence *dma_fence_get_rcu(struct 
> dma_fence *fence)
>   * The caller is required to hold the RCU read lock.
>   */
>  static inline struct dma_fence *
> -dma_fence_get_rcu_safe(struct dma_fence * __rcu *fencep)
> +dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep)
>  {
>   do {
>   struct dma_fence *fence;
> -- 
> 2.13.6
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH][V2] bdisp: remove redundant assignment to pix

[Fabien DESSENNE]

Hi Colin

Thank you for the patch.


On 29/10/17 14:43, Colin King wrote:
> From: Colin Ian King 
>
> Pointer pix is being initialized to a value and a little later
> being assigned the same value again. Remove the initial assignment to
> avoid a duplicate assignment. Cleans up the clang warning:
>
> drivers/media/platform/sti/bdisp/bdisp-v4l2.c:726:26: warning: Value
> stored to 'pix' during its initialization is never read
>
> Signed-off-by: Colin Ian King 
Reviewed-by: Fabien Dessenne 
> ---
>   drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c 
> b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
> index 939da6da7644..7e9ed9c7b3e1 100644
> --- a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
> +++ b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
> @@ -723,7 +723,7 @@ static int bdisp_enum_fmt(struct file *file, void *fh, 
> struct v4l2_fmtdesc *f)
>   static int bdisp_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
>   {
>   struct bdisp_ctx *ctx = fh_to_ctx(fh);
> - struct v4l2_pix_format *pix = >fmt.pix;
> + struct v4l2_pix_format *pix;
>   struct bdisp_frame *frame  = ctx_get_frame(ctx, f->type);
>   
>   if (IS_ERR(frame)) {