cron job: media_tree daily build: OK

[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:   Fri Apr 27 05:00:10 CEST 2018
media-tree git hash:a2b2eff6ac2716f499defa590a6ec4ba379d765e
media_build git hash:   2945d108c680b3c09c9843e001e84a9797d7f379
v4l-utils git hash: 03e763fd4b361b2082019032fc315b7606669335
gcc version:i686-linux-gcc (GCC) 7.3.0
sparse version: 0.5.2-RC1
smatch version: 0.5.1
host hardware:  x86_64
host os:4.15.0-2-amd64

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-arm-stm32: OK
linux-git-arm64: OK
linux-git-i686: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
Check COMPILE_TEST: OK
linux-2.6.36.4-i686: OK
linux-2.6.36.4-x86_64: OK
linux-2.6.37.6-i686: OK
linux-2.6.37.6-x86_64: OK
linux-2.6.38.8-i686: OK
linux-2.6.38.8-x86_64: OK
linux-2.6.39.4-i686: OK
linux-2.6.39.4-x86_64: OK
linux-3.0.101-i686: OK
linux-3.0.101-x86_64: OK
linux-3.1.10-i686: OK
linux-3.1.10-x86_64: OK
linux-3.2.101-i686: OK
linux-3.2.101-x86_64: OK
linux-3.3.8-i686: OK
linux-3.3.8-x86_64: OK
linux-3.4.113-i686: OK
linux-3.4.113-x86_64: OK
linux-3.5.7-i686: OK
linux-3.5.7-x86_64: OK
linux-3.6.11-i686: OK
linux-3.6.11-x86_64: OK
linux-3.7.10-i686: OK
linux-3.7.10-x86_64: OK
linux-3.8.13-i686: OK
linux-3.8.13-x86_64: OK
linux-3.9.11-i686: OK
linux-3.9.11-x86_64: OK
linux-3.10.108-i686: OK
linux-3.10.108-x86_64: OK
linux-3.11.10-i686: OK
linux-3.11.10-x86_64: OK
linux-3.12.74-i686: OK
linux-3.12.74-x86_64: OK
linux-3.13.11-i686: OK
linux-3.13.11-x86_64: OK
linux-3.14.79-i686: OK
linux-3.14.79-x86_64: OK
linux-3.15.10-i686: OK
linux-3.15.10-x86_64: OK
linux-3.16.56-i686: OK
linux-3.16.56-x86_64: OK
linux-3.17.8-i686: OK
linux-3.17.8-x86_64: OK
linux-3.18.102-i686: OK
linux-3.18.102-x86_64: OK
linux-3.19.8-i686: OK
linux-3.19.8-x86_64: OK
linux-4.0.9-i686: OK
linux-4.0.9-x86_64: OK
linux-4.1.51-i686: OK
linux-4.1.51-x86_64: OK
linux-4.2.8-i686: OK
linux-4.2.8-x86_64: OK
linux-4.3.6-i686: OK
linux-4.3.6-x86_64: OK
linux-4.4.109-i686: OK
linux-4.4.109-x86_64: OK
linux-4.5.7-i686: OK
linux-4.5.7-x86_64: OK
linux-4.6.7-i686: OK
linux-4.6.7-x86_64: OK
linux-4.7.10-i686: OK
linux-4.7.10-x86_64: OK
linux-4.8.17-i686: OK
linux-4.8.17-x86_64: OK
linux-4.9.91-i686: OK
linux-4.9.91-x86_64: OK
linux-4.14.31-i686: OK
linux-4.14.31-x86_64: OK
linux-4.15.14-i686: OK
linux-4.15.14-x86_64: OK
linux-4.16-i686: OK
linux-4.16-x86_64: OK
apps: OK
spec-git: OK
sparse: WARNINGS
smatch: OK

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

Re: [PATCH v2 08/10] dt-bindings: media: Document bindings for the Sunxi-Cedrus VPU driver

[Rob Herring]

On Thu, Apr 19, 2018 at 05:45:34PM +0200, Paul Kocialkowski wrote:
> This adds a device-tree binding document that specifies the properties
> used by the Sunxi-Cedurs VPU driver, as well as examples.
> 
> Signed-off-by: Paul Kocialkowski 
> ---
>  .../devicetree/bindings/media/sunxi-cedrus.txt | 50 
> ++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/sunxi-cedrus.txt

Other than the one nit about the node name,

Reviewed-by: Rob Herring 

Re: [PATCH v2 08/10] dt-bindings: media: Document bindings for the Sunxi-Cedrus VPU driver

[Rob Herring]

On Fri, Apr 20, 2018 at 09:22:20AM +0200, Paul Kocialkowski wrote:
> Hi and thanks for the review,
> 
> On Fri, 2018-04-20 at 01:31 +, Tomasz Figa wrote:
> > Hi Paul, Philipp,
> > 
> > On Fri, Apr 20, 2018 at 1:04 AM Philipp Zabel 
> > wrote:
> > 
> > > Hi Paul,
> > > On Thu, 2018-04-19 at 17:45 +0200, Paul Kocialkowski wrote:
> > > > This adds a device-tree binding document that specifies the
> > > > properties
> > > > used by the Sunxi-Cedurs VPU driver, as well as examples.
> > > > 
> > > > Signed-off-by: Paul Kocialkowski 
> > > > ---
> > > >  .../devicetree/bindings/media/sunxi-cedrus.txt | 50
> > 
> > ++
> > > >  1 file changed, 50 insertions(+)
> > > >  create mode 100644
> > 
> > Documentation/devicetree/bindings/media/sunxi-cedrus.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/sunxi-
> > > > cedrus.txt
> > 
> > b/Documentation/devicetree/bindings/media/sunxi-cedrus.txt
> > > > new file mode 100644
> > > > index ..71ad3f9c3352
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/sunxi-cedrus.txt
> > > > @@ -0,0 +1,50 @@
> > > > +Device-tree bindings for the VPU found in Allwinner SoCs,
> > > > referred to
> > 
> > as the
> > > > +Video Engine (VE) in Allwinner literature.
> > > > +
> > > > +The VPU can only access the first 256 MiB of DRAM, that are DMA-
> > > > mapped
> > 
> > starting
> > > > +from the DRAM base. This requires specific memory allocation and
> > 
> > handling.
> > 
> > And no IOMMU? Brings back memories.
> 
> Exactly, no IOMMU so we don't have much choice but cope with that
> hardware limitation...
> 
> > > > +
> > > > +Required properties:
> > > > +- compatible : "allwinner,sun4i-a10-video-engine";
> > > > +- memory-region : DMA pool for buffers allocation;
> > > > +- clocks : list of clock specifiers, corresponding to
> > 
> > entries in
> > > > +  the clock-names property;
> > > > +- clock-names: should contain "ahb", "mod" and
> > > > "ram"
> > 
> > entries;
> > > > +- assigned-clocks   : list of clocks assigned to the VE;
> > > > +- assigned-clocks-rates : list of clock rates for the clocks
> > > > assigned
> > 
> > to the VE;
> > > > +- resets : phandle for reset;
> > > > +- interrupts : should contain VE interrupt number;
> > > > +- reg: should contain register base and
> > > > length
> > 
> > of VE.
> > > > +
> > > > +Example:
> > > > +
> > > > +reserved-memory {
> > > > + #address-cells = <1>;
> > > > + #size-cells = <1>;
> > > > + ranges;
> > > > +
> > > > + /* Address must be kept in the lower 256 MiBs of DRAM for
> > > > VE. */
> > > > + ve_memory: cma@4a00 {
> > > > + compatible = "shared-dma-pool";
> > > > + reg = <0x4a00 0x600>;
> > > > + no-map;
> > > > + linux,cma-default;
> > > > + };
> > > > +};
> > > > +
> > > > +video-engine@1c0e000 {
> > > This is not really required by any specification, and not as common
> > > as
> > > gpu@..., but could this reasonably be called "vpu@1c0e000" to follow
> > > somewhat-common practice?
> > 
> > AFAIR the name is supposed to be somewhat readable for someone that
> > doesn't know the hardware. To me, "video-engine" sounds more obvious
> > than "vpu", but we actually use "codec" already, in case of MFC and
> > JPEG codec on Exynos. If encode/decode is the only functionality of
> > this block, I'd personally go with "codec". If it can do other things,
> > e.g. scaling/rotation without encode/decode, I'd probably call it
> > "video-processor".
> 
> I agree that the term VPU is more commonly associated with video
> decoding, while video engine could mean a number of things.
> 
> The reason I went with "video-engine" here (while still presenting the
> driver as a VPU driver) is that Video Engine is the term used in
> Allwinner's litterature. Other nodes in Allwinner device-trees generally
> stick to these terms (for instance, we have "display-engine" nodes).
> This also makes it easier to find the matching parts in the
> documentation.

'video-codec' is what is defined in the DT spec.

Rob

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Thu, 26 Apr 2018 16:41:56 -0500
"Gustavo A. R. Silva"  escreveu:

> Hi Mauro,
> 
> On 04/23/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 23 Apr 2018 14:11:02 -0500
> > 
> > Thanks, I 'll mark this series as rejected at patchwork.linuxtv.org.
> > Please feel free to resubmit any patch if they represent a real
> > threat, adding a corresponding description about the threat scenario
> > at the body of the e-mail.
> > 
> >> Sorry for the noise and thanks for the feedback.
> > 
> > Anytime.
> > 
> 
> I noticed you changed the status of this series from rejected to new.

Yes.

> Also, there are other similar issues in media/pci/

Well, the issues will be there everywhere on all media drivers.

I marked your patches because I need to study it carefully, after
Peter's explanations. My plan is to do it next week. Still not
sure if the approach you took is the best one or not.

As I said, one possibility is to change the way v4l2-core handles 
VIDIOC_ENUM_foo ioctls, but that would be make harder to -stable
backports.

I need a weekend to sleep on it.

> 
> I can write proper patches for all of them if you agree those are not 
> False Positives:
> 
> diff --git a/drivers/media/pci/cx18/cx18-ioctl.c 
> b/drivers/media/pci/cx18/cx18-ioctl.c
> index 80b902b..63f4388 100644
> --- a/drivers/media/pci/cx18/cx18-ioctl.c
> +++ b/drivers/media/pci/cx18/cx18-ioctl.c
> @@ -36,6 +36,8 @@
>   #include 
>   #include 
> 
> +#include 
> +
>   u16 cx18_service2vbi(int type)
>   {
>  switch (type) {
> @@ -488,8 +490,9 @@ static int cx18_enum_fmt_vid_cap(struct file *file, 
> void *fh,
>  },
>  };
> 
> -   if (fmt->index > ARRAY_SIZE(formats) - 1)
> +   if (fmt->index >= ARRAY_SIZE(formats))
>  return -EINVAL;
> +   fmt->index = array_index_nospec(fmt->index, ARRAY_SIZE(formats));
>  *fmt = formats[fmt->index];
>  return 0;
>   }
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
> b/drivers/media/pci/saa7134/saa7134-video.c
> index 1a50ec9..d93cf09 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -30,6 +30,8 @@
>   #include 
>   #include 
> 
> +#include 
> +
>   /* -- */
> 
>   unsigned int video_debug;
> @@ -1819,6 +1821,8 @@ static int saa7134_enum_fmt_vid_cap(struct file 
> *file, void  *priv,
>  if (f->index >= FORMATS)
>  return -EINVAL;
> 
> +   f->index = array_index_nospec(f->index, FORMATS);
> +
>  strlcpy(f->description, formats[f->index].name,
>  sizeof(f->description));
> 
> diff --git a/drivers/media/pci/tw68/tw68-video.c 
> b/drivers/media/pci/tw68/tw68-video.c
> index 8c1f4a0..a6cfb4b 100644
> --- a/drivers/media/pci/tw68/tw68-video.c
>   #include 
>   #include 
> 
> +#include 
> +
>   #include "tw68.h"
>   #include "tw68-reg.h"
> 
> @@ -789,6 +791,8 @@ static int tw68_enum_fmt_vid_cap(struct file *file, 
> void  *priv,
>  if (f->index >= FORMATS)
>  return -EINVAL;
> 
> +   f->index = array_index_nospec(f->index, FORMATS);
> +
>  strlcpy(f->description, formats[f->index].name,
>  sizeof(f->description));
> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
> b/drivers/media/pci/tw686x/tw686x-video.c
> index c3fafa9..281d722 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -25,6 +25,8 @@
>   #include "tw686x.h"
>   #include "tw686x-regs.h"
> 
> +#include 
> +
>   #define TW686X_INPUTS_PER_CH   4
>   #define TW686X_VIDEO_WIDTH 720
>   #define TW686X_VIDEO_HEIGHT(id)((id & V4L2_STD_525_60) 
> ? 480 : 576)
> @@ -981,6 +983,7 @@ static int tw686x_enum_fmt_vid_cap(struct file 
> *file, void *priv,
>   {
>  if (f->index >= ARRAY_SIZE(formats))
>  return -EINVAL;
> +   f->index = array_index_nospec(f->index, ARRAY_SIZE(formats));
>  f->pixelformat = formats[f->index].fourcc;
>  return 0;
>   }
> 
> 
> Thanks
> --
> Gustavo



Thanks,
Mauro

Re: [PATCH v14 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-04-27 00:30:25 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thursday, 26 April 2018 23:21:21 EEST Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> > connected between the video sources and the video grabbers (VIN).
> > 
> > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> > 
> > Signed-off-by: Niklas Söderlund 
> > Reviewed-by: Hans Verkuil 
> > Reviewed-by: Maxime Ripard 
> > Reviewed-by: Laurent Pinchart 
> > 
> > ---
> > 
> > * Changes since v13
> > - Change return rcar_csi2_formats + i to return _csi2_formats[i].
> > - Add define for PHCLM_STOPSTATECKL.
> > - Update spelling in comments.
> > - Update calculation in rcar_csi2_calc_phypll() according to
> >   https://linuxtv.org/downloads/v4l-dvb-apis/kapi/csi2.html. The one
> >   before v14 did not take into account that 2 bits per sample is
> >   transmitted.
> 
> Just one small comment about this, please see below.
> 
> > - Use Geert's suggestion of (1 << priv->lanes) - 1 instead of switch
> >   statement to set correct number of lanes to enable.
> > - Change hex constants in hsfreqrange_m3w_h3es1[] to lower case to match
> >   style of rest of file.
> > - Switch to %u instead of 0x%x when printing bus type.
> > - Switch to %u instead of %d for priv->lanes which is unsigned.
> > - Add MEDIA_BUS_FMT_YUYV8_1X16 to the list of supported formats in
> >   rcar_csi2_formats[].
> > - Fixed bps for MEDIA_BUS_FMT_YUYV10_2X10 to 20 and not 16.
> > - Set INTSTATE after PL-11 is confirmed to match flow chart in
> >   datasheet.
> > - Change priv->notifier.subdevs == NULL to !priv->notifier.subdevs.
> > - Add Maxime's and laurent's tags.
> > ---
> >  drivers/media/platform/rcar-vin/Kconfig |  12 +
> >  drivers/media/platform/rcar-vin/Makefile|   1 +
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 883 
> >  3 files changed, 896 insertions(+)
> >  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> 
> [snip]
> 
> 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644
> > index ..49b29d5680f9d80b
> > --- /dev/null
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> 
> [snip]
> 
> > +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp,
> > +u32 *phypll)
> > +{
> > +   const struct phypll_hsfreqrange *hsfreq;
> > +   struct v4l2_subdev *source;
> > +   struct v4l2_ctrl *ctrl;
> > +   u64 mbps;
> > +
> > +   if (!priv->remote)
> > +   return -ENODEV;
> > +
> > +   source = priv->remote;
> > +
> > +   /* Read the pixel rate control from remote */
> > +   ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > +   if (!ctrl) {
> > +   dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> > +   source->name);
> > +   return -EINVAL;
> > +   }
> > +
> > +   /*
> > +* Calculate the phypll in mbps (from v4l2 documentation)
> > +* link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
> > +*/
> > +   mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > +   do_div(mbps, priv->lanes * 200);
> 
> pixel rate * bits per sample will give you the overall bit rate, which you 
> then divide by the number of lanes to get the bitrate per lane, and then by 2 
> as D-PHY is a DDR PHY and transmits 2 bits per clock cycle. You then end up 
> with the link frequency, which is thus expressed in MHz, not in Mbps. I would 
> thus name the mbps variable freq, and rename the phypll_hsfreqrange mbps 
> field 
> to freq (maybe with a small comment right after the field to tell the value 
> is 
> expressed in MHz).

I agree that freq would be a better name for what it represents. Never 
the less I prefer to stick with mbps as that is whats used in the 
datasheet. See Table '25.9 HSFREQRANGE Bit Set Values'.

With this in mind if you still feel strongly about renaming it I could 
do so. But at lest for me it feels more useful to easily be able to map 
the variable to the datasheet tables :-)

> 
> > +   for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > +   if (hsfreq->mbps >= mbps)
> > +   break;
> > +
> > +   if (!hsfreq->mbps) {
> > +   dev_err(priv->dev, "Unsupported PHY speed (%llu Mbps)", mbps);
> > +   return -ERANGE;
> > +   }
> > +
> > +   dev_dbg(priv->dev, "PHY HSFREQRANGE requested %llu got %u Mbps\n", mbps,
> > +   hsfreq->mbps);
> > +
> > +   *phypll = PHYPLL_HSFREQRANGE(hsfreq->reg);
> > +
> > +   return 0;
> > +}
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH] rcar-vin: enable field toggle after a set number of lines for Gen3

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wednesday, 25 April 2018 02:56:52 EEST Niklas Söderlund wrote:
> The VIN Gen3 hardware don't have Line Post-Clip capabilities as VIN Gen2
> hardware have. To protect against writing outside the capture window
> enable field toggle after a set number of lines have been captured.
> 
> Capturing outside the allocated capture buffer where observed on R-Car

s/Capturing/Capture/
s/where/has been/

> Gen3 Salvator-XS H3 from the CVBS input if the standard is
> misconfigured. That is if a PAL source is connected to the system but
> the adv748x standard is set to NTSC. In this case the format reported by
> the adv748x is 720x480 and that is what is used for the media pipeline.
> The PAL source generates frames in the format of 720x576 and the field
> is not toggled until the VSYNC is detected and at that time data have
> already been written outside the allocated capture buffer.
> 
> With this change the capture in the situation described above results in
> garbage frames but that is far better then writing outside the capture

s/then/than/

> buffer.
> 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 20 +++-
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> ac07f99e3516a620..b41ba9a4a2b3ac90 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -124,7 +124,9 @@
>  #define VNDMR2_VPS   (1 << 30)
>  #define VNDMR2_HPS   (1 << 29)
>  #define VNDMR2_FTEV  (1 << 17)
> +#define VNDMR2_FTEH  (1 << 16)
>  #define VNDMR2_VLV(n)((n & 0xf) << 12)
> +#define VNDMR2_HLV(n)((n) & 0xfff)
> 
>  /* Video n CSI2 Interface Mode Register (Gen3) */
>  #define VNCSI_IFMD_DES1  (1 << 26)
> @@ -612,8 +614,9 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> 
>  static int rvin_setup(struct rvin_dev *vin)
>  {
> - u32 vnmc, dmr, dmr2, interrupts;
> + u32 vnmc, dmr, dmr2, interrupts, lines;
>   bool progressive = false, output_is_yuv = false, input_is_yuv = false;
> + bool halfsize = false;
> 
>   switch (vin->format.field) {
>   case V4L2_FIELD_TOP:
> @@ -628,12 +631,15 @@ static int rvin_setup(struct rvin_dev *vin)
>   /* Use BT if video standard can be read and is 60 Hz format */
>   if (!vin->info->use_mc && vin->std & V4L2_STD_525_60)
>   vnmc = VNMC_IM_FULL | VNMC_FOC;
> + halfsize = true;
>   break;
>   case V4L2_FIELD_INTERLACED_TB:
>   vnmc = VNMC_IM_FULL;
> + halfsize = true;
>   break;
>   case V4L2_FIELD_INTERLACED_BT:
>   vnmc = VNMC_IM_FULL | VNMC_FOC;
> + halfsize = true;
>   break;
>   case V4L2_FIELD_NONE:
>   vnmc = VNMC_IM_ODD_EVEN;
> @@ -676,11 +682,15 @@ static int rvin_setup(struct rvin_dev *vin)
>   break;
>   }
> 
> - /* Enable VSYNC Field Toogle mode after one VSYNC input */
> - if (vin->info->model == RCAR_GEN3)
> - dmr2 = VNDMR2_FTEV;
> - else
> + if (vin->info->model == RCAR_GEN3) {
> + /* Enable HSYNC Field Toggle mode after height HSYNC inputs. */
> + lines = vin->format.height / (halfsize ? 2 : 1);
> + dmr2 = VNDMR2_FTEH | VNDMR2_HLV(lines);
> + vin_dbg(vin, "Field Toogle after %u lines\n", lines);
> + } else {
> + /* Enable VSYNC Field Toogle mode after one VSYNC input. */
>   dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
> + }
> 
>   /* Hsync Signal Polarity Select */
>   if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))

-- 
Regards,

Laurent Pinchart

Re: [PATCH] rcar-vin: add support for MEDIA_BUS_FMT_UYVY8_1X16

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wednesday, 25 April 2018 02:46:07 EEST Niklas Söderlund wrote:
> By setting VNMC_YCAL rcar-vin can support input video in
> MEDIA_BUS_FMT_UYVY8_1X16 format.
> 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 1 +
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 5 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 55b745ac86a5884d..7bc2774a11232362 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -404,6 +404,7 @@ static int rvin_digital_subdevice_attach(struct rvin_dev
> *vin, code.index++;
>   switch (code.code) {
>   case MEDIA_BUS_FMT_YUYV8_1X16:
> + case MEDIA_BUS_FMT_UYVY8_1X16:
>   case MEDIA_BUS_FMT_UYVY8_2X8:
>   case MEDIA_BUS_FMT_UYVY10_2X10:
>   case MEDIA_BUS_FMT_RGB888_1X24:
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> 4a3a195e7f59047c..ac07f99e3516a620 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -653,6 +653,10 @@ static int rvin_setup(struct rvin_dev *vin)
>   vnmc |= VNMC_INF_YUV16;
>   input_is_yuv = true;
>   break;
> + case MEDIA_BUS_FMT_UYVY8_1X16:
> + vnmc |= VNMC_INF_YUV16 | VNMC_YCAL;
> + input_is_yuv = true;
> + break;
>   case MEDIA_BUS_FMT_UYVY8_2X8:
>   /* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
>   vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
> @@ -1009,6 +1013,7 @@ static int rvin_mc_validate_format(struct rvin_dev
> *vin, struct v4l2_subdev *sd,
> 
>   switch (fmt.format.code) {
>   case MEDIA_BUS_FMT_YUYV8_1X16:
> + case MEDIA_BUS_FMT_UYVY8_1X16:
>   case MEDIA_BUS_FMT_UYVY8_2X8:
>   case MEDIA_BUS_FMT_UYVY10_2X10:
>   case MEDIA_BUS_FMT_RGB888_1X24:


-- 
Regards,

Laurent Pinchart

Re: [PATCH] rcar-vin: remove generic gen3 compatible string

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wednesday, 25 April 2018 02:43:21 EEST Niklas Söderlund wrote:
> The compatible string "renesas,rcar-gen3-vin" was added before the
> Gen3 driver code was added but it's not possible to use. Each SoC in the
> Gen3 series require SoC specific knowledge in the driver to function.
> Remove it before it is added to any device tree descriptions.
> 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  Documentation/devicetree/bindings/media/rcar_vin.txt | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt
> b/Documentation/devicetree/bindings/media/rcar_vin.txt index
> ba31431d4b1fbdbb..a19517e1c669eb35 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -24,7 +24,6 @@ on Gen3 platforms to a CSI-2 receiver.
> - "renesas,vin-r8a77970" for the R8A77970 device
> - "renesas,rcar-gen2-vin" for a generic R-Car Gen2 or RZ/G1 compatible
>   device.
> -   - "renesas,rcar-gen3-vin" for a generic R-Car Gen3 compatible device.
> 
> When compatible with the generic version nodes must list the
> SoC-specific version corresponding to the platform first


-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

Hi Mauro,

On 04/23/2018 02:17 PM, Mauro Carvalho Chehab wrote:

Em Mon, 23 Apr 2018 14:11:02 -0500

Thanks, I 'll mark this series as rejected at patchwork.linuxtv.org.
Please feel free to resubmit any patch if they represent a real
threat, adding a corresponding description about the threat scenario
at the body of the e-mail.


Sorry for the noise and thanks for the feedback.


Anytime.



I noticed you changed the status of this series from rejected to new.

Also, there are other similar issues in media/pci/

I can write proper patches for all of them if you agree those are not 
False Positives:


diff --git a/drivers/media/pci/cx18/cx18-ioctl.c 
b/drivers/media/pci/cx18/cx18-ioctl.c

index 80b902b..63f4388 100644
--- a/drivers/media/pci/cx18/cx18-ioctl.c
+++ b/drivers/media/pci/cx18/cx18-ioctl.c
@@ -36,6 +36,8 @@
 #include 
 #include 

+#include 
+
 u16 cx18_service2vbi(int type)
 {
switch (type) {
@@ -488,8 +490,9 @@ static int cx18_enum_fmt_vid_cap(struct file *file, 
void *fh,

},
};

-   if (fmt->index > ARRAY_SIZE(formats) - 1)
+   if (fmt->index >= ARRAY_SIZE(formats))
return -EINVAL;
+   fmt->index = array_index_nospec(fmt->index, ARRAY_SIZE(formats));
*fmt = formats[fmt->index];
return 0;
 }
diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
b/drivers/media/pci/saa7134/saa7134-video.c

index 1a50ec9..d93cf09 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -30,6 +30,8 @@
 #include 
 #include 

+#include 
+
 /* -- */

 unsigned int video_debug;
@@ -1819,6 +1821,8 @@ static int saa7134_enum_fmt_vid_cap(struct file 
*file, void  *priv,

if (f->index >= FORMATS)
return -EINVAL;

+   f->index = array_index_nospec(f->index, FORMATS);
+
strlcpy(f->description, formats[f->index].name,
sizeof(f->description));

diff --git a/drivers/media/pci/tw68/tw68-video.c 
b/drivers/media/pci/tw68/tw68-video.c

index 8c1f4a0..a6cfb4b 100644
--- a/drivers/media/pci/tw68/tw68-video.c
 #include 
 #include 

+#include 
+
 #include "tw68.h"
 #include "tw68-reg.h"

@@ -789,6 +791,8 @@ static int tw68_enum_fmt_vid_cap(struct file *file, 
void  *priv,

if (f->index >= FORMATS)
return -EINVAL;

+   f->index = array_index_nospec(f->index, FORMATS);
+
strlcpy(f->description, formats[f->index].name,
sizeof(f->description));

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c

index c3fafa9..281d722 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -25,6 +25,8 @@
 #include "tw686x.h"
 #include "tw686x-regs.h"

+#include 
+
 #define TW686X_INPUTS_PER_CH   4
 #define TW686X_VIDEO_WIDTH 720
 #define TW686X_VIDEO_HEIGHT(id)((id & V4L2_STD_525_60) 
? 480 : 576)
@@ -981,6 +983,7 @@ static int tw686x_enum_fmt_vid_cap(struct file 
*file, void *priv,

 {
if (f->index >= ARRAY_SIZE(formats))
return -EINVAL;
+   f->index = array_index_nospec(f->index, ARRAY_SIZE(formats));
f->pixelformat = formats[f->index].fourcc;
return 0;
 }


Thanks
--
Gustavo

Re: [PATCH v2] media: v4l2-compat-ioctl32: better document the code

[Sakari Ailus]

Hi Mauro,

Thanks. It's nice to have these things documented. A few comments below.

On Fri, Apr 20, 2018 at 07:45:46AM -0400, Mauro Carvalho Chehab wrote:
> This file does a lot of non-trivial struff. Document it using
> kernel-doc markups where needed and improve the comments inside
> do_video_ioctl().
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 165 
> +-
>  1 file changed, 159 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c 
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d2f0268427c2..9611c3aae8ca 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -22,7 +22,18 @@
>  #include 
>  #include 
>  
> -/* Use the same argument order as copy_in_user */
> +/**
> + * assign_in_user() - Copy from one __user var to another one

No need for empty parentheses --- I think we generally don't have them
elsewhere either, albeit I remember seeing them somewhere. But they're
still redundant. :-)

> + *
> + * @to: __user var where data will be stored
> + * @from: __user var where data will be retrieved.

Please use the full stop consistently; I guess in most of the cases we
don't have that in argument descriptions.

> + *
> + * As this code very often needs to allocate userspace memory, it is easier
> + * to have a macro that will do both get_user() and put_user() at once.

How about "Read a value from __user memory at @from using get_user() and
write it back to __user memory at @to using put_user(). A temporary
variable needed for this is allocated in the stack." ? Up to you.

> + *
> + * This function complements the macros defined at asm-generic/uaccess.h.
> + * It uses the same argument order as copy_in_user()
> + */

This starts looking so good it might be material to
include/asm-generic/uaccess.h, but let's think about that later on. :-)

>  #define assign_in_user(to, from) \
>  ({   \
>   typeof(*from) __assign_tmp; \
> @@ -30,16 +41,56 @@
>   get_user(__assign_tmp, from) || put_user(__assign_tmp, to); \
>  })
>  
> +/**
> + * get_user_cast() - Stores at a kernelspace local var the contents from a
> + *   pointer with userspace data that is not tagged with __user.
> + *
> + * @__x: var where data will be stored
> + * @__ptr: var where data will be retrieved.
> + *
> + * Sometimes we need to declare a pointer without __user because it
> + * comes from a pointer struct field that will be retrieved from userspace
> + * by the 64-bit native ioctl handler. This function ensures that the
> + * @__ptr will be cast to __user before calling get_user() in order to
> + * avoid warnings with static code analyzers like smatch.
> + */
>  #define get_user_cast(__x, __ptr)\
>  ({   \
>   get_user(__x, (typeof(*__ptr) __user *)(__ptr));\
>  })
>  
> +/**
> + * put_user_force() - Stores the contents of a kernelspace local var
> + * into an userspace pointer, removing any __user cast.
> + *
> + * @__x: var where data will be stored
> + * @__ptr: var where data will be retrieved.
> + *
> + * Sometimes we need to remove the __user attribute from some data,
> + * by passing the __force macro. This function ensures that the
> + * @__ptr will be cast with __force before calling put_user(), in order to
> + * avoid warnings with static code analyzers like smatch.
> + */
>  #define put_user_force(__x, __ptr)   \
>  ({   \
>   put_user((typeof(*__x) __force *)(__x), __ptr); \
>  })
>  
> +/**
> + * assign_in_user_cast() - Copy from one __user var to another one
> + *
> + * @to: __user var where data will be stored
> + * @from: var where data will be retrieved that needs to be cast to __user.
> + *
> + * As this code very often needs to allocate userspace memory, it is easier
> + * to have a macro that will do both get_user_cast() and put_user() at once.
> + *
> + * This function should be used instead of assign_in_user() when the @from
> + * variable was not declared as __user. See get_user_cast() for more details.
> + *
> + * This function complements the macros defined at asm-generic/uaccess.h.
> + * It uses the same argument order as copy_in_user()
> + */
>  #define assign_in_user_cast(to, from)
> \
>  ({   \
>   typeof(*from) __assign_tmp; \
> @@ -47,7 +98,16 @@
>   get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
>  })
>  
> -
> +/**
> + * 

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[Laurent Pinchart]

Hi Mita-san,

On Thursday, 26 April 2018 19:17:55 EEST Akinobu Mita wrote:
> 2018-04-26 7:40 GMT+09:00 Laurent Pinchart:
> > On Wednesday, 25 April 2018 19:19:11 EEST Akinobu Mita wrote:
> >> 2018-04-24 0:54 GMT+09:00 Akinobu Mita :
> >> > 2018-04-23 18:17 GMT+09:00 Laurent Pinchart:
> >> >> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
> >> >>> This adds a device tree binding documentation for OV7720/OV7725
> >> >>> sensor.
> >> >>> 
> >> >>> Cc: Jacopo Mondi 
> >> >>> Cc: Laurent Pinchart 
> >> >>> Cc: Hans Verkuil 
> >> >>> Cc: Sakari Ailus 
> >> >>> Cc: Mauro Carvalho Chehab 
> >> >>> Cc: Rob Herring 
> >> >>> Reviewed-by: Rob Herring 
> >> >>> Reviewed-by: Jacopo Mondi 
> >> >>> Signed-off-by: Akinobu Mita 
> >> >>> ---
> >> >>> * v3
> >> >>> - Add Reviewed-by: lines
> >> >>> 
> >> >>>  .../devicetree/bindings/media/i2c/ov772x.txt   | 42
> >> >>>  +++
> >> >>>  MAINTAINERS|  1 +
> >> >>>  2 files changed, 43 insertions(+)
> >> >>>  create mode 100644
> >> >>>  Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>> 
> >> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>> b/Documentation/devicetree/bindings/media/i2c/ov772x.txt new file
> >> >>> mode
> >> >>> 100644
> >> >>> index 000..b045503
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>> @@ -0,0 +1,42 @@
> >> >>> +* Omnivision OV7720/OV7725 CMOS sensor
> >> >>> +
> >> >>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions
> >> >>> output,
> >> >>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It
> >> >>> also
> >> >>> can +support the YUV422, RGB565/555/444, GRB422 or raw RGB output
> >> >>> formats. +
> >> >>> +Required Properties:
> >> >>> +- compatible: shall be one of
> >> >>> + "ovti,ov7720"
> >> >>> + "ovti,ov7725"
> >> >>> +- clocks: reference to the xclk input clock.
> >> >>> +- clock-names: shall be "xclk".
> >> >> 
> >> >> As there's a single clock we could omit clock-names, couldn't we ?
> >> > 
> >> > Sounds good.
> >> > 
> >> > I'll prepare another patch that replaces the clock consumer ID argument
> >> > of clk_get() from "xclk" to NULL, and remove the above line in this
> >> > bindings.
> >> 
> >> I thought it's easy to do.  However, there is a non-DT user
> >> (arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.
> >> 
> >> This can be resolved by retrying clk_get() with NULL if no entry
> >> with "xclk".  But should we do so or leave as is?
> > 
> > How about patching the board code to register the clock alias with
> > 
> > clk_add_alias(NULL, "0-0021", "video_clk", NULL);
> 
> Sounds good.
> 
> But I'm a bit worried about whether clk_add_alias() can be called with
> alias == NULL.  I couldn't find such use case.

There aren't many occurrences, but

$ find . -type f -exec grep -l 'clk_add_alias(NULL' {} \;
/drivers/clk/ti/clk.c
/drivers/clk/ti/fixed-factor.c
/drivers/clk/ti/clk-dra7-atl.c
/drivers/clk/ti/composite.c

A quick code analysis also shows me that this should be supported.

> Probably Jacopo can verify whether it works or not with v4 patchset.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v14 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thursday, 26 April 2018 23:21:21 EEST Niklas Söderlund wrote:
> A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> connected between the video sources and the video grabbers (VIN).
> 
> Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> 
> Signed-off-by: Niklas Söderlund 
> Reviewed-by: Hans Verkuil 
> Reviewed-by: Maxime Ripard 
> Reviewed-by: Laurent Pinchart 
> 
> ---
> 
> * Changes since v13
> - Change return rcar_csi2_formats + i to return _csi2_formats[i].
> - Add define for PHCLM_STOPSTATECKL.
> - Update spelling in comments.
> - Update calculation in rcar_csi2_calc_phypll() according to
>   https://linuxtv.org/downloads/v4l-dvb-apis/kapi/csi2.html. The one
>   before v14 did not take into account that 2 bits per sample is
>   transmitted.

Just one small comment about this, please see below.

> - Use Geert's suggestion of (1 << priv->lanes) - 1 instead of switch
>   statement to set correct number of lanes to enable.
> - Change hex constants in hsfreqrange_m3w_h3es1[] to lower case to match
>   style of rest of file.
> - Switch to %u instead of 0x%x when printing bus type.
> - Switch to %u instead of %d for priv->lanes which is unsigned.
> - Add MEDIA_BUS_FMT_YUYV8_1X16 to the list of supported formats in
>   rcar_csi2_formats[].
> - Fixed bps for MEDIA_BUS_FMT_YUYV10_2X10 to 20 and not 16.
> - Set INTSTATE after PL-11 is confirmed to match flow chart in
>   datasheet.
> - Change priv->notifier.subdevs == NULL to !priv->notifier.subdevs.
> - Add Maxime's and laurent's tags.
> ---
>  drivers/media/platform/rcar-vin/Kconfig |  12 +
>  drivers/media/platform/rcar-vin/Makefile|   1 +
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 883 
>  3 files changed, 896 insertions(+)
>  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c

[snip]


> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644
> index ..49b29d5680f9d80b
> --- /dev/null
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c

[snip]

> +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp,
> +  u32 *phypll)
> +{
> + const struct phypll_hsfreqrange *hsfreq;
> + struct v4l2_subdev *source;
> + struct v4l2_ctrl *ctrl;
> + u64 mbps;
> +
> + if (!priv->remote)
> + return -ENODEV;
> +
> + source = priv->remote;
> +
> + /* Read the pixel rate control from remote */
> + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> + if (!ctrl) {
> + dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> + source->name);
> + return -EINVAL;
> + }
> +
> + /*
> +  * Calculate the phypll in mbps (from v4l2 documentation)
> +  * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
> +  */
> + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> + do_div(mbps, priv->lanes * 200);

pixel rate * bits per sample will give you the overall bit rate, which you 
then divide by the number of lanes to get the bitrate per lane, and then by 2 
as D-PHY is a DDR PHY and transmits 2 bits per clock cycle. You then end up 
with the link frequency, which is thus expressed in MHz, not in Mbps. I would 
thus name the mbps variable freq, and rename the phypll_hsfreqrange mbps field 
to freq (maybe with a small comment right after the field to tell the value is 
expressed in MHz).

> + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> + if (hsfreq->mbps >= mbps)
> + break;
> +
> + if (!hsfreq->mbps) {
> + dev_err(priv->dev, "Unsupported PHY speed (%llu Mbps)", mbps);
> + return -ERANGE;
> + }
> +
> + dev_dbg(priv->dev, "PHY HSFREQRANGE requested %llu got %u Mbps\n", mbps,
> + hsfreq->mbps);
> +
> + *phypll = PHYPLL_HSFREQRANGE(hsfreq->reg);
> +
> + return 0;
> +}

[snip]

-- 
Regards,

Laurent Pinchart

Proposal

[MS Zeliha Omer Faruk]

Hello

   Greetings to you today i asked before but i did't get a response please
i know this might come to you as a surprise because you do not know me
personally i have a business proposal for you please reply for more
info.



Best Regards,

Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
 Sisli - Istanbul, Turkey

[PATCH v14 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
connected between the video sources and the video grabbers (VIN).

Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.

Signed-off-by: Niklas Söderlund 
Reviewed-by: Hans Verkuil 
Reviewed-by: Maxime Ripard 
Reviewed-by: Laurent Pinchart 

---

* Changes since v13
- Change return rcar_csi2_formats + i to return _csi2_formats[i].
- Add define for PHCLM_STOPSTATECKL.
- Update spelling in comments.
- Update calculation in rcar_csi2_calc_phypll() according to
  https://linuxtv.org/downloads/v4l-dvb-apis/kapi/csi2.html. The one
  before v14 did not take into account that 2 bits per sample is
  transmitted.
- Use Geert's suggestion of (1 << priv->lanes) - 1 instead of switch
  statement to set correct number of lanes to enable.
- Change hex constants in hsfreqrange_m3w_h3es1[] to lower case to match
  style of rest of file.
- Switch to %u instead of 0x%x when printing bus type.
- Switch to %u instead of %d for priv->lanes which is unsigned.
- Add MEDIA_BUS_FMT_YUYV8_1X16 to the list of supported formats in
  rcar_csi2_formats[].
- Fixed bps for MEDIA_BUS_FMT_YUYV10_2X10 to 20 and not 16.
- Set INTSTATE after PL-11 is confirmed to match flow chart in
  datasheet.
- Change priv->notifier.subdevs == NULL to !priv->notifier.subdevs.
- Add Maxime's and laurent's tags.
---
 drivers/media/platform/rcar-vin/Kconfig |  12 +
 drivers/media/platform/rcar-vin/Makefile|   1 +
 drivers/media/platform/rcar-vin/rcar-csi2.c | 883 
 3 files changed, 896 insertions(+)
 create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c

diff --git a/drivers/media/platform/rcar-vin/Kconfig 
b/drivers/media/platform/rcar-vin/Kconfig
index 8fa7ee468c63afb9..3dfeb91f8f186528 100644
--- a/drivers/media/platform/rcar-vin/Kconfig
+++ b/drivers/media/platform/rcar-vin/Kconfig
@@ -1,3 +1,15 @@
+config VIDEO_RCAR_CSI2
+   tristate "R-Car MIPI CSI-2 Receiver"
+   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
+   depends on ARCH_RENESAS || COMPILE_TEST
+   select V4L2_FWNODE
+   ---help---
+ Support for Renesas R-Car MIPI CSI-2 receiver.
+ Supports R-Car Gen3 SoCs.
+
+ To compile this driver as a module, choose M here: the
+ module will be called rcar-csi2.
+
 config VIDEO_RCAR_VIN
tristate "R-Car Video Input (VIN) Driver"
depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF && HAS_DMA && 
MEDIA_CONTROLLER
diff --git a/drivers/media/platform/rcar-vin/Makefile 
b/drivers/media/platform/rcar-vin/Makefile
index 48c5632c21dc060b..5ab803d3e7c1aa57 100644
--- a/drivers/media/platform/rcar-vin/Makefile
+++ b/drivers/media/platform/rcar-vin/Makefile
@@ -1,3 +1,4 @@
 rcar-vin-objs = rcar-core.o rcar-dma.o rcar-v4l2.o
 
+obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o
 obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin.o
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c 
b/drivers/media/platform/rcar-vin/rcar-csi2.c
new file mode 100644
index ..49b29d5680f9d80b
--- /dev/null
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -0,0 +1,883 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Renesas R-Car MIPI CSI-2 Receiver
+ *
+ * Copyright (C) 2018 Renesas Electronics Corp.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Register offsets and bits */
+
+/* Control Timing Select */
+#define TREF_REG   0x00
+#define TREF_TREF  BIT(0)
+
+/* Software Reset */
+#define SRST_REG   0x04
+#define SRST_SRST  BIT(0)
+
+/* PHY Operation Control */
+#define PHYCNT_REG 0x08
+#define PHYCNT_SHUTDOWNZ   BIT(17)
+#define PHYCNT_RSTZBIT(16)
+#define PHYCNT_ENABLECLK   BIT(4)
+#define PHYCNT_ENABLE_3BIT(3)
+#define PHYCNT_ENABLE_2BIT(2)
+#define PHYCNT_ENABLE_1BIT(1)
+#define PHYCNT_ENABLE_0BIT(0)
+
+/* Checksum Control */
+#define CHKSUM_REG 0x0c
+#define CHKSUM_ECC_EN  BIT(1)
+#define CHKSUM_CRC_EN  BIT(0)
+
+/*
+ * Channel Data Type Select
+ * VCDT[0-15]:  Channel 1 VCDT[16-31]:  Channel 2
+ * VCDT2[0-15]: Channel 3 VCDT2[16-31]: Channel 4
+ */
+#define VCDT_REG   0x10
+#define VCDT2_REG  0x14
+#define VCDT_VCDTN_EN  BIT(15)
+#define VCDT_SEL_VC(n) (((n) & 0x3) << 8)
+#define VCDT_SEL_DTN_ONBIT(6)
+#define VCDT_SEL_DT(n) (((n) & 0x3f) << 0)
+
+/* Frame Data 

[PATCH v14 0/2] rcar-csi2: add Renesas R-Car MIPI CSI-2

[Niklas Söderlund]

Hi,

This is the latest incarnation of R-Car MIPI CSI-2 receiver driver. It's
based on top of the media-tree and are tested on Renesas Salvator-X
together with adv7482 and the now in tree rcar-vin driver :-)

I hope this is the last incarnation of this patch-set, I do think it is
ready for upstream consumption :-)

* Changes since v13
- Change return rcar_csi2_formats + i to return _csi2_formats[i].
- Add define for PHCLM_STOPSTATECKL.
- Update spelling in comments.
- Update calculation in rcar_csi2_calc_phypll() according to 
  https://linuxtv.org/downloads/v4l-dvb-apis/kapi/csi2.html. The one 
  before v14 did not take into account that 2 bits per sample is 
  transmitted.
- Use Geert's suggestion of (1 << priv->lanes) - 1 instead of switch 
  statement to set correct number of lanes to enable.
- Change hex constants in hsfreqrange_m3w_h3es1[] to lower case to match 
  style of rest of file.
- Switch to %u instead of 0x%x when printing bus type.
- Switch to %u instead of %d for priv->lanes which is unsigned.
- Add MEDIA_BUS_FMT_YUYV8_1X16 to the list of supported formats in 
  rcar_csi2_formats[].
- Fixed bps for MEDIA_BUS_FMT_YUYV10_2X10 to 20 and not 16.
- Set INTSTATE after PL-11 is confirmed to match flow chart in 
  datasheet.
- Change priv->notifier.subdevs == NULL to !priv->notifier.subdevs.
- Add Maxime's and laurent's tags.

* Changes since v12
- Fixed spelling mistakes in commit messages and documentation patch,
  thanks Laurent.
- Mark endpoints in port 1 as optional as it is allowed to not connect a
  VIN to the CSI-2 and instead have it only use its parallel input
  source (for those VIN that have one).
- Added Ack from Sakari, thanks!
- Media bus codes are u32 not unsigned int.
- Ignore error check for v4l2_subdev_call(sd, video, s_stream, 0)
- Do not set subdev host data as it's not used,
  v4l2_set_subdev_hostdata().
- Fixed spelling errors in commit message.
- Add SPDX header
- Rename badly named variable tmp to vcdt_part.
- Cache subdevice in bound callback instead of looking it up in the
  media graph. By doing this rcar_csi2_sd_info() can be removed.
- Print unsigned using %u not %d.
- Call pm_runtime_enable() before calling v4l2_async_register_subdev().
- Dropped of_match_ptr() as OF is required.

* Changes since v11
- Added missing call to v4l2_async_notifier_unregister().
- Fixed missing reg popery in bindings documentation.
- Add Rob's ack to 01/02.
- Dropped 'media:' prefix from patch subjects as it seems they are added
  first when a patch is picked up by the maintainer.
- Fixed typo in comment enpoint -> endpoint, thanks Hans.
- Added Hans Reviewed-by to driver.

* Changes since v10
- Renamed Documentation/devicetree/bindings/media/rcar-csi2.txt to
  Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
- Add extra newline in rcar_csi2_code_to_fmt()
- Use locally stored format information instead of reading it from the
  remote subdevice, Sakari pointed out that the pipeline is validated
  before .s_stream() is called so this is safe.
- Do not check return from media_entity_to_v4l2_subdev() in
  rcar_csi2_start(), Sakari pointed out it can't fail. Also move logic
  to find the remote subdevice is moved to the only user of it,
  rcar_csi2_calc_phypll().
- Move pm_runtime_get_sync() and pm_runtime_put() to
  rcar_csi2_s_stream() and remove rcar_csi2_s_power().
- Add validation of pixel code to rcar_csi2_set_pad_format().
- Remove static rcar_csi2_notify_unbind() as it only printed a debug
  message.

* Changes since v9
- Add reset property to device tree example
- Use BIT(x) instead of (1 << x)
- Use u16 in struct phypll_hsfreqrange to pack struct better.
- Use unsigned int type for loop variable in rcar_csi2_code_to_fmt
- Move fields inside struct struct rcar_csi2_info and struct rcar_csi2
  to pack struct's tighter.
- Use %u instead of %d when printing __u32.
- Don't check return of platform_get_resource(), let
  devm_ioremap_resource() handle it.
- Store quirk workaround for r8a7795 ES1.0 in the data field of struct
  soc_device_attribute.

Changes since v8:
- Updated bindings documentation, thanks Rob!
- Make use of the now in media-tree sub-notifier V4L2 API
- Add delay when resetting the IP to allow for a proper reset
- Fix bug in s_stream error path where the usage count was off if an
  error was hit.
- Add support for H3 ES2.0

Changes since v7:
- Rebase on top of the latest incremental async patches.
- Fix comments on DT documentation.
- Use v4l2_ctrl_g_ctrl_int64() instead of v4l2_g_ext_ctrls().
- Handle try formats in .set_fmt() and .get_fmt().
- Don't call v4l2_device_register_subdev_nodes() as this is not needed
  with the complete() callbacks synchronized.
- Fix line over 80 chars.
- Fix varies spelling mistakes.

Changes since v6:
- Rebased on top of Sakaris fwnode patches.
- Changed of RCAR_CSI2_PAD_MAX to NR_OF_RCAR_CSI2_PAD.
- Remove assumption about unknown media bus type, thanks Sakari for
  pointing this out.
- Created table for supported format 

[PATCH v14 1/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver documentation

[Niklas Söderlund]

Documentation for Renesas R-Car MIPI CSI-2 receiver. The CSI-2 receivers
are located between the video sources (CSI-2 transmitters) and the video
grabbers (VIN) on Gen3 of Renesas R-Car SoC.

Each CSI-2 device is connected to more than one VIN device which
simultaneously can receive video from the same CSI-2 device. Each VIN
device can also be connected to more than one CSI-2 device. The routing
of which links are used is controlled by the VIN devices. There are only
a few possible routes which are set by hardware limitations, which are
different for each SoC in the Gen3 family.

Signed-off-by: Niklas Söderlund 
Acked-by: Rob Herring 
Acked-by: Sakari Ailus 
Reviewed-by: Laurent Pinchart 

---

* Changes since v13
- Add Laurent's tag.
---
 .../bindings/media/renesas,rcar-csi2.txt  | 99 +++
 MAINTAINERS   |  1 +
 2 files changed, 100 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt

diff --git a/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt 
b/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
new file mode 100644
index ..6f71f997dc48eee9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
@@ -0,0 +1,99 @@
+Renesas R-Car MIPI CSI-2
+
+
+The R-Car CSI-2 receiver device provides MIPI CSI-2 capabilities for the
+Renesas R-Car family of devices. It is used in conjunction with the
+R-Car VIN module, which provides the video capture capabilities.
+
+Mandatory properties
+
+ - compatible: Must be one or more of the following
+   - "renesas,r8a7795-csi2" for the R8A7795 device.
+   - "renesas,r8a7796-csi2" for the R8A7796 device.
+
+ - reg: the register base and size for the device registers
+ - interrupts: the interrupt for the device
+ - clocks: reference to the parent clock
+
+The device node shall contain two 'port' child nodes according to the
+bindings defined in Documentation/devicetree/bindings/media/
+video-interfaces.txt. Port 0 shall connect to the CSI-2 source. Port 1
+shall connect to all the R-Car VIN modules that have a hardware
+connection to the CSI-2 receiver.
+
+- Port 0 - Video source (mandatory)
+   - Endpoint 0 - sub-node describing the endpoint that is the video source
+
+- Port 1 - VIN instances (optional)
+   - One endpoint sub-node for every R-Car VIN instance which is connected
+ to the R-Car CSI-2 receiver.
+
+Example:
+
+   csi20: csi2@fea8 {
+   compatible = "renesas,r8a7796-csi2";
+   reg = <0 0xfea8 0 0x1>;
+   interrupts = <0 184 IRQ_TYPE_LEVEL_HIGH>;
+   clocks = < CPG_MOD 714>;
+   power-domains = < R8A7796_PD_ALWAYS_ON>;
+   resets = < 714>;
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   reg = <0>;
+
+   csi20_in: endpoint@0 {
+   reg = <0>;
+   clock-lanes = <0>;
+   data-lanes = <1>;
+   remote-endpoint = <_txb>;
+   };
+   };
+
+   port@1 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   reg = <1>;
+
+   csi20vin0: endpoint@0 {
+   reg = <0>;
+   remote-endpoint = <>;
+   };
+   csi20vin1: endpoint@1 {
+   reg = <1>;
+   remote-endpoint = <>;
+   };
+   csi20vin2: endpoint@2 {
+   reg = <2>;
+   remote-endpoint = <>;
+   };
+   csi20vin3: endpoint@3 {
+   reg = <3>;
+   remote-endpoint = <>;
+   };
+   csi20vin4: endpoint@4 {
+   reg = <4>;
+   remote-endpoint = <>;
+   };
+   csi20vin5: endpoint@5 {
+   reg = <5>;
+   remote-endpoint = <>;
+  

[PATCH] media: vb2: Print the queue pointer in debug messages

[Laurent Pinchart]

When debugging issues that involve more than one video queue, messages
related to multiple queues get interleaved without any easy way to tell
which queue they relate to. Fix this by printing the queue pointer for
all debug messages in the vb2 core and V4L2 layers.

Signed-off-by: Laurent Pinchart 
---
 drivers/media/common/videobuf2/videobuf2-core.c | 193 
 drivers/media/common/videobuf2/videobuf2-v4l2.c |  39 ++---
 2 files changed, 118 insertions(+), 114 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
b/drivers/media/common/videobuf2/videobuf2-core.c
index d3f7bb33a54d..e35e79c32550 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -34,10 +34,10 @@
 static int debug;
 module_param(debug, int, 0644);
 
-#define dprintk(level, fmt, arg...)\
-   do {\
-   if (debug >= level) \
-   pr_info("%s: " fmt, __func__, ## arg);  \
+#define dprintk(q, level, fmt, arg...) \
+   do {\
+   if (debug >= level) \
+   pr_info("(q=%p) %s: " fmt, q, __func__, ## arg);\
} while (0)
 
 #ifdef CONFIG_VIDEO_ADV_DEBUG
@@ -51,8 +51,8 @@ module_param(debug, int, 0644);
  */
 
 #define log_memop(vb, op)  \
-   dprintk(2, "call_memop(%p, %d, %s)%s\n",\
-   (vb)->vb2_queue, (vb)->index, #op,  \
+   dprintk((vb)->vb2_queue, 2, "call_memop(%p, %d, %s)%s\n",   \
+   (vb)->index, #op,   \
(vb)->vb2_queue->mem_ops->op ? "" : " (nop)")
 
 #define call_memop(vb, op, args...)\
@@ -90,7 +90,7 @@ module_param(debug, int, 0644);
 })
 
 #define log_qop(q, op) \
-   dprintk(2, "call_qop(%p, %s)%s\n", q, #op,  \
+   dprintk(q, 2, "call_qop(%p, %s)%s\n", q, #op,   \
(q)->ops->op ? "" : " (nop)")
 
 #define call_qop(q, op, args...)   \
@@ -113,8 +113,8 @@ module_param(debug, int, 0644);
 })
 
 #define log_vb_qop(vb, op, args...)\
-   dprintk(2, "call_vb_qop(%p, %d, %s)%s\n",   \
-   (vb)->vb2_queue, (vb)->index, #op,  \
+   dprintk((vb)->vb2_queue, 2, "call_vb_qop(%p, %d, %s)%s\n",  \
+   (vb)->index, #op,   \
(vb)->vb2_queue->ops->op ? "" : " (nop)")
 
 #define call_vb_qop(vb, op, args...)   \
@@ -241,7 +241,8 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
for (plane = 0; plane < vb->num_planes; ++plane) {
call_void_memop(vb, put, vb->planes[plane].mem_priv);
vb->planes[plane].mem_priv = NULL;
-   dprintk(3, "freed plane %d of buffer %d\n", plane, vb->index);
+   dprintk(vb->vb2_queue, 3, "freed plane %d of buffer %d\n",
+   plane, vb->index);
}
 }
 
@@ -311,7 +312,7 @@ static void __setup_offsets(struct vb2_buffer *vb)
for (plane = 0; plane < vb->num_planes; ++plane) {
vb->planes[plane].m.offset = off;
 
-   dprintk(3, "buffer %d, plane %d offset 0x%08lx\n",
+   dprintk(q, 3, "buffer %d, plane %d offset 0x%08lx\n",
vb->index, plane, off);
 
off += vb->planes[plane].length;
@@ -342,7 +343,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
vb2_memory memory,
/* Allocate videobuf buffer structures */
vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
if (!vb) {
-   dprintk(1, "memory alloc for buffer struct failed\n");
+   dprintk(q, 1, "memory alloc for buffer struct 
failed\n");
break;
}
 
@@ -362,7 +363,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
vb2_memory memory,
if (memory == VB2_MEMORY_MMAP) {
ret = __vb2_buf_mem_alloc(vb);
if (ret) {
-   dprintk(1, "failed allocating memory for buffer 
%d\n",
+   dprintk(q, 1, "failed allocating memory for 
buffer %d\n",
buffer);
q->bufs[vb->index] = NULL;
kfree(vb);
@@ -376,7 +377,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, 

[Solved] How to proceed ? => [PATCH ?] EM28xx driver ?

[mjs]

Solved.
See msg 130325.

Thanks,
  Marcel

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[Laurent Pinchart]

Hi Jacopo,

On Thursday, 26 April 2018 21:40:56 EEST jacopo mondi wrote:
> On Mon, Apr 23, 2018 at 12:27:39PM +0300, Laurent Pinchart wrote:
> > On Thursday, 19 April 2018 12:31:02 EEST Jacopo Mondi wrote:
> >> Add support for storing image format information in DRM bridges with
> >> associated helper function.
> >> 
> >> This patch replicates for bridges what
> >> 'drm_display_info_set_bus_formats()'
> >> is for connectors.
> >> 
> >> Signed-off-by: Jacopo Mondi 
> >> ---
> >>  drivers/gpu/drm/drm_bridge.c | 30 ++
> >>  include/drm/drm_bridge.h |  8 
> >>  2 files changed, 38 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 1638bfe..e2ad098 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >>  }
> >>  
> >>  /**
> >> + * drm_bridge_set_bus_formats() - set bridge supported image formats
> >> + * @bridge: the bridge to set image formats in
> >> + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
> > 
> > Why the \ (here and below) ?
> 
> mmm, I can't tell where I made that up from... Will change with
> MEDIA_BUS_FMT_*
> 
> >> + * @num_formats: number of elements in the @formats array
> >> + *
> >> + * Store a list of supported image formats in a bridge.
> >> + * See MEDIA_BUS_FMT_* definitions in
> >> include/uapi/linux/media-bus-format.h for
> >> + * a full list of available formats.
> >> + */
> >> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> >> *formats,
> >> + unsigned int num_formats)
> >> +{
> >> +  u32 *fmts;
> >> +
> >> +  if (!formats || !num_formats)
> >> +  return -EINVAL;
> >> +
> >> +  fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
> > 
> > This memory will be leaked when the bridge is destroyed.
> 
> Right. I'm afraid this may open a pandora box, but, ehm, where is the
> bridge objects lifetime managed? The best I can think of is to use the
> resource managed version of kmemdup, associating that memory to
> the drm_device device object. That means the memory will be freed at
> DRM pipeline teardown time only if I'm not wrong. Can a bridge be
> destroyed before that?

The lifetime of the bridge is independent from the lifetime of the drm_device, 
so that won't work. It looks like we need to add a bridge cleanup operation 
:-/ You're right, it opens pandora's box.

My recommendation is to add a .release() operation to the bridge ops 
structure, and to implement a drm_bridge_cleanup() function that frees the 
bus_formats memory. Then, a drm_bridge_release() function can wrap the 
release() ops, and that should be called from the bridge driver .remove() 
handler. Or even butter, call the drm_bridge_release() function 
drm_bridge_put(), to pave the way for a reference-count-based implementation.

> >> +  if (!fmts)
> >> +  return -ENOMEM;
> >> +
> >> +  kfree(bridge->bus_formats);
> >> +  bridge->bus_formats = fmts;
> >> +  bridge->num_bus_formats = num_formats;
> >> +
> >> +  return 0;
> >> +}
> >> +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> >> +
> >> +/**
> >>   * DOC: bridge callbacks
> >>   *
> >>   * The _bridge_funcs ops are populated by the bridge driver. The
> >>   DRM
> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >> index 3270fec..6b3648c 100644
> >> --- a/include/drm/drm_bridge.h
> >> +++ b/include/drm/drm_bridge.h
> >> @@ -258,6 +258,9 @@ struct drm_bridge_timings {
> >>   * @encoder: encoder to which this bridge is connected
> >>   * @next: the next bridge in the encoder chain
> >>   * @of_node: device node pointer to the bridge
> >> + * @bus_formats: wire image formats. Array of @num_bus_formats
> >> MEDIA_BUS_FMT\_
> >> + * elements
> >> + * @num_bus_formats: size of @bus_formats array
> >>   * @list: to keep track of all added bridges
> >>   * @timings: the timing specification for the bridge, if any (may
> >>   * be NULL)
> >> @@ -271,6 +274,9 @@ struct drm_bridge {
> >>  #ifdef CONFIG_OF
> >>struct device_node *of_node;
> >>  #endif
> >> +  const u32 *bus_formats;
> >> +  unsigned int num_bus_formats;
> >> +
> >>struct list_head list;
> >>const struct drm_bridge_timings *timings;
> >> 
> >> @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >>struct drm_display_mode *adjusted_mode);
> >>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >>  void drm_bridge_enable(struct drm_bridge *bridge);
> >> +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> >> *fmts,
> >> + unsigned int num_fmts);
> >> 
> >>  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,

-- 
Regards,

Laurent Pinchart

[Solved] Problem retrieving zl10353 information: Resource temporarily unavailable (but signal =71% ?)

[mjs]

Solved.
See msg130325.

Thanks,
  Marcel

[Solved] Hm, seems I got me a \"msg08693 (2009)\" situation, smelly (hot) dvb-t electronics

[mjs]

Solved.
See msg130325

Thanks,
  Marcel

[Solved] Help needed for zolid usb dvb-t stick

[mjs]

Solved.
See msg130325.

Thanks,
  Marcel

[Solved] ZOLID new usb dvb-t

[mjs]

Solved.
See msg130325

Thanks,
  Marcel

[PATCH] [2] [superseded] Add new dvb-t board \":Zolid Hybrid Tv Stick\"

[mjs]

superseded by msg130325

Thanks,
  Marcel

Re: [PATCH] [superseded] Add new dvb-t board \":Zolid Hybrid Tv Stick\"

[mjs]

superseded by msg130325

Thanks,
  Marcel

[PATCH] [superseded] remove 2 excess lines in driver module em28xx

[mjs]

superseded by msg130250

Thanks,
  Marcel

[PATCH] [3] Add new dvb-t board ":Zolid Hybrid Tv Stick"

[mjs]

From 40e6302a75521d3a2aa8d67b2945b4940f98427b Mon Sep 17 00:00:00 2001
From: Marcel Stork 
Date: Thu, 26 Apr 2018 21:17:02 +0200
Subject: [PATCH] Add new dvb-t board ":Zolid Hybrid Tv Stick".

Extra code to be able to use this stick, only digital, not analog nor 
remote-control.

Changes to be committed:
modified: drivers/media/usb/em28xx/em28xx-cards.c
modified: drivers/media/usb/em28xx/em28xx-dvb.c
modified: drivers/media/usb/em28xx/em28xx.h

Signed-off-by: Marcel Stork 

---
 drivers/media/usb/em28xx/em28xx-cards.c | 30 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |  1 +
 drivers/media/usb/em28xx/em28xx.h   |  1 +
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c 
b/drivers/media/usb/em28xx/em28xx-cards.c
index 6e0e67d2..a62e6f4b 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -87,6 +87,21 @@ static const struct em28xx_reg_seq default_digital[] = {
{   -1, -1, -1, -1},
 };
 
+/* Board :Zolid Hybrid Tv Stick */
+static struct em28xx_reg_seq zolid_tuner[] = {
+   {EM2820_R08_GPIO_CTRL,  0xfd,   0xff,   100},
+   {EM2820_R08_GPIO_CTRL,  0xfe,   0xff,   100},
+   {   -1, -1, 
-1,  -1},
+};
+
+static struct em28xx_reg_seq zolid_digital[] = {
+   {EM2820_R08_GPIO_CTRL,  0x6a,   0xff,   100},
+   {EM2820_R08_GPIO_CTRL,  0x7a,   0xff,   100},
+   {EM2880_R04_GPO,0x04,   0xff,   100},
+   {EM2880_R04_GPO,0x0c,   0xff,   100},
+   {   -1, -1, 
-1,  -1},
+};
+
 /* Board Hauppauge WinTV HVR 900 analog */
 static const struct em28xx_reg_seq hauppauge_wintv_hvr_900_analog[] = {
{EM2820_R08_GPIO_CTRL,  0x2d,   ~EM_GPIO_4, 10},
@@ -666,6 +681,16 @@ const struct em28xx_board em28xx_boards[] = {
.tuner_type= TUNER_ABSENT,
.is_webcam = 1, /* To enable sensor probe */
},
+   [EM2882_BOARD_ZOLID_HYBRID_TV_STICK] = {
+   .name   = ":ZOLID HYBRID TV STICK",
+   .tuner_type = TUNER_XC2028,
+   .tuner_gpio = zolid_tuner,
+   .decoder= EM28XX_TVP5150,
+   .xclk   = EM28XX_XCLK_FREQUENCY_12MHZ,
+   .mts_firmware   = 1,
+   .has_dvb= 1,
+   .dvb_gpio   = zolid_digital,
+   },
[EM2750_BOARD_DLCW_130] = {
/* Beijing Huaqi Information Digital Technology Co., Ltd */
.name  = "Huaqi DLCW-130",
@@ -2493,7 +2518,7 @@ struct usb_device_id em28xx_id_table[] = {
.driver_info = EM2820_BOARD_UNKNOWN },
{ USB_DEVICE(0xeb1a, 0x2881),
.driver_info = EM2820_BOARD_UNKNOWN },
-   { USB_DEVICE(0xeb1a, 0x2883),
+   { USB_DEVICE(0xeb1a, 0x2883), /* used by :Zolid Hybrid Tv Stick */
.driver_info = EM2820_BOARD_UNKNOWN },
{ USB_DEVICE(0xeb1a, 0x2868),
.driver_info = EM2820_BOARD_UNKNOWN },
@@ -2688,6 +2713,7 @@ static const struct em28xx_hash_table 
em28xx_eeprom_hash[] = {
{0xb8846b20, EM2881_BOARD_PINNACLE_HYBRID_PRO, TUNER_XC2028},
{0x63f653bd, EM2870_BOARD_REDDO_DVB_C_USB_BOX, TUNER_ABSENT},
{0x4e913442, EM2882_BOARD_DIKOM_DK300, TUNER_XC2028},
+   {0x85dd871e, EM2882_BOARD_ZOLID_HYBRID_TV_STICK, TUNER_XC2028},
 };
 
 /* I2C devicelist hash table for devices with generic USB IDs */
@@ -2699,6 +2725,7 @@ static const struct em28xx_hash_table em28xx_i2c_hash[] = 
{
{0xc51200e3, EM2820_BOARD_GADMEI_TVR200, TUNER_LG_PAL_NEW_TAPC},
{0x4ba50080, EM2861_BOARD_GADMEI_UTV330PLUS, TUNER_TNF_5335MF},
{0x6b800080, EM2874_BOARD_LEADERSHIP_ISDBT, TUNER_ABSENT},
+   {0x27e10080, EM2882_BOARD_ZOLID_HYBRID_TV_STICK, TUNER_XC2028},
 };
 
 /* NOTE: introduce a separate hash table for devices with 16 bit eeproms */
@@ -3187,6 +3214,7 @@ void em28xx_setup_xc3028(struct em28xx *dev, struct 
xc2028_ctrl *ctl)
case EM2880_BOARD_TERRATEC_HYBRID_XS:
case EM2880_BOARD_TERRATEC_HYBRID_XS_FR:
case EM2881_BOARD_PINNACLE_HYBRID_PRO:
+   case EM2882_BOARD_ZOLID_HYBRID_TV_STICK:
ctl->demod = XC3028_FE_ZARLINK456;
break;
case EM2880_BOARD_HAUPPAUGE_WINTV_HVR_900_R2:
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c 
b/drivers/media/usb/em28xx/em28xx-dvb.c
index a54cb8dc..67b16036 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ 

[PATCH v2 1/2] dt-bindings: media: renesas-ceu: Add R-Mobile R8A7740

[Jacopo Mondi]

Add R-Mobile A1 R8A7740 SoC to the list of compatible values for the CEU
unit.

Signed-off-by: Jacopo Mondi 
---
 Documentation/devicetree/bindings/media/renesas,ceu.txt | 7 ---
 drivers/media/platform/renesas-ceu.c| 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/renesas,ceu.txt 
b/Documentation/devicetree/bindings/media/renesas,ceu.txt
index 3fc66df..8a7a616 100644
--- a/Documentation/devicetree/bindings/media/renesas,ceu.txt
+++ b/Documentation/devicetree/bindings/media/renesas,ceu.txt
@@ -2,14 +2,15 @@ Renesas Capture Engine Unit (CEU)
 --
 
 The Capture Engine Unit is the image capture interface found in the Renesas
-SH Mobile and RZ SoCs.
+SH Mobile, R-Mobile and RZ SoCs.
 
 The interface supports a single parallel input with data bus width of 8 or 16
 bits.
 
 Required properties:
-- compatible: Shall be "renesas,r7s72100-ceu" for CEU units found in RZ/A1H
-  and RZ/A1M SoCs.
+- compatible: Shall be one of the following values:
+   "renesas,r7s72100-ceu" for CEU units found in RZ/A1H and RZ/A1M SoCs
+   "renesas,r8a7740-ceu" for CEU units found in R-Mobile A1 R8A7740 SoCs
 - reg: Registers address base and size.
 - interrupts: The interrupt specifier.
 
diff --git a/drivers/media/platform/renesas-ceu.c 
b/drivers/media/platform/renesas-ceu.c
index 6599dba..c964a56 100644
--- a/drivers/media/platform/renesas-ceu.c
+++ b/drivers/media/platform/renesas-ceu.c
@@ -1545,6 +1545,7 @@ static const struct ceu_data ceu_data_sh4 = {
 #if IS_ENABLED(CONFIG_OF)
 static const struct of_device_id ceu_of_match[] = {
{ .compatible = "renesas,r7s72100-ceu", .data = _data_rz },
+   { .compatible = "renesas,r8a7740-ceu", .data = _data_rz },
{ }
 };
 MODULE_DEVICE_TABLE(of, ceu_of_match);
-- 
2.7.4

[PATCH v2 2/2] ARM: dts: r8a7740: Add CEU0

[Jacopo Mondi]

Describe CEU0 peripheral for Renesas R-Mobile A1 R8A7740 Soc.

Reported-by: Geert Uytterhoeven 
Signed-off-by: Jacopo Mondi 
---
 arch/arm/boot/dts/r8a7740.dtsi | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7740.dtsi b/arch/arm/boot/dts/r8a7740.dtsi
index afd3bc5..508d934 100644
--- a/arch/arm/boot/dts/r8a7740.dtsi
+++ b/arch/arm/boot/dts/r8a7740.dtsi
@@ -67,6 +67,16 @@
power-domains = <_d4>;
};
 
+   ceu0: ceu@fe91 {
+   reg = <0xfe91 0x3000>;
+   compatible = "renesas,r8a7740-ceu";
+   interrupts = ;
+   clocks = <_clks R8A7740_CLK_CEU20>;
+   clock-names = "ceu20";
+   power-domains = <_a4r>;
+   status = "disabled";
+   };
+
cmt1: timer@e6138000 {
compatible = "renesas,cmt-48-r8a7740", "renesas,cmt-48";
reg = <0xe6138000 0x170>;
-- 
2.7.4

[PATCH v2 0/2]

[Jacopo Mondi]

Hello,
   this small series add R-Mobile A1 R8A7740 to the list of CEU supported
SoCs, and adds the CEU node to r8a7740.dtsi.

All the information on CEU clocks, power domains and memory regions have been
deducted from the now-deleted board file:
arch/arm/mach-shmobile/board-armadillo800eva.c

Thanks
   j

v1 -> v2:
- Enlarge the memory range as suggested by Simon
- Fix power domain, as reported by Simon
- s/Enable/[Describe|Add] in commit message

Jacopo Mondi (2):
  dt-bindings: media: renesas-ceu: Add R-Mobile R8A7740
  ARM: dts: r8a7740: Add CEU0

 Documentation/devicetree/bindings/media/renesas,ceu.txt |  7 ---
 arch/arm/boot/dts/r8a7740.dtsi  | 10 ++
 drivers/media/platform/renesas-ceu.c|  1 +
 3 files changed, 15 insertions(+), 3 deletions(-)

--
2.7.4

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[jacopo mondi]

Hi Peter,

On Sun, Apr 22, 2018 at 10:02:23PM +0200, Peter Rosin wrote:
> On 2018-04-19 11:31, Jacopo Mondi wrote:
> > Add support for storing image format information in DRM bridges with
> > associated helper function.
> >
> > This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> > is for connectors.
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 30 ++
> >  include/drm/drm_bridge.h |  8 
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 1638bfe..e2ad098 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >  }
> >
> >  /**
> > + * drm_bridge_set_bus_formats() - set bridge supported image formats
> > + * @bridge: the bridge to set image formats in
> > + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
> > + * @num_formats: number of elements in the @formats array
> > + *
> > + * Store a list of supported image formats in a bridge.
> > + * See MEDIA_BUS_FMT_* definitions in 
> > include/uapi/linux/media-bus-format.h for
> > + * a full list of available formats.
> > + */
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 
> > *formats,
> > +  unsigned int num_formats)
> > +{
> > +   u32 *fmts;
> > +
> > +   if (!formats || !num_formats)
> > +   return -EINVAL;
>
> I see no compelling reason to forbid restoring the number of reported
> input formats to zero? I can't think of a use right now of course, but it
> seems a bit odd all the same.

It is, you're right. Will fix in v2 and will allow bridges to just
restore formats to 0 (as it is done for drm_connectors, by the way)

Thanks
   j
>
> Cheers,
> Peter
>
> > +
> > +   fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
> > +   if (!fmts)
> > +   return -ENOMEM;
> > +
> > +   kfree(bridge->bus_formats);
> > +   bridge->bus_formats = fmts;
> > +   bridge->num_bus_formats = num_formats;
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> > +
> > +/**
> >   * DOC: bridge callbacks
> >   *
> >   * The _bridge_funcs ops are populated by the bridge driver. The DRM
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 3270fec..6b3648c 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -258,6 +258,9 @@ struct drm_bridge_timings {
> >   * @encoder: encoder to which this bridge is connected
> >   * @next: the next bridge in the encoder chain
> >   * @of_node: device node pointer to the bridge
> > + * @bus_formats: wire image formats. Array of @num_bus_formats 
> > MEDIA_BUS_FMT\_
> > + * elements
> > + * @num_bus_formats: size of @bus_formats array
> >   * @list: to keep track of all added bridges
> >   * @timings: the timing specification for the bridge, if any (may
> >   * be NULL)
> > @@ -271,6 +274,9 @@ struct drm_bridge {
> >  #ifdef CONFIG_OF
> > struct device_node *of_node;
> >  #endif
> > +   const u32 *bus_formats;
> > +   unsigned int num_bus_formats;
> > +
> > struct list_head list;
> > const struct drm_bridge_timings *timings;
> >
> > @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> > struct drm_display_mode *adjusted_mode);
> >  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >  void drm_bridge_enable(struct drm_bridge *bridge);
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> > +  unsigned int num_fmts);
> >
> >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >
>


signature.asc
Description: PGP signature

Re: [PATCH 1/8] drm: bridge: Add support for static image formats

[jacopo mondi]

Hi Laurent,

On Mon, Apr 23, 2018 at 12:27:39PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thursday, 19 April 2018 12:31:02 EEST Jacopo Mondi wrote:
> > Add support for storing image format information in DRM bridges with
> > associated helper function.
> >
> > This patch replicates for bridges what 'drm_display_info_set_bus_formats()'
> > is for connectors.
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 30 ++
> >  include/drm/drm_bridge.h |  8 
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 1638bfe..e2ad098 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -157,6 +157,36 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >  }
> >
> >  /**
> > + * drm_bridge_set_bus_formats() - set bridge supported image formats
> > + * @bridge: the bridge to set image formats in
> > + * @formats: array of MEDIA_BUS_FMT\_ supported image formats
>
> Why the \ (here and below) ?

mmm, I can't tell where I made that up from... Will change with
MEDIA_BUS_FMT_*

>
> > + * @num_formats: number of elements in the @formats array
> > + *
> > + * Store a list of supported image formats in a bridge.
> > + * See MEDIA_BUS_FMT_* definitions in include/uapi/linux/media-bus-format.h
> > for
> > + * a full list of available formats.
> > + */
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32
> > *formats,
> > +  unsigned int num_formats)
> > +{
> > +   u32 *fmts;
> > +
> > +   if (!formats || !num_formats)
> > +   return -EINVAL;
> > +
> > +   fmts = kmemdup(formats, sizeof(*formats) * num_formats, GFP_KERNEL);
>
> This memory will be leaked when the bridge is destroyed.

Right. I'm afraid this may open a pandora box, but, ehm, where is the
bridge objects lifetime managed? The best I can think of is to use the
resource managed version of kmemdup, associating that memory to
the drm_device device object. That means the memory will be freed at
DRM pipeline teardown time only if I'm not wrong. Can a bridge be
destroyed before that?

Thanks
   j

>
> > +   if (!fmts)
> > +   return -ENOMEM;
> > +
> > +   kfree(bridge->bus_formats);
> > +   bridge->bus_formats = fmts;
> > +   bridge->num_bus_formats = num_formats;
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL(drm_bridge_set_bus_formats);
> > +
> > +/**
> >   * DOC: bridge callbacks
> >   *
> >   * The _bridge_funcs ops are populated by the bridge driver. The DRM
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 3270fec..6b3648c 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -258,6 +258,9 @@ struct drm_bridge_timings {
> >   * @encoder: encoder to which this bridge is connected
> >   * @next: the next bridge in the encoder chain
> >   * @of_node: device node pointer to the bridge
> > + * @bus_formats: wire image formats. Array of @num_bus_formats
> > MEDIA_BUS_FMT\_
> > + * elements
> > + * @num_bus_formats: size of @bus_formats array
> >   * @list: to keep track of all added bridges
> >   * @timings: the timing specification for the bridge, if any (may
> >   * be NULL)
> > @@ -271,6 +274,9 @@ struct drm_bridge {
> >  #ifdef CONFIG_OF
> > struct device_node *of_node;
> >  #endif
> > +   const u32 *bus_formats;
> > +   unsigned int num_bus_formats;
> > +
> > struct list_head list;
> > const struct drm_bridge_timings *timings;
> >
> > @@ -296,6 +302,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> > struct drm_display_mode *adjusted_mode);
> >  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >  void drm_bridge_enable(struct drm_bridge *bridge);
> > +int drm_bridge_set_bus_formats(struct drm_bridge *bridge, const u32 *fmts,
> > +  unsigned int num_fmts);
> >
> >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>


signature.asc
Description: PGP signature

[PATCH] [2] [ do not use] Add new dvb-t board \":Zolid Hybrid Tv Stick\"

[mjs]

I am sorry, I have made a mistake.
I will sent in a correct version.

Thanks,
  Marcel

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[jacopo mondi]

Hello Mita-san,

On Fri, Apr 27, 2018 at 01:17:55AM +0900, Akinobu Mita wrote:
> 2018-04-26 7:40 GMT+09:00 Laurent Pinchart 
> :
> > Hi Mita-san,
> >
> > On Wednesday, 25 April 2018 19:19:11 EEST Akinobu Mita wrote:
> >> 2018-04-24 0:54 GMT+09:00 Akinobu Mita :
> >> > 2018-04-23 18:17 GMT+09:00 Laurent Pinchart:
> >> >> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
> >> >>> This adds a device tree binding documentation for OV7720/OV7725 sensor.
> >> >>>
> >> >>> Cc: Jacopo Mondi 
> >> >>> Cc: Laurent Pinchart 
> >> >>> Cc: Hans Verkuil 
> >> >>> Cc: Sakari Ailus 
> >> >>> Cc: Mauro Carvalho Chehab 
> >> >>> Cc: Rob Herring 
> >> >>> Reviewed-by: Rob Herring 
> >> >>> Reviewed-by: Jacopo Mondi 
> >> >>> Signed-off-by: Akinobu Mita 
> >> >>> ---
> >> >>> * v3
> >> >>> - Add Reviewed-by: lines
> >> >>>
> >> >>>  .../devicetree/bindings/media/i2c/ov772x.txt   | 42 
> >> >>> +++
> >> >>>  MAINTAINERS|  1 +
> >> >>>  2 files changed, 43 insertions(+)
> >> >>>  create mode 100644
> >> >>>  Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>>
> >> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>> b/Documentation/devicetree/bindings/media/i2c/ov772x.txt new file mode
> >> >>> 100644
> >> >>> index 000..b045503
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>> @@ -0,0 +1,42 @@
> >> >>> +* Omnivision OV7720/OV7725 CMOS sensor
> >> >>> +
> >> >>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions
> >> >>> output,
> >> >>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It 
> >> >>> also
> >> >>> can +support the YUV422, RGB565/555/444, GRB422 or raw RGB output
> >> >>> formats. +
> >> >>> +Required Properties:
> >> >>> +- compatible: shall be one of
> >> >>> + "ovti,ov7720"
> >> >>> + "ovti,ov7725"
> >> >>> +- clocks: reference to the xclk input clock.
> >> >>> +- clock-names: shall be "xclk".
> >> >>
> >> >> As there's a single clock we could omit clock-names, couldn't we ?
> >> >
> >> > Sounds good.
> >> >
> >> > I'll prepare another patch that replaces the clock consumer ID argument
> >> > of clk_get() from "xclk" to NULL, and remove the above line in this
> >> > bindings.
> >>
> >> I thought it's easy to do.  However, there is a non-DT user
> >> (arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.
> >>
> >> This can be resolved by retrying clk_get() with NULL if no entry
> >> with "xclk".  But should we do so or leave as is?
> >
> > How about patching the board code to register the clock alias with
> >
> > clk_add_alias(NULL, "0-0021", "video_clk", NULL);
>
> Sounds good.
>
> But I'm a bit worried about whether clk_add_alias() can be called with
> alias == NULL.  I couldn't find such use case.
>
> Probably Jacopo can verify whether it works or not with v4 patchset.

Yes, you can use NULL to register a clock alias. Just make sure to drop the
clock name in ov772x driver (I have just verified the following works :)

diff --git a/arch/sh/boards/mach-migor/setup.c 
b/arch/sh/boards/mach-migor/setup.c
index 3d7d004..2deee53 100644
--- a/arch/sh/boards/mach-migor/setup.c
+++ b/arch/sh/boards/mach-migor/setup.c
@@ -592,7 +592,7 @@ static int __init migor_devices_setup(void)
}

/* Add a clock alias for ov7725 xclk source. */
-   clk_add_alias("xclk", "0-0021", "video_clk", NULL);
+   clk_add_alias(NULL, "0-0021", "video_clk", NULL);

/* Register GPIOs for video sources. */
gpiod_add_lookup_table(_gpios);
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..e1f4076 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1281,7 +1281,7 @@ static int ov772x_probe(struct i2c_client *client,
if (priv->hdl.error)
return priv->hdl.error;

-   priv->clk = clk_get(>dev, "xclk");
+   priv->clk = clk_get(>dev, NULL);
if (IS_ERR(priv->clk)) {
dev_err(>dev, "Unable
Thanks
   j


signature.asc
Description: PGP signature

Re: [PATCH] media: zoran: move to dma-mapping interface

[kbuild test robot]

Hi Arnd,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.17-rc2 next-20180426]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Arnd-Bergmann/media-zoran-move-to-dma-mapping-interface/20180426-032120
base:   git://linuxtv.org/media_tree.git master
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/media/pci/zoran/zoran_driver.c:419:33: sparse: incorrect type in 
>> argument 2 (different base types) @@expected unsigned long long 
>> [unsigned] [usertype] addr @@got nsigned long long [unsigned] [usertype] 
>> addr @@
   drivers/media/pci/zoran/zoran_driver.c:419:33:expected unsigned long 
long [unsigned] [usertype] addr
   drivers/media/pci/zoran/zoran_driver.c:419:33:got restricted __le32 
[assigned] [usertype] frag_tab

vim +419 drivers/media/pci/zoran/zoran_driver.c

   395  
   396  /* free the MJPEG grab buffers */
   397  static void jpg_fbuffer_free(struct zoran_fh *fh)
   398  {
   399  struct zoran *zr = fh->zr;
   400  int i, j, off;
   401  unsigned char *mem;
   402  __le32 frag_tab;
   403  struct zoran_buffer *buffer;
   404  
   405  dprintk(4, KERN_DEBUG "%s: %s\n", ZR_DEVNAME(zr), __func__);
   406  
   407  for (i = 0, buffer = >buffers.buffer[0];
   408   i < fh->buffers.num_buffers; i++, buffer++) {
   409  if (!buffer->jpg.frag_tab)
   410  continue;
   411  
   412  if (fh->buffers.need_contiguous) {
   413  frag_tab = buffer->jpg.frag_tab[0];
   414  
   415  if (frag_tab) {
   416  mem = buffer->jpg.frag_virt_tab[0];
   417  for (off = 0; off < 
fh->buffers.buffer_size; off += PAGE_SIZE)
   418  
ClearPageReserved(virt_to_page(mem + off));
 > 419  dma_unmap_single(>pci_dev->dev, 
 > frag_tab, PAGE_SIZE, DMA_FROM_DEVICE);
   420  kfree(mem);
   421  buffer->jpg.frag_tab[0] = 0;
   422  buffer->jpg.frag_tab[1] = 0;
   423  }
   424  } else {
   425  for (j = 0; j < fh->buffers.buffer_size / 
PAGE_SIZE; j++) {
   426  frag_tab = buffer->jpg.frag_tab[2 * j];
   427  
   428  if (!frag_tab)
   429  break;
   430  
ClearPageReserved(virt_to_page(buffer->jpg.frag_virt_tab[j]));
   431  dma_unmap_single(>pci_dev->dev, 
le32_to_cpu(frag_tab), PAGE_SIZE, DMA_FROM_DEVICE);
   432  free_page((unsigned 
long)buffer->jpg.frag_virt_tab[j]);
   433  buffer->jpg.frag_virt_tab[j] = NULL;
   434  buffer->jpg.frag_tab[2 * j] = 0;
   435  buffer->jpg.frag_tab[2 * j + 1] = 0;
   436  }
   437  }
   438  
   439  dma_unmap_single(>pci_dev->dev, 
buffer->jpg.frag_tab_dma, PAGE_SIZE, DMA_TO_DEVICE);
   440  free_page((unsigned long)buffer->jpg.frag_tab);
   441  free_page((unsigned long)buffer->jpg.frag_virt_tab);
   442  buffer->jpg.frag_virt_tab = NULL;
   443  buffer->jpg.frag_tab = NULL;
   444  }
   445  
   446  fh->buffers.allocated = 0;
   447  }
   448  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

[PATCH 1/7] Disable VIDEO_ADV748X for kernels older than 4.8

[Brad Love]

Needs i2c_new_secondary_device

Signed-off-by: Brad Love 
---
 v4l/versions.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/v4l/versions.txt b/v4l/versions.txt
index 6220485..ae0731d 100644
--- a/v4l/versions.txt
+++ b/v4l/versions.txt
@@ -13,6 +13,10 @@ RADIO_WL128X
 # needs *probe_new in struct i2c_driver
 VIDEO_OV5670
 
+[4.8.0]
+# needs i2c_new_secondary_device
+VIDEO_ADV748X
+
 [4.7.0]
 # needs i2c_mux_alloc
 DVB_RTL2830
-- 
2.7.4

[PATCH 2/7] Disable additional drivers requiring gpio/consumer.h

[Brad Love]

One driver migrated to 3.13 from 3.5

Signed-off-by: Brad Love 
---
 v4l/versions.txt | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/v4l/versions.txt b/v4l/versions.txt
index ae0731d..2306830 100644
--- a/v4l/versions.txt
+++ b/v4l/versions.txt
@@ -107,6 +107,15 @@ VIDEO_VIM2M
 [3.13.0]
 # needs gpio/consumer.h
 RADIO_SI4713
+VIDEO_OV2685
+VIDEO_OV5695
+VIDEO_OV9650
+VIDEO_MT9T112
+SOC_CAMERA_MT9T112
+VIDEO_OV772X
+SOC_CAMERA_OV772X
+VIDEO_TW9910
+SOC_CAMERA_TW9910
 
 [3.12.0]
 # BIN_ATTR_RW was changed
@@ -221,7 +230,6 @@ SOC_CAMERA
 SOC_CAMERA_MT9V022
 SOC_CAMERA_MT9M001
 SOC_CAMERA_MT9T031
-SOC_CAMERA_OV772X
 SOC_CAMERA_PLATFORM
 # Needs of_match_ptr
 VIDEO_THS8200
-- 
2.7.4

[PATCH 0/7] media_build: various kernel version fixes

[Brad Love]

This first four patches in this set disables drivers which cannot
be compiled before a specific kernel revision.

To fix of_find_i2c_device_by_node|of_find_i2c_adapter_by_node in
kernels 3.5 to 3.11.x the correct header is included.

The frame_vector.c wildcard check also appears to be broken for me.
The check is changed from using relative patch to absolute path,
and verifying frame_vector.c is in the build dir (v4l/) instead
of in the linux/ directory.

Lastly, I have one new addition to contemplate. I maintain driver
packages for a lot of different kernel revisions on a lot of different
architectures and some times make_config_compat.pl incorrectly
enables backport options which cause build failure. Instead of
making the config check more complicated I propose creation and
inclusion of an empty config-mycompat.h, after config-compat.h is included
in compat.h, which would allow for overriding any options necessary
due to symbols/macros/etc already existing in the target kernel.
config-mycompat.h would be touched before make_config_compat.h is
called and deleted during distclean, allowing a builder to copy any
overrides into the header before starting the compilation process.
This would allow usage of the media_build system without having to
supply out of tree patches to correct the 'bad' options. If I
somehow missed this functionality a pointer to it would be lovely.


Brad Love (7):
  Disable VIDEO_ADV748X for kernels older than 4.8
  Disable additional drivers requiring gpio/consumer.h
  Disable DVBC8SECTPFE for kernels older than 3.5
  Disable SOC_CAMERA for kernels older than 3.5
  Header location fix for 3.5.0 to 3.11.x
  Fix frame vector wildcard file check
  Add config-compat.h override config-mycompat.h

 v4l/Makefile |  5 +++--
 v4l/compat.h | 14 ++
 v4l/versions.txt | 18 --
 3 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH 3/7] Disable DVBC8SECTPFE for kernels older than 3.5

[Brad Love]

Needs of_find_i2c_adapter_by_node

Signed-off-by: Brad Love 
---
 v4l/versions.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/v4l/versions.txt b/v4l/versions.txt
index 2306830..f5e9a42 100644
--- a/v4l/versions.txt
+++ b/v4l/versions.txt
@@ -182,6 +182,8 @@ VIDEO_S5K6AA
 MEDIA_TUNER_E4000
 # needs regmap_init with 4 arguments
 DVB_USB_AF9015
+# needs of_find_i2c_adapter_by_node
+DVB_C8SECTPFE
 
 [3.4.0]
 # needs devm_regulator_bulk_get
-- 
2.7.4

[PATCH 5/7] Header location fix for 3.5.0 to 3.11.x

[Brad Love]

Header does not exist before 3.5.0 and is merged into linux/i2c.h
in 3.12.0.

Signed-off-by: Brad Love 
---
 v4l/compat.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/v4l/compat.h b/v4l/compat.h
index d52c602..87ce401 100644
--- a/v4l/compat.h
+++ b/v4l/compat.h
@@ -2414,4 +2414,11 @@ static inline void *memdup_user_nul(const void __user 
*src, size_t len)
 #include 
 #endif
 
+/* header location for of_find_i2c_[device,adapter]_by_node */
+#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 12, 0)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 5, 0)
+#include 
+#endif
+#endif
+
 #endif /*  _COMPAT_H */
-- 
2.7.4

[PATCH 4/7] Disable SOC_CAMERA for kernels older than 3.5

[Brad Love]

Needs of_find_i2c_adapter_by_node

Migrated from 3.5 to 3.12.

Signed-off-by: Brad Love 
---
 v4l/versions.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/v4l/versions.txt b/v4l/versions.txt
index f5e9a42..fcd6949 100644
--- a/v4l/versions.txt
+++ b/v4l/versions.txt
@@ -184,6 +184,7 @@ MEDIA_TUNER_E4000
 DVB_USB_AF9015
 # needs of_find_i2c_adapter_by_node
 DVB_C8SECTPFE
+SOC_CAMERA
 
 [3.4.0]
 # needs devm_regulator_bulk_get
@@ -228,7 +229,6 @@ VIDEO_GS1662
 
 [3.2.0]
 # due to the rename at include/linux from "pm_qos_params.h" to "pm_qos.h"
-SOC_CAMERA
 SOC_CAMERA_MT9V022
 SOC_CAMERA_MT9M001
 SOC_CAMERA_MT9T031
-- 
2.7.4

[PATCH 6/7] Fix frame vector wildcard file check

[Brad Love]

This check was consistently failing on all systems tested.
The path to object directory is used here to explicitly override
CWD. The thought is, if frame_vector.c exists in the build
directory then the build system has determined it is required,
and the source therefore should be compiled. The module will
not be built unless the build system has enabled it's config
option anyways, so this change should be safe in all circumstances.

Signed-off-by: Brad Love 
---
 v4l/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/v4l/Makefile b/v4l/Makefile
index b512600..270a624 100644
--- a/v4l/Makefile
+++ b/v4l/Makefile
@@ -88,7 +88,7 @@ ifneq ($(filter $(no-makefile-media-targets), 
$(MAKECMDGOALS)),)
 endif
 
 makefile-mm := 1
-ifeq ($(wildcard ../linux/mm/frame_vector.c),)
+ifeq ("$(wildcard $(obj)/frame_vector.c)","")
makefile-mm := 0
 endif
 
-- 
2.7.4

[PATCH 7/7] Add config-compat.h override config-mycompat.h

[Brad Love]

config-mycompat.h is for overriding macros which are incorrectly
enabled on certain kernels by the build system. The file should be
left empty, unless build errors are encountered for a kernel. The
file is removed by distclean, therefore should be externally
sourced, before the build process starts, when required.

In standard operation the file is empty, but if a particular kernel has
incorrectly enabled options defined this allows them to be undefined.

Signed-off-by: Brad Love 
---
 v4l/Makefile | 3 ++-
 v4l/compat.h | 7 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/v4l/Makefile b/v4l/Makefile
index 270a624..ee18d11 100644
--- a/v4l/Makefile
+++ b/v4l/Makefile
@@ -273,6 +273,7 @@ links::
@find ../linux/drivers/misc -name '*.[ch]' -type f -print0 | xargs -0n 
255 ln -sf --target-directory=.
 
 config-compat.h:: $(obj)/.version .myconfig scripts/make_config_compat.pl
+   -touch $(obj)/config-mycompat.h
perl scripts/make_config_compat.pl $(SRCDIR) $(obj)/.myconfig 
$(obj)/config-compat.h
 
 kernel-links makelinks::
@@ -298,7 +299,7 @@ clean::
 distclean:: clean
-rm -f .version .*.o.flags .*.o.d *.mod.gcno Makefile.media \
Kconfig Kconfig.kern .config .config.cmd .myconfig \
-   .kconfig.dep
+   .kconfig.dep config-mycompat.h
-rm -rf .tmp_versions .tmp*.ver .tmp*.o .*.gcno .cache.mk
-rm -f scripts/lxdialog scripts/kconfig
@find .. -name '*.orig' -exec rm '{}' \;
diff --git a/v4l/compat.h b/v4l/compat.h
index 87ce401..db48fdf 100644
--- a/v4l/compat.h
+++ b/v4l/compat.h
@@ -8,6 +8,13 @@
 #include 
 
 #include "config-compat.h"
+/* config-mycompat.h is for overriding #defines which
+ * are incorrectly enabled on certain kernels. The file
+ * should be left empty, unless build errors are encountered
+ * for a kernel. The file is removed by distclean, therefore
+ * should be externally sourced, before compilation, when required.
+ */
+#include "config-mycompat.h"
 
 #ifndef SZ_512
 #define SZ_512 0x0200
-- 
2.7.4

RE: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device driver

[Zhi, Yong]

Hi, Tomasz,

Thanks for the review again.

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Thursday, April 26, 2018 12:15 AM
> To: Zhi, Yong 
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan
> ; Toivonen, Tuukka
> ; Hu, Jerry W ; Zheng,
> Jian Xu 
> Subject: Re: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device
> driver
> 
> Hi Yong,
> 
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi  wrote:
> [snip]
> > +static int imgu_video_nodes_init(struct imgu_device *imgu) {
> > +   struct v4l2_pix_format_mplane *fmts[IPU3_CSS_QUEUES] = { NULL };
> > +   struct v4l2_rect *rects[IPU3_CSS_RECTS] = { NULL };
> > +   unsigned int i;
> > +   int r;
> > +
> > +   imgu->buf_struct_size = sizeof(struct imgu_buffer);
> > +
> > +   for (i = 0; i < IMGU_NODE_NUM; i++) {
> > +   imgu->nodes[i].name = imgu_node_map[i].name;
> > +   imgu->nodes[i].output = i < IMGU_QUEUE_FIRST_INPUT;
> > +   imgu->nodes[i].immutable = false;
> > +   imgu->nodes[i].enabled = false;
> > +
> > +   if (i != IMGU_NODE_PARAMS && i != IMGU_NODE_STAT_3A)
> > +   fmts[imgu_node_map[i].css_queue] =
> > +   >nodes[i].vdev_fmt.fmt.pix_mp;
> > +   atomic_set(>nodes[i].sequence, 0);
> > +   }
> > +
> > +   /* Master queue is always enabled */
> > +   imgu->nodes[IMGU_QUEUE_MASTER].immutable = true;
> > +   imgu->nodes[IMGU_QUEUE_MASTER].enabled = true;
> > +
> > +   r = ipu3_v4l2_register(imgu);
> > +   if (r)
> > +   return r;
> > +
> > +   /* Set initial formats and initialize formats of video nodes */
> > +   rects[IPU3_CSS_RECT_EFFECTIVE] = >rect.eff;
> > +   rects[IPU3_CSS_RECT_BDS] = >rect.bds;
> > +   ipu3_css_fmt_set(>css, fmts, rects);
> > +
> > +   /* Pre-allocate dummy buffers */
> > +   r = imgu_dummybufs_preallocate(imgu);
> > +   if (r) {
> > +   dev_err(>pci_dev->dev,
> > +   "failed to pre-allocate dummy buffers (%d)", r);
> > +   imgu_dummybufs_cleanup(imgu);
> 
> No need to call ipu3_v4l2_unregister() here?
> 
> (That's why I keep suggesting use of single return error path with labels
> named after the first cleanup step that needs to be done, as it makes it
> easier to spot such mistakes.)
> 

Good catch, suggestion taken :)

Maybe I should move the imgu_dummybufs_preallocate() out from 
imgu_video_nodes_init().

> > +   return r;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static void imgu_video_nodes_exit(struct imgu_device *imgu) {
> > +   imgu_dummybufs_cleanup(imgu);
> > +   ipu3_v4l2_unregister(imgu);
> > +}
> 
> Best regards,
> Tomasz

RE: [PATCH v6 10/12] intel-ipu3: Add css pipeline programming

[Zhi, Yong]

Hi, Tomasz,

Thanks for the code review.

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Thursday, April 26, 2018 12:12 AM
> To: Zhi, Yong 
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan
> ; Toivonen, Tuukka
> ; Hu, Jerry W ; Zheng,
> Jian Xu 
> Subject: Re: [PATCH v6 10/12] intel-ipu3: Add css pipeline programming
> 
> Hi Yong,
> 
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi  wrote:
> [snip]
> > +int ipu3_css_init(struct device *dev, struct ipu3_css *css,
> > + void __iomem *base, int length) {
> > +   int r, p, q, i;
> > +
> > +   /* Initialize main data structure */
> > +   css->dev = dev;
> > +   css->base = base;
> > +   css->iomem_length = length;
> > +   css->current_binary = IPU3_CSS_DEFAULT_BINARY;
> > +   css->pipe_id = IPU3_CSS_PIPE_ID_NUM;
> > +   css->vf_output_en = IPU3_NODE_VF_DISABLED;
> > +   spin_lock_init(>qlock);
> > +
> > +   for (q = 0; q < IPU3_CSS_QUEUES; q++) {
> > +   r = ipu3_css_queue_init(>queue[q], NULL, 0);
> > +   if (r)
> > +   return r;
> > +   }
> > +
> > +   r = ipu3_css_fw_init(css);
> > +   if (r)
> > +   return r;
> > +
> > +   /* Allocate and map common structures with imgu hardware */
> > +
> > +   for (p = 0; p < IPU3_CSS_PIPE_ID_NUM; p++)
> > +   for (i = 0; i < IMGU_ABI_MAX_STAGES; i++) {
> > +   if (!ipu3_dmamap_alloc(dev,
> > +
>   >xmem_sp_stage_ptrs[p][i],
> > +  sizeof(struct
> imgu_abi_sp_stage)))
> 
> checkpatch reports line over 80 characters here.

Ack, I opted for alignment over line limit here , will fix in v7.
> 
> > +   goto error_no_memory;
> > +   if (!ipu3_dmamap_alloc(dev,
> > +
>   >xmem_isp_stage_ptrs[p][i],
> > +  sizeof(struct
> imgu_abi_isp_stage)))
> 
> Ditto.

Sure, thanks!!
> 
> > +   goto error_no_memory;
> > +   }
> 
> Best regards,
> Tomasz

Compressed formats - framed or unframed?

[Dave Stevenson]

Hi All.

I'm trying to get a V4L2 M2M driver sorted for the Raspberry Pi to
allow access to the video codecs. Much of it is working fine.

One thing that isn't clear relates to video decode. Do the compressed
formats (eg V4L2_PIX_FMT_H264) have to be framed into one frame per
V4L2 buffer, or is providing unframed chunks of an elementary stream
permitted. The docs only say "H264 video elementary stream with start
codes.". Admittedly timestamps are nearly meaningless if you feed in
unframed data, but could potentially be interpolated.

What does other hardware support?

I could handle it either way, but there are some performance tweaks I
can do if I know the data must be framed.

Thanks in advance.
  Dave

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[Akinobu Mita]

2018-04-26 7:40 GMT+09:00 Laurent Pinchart :
> Hi Mita-san,
>
> On Wednesday, 25 April 2018 19:19:11 EEST Akinobu Mita wrote:
>> 2018-04-24 0:54 GMT+09:00 Akinobu Mita :
>> > 2018-04-23 18:17 GMT+09:00 Laurent Pinchart:
>> >> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
>> >>> This adds a device tree binding documentation for OV7720/OV7725 sensor.
>> >>>
>> >>> Cc: Jacopo Mondi 
>> >>> Cc: Laurent Pinchart 
>> >>> Cc: Hans Verkuil 
>> >>> Cc: Sakari Ailus 
>> >>> Cc: Mauro Carvalho Chehab 
>> >>> Cc: Rob Herring 
>> >>> Reviewed-by: Rob Herring 
>> >>> Reviewed-by: Jacopo Mondi 
>> >>> Signed-off-by: Akinobu Mita 
>> >>> ---
>> >>> * v3
>> >>> - Add Reviewed-by: lines
>> >>>
>> >>>  .../devicetree/bindings/media/i2c/ov772x.txt   | 42 +++
>> >>>  MAINTAINERS|  1 +
>> >>>  2 files changed, 43 insertions(+)
>> >>>  create mode 100644
>> >>>  Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> >>>
>> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> >>> b/Documentation/devicetree/bindings/media/i2c/ov772x.txt new file mode
>> >>> 100644
>> >>> index 000..b045503
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> >>> @@ -0,0 +1,42 @@
>> >>> +* Omnivision OV7720/OV7725 CMOS sensor
>> >>> +
>> >>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions
>> >>> output,
>> >>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also
>> >>> can +support the YUV422, RGB565/555/444, GRB422 or raw RGB output
>> >>> formats. +
>> >>> +Required Properties:
>> >>> +- compatible: shall be one of
>> >>> + "ovti,ov7720"
>> >>> + "ovti,ov7725"
>> >>> +- clocks: reference to the xclk input clock.
>> >>> +- clock-names: shall be "xclk".
>> >>
>> >> As there's a single clock we could omit clock-names, couldn't we ?
>> >
>> > Sounds good.
>> >
>> > I'll prepare another patch that replaces the clock consumer ID argument
>> > of clk_get() from "xclk" to NULL, and remove the above line in this
>> > bindings.
>>
>> I thought it's easy to do.  However, there is a non-DT user
>> (arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.
>>
>> This can be resolved by retrying clk_get() with NULL if no entry
>> with "xclk".  But should we do so or leave as is?
>
> How about patching the board code to register the clock alias with
>
> clk_add_alias(NULL, "0-0021", "video_clk", NULL);

Sounds good.

But I'm a bit worried about whether clk_add_alias() can be called with
alias == NULL.  I couldn't find such use case.

Probably Jacopo can verify whether it works or not with v4 patchset.

[RFC PATCH] media: i2c: add SCCB helpers

[Akinobu Mita]

(This patch is in prototype stage)

This adds SCCB helper functions (sccb_read_byte and sccb_write_byte) that
are intended to be used by some of Omnivision sensor drivers.

The ov772x driver is going to use these functions in order to make it work
with most i2c controllers.

As the ov772x device doesn't support repeated starts, this driver currently
requires I2C_FUNC_PROTOCOL_MANGLING that is not supported by many i2c
controller drivers.

With the sccb_read_byte() that issues two separated requests in order to
avoid repeated start, the driver doesn't require I2C_FUNC_PROTOCOL_MANGLING.

Cc: Wolfram Sang 
Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
 drivers/media/i2c/Kconfig  |  4 
 drivers/media/i2c/Makefile |  1 +
 drivers/media/i2c/sccb.c   | 35 +++
 drivers/media/i2c/sccb.h   | 14 ++
 4 files changed, 54 insertions(+)
 create mode 100644 drivers/media/i2c/sccb.c
 create mode 100644 drivers/media/i2c/sccb.h

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 541f0d28..19e5601 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -569,6 +569,9 @@ config VIDEO_THS8200
 
 comment "Camera sensor devices"
 
+config SCCB
+   bool
+
 config VIDEO_APTINA_PLL
tristate
 
@@ -692,6 +695,7 @@ config VIDEO_OV772X
tristate "OmniVision OV772x sensor support"
depends on I2C && VIDEO_V4L2
depends on MEDIA_CAMERA_SUPPORT
+   select SCCB
---help---
  This is a Video4Linux2 sensor-level driver for the OmniVision
  OV772x camera.
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index ea34aee..82fbd78 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
 obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
 obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
 obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
+obj-$(CONFIG_SCCB) += sccb.o
 obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
 obj-$(CONFIG_VIDEO_OV2685) += ov2685.o
 obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
diff --git a/drivers/media/i2c/sccb.c b/drivers/media/i2c/sccb.c
new file mode 100644
index 000..80a3fb7
--- /dev/null
+++ b/drivers/media/i2c/sccb.c
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include 
+
+int sccb_read_byte(struct i2c_client *client, u8 addr)
+{
+   int ret;
+   u8 val;
+
+   /* Issue two separated requests in order to avoid repeated start */
+
+   ret = i2c_master_send(client, , 1);
+   if (ret < 0)
+   return ret;
+
+   ret = i2c_master_recv(client, , 1);
+   if (ret < 0)
+   return ret;
+
+   return val;
+}
+EXPORT_SYMBOL_GPL(sccb_read_byte);
+
+int sccb_write_byte(struct i2c_client *client, u8 addr, u8 data)
+{
+   int ret;
+   unsigned char msgbuf[] = { addr, data };
+
+   ret = i2c_master_send(client, msgbuf, 2);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(sccb_write_byte);
diff --git a/drivers/media/i2c/sccb.h b/drivers/media/i2c/sccb.h
new file mode 100644
index 000..68da0e9
--- /dev/null
+++ b/drivers/media/i2c/sccb.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * SCCB helper functions
+ */
+
+#ifndef __SCCB_H__
+#define __SCCB_H__
+
+#include 
+
+int sccb_read_byte(struct i2c_client *client, u8 addr);
+int sccb_write_byte(struct i2c_client *client, u8 addr, u8 data);
+
+#endif /* __SCCB_H__ */
-- 
2.7.4

Re: [PATCH] rcar-vin: fix null pointer dereference in rvin_group_get()

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2018-04-25 09:25:56 +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 25, 2018 at 1:45 AM, Niklas Söderlund
>  wrote:
> > Store the group pointer before disassociating the VIN from the group.
> 
> s/get/put/ in one-line summary?

Yes, silly copy paste error, must have copied function name from the @@ 
context line and not from the diff itself. Thanks for noticing.

Will send a v2 after I have checked with Simon that the is happy with 
the change itself.

> 
> > Fixes: 3bb4c3bc85bf77a7 ("media: rcar-vin: add group allocator functions")
> > Reported-by: Colin Ian King 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 12 +++-
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> > b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 7bc2774a11232362..d3072e166a1ca24f 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -338,19 +338,21 @@ static int rvin_group_get(struct rvin_dev *vin)
> >
> >  static void rvin_group_put(struct rvin_dev *vin)
> >  {
> > -   mutex_lock(>group->lock);
> > +   struct rvin_group *group = vin->group;
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

-- 
Regards,
Niklas Söderlund

Re: [PATCH] rcar-vin: fix null pointer dereference in rvin_group_get()

[Niklas Söderlund]

Hi Simon,

Thanks for your feedback.

On 2018-04-25 09:18:51 +0200, Simon Horman wrote:
> On Wed, Apr 25, 2018 at 01:45:06AM +0200, Niklas Söderlund wrote:
> > Store the group pointer before disassociating the VIN from the group.
> > 
> > Fixes: 3bb4c3bc85bf77a7 ("media: rcar-vin: add group allocator functions")
> > Reported-by: Colin Ian King 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 12 +++-
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> > b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 7bc2774a11232362..d3072e166a1ca24f 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -338,19 +338,21 @@ static int rvin_group_get(struct rvin_dev *vin)
> >  
> >  static void rvin_group_put(struct rvin_dev *vin)
> >  {
> > -   mutex_lock(>group->lock);
> > +   struct rvin_group *group = vin->group;
> > +
> > +   mutex_lock(>lock);
> 
> Hi Niklas, its not clear to me why moving the lock is safe.
> Could you explain the locking scheme a little?

The lock here protects the members of the group struct and not any of 
the members of the vin struct. The intent of the rvin_group_put() 
function is:

1. Disassociate the vin struct from the group struct. This is done by 
   removing the pointer to the vin from the group->vin array and 
   removing the pointer from vin->group to the group struct. Here the 
   lock is needed to protect access to the group->vin array.

2. Decrease the refcount of the struct group and if we are the last one 
   out release the group.

The problem with the original code is that I first disassociate group 
from the vin 'vin->group = NULL' but still use the pointer stored in the 
vin struct when I try to disassociate the vin from the group 
'vin->group->vin[vin->id]'.

AFIK can tell the locking here is fine, the problem was that I pulled 
the rug from under my own feet in how I access the lock in order to not 
having to declare a variable to store the pointer in ;-)

Do this explanation help put you at ease?

> 
> >  
> > vin->group = NULL;
> > vin->v4l2_dev.mdev = NULL;
> >  
> > -   if (WARN_ON(vin->group->vin[vin->id] != vin))
> > +   if (WARN_ON(group->vin[vin->id] != vin))
> > goto out;
> >  
> > -   vin->group->vin[vin->id] = NULL;
> > +   group->vin[vin->id] = NULL;
> >  out:
> > -   mutex_unlock(>group->lock);
> > +   mutex_unlock(>lock);
> >  
> > -   kref_put(>group->refcount, rvin_group_release);
> > +   kref_put(>refcount, rvin_group_release);
> >  }
> >  
> >  /* 
> > -
> > -- 
> > 2.17.0
> > 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 13/13] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Hi Sakari,

thanks for your feedback, see below my replies.

On 26/04/2018 10:13, Sakari Ailus wrote:
> Hi Luca,
> 
> On Tue, Apr 24, 2018 at 04:30:38PM +0200, Luca Ceresoli wrote:
>> Hi Sakari,
>>
>> On 24/04/2018 11:59, Sakari Ailus wrote:
>>> Hi Luca,
>>>
>>> Thank you for the patchset.
>>>
>>> Some comments below... what I propose is that I apply the rest of the
>>> patches and then the comments to this one could be addressed separately.
>>> Would that work for you?
>>
>> Yes, but I suggest you only apply patches 1-6. I noticed a warning in
>> patch 9, and patches 7-8 are not very useful without it. Will fix it in v3.
> 
> Ack. I'll apply 1--6 then.

Thanks.

>> I'll wait for the outcome of the discussion below before sending v3.
>> Tell me if you prefer me to do it earlier.
>>
>>> On Tue, Apr 24, 2018 at 10:24:18AM +0200, Luca Ceresoli wrote:
 Currently this driver does not support cropping. The supported modes
 are the following, all capturing the entire area:

  - 3840x2160, 1:1 binning (native sensor resolution)
  - 1920x1080, 2:1 binning
  - 1280x720,  3:1 binning

 The set_fmt callback chooses among these 3 configurations the one that
 matches the requested format.

 Adding support to VIDIOC_SUBDEV_G_SELECTION and
 VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
 which now chooses the most appropriate binning based on the ratio
 between the previously-set cropping size and the format size being
 requested.

 Note that the names in enum imx274_mode change from being
 resolution-based to being binning-based. Without cropping, the two
 naming are equivalent. With cropping, the resolution could be
 different, e.g. using 2:1 binning mode to crop 1200x960 and output a
 600x480 format. Using binning in the names avoids anny
 misunderstanding.

 Signed-off-by: Luca Ceresoli 

 ---
 Changed v1 -> v2:
  - add "media: " prefix to commit message
 ---
  drivers/media/i2c/imx274.c | 266 
 -
  1 file changed, 192 insertions(+), 74 deletions(-)

 diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
 index b6c54712f2aa..ceb5b3e498c6 100644
 --- a/drivers/media/i2c/imx274.c
 +++ b/drivers/media/i2c/imx274.c
 @@ -19,6 +19,7 @@
   * along with this program.  If not, see .
   */
  
 +#include 
  #include 
  #include 
  #include 
 @@ -74,7 +75,7 @@
   */
  #define IMX274_MIN_EXPOSURE_TIME  (4 * 260 / 72)
  
 -#define IMX274_DEFAULT_MODE   IMX274_MODE_3840X2160
 +#define IMX274_DEFAULT_MODE   IMX274_MODE_BINNING_OFF
  #define IMX274_MAX_WIDTH  (3840)
  #define IMX274_MAX_HEIGHT (2160)
  #define IMX274_MAX_FRAME_RATE (120)
 @@ -111,6 +112,20 @@
  #define IMX274_SHR_REG_LSB0x300C /* SHR */
  #define IMX274_SVR_REG_MSB0x300F /* SVR */
  #define IMX274_SVR_REG_LSB0x300E /* SVR */
 +#define IMX274_HTRIM_EN_REG   0x3037
 +#define IMX274_HTRIM_START_REG_LSB0x3038
 +#define IMX274_HTRIM_START_REG_MSB0x3039
 +#define IMX274_HTRIM_END_REG_LSB  0x303A
 +#define IMX274_HTRIM_END_REG_MSB  0x303B
 +#define IMX274_VWIDCUTEN_REG  0x30DD
 +#define IMX274_VWIDCUT_REG_LSB0x30DE
 +#define IMX274_VWIDCUT_REG_MSB0x30DF
 +#define IMX274_VWINPOS_REG_LSB0x30E0
 +#define IMX274_VWINPOS_REG_MSB0x30E1
 +#define IMX274_WRITE_VSIZE_REG_LSB0x3130
 +#define IMX274_WRITE_VSIZE_REG_MSB0x3131
 +#define IMX274_Y_OUT_SIZE_REG_LSB 0x3132
 +#define IMX274_Y_OUT_SIZE_REG_MSB 0x3133
  #define IMX274_VMAX_REG_1 0x30FA /* VMAX, MSB */
  #define IMX274_VMAX_REG_2 0x30F9 /* VMAX */
  #define IMX274_VMAX_REG_3 0x30F8 /* VMAX, LSB */
 @@ -141,9 +156,9 @@ static const struct regmap_config imx274_regmap_config 
 = {
  };
  
  enum imx274_mode {
 -  IMX274_MODE_3840X2160,
 -  IMX274_MODE_1920X1080,
 -  IMX274_MODE_1280X720,
 +  IMX274_MODE_BINNING_OFF,
 +  IMX274_MODE_BINNING_2_1,
 +  IMX274_MODE_BINNING_3_1,
  };
  
  /*
 @@ -215,31 +230,14 @@ static const struct reg_8 
 imx274_mode1_3840x2160_raw10[] = {
{0x3004, 0x01},
{0x3005, 0x01},
{0x3006, 0x00},
 -  {0x3007, 0x02},
 +  {0x3007, 0xa2},
  
{0x3018, 0xA2}, /* output XVS, HVS */
  
{0x306B, 0x05},
{0x30E2, 0x01},
 -  {0x30F6, 0x07}, 

Re: [PATCH/RFC 1/4] drm: Add colorkey properties

[Ville Syrjälä]

On Sun, Dec 17, 2017 at 02:17:21AM +0200, Laurent Pinchart wrote:
> Color keying is the action of replacing pixels matching a given color
> (or range of colors) with transparent pixels in an overlay when
> performing blitting. Depending on the hardware capabilities, the
> matching pixel can either become fully transparent, or gain a
> programmable alpha value.
> 
> Color keying is found in a large number of devices whose capabilities
> often differ, but they still have enough common features in range to
> standardize color key properties. This commit adds four properties
> related to color keying named colorkey.min, colorkey.max, colorkey.alpha
> and colorkey.mode. Additional properties can be defined by drivers to
> expose device-specific features.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/gpu/drm/drm_atomic.c |  16 +++
>  drivers/gpu/drm/drm_blend.c  | 108 
> +++
>  include/drm/drm_blend.h  |   4 ++
>  include/drm/drm_plane.h  |  28 ++-
>  4 files changed, 155 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..4f57ec25e04d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -756,6 +756,14 @@ static int drm_atomic_plane_set_property(struct 
> drm_plane *plane,
>   state->rotation = val;
>   } else if (property == plane->zpos_property) {
>   state->zpos = val;
> + } else if (property == plane->colorkey.mode_property) {
> + state->colorkey.mode = val;
> + } else if (property == plane->colorkey.min_property) {
> + state->colorkey.min = val;
> + } else if (property == plane->colorkey.max_property) {
> + state->colorkey.max = val;
> + } else if (property == plane->colorkey.value_property) {
> + state->colorkey.value = val;
>   } else if (plane->funcs->atomic_set_property) {
>   return plane->funcs->atomic_set_property(plane, state,
>   property, val);
> @@ -815,6 +823,14 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
>   *val = state->zpos;
> + } else if (property == plane->colorkey.mode_property) {
> + *val = state->colorkey.mode;
> + } else if (property == plane->colorkey.min_property) {
> + *val = state->colorkey.min;
> + } else if (property == plane->colorkey.max_property) {
> + *val = state->colorkey.max;
> + } else if (property == plane->colorkey.value_property) {
> + *val = state->colorkey.value;
>   } else if (plane->funcs->atomic_get_property) {
>   return plane->funcs->atomic_get_property(plane, state, 
> property, val);
>   } else {
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 2e5e089dd912..79da7d8a22e2 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -98,6 +98,10 @@
>   *   planes. Without this property the primary plane is always below the 
> cursor
>   *   plane, and ordering between all other planes is undefined.
>   *
> + * - Color keying is set up with drm_plane_create_colorkey_properties(). It 
> adds
> + *   support for replacing a range of colors with a transparent color in the
> + *   plane.
> + *
>   * Note that all the property extensions described here apply either to the
>   * plane or the CRTC (e.g. for the background color, which currently is not
>   * exposed and assumed to be black).
> @@ -405,3 +409,107 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_normalize_zpos);
> +
> +/**
> + * drm_plane_create_colorkey_properties - create colorkey properties
> + * @plane: drm plane
> + * @modes: array of supported color keying modes
> + * @num_modes: number of modes in the modes array
> + * @replace: if true create the colorkey.replacement property
> + *
> + * This function creates the generic color keying properties and attach them 
> to
> + * the plane to enable color keying control for blending operations.
> + *
> + * Color keying is controlled through four properties:
> + *
> + * colorkey.mode:
> + *   The mode is an enumerated property that controls how color keying
> + *   operates. Modes are driver-specific, except for a "disabled" mode that
> + *   disables color keying and is guaranteed to exist if color keying is
> + *   supported.

I don't see why we need driver specific modes. It should be possible to
come up with some standard modes. I suppose a simple src vs. dst doesn't
suffice, but if we combine that with a bit more information it should be
good enough? Eg. i915 would expose something like src-min-max and
dst-value-mask.

> + *
> + * colorkey.min, colorkey.max:
> + *   Those two properties 

Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor

[Todor Tomov]

On 26.04.2018 15:04, Sakari Ailus wrote:
> On Thu, Apr 26, 2018 at 10:16:56AM +0300, Sakari Ailus wrote:
>> On Thu, Apr 26, 2018 at 10:04:25AM +0300, Todor Tomov wrote:
>>> Hi Sakari,
>>>
>>> On 26.04.2018 09:50, Sakari Ailus wrote:
 Hi Todor,

 On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote:
 ...
> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val)
> +{
> + u8 regbuf[3];
> + int ret;
> +
> + regbuf[0] = reg >> 8;
> + regbuf[1] = reg & 0xff;
> + regbuf[2] = val;
> +
> + ret = i2c_master_send(ov7251->i2c_client, regbuf, 3);
> + if (ret < 0) {
> + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n",
> + __func__, ret, reg, val);
> + return ret;
> + }
> +
> + return 0;

 How about:

return ov7251_write_seq_regs(ov7251, reg, , 1);

 And put the function below ov2751_write_seq_regs().
>>>
>>> I'm not sure... It will calculate message length each time and then check
>>> that it is not greater than 5, which it is. Seems redundant.
>>>

> +}
> +
> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val,
> +  u8 num)
> +{
> + const u8 maxregbuf = 5;
> + u8 regbuf[maxregbuf];
> 
> Apparently this leads to bad positive sparse warning. I'd fix it by:
> 
> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> index 3e2c0c03dfa9..d3ebb7529fca 100644
> --- a/drivers/media/i2c/ov7251.c
> +++ b/drivers/media/i2c/ov7251.c
> @@ -643,12 +643,11 @@ static int ov7251_write_reg(struct ov7251 *ov7251, u16 
> reg, u8 val)
>  static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val,
>  u8 num)
>  {
> -   const u8 maxregbuf = 5;
> -   u8 regbuf[maxregbuf];
> +   u8 regbuf[5];
> u8 nregbuf = sizeof(reg) + num * sizeof(*val);
> int ret = 0;
>  
> -   if (nregbuf > maxregbuf)
> +   if (nregbuf > sizeof(regbuf))
> return -EINVAL;
>  
> regbuf[0] = reg >> 8;
> 
> Let me know if you're happy with that; I can merge it to the original
> patch.

Yes, thanks.

> 
> + u8 nregbuf = sizeof(reg) + num * sizeof(*val);
> + int ret = 0;
> +
> + if (nregbuf > maxregbuf)
> + return -EINVAL;
> +
> + regbuf[0] = reg >> 8;
> + regbuf[1] = reg & 0xff;
> +
> + memcpy(regbuf + 2, val, num);
> +
> + ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf);
> + if (ret < 0) {
> + dev_err(ov7251->dev, "%s: write seq regs error %d: first 
> reg=%x\n",

 This line is over 80... 
>>>
>>> Yes indeed. Somehow checkpatch does not report this line, I don't know why.
>>>

 If you're happy with these, I can make the changes, too; they're trivial.
>>>
>>> Only the second one? Thanks :)
>>
>> Works for me. I'd still think the overhead of managing the buffer is
>> irrelevant where to having an extra function to do essentially the same
>> thing is a source of maintenance and review work. Note that we're even now
>> spending time to discuss it. ;-)
>>
>> -- 
>> Kind regards,
>>
>> Sakari Ailus
>> e-mail: sakari.ai...@iki.fi
> 

-- 
Best regards,
Todor Tomov

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Wolfram Sang]

> > Ah, didn't notice that so far. Can't find it in drivers/i2c/busses.
> > Where are those?
> 
> IIRC the OMAP I2C adapter supports SCCB natively. I'm not sure the driver 
> implements that though.

It doesn't currently. And seeing how long it sits in HW without a driver
for it, I don't have much expectations.

> > > we need to forward native SCCB calls all the way down the stack in that
> > > case.
> > 
> > And how is it done currently?
> 
> Currently we go down to .master_xfer(), and adapters can then decide to use 
> the hardware SCCB support. Again, it might not be implemented :-)

To sum it up: hardware-driven SCCB support is a very rare exception not
implemented anywhere in all those years. From a pragmatic point of view,
I'd say: we should be open for it, but we don't need to design around
it.

> I'm fine with SCCB helpers. Please note, however, that SCCB differs from 
> SMBus 
> in two ways: NACKs shall be ignored by the master (even though most SCCB 
> devices generate an ack, so we could likely ignore this), and write-read 
> sequences shouldn't use a repeated start. Apart from that register reads and 

Especially the latter is a huge difference to SMBus, and so I think it
will be much safer to not abuse SMBus calls for SCCB.

> register writes are identical to SMBus, which prompted the reuse (or abuse) 
> of 
> the SMBus API. If we end up implementing SCCB helpers, they will likely look 
> very, very similar to the SMBus implementation, including the SMBus emulated 
> transfer helper.

I don't think so. SCCB has much less transaction types than SMBus. Also, the
fallback as sketched in this patch (one master write, then a master
read) would be impossible on SMBus.

I have an idea in my mind. Maybe it is better to implement an RFC first,
so we can talk over code (even if only in prototype stage). I already
found this in ov7670.c, so I am proven wrong on this one already:

 472  * Note that there are two versions of these.  On the XO 1, the
 473  * i2c controller only does SMBUS, so that's what we use.  The
 474  * ov7670 is not really an SMBUS device, though, so the communication
 475  * is not always entirely reliable.

Sigh...



signature.asc
Description: PGP signature

Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor

[Sakari Ailus]

On Thu, Apr 26, 2018 at 10:16:56AM +0300, Sakari Ailus wrote:
> On Thu, Apr 26, 2018 at 10:04:25AM +0300, Todor Tomov wrote:
> > Hi Sakari,
> > 
> > On 26.04.2018 09:50, Sakari Ailus wrote:
> > > Hi Todor,
> > > 
> > > On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote:
> > > ...
> > >> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val)
> > >> +{
> > >> +u8 regbuf[3];
> > >> +int ret;
> > >> +
> > >> +regbuf[0] = reg >> 8;
> > >> +regbuf[1] = reg & 0xff;
> > >> +regbuf[2] = val;
> > >> +
> > >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, 3);
> > >> +if (ret < 0) {
> > >> +dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, 
> > >> val=%x\n",
> > >> +__func__, ret, reg, val);
> > >> +return ret;
> > >> +}
> > >> +
> > >> +return 0;
> > > 
> > > How about:
> > > 
> > >   return ov7251_write_seq_regs(ov7251, reg, , 1);
> > > 
> > > And put the function below ov2751_write_seq_regs().
> > 
> > I'm not sure... It will calculate message length each time and then check
> > that it is not greater than 5, which it is. Seems redundant.
> > 
> > > 
> > >> +}
> > >> +
> > >> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 
> > >> *val,
> > >> + u8 num)
> > >> +{
> > >> +const u8 maxregbuf = 5;
> > >> +u8 regbuf[maxregbuf];

Apparently this leads to bad positive sparse warning. I'd fix it by:

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 3e2c0c03dfa9..d3ebb7529fca 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -643,12 +643,11 @@ static int ov7251_write_reg(struct ov7251 *ov7251, u16 
reg, u8 val)
 static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val,
 u8 num)
 {
-   const u8 maxregbuf = 5;
-   u8 regbuf[maxregbuf];
+   u8 regbuf[5];
u8 nregbuf = sizeof(reg) + num * sizeof(*val);
int ret = 0;
 
-   if (nregbuf > maxregbuf)
+   if (nregbuf > sizeof(regbuf))
return -EINVAL;
 
regbuf[0] = reg >> 8;

Let me know if you're happy with that; I can merge it to the original
patch.

> > >> +u8 nregbuf = sizeof(reg) + num * sizeof(*val);
> > >> +int ret = 0;
> > >> +
> > >> +if (nregbuf > maxregbuf)
> > >> +return -EINVAL;
> > >> +
> > >> +regbuf[0] = reg >> 8;
> > >> +regbuf[1] = reg & 0xff;
> > >> +
> > >> +memcpy(regbuf + 2, val, num);
> > >> +
> > >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf);
> > >> +if (ret < 0) {
> > >> +dev_err(ov7251->dev, "%s: write seq regs error %d: 
> > >> first reg=%x\n",
> > > 
> > > This line is over 80... 
> > 
> > Yes indeed. Somehow checkpatch does not report this line, I don't know why.
> > 
> > > 
> > > If you're happy with these, I can make the changes, too; they're trivial.
> > 
> > Only the second one? Thanks :)
> 
> Works for me. I'd still think the overhead of managing the buffer is
> irrelevant where to having an extra function to do essentially the same
> thing is a source of maintenance and review work. Note that we're even now
> spending time to discuss it. ;-)
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: noveau vs arm dma ops

[Russell King - ARM Linux]

(While there's a rain shower...)

On Thu, Apr 26, 2018 at 02:09:42AM -0700, Christoph Hellwig wrote:
> synopsis:
> 
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c:pdevinfo.dma_mask 
>   = DMA_BIT_MASK(32);
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c:  pdevinfo.dma_mask = 
> DMA_BIT_MASK(32);

This is for the AHB audio driver, and is a correct default on two counts:

1. It is historically usual to initialise DMA masks to 32-bit, and leave
   it to the device drivers to negotiate via the DMA mask functions if
   they wish to use higher orders of bits.

2. The AHB audio hardware in the HDMI block only supports 32-bit addresses.

What I've missed from the AHB audio driver is calling the DMA mask
functions... oops.  Patch below.

> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c:  pdevinfo.dma_mask = 
> DMA_BIT_MASK(32);

This is for the I2S audio driver, and I suspect is wrong - I doubt
that the I2S sub-device itself does any DMA what so ever.

8<===
From: Russell King 
Subject: drm: bridge: dw-hdmi: Negotiate dma mask with DMA API

DMA drivers are supposed to negotiate the DMA mask with the DMA API,
but this was missing from the AHB audio driver.  Add the necessary
call.

Signed-off-by: Russell King 
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index cf3f0caf9c63..16c45b6cd6af 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -539,6 +539,10 @@ static int snd_dw_hdmi_probe(struct platform_device *pdev)
unsigned revision;
int ret;
 
+   ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+   if (ret)
+   return ret;
+
writeb_relaxed(HDMI_IH_MUTE_AHBDMAAUD_STAT0_ALL,
   data->base + HDMI_IH_MUTE_AHBDMAAUD_STAT0);
revision = readb_relaxed(data->base + HDMI_REVISION_ID);


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH][next] media: ispstat: don't dereference user_cfg before a null check

[Laurent Pinchart]

Hi Sakari,

On Thursday, 26 April 2018 11:37:31 EEST Sakari Ailus wrote:
> On Tue, Apr 24, 2018 at 02:06:18PM +0100, Colin King wrote:
> > From: Colin Ian King 
> > 
> > The pointer user_cfg (a copy of new_conf) is dereference before
> > new_conf is null checked, hence we may have a null pointer dereference
> > on user_cfg when assigning buf_size from user_cfg->buf_size. Ensure
> > this does not occur by moving the assignment of buf_size after the
> > null check.
> > 
> > Detected by CoverityScan, CID#1468386 ("Dereference before null check")
> > 
> > Fixes: 68e342b3068c ("[media] omap3isp: Statistics")
> > Signed-off-by: Colin Ian King 
> 
> Thanks for the patch.
> 
> Gustavo sent effectively the same patch a moment earlier, and that patch
> got applied instead.

Isn't there a guarantee that new_buf won't be NULL ? The new_buf pointer comes 
from the parg variable in video_usercopy(), which should always point to a 
valid buffer given that the ioctl number specifies a non-zero size.

-- 
Regards,

Laurent Pinchart

Re: noveau vs arm dma ops

[Daniel Vetter]

On Thu, Apr 26, 2018 at 11:09 AM, Christoph Hellwig  wrote:
> On Wed, Apr 25, 2018 at 11:35:13PM +0200, Daniel Vetter wrote:
>> > get_required_mask() is supposed to tell you if you are safe.  However
>> > we are missing lots of implementations of it for iommus so you might get
>> > some false negatives, improvements welcome.  It's been on my list of
>> > things to fix in the DMA API, but it is nowhere near the top.
>>
>> I hasn't come up in a while in some fireworks, so I honestly don't
>> remember exactly what the issues have been. But
>>
>> commit d766ef53006c2c38a7fe2bef0904105a793383f2
>> Author: Chris Wilson 
>> Date:   Mon Dec 19 12:43:45 2016 +
>>
>> drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping
>>
>> and the various bits of code that a
>>
>> $ git grep SWIOTLB -- drivers/gpu
>>
>> turns up is what we're doing to hack around that stuff. And in general
>> (there's some exceptions) gpus should be able to address everything,
>> so I never fully understood where that's even coming from.
>
> I'm pretty sure I've seen some oddly low dma masks in GPU drivers.  E.g.
> duplicated in various AMD files:
>
> adev->need_dma32 = false;
> dma_bits = adev->need_dma32 ? 32 : 40;
> r = pci_set_dma_mask(adev->pdev, DMA_BIT_MASK(dma_bits));
> if (r) {
> adev->need_dma32 = true;
> dma_bits = 32;
> dev_warn(adev->dev, "amdgpu: No suitable DMA available.\n");
> }
>
> synopsis:
>
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c:pdevinfo.dma_mask 
>   = DMA_BIT_MASK(32);
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c:  pdevinfo.dma_mask = 
> DMA_BIT_MASK(32);
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c:  pdevinfo.dma_mask = 
> DMA_BIT_MASK(32);
>
> etnaviv gets it right:
>
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c:  u32 dma_mask = 
> (u32)dma_get_required_mask(gpu->dev);
>
>
> But yes, the swiotlb hackery really irks me.  I just have some more
> important and bigger fires to fight first, but I plan to get back to the
> root cause of that eventually.
>
>>
>> >> - dma api hides the cache flushing requirements from us. GPUs love
>> >>   non-snooped access, and worse give userspace control over that. We want
>> >>   a strict separation between mapping stuff and flushing stuff. With the
>> >>   IOMMU api we mostly have the former, but for the later arch maintainers
>> >>   regularly tells they won't allow that. So we have drm_clflush.c.
>> >
>> > The problem is that a cache flushing API entirely separate is hard. That
>> > being said if you look at my generic dma-noncoherent API series it tries
>> > to move that way.  So far it is in early stages and apparently rather
>> > buggy unfortunately.
>>
>> I'm assuming this stuff here?
>>
>> https://lkml.org/lkml/2018/4/20/146
>>
>> Anyway got lost in all that work a bit, looks really nice.
>
> That url doesn't seem to work currently.  But I am talking about the
> thread titled '[RFC] common non-cache coherent direct dma mapping ops'
>
>> Yeah the above is pretty much what we do on x86. dma-api believes
>> everything is coherent, so dma_map_sg does the mapping we want and
>> nothing else (minus swiotlb fun). Cache flushing, allocations, all
>> done by the driver.
>
> Which sounds like the right thing to do to me.
>
>> On arm that doesn't work. The iommu api seems like a good fit, except
>> the dma-api tends to get in the way a bit (drm/msm apparently has
>> similar problems like tegra), and if you need contiguous memory
>> dma_alloc_coherent is the only way to get at contiguous memory. There
>> was a huge discussion years ago about that, and direct cma access was
>> shot down because it would have exposed too much of the caching
>> attribute mangling required (most arm platforms need wc-pages to not
>> be in the kernel's linear map apparently).
>
> Simple cma_alloc() doesn't do anything about cache handling, it
> just is a very dumb allocator for large contiguous regions inside
> a big pool.
>
> I'm not the CMA maintainer, but in general I'd love to see an
> EXPORT_SYMBOL_GPL slapped onto cma_alloc/release and drivers use
> that were needed.  Using that plus dma_map*/dma_unmap* sounds like
> a much saner interface than dma_alloc_attrs + DMA_ATTR_NON_CONSISTENT
> or DMA_ATTR_NO_KERNEL_MAPPING.
>
> You don't happen to have a pointer to that previous discussion?

I'll try to dig them up, I tried to stay as far away from that
discussion as possible (since I have the luxury to not care for intel
gpus).

>> Anything that separate these 3 things more (allocation pools, mapping
>> through IOMMUs and flushing cpu caches) sounds like the right
>> direction to me. Even if that throws some portability across platforms
>> away - drivers who want to control things in this much detail aren't
>> really portable (without some serious work) anyway.
>
> As long as we stay away from the dma coherent 

Re: [Linaro-mm-sig] noveau vs arm dma ops

[Daniel Vetter]

On Thu, Apr 26, 2018 at 11:24 AM, Christoph Hellwig  wrote:
> On Thu, Apr 26, 2018 at 11:20:44AM +0200, Daniel Vetter wrote:
>> The above is already what we're implementing in i915, at least
>> conceptually (it all boils down to clflush instructions because those
>> both invalidate and flush).
>
> The clwb instruction that just writes back dirty cache lines might
> be very interesting for the x86 non-coherent dma case.  A lot of
> architectures use their equivalent to prepare to to device transfers.

Iirc didn't help for i915 use-cases much. Either data gets streamed
between cpu and gpu, and then keeping the clean cacheline around
doesn't buy you anything. In other cases we need to flush because the
gpu really wants to use non-snooped transactions (faster/lower
latency/less power required for display because you can shut down the
caches), and then there's also no benefit with keeping the cacheline
around (no one will ever need it again).

I think clwb is more for persistent memory and stuff like that, not so
much for gpus.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v7 1/2] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

Hi Laurent,

On Tue, 10 Apr 2018, Guennadi Liakhovetski wrote:

[snip]

> > > @@ -1488,6 +1591,25 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
> > >   return -EINVAL;
> > >   if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
> > >   return -EACCES;
> > > + if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) {
> > > + if (ctrl->handle)
> > > + /*
> > > +  * We have already sent this control to the camera
> > > +  * recently and are currently waiting for a completion
> > > +  * notification. The camera might already have completed
> > > +  * its processing and is ready to accept a new control
> > > +  * or it's still busy processing. If we send a new
> > > +  * instance of this control now, in the former case the
> > > +  * camera will process this one too and we'll get
> > > +  * completions for both, but we will only deliver an
> > > +  * event for one of them back to the user. In the latter
> > > +  * case the camera will reply with a STALL. It's easier
> > > +  * and more reliable to return an error now and let the
> > > +  * user retry.
> > > +  */
> > > + return -EBUSY;
> > > + ctrl->handle = handle;
> > 
> > This part worries me. If the control change event isn't received for any 
> > reason (such as a buggy device for instance, or uvc_ctrl_status_event() 
> > being 
> > called with the previous event not processed yet), the control will stay 
> > busy 
> > forever.
> > 
> > I see two approaches to fix this. One would be to forward all received 
> > control 
> > change events to all file handles unconditionally and remove the handle 
> > field 
> > from the uvc_control structure.
> 
> How is this a solution? A case of senging a repeated control to the camera 
> and causing a STALL would still be possible. If you prefer STALLs, you 
> could just remove this check here.
> 
> > Another one would be to add a timeout, storing 
> > the time at which the control has been set in the uvc_control structure, 
> > and 
> > checking whether the time difference exceeds a fixed timeout here. We could 
> > also combine the two, replacing the handle field with a timestamp field.
> 
> Don't think you can remove the handle field, there are a couple of things, 
> that need it, also you'll have to send events to all listeners, including 
> the thread, that has sent the control, which contradicts the API? Assuming 
> it hasn't set the V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag.
> 
> I can add a timeout, even though that doesn't seem to be very clean to me. 
> According to the UVC standard some controls can take a while to complete, 
> possibly seconds. How long would you propose that timeout to be?

How about just not checking when setting control and letting the camera 
decide? That's what the STALL mechanism is there for. The only advantage 
of using this is to provide a short-cut when we're "sure," that the 
hardware isn't ready to take a new request yet, but if implementing such a 
shortcut becomes too cumbersome, we can give control back to the hardware.

Thanks
Guennadi

Re: [Linaro-mm-sig] noveau vs arm dma ops

[Christoph Hellwig]

On Thu, Apr 26, 2018 at 11:20:44AM +0200, Daniel Vetter wrote:
> The above is already what we're implementing in i915, at least
> conceptually (it all boils down to clflush instructions because those
> both invalidate and flush).

The clwb instruction that just writes back dirty cache lines might
be very interesting for the x86 non-coherent dma case.  A lot of
architectures use their equivalent to prepare to to device transfers.

> One architectural guarantee we're exploiting is that prefetched (and
> hence non-dirty) cachelines will never get written back, but dropped
> instead.

And to make this work you'll need exactly this guarantee.

Re: [Linaro-mm-sig] noveau vs arm dma ops

[Daniel Vetter]

On Thu, Apr 26, 2018 at 12:54 AM, Russell King - ARM Linux
 wrote:
> On Wed, Apr 25, 2018 at 08:33:12AM -0700, Christoph Hellwig wrote:
>> On Wed, Apr 25, 2018 at 12:04:29PM +0200, Daniel Vetter wrote:
>> > - dma api hides the cache flushing requirements from us. GPUs love
>> >   non-snooped access, and worse give userspace control over that. We want
>> >   a strict separation between mapping stuff and flushing stuff. With the
>> >   IOMMU api we mostly have the former, but for the later arch maintainers
>> >   regularly tells they won't allow that. So we have drm_clflush.c.
>>
>> The problem is that a cache flushing API entirely separate is hard. That
>> being said if you look at my generic dma-noncoherent API series it tries
>> to move that way.  So far it is in early stages and apparently rather
>> buggy unfortunately.
>
> And if folk want a cacheable mapping with explicit cache flushing, the
> cache flushing must not be defined in terms of "this is what CPU seems
> to need" but from the point of view of a CPU with infinite prefetching,
> infinite caching and infinite capacity to perform writebacks of dirty
> cache lines at unexpected moments when the memory is mapped in a
> cacheable mapping.
>
> (The reason for that is you're operating in a non-CPU specific space,
> so you can't make any guarantees as to how much caching or prefetching
> will occur by the CPU - different CPUs will do different amounts.)
>
> So, for example, the sequence:
>
> GPU writes to memory
> CPU reads from cacheable memory
>
> if the memory was previously dirty (iow, CPU has written), you need to
> flush the dirty cache lines _before_ the GPU writes happen, but you
> don't know whether the CPU has speculatively prefetched, so you need
> to flush any prefetched cache lines before reading from the cacheable
> memory _after_ the GPU has finished writing.
>
> Also note that "flush" there can be "clean the cache", "clean and
> invalidate the cache" or "invalidate the cache" as appropriate - some
> CPUs are able to perform those three operations, and the appropriate
> one depends on not only where in the above sequence it's being used,
> but also on what the operations are.
>
> So, the above sequence could be:
>
> CPU invalidates cache for memory
> (due to possible dirty cache lines)
> GPU writes to memory
> CPU invalidates cache for memory
> (to get rid of any speculatively prefetched
>  lines)
> CPU reads from cacheable memory
>
> Yes, in the above case, _two_ cache operations are required to ensure
> correct behaviour.  However, if you know for certain that the memory was
> previously clean, then the first cache operation can be skipped.
>
> What I'm pointing out is there's much more than just "I want to flush
> the cache" here, which is currently what DRM seems to assume at the
> moment with the code in drm_cache.c.
>
> If we can agree a set of interfaces that allows _proper_ use of these
> facilities, one which can be used appropriately, then there shouldn't
> be a problem.  The DMA API does that via it's ideas about who owns a
> particular buffer (because of the above problem) and that's something
> which would need to be carried over to such a cache flushing API (it
> should be pretty obvious that having a GPU read or write memory while
> the cache for that memory is being cleaned will lead to unexpected
> results.)
>
> Also note that things get even more interesting in a SMP environment
> if cache operations aren't broadcasted across the SMP cluster (which
> means cache operations have to be IPI'd to other CPUs.)
>
> The next issue, which I've brought up before, is that exposing cache
> flushing to userspace on architectures where it isn't already exposed
> comes.  As has been shown by Google Project Zero, this risks exposing
> those architectures to Spectre and Meltdown exploits where they weren't
> at such a risk before.  (I've pretty much shown here that you _do_
> need to control which cache lines get flushed to make these exploits
> work, and flushing the cache by reading lots of data in liu of having
> the ability to explicitly flush bits of cache makes it very difficult
> to impossible for them to work.)

The above is already what we're implementing in i915, at least
conceptually (it all boils down to clflush instructions because those
both invalidate and flush).

One architectural guarantee we're exploiting is that prefetched (and
hence non-dirty) cachelines will never get written back, but dropped
instead. But we kinda need that, otherwise the cpu could randomly
corrupt the data the gpu is writing and non-coherent would just not
work on those platforms. But aside from that, yes we do an invalidate
before reading, and flushing after every writing (or anything else
that could leave dirty cachelines behind). 

Re: noveau vs arm dma ops

[Daniel Vetter]

On Thu, Apr 26, 2018 at 1:26 AM, Russell King - ARM Linux
 wrote:
> On Wed, Apr 25, 2018 at 11:35:13PM +0200, Daniel Vetter wrote:
>> On arm that doesn't work. The iommu api seems like a good fit, except
>> the dma-api tends to get in the way a bit (drm/msm apparently has
>> similar problems like tegra), and if you need contiguous memory
>> dma_alloc_coherent is the only way to get at contiguous memory. There
>> was a huge discussion years ago about that, and direct cma access was
>> shot down because it would have exposed too much of the caching
>> attribute mangling required (most arm platforms need wc-pages to not
>> be in the kernel's linear map apparently).
>
> I think you completely misunderstand ARM from what you've written above,
> and this worries me greatly about giving DRM the level of control that
> is being asked for.
>
> Modern ARMs have a PIPT cache or a non-aliasing VIPT cache, and cache
> attributes are stored in the page tables.  These caches are inherently
> non-aliasing when there are multiple mappings (which is a great step
> forward compared to the previous aliasing caches.)
>
> As the cache attributes are stored in the page tables, this in theory
> allows different virtual mappings of the same physical memory to have
> different cache attributes.  However, there's a problem, and that's
> called speculative prefetching.
>
> Let's say you have one mapping which is cacheable, and another that is
> marked as write combining.  If a cache line is speculatively prefetched
> through the cacheable mapping of this memory, and then you read the
> same physical location through the write combining mapping, it is
> possible that you could read cached data.
>
> So, it is generally accepted that all mappings of any particular
> physical bit of memory should have the same cache attributes to avoid
> unpredictable behaviour.
>
> This presents a problem with what is generally called "lowmem" where
> the memory is mapped in kernel virtual space with cacheable
> attributes.  It can also happen with highmem if the memory is
> kmapped.
>
> This is why, on ARM, you can't use something like get_free_pages() to
> grab some pages from the system, pass it to the GPU, map it into
> userspace as write-combining, etc.  It _might_ work for some CPUs,
> but ARM CPUs vary in how much prefetching they do, and what may work
> for one particular CPU is in no way guaranteed to work for another
> ARM CPU.
>
> The official line from architecture folk is to assume that the caches
> infinitely speculate, are of infinite size, and can writeback *dirty*
> data at any moment.
>
> The way to stop things like speculative prefetches to particular
> physical memory is to, quite "simply", not have any cacheable
> mappings of that physical memory anywhere in the system.
>
> Now, cache flushes on ARM tend to be fairly expensive for GPU buffers.
> If you have, say, an 8MB buffer (for a 1080p frame) and you need to
> do a cache operation on that buffer, you'll be iterating over it
> 32 or maybe 64 bytes at a time "just in case" there's a cache line
> present.  Referring to my previous email, where I detailed the
> potential need for _two_ flushes, one before the GPU operation and
> one after, and this becomes _really_ expensive.  At that point, you're
> probably way better off using write-combine memory where you don't
> need to spend CPU cycles performing cache flushing - potentially
> across all CPUs in the system if cache operations aren't broadcasted.
>
> This isn't a simple matter of "just provide some APIs for cache
> operations" - there's much more that needs to be understood by
> all parties here, especially when we have GPU drivers that can be
> used with quite different CPUs.
>
> It may well be that for some combinations of CPUs and workloads, it's
> better to use write-combine memory without cache flushing, but for
> other CPUs that tradeoff (for the same workload) could well be
> different.
>
> Older ARMs get more interesting, because they have aliasing caches.
> That means the CPU cache aliases across different virtual space
> mappings in some way, which complicates (a) the mapping of memory
> and (b) handling the cache operations on it.
>
> It's too late for me to go into that tonight, and I probably won't
> be reading mail for the next week and a half, sorry.

I didn't know all the details well enough (and neither had the time to
write a few paragraphs like you did), but the above is what I had in
mind and meant. Sorry if my sloppy reply sounded like I'm mixing stuff
up.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: noveau vs arm dma ops

[Christoph Hellwig]

On Wed, Apr 25, 2018 at 11:54:43PM +0100, Russell King - ARM Linux wrote:
> 
> if the memory was previously dirty (iow, CPU has written), you need to
> flush the dirty cache lines _before_ the GPU writes happen, but you
> don't know whether the CPU has speculatively prefetched, so you need
> to flush any prefetched cache lines before reading from the cacheable
> memory _after_ the GPU has finished writing.
> 
> Also note that "flush" there can be "clean the cache", "clean and
> invalidate the cache" or "invalidate the cache" as appropriate - some
> CPUs are able to perform those three operations, and the appropriate
> one depends on not only where in the above sequence it's being used,
> but also on what the operations are.

Agreed on all these counts.

> If we can agree a set of interfaces that allows _proper_ use of these
> facilities, one which can be used appropriately, then there shouldn't
> be a problem.  The DMA API does that via it's ideas about who owns a
> particular buffer (because of the above problem) and that's something
> which would need to be carried over to such a cache flushing API (it
> should be pretty obvious that having a GPU read or write memory while
> the cache for that memory is being cleaned will lead to unexpected
> results.)

I've been trying to come up with such an interface, for now only for
internal use in a generic set of noncoherent ops.  The API is basically
a variant of the existing dma_sync_single_to_device/cpu calls:

http://git.infradead.org/users/hch/misc.git/commitdiff/044dae5f94509288f4655de2f22cb0bea85778af

Review welcome!

> The next issue, which I've brought up before, is that exposing cache
> flushing to userspace on architectures where it isn't already exposed
> comes.  As has been shown by Google Project Zero, this risks exposing
> those architectures to Spectre and Meltdown exploits where they weren't
> at such a risk before.  (I've pretty much shown here that you _do_
> need to control which cache lines get flushed to make these exploits
> work, and flushing the cache by reading lots of data in liu of having
> the ability to explicitly flush bits of cache makes it very difficult
> to impossible for them to work.)

Extending dma coherence to userspace is going to be the next major
nightmare indeed.  I'm not sure how much of that actually still is
going on in the graphics world, but we'll need a coherent (pun intended)
plan how to deal with it.

Re: noveau vs arm dma ops

[Christoph Hellwig]

On Wed, Apr 25, 2018 at 11:35:13PM +0200, Daniel Vetter wrote:
> > get_required_mask() is supposed to tell you if you are safe.  However
> > we are missing lots of implementations of it for iommus so you might get
> > some false negatives, improvements welcome.  It's been on my list of
> > things to fix in the DMA API, but it is nowhere near the top.
> 
> I hasn't come up in a while in some fireworks, so I honestly don't
> remember exactly what the issues have been. But
> 
> commit d766ef53006c2c38a7fe2bef0904105a793383f2
> Author: Chris Wilson 
> Date:   Mon Dec 19 12:43:45 2016 +
> 
> drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping
> 
> and the various bits of code that a
> 
> $ git grep SWIOTLB -- drivers/gpu
> 
> turns up is what we're doing to hack around that stuff. And in general
> (there's some exceptions) gpus should be able to address everything,
> so I never fully understood where that's even coming from.

I'm pretty sure I've seen some oddly low dma masks in GPU drivers.  E.g.
duplicated in various AMD files:

adev->need_dma32 = false;
dma_bits = adev->need_dma32 ? 32 : 40;
r = pci_set_dma_mask(adev->pdev, DMA_BIT_MASK(dma_bits));
if (r) {
adev->need_dma32 = true;
dma_bits = 32;
dev_warn(adev->dev, "amdgpu: No suitable DMA available.\n");
}

synopsis:

drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c:pdevinfo.dma_mask   
= DMA_BIT_MASK(32);
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c:  pdevinfo.dma_mask = 
DMA_BIT_MASK(32);
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c:  pdevinfo.dma_mask = 
DMA_BIT_MASK(32);

etnaviv gets it right:

drivers/gpu/drm/etnaviv/etnaviv_gpu.c:  u32 dma_mask = 
(u32)dma_get_required_mask(gpu->dev);


But yes, the swiotlb hackery really irks me.  I just have some more
important and bigger fires to fight first, but I plan to get back to the
root cause of that eventually.

> 
> >> - dma api hides the cache flushing requirements from us. GPUs love
> >>   non-snooped access, and worse give userspace control over that. We want
> >>   a strict separation between mapping stuff and flushing stuff. With the
> >>   IOMMU api we mostly have the former, but for the later arch maintainers
> >>   regularly tells they won't allow that. So we have drm_clflush.c.
> >
> > The problem is that a cache flushing API entirely separate is hard. That
> > being said if you look at my generic dma-noncoherent API series it tries
> > to move that way.  So far it is in early stages and apparently rather
> > buggy unfortunately.
> 
> I'm assuming this stuff here?
> 
> https://lkml.org/lkml/2018/4/20/146
> 
> Anyway got lost in all that work a bit, looks really nice.

That url doesn't seem to work currently.  But I am talking about the
thread titled '[RFC] common non-cache coherent direct dma mapping ops'

> Yeah the above is pretty much what we do on x86. dma-api believes
> everything is coherent, so dma_map_sg does the mapping we want and
> nothing else (minus swiotlb fun). Cache flushing, allocations, all
> done by the driver.

Which sounds like the right thing to do to me.

> On arm that doesn't work. The iommu api seems like a good fit, except
> the dma-api tends to get in the way a bit (drm/msm apparently has
> similar problems like tegra), and if you need contiguous memory
> dma_alloc_coherent is the only way to get at contiguous memory. There
> was a huge discussion years ago about that, and direct cma access was
> shot down because it would have exposed too much of the caching
> attribute mangling required (most arm platforms need wc-pages to not
> be in the kernel's linear map apparently).

Simple cma_alloc() doesn't do anything about cache handling, it
just is a very dumb allocator for large contiguous regions inside
a big pool.

I'm not the CMA maintainer, but in general I'd love to see an
EXPORT_SYMBOL_GPL slapped onto cma_alloc/release and drivers use
that were needed.  Using that plus dma_map*/dma_unmap* sounds like
a much saner interface than dma_alloc_attrs + DMA_ATTR_NON_CONSISTENT
or DMA_ATTR_NO_KERNEL_MAPPING.

You don't happen to have a pointer to that previous discussion?

> Anything that separate these 3 things more (allocation pools, mapping
> through IOMMUs and flushing cpu caches) sounds like the right
> direction to me. Even if that throws some portability across platforms
> away - drivers who want to control things in this much detail aren't
> really portable (without some serious work) anyway.

As long as we stay away from the dma coherent allocations separating
them is fine, and I'm working towards it.  dma coherent allocations on
the other hand are very hairy beasts, and provide by very different
implementations depending on the architecture, or often even depending
on the specifics of individual implementations inside the architecture.

So for your GPU/media case it seems like you'd better 

Re: [PATCH 11/28] venus: add common capability parser

[kbuild test robot]

Hi Stanimir,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.17-rc2 next-20180424]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Stanimir-Varbanov/Venus-updates/20180426-030344
base:   git://linuxtv.org/media_tree.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/media/platform/qcom/venus/core.c: In function 
'venus_enumerate_codecs':
>> drivers/media/platform/qcom/venus/core.c:225:1: warning: the frame size of 
>> 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
}
^

vim +225 drivers/media/platform/qcom/venus/core.c

   181  
   182  static int venus_enumerate_codecs(struct venus_core *core, u32 type)
   183  {
   184  const struct hfi_inst_ops dummy_ops = {};
   185  struct venus_inst inst;
   186  unsigned int i;
   187  u32 codec, codecs;
   188  int ret;
   189  
   190  if (core->res->hfi_version != HFI_VERSION_1XX)
   191  return 0;
   192  
   193  memset(, 0, sizeof(inst));
   194  mutex_init();
   195  inst.core = core;
   196  inst.session_type = type;
   197  if (type == VIDC_SESSION_TYPE_DEC)
   198  codecs = core->dec_codecs;
   199  else
   200  codecs = core->enc_codecs;
   201  
   202  ret = hfi_session_create(, _ops);
   203  if (ret)
   204  return ret;
   205  
   206  for (i = 0; i < MAX_CODEC_NUM; i++) {
   207  codec = (1 << i) & codecs;
   208  if (!codec)
   209  continue;
   210  
   211  ret = hfi_session_init(, 
to_v4l2_codec_type(codec));
   212  if (ret)
   213  goto done;
   214  
   215  ret = hfi_session_deinit();
   216  if (ret)
   217  goto done;
   218  }
   219  
   220  done:
   221  hfi_session_destroy();
   222  mutex_destroy();
   223  
   224  return ret;
 > 225  }
   226  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 2/2] ARM: dts: r8a7740: Enable CEU0

[Simon Horman]

On Thu, Apr 26, 2018 at 09:26:09AM +0200, jacopo mondi wrote:
> Hi Simon,
> 
> On Thu, Apr 26, 2018 at 08:11:30AM +0200, Simon Horman wrote:
> > Thanks Jacopo,
> >
> > I'm very pleased to see this series.
> 
> Credits to Geert that pointed out to me R-Mobile A1 comes with a CEU.
> I should mention him in next iteration actually, sorry about that.
> 
> >
> > On Wed, Apr 25, 2018 at 01:15:20PM +0200, Jacopo Mondi wrote:
> > > Enable CEU0 peripheral for Renesas R-Mobile A1 R8A7740.
> >
> > Given 'status = "disabled"' below I think you
> > are describing but not enabling CEU0. Also in the subject.
> 
> Right.
> 
> >
> > Should we also describe CEU1?
> 
> Armadillo board file only describe CEU0. If there are R-Mobile A1
> board files where I can steal informations from I can do that. If
> there's a public datasheet, that would be even better.

I have the datasheet, so perhaps I can add CEU1 after you have added CEU0.

> > > Signed-off-by: Jacopo Mondi 
> > > ---
> > >  arch/arm/boot/dts/r8a7740.dtsi | 10 ++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/r8a7740.dtsi 
> > > b/arch/arm/boot/dts/r8a7740.dtsi
> > > index afd3bc5..05ec41e 100644
> > > --- a/arch/arm/boot/dts/r8a7740.dtsi
> > > +++ b/arch/arm/boot/dts/r8a7740.dtsi
> > > @@ -67,6 +67,16 @@
> > >   power-domains = <_d4>;
> > >   };
> > >
> > > + ceu0: ceu@fe91 {
> > > + reg = <0xfe91 0x100>;
> >
> > Should the size of the range be 0x3000 ?
> > That would seem to match my reading of table 32.3
> > and also be consistent with r7s72100.dtsi.
> 
> I got this from
> 
> static struct resource ceu0_resources[] = {
>   [0] = {
>   .name   = "CEU",
>   .start  = 0xfe91,
>   .end= 0xfe91009f,
>   .flags  = IORESOURCE_MEM,
>   },
> but I also noticed the r7s72100 one was bigger.
> I'm fine enlarging this, if that's what the manual reports too.

I think that would be best.

> > > + compatible = "renesas,r8a7740-ceu";
> > > + interrupts = ;
> > > + clocks = <_clks R8A7740_CLK_CEU20>;
> > > + clock-names = "ceu20";
> > > + power-domains = <_a4mp>;
> >
> > My reading of table 1.7 is that the power domain should be A4R (_a4r).
> 
> Ah yes, my bad.
> 
> The long time goal would be describe the camera module (mt9t112) which
> is installed on armadillo. Unfortunately that would probably require
> some more work on the CEU side.

Thanks, understood.

Re: [PATCH][next] media: ispstat: don't dereference user_cfg before a null check

[Sakari Ailus]

On Tue, Apr 24, 2018 at 02:06:18PM +0100, Colin King wrote:
> From: Colin Ian King 
> 
> The pointer user_cfg (a copy of new_conf) is dereference before
> new_conf is null checked, hence we may have a null pointer dereference
> on user_cfg when assigning buf_size from user_cfg->buf_size. Ensure
> this does not occur by moving the assignment of buf_size after the
> null check.
> 
> Detected by CoverityScan, CID#1468386 ("Dereference before null check")
> 
> Fixes: 68e342b3068c ("[media] omap3isp: Statistics")
> Signed-off-by: Colin Ian King 

Thanks for the patch.

Gustavo sent effectively the same patch a moment earlier, and that patch
got applied instead.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

[Sakari Ailus]

On Tue, Apr 24, 2018 at 02:03:22PM +0200, Daniel Mack wrote:
> Hi,
> 
> On Tuesday, April 24, 2018 12:22 PM, Sakari Ailus wrote:
> > On Fri, Apr 20, 2018 at 11:44:18AM +0200, Daniel Mack wrote:
> >> Add v4l2 controls to report the pixel and MIPI link rates of each mode.
> >> The camss camera subsystem needs them to set up the correct hardware
> >> clocks.
> >>
> >> Tested on msm8016 based hardware.
> >>
> >> Signed-off-by: Daniel Mack 
> > 
> > Maxime has written a number of patches against the driver that seem very
> > much related; could you rebase these on his set (v2)?
> > 
> > 
> 
> I didn't know about the ongoing work in this area, so I think both this
> and 3/3 are not needed. If you want, you can still pick the 1st patch in
> this series, but that's just a cosmetic cleanup.

That patch, too, would effectively need a rebase.

I'd also suggest adding a macro that constructs the entries in the array
--- much of what changes from entry to entry are fps, width, height and
whether subsampling or scaling has been used. That information would likely
fit nicely on a single line.

The resolution names are also redundant as the size is explicitly part of
the mode list variable names.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH 0/9] Do some atomisp cleanups

[Sakari Ailus]

On Mon, Apr 16, 2018 at 12:37:03PM -0400, Mauro Carvalho Chehab wrote:
> When I started building media subsystem with the atomisp driver,
> I ended by adding several hacks on their Makefiles, in order to
> get rid of thousands of warnings. I felt a little guty of hiding how
> broken is this driver, so I decided t remove two Makefile hacks that
> affect sensors and fix the warnings. 
> 
> Yet, there's still one such hack at 
> drivers/staging/media/atomisp/pci/atomisp2/Makefile, with:
> 
> # HACK! While this driver is in bad shape, don't enable several warnings
> #   that would be otherwise enabled with W=1
> ccflags-y += $(call cc-disable-warning, implicit-fallthrough)
> ccflags-y += $(call cc-disable-warning, missing-prototypes)
> ccflags-y += $(call cc-disable-warning, missing-declarations)
> ccflags-y += $(call cc-disable-warning, suggest-attribute=format)
> ccflags-y += $(call cc-disable-warning, unused-const-variable)
> ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
> 
> Getting his of those is a big task, as there are thousands of warnings
> hidden there. In order to seriously get rid of them, one should start
> getting rid of the several abstraction layers at the driver and have
> hardware for test.
> 
> As I don't have any hardware to test, nor any reason why
> dedicating myself to such task, I'll just leave this task for others
> to do.

Thanks. Feel free to add:

Acked-by: Sakari Ailus 

And take the patches through your tree.

-- 
Kind regards,

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

Re: [PATCH v2 13/13] media: imx274: add SELECTION support for cropping

[Sakari Ailus]

Hi Luca,

On Tue, Apr 24, 2018 at 04:30:38PM +0200, Luca Ceresoli wrote:
> Hi Sakari,
> 
> On 24/04/2018 11:59, Sakari Ailus wrote:
> > Hi Luca,
> > 
> > Thank you for the patchset.
> > 
> > Some comments below... what I propose is that I apply the rest of the
> > patches and then the comments to this one could be addressed separately.
> > Would that work for you?
> 
> Yes, but I suggest you only apply patches 1-6. I noticed a warning in
> patch 9, and patches 7-8 are not very useful without it. Will fix it in v3.

Ack. I'll apply 1--6 then.

> 
> I'll wait for the outcome of the discussion below before sending v3.
> Tell me if you prefer me to do it earlier.
> 
> > On Tue, Apr 24, 2018 at 10:24:18AM +0200, Luca Ceresoli wrote:
> >> Currently this driver does not support cropping. The supported modes
> >> are the following, all capturing the entire area:
> >>
> >>  - 3840x2160, 1:1 binning (native sensor resolution)
> >>  - 1920x1080, 2:1 binning
> >>  - 1280x720,  3:1 binning
> >>
> >> The set_fmt callback chooses among these 3 configurations the one that
> >> matches the requested format.
> >>
> >> Adding support to VIDIOC_SUBDEV_G_SELECTION and
> >> VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
> >> which now chooses the most appropriate binning based on the ratio
> >> between the previously-set cropping size and the format size being
> >> requested.
> >>
> >> Note that the names in enum imx274_mode change from being
> >> resolution-based to being binning-based. Without cropping, the two
> >> naming are equivalent. With cropping, the resolution could be
> >> different, e.g. using 2:1 binning mode to crop 1200x960 and output a
> >> 600x480 format. Using binning in the names avoids anny
> >> misunderstanding.
> >>
> >> Signed-off-by: Luca Ceresoli 
> >>
> >> ---
> >> Changed v1 -> v2:
> >>  - add "media: " prefix to commit message
> >> ---
> >>  drivers/media/i2c/imx274.c | 266 
> >> -
> >>  1 file changed, 192 insertions(+), 74 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> >> index b6c54712f2aa..ceb5b3e498c6 100644
> >> --- a/drivers/media/i2c/imx274.c
> >> +++ b/drivers/media/i2c/imx274.c
> >> @@ -19,6 +19,7 @@
> >>   * along with this program.  If not, see .
> >>   */
> >>  
> >> +#include 
> >>  #include 
> >>  #include 
> >>  #include 
> >> @@ -74,7 +75,7 @@
> >>   */
> >>  #define IMX274_MIN_EXPOSURE_TIME  (4 * 260 / 72)
> >>  
> >> -#define IMX274_DEFAULT_MODE   IMX274_MODE_3840X2160
> >> +#define IMX274_DEFAULT_MODE   IMX274_MODE_BINNING_OFF
> >>  #define IMX274_MAX_WIDTH  (3840)
> >>  #define IMX274_MAX_HEIGHT (2160)
> >>  #define IMX274_MAX_FRAME_RATE (120)
> >> @@ -111,6 +112,20 @@
> >>  #define IMX274_SHR_REG_LSB0x300C /* SHR */
> >>  #define IMX274_SVR_REG_MSB0x300F /* SVR */
> >>  #define IMX274_SVR_REG_LSB0x300E /* SVR */
> >> +#define IMX274_HTRIM_EN_REG   0x3037
> >> +#define IMX274_HTRIM_START_REG_LSB0x3038
> >> +#define IMX274_HTRIM_START_REG_MSB0x3039
> >> +#define IMX274_HTRIM_END_REG_LSB  0x303A
> >> +#define IMX274_HTRIM_END_REG_MSB  0x303B
> >> +#define IMX274_VWIDCUTEN_REG  0x30DD
> >> +#define IMX274_VWIDCUT_REG_LSB0x30DE
> >> +#define IMX274_VWIDCUT_REG_MSB0x30DF
> >> +#define IMX274_VWINPOS_REG_LSB0x30E0
> >> +#define IMX274_VWINPOS_REG_MSB0x30E1
> >> +#define IMX274_WRITE_VSIZE_REG_LSB0x3130
> >> +#define IMX274_WRITE_VSIZE_REG_MSB0x3131
> >> +#define IMX274_Y_OUT_SIZE_REG_LSB 0x3132
> >> +#define IMX274_Y_OUT_SIZE_REG_MSB 0x3133
> >>  #define IMX274_VMAX_REG_1 0x30FA /* VMAX, MSB */
> >>  #define IMX274_VMAX_REG_2 0x30F9 /* VMAX */
> >>  #define IMX274_VMAX_REG_3 0x30F8 /* VMAX, LSB */
> >> @@ -141,9 +156,9 @@ static const struct regmap_config imx274_regmap_config 
> >> = {
> >>  };
> >>  
> >>  enum imx274_mode {
> >> -  IMX274_MODE_3840X2160,
> >> -  IMX274_MODE_1920X1080,
> >> -  IMX274_MODE_1280X720,
> >> +  IMX274_MODE_BINNING_OFF,
> >> +  IMX274_MODE_BINNING_2_1,
> >> +  IMX274_MODE_BINNING_3_1,
> >>  };
> >>  
> >>  /*
> >> @@ -215,31 +230,14 @@ static const struct reg_8 
> >> imx274_mode1_3840x2160_raw10[] = {
> >>{0x3004, 0x01},
> >>{0x3005, 0x01},
> >>{0x3006, 0x00},
> >> -  {0x3007, 0x02},
> >> +  {0x3007, 0xa2},
> >>  
> >>{0x3018, 0xA2}, /* output XVS, HVS */
> >>  
> >>{0x306B, 0x05},
> >>{0x30E2, 0x01},
> >> -  {0x30F6, 0x07}, /* HMAX, 263 */
> >> -  {0x30F7, 0x01}, /* HMAX */
> >> -
> >> -  {0x30dd, 0x01}, /* crop to 2160 */
> >> -  {0x30de, 

Re: [PATCH 2/2] ARM: dts: r8a7740: Enable CEU0

[jacopo mondi]

Hi Simon,

On Thu, Apr 26, 2018 at 08:11:30AM +0200, Simon Horman wrote:
> Thanks Jacopo,
>
> I'm very pleased to see this series.

Credits to Geert that pointed out to me R-Mobile A1 comes with a CEU.
I should mention him in next iteration actually, sorry about that.

>
> On Wed, Apr 25, 2018 at 01:15:20PM +0200, Jacopo Mondi wrote:
> > Enable CEU0 peripheral for Renesas R-Mobile A1 R8A7740.
>
> Given 'status = "disabled"' below I think you
> are describing but not enabling CEU0. Also in the subject.

Right.

>
> Should we also describe CEU1?

Armadillo board file only describe CEU0. If there are R-Mobile A1
board files where I can steal informations from I can do that. If
there's a public datasheet, that would be even better.
>
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  arch/arm/boot/dts/r8a7740.dtsi | 10 ++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/r8a7740.dtsi b/arch/arm/boot/dts/r8a7740.dtsi
> > index afd3bc5..05ec41e 100644
> > --- a/arch/arm/boot/dts/r8a7740.dtsi
> > +++ b/arch/arm/boot/dts/r8a7740.dtsi
> > @@ -67,6 +67,16 @@
> > power-domains = <_d4>;
> > };
> >
> > +   ceu0: ceu@fe91 {
> > +   reg = <0xfe91 0x100>;
>
> Should the size of the range be 0x3000 ?
> That would seem to match my reading of table 32.3
> and also be consistent with r7s72100.dtsi.

I got this from

static struct resource ceu0_resources[] = {
[0] = {
.name   = "CEU",
.start  = 0xfe91,
.end= 0xfe91009f,
.flags  = IORESOURCE_MEM,
},
but I also noticed the r7s72100 one was bigger.
I'm fine enlarging this, if that's what the manual reports too.

> > +   compatible = "renesas,r8a7740-ceu";
> > +   interrupts = ;
> > +   clocks = <_clks R8A7740_CLK_CEU20>;
> > +   clock-names = "ceu20";
> > +   power-domains = <_a4mp>;
>
> My reading of table 1.7 is that the power domain should be A4R (_a4r).

Ah yes, my bad.

The long time goal would be describe the camera module (mt9t112) which
is installed on armadillo. Unfortunately that would probably require
some more work on the CEU side.

Thanks
   j

>
> > +   status = "disabled";
> > +   };
> > +
> > cmt1: timer@e6138000 {
> > compatible = "renesas,cmt-48-r8a7740", "renesas,cmt-48";
> > reg = <0xe6138000 0x170>;
> > --
> > 2.7.4
> >


signature.asc
Description: PGP signature

Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor

[Sakari Ailus]

On Thu, Apr 26, 2018 at 10:04:25AM +0300, Todor Tomov wrote:
> Hi Sakari,
> 
> On 26.04.2018 09:50, Sakari Ailus wrote:
> > Hi Todor,
> > 
> > On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote:
> > ...
> >> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val)
> >> +{
> >> +  u8 regbuf[3];
> >> +  int ret;
> >> +
> >> +  regbuf[0] = reg >> 8;
> >> +  regbuf[1] = reg & 0xff;
> >> +  regbuf[2] = val;
> >> +
> >> +  ret = i2c_master_send(ov7251->i2c_client, regbuf, 3);
> >> +  if (ret < 0) {
> >> +  dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n",
> >> +  __func__, ret, reg, val);
> >> +  return ret;
> >> +  }
> >> +
> >> +  return 0;
> > 
> > How about:
> > 
> > return ov7251_write_seq_regs(ov7251, reg, , 1);
> > 
> > And put the function below ov2751_write_seq_regs().
> 
> I'm not sure... It will calculate message length each time and then check
> that it is not greater than 5, which it is. Seems redundant.
> 
> > 
> >> +}
> >> +
> >> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val,
> >> +   u8 num)
> >> +{
> >> +  const u8 maxregbuf = 5;
> >> +  u8 regbuf[maxregbuf];
> >> +  u8 nregbuf = sizeof(reg) + num * sizeof(*val);
> >> +  int ret = 0;
> >> +
> >> +  if (nregbuf > maxregbuf)
> >> +  return -EINVAL;
> >> +
> >> +  regbuf[0] = reg >> 8;
> >> +  regbuf[1] = reg & 0xff;
> >> +
> >> +  memcpy(regbuf + 2, val, num);
> >> +
> >> +  ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf);
> >> +  if (ret < 0) {
> >> +  dev_err(ov7251->dev, "%s: write seq regs error %d: first 
> >> reg=%x\n",
> > 
> > This line is over 80... 
> 
> Yes indeed. Somehow checkpatch does not report this line, I don't know why.
> 
> > 
> > If you're happy with these, I can make the changes, too; they're trivial.
> 
> Only the second one? Thanks :)

Works for me. I'd still think the overhead of managing the buffer is
irrelevant where to having an extra function to do essentially the same
thing is a source of maintenance and review work. Note that we're even now
spending time to discuss it. ;-)

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device driver

[Tomasz Figa]

Hi Yong,

On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi  wrote:
[snip]
> +static int imgu_video_nodes_init(struct imgu_device *imgu)
> +{
> +   struct v4l2_pix_format_mplane *fmts[IPU3_CSS_QUEUES] = { NULL };
> +   struct v4l2_rect *rects[IPU3_CSS_RECTS] = { NULL };
> +   unsigned int i;
> +   int r;
> +
> +   imgu->buf_struct_size = sizeof(struct imgu_buffer);
> +
> +   for (i = 0; i < IMGU_NODE_NUM; i++) {
> +   imgu->nodes[i].name = imgu_node_map[i].name;
> +   imgu->nodes[i].output = i < IMGU_QUEUE_FIRST_INPUT;
> +   imgu->nodes[i].immutable = false;
> +   imgu->nodes[i].enabled = false;
> +
> +   if (i != IMGU_NODE_PARAMS && i != IMGU_NODE_STAT_3A)
> +   fmts[imgu_node_map[i].css_queue] =
> +   >nodes[i].vdev_fmt.fmt.pix_mp;
> +   atomic_set(>nodes[i].sequence, 0);
> +   }
> +
> +   /* Master queue is always enabled */
> +   imgu->nodes[IMGU_QUEUE_MASTER].immutable = true;
> +   imgu->nodes[IMGU_QUEUE_MASTER].enabled = true;
> +
> +   r = ipu3_v4l2_register(imgu);
> +   if (r)
> +   return r;
> +
> +   /* Set initial formats and initialize formats of video nodes */
> +   rects[IPU3_CSS_RECT_EFFECTIVE] = >rect.eff;
> +   rects[IPU3_CSS_RECT_BDS] = >rect.bds;
> +   ipu3_css_fmt_set(>css, fmts, rects);
> +
> +   /* Pre-allocate dummy buffers */
> +   r = imgu_dummybufs_preallocate(imgu);
> +   if (r) {
> +   dev_err(>pci_dev->dev,
> +   "failed to pre-allocate dummy buffers (%d)", r);
> +   imgu_dummybufs_cleanup(imgu);

No need to call ipu3_v4l2_unregister() here?

(That's why I keep suggesting use of single return error path with labels
named after the first cleanup step that needs to be done, as it makes it
easier to spot such mistakes.)

> +   return r;
> +   }
> +
> +   return 0;
> +}
> +
> +static void imgu_video_nodes_exit(struct imgu_device *imgu)
> +{
> +   imgu_dummybufs_cleanup(imgu);
> +   ipu3_v4l2_unregister(imgu);
> +}

Best regards,
Tomasz

Re: [PATCH v6 10/12] intel-ipu3: Add css pipeline programming

[Tomasz Figa]

Hi Yong,

On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi  wrote:
[snip]
> +int ipu3_css_init(struct device *dev, struct ipu3_css *css,
> + void __iomem *base, int length)
> +{
> +   int r, p, q, i;
> +
> +   /* Initialize main data structure */
> +   css->dev = dev;
> +   css->base = base;
> +   css->iomem_length = length;
> +   css->current_binary = IPU3_CSS_DEFAULT_BINARY;
> +   css->pipe_id = IPU3_CSS_PIPE_ID_NUM;
> +   css->vf_output_en = IPU3_NODE_VF_DISABLED;
> +   spin_lock_init(>qlock);
> +
> +   for (q = 0; q < IPU3_CSS_QUEUES; q++) {
> +   r = ipu3_css_queue_init(>queue[q], NULL, 0);
> +   if (r)
> +   return r;
> +   }
> +
> +   r = ipu3_css_fw_init(css);
> +   if (r)
> +   return r;
> +
> +   /* Allocate and map common structures with imgu hardware */
> +
> +   for (p = 0; p < IPU3_CSS_PIPE_ID_NUM; p++)
> +   for (i = 0; i < IMGU_ABI_MAX_STAGES; i++) {
> +   if (!ipu3_dmamap_alloc(dev,
> +
  >xmem_sp_stage_ptrs[p][i],
> +  sizeof(struct
imgu_abi_sp_stage)))

checkpatch reports line over 80 characters here.

> +   goto error_no_memory;
> +   if (!ipu3_dmamap_alloc(dev,
> +
  >xmem_isp_stage_ptrs[p][i],
> +  sizeof(struct
imgu_abi_isp_stage)))

Ditto.

> +   goto error_no_memory;
> +   }

Best regards,
Tomasz

Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor

[Todor Tomov]

Hi Sakari,

On 26.04.2018 09:50, Sakari Ailus wrote:
> Hi Todor,
> 
> On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote:
> ...
>> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val)
>> +{
>> +u8 regbuf[3];
>> +int ret;
>> +
>> +regbuf[0] = reg >> 8;
>> +regbuf[1] = reg & 0xff;
>> +regbuf[2] = val;
>> +
>> +ret = i2c_master_send(ov7251->i2c_client, regbuf, 3);
>> +if (ret < 0) {
>> +dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n",
>> +__func__, ret, reg, val);
>> +return ret;
>> +}
>> +
>> +return 0;
> 
> How about:
> 
>   return ov7251_write_seq_regs(ov7251, reg, , 1);
> 
> And put the function below ov2751_write_seq_regs().

I'm not sure... It will calculate message length each time and then check
that it is not greater than 5, which it is. Seems redundant.

> 
>> +}
>> +
>> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val,
>> + u8 num)
>> +{
>> +const u8 maxregbuf = 5;
>> +u8 regbuf[maxregbuf];
>> +u8 nregbuf = sizeof(reg) + num * sizeof(*val);
>> +int ret = 0;
>> +
>> +if (nregbuf > maxregbuf)
>> +return -EINVAL;
>> +
>> +regbuf[0] = reg >> 8;
>> +regbuf[1] = reg & 0xff;
>> +
>> +memcpy(regbuf + 2, val, num);
>> +
>> +ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf);
>> +if (ret < 0) {
>> +dev_err(ov7251->dev, "%s: write seq regs error %d: first 
>> reg=%x\n",
> 
> This line is over 80... 

Yes indeed. Somehow checkpatch does not report this line, I don't know why.

> 
> If you're happy with these, I can make the changes, too; they're trivial.

Only the second one? Thanks :)

> 
>> +__func__, ret, reg);
>> +return ret;
>> +}
>> +
>> +return 0;
>> +}
> 

-- 
Best regards,
Todor Tomov

Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor

[Sakari Ailus]

Hi Todor,

On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote:
...
> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val)
> +{
> + u8 regbuf[3];
> + int ret;
> +
> + regbuf[0] = reg >> 8;
> + regbuf[1] = reg & 0xff;
> + regbuf[2] = val;
> +
> + ret = i2c_master_send(ov7251->i2c_client, regbuf, 3);
> + if (ret < 0) {
> + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n",
> + __func__, ret, reg, val);
> + return ret;
> + }
> +
> + return 0;

How about:

return ov7251_write_seq_regs(ov7251, reg, , 1);

And put the function below ov2751_write_seq_regs().

> +}
> +
> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val,
> +  u8 num)
> +{
> + const u8 maxregbuf = 5;
> + u8 regbuf[maxregbuf];
> + u8 nregbuf = sizeof(reg) + num * sizeof(*val);
> + int ret = 0;
> +
> + if (nregbuf > maxregbuf)
> + return -EINVAL;
> +
> + regbuf[0] = reg >> 8;
> + regbuf[1] = reg & 0xff;
> +
> + memcpy(regbuf + 2, val, num);
> +
> + ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf);
> + if (ret < 0) {
> + dev_err(ov7251->dev, "%s: write seq regs error %d: first 
> reg=%x\n",

This line is over 80... 

If you're happy with these, I can make the changes, too; they're trivial.

> + __func__, ret, reg);
> + return ret;
> + }
> +
> + return 0;
> +}

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH 1/5] dma-buf: use parameter structure for dma_buf_attach

[kbuild test robot]

Hi Christian,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.17-rc2 next-20180424]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Christian-K-nig/dma-buf-use-parameter-structure-for-dma_buf_attach/20180413-080639
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: openrisc-allyesconfig (attached as .config)
compiler: or1k-linux-gcc (GCC) 6.0.0 20160327 (experimental)
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=openrisc 

All errors (new ones prefixed by >>):

   drivers/staging//media/tegra-vde/tegra-vde.c: In function 
'tegra_vde_attach_dmabuf':
>> drivers/staging//media/tegra-vde/tegra-vde.c:534:13: error: 'dmabuf' 
>> undeclared (first use in this function)
  .dmabuf = dmabuf
^~
   drivers/staging//media/tegra-vde/tegra-vde.c:534:13: note: each undeclared 
identifier is reported only once for each function it appears in

vim +/dmabuf +534 drivers/staging//media/tegra-vde/tegra-vde.c

   521  
   522  static int tegra_vde_attach_dmabuf(struct device *dev,
   523 int fd,
   524 unsigned long offset,
   525 unsigned int min_size,
   526 struct dma_buf_attachment **a,
   527 dma_addr_t *addr,
   528 struct sg_table **s,
   529 size_t *size,
   530 enum dma_data_direction dma_dir)
   531  {
   532  struct dma_buf_attach_info attach_info = {
   533  .dev = dev,
 > 534  .dmabuf = dmabuf
   535  };
   536  struct dma_buf_attachment *attachment;
   537  struct dma_buf *dmabuf;
   538  struct sg_table *sgt;
   539  int err;
   540  
   541  dmabuf = dma_buf_get(fd);
   542  if (IS_ERR(dmabuf)) {
   543  dev_err(dev, "Invalid dmabuf FD\n");
   544  return PTR_ERR(dmabuf);
   545  }
   546  
   547  if ((u64)offset + min_size > dmabuf->size) {
   548  dev_err(dev, "Too small dmabuf size %zu @0x%lX, "
   549   "should be at least %d\n",
   550  dmabuf->size, offset, min_size);
   551  return -EINVAL;
   552  }
   553  
   554  attachment = dma_buf_attach(_info);
   555  if (IS_ERR(attachment)) {
   556  dev_err(dev, "Failed to attach dmabuf\n");
   557  err = PTR_ERR(attachment);
   558  goto err_put;
   559  }
   560  
   561  sgt = dma_buf_map_attachment(attachment, dma_dir);
   562  if (IS_ERR(sgt)) {
   563  dev_err(dev, "Failed to get dmabufs sg_table\n");
   564  err = PTR_ERR(sgt);
   565  goto err_detach;
   566  }
   567  
   568  if (sgt->nents != 1) {
   569  dev_err(dev, "Sparse DMA region is unsupported\n");
   570  err = -EINVAL;
   571  goto err_unmap;
   572  }
   573  
   574  *addr = sg_dma_address(sgt->sgl) + offset;
   575  *a = attachment;
   576  *s = sgt;
   577  
   578  if (size)
   579  *size = dmabuf->size - offset;
   580  
   581  return 0;
   582  
   583  err_unmap:
   584  dma_buf_unmap_attachment(attachment, sgt, dma_dir);
   585  err_detach:
   586  dma_buf_detach(dmabuf, attachment);
   587  err_put:
   588  dma_buf_put(dmabuf);
   589  
   590  return err;
   591  }
   592  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v6 00/12] Intel IPU3 ImgU patchset

[Sakari Ailus]

On Thu, Mar 29, 2018 at 09:14:48PM -0500, Yong Zhi wrote:
> Patch 7-10 are basically IPU3 CSS specific implementations:

The 7th patch is here (it didn't make to the list due to being too big):



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

Re: [PATCH v6 07/12] intel-ipu3: css: Add static settings for image pipeline

[Sakari Ailus]

On Wed, Apr 25, 2018 at 09:11:53AM +, Tomasz Figa wrote:
> Hi Yong,
> 
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi  wrote:
> 
> > This adds coeff, config parameters etc const definitions for
> > IPU3 programming.
> 
> > Signed-off-by: Yong Zhi 
> > ---
> >   drivers/media/pci/intel/ipu3/ipu3-tables.c | 9609
> 
> >   drivers/media/pci/intel/ipu3/ipu3-tables.h |   66 +
> >   2 files changed, 9675 insertions(+)
> >   create mode 100644 drivers/media/pci/intel/ipu3/ipu3-tables.c
> >   create mode 100644 drivers/media/pci/intel/ipu3/ipu3-tables.h
> 
> I believe none of the 6 revisions of this patch actually reached the
> mailing lists. It's too big. Could we do something about it? Splitting into
> smaller patches might help, but we should provide a link in cover letter to
> a public git branch where the whole series can be found applied to current
> Linux.

Well spotted. I've applied the patches on media tree master here:



The missing patch itself is here:



-- 
Regards,

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

[PATCH 3/3] [media] cxd2880: Changed version information

[Yasunari.Takiguchi]

From: Yasunari Takiguchi 

This is the version update for this cxd2880 driver changing. 

Signed-off-by: Yasunari Takiguchi 
Signed-off-by: Masayuki Yamamoto 
Signed-off-by: Hideki Nozawa 
Signed-off-by: Kota Yonezawa 
Signed-off-by: Toshihiko Matsumoto 
Signed-off-by: Satoshi Watanabe 
---

[Change list]
drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_driver_version.h
   -updated version information

 drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_driver_version.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git 
a/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_driver_version.h 
b/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_driver_version.h
index fab55038b37b..c6d6c8dd16a1 100644
--- a/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_driver_version.h
+++ b/drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_driver_version.h
@@ -7,6 +7,6 @@
  * Copyright (C) 2016, 2017, 2018 Sony Semiconductor Solutions Corporation
  */
 
-#define CXD2880_TNRDMD_DRIVER_VERSION "1.4.1 - 1.0.4"
+#define CXD2880_TNRDMD_DRIVER_VERSION "1.4.1 - 1.0.5"
 
-#define CXD2880_TNRDMD_DRIVER_RELEASE_DATE "2018-01-17"
+#define CXD2880_TNRDMD_DRIVER_RELEASE_DATE "2018-04-25"
-- 
2.15.1

Re: [PATCH 5/7] omapfb: omapfb_dss.h: add stubs to build with COMPILE_TEST && DRM_OMAP

[Tomi Valkeinen]

On 25/04/18 14:13, Bartlomiej Zolnierkiewicz wrote:
> 
> On Monday, April 23, 2018 05:11:14 PM Tomi Valkeinen wrote:
>> On 23/04/18 16:56, Bartlomiej Zolnierkiewicz wrote:
>>
>>> Ideally we should be able to build both drivers in the same kernel
>>> (especially as modules).
>>>
>>> I was hoping that it could be fixed easily but then I discovered
>>> the root source of the problem:
>>>
>>> drivers/gpu/drm/omapdrm/dss/display.o: In function 
>>> `omapdss_unregister_display':
>>> display.c:(.text+0x2c): multiple definition of `omapdss_unregister_display'
>>> drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198): 
>>> first defined here
>>
>> The main problem is that omapdrm and omapfb are two different drivers
>> for the same HW. You need to pick one, even if we would change those
>> functions and fix the link issue.
> 
> With proper resource allocation in both drivers this shouldn't be
> a problem - the one which allocates resources first will be used
> (+ we can initialize omapdrm first in case it is built-in). This is
> how similar situations are handled in other kernel subsystems.

We have boot time code, which is always built-in, for both drivers which
adjusts device tree data. I imagine those could easily conflict. Maybe
there's something else too.

And it's not only about the main omapfb or omapdrm driver. We have
drivers for the encoders and panels. Those would conflict too. I guess
we could have the case where omapdrm probes, and then a panel driver
from omapfb probes.

Actually, many of the panel and encoder drivers probably have same
symbols too, which would prevent linking.

> It seems that the real root problem is commit f76ee892a99e ("omapfb:
> copy omapdss & displays for omapfb") from Dec 2015 which resulted in
> duplication of ~30 KLOC of code. The code in question seems to be
> both fbdev & drm independent:
> 
> "
> * omapdss, located in drivers/video/fbdev/omap2/dss/. This is a driver 
> for the
>   display subsystem IPs used on OMAP (and related) SoCs. It offers only a
>   kernel internal API, and does not implement anything for fbdev or drm.
> 
> * omapdss panels and encoders, located in
>   drivers/video/fbdev/omap2/displays-new/. These are panel and external 
> encoder
>   drivers, which use APIs offered by omapdss driver. These also don't 
> implement
>   anything for fbdev or drm.
> "
> 
> While I understand some motives behind this change I'm not overall
> happy with it..

Neither was I, but I have to say it was a game-changer for omapdrm
development. Doing anything new on omapdrm meant trying to get it to
work on omapfb too (in the same commit, so cross-tree), and many changes
were just too complex to even try. After that commit we were free to
really start restructuring the code to fit the DRM world.

>> At some point in time we could compile both as modules (but not
>> built-in), but the only use for that was development/testing and in the
>> end made our life more difficult. So, now you must fully disable one of
>> them to enable the other. And, actually, we even have boot-time code,
>> not included in the module itself, which gets enabled when omapdrm is
>> enabled.
> 
> Do you mean some code in arch/arm/mach-omap2/ or something else?

That and the omapdss-boot-init.c we have for both omapfb and omapdrm.

>> While it's of course good to support COMPILE_TEST, if using COMPILE_TEST
>> with omapfb is problematic, I'm not sure if it's worth to spend time on
>> that. We should be moving away from omapfb to omapdrm.
> 
> Is there some approximate schedule for omapfb removal available?

Unfortunatey not. omapfb still has support for some legacy devices that
omapdrm doesn't support.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/3] [media] cxd2880:Optimized spi drive current and BER/PER set/get condition

[Yasunari.Takiguchi]

From: Yasunari Takiguchi 

This is the optimization for SPI drive current and 
signal lock condition check part for BER/PER measure
to ensure BER/PER are stable

Signed-off-by: Yasunari Takiguchi 
Signed-off-by: Masayuki Yamamoto 
Signed-off-by: Hideki Nozawa 
Signed-off-by: Kota Yonezawa 
Signed-off-by: Toshihiko Matsumoto 
Signed-off-by: Satoshi Watanabe 
---

[Change list]
   drivers/media/dvb-frontends/cxd2880/cxd2880_top.c
  -reduced the SPI output drive current
  -optimized signal lock condition check part for BER/PER measure
   to ensure BER/PER are stable

 drivers/media/dvb-frontends/cxd2880/cxd2880_top.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/cxd2880/cxd2880_top.c 
b/drivers/media/dvb-frontends/cxd2880/cxd2880_top.c
index d474dc1b05da..bd9101e246d5 100644
--- a/drivers/media/dvb-frontends/cxd2880/cxd2880_top.c
+++ b/drivers/media/dvb-frontends/cxd2880/cxd2880_top.c
@@ -520,6 +520,15 @@ static int cxd2880_init(struct dvb_frontend *fe)
pr_err("cxd2880 integ init failed %d\n", ret);
return ret;
}
+
+   ret = cxd2880_tnrdmd_set_cfg(>tnrdmd,
+CXD2880_TNRDMD_CFG_TSPIN_CURRENT,
+0x00);
+   if (ret) {
+   mutex_unlock(priv->spi_mutex);
+   pr_err("cxd2880 set config failed %d\n", ret);
+   return ret;
+   }
mutex_unlock(priv->spi_mutex);
 
pr_debug("OK.\n");
@@ -1126,7 +1135,7 @@ static int cxd2880_get_stats(struct dvb_frontend *fe,
priv = fe->demodulator_priv;
c = >dtv_property_cache;
 
-   if (!(status & FE_HAS_LOCK)) {
+   if (!(status & FE_HAS_LOCK) || !(status & FE_HAS_CARRIER)) {
c->pre_bit_error.len = 1;
c->pre_bit_error.stat[0].scale = FE_SCALE_NOT_AVAILABLE;
c->pre_bit_count.len = 1;
@@ -1345,7 +1354,8 @@ static int cxd2880_read_status(struct dvb_frontend *fe,
 
pr_debug("status %d\n", *status);
 
-   if (priv->s == 0 && (*status & FE_HAS_LOCK)) {
+   if (priv->s == 0 && (*status & FE_HAS_LOCK) &&
+   (*status & FE_HAS_CARRIER)) {
mutex_lock(priv->spi_mutex);
if (c->delivery_system == SYS_DVBT) {
ret = cxd2880_set_ber_per_period_t(fe);
-- 
2.15.1

[PATCH 1/3] [media] cxd2880-spi: Modified how to declare structure

[Yasunari.Takiguchi]

From: Yasunari Takiguchi 

This is the modification of structure declaration for spi_transfer. 

Signed-off-by: Yasunari Takiguchi 
Signed-off-by: Masayuki Yamamoto 
Signed-off-by: Hideki Nozawa 
Signed-off-by: Kota Yonezawa 
Signed-off-by: Toshihiko Matsumoto 
Signed-off-by: Satoshi Watanabe 
---
[Change list]
   drivers/media/spi/cxd2880-spi.c
  -modified how to declare spi_transfer structure

 drivers/media/spi/cxd2880-spi.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/spi/cxd2880-spi.c b/drivers/media/spi/cxd2880-spi.c
index 4df3bd312f48..754940f7e964 100644
--- a/drivers/media/spi/cxd2880-spi.c
+++ b/drivers/media/spi/cxd2880-spi.c
@@ -60,14 +60,13 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 static int cxd2880_write_spi(struct spi_device *spi, u8 *data, u32 size)
 {
struct spi_message msg;
-   struct spi_transfer tx;
+   struct spi_transfer tx = {};
 
if (!spi || !data) {
pr_err("invalid arg\n");
return -EINVAL;
}
 
-   memset(, 0, sizeof(tx));
tx.tx_buf = data;
tx.len = size;
 
@@ -130,7 +129,7 @@ static int cxd2880_spi_read_ts(struct spi_device *spi,
int ret;
u8 data[3];
struct spi_message message;
-   struct spi_transfer transfer[2];
+   struct spi_transfer transfer[2] = {};
 
if (!spi || !read_data || !packet_num) {
pr_err("invalid arg\n");
@@ -146,7 +145,6 @@ static int cxd2880_spi_read_ts(struct spi_device *spi,
data[2] = packet_num;
 
spi_message_init();
-   memset(transfer, 0, sizeof(transfer));
 
transfer[0].len = 3;
transfer[0].tx_buf = data;
@@ -383,7 +381,7 @@ static int cxd2880_start_feed(struct dvb_demux_feed *feed)
}
}
if (i == CXD2880_MAX_FILTER_SIZE) {
-   pr_err("PID filter is full. Assumed bug.\n");
+   pr_err("PID filter is full.\n");
return -EINVAL;
}
if (!dvb_spi->all_pid_feed_count)
-- 
2.15.1

[PATCH 0/3] [media] cxd2880: modified structure declaration and optimized the driver

[Yasunari.Takiguchi]

From: Yasunari Takiguchi 

Hi,

This is the update patch for patch Sony CXD2880 DVB-T2/T tuner + 
demodulator driver.

We modified how to declare structure and
optimized spi drive current and 
signal lock condition check part for BER/PER measure
to ensure BER/PER are stable.

The change history of this patch series is as below.

[Change list]
(1)The detail change items of each files are as below.
[PATCH 1/3]
   drivers/media/spi/cxd2880-spi.c
 -modified how to declare spi_transfer structure

[PATCH 2/3]
   drivers/media/dvb-frontends/cxd2880/cxd2880_top.c
 -reduced the SPI output drive current
 -optimized signal lock condition check part for BER/PER measure
  to ensure BER/PER are stable

[PATCH 3/3]
   drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_driver_version.h
  -updated version information

Thanks,
Takiguchi
---
 drivers/media/spi/cxd2880-spi.c | 8 
+++-
 drivers/media/dvb-frontends/cxd2880/cxd2880_top.c   | 14 
--
 drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_driver_version.h | 4 ++--

 3 files changed, 17 insertions(+), 9 deletions(-)

2.15.1

Re: [PATCH v8 09/13] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 04/25/2018 10:11 PM, Ezequiel Garcia wrote:
> On 14 March 2018 at 12:55, Hans Verkuil  wrote:
>> On 03/09/2018 09:49 AM, Gustavo Padovan wrote:
>>> From: Gustavo Padovan 
>>>
>>> Receive in-fence from userspace and add support for waiting on them
>>> before queueing the buffer to the driver. Buffers can't be queued to the
>>> driver before its fences signal. And a buffer can't be queue to the driver
>>
>> queue -> queued
>>
>>> out of the order they were queued from userspace. That means that even if
>>> it fence signal it must wait all other buffers, ahead of it in the queue,
>>
>> it fence signal -> its fence signals
>> wait all -> wait for all
>>
>>> to signal first.
>>>
>>> If the fence for some buffer fails we do not queue it to the driver,
>>> instead we mark it as error and wait until the previous buffer is done
>>> to notify userspace of the error. We wait here to deliver the buffers back
>>> to userspace in order.
>>>
>>> v9:   - rename fence to in_fence in many places
>>>   - handle fences signalling with error better (Hans Verkuil)
>>>
>>> v8:   - improve comments and docs (Hans Verkuil)
>>>   - fix unlocking of vb->fence_cb_lock on vb2_core_qbuf (Hans Verkuil)
>>>   - move in-fences code that was in the out-fences patch here (Alex)
>>>
>>> v8:   - improve comments about fences with errors
>>
>> v9? Two v8 entries?
>>
>>>
>>> v7:
>>>   - get rid of the fence array stuff for ordering and just use
>>>   get_num_buffers_ready() (Hans)
>>>   - fix issue of queuing the buffer twice (Hans)
>>>   - avoid the dma_fence_wait() in core_qbuf() (Alex)
>>>   - merge preparation commit in
>>>
>>> v6:
>>>   - With fences always keep the order userspace queues the buffers.
>>>   - Protect in_fence manipulation with a lock (Brian Starkey)
>>>   - check if fences have the same context before adding a fence array
>>>   - Fix last_fence ref unbalance in __set_in_fence() (Brian Starkey)
>>>   - Clean up fence if __set_in_fence() fails (Brian Starkey)
>>>   - treat -EINVAL from dma_fence_add_callback() (Brian Starkey)
>>>
>>> v5:   - use fence_array to keep buffers ordered in vb2 core when
>>>   needed (Brian Starkey)
>>>   - keep backward compat on the reserved2 field (Brian Starkey)
>>>   - protect fence callback removal with lock (Brian Starkey)
>>>
>>> v4:
>>>   - Add a comment about dma_fence_add_callback() not returning a
>>>   error (Hans)
>>>   - Call dma_fence_put(vb->in_fence) if fence signaled (Hans)
>>>   - select SYNC_FILE under config VIDEOBUF2_CORE (Hans)
>>>   - Move dma_fence_is_signaled() check to __enqueue_in_driver() (Hans)
>>>   - Remove list_for_each_entry() in __vb2_core_qbuf() (Hans)
>>>   -  Remove if (vb->state != VB2_BUF_STATE_QUEUED) from
>>>   vb2_start_streaming() (Hans)
>>>   - set IN_FENCE flags on __fill_v4l2_buffer (Hans)
>>>   - Queue buffers to the driver as soon as they are ready (Hans)
>>>   - call fill_user_buffer() after queuing the buffer (Hans)
>>>   - add err: label to clean up fence
>>>   - add dma_fence_wait() before calling vb2_start_streaming()
>>>
>>> v3:   - document fence parameter
>>>   - remove ternary if at vb2_qbuf() return (Mauro)
>>>   - do not change if conditions behaviour (Mauro)
>>>
>>> v2:
>>>   - fix vb2_queue_or_prepare_buf() ret check
>>>   - remove check for VB2_MEMORY_DMABUF only (Javier)
>>>   - check num of ready buffers to start streaming
>>>   - when queueing, start from the first ready buffer
>>>   - handle queue cancel
>>>
>>> Signed-off-by: Gustavo Padovan 
>>> ---
>>>  drivers/media/common/videobuf2/videobuf2-core.c | 197 
>>> 
>>>  drivers/media/common/videobuf2/videobuf2-v4l2.c |  34 +++-
>>>  drivers/media/v4l2-core/Kconfig |  33 
>>>  include/media/videobuf2-core.h  |  14 +-
>>>  4 files changed, 248 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
>>> b/drivers/media/common/videobuf2/videobuf2-core.c
>>> index d3f7bb33a54d..5de5e35cfc40 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>> @@ -352,6 +352,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
>>> vb2_memory memory,
>>>   vb->index = q->num_buffers + buffer;
>>>   vb->type = q->type;
>>>   vb->memory = memory;
>>> + spin_lock_init(>fence_cb_lock);
>>>   for (plane = 0; plane < num_planes; ++plane) {
>>>   vb->planes[plane].length = plane_sizes[plane];
>>>   vb->planes[plane].min_length = plane_sizes[plane];
>>> @@ -891,20 +892,12 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, 
>>> unsigned int plane_no)
>>>  }
>>>  EXPORT_SYMBOL_GPL(vb2_plane_cookie);
>>>

Re: [PATCH] rcar-vin: remove generic gen3 compatible string

[Simon Horman]

On Wed, Apr 25, 2018 at 01:43:21AM +0200, Niklas Söderlund wrote:
> The compatible string "renesas,rcar-gen3-vin" was added before the
> Gen3 driver code was added but it's not possible to use. Each SoC in the
> Gen3 series require SoC specific knowledge in the driver to function.
> Remove it before it is added to any device tree descriptions.
> 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Simon Horman 

> ---
>  Documentation/devicetree/bindings/media/rcar_vin.txt | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt 
> b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index ba31431d4b1fbdbb..a19517e1c669eb35 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -24,7 +24,6 @@ on Gen3 platforms to a CSI-2 receiver.
> - "renesas,vin-r8a77970" for the R8A77970 device
> - "renesas,rcar-gen2-vin" for a generic R-Car Gen2 or RZ/G1 compatible
>   device.
> -   - "renesas,rcar-gen3-vin" for a generic R-Car Gen3 compatible device.
>  
> When compatible with the generic version nodes must list the
> SoC-specific version corresponding to the platform first
> -- 
> 2.17.0
> 

Re: [PATCH 2/2] ARM: dts: r8a7740: Enable CEU0

[Simon Horman]

Thanks Jacopo,

I'm very pleased to see this series.

On Wed, Apr 25, 2018 at 01:15:20PM +0200, Jacopo Mondi wrote:
> Enable CEU0 peripheral for Renesas R-Mobile A1 R8A7740.

Given 'status = "disabled"' below I think you
are describing but not enabling CEU0. Also in the subject.

Should we also describe CEU1?

> 
> Signed-off-by: Jacopo Mondi 
> ---
>  arch/arm/boot/dts/r8a7740.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r8a7740.dtsi b/arch/arm/boot/dts/r8a7740.dtsi
> index afd3bc5..05ec41e 100644
> --- a/arch/arm/boot/dts/r8a7740.dtsi
> +++ b/arch/arm/boot/dts/r8a7740.dtsi
> @@ -67,6 +67,16 @@
>   power-domains = <_d4>;
>   };
>  
> + ceu0: ceu@fe91 {
> + reg = <0xfe91 0x100>;

Should the size of the range be 0x3000 ?
That would seem to match my reading of table 32.3
and also be consistent with r7s72100.dtsi.

> + compatible = "renesas,r8a7740-ceu";
> + interrupts = ;
> + clocks = <_clks R8A7740_CLK_CEU20>;
> + clock-names = "ceu20";
> + power-domains = <_a4mp>;

My reading of table 1.7 is that the power domain should be A4R (_a4r).

> + status = "disabled";
> + };
> +
>   cmt1: timer@e6138000 {
>   compatible = "renesas,cmt-48-r8a7740", "renesas,cmt-48";
>   reg = <0xe6138000 0x170>;
> -- 
> 2.7.4
> 

Re: [PATCH] rcar-vin: fix null pointer dereference in rvin_group_get()

[Simon Horman]

On Wed, Apr 25, 2018 at 01:45:06AM +0200, Niklas Söderlund wrote:
> Store the group pointer before disassociating the VIN from the group.
> 
> Fixes: 3bb4c3bc85bf77a7 ("media: rcar-vin: add group allocator functions")
> Reported-by: Colin Ian King 
> Signed-off-by: Niklas Söderlund 
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index 7bc2774a11232362..d3072e166a1ca24f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -338,19 +338,21 @@ static int rvin_group_get(struct rvin_dev *vin)
>  
>  static void rvin_group_put(struct rvin_dev *vin)
>  {
> - mutex_lock(>group->lock);
> + struct rvin_group *group = vin->group;
> +
> + mutex_lock(>lock);

Hi Niklas, its not clear to me why moving the lock is safe.
Could you explain the locking scheme a little?

>  
>   vin->group = NULL;
>   vin->v4l2_dev.mdev = NULL;
>  
> - if (WARN_ON(vin->group->vin[vin->id] != vin))
> + if (WARN_ON(group->vin[vin->id] != vin))
>   goto out;
>  
> - vin->group->vin[vin->id] = NULL;
> + group->vin[vin->id] = NULL;
>  out:
> - mutex_unlock(>group->lock);
> + mutex_unlock(>lock);
>  
> - kref_put(>group->refcount, rvin_group_release);
> + kref_put(>refcount, rvin_group_release);
>  }
>  
>  /* 
> -
> -- 
> 2.17.0
>