Re: [PATCH v2 3/5] mfd: cros-ec: Introduce CEC commands and events definitions.

[Benson Leung]

Hi Neil, Hi Stefan,

On Fri, May 18, 2018 at 03:05:02PM +0200, Neil Armstrong wrote:
> The EC can expose a CEC bus, this patch adds the CEC related definitions
> needed by the cros-ec-cec driver.
> Having a 16 byte mkbp event size makes it possible to send CEC
> messages from the EC to the AP directly inside the mkbp event
> instead of first doing a notification and then a read.
> 
> Signed-off-by: Stefan Adolfsson 
> Signed-off-by: Neil Armstrong 

It looks like this change squashes together this chromeos-4.4 CL
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1061879
and some other new changes to add cec to cros_ec_commands.

Could you separate the two? One patch that's as close to Stefan's
which introduces the 16 byte mkbp, and a second one that
adds the HDMI CEC commands? 

Thanks,
Benson
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 40 +
>  include/linux/mfd/cros_ec.h |  2 +-
>  include/linux/mfd/cros_ec_commands.h| 80 
> +
>  3 files changed, 112 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c 
> b/drivers/platform/chrome/cros_ec_proto.c
> index e7bbdf9..c4f6c44 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -504,10 +504,31 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device 
> *ec_dev,
>  }
>  EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
>  
> +static int get_next_event_xfer(struct cros_ec_device *ec_dev,
> +struct cros_ec_command *msg,
> +int version, uint32_t size)
> +{
> + int ret;
> +
> + msg->version = version;
> + msg->command = EC_CMD_GET_NEXT_EVENT;
> + msg->insize = size;
> + msg->outsize = 0;
> +
> + ret = cros_ec_cmd_xfer(ec_dev, msg);
> + if (ret > 0) {
> + ec_dev->event_size = ret - 1;
> + memcpy(_dev->event_data, msg->data, ec_dev->event_size);
> + }
> +
> + return ret;
> +}
> +
>  static int get_next_event(struct cros_ec_device *ec_dev)
>  {
>   u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
>   struct cros_ec_command *msg = (struct cros_ec_command *)
> + static int cmd_version = 1;
>   int ret;
>  
>   if (ec_dev->suspended) {
> @@ -515,18 +536,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
>   return -EHOSTDOWN;
>   }
>  
> - msg->version = 0;
> - msg->command = EC_CMD_GET_NEXT_EVENT;
> - msg->insize = sizeof(ec_dev->event_data);
> - msg->outsize = 0;
> + if (cmd_version == 1) {
> + ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> + sizeof(struct ec_response_get_next_event_v1));
> + if (ret < 0 || msg->result != EC_RES_INVALID_VERSION)
> + return ret;
>  
> - ret = cros_ec_cmd_xfer(ec_dev, msg);
> - if (ret > 0) {
> - ec_dev->event_size = ret - 1;
> - memcpy(_dev->event_data, msg->data,
> -sizeof(ec_dev->event_data));
> + /* Fallback to version 0 for future send attempts */
> + cmd_version = 0;
>   }
>  
> + ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> +   sizeof(struct ec_response_get_next_event));
> +
>   return ret;
>  }
>  
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index f36125e..32caef3 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -147,7 +147,7 @@ struct cros_ec_device {
>   bool mkbp_event_supported;
>   struct blocking_notifier_head event_notifier;
>  
> - struct ec_response_get_next_event event_data;
> + struct ec_response_get_next_event_v1 event_data;
>   int event_size;
>   u32 host_event_wake_mask;
>  };
> diff --git a/include/linux/mfd/cros_ec_commands.h 
> b/include/linux/mfd/cros_ec_commands.h
> index f2edd99..16c3a2b 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -804,6 +804,8 @@ enum ec_feature_code {
>   EC_FEATURE_MOTION_SENSE_FIFO = 24,
>   /* EC has RTC feature that can be controlled by host commands */
>   EC_FEATURE_RTC = 27,
> + /* EC supports CEC commands */
> + EC_FEATURE_CEC = 35,
>  };
>  
>  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> @@ -2078,6 +2080,12 @@ enum ec_mkbp_event {
>   /* EC sent a sysrq command */
>   EC_MKBP_EVENT_SYSRQ = 6,
>  
> + /* Notify the AP that something happened on CEC */
> + EC_MKBP_CEC_EVENT = 8,
> +
> + /* Send an incoming CEC message to the AP */
> + EC_MKBP_EVENT_CEC_MESSAGE = 9,
> +
>   /* Number of MKBP events */
>   EC_MKBP_EVENT_COUNT,
>  };
> @@ -2093,12 +2101,31 @@ union ec_response_get_next_data {
>   uint32_t   sysrq;
>  } __packed;
>  
> +union 

Re: [PATCHv13 22/28] Documentation: v4l: document request API

[Tomasz Figa]

Hi Laurent,

Thanks for detailed review. Please let me add my thoughts inline as well.

On Fri, May 18, 2018 at 11:46 PM Laurent Pinchart <
laurent.pinch...@ideasonboard.com> wrote:

> Hi Hans,

> Thank you for the patch.

> On Thursday, 3 May 2018 17:53:12 EEST Hans Verkuil wrote:

[snip]

> > +Name
> > +
> > +
> > +MEDIA_REQUEST_IOC_REINIT - Re-initialize a request
> > +
> > +
> > +Synopsis
> > +
> > +
> > +.. c:function:: int ioctl( int request_fd, MEDIA_REQUEST_IOC_REINIT )
> > +:name: MEDIA_REQUEST_IOC_REINIT
> > +
> > +
> > +Arguments
> > +=
> > +
> > +``request_fd``
> > +File descriptor returned by :ref:`MEDIA_IOC_REQUEST_ALLOC`.

> I was expecting an argument to this ioctl, with a reference to another
request
> (and I expected the same reference for the MEDIA_IOC_REQUEST_ALLOC
ioctl). I
> think this topic has been discussed several times in the past, but
possibly
> not in venues where everybody was present, so I'll detail the problem
here.

> The idea behind this version of the request API is to split request
handling
> in three parts:

> 1. A request is allocated (MEDIA_IOC_REQUEST_ALLOC) or an existing
request is
> re-initialized (MEDIA_REQUEST_IOC_REINIT). At that point the request
contains
> no property (control, format, ...) to be modified.

> 2. The request is filled with properties to be modified using existing
V4L2
> ioctls (VIDIOC_S_EXT_CTRLS, VIDIOC_SUBDEV_S_FMT,
VIDIOC_SUBDEV_S_SELECTION,
> ..). The current implementation supports controls only, with support for
> formats and selection rectangles to be added later.

> 3. The request is queued with MEDIA_REQUEST_IOC_QUEUE.

> As usual the devil is in the details, and the details of the second step
in
> particular.

> The idea behind splitting request handling in those three steps is to
reuse
> existing V4L2 ioctls as much as possible, and to keep the existing
semantics
> of those ioctls. I believe this isn't possible with the proposed
> MEDIA_IOC_REQUEST_ALLOC and MEDIA_REQUEST_IOC_REINIT ioctls, neither for
> controls nor for formats or selection rectangles, although the problem is
more
> limited for controls. I will thus take formats and selection rectangles to
> explain where the problem lies.

> The S_FMT and S_SELECTION semantics on subdevs (and, for that matter, on
video
> nodes too) is that those ioctls don't fail when passed parameters invalid
for
> the device, but return a valid configuration as close as possible to the
> requested one. This allows implementing negotiation of formats and
selection
> rectangles through an iterative process that involves back-and-forth calls
> between user space and kernel space until the application finds a usable
> configuration accepted by the driver, or until it decides to fail because
such
> a configuration doesn't exist. (As a reminder, such negotiation is needed
> because it was deemed impossible to create a proper API that would expose
all
> device constraints in a static way.)

> Formats and selection rectangles are propagated within subdevs from sink
pads
> to source pads by the driver. Sink formats and selection rectangles thus
> influence the result of negotiation on source pads. For instance, on a
scaler
> subdev that can't perform format conversion, the format on the source pad
will
> be dictated by the format on the sink pad. In order to implement S_FMT on
the
> source pad the kernel driver thus needs context information to get the
format
> on the sink pad.

> Without the request API this is easy. The context is either stored in the
> driver-specific device structure (for active formats) or in the file
handle
> (for try formats). With the proposed request API, however, we have no such
> context: the requests are created empty (or re-initialized to be empty).
> Kernel drivers can access the active state of the device, or the state of
> previously queued requests, but can't access the state of prepared but
not yet
> queued requests.

> My proposal to solve this was to link requests by specifying the handle
of a
> previous request to the MEDIA_IOC_REQUEST_ALLOC and
MEDIA_REQUEST_IOC_REINIT
> ioctls. That way kernel drivers would know in which order requests will be
> queued, and would be able to access the correct context information to
> implement the existing semantics of the S_FMT and S_SELECTION ioctls (as
well
> as being able to implement the G_FMT and G_SELECTION ioctls). Note that
the
> problem isn't limited to format and selection rectangles, similar
problems can
> exist when multiple controls are related, such as auto and manual
controls.

> This would result in a more complex API and a more complex request
lifetime
> management, as well as a more complex semantics of the request API. While
I
> thought this was unavoidable, I came to reconsider my position on this
topic,
> and I believe that the current request API proposal is acceptable in that
> regard, provided that we agree that the existing semantics of the S_FMT
and
> S_SELECTION ioctls won't 

cron job: media_tree daily build: WARNINGS

[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:   Thu May 24 05:00:17 CEST 2018
media-tree git hash:8ed8bba70b4355b1ba029b151ade84475dd12991
media_build git hash:   6d922b94bf0fcc9f0a1e52370b7d1d4fc2a59f8d
v4l-utils git hash: 2a12796b5c22cd1a549eb8fa25db873ced811ca5
gcc version:i686-linux-gcc (GCC) 8.1.0
sparse version: 0.5.2-RC1
smatch version: 0.5.1
host hardware:  x86_64
host os:4.16.0-1-amd64

linux-git-arm-at91: WARNINGS
linux-git-arm-davinci: OK
linux-git-arm-multi: WARNINGS
linux-git-arm-pxa: WARNINGS
linux-git-arm-stm32: OK
linux-git-arm64: WARNINGS
linux-git-i686: WARNINGS
linux-git-mips: OK
linux-git-powerpc64: WARNINGS
linux-git-sh: OK
linux-git-x86_64: WARNINGS
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: WARNINGS
linux-4.8.17-i686: OK
linux-4.8.17-x86_64: WARNINGS
linux-4.9.91-i686: OK
linux-4.9.91-x86_64: WARNINGS
linux-4.10.17-i686: OK
linux-4.10.17-x86_64: WARNINGS
linux-4.11.12-i686: OK
linux-4.11.12-x86_64: WARNINGS
linux-4.12.14-i686: OK
linux-4.12.14-x86_64: WARNINGS
linux-4.13.16-i686: WARNINGS
linux-4.13.16-x86_64: WARNINGS
linux-4.14.42-i686: WARNINGS
linux-4.14.42-x86_64: WARNINGS
linux-4.15.14-i686: WARNINGS
linux-4.15.14-x86_64: WARNINGS
linux-4.16.8-i686: WARNINGS
linux-4.16.8-x86_64: WARNINGS
linux-4.17-rc4-i686: WARNINGS
linux-4.17-rc4-x86_64: WARNINGS
apps: OK
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

Re: [PATCH v3 2/9] media: rcar-vin: Remove two empty lines

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2018-05-18 16:40:38 +0200, Jacopo Mondi wrote:
> Remove un-necessary empty lines.
> 
> Signed-off-by: Jacopo Mondi 

Acked-by: Niklas Söderlund 

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index 6b80f98..1aadd90 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -707,11 +707,9 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>   return -EINVAL;
>  
>   if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> -
>   vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
>   to_of_node(asd->match.fwnode));
>   return -ENOTCONN;
> -
>   }
>  
>   if (vin->group->csi[vep->base.id].fwnode) {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 1/9] media: rcar-vin: Rename 'digital' to 'parallel'

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:37 +0200, Jacopo Mondi wrote:
> As the term 'digital' is used all over the rcar-vin code in place of
> 'parallel', rename all the occurrencies.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 72 
> ++---
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  4 +-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++---
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  6 +--
>  4 files changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index d3072e1..6b80f98 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -376,12 +376,12 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int 
> direction)
>  }
>  
>  /* 
> -
> - * Digital async notifier
> + * Parallel async notifier
>   */
>  
>  /* The vin lock should be held when calling the subdevice attach and detach 
> */
> -static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
> -  struct v4l2_subdev *subdev)
> +static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> +   struct v4l2_subdev *subdev)
>  {
>   struct v4l2_subdev_mbus_code_enum code = {
>   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> @@ -392,15 +392,15 @@ static int rvin_digital_subdevice_attach(struct 
> rvin_dev *vin,
>   ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
>   if (ret < 0)
>   return ret;
> - vin->digital->source_pad = ret;
> + vin->parallel->source_pad = ret;
>  
>   ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> - vin->digital->sink_pad = ret < 0 ? 0 : ret;
> + vin->parallel->sink_pad = ret < 0 ? 0 : ret;
>  
>   /* Find compatible subdevices mbus format */
>   vin->mbus_code = 0;
>   code.index = 0;
> - code.pad = vin->digital->source_pad;
> + code.pad = vin->parallel->source_pad;
>   while (!vin->mbus_code &&
>  !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, )) {
>   code.index++;
> @@ -450,21 +450,21 @@ static int rvin_digital_subdevice_attach(struct 
> rvin_dev *vin,
>  
>   vin->vdev.ctrl_handler = >ctrl_handler;
>  
> - vin->digital->subdev = subdev;
> + vin->parallel->subdev = subdev;
>  
>   return 0;
>  }
>  
> -static void rvin_digital_subdevice_detach(struct rvin_dev *vin)
> +static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  {
>   rvin_v4l2_unregister(vin);
>   v4l2_ctrl_handler_free(>ctrl_handler);
>  
>   vin->vdev.ctrl_handler = NULL;
> - vin->digital->subdev = NULL;
> + vin->parallel->subdev = NULL;
>  }
>  
> -static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> +static int rvin_parallel_notify_complete(struct v4l2_async_notifier 
> *notifier)
>  {
>   struct rvin_dev *vin = notifier_to_vin(notifier);
>   int ret;
> @@ -478,28 +478,28 @@ static int rvin_digital_notify_complete(struct 
> v4l2_async_notifier *notifier)
>   return rvin_v4l2_register(vin);
>  }
>  
> -static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> -struct v4l2_subdev *subdev,
> -struct v4l2_async_subdev *asd)
> +static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)

When I run my indentation script this indentation changes from spaces to 
all tabs. If possible I would like to keep that as I usually run it on 
these files before submitting any patches, but it's not a big deal.

Whit this fixed, thanks for clearing this up!

Acked-by: Niklas Söderlund 

>  {
>   struct rvin_dev *vin = notifier_to_vin(notifier);
>  
> - vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> + vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
>  
>   mutex_lock(>lock);
> - rvin_digital_subdevice_detach(vin);
> + rvin_parallel_subdevice_detach(vin);
>   mutex_unlock(>lock);
>  }
>  
> -static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> -  struct v4l2_subdev *subdev,
> -  struct v4l2_async_subdev *asd)
> +static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> +   struct v4l2_subdev *subdev,
> +   struct v4l2_async_subdev *asd)
>  {
>   struct rvin_dev *vin = notifier_to_vin(notifier);
>   int ret;
>  
>   mutex_lock(>lock);
> - 

Re: [PATCH 05/15] mtd: nand: pxa3xx: remove the dmaengine compat need

[Daniel Mack]

Hi Robert,

Please refer to the attached patch instead of the one I sent earlier. I 
missed to also remove the platform_get_resource(IORESOURCE_DMA) call.



Thanks,
Daniel


On Friday, May 18, 2018 11:31 PM, Daniel Mack wrote:

Hi Robert,

Thanks for this series.

On Monday, April 02, 2018 04:26 PM, Robert Jarzmik wrote:

From: Robert Jarzmik 

As the pxa architecture switched towards the dmaengine slave map, the
old compatibility mechanism to acquire the dma requestor line number and
priority are not needed anymore.

This patch simplifies the dma resource acquisition, using the more
generic function dma_request_slave_channel().

Signed-off-by: Robert Jarzmik 
---
   drivers/mtd/nand/pxa3xx_nand.c | 10 +-


This driver was replaced by drivers/mtd/nand/raw/marvell_nand.c
recently, so this patch can be dropped. I attached a version for the new
driver which you can pick instead.


Thanks,
Daniel



>From 72a306157dedb21f8c3289f0f7a288fc4542bd96 Mon Sep 17 00:00:00 2001
From: Daniel Mack 
Date: Sat, 12 May 2018 21:50:13 +0200
Subject: [PATCH] mtd: rawnand: marvell: remove dmaengine compat code

As the pxa architecture switched towards the dmaengine slave map, the
old compatibility mechanism to acquire the dma requestor line number and
priority are not needed anymore.

This patch simplifies the dma resource acquisition, using the more
generic function dma_request_slave_channel().

Signed-off-by: Daniel Mack 
---
 drivers/mtd/nand/raw/marvell_nand.c | 17 +
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index ebb1d141b900..319fea77daf1 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2612,8 +2612,6 @@ static int marvell_nfc_init_dma(struct marvell_nfc *nfc)
 		dev);
 	struct dma_slave_config config = {};
 	struct resource *r;
-	dma_cap_mask_t mask;
-	struct pxad_param param;
 	int ret;
 
 	if (!IS_ENABLED(CONFIG_PXA_DMA)) {
@@ -2626,20 +2624,7 @@ static int marvell_nfc_init_dma(struct marvell_nfc *nfc)
 	if (ret)
 		return ret;
 
-	r = platform_get_resource(pdev, IORESOURCE_DMA, 0);
-	if (!r) {
-		dev_err(nfc->dev, "No resource defined for data DMA\n");
-		return -ENXIO;
-	}
-
-	param.drcmr = r->start;
-	param.prio = PXAD_PRIO_LOWEST;
-	dma_cap_zero(mask);
-	dma_cap_set(DMA_SLAVE, mask);
-	nfc->dma_chan =
-		dma_request_slave_channel_compat(mask, pxad_filter_fn,
-		 , nfc->dev,
-		 "data");
+	nfc->dma_chan = dma_request_slave_channel(nfc->dev, "data");
 	if (!nfc->dma_chan) {
 		dev_err(nfc->dev,
 			"Unable to request data DMA channel\n");
-- 
2.14.3

Re: [PATCH 06/20] omap4iss: Add video_device and vb2_queue locks

[Ezequiel Garcia]

On Tue, 2018-05-22 at 11:09 +0200, Hans Verkuil wrote:
> On 18/05/18 20:51, Ezequiel Garcia wrote:
> > video_device and vb2_queue locks are now both
> > mandatory. Add them, remove driver ad-hoc locks,
> > and implement wait_{prepare, finish}.
> > 
> > To stay on the safe side, this commit uses a single mutex
> > for both locks. Better latency can be obtained by separating
> > these if needed.
> > 
> > Signed-off-by: Ezequiel Garcia 
> > ---
> >  drivers/staging/media/omap4iss/iss_video.c | 32 
> > +++---
> >  drivers/staging/media/omap4iss/iss_video.h |  2 +-
> >  2 files changed, 8 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/staging/media/omap4iss/iss_video.c 
> > b/drivers/staging/media/omap4iss/iss_video.c
> > index a3a83424a926..380cfd230262 100644
> > --- a/drivers/staging/media/omap4iss/iss_video.c
> > +++ b/drivers/staging/media/omap4iss/iss_video.c
> > @@ -260,10 +260,7 @@ __iss_video_get_format(struct iss_video *video,
> > fmt.pad = pad;
> > fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >  
> > -   mutex_lock(>mutex);
> > ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, );
> > -   mutex_unlock(>mutex);
> > -
> > if (ret)
> > return ret;
> >  
> > @@ -411,6 +408,8 @@ static const struct vb2_ops iss_video_vb2ops = {
> > .buf_prepare= iss_video_buf_prepare,
> > .buf_queue  = iss_video_buf_queue,
> > .buf_cleanup= iss_video_buf_cleanup,
> > +   .wait_prepare   = vb2_ops_wait_prepare,
> > +   .wait_finish= vb2_ops_wait_finish,
> >  };
> >  
> >  /*
> > @@ -592,9 +591,7 @@ iss_video_get_format(struct file *file, void *fh, 
> > struct v4l2_format *format)
> > if (format->type != video->type)
> > return -EINVAL;
> >  
> > -   mutex_lock(>mutex);
> > *format = vfh->format;
> > -   mutex_unlock(>mutex);
> >  
> > return 0;
> >  }
> > @@ -609,8 +606,6 @@ iss_video_set_format(struct file *file, void *fh, 
> > struct v4l2_format *format)
> > if (format->type != video->type)
> > return -EINVAL;
> >  
> > -   mutex_lock(>mutex);
> > -
> > /*
> >  * Fill the bytesperline and sizeimage fields by converting to media bus
> >  * format and back to pixel format.
> > @@ -620,7 +615,6 @@ iss_video_set_format(struct file *file, void *fh, 
> > struct v4l2_format *format)
> >  
> > vfh->format = *format;
> >  
> > -   mutex_unlock(>mutex);
> > return 0;
> >  }
> >  
> > @@ -741,9 +735,7 @@ iss_video_set_selection(struct file *file, void *fh, 
> > struct v4l2_selection *sel)
> > return -EINVAL;
> >  
> > sdsel.pad = pad;
> > -   mutex_lock(>mutex);
> > ret = v4l2_subdev_call(subdev, pad, set_selection, NULL, );
> > -   mutex_unlock(>mutex);
> > if (!ret)
> > sel->r = sdsel.r;
> >  
> > @@ -873,8 +865,6 @@ iss_video_streamon(struct file *file, void *fh, enum 
> > v4l2_buf_type type)
> > if (type != video->type)
> > return -EINVAL;
> >  
> > -   mutex_lock(>stream_lock);
> > -
> > /*
> >  * Start streaming on the pipeline. No link touching an entity in the
> >  * pipeline can be activated or deactivated once streaming is started.
> > @@ -978,8 +968,6 @@ iss_video_streamon(struct file *file, void *fh, enum 
> > v4l2_buf_type type)
> >  
> > media_graph_walk_cleanup();
> >  
> > -   mutex_unlock(>stream_lock);
> > -
> > return 0;
> >  
> >  err_omap4iss_set_stream:
> > @@ -996,8 +984,6 @@ iss_video_streamon(struct file *file, void *fh, enum 
> > v4l2_buf_type type)
> >  err_graph_walk_init:
> > media_entity_enum_cleanup(>ent_enum);
> >  
> > -   mutex_unlock(>stream_lock);
> > -
> > return ret;
> >  }
> >  
> > @@ -1013,10 +999,8 @@ iss_video_streamoff(struct file *file, void *fh, enum 
> > v4l2_buf_type type)
> > if (type != video->type)
> > return -EINVAL;
> >  
> > -   mutex_lock(>stream_lock);
> > -
> > if (!vb2_is_streaming(>queue))
> > -   goto done;
> > +   return 0;
> >  
> > /* Update the pipeline state. */
> > if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > @@ -1041,8 +1025,6 @@ iss_video_streamoff(struct file *file, void *fh, enum 
> > v4l2_buf_type type)
> > video->iss->pdata->set_constraints(video->iss, false);
> > media_pipeline_stop(>video.entity);
> >  
> > -done:
> > -   mutex_unlock(>stream_lock);
> > return 0;
> >  }
> >  
> > @@ -1137,6 +1119,7 @@ static int iss_video_open(struct file *file)
> > q->buf_struct_size = sizeof(struct iss_buffer);
> > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > q->dev = video->iss->dev;
> > +   q->lock = >v4l_lock;
> 
> I feel too much is changed in this driver (and also the omap3isp driver in the
> next patch). Why not just set q->lock to stream_lock (queue_lock for 
> omap3isp)?
> 

OK. I'll set q->lock to stream_lock (queue_lock) and then of course,
drop all the mutex_lock/unlock that the driver is doing.

> There is no need to 

Re: [PATCHv2 0/4] gspca: convert to vb2gspca: convert to vb2

[Ezequiel Garcia]

On 22 May 2018 at 05:16, Hans Verkuil  wrote:
> On 18/05/18 19:51, Ezequiel Garcia wrote:
>> On 13 May 2018 at 06:47, Hans Verkuil  wrote:
>>> From: Hans Verkuil 
>>>
>>> The first patch converts the gspca driver to the vb2 framework.
>>> It was much easier to do than I expected and it saved almost 600
>>> lines of gspca driver code.
>>>
>>> The second patch fixes v4l2-compliance warnings for g/s_parm.
>>>
>>> The third patch clears relevant fields in v4l2_streamparm in
>>> v4l_s_parm(). This was never done before since v4l2-compliance
>>> didn't check this.
>>>
>>> The final patch deletes the now unused v4l2_disable_ioctl_locking()
>>> function.
>>>
>>> Tested with three different gspca webcams, and tested suspend/resume
>>> as well.
>>>
>>> I'll test with a few more webcams next week and if those tests all
>>> succeed then I'll post a pull request.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>> Changes since v1:
>>>
>>> - Re-added 'if (gspca_dev->present)' before the dq_callback call.
>>> - Added Reviewed-by tags from Hans de Goede.
>>>
>>> Hans Verkuil (4):
>>>   gspca: convert to vb2
>>>   gspca: fix g/s_parm handling
>>>   v4l2-ioctl: clear fields in s_parm
>>>   v4l2-ioctl: delete unused v4l2_disable_ioctl_locking
>>>
>>>  drivers/media/usb/gspca/Kconfig|   1 +
>>>  drivers/media/usb/gspca/gspca.c| 925 -
>>>  drivers/media/usb/gspca/gspca.h|  38 +-
>>>  drivers/media/usb/gspca/m5602/m5602_core.c |   4 +-
>>>  drivers/media/usb/gspca/ov534.c|   1 -
>>>  drivers/media/usb/gspca/topro.c|   1 -
>>>  drivers/media/usb/gspca/vc032x.c   |   2 +-
>>>  drivers/media/v4l2-core/v4l2-ioctl.c   |  19 +-
>>>  include/media/v4l2-dev.h   |  15 -
>>>  9 files changed, 210 insertions(+), 796 deletions(-)
>>>
>>
>> Got a NULL pointer testing this series. However, I don't think
>> the problem is with this series per-se, but more of a long-standing
>> race.
>>
>> [ 1133.771530] BUG: unable to handle kernel NULL pointer dereference
>> at 00c4
>> [ 1133.779354] PGD 0 P4D 0
>> [ 1133.781885] Oops:  [#1] PREEMPT SMP PTI
>> [ 1133.786065] CPU: 1 PID: 0 Comm: swapper/1 Not tainted
>> 4.17.0-rc3-next-20180503-ARCH-00029-gb14b92b054cc-dirty #11
>> [ 1133.796306] Hardware name: ASUS All Series/H81M-D R2.0, BIOS 0504 
>> 05/14/2015
>> [ 1133.803346] RIP: 0010:sd_pkt_scan+0x246/0x350 [gspca_sonixj]
>> [ 1133.808994] Code: 00 89 d9 4c 89 e2 be 03 00 00 00 4c 89 ef e8 f1
>> 06 c9 ff 49 8b 95 30 05 00 00 41 6b 85 d0 08 00 00 64 41 0f b7 8d d4
>> 08 00 00 <0f> af 8a c4 00 00 00 31 d2 f7 f1 83 f8 54 0f 8f a6 00 00 00
>> 83 f8
>> [ 1133.827845] RSP: 0018:88011fa83c78 EFLAGS: 00010002
>> [ 1133.833061] RAX: 00282d8c RBX: 02ee RCX: 
>> 0027
>> [ 1133.840204] RDX:  RSI: 0003 RDI: 
>> 8800c10b97e0
>> [ 1133.847343] RBP: 88011fa83ca0 R08: 000241a0 R09: 
>> a021d45e
>> [ 1133.854469] R10: 032c R11: 880115938700 R12: 
>> 8800c765b840
>> [ 1133.861591] R13: 8800c10b9000 R14: 032c R15: 
>> 032c
>> [ 1133.868726] FS:  () GS:88011fa8()
>> knlGS:
>> [ 1133.876802] CS:  0010 DS:  ES:  CR0: 80050033
>> [ 1133.882540] CR2: 00c4 CR3: 0200a004 CR4: 
>> 001606e0
>> [ 1133.889663] DR0:  DR1:  DR2: 
>> 
>> [ 1133.896788] DR3:  DR6: fffe0ff0 DR7: 
>> 0400
>> [ 1133.903920] Call Trace:
>> [ 1133.906365]  
>> [ 1133.908376]  ? sd_stop0+0x40/0x40 [gspca_sonixj]
>> [ 1133.912988]  isoc_irq+0xb8/0x130 [gspca_main]
>> [ 1133.917340]  __usb_hcd_giveback_urb+0x64/0xe0 [usbcore]
>> [ 1133.922565]  usb_hcd_giveback_urb+0x11f/0x130 [usbcore]
>> [ 1133.927782]  xhci_giveback_urb_in_irq.isra.20+0x84/0x100 [xhci_hcd]
>> [ 1133.934038]  ? handle_cmd_completion+0x2cd/0x1100 [xhci_hcd]
>> [ 1133.939689]  xhci_td_cleanup+0xfb/0x170 [xhci_hcd]
>> [ 1133.944490]  finish_td+0xb3/0xf0 [xhci_hcd]
>> [ 1133.948669]  xhci_irq+0x1532/0x2130 [xhci_hcd]
>> [ 1133.953106]  ? handle_irq_event+0x47/0x5b
>> [ 1133.957110]  xhci_msi_irq+0x11/0x20 [xhci_hcd]
>> [ 1133.961546]  __handle_irq_event_percpu+0x42/0x1b0
>> [ 1133.966243]  handle_irq_event_percpu+0x32/0x80
>> [ 1133.970681]  handle_irq_event+0x3c/0x5b
>> [ 1133.974511]  handle_edge_irq+0x7f/0x1b0
>> [ 1133.978342]  handle_irq+0x1a/0x30
>> [ 1133.981654]  do_IRQ+0x46/0xd0
>> [ 1133.984617]  common_interrupt+0xf/0xf
>> [ 1133.988274]  
>>
>> Common sense tells me that the gspca_dev->urb[0] is
>> set to NULL in destroy_urbs() and then accessed:
>>
>> static void sd_pkt_scan(struct gspca_dev *gspca_dev,
>> u8 *data,   /* isoc packet */
>> int len)/* iso 

Re: [PATCH v6 1/2] media: ov2680: dt: Add bindings for OV2680

[Rob Herring]

On Fri, May 18, 2018 at 10:27 AM, Rui Miguel Silva  wrote:
> Hi Rob,
> On Fri 18 May 2018 at 14:18, Rob Herring wrote:
>>
>> On Wed, May 09, 2018 at 03:31:58PM +0100, Rui Miguel Silva wrote:
>>>
>>> Add device tree binding documentation for the OV2680 camera sensor.
>>>
>>> CC: devicet...@vger.kernel.org
>>> Signed-off-by: Rui Miguel Silva 
>>> ---
>>>  .../devicetree/bindings/media/i2c/ov2680.txt  | 46  +++
>>>  1 file changed, 46 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/media/i2c/ov2680.txt
>>
>>
>> Please add acks/reviewed bys on new versions.
>
>
> I have add this to the cover letter [0]:
> - Removed Rob Herring Reviewed-by tag, since bindings have changed  since
> his
>  ack.
>
> But only now I notice that I did not CC the devicetree list for the all
> series, but only for this particular patch. Sorry about that.

NP. It's better to put this and revision history in the individual
patches. I don't always read cover letters anyways.

Rob

Re: [PATCH 1/6] dt-bindings: media: rcar-vin: Describe optional ep properties

[Rob Herring]

On Wed, May 23, 2018 at 2:38 PM, Laurent Pinchart
 wrote:
> Hi Rob,
>
> On Wednesday, 23 May 2018 19:29:47 EEST Rob Herring wrote:
>> On Wed, May 16, 2018 at 06:32:27PM +0200, Jacopo Mondi wrote:
>> > Describe the optional endpoint properties for endpoint nodes of port@0
>> > and port@1 of the R-Car VIN driver device tree bindings documentation.
>> >
>> > Signed-off-by: Jacopo Mondi 
>> > ---
>> >
>> >  Documentation/devicetree/bindings/media/rcar_vin.txt | 13 -
>> >  1 file changed, 12 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt
>> > b/Documentation/devicetree/bindings/media/rcar_vin.txt index
>> > a19517e1..c53ce4e 100644
>> > --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
>> > +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
>> > @@ -53,6 +53,16 @@ from local SoC CSI-2 receivers (port1) depending on
>> > SoC.
>> >
>> >from external SoC pins described in video-interfaces.txt[1].
>> >Describing more then one endpoint in port 0 is invalid. Only VIN
>> >instances that are connected to external pins should have port 0.
>> >
>> > +
>> > +  - Optional properties for endpoint nodes of port@0:
>> > +- hsync-active: active state of the HSYNC signal, 0/1 for
>> > LOW/HIGH
>> > + respectively. Default is active high.
>> > +- vsync-active: active state of the VSYNC signal, 0/1 for
>> > LOW/HIGH
>> > + respectively. Default is active high.
>> > +
>> > +   If both HSYNC and VSYNC polarities are not specified, embedded
>> > +   synchronization is selected.
>>
>> No need to copy-n-paste from video-interfaces.txt. Just "see
>> video-interfaces.txt" for the description is fine.
>
> I would still explicitly list the properties that apply to this binding. I
> agree that there's no need to copy & paste the description of those properties
> though.

Yes, that's what I meant. List each property with "see
video-interfaces.txt" for the description of each.

Rob

Re: [PATCH v4 0/3] IR decoding using BPF

[Sean Young]

On Wed, May 23, 2018 at 02:21:27PM +0200, Daniel Borkmann wrote:
> On 05/18/2018 04:07 PM, Sean Young wrote:
> > The kernel IR decoders (drivers/media/rc/ir-*-decoder.c) support the most
> > widely used IR protocols, but there are many protocols which are not
> > supported[1]. For example, the lirc-remotes[2] repo has over 2700 remotes,
> > many of which are not supported by rc-core. There is a "long tail" of
> > unsupported IR protocols, for which lircd is need to decode the IR .
> > 
> > IR encoding is done in such a way that some simple circuit can decode it;
> > therefore, bpf is ideal.
> > 
> > In order to support all these protocols, here we have bpf based IR decoding.
> > The idea is that user-space can define a decoder in bpf, attach it to
> > the rc device through the lirc chardev.
> > 
> > Separate work is underway to extend ir-keytable to have an extensive library
> > of bpf-based decoders, and a much expanded library of rc keymaps.
> > 
> > Another future application would be to compile IRP[3] to a IR BPF program, 
> > and
> > so support virtually every remote without having to write a decoder for 
> > each.
> > It might also be possible to support non-button devices such as analog
> > directional pads or air conditioning remote controls and decode the target
> > temperature in bpf, and pass that to an input device.
> 
> Mauro, are you fine with this series going via bpf-next? How ugly would this
> get with regards to merge conflicts wrt drivers/media/rc/?

There are no merge conflict and as of yet, I'm not expecting any. If anything
I suspect the bpf tree is more likely to change, so merging via bpf-next
might make more sense.

Thanks

Sean

> 
> Thanks,
> Daniel
> 
> > Thanks,
> > 
> > Sean Young
> > 
> > [1] http://www.hifi-remote.com/wiki/index.php?title=DecodeIR
> > [2] https://sourceforge.net/p/lirc-remotes/code/ci/master/tree/remotes/
> > [3] http://www.hifi-remote.com/wiki/index.php?title=IRP_Notation
> > 
> > Changes since v3:
> >  - Implemented review comments from Quentin Monnet and Y Song (thanks!)
> >  - More helpful and better formatted bpf helper documentation
> >  - Changed back to bpf_prog_array rather than open-coded implementation
> >  - scancodes can be 64 bit
> >  - bpf gets passed values in microseconds, not nanoseconds.
> >microseconds is more than than enough (IR receivers support carriers upto
> >70kHz, at which point a single period is already 14 microseconds). Also,
> >this makes it much more consistent with lirc mode2.
> >  - Since it looks much more like lirc mode2, rename the program type to
> >BPF_PROG_TYPE_LIRC_MODE2.
> >  - Rebased on bpf-next
> > 
> > Changes since v2:
> >  - Fixed locking issues
> >  - Improved self-test to cover more cases
> >  - Rebased on bpf-next again
> > 
> > Changes since v1:
> >  - Code review comments from Y Song  and
> >Randy Dunlap 
> >  - Re-wrote sample bpf to be selftest
> >  - Renamed RAWIR_DECODER -> RAWIR_EVENT (Kconfig, context, bpf prog type)
> >  - Rebase on bpf-next
> >  - Introduced bpf_rawir_event context structure with simpler access checking
> > 
> > Sean Young (3):
> >   bpf: bpf_prog_array_copy() should return -ENOENT if exclude_prog not
> > found
> >   media: rc: introduce BPF_PROG_LIRC_MODE2
> >   bpf: add selftest for lirc_mode2 type program
> > 
> >  drivers/media/rc/Kconfig  |  13 +
> >  drivers/media/rc/Makefile |   1 +
> >  drivers/media/rc/bpf-lirc.c   | 308 ++
> >  drivers/media/rc/lirc_dev.c   |  30 ++
> >  drivers/media/rc/rc-core-priv.h   |  22 ++
> >  drivers/media/rc/rc-ir-raw.c  |  12 +-
> >  include/linux/bpf_rcdev.h |  30 ++
> >  include/linux/bpf_types.h |   3 +
> >  include/uapi/linux/bpf.h  |  53 ++-
> >  kernel/bpf/core.c |  11 +-
> >  kernel/bpf/syscall.c  |   7 +
> >  kernel/trace/bpf_trace.c  |   2 +
> >  tools/bpf/bpftool/prog.c  |   1 +
> >  tools/include/uapi/linux/bpf.h|  53 ++-
> >  tools/include/uapi/linux/lirc.h   | 217 
> >  tools/lib/bpf/libbpf.c|   1 +
> >  tools/testing/selftests/bpf/Makefile  |   8 +-
> >  tools/testing/selftests/bpf/bpf_helpers.h |   6 +
> >  .../testing/selftests/bpf/test_lirc_mode2.sh  |  28 ++
> >  .../selftests/bpf/test_lirc_mode2_kern.c  |  23 ++
> >  .../selftests/bpf/test_lirc_mode2_user.c  | 154 +
> >  21 files changed, 974 insertions(+), 9 deletions(-)
> >  create mode 100644 drivers/media/rc/bpf-lirc.c
> >  create mode 100644 include/linux/bpf_rcdev.h
> >  create mode 100644 tools/include/uapi/linux/lirc.h
> >  create mode 100755 tools/testing/selftests/bpf/test_lirc_mode2.sh
> >  create mode 100644 

Re: [PATCH 1/6] dt-bindings: media: rcar-vin: Describe optional ep properties

[Laurent Pinchart]

Hi Rob,

On Wednesday, 23 May 2018 19:29:47 EEST Rob Herring wrote:
> On Wed, May 16, 2018 at 06:32:27PM +0200, Jacopo Mondi wrote:
> > Describe the optional endpoint properties for endpoint nodes of port@0
> > and port@1 of the R-Car VIN driver device tree bindings documentation.
> > 
> > Signed-off-by: Jacopo Mondi 
> > ---
> > 
> >  Documentation/devicetree/bindings/media/rcar_vin.txt | 13 -
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt
> > b/Documentation/devicetree/bindings/media/rcar_vin.txt index
> > a19517e1..c53ce4e 100644
> > --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> > +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> > @@ -53,6 +53,16 @@ from local SoC CSI-2 receivers (port1) depending on
> > SoC.
> > 
> >from external SoC pins described in video-interfaces.txt[1].
> >Describing more then one endpoint in port 0 is invalid. Only VIN
> >instances that are connected to external pins should have port 0.
> > 
> > +
> > +  - Optional properties for endpoint nodes of port@0:
> > +- hsync-active: active state of the HSYNC signal, 0/1 for
> > LOW/HIGH
> > + respectively. Default is active high.
> > +- vsync-active: active state of the VSYNC signal, 0/1 for
> > LOW/HIGH
> > + respectively. Default is active high.
> > +
> > +   If both HSYNC and VSYNC polarities are not specified, embedded
> > +   synchronization is selected.
> 
> No need to copy-n-paste from video-interfaces.txt. Just "see
> video-interfaces.txt" for the description is fine.

I would still explicitly list the properties that apply to this binding. I 
agree that there's no need to copy & paste the description of those properties 
though.

> > +
> > 
> >  - port 1 - sub-nodes describing one or more endpoints connected to
> >  
> >the VIN from local SoC CSI-2 receivers. The endpoint numbers must
> >use the following schema.
> > 
> > @@ -62,6 +72,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
> > 
> >  - Endpoint 2 - sub-node describing the endpoint connected to
> >  CSI40
> >  - Endpoint 3 - sub-node describing the endpoint connected to
> >  CSI41
> > 
> > +  Endpoint nodes of port@1 do not support any optional endpoint
> > property. +
> > 
> >  Device node example for Gen2 platforms
> >  --
> > 
> > @@ -112,7 +124,6 @@ Board setup example for Gen2 platforms (vin1 composite
> > video input)> 
> >  vin1ep0: endpoint {
> >  
> >  remote-endpoint = <>;
> > 
> > -bus-width = <8>;
> > 
> >  };
> >  
> >  };
> >  
> >  };

-- 
Regards,

Laurent Pinchart

Re: [ANN] Meeting to discuss improvements to support MC-based cameras on generic apps

[Laurent Pinchart]

Hi Hans,

On Wednesday, 23 May 2018 19:19:37 EEST Hans Verkuil wrote:
> On 18/05/18 13:24, Mauro Carvalho Chehab wrote:
> > One of the biggest reasons why we decided to start libv4l project,
> > in the past, was to ensure an open source solution. The problem we
> > faced on that time is to ensure that, when a new media driver were
> > added with some proprietary output format, an open source decoding
> > software were also added at libv4l.
> > 
> > This approach ensured that all non-MC cameras are supported by all
> > V4L2 applications.
> > 
> > Before libv4l, media support for a given device were limited to a few
> > apps that knew how to decode the format. There were even cases were a
> > proprietary app were required, as no open source decoders were available.
> > 
> > From my PoV, the biggest gain with libv4l is that the same group of
> > maintainers can ensure that the entire solution (Kernel driver and
> > low level userspace support) will provide everything required for an
> > open source app to work with it.
> > 
> > I'm not sure how we would keep enforcing it if the pipeline setting
> > and control propagation logic for an specific hardware will be
> > delegated to PipeWire. It seems easier to keep doing it on a libv4l
> > (version 2) and let PipeWire to use it.
> 
> I've decided not to attend this meeting. It is not quite my core expertise
> and it is a bit too far to make it worth my time. If there are good reasons
> for me being there that I missed, then please let me know asap and I might
> reconsider this.
> 
> What I would like to say though it that I think libv4l is a bit of a dead
> end and probably not suitable for adding support for this.
> 
> Currently libv4l2 is too intertwined with libv4lconvert and too messy.
> Its original motivation was for converting custom formats and that is
> mostly obsolete now that UVC has standardized formats to just a few.
> 
> I think a core library is needed that provides the basic functionality
> and that can be used directly by applications if they don't want to use
> v4l2_open() and friends.
> 
> I.e. it should be possible for e.g. gstreamer to use this core library
> to easily configure and use the MC instead of having to call v4l2_open()
> etc. and rely on magic code to do this for them. It's simply ugly to
> overload mmap with v4l2_mmap or to emulate read() if the driver doesn't
> support it.
> 
> We might still have a libv4l2-like library sitting on top of this, but
> perhaps with limited functionality. For example, I think it would be
> reasonable to no longer support custom formats. If an application wants
> to support that, then it should call conversion functions for the core
> library explicitly. This has the big advantage of solving the dmabuf
> and mmap issues in today's libv4l2.

I agree with all that. I also believe we need to design a clean solution 
without caring about the existing libv4l2 API, and then implement a 
compatibility layer on top of our new library. The way to move away from the 
stone age is to design a new technology for the future, and then help the past 
climb on the bandwagon.

-- 
Regards,

Laurent Pinchart

Re: Bugfix for Tevii S650

[Light]

+Cc: Olli

The problem started with this commit. it added function 
dw2102_disconnect() to dw2102.c


author     Olli Salonen     2015-03-16 17:14:05 (GMT)
committer    Mauro Carvalho Chehab  2015-04-03 
01:24:01 (GMT)

commit    70769b24d2973428907de3429dd2a5792e4ce317 (patch)

[media] dw2102: store i2c client for tuner into dw2102_state


On 21.05.2018 22:30, Andy Shevchenko wrote:

+Cc: Mauro

On Mon, May 21, 2018 at 3:01 PM, Light  wrote:

Hi,

starting with kernel 4.1 the tevii S650 usb box is not working any more, last
working version was 4.0.

The  bug was also reported here
https://www.spinics.net/lists/linux-media/msg121356.html

I found a solution for it and uploaded a patch to the kernel bugzilla.

See here: https://bugzilla.kernel.org/show_bug.cgi?id=197731

Can somebody of the maintainers have a look on it and apply the patch to the
kernes sources?

You forget to Cc to maintainers (at least Mauro).

Re: [PATCH] dt-bindings: media: rcar_vin: fix style for ports and endpoints

[Rob Herring]

On Thu, May 17, 2018 at 01:32:12AM +0200, Niklas Söderlund wrote:
> The style for referring to ports and endpoint are wrong. Refer to them
> using lowercase and a unit address, port@x and endpoint@x.
> 
> Signed-off-by: Niklas Söderlund 
> Reported-by: Geert Uytterhoeven 
> ---
>  .../devicetree/bindings/media/rcar_vin.txt| 20 +--
>  1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Rob Herring 

Re: [PATCH 2/6] dt-bindings: media: rcar-vin: Document data-active

[Rob Herring]

On Wed, May 16, 2018 at 06:32:28PM +0200, Jacopo Mondi wrote:
> Document 'data-active' property in R-Car VIN device tree bindings.
> The property is optional when running with explicit synchronization
> (eg. BT.601) but mandatory when embedded synchronization is in use (eg.
> BT.656) as specified by the hardware manual.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  Documentation/devicetree/bindings/media/rcar_vin.txt | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt 
> b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index c53ce4e..17eac8a 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -63,6 +63,11 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
>   If both HSYNC and VSYNC polarities are not specified, embedded
>   synchronization is selected.
> 
> +- data-active: active state of data enable signal (CLOCKENB pin).
> +  0/1 for LOW/HIGH respectively. If not specified, use HSYNC as
> +  data enable signal. When using embedded synchronization this
> +  property is mandatory.

This doesn't match the common property's definition which AIUI is for 
the data lines' polarity. You need a new property. Perhaps a common one.

> +
>  - port 1 - sub-nodes describing one or more endpoints connected to
>the VIN from local SoC CSI-2 receivers. The endpoint numbers must
>use the following schema.
> --
> 2.7.4
> 

Re: [PATCH 5/6] ARM: dts: rcar-gen2: Remove unused VIN properties

[Rob Herring]

On Thu, May 17, 2018 at 12:13:07AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
> 
> Thanks for your work.
> 
> On 2018-05-16 18:32:31 +0200, Jacopo Mondi wrote:
> > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN
> > driver and only confuse users. Remove them in all Gen2 SoC that used
> > them.
> > 
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  arch/arm/boot/dts/r8a7790-lager.dts   | 3 ---
> >  arch/arm/boot/dts/r8a7791-koelsch.dts | 3 ---
> >  arch/arm/boot/dts/r8a7791-porter.dts  | 1 -
> >  arch/arm/boot/dts/r8a7793-gose.dts| 3 ---
> >  arch/arm/boot/dts/r8a7794-alt.dts | 1 -
> >  arch/arm/boot/dts/r8a7794-silk.dts| 1 -
> >  6 files changed, 12 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts 
> > b/arch/arm/boot/dts/r8a7790-lager.dts
> > index 063fdb6..b56b309 100644
> > --- a/arch/arm/boot/dts/r8a7790-lager.dts
> > +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> > @@ -873,10 +873,8 @@
> > port {
> > vin0ep2: endpoint {
> > remote-endpoint = <_out>;
> > -   bus-width = <24>;
> 
> I can't really make up my mind if this is a good thing or not. Device 
> tree describes the hardware and not what the drivers make use of. And 
> the fact is that this bus is 24 bits wide. So I'm not sure we should 
> remove these properties. But I would love to hear what others think 
> about this.

IMO, this property should only be present when all the pins are not 
connected. And by "all", I mean the minimum of what each end of the 
graph can support.

Rob

Re: [PATCH 1/6] dt-bindings: media: rcar-vin: Describe optional ep properties

[Rob Herring]

On Wed, May 16, 2018 at 06:32:27PM +0200, Jacopo Mondi wrote:
> Describe the optional endpoint properties for endpoint nodes of port@0
> and port@1 of the R-Car VIN driver device tree bindings documentation.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  Documentation/devicetree/bindings/media/rcar_vin.txt | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt 
> b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index a19517e1..c53ce4e 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -53,6 +53,16 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
>from external SoC pins described in video-interfaces.txt[1].
>Describing more then one endpoint in port 0 is invalid. Only VIN
>instances that are connected to external pins should have port 0.
> +
> +  - Optional properties for endpoint nodes of port@0:
> +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH
> +   respectively. Default is active high.
> +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH
> +   respectively. Default is active high.
> +
> + If both HSYNC and VSYNC polarities are not specified, embedded
> + synchronization is selected.

No need to copy-n-paste from video-interfaces.txt. Just "see 
video-interfaces.txt" for the description is fine.

> +
>  - port 1 - sub-nodes describing one or more endpoints connected to
>the VIN from local SoC CSI-2 receivers. The endpoint numbers must
>use the following schema.
> @@ -62,6 +72,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
>  - Endpoint 2 - sub-node describing the endpoint connected to CSI40
>  - Endpoint 3 - sub-node describing the endpoint connected to CSI41
> 
> +  Endpoint nodes of port@1 do not support any optional endpoint property.
> +
>  Device node example for Gen2 platforms
>  --
> 
> @@ -112,7 +124,6 @@ Board setup example for Gen2 platforms (vin1 composite 
> video input)
> 
>  vin1ep0: endpoint {
>  remote-endpoint = <>;
> -bus-width = <8>;
>  };
>  };
>  };
> --
> 2.7.4
> 

Re: [ANN] Meeting to discuss improvements to support MC-based cameras on generic apps

[Hans Verkuil]

On 18/05/18 13:24, Mauro Carvalho Chehab wrote:
> One of the biggest reasons why we decided to start libv4l project,
> in the past, was to ensure an open source solution. The problem we
> faced on that time is to ensure that, when a new media driver were
> added with some proprietary output format, an open source decoding
> software were also added at libv4l.
> 
> This approach ensured that all non-MC cameras are supported by all
> V4L2 applications.
> 
> Before libv4l, media support for a given device were limited to a few 
> apps that knew how to decode the format. There were even cases were a
> proprietary app were required, as no open source decoders were available.
> 
> From my PoV, the biggest gain with libv4l is that the same group of
> maintainers can ensure that the entire solution (Kernel driver and
> low level userspace support) will provide everything required for an
> open source app to work with it.
> 
> I'm not sure how we would keep enforcing it if the pipeline setting
> and control propagation logic for an specific hardware will be
> delegated to PipeWire. It seems easier to keep doing it on a libv4l
> (version 2) and let PipeWire to use it.

I've decided not to attend this meeting. It is not quite my core expertise
and it is a bit too far to make it worth my time. If there are good reasons
for me being there that I missed, then please let me know asap and I might
reconsider this.

What I would like to say though it that I think libv4l is a bit of a dead
end and probably not suitable for adding support for this.

Currently libv4l2 is too intertwined with libv4lconvert and too messy.
Its original motivation was for converting custom formats and that is
mostly obsolete now that UVC has standardized formats to just a few.

I think a core library is needed that provides the basic functionality
and that can be used directly by applications if they don't want to use
v4l2_open() and friends.

I.e. it should be possible for e.g. gstreamer to use this core library
to easily configure and use the MC instead of having to call v4l2_open() etc.
and rely on magic code to do this for them. It's simply ugly to overload
mmap with v4l2_mmap or to emulate read() if the driver doesn't support it.

We might still have a libv4l2-like library sitting on top of this, but
perhaps with limited functionality. For example, I think it would be
reasonable to no longer support custom formats. If an application wants
to support that, then it should call conversion functions for the core
library explicitly. This has the big advantage of solving the dmabuf
and mmap issues in today's libv4l2.

Regards,

Hans

Re: [PATCH v4 0/3] IR decoding using BPF

[Daniel Borkmann]

On 05/18/2018 04:07 PM, Sean Young wrote:
> The kernel IR decoders (drivers/media/rc/ir-*-decoder.c) support the most
> widely used IR protocols, but there are many protocols which are not
> supported[1]. For example, the lirc-remotes[2] repo has over 2700 remotes,
> many of which are not supported by rc-core. There is a "long tail" of
> unsupported IR protocols, for which lircd is need to decode the IR .
> 
> IR encoding is done in such a way that some simple circuit can decode it;
> therefore, bpf is ideal.
> 
> In order to support all these protocols, here we have bpf based IR decoding.
> The idea is that user-space can define a decoder in bpf, attach it to
> the rc device through the lirc chardev.
> 
> Separate work is underway to extend ir-keytable to have an extensive library
> of bpf-based decoders, and a much expanded library of rc keymaps.
> 
> Another future application would be to compile IRP[3] to a IR BPF program, and
> so support virtually every remote without having to write a decoder for each.
> It might also be possible to support non-button devices such as analog
> directional pads or air conditioning remote controls and decode the target
> temperature in bpf, and pass that to an input device.

Mauro, are you fine with this series going via bpf-next? How ugly would this
get with regards to merge conflicts wrt drivers/media/rc/?

Thanks,
Daniel

> Thanks,
> 
> Sean Young
> 
> [1] http://www.hifi-remote.com/wiki/index.php?title=DecodeIR
> [2] https://sourceforge.net/p/lirc-remotes/code/ci/master/tree/remotes/
> [3] http://www.hifi-remote.com/wiki/index.php?title=IRP_Notation
> 
> Changes since v3:
>  - Implemented review comments from Quentin Monnet and Y Song (thanks!)
>  - More helpful and better formatted bpf helper documentation
>  - Changed back to bpf_prog_array rather than open-coded implementation
>  - scancodes can be 64 bit
>  - bpf gets passed values in microseconds, not nanoseconds.
>microseconds is more than than enough (IR receivers support carriers upto
>70kHz, at which point a single period is already 14 microseconds). Also,
>this makes it much more consistent with lirc mode2.
>  - Since it looks much more like lirc mode2, rename the program type to
>BPF_PROG_TYPE_LIRC_MODE2.
>  - Rebased on bpf-next
> 
> Changes since v2:
>  - Fixed locking issues
>  - Improved self-test to cover more cases
>  - Rebased on bpf-next again
> 
> Changes since v1:
>  - Code review comments from Y Song  and
>Randy Dunlap 
>  - Re-wrote sample bpf to be selftest
>  - Renamed RAWIR_DECODER -> RAWIR_EVENT (Kconfig, context, bpf prog type)
>  - Rebase on bpf-next
>  - Introduced bpf_rawir_event context structure with simpler access checking
> 
> Sean Young (3):
>   bpf: bpf_prog_array_copy() should return -ENOENT if exclude_prog not
> found
>   media: rc: introduce BPF_PROG_LIRC_MODE2
>   bpf: add selftest for lirc_mode2 type program
> 
>  drivers/media/rc/Kconfig  |  13 +
>  drivers/media/rc/Makefile |   1 +
>  drivers/media/rc/bpf-lirc.c   | 308 ++
>  drivers/media/rc/lirc_dev.c   |  30 ++
>  drivers/media/rc/rc-core-priv.h   |  22 ++
>  drivers/media/rc/rc-ir-raw.c  |  12 +-
>  include/linux/bpf_rcdev.h |  30 ++
>  include/linux/bpf_types.h |   3 +
>  include/uapi/linux/bpf.h  |  53 ++-
>  kernel/bpf/core.c |  11 +-
>  kernel/bpf/syscall.c  |   7 +
>  kernel/trace/bpf_trace.c  |   2 +
>  tools/bpf/bpftool/prog.c  |   1 +
>  tools/include/uapi/linux/bpf.h|  53 ++-
>  tools/include/uapi/linux/lirc.h   | 217 
>  tools/lib/bpf/libbpf.c|   1 +
>  tools/testing/selftests/bpf/Makefile  |   8 +-
>  tools/testing/selftests/bpf/bpf_helpers.h |   6 +
>  .../testing/selftests/bpf/test_lirc_mode2.sh  |  28 ++
>  .../selftests/bpf/test_lirc_mode2_kern.c  |  23 ++
>  .../selftests/bpf/test_lirc_mode2_user.c  | 154 +
>  21 files changed, 974 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/media/rc/bpf-lirc.c
>  create mode 100644 include/linux/bpf_rcdev.h
>  create mode 100644 tools/include/uapi/linux/lirc.h
>  create mode 100755 tools/testing/selftests/bpf/test_lirc_mode2.sh
>  create mode 100644 tools/testing/selftests/bpf/test_lirc_mode2_kern.c
>  create mode 100644 tools/testing/selftests/bpf/test_lirc_mode2_user.c
> 

Re: [PATCH v10 08/16] v4l: mark unordered formats

[Hans Verkuil]

On 23/05/18 12:30, Ezequiel Garcia wrote:
> On Tue, 2018-05-22 at 13:55 +0200, Hans Verkuil wrote:
>> On 21/05/18 18:59, Ezequiel Garcia wrote:
>>> From: Gustavo Padovan 
>>>
>>> Now that we've introduced the V4L2_FMT_FLAG_UNORDERED flag,
>>> mark the appropriate formats.
>>>
>>> v2: Set unordered flag before calling the driver callback.
>>>
>>> Signed-off-by: Gustavo Padovan 
>>> Signed-off-by: Ezequiel Garcia 
>>> ---
>>>  drivers/media/v4l2-core/v4l2-ioctl.c | 74 
>>> +++-
>>>  1 file changed, 57 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
>>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index f48c505550e0..2135ac235a96 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -1102,6 +1102,27 @@ static int v4l_enumoutput(const struct 
>>> v4l2_ioctl_ops *ops,
>>> return ops->vidioc_enum_output(file, fh, p);
>>>  }
>>>  
>>> +static void v4l_fill_unordered_fmtdesc(struct v4l2_fmtdesc *fmt)
>>> +{
>>> +   switch (fmt->pixelformat) {
>>> +   case V4L2_PIX_FMT_MPEG:
>>> +   case V4L2_PIX_FMT_H264:
>>> +   case V4L2_PIX_FMT_H264_NO_SC:
>>> +   case V4L2_PIX_FMT_H264_MVC:
>>> +   case V4L2_PIX_FMT_H263:
>>> +   case V4L2_PIX_FMT_MPEG1:
>>> +   case V4L2_PIX_FMT_MPEG2:
>>> +   case V4L2_PIX_FMT_MPEG4:
>>> +   case V4L2_PIX_FMT_XVID:
>>> +   case V4L2_PIX_FMT_VC1_ANNEX_G:
>>> +   case V4L2_PIX_FMT_VC1_ANNEX_L:
>>> +   case V4L2_PIX_FMT_VP8:
>>> +   case V4L2_PIX_FMT_VP9:
>>> +   case V4L2_PIX_FMT_HEVC:
>>> +   fmt->flags |= V4L2_FMT_FLAG_UNORDERED;
>>
>> Please add a break here. This prevents potential future errors if new cases
>> are added below this line.
>>
> 
> Sure.
> 
>>> +   }
>>> +}
>>> +
>>>  static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>>>  {
>>> const unsigned sz = sizeof(fmt->description);
>>> @@ -1310,61 +1331,80 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc 
>>> *fmt)
>>>  
>>> if (descr)
>>> WARN_ON(strlcpy(fmt->description, descr, sz) >= sz);
>>> -   fmt->flags = flags;
>>> +   fmt->flags |= flags;
>>>  }
>>>  
>>> -static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>> -   struct file *file, void *fh, void *arg)
>>> -{
>>> -   struct v4l2_fmtdesc *p = arg;
>>> -   int ret = check_fmt(file, p->type);
>>>  
>>> -   if (ret)
>>> -   return ret;
>>> -   ret = -EINVAL;
>>> +static int __vidioc_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>> +struct v4l2_fmtdesc *p,
>>> +struct file *file, void *fh)
>>> +{
>>> +   int ret = 0;
>>>  
>>> switch (p->type) {
>>> case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>> if (unlikely(!ops->vidioc_enum_fmt_vid_cap))
>>> break;
>>> -   ret = ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
>>> +   ret = ops->vidioc_enum_fmt_vid_cap(file, fh, p);
>>> break;
>>> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>> if (unlikely(!ops->vidioc_enum_fmt_vid_cap_mplane))
>>> break;
>>> -   ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, arg);
>>> +   ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, p);
>>> break;
>>> case V4L2_BUF_TYPE_VIDEO_OVERLAY:
>>> if (unlikely(!ops->vidioc_enum_fmt_vid_overlay))
>>> break;
>>> -   ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, arg);
>>> +   ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, p);
>>> break;
>>> case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>>> if (unlikely(!ops->vidioc_enum_fmt_vid_out))
>>> break;
>>> -   ret = ops->vidioc_enum_fmt_vid_out(file, fh, arg);
>>> +   ret = ops->vidioc_enum_fmt_vid_out(file, fh, p);
>>> break;
>>> case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>> if (unlikely(!ops->vidioc_enum_fmt_vid_out_mplane))
>>> break;
>>> -   ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
>>> +   ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, p);
>>> break;
>>> case V4L2_BUF_TYPE_SDR_CAPTURE:
>>> if (unlikely(!ops->vidioc_enum_fmt_sdr_cap))
>>> break;
>>> -   ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
>>> +   ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, p);
>>> break;
>>> case V4L2_BUF_TYPE_SDR_OUTPUT:
>>> if (unlikely(!ops->vidioc_enum_fmt_sdr_out))
>>> break;
>>> -   ret = ops->vidioc_enum_fmt_sdr_out(file, fh, arg);
>>> +   ret = ops->vidioc_enum_fmt_sdr_out(file, fh, p);
>>> break;
>>> case V4L2_BUF_TYPE_META_CAPTURE:
>>> if (unlikely(!ops->vidioc_enum_fmt_meta_cap))
>>> break;
>>> -   ret = 

Re: [PATCH] gpu: ipu-v3: Fix BT1120 interlaced CCIR codes

[Marek Vasut]

On 05/22/2018 01:07 PM, Philipp Zabel wrote:
> Hi Marek,
> 
> On Fri, 2018-05-18 at 18:21 +0200, Marek Vasut wrote:
>> On 05/18/2018 05:51 PM, Philipp Zabel wrote:
>>> Hi Marek,
>>>
>>> On Sat, 2018-04-07 at 15:04 +0200, Marek Vasut wrote:
 The BT1120 interlaced CCIR codes are the same as BT656 ones
 and different than BT656 progressive CCIR codes, fix this.
>>>
>>> thank you for the patch, and sorry for the delay.
>>>
 Signed-off-by: Marek Vasut 
 Cc: Steve Longerbeam 
 Cc: Philipp Zabel 
 ---
  drivers/gpu/ipu-v3/ipu-csi.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

 diff --git a/drivers/gpu/ipu-v3/ipu-csi.c b/drivers/gpu/ipu-v3/ipu-csi.c
 index caa05b0702e1..301a729581ce 100644
 --- a/drivers/gpu/ipu-v3/ipu-csi.c
 +++ b/drivers/gpu/ipu-v3/ipu-csi.c
 @@ -435,12 +435,16 @@ int ipu_csi_init_interface(struct ipu_csi *csi,
break;
case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_DDR:
case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_SDR:
 -  case IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_DDR:
 -  case IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR:
ipu_csi_write(csi, 0x40030 | CSI_CCIR_ERR_DET_EN,
   CSI_CCIR_CODE_1);
ipu_csi_write(csi, 0xFF, CSI_CCIR_CODE_3);
break;
 +  case IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_DDR:
 +  case IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR:
 +  ipu_csi_write(csi, 0x40596 | CSI_CCIR_ERR_DET_EN, 
 CSI_CCIR_CODE_1);
 +  ipu_csi_write(csi, 0xD07DF, CSI_CCIR_CODE_2);
 +  ipu_csi_write(csi, 0xFF, CSI_CCIR_CODE_3);
>>>
>>> If these are the same as BT656 codes (so this case would be for PAL?),
>>> could this just be moved up into the IPU_CSI_CLK_MODE_CCIR656_INTERLACED
>>> case? Would the NTSC CCIR codes be the same as well?
>>
>> Dunno, I don't have any NTSC device to test. But the above was tested
>> with a PAL device I had.
>>
>> I think the CCIR codes are different from BT656, although I might be wrong.
> 
> The driver currently has:
> 
> case IPU_CSI_CLK_MODE_CCIR656_INTERLACED:
> if (mbus_fmt->width == 720 && mbus_fmt->height == 576) {
> /*
>  * PAL case
>  *
>  * Field0BlankEnd = 0x6, Field0BlankStart = 0x2,
>  * Field0ActiveEnd = 0x4, Field0ActiveStart = 0
>  * Field1BlankEnd = 0x7, Field1BlankStart = 0x3,
>  * Field1ActiveEnd = 0x5, Field1ActiveStart = 0x1
>  */
> height = 625; /* framelines for PAL */
> 
> ipu_csi_write(csi, 0x40596 | CSI_CCIR_ERR_DET_EN,
>   CSI_CCIR_CODE_1);
> ipu_csi_write(csi, 0xD07DF, CSI_CCIR_CODE_2);
> ipu_csi_write(csi, 0xFF, CSI_CCIR_CODE_3);
> } else if (mbus_fmt->width == 720 && mbus_fmt->height == 480) 
> {   
> /*
>  * NTSC case
>  *
>  * Field0BlankEnd = 0x7, Field0BlankStart = 0x3,
>  * Field0ActiveEnd = 0x5, Field0ActiveStart = 0x1
>  * Field1BlankEnd = 0x6, Field1BlankStart = 0x2,
>  * Field1ActiveEnd = 0x4, Field1ActiveStart = 0
>  */
> height = 525; /* framelines for NTSC */
> 
> ipu_csi_write(csi, 0xD07DF | CSI_CCIR_ERR_DET_EN,
>   CSI_CCIR_CODE_1);
> ipu_csi_write(csi, 0x40596, CSI_CCIR_CODE_2);
> ipu_csi_write(csi, 0xFF, CSI_CCIR_CODE_3);
> } else {
> dev_err(csi->ipu->dev,
> "Unsupported CCIR656 interlaced video 
> mode\n");
> spin_unlock_irqrestore(>lock, flags);
> return -EINVAL;
> }
> break;
> 
> The PAL codes are exactly the same as in your patch. That's why I wonder
> whether we should just move
>   case IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_DDR:
>   case IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR:
> up before
> case IPU_CSI_CLK_MODE_CCIR656_INTERLACED:
> as follows:

Isn't the PAL code doing some sort of resolution check too ? Although
this would probably work in my case too.

> --8<--
> diff --git a/drivers/gpu/ipu-v3/ipu-csi.c b/drivers/gpu/ipu-v3/ipu-csi.c
> index caa05b0702e1..7e96382f9cb1 100644
> --- a/drivers/gpu/ipu-v3/ipu-csi.c
> +++ b/drivers/gpu/ipu-v3/ipu-csi.c
> @@ -396,6 +396,8 @@ int ipu_csi_init_interface(struct ipu_csi *csi,
>   

Re: [PATCH v10 08/16] v4l: mark unordered formats

[Ezequiel Garcia]

On Tue, 2018-05-22 at 13:55 +0200, Hans Verkuil wrote:
> On 21/05/18 18:59, Ezequiel Garcia wrote:
> > From: Gustavo Padovan 
> > 
> > Now that we've introduced the V4L2_FMT_FLAG_UNORDERED flag,
> > mark the appropriate formats.
> > 
> > v2: Set unordered flag before calling the driver callback.
> > 
> > Signed-off-by: Gustavo Padovan 
> > Signed-off-by: Ezequiel Garcia 
> > ---
> >  drivers/media/v4l2-core/v4l2-ioctl.c | 74 
> > +++-
> >  1 file changed, 57 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
> > b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index f48c505550e0..2135ac235a96 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1102,6 +1102,27 @@ static int v4l_enumoutput(const struct 
> > v4l2_ioctl_ops *ops,
> > return ops->vidioc_enum_output(file, fh, p);
> >  }
> >  
> > +static void v4l_fill_unordered_fmtdesc(struct v4l2_fmtdesc *fmt)
> > +{
> > +   switch (fmt->pixelformat) {
> > +   case V4L2_PIX_FMT_MPEG:
> > +   case V4L2_PIX_FMT_H264:
> > +   case V4L2_PIX_FMT_H264_NO_SC:
> > +   case V4L2_PIX_FMT_H264_MVC:
> > +   case V4L2_PIX_FMT_H263:
> > +   case V4L2_PIX_FMT_MPEG1:
> > +   case V4L2_PIX_FMT_MPEG2:
> > +   case V4L2_PIX_FMT_MPEG4:
> > +   case V4L2_PIX_FMT_XVID:
> > +   case V4L2_PIX_FMT_VC1_ANNEX_G:
> > +   case V4L2_PIX_FMT_VC1_ANNEX_L:
> > +   case V4L2_PIX_FMT_VP8:
> > +   case V4L2_PIX_FMT_VP9:
> > +   case V4L2_PIX_FMT_HEVC:
> > +   fmt->flags |= V4L2_FMT_FLAG_UNORDERED;
> 
> Please add a break here. This prevents potential future errors if new cases
> are added below this line.
> 

Sure.

> > +   }
> > +}
> > +
> >  static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> >  {
> > const unsigned sz = sizeof(fmt->description);
> > @@ -1310,61 +1331,80 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc 
> > *fmt)
> >  
> > if (descr)
> > WARN_ON(strlcpy(fmt->description, descr, sz) >= sz);
> > -   fmt->flags = flags;
> > +   fmt->flags |= flags;
> >  }
> >  
> > -static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > -   struct file *file, void *fh, void *arg)
> > -{
> > -   struct v4l2_fmtdesc *p = arg;
> > -   int ret = check_fmt(file, p->type);
> >  
> > -   if (ret)
> > -   return ret;
> > -   ret = -EINVAL;
> > +static int __vidioc_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > +struct v4l2_fmtdesc *p,
> > +struct file *file, void *fh)
> > +{
> > +   int ret = 0;
> >  
> > switch (p->type) {
> > case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > if (unlikely(!ops->vidioc_enum_fmt_vid_cap))
> > break;
> > -   ret = ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
> > +   ret = ops->vidioc_enum_fmt_vid_cap(file, fh, p);
> > break;
> > case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > if (unlikely(!ops->vidioc_enum_fmt_vid_cap_mplane))
> > break;
> > -   ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, arg);
> > +   ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, p);
> > break;
> > case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> > if (unlikely(!ops->vidioc_enum_fmt_vid_overlay))
> > break;
> > -   ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, arg);
> > +   ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, p);
> > break;
> > case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> > if (unlikely(!ops->vidioc_enum_fmt_vid_out))
> > break;
> > -   ret = ops->vidioc_enum_fmt_vid_out(file, fh, arg);
> > +   ret = ops->vidioc_enum_fmt_vid_out(file, fh, p);
> > break;
> > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > if (unlikely(!ops->vidioc_enum_fmt_vid_out_mplane))
> > break;
> > -   ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
> > +   ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, p);
> > break;
> > case V4L2_BUF_TYPE_SDR_CAPTURE:
> > if (unlikely(!ops->vidioc_enum_fmt_sdr_cap))
> > break;
> > -   ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
> > +   ret = ops->vidioc_enum_fmt_sdr_cap(file, fh, p);
> > break;
> > case V4L2_BUF_TYPE_SDR_OUTPUT:
> > if (unlikely(!ops->vidioc_enum_fmt_sdr_out))
> > break;
> > -   ret = ops->vidioc_enum_fmt_sdr_out(file, fh, arg);
> > +   ret = ops->vidioc_enum_fmt_sdr_out(file, fh, p);
> > break;
> > case V4L2_BUF_TYPE_META_CAPTURE:
> > if (unlikely(!ops->vidioc_enum_fmt_meta_cap))
> > break;
> > -   ret = ops->vidioc_enum_fmt_meta_cap(file, fh, arg);
> > + 

Re: [PATCH v14 24/36] videobuf2-v4l2: Lock the media request for update for QBUF

[Hans Verkuil]

On 21/05/18 10:54, Sakari Ailus wrote:
> Lock the media request for updating on QBUF IOCTL using
> media_request_lock_for_update().
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c 
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 0a68b19b40da7..8b390960ca671 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -398,12 +398,13 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue 
> *q, struct media_device *md
>   if (IS_ERR(req)) {
>   dprintk(1, "%s: invalid request_fd\n", opname);
>   return PTR_ERR(req);
> - }
> -
> - if (atomic_read(>state) != MEDIA_REQUEST_STATE_IDLE) {
> - dprintk(1, "%s: request is not idle\n", opname);
> - media_request_put(req);
> - return -EBUSY;
> + } else {
> + ret = media_request_lock_for_update(req);
> + if (ret) {
> + media_request_put(req);
> + dprintk(1, "%s: request %d busy\n", opname, 
> b->request_fd);
> + return PTR_ERR(req);
> + }
>   }
>  
>   *p_req = req;
> @@ -683,8 +684,10 @@ int vb2_qbuf(struct vb2_queue *q, struct media_device 
> *mdev,
>   if (ret)
>   return ret;
>   ret = vb2_core_qbuf(q, b->index, b, req);
> - if (req)
> + if (req) {
> + media_request_unlock_for_update(req);
>   media_request_put(req);
> + }
>   return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);
> 

The media_request_(un)lock_for_update calls shouldn't be done here, instead they
should happen in videobuf2-core.c in vb2_core_qbuf, right before and after the
call to media_request_object_bind().

The atomic_read in the original patch in vb2_queue_or_prepare_buf() was there as
an early sanity check. The call to media_request_object_bind() is where the real
check for the request state takes place.

Regards,

Hans

[PATCH v3 1/7] media: imx274: initialize format before v4l2 controls

[Luca Ceresoli]

The current probe function calls v4l2_ctrl_handler_setup() before
initializing the format info. This triggers call paths such as:
imx274_probe -> v4l2_ctrl_handler_setup -> imx274_s_ctrl ->
imx274_set_exposure, where priv->mode_index is accessed before being
assigned.

This is wrong but does not trigger a visible bug because priv is
zero-initialized and 0 is the default value for priv->mode_index. But
this would become a crash in follow-up commits when mode_index is
replaced by a pointer that must always be valid.

Fix the bug before it shows up by initializing struct members early.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v2 -> v3: nothing

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 63fb94e7da37..8a8a11b8d75d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1632,6 +1632,16 @@ static int imx274_probe(struct i2c_client *client,
 
mutex_init(>lock);
 
+   /* initialize format */
+   imx274->mode_index = IMX274_MODE_3840X2160;
+   imx274->format.width = imx274_formats[0].size.width;
+   imx274->format.height = imx274_formats[0].size.height;
+   imx274->format.field = V4L2_FIELD_NONE;
+   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
+   imx274->frame_interval.numerator = 1;
+   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+
/* initialize regmap */
imx274->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(imx274->regmap)) {
@@ -1720,16 +1730,6 @@ static int imx274_probe(struct i2c_client *client,
goto err_ctrls;
}
 
-   /* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->format.width = imx274_formats[0].size.width;
-   imx274->format.height = imx274_formats[0].size.height;
-   imx274->format.field = V4L2_FIELD_NONE;
-   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
-   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-   imx274->frame_interval.numerator = 1;
-   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
-
/* load default control values */
ret = imx274_load_default(imx274);
if (ret) {
-- 
2.7.4

[PATCH v3 3/7] media: imx274: get rid of mode_index

[Luca Ceresoli]

After restructuring struct imx274_frmfmt, the mode_index field is
still in use only for two dev_dbg() calls in imx274_s_stream(). Let's
remove it and avoid duplicated information.

Replacing the first usage requires some rather annoying but trivial
pointer math. The other one can be removed entirely since it would
print the same value anyway.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v2 -> v3:
 - really fix dev_dbg() format mismatch warning for both 32 and 64 bit

Changed v1 -> v2:
 - add "media: " prefix to commit message
 - fix dev_dbg() format mismatch warning
   ("warning: format ‘%ld’ expects argument of type ‘long int’, but argument 6 
has type ‘int’")
 - slightly improve commit message
---
 drivers/media/i2c/imx274.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2ec31ae4e60d..f075715ffced 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -553,8 +553,6 @@ struct imx274_ctrls {
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
  * @mode: Parameters for the selected readout mode
- *(points to imx274_formats[mode_index])
- * @mode_index: Resolution mode index
  */
 struct stimx274 {
struct v4l2_subdev sd;
@@ -567,7 +565,6 @@ struct stimx274 {
struct gpio_desc *reset_gpio;
struct mutex lock; /* mutex lock for operations */
const struct imx274_frmfmt *mode;
-   u32 mode_index;
 };
 
 /*
@@ -880,7 +877,6 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
index = 0;
}
 
-   imx274->mode_index = index;
imx274->mode = _formats[index];
 
if (fmt->width > IMX274_MAX_WIDTH)
@@ -1028,8 +1024,9 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
struct stimx274 *imx274 = to_imx274(sd);
int ret = 0;
 
-   dev_dbg(>client->dev, "%s : %s, mode index = %d\n", __func__,
-   on ? "Stream Start" : "Stream Stop", imx274->mode_index);
+   dev_dbg(>client->dev, "%s : %s, mode index = %td\n", __func__,
+   on ? "Stream Start" : "Stream Stop",
+   imx274->mode - _formats[0]);
 
mutex_lock(>lock);
 
@@ -1068,8 +1065,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
}
 
mutex_unlock(>lock);
-   dev_dbg(>client->dev,
-   "%s : Done: mode = %d\n", __func__, imx274->mode_index);
+   dev_dbg(>client->dev, "%s : Done\n", __func__);
return 0;
 
 fail:
@@ -1625,8 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->mode = _formats[imx274->mode_index];
+   imx274->mode = _formats[IMX274_MODE_3840X2160];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v3 7/7] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

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.

Also change the names in enum imx274_mode from being resolution-based
to being binning-based (e.g. from IMX274_MODE_1920X1080 to
IMX274_MODE_BINNING_2_1). 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 any misunderstanding.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v2 -> v3:
 - Remove hard-coded HMAX reg setting from all modes, not only the
   first one. HMAX is always computed and set in s_stream now.
 - Reword commit log message.

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index e5ba19b97083..f5819ce99e98 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_MODEIMX274_MODE_3840X2160
+#define IMX274_DEFAULT_MODEIMX274_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_LSB 0x300C /* SHR */
 #define IMX274_SVR_REG_MSB 0x300F /* SVR */
 #define IMX274_SVR_REG_LSB 0x300E /* SVR */
+#define IMX274_HTRIM_EN_REG0x3037
+#define IMX274_HTRIM_START_REG_LSB 0x3038
+#define IMX274_HTRIM_START_REG_MSB 0x3039
+#define IMX274_HTRIM_END_REG_LSB   0x303A
+#define IMX274_HTRIM_END_REG_MSB   0x303B
+#define IMX274_VWIDCUTEN_REG   0x30DD
+#define IMX274_VWIDCUT_REG_LSB 0x30DE
+#define IMX274_VWIDCUT_REG_MSB 0x30DF
+#define IMX274_VWINPOS_REG_LSB 0x30E0
+#define IMX274_VWINPOS_REG_MSB 0x30E1
+#define IMX274_WRITE_VSIZE_REG_LSB 0x3130
+#define IMX274_WRITE_VSIZE_REG_MSB 0x3131
+#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, 0x06},
-   {0x30df, 0x00},
-   {0x30e0, 0x12},
-   {0x30e1, 0x00},
-   {0x3037, 0x01}, /* to crop to 3840 */
-   {0x3038, 0x0c},
-   {0x3039, 0x00},
-   {0x303a, 0x0c},
-   {0x303b, 0x0f},
 
{0x30EE, 0x01},
-   {0x3130, 0x86},
-   {0x3131, 0x08},
-   {0x3132, 0x7E},
-   {0x3133, 0x08},
{0x3342, 0x0A},
{0x3343, 0x00},
{0x3344, 0x16},
@@ -273,32 +271,14 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] 
= {
{0x3004, 0x02},
{0x3005, 0x21},
{0x3006, 0x00},
-   {0x3007, 0x11},
+   {0x3007, 0xb1},
 
{0x3018, 0xA2}, /* output XVS, HVS */
 
{0x306B, 0x05},
{0x30E2, 0x02},
 
-   {0x30F6, 0x04}, /* HMAX, 260 */
-   {0x30F7, 0x01}, /* HMAX */
-
-   {0x30dd, 0x01}, /* to crop to 1920x1080 */
-   {0x30de, 0x05},
-   {0x30df, 

[PATCH v3 4/7] media: imx274: actually use IMX274_DEFAULT_MODE

[Luca Ceresoli]

IMX274_DEFAULT_MODE is defined but not used. Start using it, so the
default can be more easily changed without digging into the code.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index f075715ffced..ceeec97cd330 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1621,7 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode = _formats[IMX274_MODE_3840X2160];
+   imx274->mode = _formats[IMX274_DEFAULT_MODE];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v3 5/7] media: imx274: simplify imx274_write_table()

[Luca Ceresoli]

imx274_write_table() is a mere wrapper (and the only user) to
imx274_regmap_util_write_table_8(). Remove this useless indirection by
merging the two functions into one.

Also get rid of the wait_ms_addr and end_addr parameters since it does
not make any sense to give them any values other than
IMX274_TABLE_WAIT_MS and IMX274_TABLE_END.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v2 -> v3: nothing

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index ceeec97cd330..48343c2ade83 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,20 +597,18 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
- * imx274_regmap_util_write_table_8 - Function for writing register table
- * @regmap: Pointer to device reg map structure
- * @table: Table containing register values
- * @wait_ms_addr: Flag for performing delay
- * @end_addr: Flag for incating end of table
+ * Writing a register table
+ *
+ * @priv: Pointer to device
+ * @table: Table containing register values (with optional delays)
  *
  * This is used to write register table into sensor's reg map.
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_regmap_util_write_table_8(struct regmap *regmap,
-   const struct reg_8 table[],
-   u16 wait_ms_addr, u16 end_addr)
+static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
 {
+   struct regmap *regmap = priv->regmap;
int err = 0;
const struct reg_8 *next;
u8 val;
@@ -622,8 +620,8 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
 
for (next = table;; next++) {
if ((next->addr != range_start + range_count) ||
-   (next->addr == end_addr) ||
-   (next->addr == wait_ms_addr) ||
+   (next->addr == IMX274_TABLE_END) ||
+   (next->addr == IMX274_TABLE_WAIT_MS) ||
(range_count == max_range_vals)) {
if (range_count == 1)
err = regmap_write(regmap,
@@ -642,10 +640,10 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
range_count = 0;
 
/* Handle special address values */
-   if (next->addr == end_addr)
+   if (next->addr == IMX274_TABLE_END)
break;
 
-   if (next->addr == wait_ms_addr) {
+   if (next->addr == IMX274_TABLE_WAIT_MS) {
msleep_range(next->val);
continue;
}
@@ -692,12 +690,6 @@ static inline int imx274_write_reg(struct stimx274 *priv, 
u16 addr, u8 val)
return err;
 }
 
-static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
-{
-   return imx274_regmap_util_write_table_8(priv->regmap,
-   table, IMX274_TABLE_WAIT_MS, IMX274_TABLE_END);
-}
-
 /*
  * Set mode registers to start stream.
  * @priv: Pointer to device structure
-- 
2.7.4

[PATCH v3 0/7] media: imx274: cleanups, improvements and SELECTION API support

[Luca Ceresoli]

Hi,

this patchset introduces cropping support for the Sony IMX274 sensor
using the SELECTION API. 

v3 has a few minor improvements over v2. It also removes the first 6
patches, already applied on the media_tree master branch.

After v2 there has been a short discussion with Sakari Ailus on how
cropping should be configured from userspace [0]. That discussion has
gone stale before I could understand the idea behind the changes
suggested by Sakari, so I'm sending the most up-to-date version of the
old implementation to give it a new spin. I'll be glad to rework my
patch when things are clearer.

Patches 1-5 are overall improvements and restructuring, mostly useful
to implement the SELECTION API in a clean way.

Patch 6 introduces a helper to allow setting many registers computed
at runtime in a straightforward way. This would not have been very
useful before because all long register write sequences came from
const tables, but it's definitely a must for the cropping code where
several register values are computed at runtime.

Patch 7 implements cropping in the set_selection pad operation. On the
v4l2 side there is nothing special. The most tricky part was
respecting all the device constraints on the horizontal crop.

Usage examples:

 * Capture the entire 4K area, downscaled to 1080p with 2:1 binning:
   media-ctl -V '"IMX274":0[crop:(0,0)/3840x2160]'
   media-ctl -V '"IMX274":0[fmt:SRGGB8_1X8/1920x1080 field:none]'

 * Capture the central 1080p area (no binning):
   media-ctl -V '"IMX274":0[crop:(960,540)/1920x1080]'
   (this also sets the fmt to 1920x1080)  

Regards,
Luca

[0] https://www.spinics.net/lists/kernel/msg2787725.html


Luca Ceresoli (7):
  media: imx274: initialize format before v4l2 controls
  media: imx274: consolidate per-mode data in imx274_frmfmt
  media: imx274: get rid of mode_index
  media: imx274: actually use IMX274_DEFAULT_MODE
  media: imx274: simplify imx274_write_table()
  media: imx274: add helper function to fill a reg_8 table chunk
  media: imx274: add SELECTION support for cropping

 drivers/media/i2c/imx274.c | 552 +++--
 1 file changed, 332 insertions(+), 220 deletions(-)

-- 
2.7.4

[PATCH v3 2/7] media: imx274: consolidate per-mode data in imx274_frmfmt

[Luca Ceresoli]

Data about the implemented readout modes is partially stored in
imx274_formats[], the rest is scattered in several arrays. The latter
are then accessed using the mode index, e.g.:

  min_frame_len[priv->mode_index]

Consolidate all these data in imx274_formats[], and store a pointer to
the selected mode (i.e. imx274_formats[priv->mode_index]) in the main
device struct. This way code to use these data becomes more readable,
e.g.:

  priv->mode->min_frame_len

This removes lots of scaffolding code and keeps data about each mode
in a unique place.

Also remove a parameter to imx274_mode_regs() that is now unused.

While this adds the mode pointer to the device struct, it does not
remove the mode_index from it because mode_index is still used in two
dev_dbg() calls.  This will be handled in a follow-up commit.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v2 -> v3: nothing

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 8a8a11b8d75d..2ec31ae4e60d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -147,10 +147,28 @@ enum imx274_mode {
 };
 
 /*
- * imx274 format related structure
+ * Parameters for each imx274 readout mode.
+ *
+ * These are the values to configure the sensor in one of the
+ * implemented modes.
+ *
+ * @size: recommended recording pixels
+ * @init_regs: registers to initialize the mode
+ * @min_frame_len: Minimum frame length for each mode (see "Frame Rate
+ * Adjustment (CSI-2)" in the datasheet)
+ * @min_SHR: Minimum SHR register value (see "Shutter Setting (CSI-2)" in the
+ *   datasheet)
+ * @max_fps: Maximum frames per second
+ * @nocpiop: Number of clocks per internal offset period (see "Integration Time
+ *   in Each Readout Drive Mode (CSI-2)" in the datasheet)
  */
 struct imx274_frmfmt {
struct v4l2_frmsize_discrete size;
+   const struct reg_8 *init_regs;
+   int min_frame_len;
+   int min_SHR;
+   int max_fps;
+   int nocpiop;
 };
 
 /*
@@ -476,58 +494,35 @@ static const struct reg_8 imx274_tp_regs[] = {
{IMX274_TABLE_END, 0x00}
 };
 
-static const struct reg_8 *mode_table[] = {
-   [IMX274_MODE_3840X2160] = imx274_mode1_3840x2160_raw10,
-   [IMX274_MODE_1920X1080] = imx274_mode3_1920x1080_raw10,
-   [IMX274_MODE_1280X720]  = imx274_mode5_1280x720_raw10,
-};
-
-/*
- * imx274 format related structure
- */
+/* nocpiop happens to be the same number for the implemented modes */
 static const struct imx274_frmfmt imx274_formats[] = {
-   { {3840, 2160} },
-   { {1920, 1080} },
-   { {1280,  720} },
-};
-
-/*
- * minimal frame length for each mode
- * refer to datasheet section "Frame Rate Adjustment (CSI-2)"
- */
-static const int min_frame_len[] = {
-   4550, /* mode 1, 4K */
-   2310, /* mode 3, 1080p */
-   2310 /* mode 5, 720p */
-};
-
-/*
- * minimal numbers of SHR register
- * refer to datasheet table "Shutter Setting (CSI-2)"
- */
-static const int min_SHR[] = {
-   12, /* mode 1, 4K */
-   8, /* mode 3, 1080p */
-   8 /* mode 5, 720p */
-};
-
-static const int max_frame_rate[] = {
-   60, /* mode 1 , 4K */
-   120, /* mode 3, 1080p */
-   120 /* mode 5, 720p */
-};
-
-/*
- * Number of clocks per internal offset period
- * a constant based on mode
- * refer to section "Integration Time in Each Readout Drive Mode (CSI-2)"
- * in the datasheet
- * for the implemented 3 modes, it happens to be the same number
- */
-static const int nocpiop[] = {
-   112, /* mode 1 , 4K */
-   112, /* mode 3, 1080p */
-   112 /* mode 5, 720p */
+   {
+   /* mode 1, 4K */
+   .size = {3840, 2160},
+   .init_regs = imx274_mode1_3840x2160_raw10,
+   .min_frame_len = 4550,
+   .min_SHR = 12,
+   .max_fps = 60,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 3, 1080p */
+   .size = {1920, 1080},
+   .init_regs = imx274_mode3_1920x1080_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 5, 720p */
+   .size = {1280, 720},
+   .init_regs = imx274_mode5_1280x720_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
 };
 
 /*
@@ -557,6 +552,8 @@ struct imx274_ctrls {
  * @regmap: Pointer to regmap structure
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
+ * @mode: Parameters for the selected readout mode
+ *(points to 

[PATCH v3 6/7] media: imx274: add helper function to fill a reg_8 table chunk

[Luca Ceresoli]

Tables of struct reg_8 are used to simplify multi-byte register
assignment. However filling these snippets with values computed at
runtime is currently implemented by very similar functions doing the
needed shift & mask manipulation.

Replace all those functions with a unique helper function to fill
reg_8 tables in a simple and clean way.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v2 -> v3:
 - minor reformatting in prepare_reg() documentation

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 48343c2ade83..e5ba19b97083 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,6 +597,58 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
+ * Fill consecutive reg_8 items in order to set a multibyte register.
+ *
+ * The sensor has many 2-bytes registers and a 3-byte register. This
+ * simplifies code to set them by filling consecutive entries of a
+ * struct reg_8 table with the data to set a register.
+ *
+ * The number of table entries that is filled is the minimum needed
+ * for the given number of bits (i.e. nbits / 8, rounded up). If nbits
+ * is not a multiple of 8, extra bits in the most significant byte are
+ * zeroed.
+ *
+ * Example:
+ *   Calling prepare_reg([10], 0x3000, 0xcafe, 12) will set:
+ *   regs[10] = { .addr = 0x3000, .val = 0xfe }
+ *   regs[11] = { .addr = 0x3001, .val = 0x0a }
+ *
+ * @table_base: Pointer to the first reg_8 struct to be filled.  The
+ *  following entries will also be set, make sure they are
+ *  properly allocated!
+ * @addr_lsb: Address of the LSB register.  Other registers must be
+ *consecutive, least-to-most significant.
+ * @value: Value to be written to the register.
+ * @nbits: Number of bits to write (range: [1..24])
+ */
+static void prepare_reg(struct reg_8 *table_base,
+   u16 addr_lsb,
+   u32 value,
+   size_t nbits)
+{
+   struct reg_8 *cmd = table_base;
+   u16 addr = addr_lsb;
+   size_t bits; /* how many bits at this round */
+
+   WARN_ON(nbits > 24);
+
+   if (nbits > 24)
+   nbits = 24;
+
+   while (nbits > 0) {
+   bits = min_t(size_t, 8, nbits);
+
+   cmd->addr = addr;
+   cmd->val = value & ((1 << bits) - 1);
+
+   nbits -= bits;
+   value >>= 8;
+   addr++;
+   cmd++;
+   }
+}
+
+/*
  * Writing a register table
  *
  * @priv: Pointer to device
@@ -1163,15 +1215,6 @@ static int imx274_set_digital_gain(struct stimx274 
*priv, u32 dgain)
reg_val & IMX274_MASK_LSB_4_BITS);
 }
 
-static inline void imx274_calculate_gain_regs(struct reg_8 regs[2], u16 gain)
-{
-   regs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;
-   regs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;
-
-   (regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;
-   (regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_gain - Function called when setting gain
  * @priv: Pointer to device structure
@@ -1229,7 +1272,7 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
if (gain_reg > IMX274_GAIN_REG_MAX)
gain_reg = IMX274_GAIN_REG_MAX;
 
-   imx274_calculate_gain_regs(reg_list, (u16)gain_reg);
+   prepare_reg(reg_list, IMX274_ANALOG_GAIN_ADDR_LSB, gain_reg, 11);
 
for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
err = imx274_write_reg(priv, reg_list[i].addr,
@@ -1258,16 +1301,6 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
return err;
 }
 
-static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
-u32 coarse_time)
-{
-   regs->addr = IMX274_SHR_REG_MSB;
-   regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
-   & IMX274_MASK_LSB_8_BITS;
-   (regs + 1)->addr = IMX274_SHR_REG_LSB;
-   (regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_coarse_time - Function called when setting SHR value
  * @priv: Pointer to device structure
@@ -1292,7 +1325,7 @@ static int imx274_set_coarse_time(struct stimx274 *priv, 
u32 *val)
goto fail;
 
/* prepare SHR registers */
-   imx274_calculate_coarse_time_regs(reg_list, coarse_time);
+   prepare_reg(reg_list, IMX274_SHR_REG_LSB, coarse_time, 16);
 
/* write to SHR registers */
for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
@@ -1429,19 +1462,6 @@ static int imx274_set_test_pattern(struct stimx274 
*priv, int val)
return err;
 }
 

Re: [PATCH v14 06/36] media-request: Add support for updating request objects optimally

[Hans Verkuil]

On 21/05/18 10:54, Sakari Ailus wrote:
> Add a new request state (UPDATING) as well as a count for updating the
> request objects. This way, several updates may take place simultaneously
> without affecting each other. The drivers (as well as frameworks) still
> must serialise access to their own data structures; what is guaranteed by
> the new state is simply correct and optimal handling of requests.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/media-request.c |  3 ++-
>  include/media/media-request.h | 53 
> +++
>  2 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> index a1576cf528605..03e74d72241a0 100644
> --- a/drivers/media/media-request.c
> +++ b/drivers/media/media-request.c
> @@ -22,6 +22,7 @@ static const char * const request_state[] = {
>   [MEDIA_REQUEST_STATE_QUEUED] = "queued",
>   [MEDIA_REQUEST_STATE_COMPLETE]   = "complete",
>   [MEDIA_REQUEST_STATE_CLEANING]   = "cleaning",
> + [MEDIA_REQUEST_STATE_UPDATING]   = "updating",
>  };
>  
>  static const char *
> @@ -385,7 +386,7 @@ int media_request_object_bind(struct media_request *req,
>  
>   spin_lock_irqsave(>lock, flags);
>  
> - if (WARN_ON(req->state != MEDIA_REQUEST_STATE_IDLE))
> + if (WARN_ON(req->state != MEDIA_REQUEST_STATE_UPDATING))
>   goto unlock;

I think the 'obj->req = req' etc. lines just before the spin_lock_irqsave 
should be
moved to after this 'if'. Otherwise there is an unexpected side-effect of 
calling
this function when it returns EBUSY.

>  
>   list_add_tail(>list, >objects);
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index e175538d3c669..42cc6e7f6e532 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -28,6 +28,9 @@
>   * @MEDIA_REQUEST_STATE_QUEUED:  Queued
>   * @MEDIA_REQUEST_STATE_COMPLETE:Completed, the request is done
>   * @MEDIA_REQUEST_STATE_CLEANING:Cleaning, the request is being re-inited
> + * @MEDIA_REQUEST_STATE_UPDATING:The request is being updated, i.e.
> + *   request objects are being added,
> + *   modified or removed
>   */
>  enum media_request_state {
>   MEDIA_REQUEST_STATE_IDLE,
> @@ -35,6 +38,7 @@ enum media_request_state {
>   MEDIA_REQUEST_STATE_QUEUED,
>   MEDIA_REQUEST_STATE_COMPLETE,
>   MEDIA_REQUEST_STATE_CLEANING,
> + MEDIA_REQUEST_STATE_UPDATING,
>  };
>  
>  struct media_request_object;
> @@ -56,6 +60,7 @@ struct media_request {
>   struct kref kref;
>   char debug_str[TASK_COMM_LEN + 11];
>   enum media_request_state state;
> + refcount_t updating_count;
>   struct list_head objects;
>   unsigned int num_incomplete_objects;
>   struct wait_queue_head poll_wait;
> @@ -65,6 +70,54 @@ struct media_request {
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  
>  /**
> + * media_request_lock_for_update - Lock the request for updating its objects
> + *
> + * @req: The media request
> + *
> + * Use before updating a request, i.e. adding, modifying or removing a 
> request
> + * object in it. A reference to the request must be held during the update. 
> This
> + * usually takes place automatically through a file handle. Use
> + * @media_request_unlock_for_update when done.
> + */
> +static inline int __must_check
> +media_request_lock_for_update(struct media_request *req)
> +{
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(>lock, flags);
> + if (req->state == MEDIA_REQUEST_STATE_IDLE ||
> + req->state == MEDIA_REQUEST_STATE_UPDATING) {
> + req->state = MEDIA_REQUEST_STATE_UPDATING;
> + refcount_inc(>updating_count);
> + } else {
> + ret = -EBUSY;
> + }
> + spin_unlock_irqrestore(>lock, flags);
> +
> + return ret;
> +}
> +
> +/**
> + * media_request_unlock_for_update - Unlock a request previously locked for
> + *update
> + *
> + * @req: The media request
> + *
> + * Unlock a request that has previously been locked using
> + * @media_request_lock_for_update.
> + */
> +static inline void media_request_unlock_for_update(struct media_request *req)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
> + if (refcount_dec_and_test(>updating_count))
> + req->state = MEDIA_REQUEST_STATE_IDLE;
> + spin_unlock_irqrestore(>lock, flags);
> +}

This is a very nice solution. Thank you!

Regards,

Hans

> +
> +/**
>   * media_request_get - Get the media request
>   *
>   * @req: The request
> 

Re: [PATCH] media: uvcvideo: Support realtek's UVC 1.5 device

[Ana Guerrero Lopez]

> > commit a9c002732695eab2096580a0d1a1687bc2f95928
> > Author: ming_qian 
> > Date:   Wed May 9 10:13:08 2018 +0800
> > 
> > media: uvcvideo: Support UVC 1.5 video probe & commit controls
> > 
> > The length of UVC 1.5 video control is 48, and it is 34 for UVC 1.1.
> > Change it to 48 for UVC 1.5 device, and the UVC 1.5 device can be
> > recognized.
> > 
> > More changes to the driver are needed for full UVC 1.5 compatibility.
> > However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have been
> > reported to work well.
> > 
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: ming_qian 
> > [Factor out code to helper function, update size checks]
> > Signed-off-by: Laurent Pinchart 
> 
> I tested this new patch and it works well.
> 
> Tested-by: Kai-Heng Feng 

I tested it as well and it works well.

Tested-by: Ana Guerrero Lopez 


> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c
> > index eb9e04a59427..285b0e813b9d 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -207,14 +207,27 @@ static void uvc_fixup_video_ctrl(struct
> > uvc_streaming *stream,
> > }
> >  }
> > 
> > +static size_t uvc_video_ctrl_size(struct uvc_streaming *stream)
> > +{
> > +   /*
> > +* Return the size of the video probe and commit controls, which depends
> > +* on the protocol version.
> > +*/
> > +   if (stream->dev->uvc_version < 0x0110)
> > +   return 26;
> > +   else if (stream->dev->uvc_version < 0x0150)
> > +   return 34;
> > +   else
> > +   return 48;
> > +}
> > +
> >  static int uvc_get_video_ctrl(struct uvc_streaming *stream,
> > struct uvc_streaming_control *ctrl, int probe, u8 query)
> >  {
> > +   u16 size = uvc_video_ctrl_size(stream);
> > u8 *data;
> > -   u16 size;
> > int ret;
> > 
> > -   size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
> > if ((stream->dev->quirks & UVC_QUIRK_PROBE_DEF) &&
> > query == UVC_GET_DEF)
> > return -EIO;
> > @@ -271,7 +284,7 @@ static int uvc_get_video_ctrl(struct uvc_streaming
> > *stream,
> > ctrl->dwMaxVideoFrameSize = get_unaligned_le32([18]);
> > ctrl->dwMaxPayloadTransferSize = get_unaligned_le32([22]);
> > 
> > -   if (size == 34) {
> > +   if (size >= 34) {
> > ctrl->dwClockFrequency = get_unaligned_le32([26]);
> > ctrl->bmFramingInfo = data[30];
> > ctrl->bPreferedVersion = data[31];
> > @@ -300,11 +313,10 @@ static int uvc_get_video_ctrl(struct uvc_streaming
> > *stream,
> >  static int uvc_set_video_ctrl(struct uvc_streaming *stream,
> > struct uvc_streaming_control *ctrl, int probe)
> >  {
> > +   u16 size = uvc_video_ctrl_size(stream);
> > u8 *data;
> > -   u16 size;
> > int ret;
> > 
> > -   size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
> > data = kzalloc(size, GFP_KERNEL);
> > if (data == NULL)
> > return -ENOMEM;
> > @@ -321,7 +333,7 @@ static int uvc_set_video_ctrl(struct uvc_streaming
> > *stream,
> > put_unaligned_le32(ctrl->dwMaxVideoFrameSize, [18]);
> > put_unaligned_le32(ctrl->dwMaxPayloadTransferSize, [22]);
> > 
> > -   if (size == 34) {
> > +   if (size >= 34) {
> > put_unaligned_le32(ctrl->dwClockFrequency, [26]);
> > data[30] = ctrl->bmFramingInfo;
> > data[31] = ctrl->bPreferedVersion;
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart

Re: [PATCH 2/2] media: platform: add driver for TI SCAN921226H video deserializer

[Philipp Zabel]

Hi Jan,

Hans just pointed out a few issues in my video-mux compliance patch [1]
that also apply to this driver, see below.

[1] v1: https://patchwork.linuxtv.org/patch/49827/
v2: https://patchwork.linuxtv.org/patch/49839/

On Fri, 2018-05-04 at 14:49 +0200, Jan Luebbe wrote:
[...]
> diff --git a/drivers/media/platform/scan921226h.c 
> b/drivers/media/platform/scan921226h.c
> new file mode 100644
> index ..59fcd55ceaa2
> --- /dev/null
> +++ b/drivers/media/platform/scan921226h.c
[...]
> +static int video_des_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *sdformat)
> +{
> + struct video_des *vdes = v4l2_subdev_to_video_des(sd);
> + struct v4l2_mbus_framefmt *mbusformat;
> + struct media_pad *pad = >pads[sdformat->pad];
> +
> + mbusformat = __video_des_get_pad_format(sd, cfg, sdformat->pad,
> + sdformat->which);
> + if (!mbusformat)
> + return -EINVAL;
> +
> + mutex_lock(>lock);
> +
> + /* Source pad mirrors sink pad, no limitations on sink pads */
> + if ((pad->flags & MEDIA_PAD_FL_SOURCE)) {
> + sdformat->format = vdes->format_mbus;
> + } else {
> + /* any sizes are allowed */
> + v4l_bound_align_image(
> + >format.width, 1, UINT_MAX-1, 0,
> + >format.height, 1, UINT_MAX-1, 0,

Reduce from UINT_MAX-1 to 65536 to avoid potential overflow issues.

> + 0);
> + if (sdformat->format.field == V4L2_FIELD_ANY)
> + sdformat->format.field = V4L2_FIELD_NONE;
> + switch (sdformat->format.code) {
> + /* only 8 bit formats are supported */
> + case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
> + case MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE:
[...]
> + case MEDIA_BUS_FMT_JPEG_1X8:
> + case MEDIA_BUS_FMT_S5C_UYVY_JPEG_1X8:
> + break;
> + default:
> + sdformat->format.code = MEDIA_BUS_FMT_Y8_1X8;

A
break;
should be added here.

> + }
> + }
> +
> + *mbusformat = sdformat->format;
> +
> + mutex_unlock(>lock);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops video_des_pad_ops = {
> + .get_fmt = video_des_get_format,
> + .set_fmt = video_des_set_format,
> +};
> +
> +static int video_des_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct video_des *vdes = v4l2_subdev_to_video_des(sd);
> + struct v4l2_mbus_framefmt *format;
> +
> + mutex_lock(>lock);
> +
> + format = v4l2_subdev_get_try_format(sd, fh->pad, 0);
> + *format = vdes->format_mbus;
> + format = v4l2_subdev_get_try_format(sd, fh->pad, 1);
> + *format = vdes->format_mbus;
> +
> + mutex_unlock(>lock);
> +
> + return 0;
> +}

This should be done in the .init_cfg pad op.

> +
> +static const struct v4l2_subdev_internal_ops video_des_subdev_internal_ops = 
> {
> + .open = video_des_open,
> +};

Internal ops can then be dropped.

regards
Philipp

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Daniel Mack]

Hi Maxime,

On Tuesday, May 22, 2018 09:54 PM, Maxime Ripard wrote:

On Mon, May 21, 2018 at 09:39:02AM +0200, Maxime Ripard wrote:

On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:



This set of patches is also not working for my MIPI platform (mine has
a 12 MHz external clock). I am pretty sure is isn't working because it
does not include the following, which my tests have found to be
necessary:

1) Setting pclk period reg in order to correct DPHY timing.
2) Disabling of MIPI lanes when streaming not enabled.
3) setting mipi_div to 1 when the scaler is disabled
4) Doubling ADC clock on faster resolutions.


Yeah, I left them out because I didn't think this was relevant to this
patchset but should come as future improvements. However, given that
it works with the parallel bus, maybe the two first are needed when
adjusting the rate.


I've checked for the pclk period, and it's hardcoded to the same value
all the time, so I guess this is not the reason it doesn't work on
MIPI CSI anymore.

Daniel, could you test:
http://code.bulix.org/ki6kgz-339327?raw


[Note that there's a missing parenthesis in this snippet]

Hmm, no, that doesn't change anything. Streaming doesn't work here, even 
if I move ov5640_load_regs() before any other initialization.


One of my test setup is the following gst pipeline:

  gst-launch-1.0\
v4l2src device=/dev/video0 ! \
videoconvert ! \
video/x-raw,format=UYVY,width=1920,height=1080 ! \
glimagesink

With the pixel clock hard-coded to 16660 in qcom camss, the setup 
works on 4.14, but as I said, it broke already before this series with 
5999f381e023 ("media: ov5640: Add horizontal and vertical

totals").

Frankly, my understanding of these chips is currently limited, so I 
don't really know where to start digging. It seems clear though that the 
timing registers setup is necessary for other register writes to succeed.


Can I help in any other way?


Thanks for all your efforts,
Daniel

Re: [PATCH] media: video-mux: fix compliance failures

[Philipp Zabel]

On Wed, 2018-05-23 at 10:53 +0200, Hans Verkuil wrote:
> On 23/05/18 10:47, Philipp Zabel wrote:
> > Hi Hans,
> > 
> > thank you for the review comments.
> > 
> > On Tue, 2018-05-22 at 19:47 +0200, Hans Verkuil wrote:
> > > On 22/05/18 18:29, Philipp Zabel wrote:
> > > > Limit frame sizes to the [1, UINT_MAX-1] interval, media bus formats to
> > > > the available list of formats, and initialize pad and try formats.
> > > > 
> > > > Reported-by: Rui Miguel Silva 
> > > > Signed-off-by: Philipp Zabel 
> > > > ---
> > > >  drivers/media/platform/video-mux.c | 110 +
> > > >  1 file changed, 110 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/platform/video-mux.c 
> > > > b/drivers/media/platform/video-mux.c
> > > > index 1fb887293337..ade1dae706aa 100644
> > > > --- a/drivers/media/platform/video-mux.c
> > > > +++ b/drivers/media/platform/video-mux.c
> > > > @@ -180,6 +180,87 @@ static int video_mux_set_format(struct v4l2_subdev 
> > > > *sd,
> > > > if (!source_mbusformat)
> > > > return -EINVAL;
> > > >  
> > > > +   /* No size limitations except V4L2 compliance requirements */
> > > > +   v4l_bound_align_image(>format.width, 1, UINT_MAX - 1, 
> > > > 0,
> > > > + >format.height, 1, UINT_MAX - 
> > > > 1, 0, 0);
> > > 
> > > This is a bit dubious. I would pick more realistic min/max values like 16 
> > > and
> > 
> > Why 16? A grayscale or RGB sensor could crop down to 1x1, see mt9v032
> > for example.
> 
> Was that ever tested? Just because the software allows it, doesn't mean it 
> actually
> works.

I don't know. I'll test this when I get access to a sensor that could
support such low cropping.

I'd just prefer this artificial limit not to be imposed by the generic
video mux driver, as a mux doesn't care about framing.

For example the i.MX media driver currently has an (also artificial)
limit to 16 pixel aligned frame sizes in the CSI subdev anyway.

regards
Philipp

Re: [PATCH 0/6] Fix issues reported by static analysis tool.

[Sakari Ailus]

Hi Pankaj,

On Wed, May 23, 2018 at 10:51:30AM +0530, Pankaj Bharadiya wrote:
> This patch series fixes some of the issues reported by static analysis
> tool.
> 
> Pankaj Bharadiya (6):
>   media: staging: atomisp: remove redundent check
>   media: staging: atomisp: Remove useless if statement
>   media: staging: atomisp: Remove useless "ifndef ISP2401"
>   media: staging: atomisp: Fix potential NULL pointer dereference
>   media: staging: atomisp: Fix potential NULL pointer dereference
>   media: staging: atomisp: Fix potential NULL pointer dereference
> 
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c   | 14 
> --
>  .../media/atomisp/pci/atomisp2/atomisp_compat_css20.c  |  2 --
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_csi2.c  |  8 
>  .../pci/atomisp2/css2400/camera/pipe/src/pipe_binarydesc.c |  3 ++-
>  .../staging/media/atomisp/pci/atomisp2/css2400/sh_css.c|  7 +--
>  5 files changed, 7 insertions(+), 27 deletions(-)
> 

I'm sorry to tell you that the atomisp driver was removed from the staging
tree some time ago.

-- 
Kind regards,

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

[PATCH v2] media: video-mux: fix compliance failures

[Philipp Zabel]

Limit frame sizes to the [1, 65536] interval, media bus formats to
the available list of formats, and initialize pad and try formats.

Reported-by: Rui Miguel Silva 
Signed-off-by: Philipp Zabel 
---
Changes since v1:
 - Limit to [1, 65536] instead of [1, UINT_MAX - 1]
 - Add missing break in default case
 - Use .init_cfg pad op instead of .open internal op
---
 drivers/media/platform/video-mux.c | 108 +
 1 file changed, 108 insertions(+)

diff --git a/drivers/media/platform/video-mux.c 
b/drivers/media/platform/video-mux.c
index 1fb887293337..d27cb42ce6b1 100644
--- a/drivers/media/platform/video-mux.c
+++ b/drivers/media/platform/video-mux.c
@@ -180,6 +180,88 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
if (!source_mbusformat)
return -EINVAL;
 
+   /* No size limitations except V4L2 compliance requirements */
+   v4l_bound_align_image(>format.width, 1, 65536, 0,
+ >format.height, 1, 65536, 0, 0);
+
+   /* All formats except LVDS and vendor specific formats are acceptable */
+   switch (sdformat->format.code) {
+   case MEDIA_BUS_FMT_RGB444_1X12:
+   case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
+   case MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE:
+   case MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE:
+   case MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE:
+   case MEDIA_BUS_FMT_RGB565_1X16:
+   case MEDIA_BUS_FMT_BGR565_2X8_BE:
+   case MEDIA_BUS_FMT_BGR565_2X8_LE:
+   case MEDIA_BUS_FMT_RGB565_2X8_BE:
+   case MEDIA_BUS_FMT_RGB565_2X8_LE:
+   case MEDIA_BUS_FMT_RGB666_1X18:
+   case MEDIA_BUS_FMT_RBG888_1X24:
+   case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
+   case MEDIA_BUS_FMT_BGR888_1X24:
+   case MEDIA_BUS_FMT_GBR888_1X24:
+   case MEDIA_BUS_FMT_RGB888_1X24:
+   case MEDIA_BUS_FMT_RGB888_2X12_BE:
+   case MEDIA_BUS_FMT_RGB888_2X12_LE:
+   case MEDIA_BUS_FMT_ARGB_1X32:
+   case MEDIA_BUS_FMT_RGB888_1X32_PADHI:
+   case MEDIA_BUS_FMT_RGB101010_1X30:
+   case MEDIA_BUS_FMT_RGB121212_1X36:
+   case MEDIA_BUS_FMT_RGB161616_1X48:
+   case MEDIA_BUS_FMT_Y8_1X8:
+   case MEDIA_BUS_FMT_UV8_1X8:
+   case MEDIA_BUS_FMT_UYVY8_1_5X8:
+   case MEDIA_BUS_FMT_VYUY8_1_5X8:
+   case MEDIA_BUS_FMT_YUYV8_1_5X8:
+   case MEDIA_BUS_FMT_YVYU8_1_5X8:
+   case MEDIA_BUS_FMT_UYVY8_2X8:
+   case MEDIA_BUS_FMT_VYUY8_2X8:
+   case MEDIA_BUS_FMT_YUYV8_2X8:
+   case MEDIA_BUS_FMT_YVYU8_2X8:
+   case MEDIA_BUS_FMT_Y10_1X10:
+   case MEDIA_BUS_FMT_UYVY10_2X10:
+   case MEDIA_BUS_FMT_VYUY10_2X10:
+   case MEDIA_BUS_FMT_YUYV10_2X10:
+   case MEDIA_BUS_FMT_YVYU10_2X10:
+   case MEDIA_BUS_FMT_Y12_1X12:
+   case MEDIA_BUS_FMT_UYVY12_2X12:
+   case MEDIA_BUS_FMT_VYUY12_2X12:
+   case MEDIA_BUS_FMT_YUYV12_2X12:
+   case MEDIA_BUS_FMT_YVYU12_2X12:
+   case MEDIA_BUS_FMT_UYVY8_1X16:
+   case MEDIA_BUS_FMT_VYUY8_1X16:
+   case MEDIA_BUS_FMT_YUYV8_1X16:
+   case MEDIA_BUS_FMT_YVYU8_1X16:
+   case MEDIA_BUS_FMT_YDYUYDYV8_1X16:
+   case MEDIA_BUS_FMT_UYVY10_1X20:
+   case MEDIA_BUS_FMT_VYUY10_1X20:
+   case MEDIA_BUS_FMT_YUYV10_1X20:
+   case MEDIA_BUS_FMT_YVYU10_1X20:
+   case MEDIA_BUS_FMT_VUY8_1X24:
+   case MEDIA_BUS_FMT_YUV8_1X24:
+   case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
+   case MEDIA_BUS_FMT_UYVY12_1X24:
+   case MEDIA_BUS_FMT_VYUY12_1X24:
+   case MEDIA_BUS_FMT_YUYV12_1X24:
+   case MEDIA_BUS_FMT_YVYU12_1X24:
+   case MEDIA_BUS_FMT_YUV10_1X30:
+   case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
+   case MEDIA_BUS_FMT_AYUV8_1X32:
+   case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
+   case MEDIA_BUS_FMT_YUV12_1X36:
+   case MEDIA_BUS_FMT_YUV16_1X48:
+   case MEDIA_BUS_FMT_UYYVYY16_0_5X48:
+   case MEDIA_BUS_FMT_JPEG_1X8:
+   case MEDIA_BUS_FMT_AHSV_1X32:
+   break;
+   default:
+   sdformat->format.code = MEDIA_BUS_FMT_Y8_1X8;
+   break;
+   }
+   if (sdformat->format.field == V4L2_FIELD_ANY)
+   sdformat->format.field = V4L2_FIELD_NONE;
+
mutex_lock(>lock);
 
/* Source pad mirrors active sink pad, no limitations on sink pads */
@@ -197,7 +279,27 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
return 0;
 }
 
+static int video_mux_init_cfg(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg)
+{
+   struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
+   struct v4l2_mbus_framefmt *mbusformat;
+   int i;
+
+   mutex_lock(>lock);
+
+   for (i = 0; i < sd->entity.num_pads; i++) {
+   mbusformat = v4l2_subdev_get_try_format(sd, cfg, i);
+   *mbusformat = vmux->format_mbus[i];
+   }
+
+   mutex_unlock(>lock);
+
+   return 0;
+}
+
 static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
+

Re: [PATCH v2] v4l: vsp1: Fix vsp1_regs.h license header

[Simon Horman]

On Wed, May 23, 2018 at 11:37:47AM +0300, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Wednesday, 23 May 2018 11:33:26 EEST Simon Horman wrote:
> > On Tue, May 22, 2018 at 01:04:56PM +0200, Geert Uytterhoeven wrote:
> > > On Tue, May 22, 2018 at 11:05 AM, Simon Horman  wrote:
> > >>> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> > >>> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> > >>> @@ -1,4 +1,4 @@
> > >>> -/* SPDX-License-Identifier: GPL-2.0 */
> > >>> +/* SPDX-License-Identifier: GPL-2.0+ */
> > >> 
> > >> While you are changing this line, I believe the correct format is
> > >> to use a '//' comment.
> > >> 
> > >> i.e.:
> > >> 
> > >> // SPDX-License-Identifier: GPL-2.0+
> > > 
> > > Not for C header files, only for C source files.
> > 
> > Wow!
> 
> Yes, it's a mess :-( The rationale is that the assembler doesn't support C++-
> style comments, so we need to use C-style comments in header files. We should 
> really have standardized usage of C-style comments everywhere, it makes no 
> sense to me.

I'm reading this email while standing on my head
and things make much more sense :)

Re: [PATCH] media: video-mux: fix compliance failures

[Hans Verkuil]

On 23/05/18 10:47, Philipp Zabel wrote:
> Hi Hans,
> 
> thank you for the review comments.
> 
> On Tue, 2018-05-22 at 19:47 +0200, Hans Verkuil wrote:
>> On 22/05/18 18:29, Philipp Zabel wrote:
>>> Limit frame sizes to the [1, UINT_MAX-1] interval, media bus formats to
>>> the available list of formats, and initialize pad and try formats.
>>>
>>> Reported-by: Rui Miguel Silva 
>>> Signed-off-by: Philipp Zabel 
>>> ---
>>>  drivers/media/platform/video-mux.c | 110 +
>>>  1 file changed, 110 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/video-mux.c 
>>> b/drivers/media/platform/video-mux.c
>>> index 1fb887293337..ade1dae706aa 100644
>>> --- a/drivers/media/platform/video-mux.c
>>> +++ b/drivers/media/platform/video-mux.c
>>> @@ -180,6 +180,87 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
>>> if (!source_mbusformat)
>>> return -EINVAL;
>>>  
>>> +   /* No size limitations except V4L2 compliance requirements */
>>> +   v4l_bound_align_image(>format.width, 1, UINT_MAX - 1, 0,
>>> + >format.height, 1, UINT_MAX - 1, 0, 0);
>>
>> This is a bit dubious. I would pick more realistic min/max values like 16 and
> 
> Why 16? A grayscale or RGB sensor could crop down to 1x1, see mt9v032
> for example.

Was that ever tested? Just because the software allows it, doesn't mean it 
actually
works.

> 
>> 65536. UINT_MAX - 1 will overflow whenever code increments/multiplies it for 
>> some
>> reason, which can cause all sorts of weird issues.
> 
> Ok. Should v4l2-compliance check for > 65536 then, instead of (or
> additionally to) UINT_MAX?

I think so, yes.

> 
>>> +
>>> +   /* All formats except LVDS and vendor specific formats are acceptable */
>>> +   switch (sdformat->format.code) {
>>> +   case MEDIA_BUS_FMT_RGB444_1X12:
>>> +   case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
> [...]
>>> +   case MEDIA_BUS_FMT_JPEG_1X8:
>>> +   case MEDIA_BUS_FMT_AHSV_1X32:
>>> +   break;
>>> +   default:
>>> +   sdformat->format.code = MEDIA_BUS_FMT_Y8_1X8;
>>
>> Add a break here.
> 
> Will do.
> 
>>> +   }
>>> +   if (sdformat->format.field == V4L2_FIELD_ANY)
>>> +   sdformat->format.field = V4L2_FIELD_NONE;
>>> +
>>> mutex_lock(>lock);
>>>  
>>> /* Source pad mirrors active sink pad, no limitations on sink pads */
>>> @@ -197,11 +278,33 @@ static int video_mux_set_format(struct v4l2_subdev 
>>> *sd,
>>> return 0;
>>>  }
>>>  
>>> +static int video_mux_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh 
>>> *fh)
>>> +{
>>> +   struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>>> +   struct v4l2_mbus_framefmt *mbusformat;
>>> +   int i;
>>> +
>>> +   mutex_lock(>lock);
>>> +
>>> +   for (i = 0; i < sd->entity.num_pads; i++) {
>>> +   mbusformat = v4l2_subdev_get_try_format(sd, fh->pad, i);
>>> +   *mbusformat = vmux->format_mbus[i];
>>> +   }
>>> +
>>> +   mutex_unlock(>lock);
>>> +
>>> +   return 0;
>>> +}
>>
>> This isn't the right approach. Instead implement the init_cfg pad op.
> 
> How embarrassing, yes.
> 
>>> +
>>>  static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
>>> .get_fmt = video_mux_get_format,
>>> .set_fmt = video_mux_set_format,
>>>  };
>>>  
>>> +static const struct v4l2_subdev_internal_ops video_mux_internal_ops = {
>>> +   .open = video_mux_open,
>>> +};
>>
>> So this can be dropped.
> 
> Ok, thanks!
> 
> regards
> Philipp
> 

Regards,

Hans

Re: [PATCH] media: dt-bindings: media: rcar_vin: add support for r8a77965

[Simon Horman]

On Sun, May 13, 2018 at 08:58:18PM +0200, Niklas Söderlund wrote:
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Simon Horman 

> ---
>  Documentation/devicetree/bindings/media/rcar_vin.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt 
> b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index a19517e1c669eb35..c2c57dcf73f4851b 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -21,6 +21,7 @@ on Gen3 platforms to a CSI-2 receiver.
> - "renesas,vin-r8a7794" for the R8A7794 device
> - "renesas,vin-r8a7795" for the R8A7795 device
> - "renesas,vin-r8a7796" for the R8A7796 device
> +   - "renesas,vin-r8a77965" for the R8A77965 device
> - "renesas,vin-r8a77970" for the R8A77970 device
> - "renesas,rcar-gen2-vin" for a generic R-Car Gen2 or RZ/G1 compatible
>   device.
> -- 
> 2.17.0
> 

Re: [PATCH] media: video-mux: fix compliance failures

[Philipp Zabel]

Hi Hans,

thank you for the review comments.

On Tue, 2018-05-22 at 19:47 +0200, Hans Verkuil wrote:
> On 22/05/18 18:29, Philipp Zabel wrote:
> > Limit frame sizes to the [1, UINT_MAX-1] interval, media bus formats to
> > the available list of formats, and initialize pad and try formats.
> > 
> > Reported-by: Rui Miguel Silva 
> > Signed-off-by: Philipp Zabel 
> > ---
> >  drivers/media/platform/video-mux.c | 110 +
> >  1 file changed, 110 insertions(+)
> > 
> > diff --git a/drivers/media/platform/video-mux.c 
> > b/drivers/media/platform/video-mux.c
> > index 1fb887293337..ade1dae706aa 100644
> > --- a/drivers/media/platform/video-mux.c
> > +++ b/drivers/media/platform/video-mux.c
> > @@ -180,6 +180,87 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
> > if (!source_mbusformat)
> > return -EINVAL;
> >  
> > +   /* No size limitations except V4L2 compliance requirements */
> > +   v4l_bound_align_image(>format.width, 1, UINT_MAX - 1, 0,
> > + >format.height, 1, UINT_MAX - 1, 0, 0);
> 
> This is a bit dubious. I would pick more realistic min/max values like 16 and

Why 16? A grayscale or RGB sensor could crop down to 1x1, see mt9v032
for example.

> 65536. UINT_MAX - 1 will overflow whenever code increments/multiplies it for 
> some
> reason, which can cause all sorts of weird issues.

Ok. Should v4l2-compliance check for > 65536 then, instead of (or
additionally to) UINT_MAX?

> > +
> > +   /* All formats except LVDS and vendor specific formats are acceptable */
> > +   switch (sdformat->format.code) {
> > +   case MEDIA_BUS_FMT_RGB444_1X12:
> > +   case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
[...]
> > +   case MEDIA_BUS_FMT_JPEG_1X8:
> > +   case MEDIA_BUS_FMT_AHSV_1X32:
> > +   break;
> > +   default:
> > +   sdformat->format.code = MEDIA_BUS_FMT_Y8_1X8;
> 
> Add a break here.

Will do.

> > +   }
> > +   if (sdformat->format.field == V4L2_FIELD_ANY)
> > +   sdformat->format.field = V4L2_FIELD_NONE;
> > +
> > mutex_lock(>lock);
> >  
> > /* Source pad mirrors active sink pad, no limitations on sink pads */
> > @@ -197,11 +278,33 @@ static int video_mux_set_format(struct v4l2_subdev 
> > *sd,
> > return 0;
> >  }
> >  
> > +static int video_mux_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh 
> > *fh)
> > +{
> > +   struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> > +   struct v4l2_mbus_framefmt *mbusformat;
> > +   int i;
> > +
> > +   mutex_lock(>lock);
> > +
> > +   for (i = 0; i < sd->entity.num_pads; i++) {
> > +   mbusformat = v4l2_subdev_get_try_format(sd, fh->pad, i);
> > +   *mbusformat = vmux->format_mbus[i];
> > +   }
> > +
> > +   mutex_unlock(>lock);
> > +
> > +   return 0;
> > +}
> 
> This isn't the right approach. Instead implement the init_cfg pad op.

How embarrassing, yes.

> > +
> >  static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> > .get_fmt = video_mux_get_format,
> > .set_fmt = video_mux_set_format,
> >  };
> >  
> > +static const struct v4l2_subdev_internal_ops video_mux_internal_ops = {
> > +   .open = video_mux_open,
> > +};
> 
> So this can be dropped.

Ok, thanks!

regards
Philipp

Re: [PATCH v2] v4l: vsp1: Fix vsp1_regs.h license header

[Laurent Pinchart]

Hi Simon,

On Wednesday, 23 May 2018 11:33:26 EEST Simon Horman wrote:
> On Tue, May 22, 2018 at 01:04:56PM +0200, Geert Uytterhoeven wrote:
> > On Tue, May 22, 2018 at 11:05 AM, Simon Horman  wrote:
> >>> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> >>> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> >>> @@ -1,4 +1,4 @@
> >>> -/* SPDX-License-Identifier: GPL-2.0 */
> >>> +/* SPDX-License-Identifier: GPL-2.0+ */
> >> 
> >> While you are changing this line, I believe the correct format is
> >> to use a '//' comment.
> >> 
> >> i.e.:
> >> 
> >> // SPDX-License-Identifier: GPL-2.0+
> > 
> > Not for C header files, only for C source files.
> 
> Wow!

Yes, it's a mess :-( The rationale is that the assembler doesn't support C++-
style comments, so we need to use C-style comments in header files. We should 
really have standardized usage of C-style comments everywhere, it makes no 
sense to me.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] v4l: vsp1: Fix vsp1_regs.h license header

[Simon Horman]

On Tue, May 22, 2018 at 01:04:56PM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Tue, May 22, 2018 at 11:05 AM, Simon Horman  wrote:
> >> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> >> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> >> @@ -1,4 +1,4 @@
> >> -/* SPDX-License-Identifier: GPL-2.0 */
> >> +/* SPDX-License-Identifier: GPL-2.0+ */
> >
> > While you are changing this line, I believe the correct format is
> > to use a '//' comment.
> >
> > i.e.:
> >
> > // SPDX-License-Identifier: GPL-2.0+
> 
> Not for C header files, only for C source files.

Wow!

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[Sakari Ailus]

Hi Jacopo and Bingbu,

On Tue, May 22, 2018 at 10:08:48PM +0200, jacopo mondi wrote:
...
> > +/* Get bayer order based on flip setting. */
> > +static __u32 imx319_get_format_code(struct imx319 *imx319)
> > +{
> > +   /*
> > +* Only one bayer order is supported.
> > +* It depends on the flip settings.
> > +*/
> > +   static const __u32 codes[2][2] = {
> > +   { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> > +   { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> > +   };
> > +
> > +   return codes[imx319->vflip->val][imx319->hflip->val];
> > +}
> 
> I don't have any major comment actually, this is pretty good for a
> first submission.
> 
> This worries me a bit though. The media bus format depends on the
> V/HFLIP value, I assume this is an hardware limitation. But if
> changing the flip changes the reported media bus format, you could
> trigger a -EPIPE error during pipeline format validation between two
> streaming sessions with different flip settings. Isn't this a bit
> dangerous?

That's how it works on raw bayer sensors; you do have to configure the
entire pipeline accordingly.

What it also means is that the two controls may not be changed during
streaming --- this needs to be prevented by the driver, and I think it's
missing at the moment.

> 
> Below some minor comments.
> 
> > +
> > +/* Read registers up to 4 at a time */
> > +static int imx319_read_reg(struct imx319 *imx319, u16 reg, u32 len, u32 
> > *val)
> > +{
> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > +   struct i2c_msg msgs[2];
> > +   u8 addr_buf[2];
> > +   u8 data_buf[4] = { 0 };
> > +   int ret;
> > +
> > +   if (len > 4)
> > +   return -EINVAL;
> > +
> > +   put_unaligned_be16(reg, addr_buf);
> > +   /* Write register address */
> > +   msgs[0].addr = client->addr;
> > +   msgs[0].flags = 0;
> > +   msgs[0].len = ARRAY_SIZE(addr_buf);
> > +   msgs[0].buf = addr_buf;
> > +
> > +   /* Read data from register */
> > +   msgs[1].addr = client->addr;
> > +   msgs[1].flags = I2C_M_RD;
> > +   msgs[1].len = len;
> > +   msgs[1].buf = _buf[4 - len];
> > +
> > +   ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > +   if (ret != ARRAY_SIZE(msgs))
> > +   return -EIO;
> > +
> > +   *val = get_unaligned_be32(data_buf);
> > +
> > +   return 0;
> > +}
> > +
> > +/* Write registers up to 4 at a time */
> > +static int imx319_write_reg(struct imx319 *imx319, u16 reg, u32 len, u32 
> > val)
> > +{
> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > +   u8 buf[6];
> > +
> > +   if (len > 4)
> > +   return -EINVAL;
> > +
> > +   put_unaligned_be16(reg, buf);
> > +   put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
> > +   if (i2c_master_send(client, buf, len + 2) != len + 2)
> > +   return -EIO;
> > +
> > +   return 0;
> > +}
> > +
> > +/* Write a list of registers */
> > +static int imx319_write_regs(struct imx319 *imx319,
> > + const struct imx319_reg *regs, u32 len)
> > +{
> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > +   int ret;
> > +   u32 i;
> > +
> > +   for (i = 0; i < len; i++) {
> > +   ret = imx319_write_reg(imx319, regs[i].address, 1,
> > +   regs[i].val);
> 
> Unaligned to open parenthesis
> 
> > +   if (ret) {
> > +   dev_err_ratelimited(
> > +   >dev,
> > +   "Failed to write reg 0x%4.4x. error = %d\n",
> > +   regs[i].address, ret);
> 
> No need to break line, align to open parenthesis, please.
> 
> > +
> > +   return ret;
> > +   }
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +/* Open sub-device */
> > +static int imx319_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > +   struct imx319 *imx319 = to_imx319(sd);
> > +   struct v4l2_mbus_framefmt *try_fmt =
> > +   v4l2_subdev_get_try_format(sd, fh->pad, 0);
> > +
> > +   mutex_lock(>mutex);
> > +
> > +   /* Initialize try_fmt */
> > +   try_fmt->width = imx319->cur_mode->width;
> > +   try_fmt->height = imx319->cur_mode->height;
> > +   try_fmt->code = imx319_get_format_code(imx319);

The initial try format should reflect the default, not the current
configuration.

> > +   try_fmt->field = V4L2_FIELD_NONE;
> > +
> > +   mutex_unlock(>mutex);
> > +
> > +   return 0;
> > +}
> > +
> > +static int imx319_update_digital_gain(struct imx319 *imx319, u32 d_gain)
> > +{
> > +   int ret;
> > +
> > +   ret = imx319_write_reg(imx319, IMX319_REG_DPGA_USE_GLOBAL_GAIN, 1, 1);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Digital gain = (d_gain & 0xFF00) + (d_gain & 0xFF)/256 times */
> > +   return imx319_write_reg(imx319, IMX319_REG_DIG_GAIN_GLOBAL, 2, d_gain);
> > +}
> > +
> > +static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +   struct imx319 *imx319 = container_of(ctrl->handler,
> > +

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[Bing Bu Cao]

On 2018年05月23日 04:08, jacopo mondi wrote:

Hello Bingbu,
thanks for the patch

On Tue, May 22, 2018 at 12:33:01PM +0800, bingbu@intel.com wrote:

From: Bingbu Cao 

Add a V4L2 sub-device driver for the Sony IMX319 image sensor.
This is a camera sensor using the I2C bus for control and the
CSI-2 bus for data.

Could you please provide a changelog between versions? Also, I know
using the --in-reply-to option is tempting to keep all patch version
in a single thread, but most people finds it confusing and I rarely
see that.

Thanks, I will send the standalone v4 patch with change log .



Signed-off-by: Bingbu Cao 
Signed-off-by: Tianshu Qiu 
---
  MAINTAINERS|7 +
  drivers/media/i2c/Kconfig  |   11 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/imx319.c | 2434 
  4 files changed, 2453 insertions(+)
  create mode 100644 drivers/media/i2c/imx319.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e73a55a6a855..87b6c338d827 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13084,6 +13084,13 @@ S: Maintained
  F:drivers/media/i2c/imx274.c
  F:Documentation/devicetree/bindings/media/i2c/imx274.txt

+SONY IMX319 SENSOR DRIVER
+M: Bingbu Cao 
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/imx319.c
+
  SONY MEMORYSTICK CARD SUPPORT
  M:Alex Dubov 
  W:http://tifmxx.berlios.de/
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 1f9d7c6aa31a..c3d279cc293e 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -604,6 +604,17 @@ config VIDEO_IMX274
  This is a V4L2 sensor-level driver for the Sony IMX274
  CMOS image sensor.

+config VIDEO_IMX319
+   tristate "Sony IMX319 sensor support"
+   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   depends on MEDIA_CAMERA_SUPPORT
+   help
+ This is a Video4Linux2 sensor driver for the Sony
+ IMX319 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx319.
+
  config VIDEO_OV2640
tristate "OmniVision OV2640 sensor support"
depends on VIDEO_V4L2 && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 16fc34eda5cc..3adb3be4a486 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -104,5 +104,6 @@ obj-$(CONFIG_VIDEO_OV2659)  += ov2659.o
  obj-$(CONFIG_VIDEO_TC358743)  += tc358743.o
  obj-$(CONFIG_VIDEO_IMX258)+= imx258.o
  obj-$(CONFIG_VIDEO_IMX274)+= imx274.o
+obj-$(CONFIG_VIDEO_IMX319) += imx319.o

  obj-$(CONFIG_SDR_MAX2175) += max2175.o

This hunk does not apply nor on media tree master nor on Linus'
master. Could you please mention on which branch the patch is meant to
be applied in the cover letter? Maybe it's obvious for most people here,
but I failed to find it.

It is based on sakari media_tree branch.

I will rebase that in follow patches.



diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
new file mode 100644
index ..706bbafc75ec
--- /dev/null
+++ b/drivers/media/i2c/imx319.c
@@ -0,0 +1,2434 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Intel Corporation
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX319_REG_MODE_SELECT 0x0100
+#define IMX319_MODE_STANDBY0x00
+#define IMX319_MODE_STREAMING  0x01
+
+/* Chip ID */
+#define IMX319_REG_CHIP_ID 0x0016
+#define IMX319_CHIP_ID 0x0319
+
+/* V_TIMING internal */
+#define IMX319_REG_FLL 0x0340
+#define IMX319_FLL_MAX 0x
+
+/* Exposure control */
+#define IMX319_REG_EXPOSURE0x0202
+#define IMX319_EXPOSURE_MIN1
+#define IMX319_EXPOSURE_STEP   1
+#define IMX319_EXPOSURE_DEFAULT0x04ee
+
+/* Analog gain control */
+#define IMX319_REG_ANALOG_GAIN 0x0204
+#define IMX319_ANA_GAIN_MIN0
+#define IMX319_ANA_GAIN_MAX960
+#define IMX319_ANA_GAIN_STEP   1
+#define IMX319_ANA_GAIN_DEFAULT0
+
+/* Digital gain control */
+#define IMX319_REG_DPGA_USE_GLOBAL_GAIN0x3ff9
+#define IMX319_REG_DIG_GAIN_GLOBAL 0x020e
+#define IMX319_DGTL_GAIN_MIN   256
+#define IMX319_DGTL_GAIN_MAX   4095
+#define IMX319_DGTL_GAIN_STEP  1
+#define IMX319_DGTL_GAIN_DEFAULT   256
+
+/* Test Pattern Control */
+#define IMX319_REG_TEST_PATTERN0x0600
+#define IMX319_TEST_PATTERN_DISABLED   0
+#define IMX319_TEST_PATTERN_SOLID_COLOR1
+#define IMX319_TEST_PATTERN_COLOR_BARS 2
+#define IMX319_TEST_PATTERN_GRAY_COLOR_BARS3