Re: [PATCH] v4l2-async: add subnotifier registration for subdevices
Hej Sakari,
Tack för dina kommentarer.
On 2017-05-03 22:51:46 +0300, Sakari Ailus wrote:
> Hejssan!
>
> On Fri, Apr 28, 2017 at 01:47:48PM +0200, Niklas Söderlund wrote:
> > On 2017-04-28 13:28:17 +0300, Sakari Ailus wrote:
> > > Hi Niklas,
> > >
> > > Thank you for the patch.
> > >
> > > Do you happen to have a driver that would use this, to see some example of
> > > how the code is to be used?
> >
> > Yes, the latest R-Car CSI-2 series make use of this, see:
> >
> > https://www.spinics.net/lists/linux-renesas-soc/msg13693.html
>
> Ah, thanks. I'll take a look at that --- which should do for other reasons
> as well...
>
> ...
>
> > > > +
> > > > + /*
> > > > +* This function can be called recursively so the list
> > > > +* might be modified in a recursive call. Start from the
> > > > +* top of the list each iteration.
> > > > +*/
> > > > + found = 1;
> > > > + while (found) {
> > > > + found = 0;
> > > >
> > > > - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> > > > - int ret;
> > > > + list_for_each_entry_safe(sd, tmp, &subdev_list,
> > > > async_list) {
> > > > + int ret;
> > > >
> > > > - asd = v4l2_async_belongs(notifier, sd);
> > > > - if (!asd)
> > > > - continue;
> > > > + asd = v4l2_async_belongs(notifier, sd);
> > > > + if (!asd)
> > > > + continue;
> > > >
> > > > - ret = v4l2_async_test_notify(notifier, sd, asd);
> > > > - if (ret < 0) {
> > > > - mutex_unlock(&list_lock);
> > > > - return ret;
> > > > + ret = v4l2_async_test_notify(notifier, sd, asd);
> > > > + if (ret < 0) {
> > > > + if (!subnotifier)
> > > > + mutex_unlock(&list_lock);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + found = 1;
> > > > + break;
> > > > }
> > > > }
> > > >
> > > > /* Keep also completed notifiers on the list */
> > > > list_add(¬ifier->list, ¬ifier_list);
> > > >
> > > > - mutex_unlock(&list_lock);
> > > > + if (!subnotifier)
> > > > + mutex_unlock(&list_lock);
> > > >
> > > > return 0;
> > > > }
> > > > +
> > > > +int v4l2_async_subnotifier_register(struct v4l2_subdev *sd,
> > > > + struct v4l2_async_notifier
> > > > *notifier)
> > > > +{
> > > > + if (!sd->v4l2_dev) {
> > > > + dev_err(sd->dev ? sd->dev : NULL,
>
> sd->dev is enough.
Thanks, but I think i will drop the dev_err() all together and just
return -EINVAL once i move this to what will be called
v4l2_async_do_notifier_register() as you suggest bellow.
>
> > > > + "Can't register subnotifier for without
> > > > v4l2_dev\n");
> > > > + return -EINVAL;
> > >
> > > When did this start happening? :-)
> >
> > What do you mean? I'm not sure I understand this comment.
>
> Uh, right. So the caller simply needs to specify v4l2_dev? The same applies
> to v4l2_async_notifier_register() which does not test that --- but it
> should.
>
> How about adding this change in a separate patch to what will be called
> v4l2_async_do_notifier_register()?
I agree, I will do this in a separate patch before I add the
v4l2_async_subnotifier_register().
>
> >
> > >
> > > > + }
> > > > +
> > > > + return v4l2_async_do_notifier_register(sd->v4l2_dev, notifier,
> > > > true);
> > > > +}
> > > > +EXPORT_SYMBOL(v4l2_async_subnotifier_register);
> > > > +
> > > > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > > > +struct v4l2_async_notifier *notifier)
> > > > +{
> > > > + return v4l2_async_do_notifier_register(v4l2_dev, notifier,
> > > > false);
> > > > +}
> > > > EXPORT_SYMBOL(v4l2_async_notifier_register);
> > > >
> > > > -void v4l2_async_notifier_unregister(struct v4l2_async_notifier
> > > > *notifier)
> > > > +static void
> > > > +v4l2_async_do_notifier_unregister(struct v4l2_async_notifier *notifier,
> > > > + bool subnotifier)
> > > > {
> > > > struct v4l2_subdev *sd, *tmp;
> > > > unsigned int notif_n_subdev = notifier->num_subdevs;
> > > > @@ -210,7 +248,8 @@ void v4l2_async_notifier_unregister(struct
> > > > v4l2_async_notifier *notifier)
> > > > "Failed to allocate device cache!\n");
> > > > }
> > > >
> > > > - mutex_lock(&list_lock);
> > > > + if (!subnotifier)
> > > > + mutex_lock(&list_lock);
> > > >
> > > > list_del(¬ifier->list);
> > > >
Re: [PATCH] v4l2-async: add subnotifier registration for subdevices
Hi Kieran,
Thanks for your feedback.
On 2017-05-03 18:07:47 +0100, Kieran Bingham wrote:
> Hi Niklas,
>
> Small thing to ponder discovered below:
>
> On 27/04/17 23:30, Niklas Söderlund wrote:
> > When registered() of v4l2_subdev_internal_ops is called the subdevice
> > have access to the master devices v4l2_dev and it's called with the
> > async frameworks list_lock held. In this context the subdevice can
> > register its own notifiers to allow for incremental discovery of
> > subdevices.
> >
> > The master device registers the subdevices closest to itself in its
> > notifier while the subdevice(s) themself register notifiers for there
> > closest neighboring devices when they are registered. Using this
> > incremental approach two problems can be solved.
> >
> > 1. The master device no longer have to care how many subdevices exist in
> >the pipeline. It only needs to care about its closest subdevice and
> >arbitrary long pipelines can be created without having to adapt the
> >master device for each case.
> >
> > 2. Subdevices which are represented as a single DT node but register
> >more then one subdevice can use this to further the pipeline
> >discovery. Since the subdevice driver is the only one who knows which
> >of its subdevices is linked with which subdevice of a neighboring DT
> >node.
> >
> > To enable subdevices to register/unregister notifiers from the
> > registered()/unregistered() callback v4l2_async_subnotifier_register()
> > and v4l2_async_subnotifier_unregister() are added. These new notifier
> > register functions are similar to the master device equivalent functions
> > but run without taking the v4l2-async list_lock which already are held
> > when he registered()/unregistered() callbacks are called.
> >
> > Signed-off-by: Niklas Söderlund
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 91
> > +---
> > include/media/v4l2-async.h | 22 +
> > 2 files changed, 95 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 96cc733f35ef72b0..d4a676a2935eb058 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -136,12 +136,13 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> > sd->dev = NULL;
> > }
> >
> > -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > -struct v4l2_async_notifier *notifier)
> > +static int v4l2_async_do_notifier_register(struct v4l2_device *v4l2_dev,
> > + struct v4l2_async_notifier *notifier,
> > + bool subnotifier)
> > {
> > struct v4l2_subdev *sd, *tmp;
> > struct v4l2_async_subdev *asd;
> > - int i;
> > + int found, i;
> >
> > if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > return -EINVAL;
> > @@ -168,32 +169,69 @@ int v4l2_async_notifier_register(struct v4l2_device
> > *v4l2_dev,
> > list_add_tail(&asd->list, ¬ifier->waiting);
> > }
> >
> > - mutex_lock(&list_lock);
> > + if (!subnotifier)
> > + mutex_lock(&list_lock);
> > +
> > + /*
> > +* This function can be called recursively so the list
> > +* might be modified in a recursive call. Start from the
> > +* top of the list each iteration.
> > +*/
> > + found = 1;
> > + while (found) {
> > + found = 0;
> >
> > - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> > - int ret;
> > + list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> > + int ret;
> >
> > - asd = v4l2_async_belongs(notifier, sd);
> > - if (!asd)
> > - continue;
> > + asd = v4l2_async_belongs(notifier, sd);
> > + if (!asd)
> > + continue;
> >
> > - ret = v4l2_async_test_notify(notifier, sd, asd);
> > - if (ret < 0) {
> > - mutex_unlock(&list_lock);
> > - return ret;
> > + ret = v4l2_async_test_notify(notifier, sd, asd);
> > + if (ret < 0) {
> > + if (!subnotifier)
> > + mutex_unlock(&list_lock);
> > + return ret;
> > + }
> > +
> > + found = 1;
> > + break;
> > }
> > }
> >
> > /* Keep also completed notifiers on the list */
> > list_add(¬ifier->list, ¬ifier_list);
> >
> > - mutex_unlock(&list_lock);
> > + if (!subnotifier)
> > + mutex_unlock(&list_lock);
> >
> > return 0;
> > }
> > +
> > +int v4l2_async_subnotifier_register(struct v4l2_subdev *sd,
> > + struct v4l2_async_notifier *notifier)
> > +{
Re: [PATCH] v4l2-async: add subnotifier registration for subdevices
Hejssan!
On Fri, Apr 28, 2017 at 01:47:48PM +0200, Niklas Söderlund wrote:
> On 2017-04-28 13:28:17 +0300, Sakari Ailus wrote:
> > Hi Niklas,
> >
> > Thank you for the patch.
> >
> > Do you happen to have a driver that would use this, to see some example of
> > how the code is to be used?
>
> Yes, the latest R-Car CSI-2 series make use of this, see:
>
> https://www.spinics.net/lists/linux-renesas-soc/msg13693.html
Ah, thanks. I'll take a look at that --- which should do for other reasons
as well...
...
> > > +
> > > + /*
> > > + * This function can be called recursively so the list
> > > + * might be modified in a recursive call. Start from the
> > > + * top of the list each iteration.
> > > + */
> > > + found = 1;
> > > + while (found) {
> > > + found = 0;
> > >
> > > - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> > > - int ret;
> > > + list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> > > + int ret;
> > >
> > > - asd = v4l2_async_belongs(notifier, sd);
> > > - if (!asd)
> > > - continue;
> > > + asd = v4l2_async_belongs(notifier, sd);
> > > + if (!asd)
> > > + continue;
> > >
> > > - ret = v4l2_async_test_notify(notifier, sd, asd);
> > > - if (ret < 0) {
> > > - mutex_unlock(&list_lock);
> > > - return ret;
> > > + ret = v4l2_async_test_notify(notifier, sd, asd);
> > > + if (ret < 0) {
> > > + if (!subnotifier)
> > > + mutex_unlock(&list_lock);
> > > + return ret;
> > > + }
> > > +
> > > + found = 1;
> > > + break;
> > > }
> > > }
> > >
> > > /* Keep also completed notifiers on the list */
> > > list_add(¬ifier->list, ¬ifier_list);
> > >
> > > - mutex_unlock(&list_lock);
> > > + if (!subnotifier)
> > > + mutex_unlock(&list_lock);
> > >
> > > return 0;
> > > }
> > > +
> > > +int v4l2_async_subnotifier_register(struct v4l2_subdev *sd,
> > > + struct v4l2_async_notifier *notifier)
> > > +{
> > > + if (!sd->v4l2_dev) {
> > > + dev_err(sd->dev ? sd->dev : NULL,
sd->dev is enough.
> > > + "Can't register subnotifier for without v4l2_dev\n");
> > > + return -EINVAL;
> >
> > When did this start happening? :-)
>
> What do you mean? I'm not sure I understand this comment.
Uh, right. So the caller simply needs to specify v4l2_dev? The same applies
to v4l2_async_notifier_register() which does not test that --- but it
should.
How about adding this change in a separate patch to what will be called
v4l2_async_do_notifier_register()?
>
> >
> > > + }
> > > +
> > > + return v4l2_async_do_notifier_register(sd->v4l2_dev, notifier, true);
> > > +}
> > > +EXPORT_SYMBOL(v4l2_async_subnotifier_register);
> > > +
> > > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > > + struct v4l2_async_notifier *notifier)
> > > +{
> > > + return v4l2_async_do_notifier_register(v4l2_dev, notifier, false);
> > > +}
> > > EXPORT_SYMBOL(v4l2_async_notifier_register);
> > >
> > > -void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > > +static void
> > > +v4l2_async_do_notifier_unregister(struct v4l2_async_notifier *notifier,
> > > + bool subnotifier)
> > > {
> > > struct v4l2_subdev *sd, *tmp;
> > > unsigned int notif_n_subdev = notifier->num_subdevs;
> > > @@ -210,7 +248,8 @@ void v4l2_async_notifier_unregister(struct
> > > v4l2_async_notifier *notifier)
> > > "Failed to allocate device cache!\n");
> > > }
> > >
> > > - mutex_lock(&list_lock);
> > > + if (!subnotifier)
> > > + mutex_lock(&list_lock);
> > >
> > > list_del(¬ifier->list);
> > >
> > > @@ -237,15 +276,20 @@ void v4l2_async_notifier_unregister(struct
> > > v4l2_async_notifier *notifier)
> > > put_device(d);
> > > }
> > >
> > > - mutex_unlock(&list_lock);
> > > + if (!subnotifier)
> > > + mutex_unlock(&list_lock);
> > >
> > > /*
> > >* Call device_attach() to reprobe devices
> > >*
> > >* NOTE: If dev allocation fails, i is 0, and the whole loop won't be
> > >* executed.
> > > + * TODO: If we are unregistering a subdevice notifier we can't reprobe
> > > + * since the lock_list is held by the master device and attaching that
> > > + * device would call v4l2_async_register_subdev() and end in a deadlock
> > > + * on list_lock.
> > >*/
> > > - while (i--) {
> > > + while (i-- && !subnotifier) {
> >
> > Why is this not done for sub-notifiers?
> >
> > That said, the code here looks really dubious. But that's out of scope of
> > the patchset.
>
> I try to explain this in the comment above :-)
>
> If this is called
Re: [PATCH] v4l2-async: add subnotifier registration for subdevices
Hi Niklas,
Small thing to ponder discovered below:
On 27/04/17 23:30, Niklas Söderlund wrote:
> When registered() of v4l2_subdev_internal_ops is called the subdevice
> have access to the master devices v4l2_dev and it's called with the
> async frameworks list_lock held. In this context the subdevice can
> register its own notifiers to allow for incremental discovery of
> subdevices.
>
> The master device registers the subdevices closest to itself in its
> notifier while the subdevice(s) themself register notifiers for there
> closest neighboring devices when they are registered. Using this
> incremental approach two problems can be solved.
>
> 1. The master device no longer have to care how many subdevices exist in
>the pipeline. It only needs to care about its closest subdevice and
>arbitrary long pipelines can be created without having to adapt the
>master device for each case.
>
> 2. Subdevices which are represented as a single DT node but register
>more then one subdevice can use this to further the pipeline
>discovery. Since the subdevice driver is the only one who knows which
>of its subdevices is linked with which subdevice of a neighboring DT
>node.
>
> To enable subdevices to register/unregister notifiers from the
> registered()/unregistered() callback v4l2_async_subnotifier_register()
> and v4l2_async_subnotifier_unregister() are added. These new notifier
> register functions are similar to the master device equivalent functions
> but run without taking the v4l2-async list_lock which already are held
> when he registered()/unregistered() callbacks are called.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/v4l2-core/v4l2-async.c | 91
> +---
> include/media/v4l2-async.h | 22 +
> 2 files changed, 95 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c
> index 96cc733f35ef72b0..d4a676a2935eb058 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -136,12 +136,13 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> sd->dev = NULL;
> }
>
> -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> - struct v4l2_async_notifier *notifier)
> +static int v4l2_async_do_notifier_register(struct v4l2_device *v4l2_dev,
> +struct v4l2_async_notifier *notifier,
> +bool subnotifier)
> {
> struct v4l2_subdev *sd, *tmp;
> struct v4l2_async_subdev *asd;
> - int i;
> + int found, i;
>
> if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> return -EINVAL;
> @@ -168,32 +169,69 @@ int v4l2_async_notifier_register(struct v4l2_device
> *v4l2_dev,
> list_add_tail(&asd->list, ¬ifier->waiting);
> }
>
> - mutex_lock(&list_lock);
> + if (!subnotifier)
> + mutex_lock(&list_lock);
> +
> + /*
> + * This function can be called recursively so the list
> + * might be modified in a recursive call. Start from the
> + * top of the list each iteration.
> + */
> + found = 1;
> + while (found) {
> + found = 0;
>
> - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> - int ret;
> + list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> + int ret;
>
> - asd = v4l2_async_belongs(notifier, sd);
> - if (!asd)
> - continue;
> + asd = v4l2_async_belongs(notifier, sd);
> + if (!asd)
> + continue;
>
> - ret = v4l2_async_test_notify(notifier, sd, asd);
> - if (ret < 0) {
> - mutex_unlock(&list_lock);
> - return ret;
> + ret = v4l2_async_test_notify(notifier, sd, asd);
> + if (ret < 0) {
> + if (!subnotifier)
> + mutex_unlock(&list_lock);
> + return ret;
> + }
> +
> + found = 1;
> + break;
> }
> }
>
> /* Keep also completed notifiers on the list */
> list_add(¬ifier->list, ¬ifier_list);
>
> - mutex_unlock(&list_lock);
> + if (!subnotifier)
> + mutex_unlock(&list_lock);
>
> return 0;
> }
> +
> +int v4l2_async_subnotifier_register(struct v4l2_subdev *sd,
> + struct v4l2_async_notifier *notifier)
> +{
> + if (!sd->v4l2_dev) {
> + dev_err(sd->dev ? sd->dev : NULL,
Just came across this, and it seems a bit 'extraneous'
'If sd->dev is not null, use sd->dev, otherwise use NULL'
--
KB
> +
Re: [RFC v2 1/2] media: platform: Add SH CEU camera interface driver
Hi Jacopo,
On Wednesday 03 May 2017 18:14:03 jmondi wrote:
> On Wed, May 03, 2017 at 06:06:19PM +0300, Laurent Pinchart wrote:
> > On Wednesday 03 May 2017 11:52:36 jmondi wrote:
> >> On Thu, Apr 27, 2017 at 02:47:14PM +0300, Laurent Pinchart wrote:
> >>> Hi Jacopo,
> >>>
> >>> Thank you for the patch.
> >>
> >> [snip]
> >>
> +/**
> + * Collect formats supported by CEU and sensor subdevice
> + */
> +static int sh_ceu_init_active_formats(struct sh_ceu_dev *pcdev)
> +{
> +struct v4l2_subdev *sensor = pcdev->sensor;
> +struct sh_ceu_fmt *active_fmts;
> +unsigned int n_active_fmts;
> +struct sh_ceu_fmt *fmt;
> +unsigned int i;
> +
> +struct v4l2_subdev_mbus_code_enum mbus_code = {
> +.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +.index = 0,
> +};
> +
> +/* Count how may formats are supported by CEU and sensor
> subdevice */
> +n_active_fmts = 0;
> +while (!v4l2_subdev_call(sensor, pad, enum_mbus_code,
> + NULL, &mbus_code)) {
> +mbus_code.index++;
> +
> +fmt = get_ceu_fmt_from_mbus(mbus_code.code);
> >>>
> >>> This is not correct. You will get one one pixel format per media bus
> >>> format supported by the sensor. The CEU supports
> >>>
> >>> 1. converting packed YUV 4:2:2 (YUYV) to YUV 4:2:2 planar (CAMCR.JPG =
> >>> 00) or capturing raw data (CAMCR.JPG = 01)
> >>>
> >>> 2. outputting YUV 4:2:2 planar to memory (CDOCR.CDS = 1) or
> >>> downsampling it to YUV 4:2:0 planar (CDOCR.CDS = 0)
> >>>
> >>> 3. swapping bytes, words and long words at the output (CDOCR.COLS,
> >>> CDOCR.COWS and CDOCR.COBS fields)
> >>>
> >>> This effectively allows to
> >>>
> >>> - capture the raw data produced by the sensor
> >>> - capture YUV images produced by the sensor in packed YUV 4:2:2
> >>> format, from any Y/U/V order to any Y/U/V order
> >>> - capture YUV images produced by the sensor in planar YUV 4:2:2 or
> >>> planar YUV 4:2:0, from any Y/U/V order to any Y/U/V order
> >>>
> >>> The list of formats you create needs to reflect that. This means that
> >>>
> >>> - if the sensor can produce a packed YUV 4:2:2 format, the CEU will be
> >>> able to output YUYV, UYVY, YVYU, VYUY, NV12, NV21, NV16 and NV16
> >>>
> >>> - for every non-YUV format produced by the sensor, the CEU will be
> >>> able to output raw data
> >>>
> >>> The former is easy. You just need to list the formats produced by the
> >>> sensor, and if one of them is packed YUV 4:2:2, flag that in the
> >>> sh_ceu_dev structure, and allow all the above output YUV formats. You
> >>> don't need to create an instance-specific list of those formats in the
> >>> sh_ceu_dev structure, as they will be either all available or all
> >>> non-available.
> >>>
> >>> The latter is a bit more complex, you need a list of media bus code to
> >>> pixel format in the driver. I'd recommend counting the non-YUV packed
> >>> formats produced by the sensor, allocating an array of sh_ceu_fmt
> >>> pointers with that number of entries, and make each entry point to the
> >>> corresponding entry in the global sh_ceu_fmts array. The global
> >>> sh_ceu_fmts needs to be extended with common sensor formats (raw Bayer
> >>> and JPEG come to mind).
> >>>
> >>> If all sensors currently used with the CEU can produce packed YUV, you
> >>> could implement YUV support only to start with, leaving raw capture
> >>> support for later.
> >>
> >> Ok, thanks for explaining.
> >>
> >> I have a proposal here, as the original driver only supported "image
> >> fetch mode" (ie. It accepts data in YUYV with components ordering
> >> arbitrary swapped) as a first step we may want to replicate this,
> >> ignoring data synch fetch mode (Chris, you have a driver for this you are
> >> already using in your BSP so I guess it's less urgent to support it,
> >> right?).
> >>
> >> Also, regarding output memory format I am sure we can produce planar
> >> formats as NV12/21 and NV16/61, but I'm not sure I have found a way to
> >> output packed YUVY (and its permutations) to memory, if not considering
> >> it a binary format, as thus using data synch fetch mode.
> >
> > As I understand it, outputting packed YUV is indeed done using data synch
> > (or enable, I'd have to check) fetch mode, and swapping components to
> > convert between different YUYV orders is done through the CDOCR.COLS,
> > CDOCR.COWS and CDOCR.COBS bits.
>
> That's waht I wanted to have confirmed, thanks.
>
> >> So, as a first step my proposal is the following one:
> >> Accept any YUYV bus format from sensor, and support planar ones as
> >> memory output formats with down-sampling support (NV12/21 and NV16/61
> >> then).
> >
> > You can start with that, but from the same YUV bus format, you should be
> > able to output packed YUV easily using data sync
Re: [RFC v2 1/2] media: platform: Add SH CEU camera interface driver
Hi Laurent,
On Wed, May 03, 2017 at 06:06:19PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wednesday 03 May 2017 11:52:36 jmondi wrote:
> > On Thu, Apr 27, 2017 at 02:47:14PM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> >
> > [snip]
> >
> > >> +/**
> > >> + * Collect formats supported by CEU and sensor subdevice
> > >> + */
> > >> +static int sh_ceu_init_active_formats(struct sh_ceu_dev *pcdev)
> > >> +{
> > >> +struct v4l2_subdev *sensor = pcdev->sensor;
> > >> +struct sh_ceu_fmt *active_fmts;
> > >> +unsigned int n_active_fmts;
> > >> +struct sh_ceu_fmt *fmt;
> > >> +unsigned int i;
> > >> +
> > >> +struct v4l2_subdev_mbus_code_enum mbus_code = {
> > >> +.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > >> +.index = 0,
> > >> +};
> > >> +
> > >> +/* Count how may formats are supported by CEU and sensor
> > >> subdevice */
> > >> +n_active_fmts = 0;
> > >> +while (!v4l2_subdev_call(sensor, pad, enum_mbus_code,
> > >> + NULL, &mbus_code)) {
> > >> +mbus_code.index++;
> > >> +
> > >> +fmt = get_ceu_fmt_from_mbus(mbus_code.code);
> > >
> > > This is not correct. You will get one one pixel format per media bus
> > > format supported by the sensor. The CEU supports
> > >
> > > 1. converting packed YUV 4:2:2 (YUYV) to YUV 4:2:2 planar (CAMCR.JPG = 00)
> > > or capturing raw data (CAMCR.JPG = 01)
> > >
> > > 2. outputting YUV 4:2:2 planar to memory (CDOCR.CDS = 1) or downsampling
> > > it to YUV 4:2:0 planar (CDOCR.CDS = 0)
> > >
> > > 3. swapping bytes, words and long words at the output (CDOCR.COLS,
> > > CDOCR.COWS and CDOCR.COBS fields)
> > >
> > > This effectively allows to
> > >
> > > - capture the raw data produced by the sensor
> > > - capture YUV images produced by the sensor in packed YUV 4:2:2 format,
> > > from any Y/U/V order to any Y/U/V order
> > > - capture YUV images produced by the sensor in planar YUV 4:2:2 or planar
> > > YUV 4:2:0, from any Y/U/V order to any Y/U/V order
> > >
> > > The list of formats you create needs to reflect that. This means that
> > >
> > > - if the sensor can produce a packed YUV 4:2:2 format, the CEU will be
> > > able to output YUYV, UYVY, YVYU, VYUY, NV12, NV21, NV16 and NV16
> > >
> > > - for every non-YUV format produced by the sensor, the CEU will be able to
> > > output raw data
> > >
> > > The former is easy. You just need to list the formats produced by the
> > > sensor, and if one of them is packed YUV 4:2:2, flag that in the
> > > sh_ceu_dev structure, and allow all the above output YUV formats. You
> > > don't need to create an instance-specific list of those formats in the
> > > sh_ceu_dev structure, as they will be either all available or all
> > > non-available.
> > >
> > > The latter is a bit more complex, you need a list of media bus code to
> > > pixel format in the driver. I'd recommend counting the non-YUV packed
> > > formats produced by the sensor, allocating an array of sh_ceu_fmt
> > > pointers with that number of entries, and make each entry point to the
> > > corresponding entry in the global sh_ceu_fmts array. The global
> > > sh_ceu_fmts needs to be extended with common sensor formats (raw Bayer
> > > and JPEG come to mind).
> > >
> > > If all sensors currently used with the CEU can produce packed YUV, you
> > > could implement YUV support only to start with, leaving raw capture
> > > support for later.
> >
> > Ok, thanks for explaining.
> >
> > I have a proposal here, as the original driver only supported "image
> > fetch mode" (ie. It accepts data in YUYV with components ordering arbitrary
> > swapped) as a first step we may want to replicate this, ignoring data
> > synch fetch mode (Chris, you have a driver for this you are already
> > using in your BSP so I guess it's less urgent to support it, right?).
> >
> > Also, regarding output memory format I am sure we can produce planar formats
> > as NV12/21 and NV16/61, but I'm not sure I have found a way to output
> > packed YUVY (and its permutations) to memory, if not considering it a
> > binary format, as thus using data synch fetch mode.
>
> As I understand it, outputting packed YUV is indeed done using data synch (or
> enable, I'd have to check) fetch mode, and swapping components to convert
> between different YUYV orders is done through the CDOCR.COLS, CDOCR.COWS and
> CDOCR.COBS bits.
That's waht I wanted to have confirmed, thanks.
>
> > So, as a first step my proposal is the following one:
> > Accept any YUYV bus format from sensor, and support planar ones as memory
> > output formats with down-sampling support (NV12/21 and NV16/61 then).
>
> You can start with that, but from the same YUV bus format, you should be able
> to output packed YUV easily using data sync fetch mode. That can be
> implemented as a second step, it will just result in extending the hardcoded
>
Re: [RFC v2 1/2] media: platform: Add SH CEU camera interface driver
Hi Jacopo,
On Wednesday 03 May 2017 11:52:36 jmondi wrote:
> On Thu, Apr 27, 2017 at 02:47:14PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
>
> [snip]
>
> >> +/**
> >> + * Collect formats supported by CEU and sensor subdevice
> >> + */
> >> +static int sh_ceu_init_active_formats(struct sh_ceu_dev *pcdev)
> >> +{
> >> + struct v4l2_subdev *sensor = pcdev->sensor;
> >> + struct sh_ceu_fmt *active_fmts;
> >> + unsigned int n_active_fmts;
> >> + struct sh_ceu_fmt *fmt;
> >> + unsigned int i;
> >> +
> >> + struct v4l2_subdev_mbus_code_enum mbus_code = {
> >> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >> + .index = 0,
> >> + };
> >> +
> >> + /* Count how may formats are supported by CEU and sensor subdevice */
> >> + n_active_fmts = 0;
> >> + while (!v4l2_subdev_call(sensor, pad, enum_mbus_code,
> >> + NULL, &mbus_code)) {
> >> + mbus_code.index++;
> >> +
> >> + fmt = get_ceu_fmt_from_mbus(mbus_code.code);
> >
> > This is not correct. You will get one one pixel format per media bus
> > format supported by the sensor. The CEU supports
> >
> > 1. converting packed YUV 4:2:2 (YUYV) to YUV 4:2:2 planar (CAMCR.JPG = 00)
> > or capturing raw data (CAMCR.JPG = 01)
> >
> > 2. outputting YUV 4:2:2 planar to memory (CDOCR.CDS = 1) or downsampling
> > it to YUV 4:2:0 planar (CDOCR.CDS = 0)
> >
> > 3. swapping bytes, words and long words at the output (CDOCR.COLS,
> > CDOCR.COWS and CDOCR.COBS fields)
> >
> > This effectively allows to
> >
> > - capture the raw data produced by the sensor
> > - capture YUV images produced by the sensor in packed YUV 4:2:2 format,
> > from any Y/U/V order to any Y/U/V order
> > - capture YUV images produced by the sensor in planar YUV 4:2:2 or planar
> > YUV 4:2:0, from any Y/U/V order to any Y/U/V order
> >
> > The list of formats you create needs to reflect that. This means that
> >
> > - if the sensor can produce a packed YUV 4:2:2 format, the CEU will be
> > able to output YUYV, UYVY, YVYU, VYUY, NV12, NV21, NV16 and NV16
> >
> > - for every non-YUV format produced by the sensor, the CEU will be able to
> > output raw data
> >
> > The former is easy. You just need to list the formats produced by the
> > sensor, and if one of them is packed YUV 4:2:2, flag that in the
> > sh_ceu_dev structure, and allow all the above output YUV formats. You
> > don't need to create an instance-specific list of those formats in the
> > sh_ceu_dev structure, as they will be either all available or all
> > non-available.
> >
> > The latter is a bit more complex, you need a list of media bus code to
> > pixel format in the driver. I'd recommend counting the non-YUV packed
> > formats produced by the sensor, allocating an array of sh_ceu_fmt
> > pointers with that number of entries, and make each entry point to the
> > corresponding entry in the global sh_ceu_fmts array. The global
> > sh_ceu_fmts needs to be extended with common sensor formats (raw Bayer
> > and JPEG come to mind).
> >
> > If all sensors currently used with the CEU can produce packed YUV, you
> > could implement YUV support only to start with, leaving raw capture
> > support for later.
>
> Ok, thanks for explaining.
>
> I have a proposal here, as the original driver only supported "image
> fetch mode" (ie. It accepts data in YUYV with components ordering arbitrary
> swapped) as a first step we may want to replicate this, ignoring data
> synch fetch mode (Chris, you have a driver for this you are already
> using in your BSP so I guess it's less urgent to support it, right?).
>
> Also, regarding output memory format I am sure we can produce planar formats
> as NV12/21 and NV16/61, but I'm not sure I have found a way to output
> packed YUVY (and its permutations) to memory, if not considering it a
> binary format, as thus using data synch fetch mode.
As I understand it, outputting packed YUV is indeed done using data synch (or
enable, I'd have to check) fetch mode, and swapping components to convert
between different YUYV orders is done through the CDOCR.COLS, CDOCR.COWS and
CDOCR.COBS bits.
> So, as a first step my proposal is the following one:
> Accept any YUYV bus format from sensor, and support planar ones as memory
> output formats with down-sampling support (NV12/21 and NV16/61 then).
You can start with that, but from the same YUV bus format, you should be able
to output packed YUV easily using data sync fetch mode. That can be
implemented as a second step, it will just result in extending the hardcoded
list of YUV pixel formats supported by the driver (as any YUV bus format can
produce any of the four NV variants and any of the four packed YUV variants).
The third step would be to enumerate the non-YUV formats supported by the
sensor, and create a list of corresponding pixel formats that can be supported
in data sync fetch mode. I'd leave that out for now.
So, if you only implement the first two steps,
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi, On 5/3/2017 3:54 PM, Sricharan R wrote: > Hi Robin, > > On 5/3/2017 3:24 PM, Robin Murphy wrote: >> Hi Geert, >> >> On 02/05/17 19:35, Geert Uytterhoeven wrote: >>> Hi Sricharan, >>> >>> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R >>> wrote: From: Laurent Pinchart Failures to look up an IOMMU when parsing the DT iommus property need to be handled separately from the .of_xlate() failures to support deferred probing. The lack of a registered IOMMU can be caused by the lack of a driver for the IOMMU, the IOMMU device probe not having been performed yet, having been deferred, or having failed. The first case occurs when the device tree describes the bus master and IOMMU topology correctly but no device driver exists for the IOMMU yet or the device driver has not been compiled in. Return NULL, the caller will configure the device without an IOMMU. The second and third cases are handled by deferring the probe of the bus master device which will eventually get reprobed after the IOMMU. The last case is currently handled by deferring the probe of the bus master device as well. A mechanism to either configure the bus master device without an IOMMU or to fail the bus master device probe depending on whether the IOMMU is optional or mandatory would be a good enhancement. Tested-by: Marek Szyprowski Signed-off-by: Laurent Pichart Signed-off-by: Sricharan R >>> >>> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers. >>> As the IOMMU nodes in DT are not yet enabled, all devices having iommus >>> properties in DT now fail to probe. >> >> How exactly do they fail to probe? Per d7b0558230e4, if there are no ops >> registered then they should merely defer until we reach the point of >> giving up and ignoring the IOMMU. Is it just that you have no other >> late-probing drivers or post-init module loads to kick the deferred >> queue after that point? I did try to find a way to explicitly kick it >> from a suitably late initcall, but there didn't seem to be any obvious >> public interface - anyone have any suggestions? >> >> I think that's more of a general problem with the probe deferral >> mechanism itself (I've seen the same thing happen with some of the >> CoreSight stuff on Juno due to the number of inter-component >> dependencies) rather than any specific fault of this series. >> > > I was thinking of an additional check like below to avoid the > situation ? > > From 499b6e662f60f23740b8880882b0a16f16434501 Mon Sep 17 00:00:00 2001 > From: Sricharan R > Date: Wed, 3 May 2017 13:16:59 +0530 > Subject: [PATCH] iommu: of: Fix check for returning EPROBE_DEFER > > While returning EPROBE_DEFER for iommu masters > take in to account of iommu nodes that could be > marked in DT as 'status=disabled', in which case > simply return NULL and let the master's probe > continue rather than deferring. > > Signed-off-by: Sricharan R > --- > drivers/iommu/of_iommu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 9f44ee8..e6e9bec 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct device_node > *np) > > ops = iommu_ops_from_fwnode(fwnode); > if ((ops && !ops->of_xlate) || > + !of_device_is_available(iommu_spec->np) || > (!ops && !of_iommu_driver_present(iommu_spec->np))) > return NULL; > While same as the other 'status=disabled' patch [1], better not to defer the probe itself in the case ? [1] https://patchwork.kernel.org/patch/9681211/ Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Robin,
On 5/3/2017 3:24 PM, Robin Murphy wrote:
> Hi Geert,
>
> On 02/05/17 19:35, Geert Uytterhoeven wrote:
>> Hi Sricharan,
>>
>> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R wrote:
>>> From: Laurent Pinchart
>>>
>>> Failures to look up an IOMMU when parsing the DT iommus property need to
>>> be handled separately from the .of_xlate() failures to support deferred
>>> probing.
>>>
>>> The lack of a registered IOMMU can be caused by the lack of a driver for
>>> the IOMMU, the IOMMU device probe not having been performed yet, having
>>> been deferred, or having failed.
>>>
>>> The first case occurs when the device tree describes the bus master and
>>> IOMMU topology correctly but no device driver exists for the IOMMU yet
>>> or the device driver has not been compiled in. Return NULL, the caller
>>> will configure the device without an IOMMU.
>>>
>>> The second and third cases are handled by deferring the probe of the bus
>>> master device which will eventually get reprobed after the IOMMU.
>>>
>>> The last case is currently handled by deferring the probe of the bus
>>> master device as well. A mechanism to either configure the bus master
>>> device without an IOMMU or to fail the bus master device probe depending
>>> on whether the IOMMU is optional or mandatory would be a good
>>> enhancement.
>>>
>>> Tested-by: Marek Szyprowski
>>> Signed-off-by: Laurent Pichart
>>> Signed-off-by: Sricharan R
>>
>> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers.
>> As the IOMMU nodes in DT are not yet enabled, all devices having iommus
>> properties in DT now fail to probe.
>
> How exactly do they fail to probe? Per d7b0558230e4, if there are no ops
> registered then they should merely defer until we reach the point of
> giving up and ignoring the IOMMU. Is it just that you have no other
> late-probing drivers or post-init module loads to kick the deferred
> queue after that point? I did try to find a way to explicitly kick it
> from a suitably late initcall, but there didn't seem to be any obvious
> public interface - anyone have any suggestions?
>
> I think that's more of a general problem with the probe deferral
> mechanism itself (I've seen the same thing happen with some of the
> CoreSight stuff on Juno due to the number of inter-component
> dependencies) rather than any specific fault of this series.
>
I was thinking of an additional check like below to avoid the
situation ?
>From 499b6e662f60f23740b8880882b0a16f16434501 Mon Sep 17 00:00:00 2001
From: Sricharan R
Date: Wed, 3 May 2017 13:16:59 +0530
Subject: [PATCH] iommu: of: Fix check for returning EPROBE_DEFER
While returning EPROBE_DEFER for iommu masters
take in to account of iommu nodes that could be
marked in DT as 'status=disabled', in which case
simply return NULL and let the master's probe
continue rather than deferring.
Signed-off-by: Sricharan R
---
drivers/iommu/of_iommu.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 9f44ee8..e6e9bec 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct device_node *np)
ops = iommu_ops_from_fwnode(fwnode);
if ((ops && !ops->of_xlate) ||
+ !of_device_is_available(iommu_spec->np) ||
(!ops && !of_iommu_driver_present(iommu_spec->np)))
return NULL;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Regards,
Sricharan
> Robin.
>
>> This can be fixed by either:
>> - Disabling CONFIG_IPMMU_VMSA, or
>> - Reverting commit 7b07cbefb68d486f (but keeping "int ret = 0;").
>>
>> Note that this was a bit hard to investigate, as R-Car Gen3 support wasn't
>> upstreamed yet, so bisection pointed to a merge commit.
>>
>>> ---
>>> [*] Fixed minor comment from Bjorn for removing the pci.h header inclusion
>>> in of_iommu.c
>>>
>>> drivers/base/dma-mapping.c | 5 +++--
>>> drivers/iommu/of_iommu.c | 4 ++--
>>> drivers/of/device.c| 7 ++-
>>> include/linux/of_device.h | 9 ++---
>>> 4 files changed, 17 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
>>> index 449b948..82bd45c 100644
>>> --- a/drivers/base/dma-mapping.c
>>> +++ b/drivers/base/dma-mapping.c
>>> @@ -353,6 +353,7 @@ int dma_configure(struct device *dev)
>>> {
>>> struct device *bridge = NULL, *dma_dev = dev;
>>> enum dev_dma_attr attr;
>>> + int ret = 0;
>>>
>>> if (dev_is_pci(dev)) {
>>> bridge = pci_get_host_bridge_device(to_pci_dev(dev));
>>> @@ -363,7 +364,7 @@ int dma_configure(struct device *dev)
>>> }
>>>
>>> if (dma_dev->of_node) {
>>> - of_dma_configure(dev, dma_dev->of_node);
>>> + ret = of_dma_configure(dev, dma_dev->of_node);
>>> } else if (has_acpi_companion(dma_de
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Geert,
On 02/05/17 19:35, Geert Uytterhoeven wrote:
> Hi Sricharan,
>
> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R wrote:
>> From: Laurent Pinchart
>>
>> Failures to look up an IOMMU when parsing the DT iommus property need to
>> be handled separately from the .of_xlate() failures to support deferred
>> probing.
>>
>> The lack of a registered IOMMU can be caused by the lack of a driver for
>> the IOMMU, the IOMMU device probe not having been performed yet, having
>> been deferred, or having failed.
>>
>> The first case occurs when the device tree describes the bus master and
>> IOMMU topology correctly but no device driver exists for the IOMMU yet
>> or the device driver has not been compiled in. Return NULL, the caller
>> will configure the device without an IOMMU.
>>
>> The second and third cases are handled by deferring the probe of the bus
>> master device which will eventually get reprobed after the IOMMU.
>>
>> The last case is currently handled by deferring the probe of the bus
>> master device as well. A mechanism to either configure the bus master
>> device without an IOMMU or to fail the bus master device probe depending
>> on whether the IOMMU is optional or mandatory would be a good
>> enhancement.
>>
>> Tested-by: Marek Szyprowski
>> Signed-off-by: Laurent Pichart
>> Signed-off-by: Sricharan R
>
> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers.
> As the IOMMU nodes in DT are not yet enabled, all devices having iommus
> properties in DT now fail to probe.
How exactly do they fail to probe? Per d7b0558230e4, if there are no ops
registered then they should merely defer until we reach the point of
giving up and ignoring the IOMMU. Is it just that you have no other
late-probing drivers or post-init module loads to kick the deferred
queue after that point? I did try to find a way to explicitly kick it
from a suitably late initcall, but there didn't seem to be any obvious
public interface - anyone have any suggestions?
I think that's more of a general problem with the probe deferral
mechanism itself (I've seen the same thing happen with some of the
CoreSight stuff on Juno due to the number of inter-component
dependencies) rather than any specific fault of this series.
Robin.
> This can be fixed by either:
> - Disabling CONFIG_IPMMU_VMSA, or
> - Reverting commit 7b07cbefb68d486f (but keeping "int ret = 0;").
>
> Note that this was a bit hard to investigate, as R-Car Gen3 support wasn't
> upstreamed yet, so bisection pointed to a merge commit.
>
>> ---
>> [*] Fixed minor comment from Bjorn for removing the pci.h header inclusion
>> in of_iommu.c
>>
>> drivers/base/dma-mapping.c | 5 +++--
>> drivers/iommu/of_iommu.c | 4 ++--
>> drivers/of/device.c| 7 ++-
>> include/linux/of_device.h | 9 ++---
>> 4 files changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
>> index 449b948..82bd45c 100644
>> --- a/drivers/base/dma-mapping.c
>> +++ b/drivers/base/dma-mapping.c
>> @@ -353,6 +353,7 @@ int dma_configure(struct device *dev)
>> {
>> struct device *bridge = NULL, *dma_dev = dev;
>> enum dev_dma_attr attr;
>> + int ret = 0;
>>
>> if (dev_is_pci(dev)) {
>> bridge = pci_get_host_bridge_device(to_pci_dev(dev));
>> @@ -363,7 +364,7 @@ int dma_configure(struct device *dev)
>> }
>>
>> if (dma_dev->of_node) {
>> - of_dma_configure(dev, dma_dev->of_node);
>> + ret = of_dma_configure(dev, dma_dev->of_node);
>> } else if (has_acpi_companion(dma_dev)) {
>> attr =
>> acpi_get_dma_attr(to_acpi_device_node(dma_dev->fwnode));
>> if (attr != DEV_DMA_NOT_SUPPORTED)
>> @@ -373,7 +374,7 @@ int dma_configure(struct device *dev)
>> if (bridge)
>> pci_put_host_bridge_device(bridge);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> void dma_deconfigure(struct device *dev)
>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>> index 1f92d98..2d04663 100644
>> --- a/drivers/iommu/of_iommu.c
>> +++ b/drivers/iommu/of_iommu.c
>> @@ -236,7 +236,7 @@ const struct iommu_ops *of_iommu_configure(struct device
>> *dev,
>> ops = ERR_PTR(err);
>> }
>>
>> - return IS_ERR(ops) ? NULL : ops;
>> + return ops;
>> }
>>
>> static int __init of_iommu_init(void)
>> @@ -247,7 +247,7 @@ static int __init of_iommu_init(void)
>> for_each_matching_node_and_match(np, matches, &match) {
>> const of_iommu_init_fn init_fn = match->data;
>>
>> - if (init_fn(np))
>> + if (init_fn && init_fn(np))
>> pr_err("Failed to initialise IOMMU %s\n",
>> of_node_full_name(np));
>> }
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index c17c19d..ba51ca6 100644
>> --- a/drivers/of/device.c
>> +++
Re: [RFC v2 1/2] media: platform: Add SH CEU camera interface driver
Hi Laurent,
On Thu, Apr 27, 2017 at 02:47:14PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
[snip]
> > +/**
> > + * Collect formats supported by CEU and sensor subdevice
> > + */
> > +static int sh_ceu_init_active_formats(struct sh_ceu_dev *pcdev)
> > +{
> > + struct v4l2_subdev *sensor = pcdev->sensor;
> > + struct sh_ceu_fmt *active_fmts;
> > + unsigned int n_active_fmts;
> > + struct sh_ceu_fmt *fmt;
> > + unsigned int i;
> > +
> > + struct v4l2_subdev_mbus_code_enum mbus_code = {
> > + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > + .index = 0,
> > + };
> > +
> > + /* Count how may formats are supported by CEU and sensor subdevice */
> > + n_active_fmts = 0;
> > + while (!v4l2_subdev_call(sensor, pad, enum_mbus_code,
> > +NULL, &mbus_code)) {
> > + mbus_code.index++;
> > +
> > + fmt = get_ceu_fmt_from_mbus(mbus_code.code);
>
> This is not correct. You will get one one pixel format per media bus format
> supported by the sensor. The CEU supports
>
> 1. converting packed YUV 4:2:2 (YUYV) to YUV 4:2:2 planar (CAMCR.JPG = 00) or
> capturing raw data (CAMCR.JPG = 01)
>
> 2. outputting YUV 4:2:2 planar to memory (CDOCR.CDS = 1) or downsampling it to
> YUV 4:2:0 planar (CDOCR.CDS = 0)
>
> 3. swapping bytes, words and long words at the output (CDOCR.COLS, CDOCR.COWS
> and CDOCR.COBS fields)
>
> This effectively allows to
>
> - capture the raw data produced by the sensor
> - capture YUV images produced by the sensor in packed YUV 4:2:2 format, from
> any Y/U/V order to any Y/U/V order
> - capture YUV images produced by the sensor in planar YUV 4:2:2 or planar YUV
> 4:2:0, from any Y/U/V order to any Y/U/V order
>
> The list of formats you create needs to reflect that. This means that
>
> - if the sensor can produce a packed YUV 4:2:2 format, the CEU will be able to
> output YUYV, UYVY, YVYU, VYUY, NV12, NV21, NV16 and NV16
>
> - for every non-YUV format produced by the sensor, the CEU will be able to
> output raw data
>
> The former is easy. You just need to list the formats produced by the sensor,
> and if one of them is packed YUV 4:2:2, flag that in the sh_ceu_dev structure,
> and allow all the above output YUV formats. You don't need to create an
> instance-specific list of those formats in the sh_ceu_dev structure, as they
> will be either all available or all non-available.
>
> The latter is a bit more complex, you need a list of media bus code to pixel
> format in the driver. I'd recommend counting the non-YUV packed formats
> produced by the sensor, allocating an array of sh_ceu_fmt pointers with that
> number of entries, and make each entry point to the corresponding entry in the
> global sh_ceu_fmts array. The global sh_ceu_fmts needs to be extended with
> common sensor formats (raw Bayer and JPEG come to mind).
>
> If all sensors currently used with the CEU can produce packed YUV, you could
> implement YUV support only to start with, leaving raw capture support for
> later.
Ok, thanks for explaining.
I have a proposal here, as the original driver only supported "image
fetch mode" (ie. It accepts data in YUYV with components ordering arbitrary
swapped) as a first step we may want to replicate this, ignoring data
synch fetch mode (Chris, you have a driver for this you are already
using in your BSP so I guess it's less urgent to support it, right?).
Also, regarding output memory format I am sure we can produce planar formats
as NV12/21 and NV16/61, but I'm not sure I have found a way to output
packed YUVY (and its permutations) to memory, if not considering it a
binary format, as thus using data synch fetch mode.
So, as a first step my proposal is the following one:
Accept any YUYV bus format from sensor, and support planar ones as memory output
formats with down-sampling support (NV12/21 and NV16/61 then).
The way I am building the supported format list is indeed wrong, and
as you suggested I should just try to make sure the sensor can output
a YUV422 bus format. From there I can output planar memory formats.
One last thing, I am not that sure how I can produce NV21/61 from
bus formats that do not already present the CbCr components in the
desired order (which is Cr then Cb for NV61/21)
Eg. Can I produce NV21 from YUYV though byte/word/long word swapping
(CDOCR.COLS/CDOCR.COWS/CDOCR.COBS) ? Won't I be swapping the luminance
components as well (see figure 46.45 in RZ/A1H TRM).
If I cannot do that, I should restrict swapped planar format
availability based on the ability of the sensor to produce
chrominance components in the desired order
(Eg. If the sensor does not support producing YVYU I cannot produce
NV21 or NV61). Is this ok?
Thanks
j
>
> > + if (!fmt)
> > + continue;
> > +
> > + fmt->active = 1;
> > + n_active_fmts++;
> > + }
> > +
> > + if (n_active_fmts == 0)
> > + return -ENXIO;
> > +
> > + pcdev->n_active_fmts
Re: [PATCH v5] media: platform: Renesas IMR driver
Hi Geert, On Wednesday 03 May 2017 09:22:00 Geert Uytterhoeven wrote: > On Tue, May 2, 2017 at 11:17 PM, Laurent Pinchart wrote: > > On Wednesday 22 Mar 2017 10:34:16 Geert Uytterhoeven wrote: > >> On Thu, Mar 9, 2017 at 9:08 PM, Sergei Shtylyov wrote: > >>> --- /dev/null > >>> +++ media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt > >>> @@ -0,0 +1,27 @@ > >>> +Renesas R-Car Image Renderer (Distortion Correction Engine) > >>> +--- > >>> + > >>> +The image renderer, or the distortion correction engine, is a drawing > >>> processor > >>> +with a simple instruction system capable of referencing video capture > >>> data or > >>> +data in an external memory as 2D texture data and performing texture > >>> mapping > >>> +and drawing with respect to any shape that is split into triangular > >>> objects. > >>> + > >>> +Required properties: > >>> + > >>> +- compatible: "renesas,-imr-lx4", "renesas,imr-lx4" as a > >>> fallback for > >>> + the image renderer light extended 4 (IMR-LX4) found in the R-Car > >>> gen3 SoCs, > >>> + where the examples with are: > >>> + - "renesas,r8a7795-imr-lx4" for R-Car H3, > >>> + - "renesas,r8a7796-imr-lx4" for R-Car M3-W. > >> > >> Laurent: what do you think about the need for SoC-specific compatible > >> values for the various IM* blocks? > > > > There's no documented IP core version register, but when dumping all > > configuration registers on H3 and M3-W I noticed that register 0x002c, not > > documented in the datasheet, reads 0x14060514 on all four IMR instances in > > H3, and 0x20150505 on both instances in M3-W. > > > > This looks like a version register to me. If my assumption is correct, we > > could do without any SoC-specific compatible string. > > I read this assumed version registers on all R-Car SoCs, after writing > zero to 0xe6150990 (SMSTPCR8). > > IMR-X2 on R-Car H2: 0x12072009 > IMR-LSX2 on R-Car H2: 0x12072009 > IMR-LSX3 on R-Car V2H: 0x13052617 > IMR-LX2 on R-Car M2-W: 0x12072009 > IMR-LX2 on R-Car M2-N: 0x12072009 > IMR-LX2 on R-Car E2:0x13091909 > IMR-LX3 on R-Car V2H: 0x13052617 > > Note that several IDs are the same, but you know the type from the > compatible value. > > It would be good to get confirmation from the hardware team that this is > indeed a version register. Thank you for checking. Morimoto-san, do you think there are still people alive in the Gen2 hardware team who could provide the information ? :-) If not, information restricted to Gen3 would still be useful. -- Regards, Laurent Pinchart
Re: [PATCH 0/9] ARM: dts: renesas: update PFC node name to pin-controller
On Wed, Apr 26, 2017 at 12:05:29PM +0200, Simon Horman wrote: > > The device trees for Renesas SoCs use either pfc or pin-controller as the > node name for the PFC device. This patch is intended to take a step towards > unifying the node name used as pin-controller which appears to be the more > generic of the two and thus more in keeping with the DT specs. > > My analysis is that this is a user-visible change to the extent that kernel > logs, and sysfs entries change from XXX.pfc and pfc@XXX to > XXX.pin-controller and pin-controller@XXX. > > Boot tested on: > r8a73a4/ape6evm > r8a7740/armadillo800evb > r8a7778/bockw > r8a7790/lager > r8a7791/koelsch > r8a7791/porter > r8a7793/gose > sh73a9/kzm9g > > Not tested on (due to lack of hw access): > evm2/kzm9d > r8a7789/marzen I have now tested this on r8a7789/marzen
Re: [PATCH v5] media: platform: Renesas IMR driver
Hi Laurent,
On Tue, May 2, 2017 at 11:17 PM, Laurent Pinchart
wrote:
> On Wednesday 22 Mar 2017 10:34:16 Geert Uytterhoeven wrote:
>> On Thu, Mar 9, 2017 at 9:08 PM, Sergei Shtylyov wrote:
>> > --- /dev/null
>> > +++ media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt
>> > @@ -0,0 +1,27 @@
>> > +Renesas R-Car Image Renderer (Distortion Correction Engine)
>> > +---
>> > +
>> > +The image renderer, or the distortion correction engine, is a drawing
>> > processor
>> > +with a simple instruction system capable of referencing video capture
>> > data or
>> > +data in an external memory as 2D texture data and performing texture
>> > mapping
>> > +and drawing with respect to any shape that is split into triangular
>> > objects.
>> > +
>> > +Required properties:
>> > +
>> > +- compatible: "renesas,-imr-lx4", "renesas,imr-lx4" as a
>> > fallback for
>> > + the image renderer light extended 4 (IMR-LX4) found in the R-Car gen3
>> > SoCs,
>> > + where the examples with are:
>> > + - "renesas,r8a7795-imr-lx4" for R-Car H3,
>> > + - "renesas,r8a7796-imr-lx4" for R-Car M3-W.
>>
>> Laurent: what do you think about the need for SoC-specific compatible
>> values for the various IM* blocks?
>
> There's no documented IP core version register, but when dumping all
> configuration registers on H3 and M3-W I noticed that register 0x002c, not
> documented in the datasheet, reads 0x14060514 on all four IMR instances in H3,
> and 0x20150505 on both instances in M3-W.
>
> This looks like a version register to me. If my assumption is correct, we
> could do without any SoC-specific compatible string.
I read this assumed version registers on all R-Car SoCs, after writing
zero to 0xe6150990 (SMSTPCR8).
IMR-X2 on R-Car H2: 0x12072009
IMR-LSX2 on R-Car H2: 0x12072009
IMR-LSX3 on R-Car V2H: 0x13052617
IMR-LX2 on R-Car M2-W: 0x12072009
IMR-LX2 on R-Car M2-N: 0x12072009
IMR-LX2 on R-Car E2:0x13091909
IMR-LX3 on R-Car V2H: 0x13052617
Note that several IDs are the same, but you know the type from the
compatible value.
It would be good to get confirmation from the hardware team that this is
indeed a version register.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v6] arm64: dts: salvator-x: Add current sense amplifiers
On Tue, May 02, 2017 at 08:46:13PM +0200, Geert Uytterhoeven wrote: > From: Jacopo Mondi > > Add device nodes for two Maxim max961x current sense amplifiers > sensing VDD_08 and DVFS_08 lines. > > Signed-off-by: Jacopo Mondi > [geert: r8a7796-salvator-x.dts => salvator-x.dtsi] > Signed-off-by: Geert Uytterhoeven > --- > v6: > - r8a7796-salvator-x.dts => salvator-x.dtsi Thanks, queued up for v4.13.
