date:20170426

Re: [patch] autogain support for bayer10 format (was Re: [patch] propagating controls in libv4l2)

2017-04-26 Thread Ivaylo Dimitrov




On 27.04.2017 01:51, Pavel Machek wrote:

Hi!


There are two separate things here:

1) Autofoucs for a device that doesn't use subdev API
2) libv4l2 support for devices that require MC and subdev API


Actually there are three: 0) autogain. Unfortunately, I need autogain
first before autofocus has a chance...

And that means... bayer10 support for autogain.

Plus, I changed avg_lum to long long. Quick calculation tells me int
could overflow with few megapixel sensor.

Oh, btw http://ytse.tricolour.net/docs/LowLightOptimization.html no
longer works.

Regards,
Pavel

diff --git a/lib/libv4lconvert/processing/autogain.c 
b/lib/libv4lconvert/processing/autogain.c
index c6866d6..0b52d0f 100644
--- a/lib/libv4lconvert/processing/autogain.c
+++ b/lib/libv4lconvert/processing/autogain.c
@@ -68,6 +71,41 @@ static void autogain_adjust(struct v4l2_queryctrl *ctrl, int 
*value,
}
}

+static int get_luminosity_bayer10(uint16_t *buf, const struct v4l2_format *fmt)
+{
+   long long avg_lum = 0;
+   int x, y;
+   
+   buf += fmt->fmt.pix.height * fmt->fmt.pix.bytesperline / 4 +
+   fmt->fmt.pix.width / 4;
+
+   for (y = 0; y < fmt->fmt.pix.height / 2; y++) {
+   for (x = 0; x < fmt->fmt.pix.width / 2; x++)


That would take some time :). AIUI, we have NEON support in ARM kernels
(CONFIG_KERNEL_MODE_NEON), I wonder if it makes sense (me) to convert the
above loop to NEON-optimized when it comes to it? Are there any drawbacks in
using NEON code in kernel?


Well, thanks for offer. This is actualy libv4l2.



Oh, somehow I got confused that this is kernel code :)


But I'd say NEON conversion is not neccessary anytime soon. First,
this is just trying to get average luminosity. We can easily skip
quite a lot of pixels, and still get reasonable answer.

Second, omap3isp actually has a hardware block computing statistics
for us. We just don't use it for simplicity.



Right, I forgot about that.


(But if you want to play with camera, I'll get you patches; there's
ton of work to be done, both kernel and userspace :-).


Well, I saw a low hanging fruit I thought I can convert to NEON in a day 
or two, while having some rest from the huge "project" I am devoting all 
my spare time recently (rebasing hildon/maemo 5 on top of devuan 
Jessie). Still, if there is something relatively small to be done, just 
email me and I'll have a look.


Regards,
Ivo

Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function

2017-04-26 Thread Herbert Xu

On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote:
> Very straightforward conversion to the new function in the caam driver
> and shash library.
> 
> Signed-off-by: Logan Gunthorpe 
> Cc: Herbert Xu 
> Cc: "David S. Miller" 
> ---
>  crypto/shash.c| 9 ++---
>  drivers/crypto/caam/caamalg.c | 8 +++-
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/crypto/shash.c b/crypto/shash.c
> index 5e31c8d..5914881 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, 
> struct shash_desc *desc)
>   if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
>   void *data;
>  
> - data = kmap_atomic(sg_page(sg));
> - err = crypto_shash_digest(desc, data + offset, nbytes,
> + data = sg_map(sg, 0, SG_KMAP_ATOMIC);
> + if (IS_ERR(data))
> + return PTR_ERR(data);
> +
> + err = crypto_shash_digest(desc, data, nbytes,
> req->result);
> - kunmap_atomic(data);
> + sg_unmap(sg, data, 0, SG_KMAP_ATOMIC);
>   crypto_yield(desc->flags);
>   } else
>   err = crypto_shash_init(desc) ?:

Nack.  This is an optimisation for the special case of a single
SG list entry.  In fact in the common case the kmap_atomic should
disappear altogether in the no-highmem case.  So replacing it
with sg_map is not acceptable.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

cron job: media_tree daily build: ERRORS

2017-04-26 Thread 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 Apr 27 05:00:16 CEST 2017
media-tree git hash:3622d3e77ecef090b5111e3c5423313f11711dfa
media_build git hash:   1af19680bde3e227d64d99ff5fdc43eb343a3b28
v4l-utils git hash: b514d615166bdc0901a4c71261b87db31e89f464
gcc version:i686-linux-gcc (GCC) 6.2.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.9.0-164

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: ERRORS
linux-2.6.37.6-i686: ERRORS
linux-2.6.38.8-i686: ERRORS
linux-2.6.39.4-i686: ERRORS
linux-3.0.60-i686: ERRORS
linux-3.1.10-i686: ERRORS
linux-3.2.37-i686: OK
linux-3.3.8-i686: OK
linux-3.4.27-i686: OK
linux-3.5.7-i686: OK
linux-3.6.11-i686: OK
linux-3.7.4-i686: OK
linux-3.8-i686: OK
linux-3.9.2-i686: OK
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: ERRORS
linux-3.12.67-i686: ERRORS
linux-3.13.11-i686: ERRORS
linux-3.14.9-i686: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.16.7-i686: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: OK
linux-4.9-i686: OK
linux-4.10.1-i686: OK
linux-4.11-rc1-i686: OK
linux-2.6.36.4-x86_64: ERRORS
linux-2.6.37.6-x86_64: ERRORS
linux-2.6.38.8-x86_64: ERRORS
linux-2.6.39.4-x86_64: ERRORS
linux-3.0.60-x86_64: ERRORS
linux-3.1.10-x86_64: ERRORS
linux-3.2.37-x86_64: OK
linux-3.3.8-x86_64: OK
linux-3.4.27-x86_64: OK
linux-3.5.7-x86_64: OK
linux-3.6.11-x86_64: OK
linux-3.7.4-x86_64: OK
linux-3.8-x86_64: OK
linux-3.9.2-x86_64: OK
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: ERRORS
linux-3.12.67-x86_64: ERRORS
linux-3.13.11-x86_64: ERRORS
linux-3.14.9-x86_64: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.7-x86_64: WARNINGS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: WARNINGS
linux-4.9-x86_64: WARNINGS
linux-4.10.1-x86_64: WARNINGS
linux-4.11-rc1-x86_64: OK
apps: WARNINGS
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 v2 01/21] scatterlist: Introduce sg_map helper functions

2017-04-26 Thread Logan Gunthorpe



On 26/04/17 02:59 AM,   wrote:
> Good to know that somebody is working on this. Those problems troubled
> us as well.

Thanks Christian. It's a daunting problem and a there's a lot of work to
do before we will ever be where we need to be so any help, even an ack,
is greatly appreciated.

Logan

Re: [patch] autogain support for bayer10 format (was Re: [patch] propagating controls in libv4l2)

2017-04-26 Thread Pavel Machek

Hi!

> >>There are two separate things here:
> >>
> >>1) Autofoucs for a device that doesn't use subdev API
> >>2) libv4l2 support for devices that require MC and subdev API
> >
> >Actually there are three: 0) autogain. Unfortunately, I need autogain
> >first before autofocus has a chance...
> >
> >And that means... bayer10 support for autogain.
> >
> >Plus, I changed avg_lum to long long. Quick calculation tells me int
> >could overflow with few megapixel sensor.
> >
> >Oh, btw http://ytse.tricolour.net/docs/LowLightOptimization.html no
> >longer works.
> >
> >Regards,
> > Pavel
> >
> >diff --git a/lib/libv4lconvert/processing/autogain.c 
> >b/lib/libv4lconvert/processing/autogain.c
> >index c6866d6..0b52d0f 100644
> >--- a/lib/libv4lconvert/processing/autogain.c
> >+++ b/lib/libv4lconvert/processing/autogain.c
> >@@ -68,6 +71,41 @@ static void autogain_adjust(struct v4l2_queryctrl *ctrl, 
> >int *value,
> > }
> > }
> >
> >+static int get_luminosity_bayer10(uint16_t *buf, const struct v4l2_format 
> >*fmt)
> >+{
> >+long long avg_lum = 0;
> >+int x, y;
> >+
> >+buf += fmt->fmt.pix.height * fmt->fmt.pix.bytesperline / 4 +
> >+fmt->fmt.pix.width / 4;
> >+
> >+for (y = 0; y < fmt->fmt.pix.height / 2; y++) {
> >+for (x = 0; x < fmt->fmt.pix.width / 2; x++)
> 
> That would take some time :). AIUI, we have NEON support in ARM kernels
> (CONFIG_KERNEL_MODE_NEON), I wonder if it makes sense (me) to convert the
> above loop to NEON-optimized when it comes to it? Are there any drawbacks in
> using NEON code in kernel?

Well, thanks for offer. This is actualy libv4l2.

But I'd say NEON conversion is not neccessary anytime soon. First,
this is just trying to get average luminosity. We can easily skip
quite a lot of pixels, and still get reasonable answer.

Second, omap3isp actually has a hardware block computing statistics
for us. We just don't use it for simplicity.

(But if you want to play with camera, I'll get you patches; there's
ton of work to be done, both kernel and userspace :-).

Regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

[bug] omap3isp: missing support for ENUM_FMT

2017-04-26 Thread Pavel Machek

Hi!

Currently, ispvideo.c does not support enum_format. This causes
problems for example for libv4l2.

Now, I'm pretty sure patch below is not the right fix. But it fixes
libv4l2 problem for me.

Pointer to right solution welcome.

Regards,
Pavel

diff --git a/drivers/media/platform/omap3isp/ispvideo.c 
b/drivers/media/platform/omap3isp/ispvideo.c
index 218e6d7..2ce0327 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -772,6 +772,44 @@ isp_video_try_format(struct file *file, void *fh, struct 
v4l2_format *format)
 }
 
 static int
+isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *format)
+{
+   struct isp_video *video = video_drvdata(file);
+   struct v4l2_subdev_format fmt;
+   struct v4l2_subdev *subdev;
+   u32 pad;
+   int ret;
+
+   printk("ispvideo: enum_fmt\n");
+
+   subdev = isp_video_remote_subdev(video, );
+   if (subdev == NULL) {
+   printk("No subdev\n");
+   //return -EINVAL;
+   }
+
+   //isp_video_pix_to_mbus(>fmt.pix, );
+   if (format->index)
+   return -EINVAL;
+   format->type = video->type;
+   format->flags = 0;
+   strcpy(format->description, "subdev description");
+   format->pixelformat = V4L2_PIX_FMT_SGRBG10;
+
+   printk("Returning SRGBG10\n");
+#if 0  
+   fmt.pad = pad;
+   fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+   ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, );
+   if (ret)
+   return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
+
+   isp_video_mbus_to_pix(video, , >fmt.pix);
+#endif
+   return 0;
+}
+
+static int
 isp_video_get_selection(struct file *file, void *fh, struct v4l2_selection 
*sel)
 {
struct isp_video *video = video_drvdata(file);
@@ -1276,6 +1314,7 @@ static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
.vidioc_g_fmt_vid_cap   = isp_video_get_format,
.vidioc_s_fmt_vid_cap   = isp_video_set_format,
.vidioc_try_fmt_vid_cap = isp_video_try_format,
+   .vidioc_enum_fmt_vid_cap= isp_video_enum_format,
.vidioc_g_fmt_vid_out   = isp_video_get_format,
.vidioc_s_fmt_vid_out   = isp_video_set_format,
.vidioc_try_fmt_vid_out = isp_video_try_format,

On Sat 2017-03-04 20:44:50, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Saturday 04 Mar 2017 17:39:46 Sakari Ailus wrote:
> > On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> > > On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> > >> 
> >  Ok, how about this one?
> >  omap3isp: add rest of CSI1 support
> >  
> >  CSI1 needs one more bit to be set up. Do just that.
> >  
> >  It is not as straightforward as I'd like, see the comments in the
> >  code for explanation.
> > >>
> > >> ...
> > >> 
> >  +  if (isp->phy_type == ISP_PHY_TYPE_3430) {
> >  +  struct media_pad *pad;
> >  +  struct v4l2_subdev *sensor;
> >  +  const struct isp_ccp2_cfg *buscfg;
> >  +
> >  +  pad = media_entity_remote_pad(
> >  ->pads[CCP2_PAD_SINK]);
> >  +  sensor = media_entity_to_v4l2_subdev(pad->entity);
> >  +  /* Struct isp_bus_cfg has union inside */
> >  +  buscfg = &((struct isp_bus_cfg *)sensor->host_priv)
> >  ->bus.ccp2;
> >  +
> >  +  csiphy_routing_cfg_3430(>isp_csiphy2,
> >  +  ISP_INTERFACE_CCP2B_PHY1,
> > >>> > + enable, !!buscfg->phy_layer,
> > >>> > + buscfg->strobe_clk_pol);
> > >>> 
> > >>> You should do this through omap3isp_csiphy_acquire(), and not call
> > >>> csiphy_routing_cfg_3430() directly from here.
> > >> 
> > >> Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> > >> assumptions hard-coded :-(.
> > >> 
> > >> This will probably fail.
> > >> 
> > >>  rval = omap3isp_csi2_reset(phy->csi2);
> > >>  if (rval < 0)
> > >>  goto done;
> > > 
> > > Could you try to two patches I've applied on the ccp2 branch (I'll remove
> > > them if there are issues).
> > > 
> > > That's compile tested for now only.
> > 
> > One more thing. What's needed for configuring the PHY for CCP2?
> > 
> > For instance, is the CSI-2 PHY regulator still needed in
> > omap3isp_csiphy_acquire()? One way to do this might go to see the original
> > driver for N900; I don't have the TRM at hand right now.
> 
> The OMAP34xx TRM and data manual both mention separate VDDS power supplies 
> for 
> the CSIb and CSI2 I/O complexes.
> 
> vdds_csi2 CSI2 Complex I/O
> vdds_csib CSIb Complex I/O
> 
> On OMAP36xx, we instead have
> 
> vdda_csiphy1  Input power for

Re: [PATCH v2 01/21] scatterlist: Introduce sg_map helper functions

2017-04-26 Thread Logan Gunthorpe

On 26/04/17 01:44 AM, Christoph Hellwig wrote:
> I think we'll at least need a draft of those to make sense of these
> patches.  Otherwise they just look very clumsy.

Ok, I'll work up a draft proposal and send it in a couple days. But
without a lot of cleanup such as this series it's not going to even be
able to compile.

> I'm sorry but this API is just a trainwreck.  Right now we have the
> nice little kmap_atomic API, which never fails and has a very nice
> calling convention where we just pass back the return address, but does
> not support sleeping inside the critical section.
> 
> And kmap, whіch may fail and requires the original page to be passed
> back.  Anything that mixes these two concepts up is simply a non-starter.

Ok, well for starters I think you are mistaken about kmap being able to
fail. I'm having a hard time finding many users of that function that
bother to check for an error when calling it. The main difficulty we
have now is that neither of those functions are expected to fail and we
need them to be able to in cases where the page doesn't map to system
RAM. This patch series is trying to address it for users of scatterlist.
I'm certainly open to other suggestions.

I also have to disagree that kmap and kmap_atomic are all that "nice".
Except for the sleeping restriction and performance, they effectively do
the same thing. And it was necessary to write a macro wrapper around
kunmap_atomic to ensure that users of that function don't screw it up.
(See 597781f3e5.) I'd say the kmap/kmap_atomic functions are the
trainwreck and I'm trying to do my best to cleanup a few cases.

There are a fair number of cases in the kernel that do something like:

if (something)
x = kmap(page);
else
x = kmap_atomic(page);
...
if (something)
kunmap(page)
else
kunmap_atomic(x)

Which just seems cumbersome to me.

In any case, if you can accept an sg_kmap and sg_kmap_atomic api just
say so and I'll make the change. But I'll still need a flags variable
for SG_MAP_MUST_NOT_FAIL to support legacy cases that have no fail path
and both of those functions will need to be pretty nearly replicas of
each other.

Logan

Re: [PATCH] [media] s5p-jpeg: fix recursive spinlock acquisition

2017-04-26 Thread Jacek Anaszewski

On 04/26/2017 04:54 AM, Alexandre Courbot wrote:
> On Wed, Apr 26, 2017 at 4:15 AM, Jacek Anaszewski
>  wrote:
>> Hi Alexandre,
>>
>> Thanks for the patch.
>>
>> On 04/25/2017 08:19 AM, Alexandre Courbot wrote:
>>> v4l2_m2m_job_finish(), which is called from the interrupt handler with
>>> slock acquired, can call the device_run() hook immediately if another
>>> context was in the queue. This hook also acquires slock, resulting in
>>> a deadlock for this scenario.
>>>
>>> Fix this by releasing slock right before calling v4l2_m2m_job_finish().
>>> This is safe to do as the state of the hardware cannot change before
>>> v4l2_m2m_job_finish() is called anyway.
>>>
>>> Signed-off-by: Alexandre Courbot 
>>> ---
>>>  drivers/media/platform/s5p-jpeg/jpeg-core.c | 12 +---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
>>> b/drivers/media/platform/s5p-jpeg/jpeg-core.c
>>> index 52dc7941db65..223b4379929e 100644
>>> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
>>> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
>>> @@ -2642,13 +2642,13 @@ static irqreturn_t s5p_jpeg_irq(int irq, void 
>>> *dev_id)
>>>   if (curr_ctx->mode == S5P_JPEG_ENCODE)
>>>   vb2_set_plane_payload(_buf->vb2_buf, 0, payload_size);
>>>   v4l2_m2m_buf_done(dst_buf, state);
>>> - v4l2_m2m_job_finish(jpeg->m2m_dev, curr_ctx->fh.m2m_ctx);
>>>
>>>   curr_ctx->subsampling = s5p_jpeg_get_subsampling_mode(jpeg->regs);
>>>   spin_unlock(>slock);
>>>
>>>   s5p_jpeg_clear_int(jpeg->regs);
>>>
>>> + v4l2_m2m_job_finish(jpeg->m2m_dev, curr_ctx->fh.m2m_ctx);
>>>   return IRQ_HANDLED;
>>>  }
>>>
>>> @@ -2707,11 +2707,12 @@ static irqreturn_t exynos4_jpeg_irq(int irq, void 
>>> *priv)
>>>   v4l2_m2m_buf_done(dst_vb, VB2_BUF_STATE_ERROR);
>>>   }
>>>
>>> - v4l2_m2m_job_finish(jpeg->m2m_dev, curr_ctx->fh.m2m_ctx);
>>>   if (jpeg->variant->version == SJPEG_EXYNOS4)
>>>   curr_ctx->subsampling = 
>>> exynos4_jpeg_get_frame_fmt(jpeg->regs);
>>>
>>>   spin_unlock(>slock);
>>> +
>>> + v4l2_m2m_job_finish(jpeg->m2m_dev, curr_ctx->fh.m2m_ctx);
>>>   return IRQ_HANDLED;
>>>  }
>>>
>>> @@ -2770,10 +2771,15 @@ static irqreturn_t exynos3250_jpeg_irq(int irq, 
>>> void *dev_id)
>>>   if (curr_ctx->mode == S5P_JPEG_ENCODE)
>>>   vb2_set_plane_payload(_buf->vb2_buf, 0, payload_size);
>>>   v4l2_m2m_buf_done(dst_buf, state);
>>> - v4l2_m2m_job_finish(jpeg->m2m_dev, curr_ctx->fh.m2m_ctx);
>>>
>>>   curr_ctx->subsampling =
>>>   exynos3250_jpeg_get_subsampling_mode(jpeg->regs);
>>> +
>>> + spin_unlock(>slock);
>>> +
>>> + v4l2_m2m_job_finish(jpeg->m2m_dev, curr_ctx->fh.m2m_ctx);
>>> + return IRQ_HANDLED;
>>> +
>>>  exit_unlock:
>>>   spin_unlock(>slock);
>>>   return IRQ_HANDLED;
>>>
>>
>> Acked-by: Jacek Anaszewski 
>>
>> Just out of curiosity - could you share how you discovered the problem -
>> by some static checkers or trying to use the driver?
> 
> We discovered this issue after adding a new unit test for the jpeg
> codec in Chromium OS:
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=705971
> 
>>From what I understand the test spawns different processes that access
> the codec device concurrently, creating the situation leading to the
> bug.

Thanks for the explanation. Nice fix.

> On a slightly related note, I was thinking whether it would make sense
> to move the call to  v4l2_m2m_job_finish() (and maybe other parts of
> the current interrupt handler) into a worker or a threaded interrupt
> handler so as to reduce the time we spend with interrupts disabled.
> Can I have your input on this idea?

Right, all remaining drivers call it from workers.
Feel free to submit a patch.

-- 
Best regards,
Jacek Anaszewski

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

2017-04-26 Thread Nicolas Dufresne

Le mercredi 26 avril 2017 à 21:31 +0200, Tobias Jakobi a écrit :
> I'm pretty sure you have misread Marek's description of the patchset.
> The picture processor API should replaced/deprecate the IPP API that is
> currently implemented in the Exynos DRM.
> 
> In particular this affects the following files:
> - drivers/gpu/drm/exynos/exynos_drm_ipp.{c,h}
> - drivers/gpu/drm/exynos/exynos_drm_fimc.{c,h}
> - drivers/gpu/drm/exynos/exynos_drm_gsc.{c,h}
> - drivers/gpu/drm/exynos/exynos_drm_rotator.{c,h}
> 
> I know only two places where the IPP API is actually used. Tizen and my
> experimental mpv backend.

Sorry for the noise then.

regards,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

2017-04-26 Thread Tobias Jakobi

Hey,

Nicolas Dufresne wrote:
> Le mercredi 26 avril 2017 à 18:52 +0200, Tobias Jakobi a écrit :
>> Hello again,
>>
>>
>> Nicolas Dufresne wrote:
>>> Le mercredi 26 avril 2017 à 01:21 +0300, Sakari Ailus a écrit :
 Hi Marek,

 On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
> Hi Laurent,
>
> On 2017-04-20 12:25, Laurent Pinchart wrote:
>> Hi Marek,
>>
>> (CC'ing Sakari Ailus)
>>
>> Thank you for the patches.
>>
>> On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
>>> Dear all,
>>>
>>> This is an updated proposal for extending EXYNOS DRM API with generic
>>> support for hardware modules, which can be used for processing image 
>>> data
>>> from the one memory buffer to another. Typical memory-to-memory 
>>> operations
>>> are: rotation, scaling, colour space conversion or mix of them. This is 
>>> a
>>> follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
>>> processors", which has been rejected as "not really needed in the DRM
>>> core":
>>> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
>>>
>>> In this proposal I moved all the code to Exynos DRM driver, so now this
>>> will be specific only to Exynos DRM. I've also changed the name from
>>> framebuffer processor (fbproc) to picture processor (pp) to avoid 
>>> confusion
>>> with fbdev API.
>>>
>>> Here is a bit more information what picture processors are:
>>>
>>> Embedded SoCs are known to have a number of hardware blocks, which 
>>> perform
>>> such operations. They can be used in paralel to the main GPU module to
>>> offload CPU from processing grapics or video data. One of example use of
>>> such modules is implementing video overlay, which usually requires color
>>> space conversion from NV12 (or similar) to RGB32 color space and 
>>> scaling to
>>> target window size.
>>>
>>> The proposed API is heavily inspired by atomic KMS approach - it is also
>>> based on DRM objects and their properties. A new DRM object is 
>>> introduced:
>>> picture processor (called pp for convenience). Such objects have a set 
>>> of
>>> standard DRM properties, which describes the operation to be performed 
>>> by
>>> respective hardware module. In typical case those properties are a 
>>> source
>>> fb id and rectangle (x, y, width, height) and destination fb id and
>>> rectangle. Optionally a rotation property can be also specified if
>>> supported by the given hardware. To perform an operation on image data,
>>> userspace provides a set of properties and their values for given fbproc
>>> object in a similar way as object and properties are provided for
>>> performing atomic page flip / mode setting.
>>>
>>> The proposed API consists of the 3 new ioctls:
>>> - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
>>>   processors,
>>> - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
>>>   processor,
>>> - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
>>>   property set.
>>>
>>> The proposed API is extensible. Drivers can attach their own, custom
>>> properties to add support for more advanced picture processing (for 
>>> example
>>> blending).
>>>
>>> This proposal aims to replace Exynos DRM IPP (Image Post Processing)
>>> subsystem. IPP API is over-engineered in general, but not really 
>>> extensible
>>> on the other side. It is also buggy, with significant design flaws - the
>>> biggest issue is the fact that the API covers memory-2-memory picture
>>> operations together with CRTC writeback and duplicating features, which
>>> belongs to video plane. Comparing with IPP subsystem, the PP framework 
>>> is
>>> smaller (1807 vs 778 lines) and allows driver simplification (Exynos
>>> rotator driver smaller by over 200 lines).
>>>
>>> Just a side note, we have written code in GStreamer using the Exnynos 4
>>> FIMC IPP driver. I don't know how many, if any, deployment still exist
>>> (Exynos 4 is relatively old now), but there exist userspace for the
>>> FIMC driver.
>>
>> I was searching for this code, but I didn't find anything. Are you sure
>> you really mean the FIMC IPP in Exynos DRM, and not just the FIMC driver
>> from the V4L2 subsystem?
> 
> Oops, I manage to be unclear. Having two drivers on the same IP isn't
> helping. We wrote code around the FIMC driver on V4L2 side. This
> driver:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/platform/exynos4-is/fimc-m2m.c
> 
> And this code:
> 
> https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/gstv4l2transform.c
> 
> Unless I have miss-read, the proposal here is to deprecate the V4L side
> and improve the DRM

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

2017-04-26 Thread Nicolas Dufresne

Le mercredi 26 avril 2017 à 18:52 +0200, Tobias Jakobi a écrit :
> Hello again,
> 
> 
> Nicolas Dufresne wrote:
> > Le mercredi 26 avril 2017 à 01:21 +0300, Sakari Ailus a écrit :
> > > Hi Marek,
> > > 
> > > On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
> > > > Hi Laurent,
> > > > 
> > > > On 2017-04-20 12:25, Laurent Pinchart wrote:
> > > > > Hi Marek,
> > > > > 
> > > > > (CC'ing Sakari Ailus)
> > > > > 
> > > > > Thank you for the patches.
> > > > > 
> > > > > On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
> > > > > > Dear all,
> > > > > > 
> > > > > > This is an updated proposal for extending EXYNOS DRM API with 
> > > > > > generic
> > > > > > support for hardware modules, which can be used for processing 
> > > > > > image data
> > > > > > from the one memory buffer to another. Typical memory-to-memory 
> > > > > > operations
> > > > > > are: rotation, scaling, colour space conversion or mix of them. 
> > > > > > This is a
> > > > > > follow-up of my previous proposal "[RFC 0/2] New feature: 
> > > > > > Framebuffer
> > > > > > processors", which has been rejected as "not really needed in the 
> > > > > > DRM
> > > > > > core":
> > > > > > http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
> > > > > > 
> > > > > > In this proposal I moved all the code to Exynos DRM driver, so now 
> > > > > > this
> > > > > > will be specific only to Exynos DRM. I've also changed the name from
> > > > > > framebuffer processor (fbproc) to picture processor (pp) to avoid 
> > > > > > confusion
> > > > > > with fbdev API.
> > > > > > 
> > > > > > Here is a bit more information what picture processors are:
> > > > > > 
> > > > > > Embedded SoCs are known to have a number of hardware blocks, which 
> > > > > > perform
> > > > > > such operations. They can be used in paralel to the main GPU module 
> > > > > > to
> > > > > > offload CPU from processing grapics or video data. One of example 
> > > > > > use of
> > > > > > such modules is implementing video overlay, which usually requires 
> > > > > > color
> > > > > > space conversion from NV12 (or similar) to RGB32 color space and 
> > > > > > scaling to
> > > > > > target window size.
> > > > > > 
> > > > > > The proposed API is heavily inspired by atomic KMS approach - it is 
> > > > > > also
> > > > > > based on DRM objects and their properties. A new DRM object is 
> > > > > > introduced:
> > > > > > picture processor (called pp for convenience). Such objects have a 
> > > > > > set of
> > > > > > standard DRM properties, which describes the operation to be 
> > > > > > performed by
> > > > > > respective hardware module. In typical case those properties are a 
> > > > > > source
> > > > > > fb id and rectangle (x, y, width, height) and destination fb id and
> > > > > > rectangle. Optionally a rotation property can be also specified if
> > > > > > supported by the given hardware. To perform an operation on image 
> > > > > > data,
> > > > > > userspace provides a set of properties and their values for given 
> > > > > > fbproc
> > > > > > object in a similar way as object and properties are provided for
> > > > > > performing atomic page flip / mode setting.
> > > > > > 
> > > > > > The proposed API consists of the 3 new ioctls:
> > > > > > - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available 
> > > > > > picture
> > > > > >   processors,
> > > > > > - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
> > > > > >   processor,
> > > > > > - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by 
> > > > > > given
> > > > > >   property set.
> > > > > > 
> > > > > > The proposed API is extensible. Drivers can attach their own, custom
> > > > > > properties to add support for more advanced picture processing (for 
> > > > > > example
> > > > > > blending).
> > > > > > 
> > > > > > This proposal aims to replace Exynos DRM IPP (Image Post Processing)
> > > > > > subsystem. IPP API is over-engineered in general, but not really 
> > > > > > extensible
> > > > > > on the other side. It is also buggy, with significant design flaws 
> > > > > > - the
> > > > > > biggest issue is the fact that the API covers memory-2-memory 
> > > > > > picture
> > > > > > operations together with CRTC writeback and duplicating features, 
> > > > > > which
> > > > > > belongs to video plane. Comparing with IPP subsystem, the PP 
> > > > > > framework is
> > > > > > smaller (1807 vs 778 lines) and allows driver simplification (Exynos
> > > > > > rotator driver smaller by over 200 lines).
> > 
> > Just a side note, we have written code in GStreamer using the Exnynos 4
> > FIMC IPP driver. I don't know how many, if any, deployment still exist
> > (Exynos 4 is relatively old now), but there exist userspace for the
> > FIMC driver.
> 
> I was searching for this code, but I didn't find anything. Are you sure
> you really mean the FIMC IPP in Exynos DRM, and not just the FIMC driver
> from the V4L2 subsystem?

Oops,

em28xx debug module parameters

2017-04-26 Thread Frank Schäfer


Hi Mauro,

is there a chance that we can clean up the em28xx debug module parameter
mess ?
There are currently 8 (!) of them.
Do we have to maintain them all forever as "stable userspace interface" ?

For example:
- "reg_debug" is actually used for usb control message debugging
- "core_debug" is abused for usb isoc debugging and not used for
anything else
- there is a module parameter "isoc_debug", too, but it is used by
em28xx-v4l only
...

Regards,
Frank

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

2017-04-26 Thread Tobias Jakobi

Hello again,


Nicolas Dufresne wrote:
> Le mercredi 26 avril 2017 à 01:21 +0300, Sakari Ailus a écrit :
>> Hi Marek,
>>
>> On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
>>> Hi Laurent,
>>>
>>> On 2017-04-20 12:25, Laurent Pinchart wrote:
 Hi Marek,

 (CC'ing Sakari Ailus)

 Thank you for the patches.

 On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
> Dear all,
>
> This is an updated proposal for extending EXYNOS DRM API with generic
> support for hardware modules, which can be used for processing image data
> from the one memory buffer to another. Typical memory-to-memory operations
> are: rotation, scaling, colour space conversion or mix of them. This is a
> follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
> processors", which has been rejected as "not really needed in the DRM
> core":
> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
>
> In this proposal I moved all the code to Exynos DRM driver, so now this
> will be specific only to Exynos DRM. I've also changed the name from
> framebuffer processor (fbproc) to picture processor (pp) to avoid 
> confusion
> with fbdev API.
>
> Here is a bit more information what picture processors are:
>
> Embedded SoCs are known to have a number of hardware blocks, which perform
> such operations. They can be used in paralel to the main GPU module to
> offload CPU from processing grapics or video data. One of example use of
> such modules is implementing video overlay, which usually requires color
> space conversion from NV12 (or similar) to RGB32 color space and scaling 
> to
> target window size.
>
> The proposed API is heavily inspired by atomic KMS approach - it is also
> based on DRM objects and their properties. A new DRM object is introduced:
> picture processor (called pp for convenience). Such objects have a set of
> standard DRM properties, which describes the operation to be performed by
> respective hardware module. In typical case those properties are a source
> fb id and rectangle (x, y, width, height) and destination fb id and
> rectangle. Optionally a rotation property can be also specified if
> supported by the given hardware. To perform an operation on image data,
> userspace provides a set of properties and their values for given fbproc
> object in a similar way as object and properties are provided for
> performing atomic page flip / mode setting.
>
> The proposed API consists of the 3 new ioctls:
> - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
>   processors,
> - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
>   processor,
> - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
>   property set.
>
> The proposed API is extensible. Drivers can attach their own, custom
> properties to add support for more advanced picture processing (for 
> example
> blending).
>
> This proposal aims to replace Exynos DRM IPP (Image Post Processing)
> subsystem. IPP API is over-engineered in general, but not really 
> extensible
> on the other side. It is also buggy, with significant design flaws - the
> biggest issue is the fact that the API covers memory-2-memory picture
> operations together with CRTC writeback and duplicating features, which
> belongs to video plane. Comparing with IPP subsystem, the PP framework is
> smaller (1807 vs 778 lines) and allows driver simplification (Exynos
> rotator driver smaller by over 200 lines).
> 
> Just a side note, we have written code in GStreamer using the Exnynos 4
> FIMC IPP driver. I don't know how many, if any, deployment still exist
> (Exynos 4 is relatively old now), but there exist userspace for the
> FIMC driver.
I was searching for this code, but I didn't find anything. Are you sure
you really mean the FIMC IPP in Exynos DRM, and not just the FIMC driver
from the V4L2 subsystem?


With best wishes,
Tobias



> We use this for color transformation (from tiled to
> linear) and scaling. The FIMC driver is in fact quite stable in
> upstream kernel today. The GScaler V4L2 M2M driver on Exynos 5 is
> largely based on it and has received some maintenance to properly work
> in GStreamer. unlike this DRM API, you can reuse the same userspace
> code across multiple platforms (which we do already). We have also
> integrated this driver in Chromium in the past (not upstream though).
> 
> I am well aware that the blitter driver has not got much attention
> though. But again, V4L2 offers a generic interface to userspace
> application. Fixing this driver could enable some work like this one:
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=772766
> 
> This work in progress feature is a generic hardware accelerated video
>

[PATCH] em28xx: fix+improve the register (usb control message) debugging

2017-04-26 Thread Frank Schäfer

- avoid duplicate debugging messages in em28xx_read_reg_req_len()
- do not describe successful usb transfers in em28xx_read_reg_len()
  as "failed"
- report errors in em28xx_write_regs_req(), too
- print the usb error numbers, too

Signed-off-by: Frank Schäfer 
---
 drivers/media/usb/em28xx/em28xx-core.c | 35 +-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c 
b/drivers/media/usb/em28xx/em28xx-core.c
index 19ccff41c7eb..76d3c2a5f9e4 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -91,22 +91,16 @@ int em28xx_read_reg_req_len(struct em28xx *dev, u8 req, u16 
reg,
if (len > URB_MAX_CTRL_SIZE)
return -EINVAL;
 
-   em28xx_regdbg("(pipe 0x%08x): IN:  %02x %02x %02x %02x %02x %02x %02x 
%02x ",
-pipe, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-req, 0, 0,
-reg & 0xff, reg >> 8,
-len & 0xff, len >> 8);
-
mutex_lock(>ctrl_urb_lock);
ret = usb_control_msg(udev, pipe, req,
  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
  0x, reg, dev->urb_buf, len, HZ);
if (ret < 0) {
-   em28xx_regdbg("(pipe 0x%08x): IN:  %02x %02x %02x %02x %02x 
%02x %02x %02x  failed\n",
+   em28xx_regdbg("(pipe 0x%08x): IN:  %02x %02x %02x %02x %02x 
%02x %02x %02x  failed with error %i\n",
 pipe, USB_DIR_IN | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE,
 req, 0, 0,
 reg & 0xff, reg >> 8,
-len & 0xff, len >> 8);
+len & 0xff, len >> 8, ret);
mutex_unlock(>ctrl_urb_lock);
return usb_translate_errors(ret);
}
@@ -116,7 +110,7 @@ int em28xx_read_reg_req_len(struct em28xx *dev, u8 req, u16 
reg,
 
mutex_unlock(>ctrl_urb_lock);
 
-   em28xx_regdbg("(pipe 0x%08x): IN:  %02x %02x %02x %02x %02x %02x %02x 
%02x  failed <<< %*ph\n",
+   em28xx_regdbg("(pipe 0x%08x): IN:  %02x %02x %02x %02x %02x %02x %02x 
%02x <<< %*ph\n",
 pipe, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 req, 0, 0,
 reg & 0xff, reg >> 8,
@@ -164,13 +158,6 @@ int em28xx_write_regs_req(struct em28xx *dev, u8 req, u16 
reg, char *buf,
if ((len < 1) || (len > URB_MAX_CTRL_SIZE))
return -EINVAL;
 
-   em28xx_regdbg("(pipe 0x%08x): OUT: %02x %02x %02x %02x %02x %02x %02x 
%02x >>> %*ph\n",
- pipe,
- USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
- req, 0, 0,
- reg & 0xff, reg >> 8,
- len & 0xff, len >> 8, len, buf);
-
mutex_lock(>ctrl_urb_lock);
memcpy(dev->urb_buf, buf, len);
ret = usb_control_msg(udev, pipe, req,
@@ -178,8 +165,22 @@ int em28xx_write_regs_req(struct em28xx *dev, u8 req, u16 
reg, char *buf,
  0x, reg, dev->urb_buf, len, HZ);
mutex_unlock(>ctrl_urb_lock);
 
-   if (ret < 0)
+   if (ret < 0) {
+   em28xx_regdbg("(pipe 0x%08x): OUT:  %02x %02x %02x %02x %02x 
%02x %02x %02x >>> %*ph  failed with error %i\n",
+ pipe,
+ USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ req, 0, 0,
+ reg & 0xff, reg >> 8,
+ len & 0xff, len >> 8, len, buf, ret);
return usb_translate_errors(ret);
+   }
+
+   em28xx_regdbg("(pipe 0x%08x): OUT:  %02x %02x %02x %02x %02x %02x %02x 
%02x >>> %*ph\n",
+ pipe,
+ USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ req, 0, 0,
+ reg & 0xff, reg >> 8,
+ len & 0xff, len >> 8, len, buf);
 
if (dev->wait_after_write)
msleep(dev->wait_after_write);
-- 
2.12.2

Re: [PATCH] [RFC] em28xx: allow setting the eeprom bus at cards struct

2017-04-26 Thread Frank Schäfer


Am 25.04.2017 um 12:06 schrieb Mauro Carvalho Chehab:
> Right now, all devices use bus 0 for eeprom. However,
> it seems that newer versions of Terratec H6 uses a different
> buffer for eeprom.
>
> So, add support to use a different I2C address for eeprom and
> add a new card ID for the board described at:
>   http://forum.kodi.tv/showthread.php?tid=312902
I'm not 100% sure, but IIRC the bridge expects the eeprom to be always
on bus 0.

Anyway, looking at the information provided by the thread creator, it
seems that the eeprom is detected as usual on bus 0.
Otherwise "board has no eeprom" would have been printed to the log.
What happens here is that em28xx_i2c_read_block() fails with -ETIMEDOUT,
which means that the i2c status reported by the em2884 is (still) 0x02
or 0x04 when the timeout is reached.
See em28xx_i2c_send_bytes() and em28xx_i2c_recv_bytes().
Further tests/debugging is required to find out what exactly is going on.
Turning on debug module paramters i2c_debug and reg_debug would help.

> PS.: This patch was meant to allow testing the device. It may
> be wrong or incomplete, as it doesn't attempt to set GPIOs.
>
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/usb/em28xx/em28xx-cards.c | 20 
>  drivers/media/usb/em28xx/em28xx-i2c.c   |  5 +
>  drivers/media/usb/em28xx/em28xx.h   |  5 -
>  3 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c 
> b/drivers/media/usb/em28xx/em28xx-cards.c
> index a12b599a1fa2..b788ae0d5646 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -1193,6 +1193,23 @@ struct em28xx_board em28xx_boards[] = {
>   .i2c_speed= EM28XX_I2C_CLK_WAIT_ENABLE |
>   EM28XX_I2C_FREQ_400_KHZ,
>   },
> + [EM2884_BOARD_TERRATEC_H6] = {
> + .name = "Terratec Cinergy H6",
> + .has_dvb  = 1,
> + .ir_codes = RC_MAP_NEC_TERRATEC_CINERGY_XS,
> +#if 0
> + .tuner_type   = TUNER_PHILIPS_TDA8290,
According to the thread, the tuner is a TDA18271 !?

Regards,
Frank

> + .tuner_addr   = 0x41,
> + .dvb_gpio = terratec_h5_digital, /* FIXME: probably wrong */
> + .tuner_gpio   = terratec_h5_gpio,
> +#else
> + .tuner_type   = TUNER_ABSENT,
> +#endif
> + .def_i2c_bus  = 1,
> + .eeprom_i2c_bus  = 1,
> + .i2c_speed= EM28XX_I2C_CLK_WAIT_ENABLE |
> + EM28XX_I2C_FREQ_400_KHZ,
> + },
>   [EM2884_BOARD_HAUPPAUGE_WINTV_HVR_930C] = {
>   .name = "Hauppauge WinTV HVR 930C",
>   .has_dvb  = 1,
> @@ -2496,6 +2513,8 @@ struct usb_device_id em28xx_id_table[] = {
>   .driver_info = EM2884_BOARD_TERRATEC_H5 },
>   { USB_DEVICE(0x0ccd, 0x10b6),   /* H5 Rev. 3 */
>   .driver_info = EM2884_BOARD_TERRATEC_H5 },
> + { USB_DEVICE(0x0ccd, 0x10b2),   /* H6 */
> + .driver_info = EM2884_BOARD_TERRATEC_H6 },
>   { USB_DEVICE(0x0ccd, 0x0084),
>   .driver_info = EM2860_BOARD_TERRATEC_AV350 },
>   { USB_DEVICE(0x0ccd, 0x0096),
> @@ -2669,6 +2688,7 @@ static inline void em28xx_set_model(struct em28xx *dev)
>  
>   /* Should be initialized early, for I2C to work */
>   dev->def_i2c_bus = dev->board.def_i2c_bus;
> + dev->eeprom_i2c_bus = dev->board.eeprom_i2c_bus;
>  }
>  
>  /* Wait until AC97_RESET reports the expected value reliably before 
> proceeding.
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c 
> b/drivers/media/usb/em28xx/em28xx-i2c.c
> index 8c472d5adb50..df0ab4b6f18f 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -665,8 +665,6 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned 
> bus,
>   *eedata = NULL;
>   *eedata_len = 0;
>  
> - /* EEPROM is always on i2c bus 0 on all known devices. */
> -
>   dev->i2c_client[bus].addr = 0xa0 >> 1;
>  
>   /* Check if board has eeprom */
> @@ -975,8 +973,7 @@ int em28xx_i2c_register(struct em28xx *dev, unsigned bus,
>   dev->i2c_client[bus] = em28xx_client_template;
>   dev->i2c_client[bus].adapter = >i2c_adap[bus];
>  
> - /* Up to now, all eeproms are at bus 0 */
> - if (!bus) {
> + if (bus == dev->eeprom_i2c_bus) {
>   retval = em28xx_i2c_eeprom(dev, bus, >eedata, 
> >eedata_len);
>   if ((retval < 0) && (retval != -ENODEV)) {
>   dev_err(>intf->dev,
> diff --git a/drivers/media/usb/em28xx/em28xx.h 
> b/drivers/media/usb/em28xx/em28xx.h
> index e8d97d5ec161..a333ca954129 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -148,6 +148,7 @@
>  #define EM28178_BOARD_PLEX_PX_BCUD98
>  #define

Re: [PATCH v2 0/3] r8a7793 Gose video input support

2017-04-26 Thread Laurent Pinchart

Hi Ulrich,

On Tuesday 21 Feb 2017 01:42:15 Laurent Pinchart wrote:
> On Thursday 20 Oct 2016 10:49:11 Simon Horman wrote:
> > On Tue, Oct 18, 2016 at 05:02:20PM +0200, Ulrich Hecht wrote:
> >> Hi!
> >> 
> >> This is a by-the-datasheet implementation of analog and digital video
> >> input on the Gose board.
> >> 
> >> I have tried to address all concerns raised by reviewers, with the
> >> exception of the composite input patch, which has been left as is for
> >> now.
> >> 
> >> CU
> >> Uli
> >> 
> >> 
> >> Changes since v1:
> >> - r8a7793.dtsi: added VIN2
> >> - modeled HDMI decoder input/output and connector
> >> - added "renesas,rcar-gen2-vin" compat strings
> >> - removed unnecessary "remote" node and aliases
> >> - set ADV7612 interrupt to GP4_2
> >> 
> >> Ulrich Hecht (3):
> >>   ARM: dts: r8a7793: Enable VIN0-VIN2
> > 
> > I have queued up the above patch with Laurent and Geert's tags.
> > 
> >>   ARM: dts: gose: add HDMI input
> >>   ARM: dts: gose: add composite video input
> > 
> > Please address the review of the above two patches and repost.
> 
> Could you please do so ? Feedback on 2/3 should be easy to handle. For 3/3,
> you might need to ping the DT maintainers.

Ping. These are the only two patches that block

VIN,v4.12,public,ulrich,Gen2 VIN integration

> >>  arch/arm/boot/dts/r8a7793-gose.dts | 100 +
> >>  arch/arm/boot/dts/r8a7793.dtsi |  27 ++
> >>  2 files changed, 127 insertions(+)

-- 
Regards,

Laurent Pinchart

Re: [patch] autogain support for bayer10 format (was Re: [patch] propagating controls in libv4l2)

2017-04-26 Thread Ivaylo Dimitrov


Hi,

On 26.04.2017 16:23, Pavel Machek wrote:

Hi!


I don't see why it would be hard to open files or have threads inside
a library. There are several libraries that do that already, specially
the ones designed to be used on multimidia apps.


Well, This is what the libv4l2 says:

   This file implements libv4l2, which offers v4l2_ prefixed versions
   of
  open/close/etc. The API is 100% the same as directly opening
   /dev/videoX
  using regular open/close/etc, the big difference is that format
   conversion

but if I open additional files in v4l2_open(), API is no longer the
same, as unix open() is defined to open just one file descriptor.

Now. There is autogain support in libv4lconvert, but it expects to use
same fd for camera and for the gain... which does not work with
subdevs.

Of course, opening subdevs by name like this is not really
acceptable. But can you suggest a method that is?


There are two separate things here:

1) Autofoucs for a device that doesn't use subdev API
2) libv4l2 support for devices that require MC and subdev API


Actually there are three: 0) autogain. Unfortunately, I need autogain
first before autofocus has a chance...

And that means... bayer10 support for autogain.

Plus, I changed avg_lum to long long. Quick calculation tells me int
could overflow with few megapixel sensor.

Oh, btw http://ytse.tricolour.net/docs/LowLightOptimization.html no
longer works.

Regards,
Pavel

diff --git a/lib/libv4lconvert/processing/autogain.c 
b/lib/libv4lconvert/processing/autogain.c
index c6866d6..0b52d0f 100644
--- a/lib/libv4lconvert/processing/autogain.c
+++ b/lib/libv4lconvert/processing/autogain.c
@@ -68,6 +71,41 @@ static void autogain_adjust(struct v4l2_queryctrl *ctrl, int 
*value,
}
 }

+static int get_luminosity_bayer10(uint16_t *buf, const struct v4l2_format *fmt)
+{
+   long long avg_lum = 0;
+   int x, y;
+   
+   buf += fmt->fmt.pix.height * fmt->fmt.pix.bytesperline / 4 +
+   fmt->fmt.pix.width / 4;
+
+   for (y = 0; y < fmt->fmt.pix.height / 2; y++) {
+   for (x = 0; x < fmt->fmt.pix.width / 2; x++)


That would take some time :). AIUI, we have NEON support in ARM kernels 
(CONFIG_KERNEL_MODE_NEON), I wonder if it makes sense (me) to convert 
the above loop to NEON-optimized when it comes to it? Are there any 
drawbacks in using NEON code in kernel?



+   avg_lum += *buf++;
+   buf += fmt->fmt.pix.bytesperline - fmt->fmt.pix.width / 2;
+   }
+   avg_lum /= fmt->fmt.pix.height * fmt->fmt.pix.width / 4;
+   avg_lum /= 4;
+   return avg_lum;
+}
+
+static int get_luminosity_bayer8(unsigned char *buf, const struct v4l2_format 
*fmt)
+{
+   long long avg_lum = 0;
+   int x, y;
+   
+   buf += fmt->fmt.pix.height * fmt->fmt.pix.bytesperline / 4 +
+   fmt->fmt.pix.width / 4;
+
+   for (y = 0; y < fmt->fmt.pix.height / 2; y++) {
+   for (x = 0; x < fmt->fmt.pix.width / 2; x++)


ditto.


+   avg_lum += *buf++;
+   buf += fmt->fmt.pix.bytesperline - fmt->fmt.pix.width / 2;
+   }
+   avg_lum /= fmt->fmt.pix.height * fmt->fmt.pix.width / 4;
+   return avg_lum;
+}
+
 /* auto gain and exposure algorithm based on the knee algorithm described here:
 http://ytse.tricolour.net/docs/LowLightOptimization.html */
 static int autogain_calculate_lookup_tables(
@@ -100,17 +142,16 @@ static int autogain_calculate_lookup_tables(
switch (fmt->fmt.pix.pixelformat) {
+   case V4L2_PIX_FMT_SGBRG10:
+   case V4L2_PIX_FMT_SGRBG10:
+   case V4L2_PIX_FMT_SBGGR10:
+   case V4L2_PIX_FMT_SRGGB10:
+   avg_lum = get_luminosity_bayer10((void *) buf, fmt);
+   break;
+
case V4L2_PIX_FMT_SGBRG8:
case V4L2_PIX_FMT_SGRBG8:
case V4L2_PIX_FMT_SBGGR8:
case V4L2_PIX_FMT_SRGGB8:
-   buf += fmt->fmt.pix.height * fmt->fmt.pix.bytesperline / 4 +
-   fmt->fmt.pix.width / 4;
-
-   for (y = 0; y < fmt->fmt.pix.height / 2; y++) {
-   for (x = 0; x < fmt->fmt.pix.width / 2; x++)
-   avg_lum += *buf++;
-   buf += fmt->fmt.pix.bytesperline - fmt->fmt.pix.width / 
2;
-   }
-   avg_lum /= fmt->fmt.pix.height * fmt->fmt.pix.width / 4;
+   avg_lum = get_luminosity_bayer8(buf, fmt);
break;

case V4L2_PIX_FMT_RGB24:
diff --git a/lib/libv4lconvert/processing/libv4lprocessing.c 
b/lib/libv4lconvert/processing/libv4lprocessing.c
index b061f50..b98d024 100644
--- a/lib/libv4lconvert/processing/libv4lprocessing.c
+++ b/lib/libv4lconvert/processing/libv4lprocessing.c
@@ -164,6 +165,10 @@ void v4lprocessing_processing(struct v4lprocessing_data 
*data,
case V4L2_PIX_FMT_SGRBG8:
case V4L2_PIX_FMT_SBGGR8:
case

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

2017-04-26 Thread Tobias Jakobi

Hello everyone,


Nicolas Dufresne wrote:
> Le mercredi 26 avril 2017 à 01:21 +0300, Sakari Ailus a écrit :
>> Hi Marek,
>>
>> On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
>>> Hi Laurent,
>>>
>>> On 2017-04-20 12:25, Laurent Pinchart wrote:
 Hi Marek,

 (CC'ing Sakari Ailus)

 Thank you for the patches.

 On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
> Dear all,
>
> This is an updated proposal for extending EXYNOS DRM API with generic
> support for hardware modules, which can be used for processing image data
> from the one memory buffer to another. Typical memory-to-memory operations
> are: rotation, scaling, colour space conversion or mix of them. This is a
> follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
> processors", which has been rejected as "not really needed in the DRM
> core":
> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
>
> In this proposal I moved all the code to Exynos DRM driver, so now this
> will be specific only to Exynos DRM. I've also changed the name from
> framebuffer processor (fbproc) to picture processor (pp) to avoid 
> confusion
> with fbdev API.
>
> Here is a bit more information what picture processors are:
>
> Embedded SoCs are known to have a number of hardware blocks, which perform
> such operations. They can be used in paralel to the main GPU module to
> offload CPU from processing grapics or video data. One of example use of
> such modules is implementing video overlay, which usually requires color
> space conversion from NV12 (or similar) to RGB32 color space and scaling 
> to
> target window size.
>
> The proposed API is heavily inspired by atomic KMS approach - it is also
> based on DRM objects and their properties. A new DRM object is introduced:
> picture processor (called pp for convenience). Such objects have a set of
> standard DRM properties, which describes the operation to be performed by
> respective hardware module. In typical case those properties are a source
> fb id and rectangle (x, y, width, height) and destination fb id and
> rectangle. Optionally a rotation property can be also specified if
> supported by the given hardware. To perform an operation on image data,
> userspace provides a set of properties and their values for given fbproc
> object in a similar way as object and properties are provided for
> performing atomic page flip / mode setting.
>
> The proposed API consists of the 3 new ioctls:
> - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
>   processors,
> - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
>   processor,
> - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
>   property set.
>
> The proposed API is extensible. Drivers can attach their own, custom
> properties to add support for more advanced picture processing (for 
> example
> blending).
>
> This proposal aims to replace Exynos DRM IPP (Image Post Processing)
> subsystem. IPP API is over-engineered in general, but not really 
> extensible
> on the other side. It is also buggy, with significant design flaws - the
> biggest issue is the fact that the API covers memory-2-memory picture
> operations together with CRTC writeback and duplicating features, which
> belongs to video plane. Comparing with IPP subsystem, the PP framework is
> smaller (1807 vs 778 lines) and allows driver simplification (Exynos
> rotator driver smaller by over 200 lines).
> 
> Just a side note, we have written code in GStreamer using the Exnynos 4
> FIMC IPP driver. I don't know how many, if any, deployment still exist
> (Exynos 4 is relatively old now), but there exist userspace for the
> FIMC driver. We use this for color transformation (from tiled to
> linear) and scaling. The FIMC driver is in fact quite stable in
> upstream kernel today. The GScaler V4L2 M2M driver on Exynos 5 is
> largely based on it and has received some maintenance to properly work
> in GStreamer. unlike this DRM API, you can reuse the same userspace
> code across multiple platforms (which we do already). We have also
> integrated this driver in Chromium in the past (not upstream though).
> 
> I am well aware that the blitter driver has not got much attention
> though. But again, V4L2 offers a generic interface to userspace
> application. Fixing this driver could enable some work like this one:
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=772766
> 
> This work in progress feature is a generic hardware accelerated video
> mixer. It has been tested with IMX.6 v4l2 m2m blitter driver (which I
> believe is in staging right now). Again, unlike the exynos/drm, this
> code could be reused between platforms.
> 
> In general,

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

2017-04-26 Thread Nicolas Dufresne

Le mercredi 26 avril 2017 à 01:21 +0300, Sakari Ailus a écrit :
> Hi Marek,
> 
> On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
> > Hi Laurent,
> > 
> > On 2017-04-20 12:25, Laurent Pinchart wrote:
> > > Hi Marek,
> > > 
> > > (CC'ing Sakari Ailus)
> > > 
> > > Thank you for the patches.
> > > 
> > > On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
> > > > Dear all,
> > > > 
> > > > This is an updated proposal for extending EXYNOS DRM API with generic
> > > > support for hardware modules, which can be used for processing image 
> > > > data
> > > > from the one memory buffer to another. Typical memory-to-memory 
> > > > operations
> > > > are: rotation, scaling, colour space conversion or mix of them. This is 
> > > > a
> > > > follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
> > > > processors", which has been rejected as "not really needed in the DRM
> > > > core":
> > > > http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
> > > > 
> > > > In this proposal I moved all the code to Exynos DRM driver, so now this
> > > > will be specific only to Exynos DRM. I've also changed the name from
> > > > framebuffer processor (fbproc) to picture processor (pp) to avoid 
> > > > confusion
> > > > with fbdev API.
> > > > 
> > > > Here is a bit more information what picture processors are:
> > > > 
> > > > Embedded SoCs are known to have a number of hardware blocks, which 
> > > > perform
> > > > such operations. They can be used in paralel to the main GPU module to
> > > > offload CPU from processing grapics or video data. One of example use of
> > > > such modules is implementing video overlay, which usually requires color
> > > > space conversion from NV12 (or similar) to RGB32 color space and 
> > > > scaling to
> > > > target window size.
> > > > 
> > > > The proposed API is heavily inspired by atomic KMS approach - it is also
> > > > based on DRM objects and their properties. A new DRM object is 
> > > > introduced:
> > > > picture processor (called pp for convenience). Such objects have a set 
> > > > of
> > > > standard DRM properties, which describes the operation to be performed 
> > > > by
> > > > respective hardware module. In typical case those properties are a 
> > > > source
> > > > fb id and rectangle (x, y, width, height) and destination fb id and
> > > > rectangle. Optionally a rotation property can be also specified if
> > > > supported by the given hardware. To perform an operation on image data,
> > > > userspace provides a set of properties and their values for given fbproc
> > > > object in a similar way as object and properties are provided for
> > > > performing atomic page flip / mode setting.
> > > > 
> > > > The proposed API consists of the 3 new ioctls:
> > > > - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
> > > >   processors,
> > > > - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
> > > >   processor,
> > > > - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
> > > >   property set.
> > > > 
> > > > The proposed API is extensible. Drivers can attach their own, custom
> > > > properties to add support for more advanced picture processing (for 
> > > > example
> > > > blending).
> > > > 
> > > > This proposal aims to replace Exynos DRM IPP (Image Post Processing)
> > > > subsystem. IPP API is over-engineered in general, but not really 
> > > > extensible
> > > > on the other side. It is also buggy, with significant design flaws - the
> > > > biggest issue is the fact that the API covers memory-2-memory picture
> > > > operations together with CRTC writeback and duplicating features, which
> > > > belongs to video plane. Comparing with IPP subsystem, the PP framework 
> > > > is
> > > > smaller (1807 vs 778 lines) and allows driver simplification (Exynos
> > > > rotator driver smaller by over 200 lines).

Just a side note, we have written code in GStreamer using the Exnynos 4
FIMC IPP driver. I don't know how many, if any, deployment still exist
(Exynos 4 is relatively old now), but there exist userspace for the
FIMC driver. We use this for color transformation (from tiled to
linear) and scaling. The FIMC driver is in fact quite stable in
upstream kernel today. The GScaler V4L2 M2M driver on Exynos 5 is
largely based on it and has received some maintenance to properly work
in GStreamer. unlike this DRM API, you can reuse the same userspace
code across multiple platforms (which we do already). We have also
integrated this driver in Chromium in the past (not upstream though).

I am well aware that the blitter driver has not got much attention
though. But again, V4L2 offers a generic interface to userspace
application. Fixing this driver could enable some work like this one:

https://bugzilla.gnome.org/show_bug.cgi?id=772766

This work in progress feature is a generic hardware accelerated video
mixer. It has been tested with IMX.6 v4l2 m2m blitter driver

Re: [PATCH] dma-buf: avoid scheduling on fence status query v2

2017-04-26 Thread Christian König


Am 26.04.2017 um 16:46 schrieb Andres Rodriguez:

When a timeout of zero is specified, the caller is only interested in
the fence status.

In the current implementation, dma_fence_default_wait will always call
schedule_timeout() at least once for an unsignaled fence. This adds a
significant overhead to a fence status query.

Avoid this overhead by returning early if a zero timeout is specified.

v2: move early return after enable_signaling

Signed-off-by: Andres Rodriguez 


Reviewed-by: Christian König 


---

  If I'm understanding correctly, I don't think we need to register the
  default wait callback. But if that isn't the case please let me know.

  This patch has the same perf improvements as v1.

  drivers/dma-buf/dma-fence.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 0918d3f..57da14c 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -402,6 +402,11 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, 
signed long timeout)
}
}
  
+	if (!timeout) {

+   ret = 0;
+   goto out;
+   }
+
cb.base.func = dma_fence_default_wait_cb;
cb.task = current;
list_add(, >cb_list);

[PATCH] dma-buf: avoid scheduling on fence status query v2

2017-04-26 Thread Andres Rodriguez

When a timeout of zero is specified, the caller is only interested in
the fence status.

In the current implementation, dma_fence_default_wait will always call
schedule_timeout() at least once for an unsignaled fence. This adds a
significant overhead to a fence status query.

Avoid this overhead by returning early if a zero timeout is specified.

v2: move early return after enable_signaling

Signed-off-by: Andres Rodriguez 
---

 If I'm understanding correctly, I don't think we need to register the
 default wait callback. But if that isn't the case please let me know.

 This patch has the same perf improvements as v1.

 drivers/dma-buf/dma-fence.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 0918d3f..57da14c 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -402,6 +402,11 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, 
signed long timeout)
}
}
 
+   if (!timeout) {
+   ret = 0;
+   goto out;
+   }
+
cb.base.func = dma_fence_default_wait_cb;
cb.task = current;
list_add(, >cb_list);
-- 
2.9.3

Re: [PATCH] dma-buf: avoid scheduling on fence status query

2017-04-26 Thread Andres Rodriguez




On 2017-04-26 06:13 AM, Christian König wrote:

Am 26.04.2017 um 11:59 schrieb Dave Airlie:

On 26 April 2017 at 17:20, Christian König 
wrote:

NAK, I'm wondering how often I have to reject that change. We should
probably add a comment here.

Even with a zero timeout we still need to enable signaling, otherwise
some
fence will never signal if userspace just polls on them.

If a caller is only interested in the fence status without enabling the
signaling it should call dma_fence_is_signaled() instead.

Can we not move the return 0 (with spin unlock) down after we enabling
signalling, but before
we enter the schedule_timeout(1)?


Yes, that would be an option.



I was actually arguing with Dave about this on IRC yesterday. Seems like 
I owe him a beer now.


-Andres


Christian.



Dave.

[patch] autogain support for bayer10 format (was Re: [patch] propagating controls in libv4l2)

2017-04-26 Thread Pavel Machek

Hi!

> > > I don't see why it would be hard to open files or have threads inside
> > > a library. There are several libraries that do that already, specially
> > > the ones designed to be used on multimidia apps.  
> > 
> > Well, This is what the libv4l2 says:
> > 
> >This file implements libv4l2, which offers v4l2_ prefixed versions
> >of
> >   open/close/etc. The API is 100% the same as directly opening
> >/dev/videoX
> >   using regular open/close/etc, the big difference is that format
> >conversion
> >
> > but if I open additional files in v4l2_open(), API is no longer the
> > same, as unix open() is defined to open just one file descriptor.
> > 
> > Now. There is autogain support in libv4lconvert, but it expects to use
> > same fd for camera and for the gain... which does not work with
> > subdevs.
> > 
> > Of course, opening subdevs by name like this is not really
> > acceptable. But can you suggest a method that is?
> 
> There are two separate things here:
> 
> 1) Autofoucs for a device that doesn't use subdev API
> 2) libv4l2 support for devices that require MC and subdev API

Actually there are three: 0) autogain. Unfortunately, I need autogain
first before autofocus has a chance...

And that means... bayer10 support for autogain.

Plus, I changed avg_lum to long long. Quick calculation tells me int
could overflow with few megapixel sensor.

Oh, btw http://ytse.tricolour.net/docs/LowLightOptimization.html no
longer works.

Regards,
Pavel

diff --git a/lib/libv4lconvert/processing/autogain.c 
b/lib/libv4lconvert/processing/autogain.c
index c6866d6..0b52d0f 100644
--- a/lib/libv4lconvert/processing/autogain.c
+++ b/lib/libv4lconvert/processing/autogain.c
@@ -68,6 +71,41 @@ static void autogain_adjust(struct v4l2_queryctrl *ctrl, int 
*value,
}
 }
 
+static int get_luminosity_bayer10(uint16_t *buf, const struct v4l2_format *fmt)
+{
+   long long avg_lum = 0;
+   int x, y;
+   
+   buf += fmt->fmt.pix.height * fmt->fmt.pix.bytesperline / 4 +
+   fmt->fmt.pix.width / 4;
+
+   for (y = 0; y < fmt->fmt.pix.height / 2; y++) {
+   for (x = 0; x < fmt->fmt.pix.width / 2; x++)
+   avg_lum += *buf++;
+   buf += fmt->fmt.pix.bytesperline - fmt->fmt.pix.width / 2;
+   }
+   avg_lum /= fmt->fmt.pix.height * fmt->fmt.pix.width / 4;
+   avg_lum /= 4;
+   return avg_lum;
+}
+
+static int get_luminosity_bayer8(unsigned char *buf, const struct v4l2_format 
*fmt)
+{
+   long long avg_lum = 0;
+   int x, y;
+   
+   buf += fmt->fmt.pix.height * fmt->fmt.pix.bytesperline / 4 +
+   fmt->fmt.pix.width / 4;
+
+   for (y = 0; y < fmt->fmt.pix.height / 2; y++) {
+   for (x = 0; x < fmt->fmt.pix.width / 2; x++)
+   avg_lum += *buf++;
+   buf += fmt->fmt.pix.bytesperline - fmt->fmt.pix.width / 2;
+   }
+   avg_lum /= fmt->fmt.pix.height * fmt->fmt.pix.width / 4;
+   return avg_lum;
+}
+
 /* auto gain and exposure algorithm based on the knee algorithm described here:
 http://ytse.tricolour.net/docs/LowLightOptimization.html */
 static int autogain_calculate_lookup_tables(
@@ -100,17 +142,16 @@ static int autogain_calculate_lookup_tables(
switch (fmt->fmt.pix.pixelformat) {
+   case V4L2_PIX_FMT_SGBRG10:
+   case V4L2_PIX_FMT_SGRBG10:
+   case V4L2_PIX_FMT_SBGGR10:
+   case V4L2_PIX_FMT_SRGGB10:
+   avg_lum = get_luminosity_bayer10((void *) buf, fmt);
+   break;
+
case V4L2_PIX_FMT_SGBRG8:
case V4L2_PIX_FMT_SGRBG8:
case V4L2_PIX_FMT_SBGGR8:
case V4L2_PIX_FMT_SRGGB8:
-   buf += fmt->fmt.pix.height * fmt->fmt.pix.bytesperline / 4 +
-   fmt->fmt.pix.width / 4;
-
-   for (y = 0; y < fmt->fmt.pix.height / 2; y++) {
-   for (x = 0; x < fmt->fmt.pix.width / 2; x++)
-   avg_lum += *buf++;
-   buf += fmt->fmt.pix.bytesperline - fmt->fmt.pix.width / 
2;
-   }
-   avg_lum /= fmt->fmt.pix.height * fmt->fmt.pix.width / 4;
+   avg_lum = get_luminosity_bayer8(buf, fmt);
break;
 
case V4L2_PIX_FMT_RGB24:
diff --git a/lib/libv4lconvert/processing/libv4lprocessing.c 
b/lib/libv4lconvert/processing/libv4lprocessing.c
index b061f50..b98d024 100644
--- a/lib/libv4lconvert/processing/libv4lprocessing.c
+++ b/lib/libv4lconvert/processing/libv4lprocessing.c
@@ -164,6 +165,10 @@ void v4lprocessing_processing(struct v4lprocessing_data 
*data,
case V4L2_PIX_FMT_SGRBG8:
case V4L2_PIX_FMT_SBGGR8:
case V4L2_PIX_FMT_SRGGB8:
+   case V4L2_PIX_FMT_SGBRG10:
+   case V4L2_PIX_FMT_SGRBG10:
+   case V4L2_PIX_FMT_SBGGR10:
+   case V4L2_PIX_FMT_SRGGB10:
case V4L2_PIX_FMT_RGB24:
case

Re: [PATCH] rc-core: use the full 32 bits for NEC scancodes

2017-04-26 Thread Sean Young

On Tue, Apr 25, 2017 at 07:58:09AM +, David Härdeman wrote:
> April 24, 2017 5:58 PM, "Sean Young"  wrote:
> > On Tue, Apr 18, 2017 at 05:50:27PM +0200, David Härdeman wrote:
> >> Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
> >> and the nec decoder without any loss of functionality. At the same time
> >> it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
> >> removes lots of duplication (as you can see from the patch, the same NEC
> >> disambiguation logic is contained in several different drivers).
> >> 
> >> Using NEC32 also removes ambiguity. For example, consider these two NEC
> >> messages:
> >> NEC16 message to address 0x05, command 0x03
> >> NEC24 message to address 0x0005, command 0x03
> >> 
> >> They'll both have scancode 0x0503, and there's no way to tell which
> >> message was received.
> > 
> > More precisely, there is no way to tell which protocol variant it was sent
> > with.
> 
> Oh, but there is. The driver/rc-core will know. It's just that userspace 
> cannot ever know.

Agreed.

> > With the Sony and rc6 protocols, you can also get the same scancode from
> > different protocol variants. I think the right solution is to pass the 
> > protocol
> > variant to user space (and the keymap mapper).
> 
> Yes, I'm working on refreshing my patches to add a new 
> EVIOCGKEYCODE_V2/EVIOCSKEYCODE_V2 ioctl which includes the protocol.

That's a good idea and I look forward to those patches.

>  And actually, those patches are greatly simplified by only using NEC32.

At the moment your patches break userspace and I see no advantage in
representing a nec16 scancode as something like 0xe71824db rather than
0xe1724. It might reduce some code by a few lines/instructions.

Also note that having different nec variants makes a lot of sense, since
some hardware that decodes nec can only only handle specific variants,
e.g. nec16 only. By folding it all into nec32 you can longer specify
the specific variant(s) that some hardware can handle.

> > This also solves some other problems, e.g. rc6_6a_20:0x75460 is also decoded
> > by the sony protocol decoder (as scancode 0).
> 
> I know. And it also makes it possible to make /sys/class/rc/rc0/protocols 
> fully automatic. And we could theoretically also refuse to set unsupported 
> protocols in the keytable (not sure yet if that's something we should do).

Let's discuss that when we have the patches.

> >> In order to maintain backwards compatibility, some heuristics are added
> >> in rc-main.c to convert scancodes to NEC32 as necessary when userspace
> >> adds entries to the keytable using the regular input ioctls.
> > 
> > This is where it falls apart. In the patch below, you guess the protocol
> > variant from the scancode value. By your own example above, nec24 with
> > an address of 0x0005 would be not be possible in a keymap since it would
> > guessed as nec16 (see to_nec32() below) and expanded to 0x05fb03fc. An
> > actual nec24 would be 0x000503fc.
> 
> It's not 100% bulletproof. There's no way to fix this issue in a 100% 
> backwards compatible manner. But the future EVIOCGKEYCODE_V2/EVIOCSKEYCODE_V2 
> ioctl would make the heuristics unnecessary.

There must be a very good reason to break this and at the moment I can't
see any advantage (at all).


Sean

Re: [patch] propagating controls in libv4l2 was Re: support autofocus / autogain in libv4l2

2017-04-26 Thread Mauro Carvalho Chehab

Em Wed, 26 Apr 2017 12:53:00 +0200
Pavel Machek  escreveu:

> Hi!
> 
> > > > IMO, the best place for autofocus is at libv4l2. Putting it on a
> > > > separate "video server" application looks really weird for me.
> > > 
> > > Well... let me see. libraries are quite limited -- it is hard to open
> > > files, or use threads/have custom main loop. It may be useful to
> > > switch resolutions -- do autofocus/autogain at lower resolution, then
> > > switch to high one for taking picture. It would be good to have that
> > > in "system" code, but I'm not at all sure libv4l2 design will allow
> > > that.  
> > 
> > I don't see why it would be hard to open files or have threads inside
> > a library. There are several libraries that do that already, specially
> > the ones designed to be used on multimidia apps.  
> 
> Well, This is what the libv4l2 says:
> 
>This file implements libv4l2, which offers v4l2_ prefixed versions
>of
>   open/close/etc. The API is 100% the same as directly opening
>/dev/videoX
>   using regular open/close/etc, the big difference is that format
>conversion
>
> but if I open additional files in v4l2_open(), API is no longer the
> same, as unix open() is defined to open just one file descriptor.
> 
> Now. There is autogain support in libv4lconvert, but it expects to use
> same fd for camera and for the gain... which does not work with
> subdevs.
> 
> Of course, opening subdevs by name like this is not really
> acceptable. But can you suggest a method that is?
> 
> Thanks,
>   Pavel
> 
> commit 4cf9d10ead014c0db25452e4bb9cd144632407c3
> Author: Pavel 
> Date:   Wed Apr 26 11:38:04 2017 +0200
> 
> Add subdevices.
> 
> diff --git a/lib/libv4l2/libv4l2-priv.h b/lib/libv4l2/libv4l2-priv.h
> index 343db5e..a6bc48e 100644
> --- a/lib/libv4l2/libv4l2-priv.h
> +++ b/lib/libv4l2/libv4l2-priv.h
> @@ -26,6 +26,7 @@
>  #include "../libv4lconvert/libv4lsyscall-priv.h"
>  
>  #define V4L2_MAX_DEVICES 16
> +#define V4L2_MAX_SUBDEVS 8

Isn't it a short number?

>  /* Warning when making this larger the frame_queued and frame_mapped members 
> of
> the v4l2_dev_info struct can no longer be a bitfield, so the code needs to
> be adjusted! */
> @@ -104,6 +105,7 @@ struct v4l2_dev_info {
>   void *plugin_library;
>   void *dev_ops_priv;
>   const struct libv4l_dev_ops *dev_ops;
> +int subdev_fds[V4L2_MAX_SUBDEVS];
>  };
>  
>  /* From v4l2-plugin.c */
> diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
> index 0ba0a88..edc9642 100644
> --- a/lib/libv4l2/libv4l2.c
> +++ b/lib/libv4l2/libv4l2.c
> @@ -1,3 +1,4 @@
> +/* -*- c-file-style: "linux" -*- */

No emacs comments, please.

>  /*
>  # (C) 2008 Hans de Goede 
>  
> @@ -789,18 +790,25 @@ no_capture:
>  
>   /* Note we always tell v4lconvert to optimize src fmt selection for
>  our default fps, the only exception is the app explicitly selecting
> -a fram erate using the S_PARM ioctl after a S_FMT */
> +a frame rate using the S_PARM ioctl after a S_FMT */
>   if (devices[index].convert)
>   v4lconvert_set_fps(devices[index].convert, V4L2_DEFAULT_FPS);
>   v4l2_update_fps(index, );
>  
> + devices[index].subdev_fds[0] = SYS_OPEN("/dev/video_sensor", O_RDWR, 0);
> + devices[index].subdev_fds[1] = SYS_OPEN("/dev/video_focus", O_RDWR, 0);
> + devices[index].subdev_fds[2] = -1;

Hardcoding names here is not a good idea. Ideally, it should open
the MC, using the newgen API, and parse the media graph.

The problem is that, even with newgen API, without the properties API
you likely won't be able to write a generic parser. So, we need a
plugin specific for OMAP3 (or at least some database that would teach
a generic plugin about OMAP3 specifics).

I guess that the approach that Jacek was taken were very close to what
a generic plugin would need:
https://lwn.net/Articles/619449/

The last version of his patch set is here:
https://patchwork.linuxtv.org/patch/37496/

I didn't review his patchset, but from what I saw, Sakari is the one
that found some issues on v7.1 patchset.

Sakari,

Could you shed us a light about why this patchset was not merged?

Are there anything really bad at the code, or just minor issues that
could be fixed later?

If it is the last case, perhaps we could merge the code, if this
would make easier for Pavel to work on a N9 solution using the
same approach.


Thanks,
Mauro

Re: [patch] propagating controls in libv4l2 was Re: support autofocus / autogain in libv4l2

2017-04-26 Thread Mauro Carvalho Chehab

Hi Pavel,

Em Wed, 26 Apr 2017 12:53:00 +0200
Pavel Machek  escreveu:

> Hi!
> 
> > > > IMO, the best place for autofocus is at libv4l2. Putting it on a
> > > > separate "video server" application looks really weird for me.
> > > 
> > > Well... let me see. libraries are quite limited -- it is hard to open
> > > files, or use threads/have custom main loop. It may be useful to
> > > switch resolutions -- do autofocus/autogain at lower resolution, then
> > > switch to high one for taking picture. It would be good to have that
> > > in "system" code, but I'm not at all sure libv4l2 design will allow
> > > that.  
> > 
> > I don't see why it would be hard to open files or have threads inside
> > a library. There are several libraries that do that already, specially
> > the ones designed to be used on multimidia apps.  
> 
> Well, This is what the libv4l2 says:
> 
>This file implements libv4l2, which offers v4l2_ prefixed versions
>of
>   open/close/etc. The API is 100% the same as directly opening
>/dev/videoX
>   using regular open/close/etc, the big difference is that format
>conversion
>
> but if I open additional files in v4l2_open(), API is no longer the
> same, as unix open() is defined to open just one file descriptor.
> 
> Now. There is autogain support in libv4lconvert, but it expects to use
> same fd for camera and for the gain... which does not work with
> subdevs.
> 
> Of course, opening subdevs by name like this is not really
> acceptable. But can you suggest a method that is?

There are two separate things here:

1) Autofoucs for a device that doesn't use subdev API
2) libv4l2 support for devices that require MC and subdev API

for (1), it should use the /dev/videoX device that was opened with
v4l2_open().

For (2), libv4l2 should be aware of MC and subdev APIs. Sakari
once tried to write a libv4l2 plugin for OMAP3, but never finished it.
A more recent trial were to add a libv4l2 plugin for Exynos.
Unfortunately, none of those code got merged. Last time I checked,
the Exynos plugin was almost ready to be merged, but Sakari asked
some changes on it. The developer that was working on it got job on
some other company. Last time I heard from him, he was still interested
on finishing his work, but in the need to setup a test environment
using his own devices.

So, currently, there's no code at all adding MC/subdev API
support merged at libv4l2.

-

IMHO, the right thing to do with regards to autofocus is to
implement it via a processing module, assuming that just one
video device is opened.

Then, add a N900 plugin to make libv4l2 aware of OMAP3 specifics.

After that, rework at the processing module to let it use a 
different file descriptor if such plugin is in usage.

-

The hole idea is that a libv4l2 client, running on a N900 device
would just open a fake /dev/video0. Internally, libv4l2 will
open whatever video nodes it needs to control the device, exporting
all hardware capabilities (video formats, controls, resolutions,
etc) as if it was a normal V4L2 camera, hiding all dirty details
about MC and subdev APIs from userspace application.

This way, a normal application, like xawtv, tvtime, camorama,
zbar, mplayer, vlc, ... will work without any changes.


> 
> Thanks,
>   Pavel
> 
> commit 4cf9d10ead014c0db25452e4bb9cd144632407c3
> Author: Pavel 
> Date:   Wed Apr 26 11:38:04 2017 +0200
> 
> Add subdevices.
> 
> diff --git a/lib/libv4l2/libv4l2-priv.h b/lib/libv4l2/libv4l2-priv.h
> index 343db5e..a6bc48e 100644
> --- a/lib/libv4l2/libv4l2-priv.h
> +++ b/lib/libv4l2/libv4l2-priv.h
> @@ -26,6 +26,7 @@
>  #include "../libv4lconvert/libv4lsyscall-priv.h"
>  
>  #define V4L2_MAX_DEVICES 16
> +#define V4L2_MAX_SUBDEVS 8
>  /* Warning when making this larger the frame_queued and frame_mapped members 
> of
> the v4l2_dev_info struct can no longer be a bitfield, so the code needs to
> be adjusted! */
> @@ -104,6 +105,7 @@ struct v4l2_dev_info {
>   void *plugin_library;
>   void *dev_ops_priv;
>   const struct libv4l_dev_ops *dev_ops;
> +int subdev_fds[V4L2_MAX_SUBDEVS];
>  };
>  
>  /* From v4l2-plugin.c */
> diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
> index 0ba0a88..edc9642 100644
> --- a/lib/libv4l2/libv4l2.c
> +++ b/lib/libv4l2/libv4l2.c
> @@ -1,3 +1,4 @@
> +/* -*- c-file-style: "linux" -*- */
>  /*
>  # (C) 2008 Hans de Goede 
>  
> @@ -789,18 +790,25 @@ no_capture:
>  
>   /* Note we always tell v4lconvert to optimize src fmt selection for
>  our default fps, the only exception is the app explicitly selecting
> -a fram erate using the S_PARM ioctl after a S_FMT */
> +a frame rate using the S_PARM ioctl after a S_FMT */
>   if (devices[index].convert)
>   v4lconvert_set_fps(devices[index].convert, V4L2_DEFAULT_FPS);
>   v4l2_update_fps(index, );
>  
> +

Re: support autofocus / autogain in libv4l2

2017-04-26 Thread Pavel Machek

On Tue 2017-04-25 12:53:27, Nicolas Dufresne wrote:
> Le mardi 25 avril 2017 à 10:05 +0200, Pavel Machek a écrit :
> > Well, fd's are hard, because application can do fork() and now
> > interesting stuff happens. Threads are tricky, because now you have
> > locking etc.
> > 
> > libv4l2 is designed to be LD_PRELOADED. That is not really feasible
> > with "complex" library.
> 
> That is incorrect. The library propose an API where you simply replace
> certain low level calls, like ioctl -> v4l2_ioctl, open -> v4l2_open().
> You have to do that explicitly in your existing code. It does not
> abstract the API itself unlike libdrm.

You are right, no LD_PRELOAD. But same API as kernel, which is really
limiting -- see my other mail.

Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

[patch] propagating controls in libv4l2 was Re: support autofocus / autogain in libv4l2

2017-04-26 Thread Pavel Machek

Hi!

> > > IMO, the best place for autofocus is at libv4l2. Putting it on a
> > > separate "video server" application looks really weird for me.  
> > 
> > Well... let me see. libraries are quite limited -- it is hard to open
> > files, or use threads/have custom main loop. It may be useful to
> > switch resolutions -- do autofocus/autogain at lower resolution, then
> > switch to high one for taking picture. It would be good to have that
> > in "system" code, but I'm not at all sure libv4l2 design will allow
> > that.
> 
> I don't see why it would be hard to open files or have threads inside
> a library. There are several libraries that do that already, specially
> the ones designed to be used on multimidia apps.

Well, This is what the libv4l2 says:

   This file implements libv4l2, which offers v4l2_ prefixed versions
   of
  open/close/etc. The API is 100% the same as directly opening
   /dev/videoX
  using regular open/close/etc, the big difference is that format
   conversion
   
but if I open additional files in v4l2_open(), API is no longer the
same, as unix open() is defined to open just one file descriptor.

Now. There is autogain support in libv4lconvert, but it expects to use
same fd for camera and for the gain... which does not work with
subdevs.

Of course, opening subdevs by name like this is not really
acceptable. But can you suggest a method that is?

Thanks,
Pavel

commit 4cf9d10ead014c0db25452e4bb9cd144632407c3
Author: Pavel 
Date:   Wed Apr 26 11:38:04 2017 +0200

Add subdevices.

diff --git a/lib/libv4l2/libv4l2-priv.h b/lib/libv4l2/libv4l2-priv.h
index 343db5e..a6bc48e 100644
--- a/lib/libv4l2/libv4l2-priv.h
+++ b/lib/libv4l2/libv4l2-priv.h
@@ -26,6 +26,7 @@
 #include "../libv4lconvert/libv4lsyscall-priv.h"
 
 #define V4L2_MAX_DEVICES 16
+#define V4L2_MAX_SUBDEVS 8
 /* Warning when making this larger the frame_queued and frame_mapped members of
the v4l2_dev_info struct can no longer be a bitfield, so the code needs to
be adjusted! */
@@ -104,6 +105,7 @@ struct v4l2_dev_info {
void *plugin_library;
void *dev_ops_priv;
const struct libv4l_dev_ops *dev_ops;
+int subdev_fds[V4L2_MAX_SUBDEVS];
 };
 
 /* From v4l2-plugin.c */
diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
index 0ba0a88..edc9642 100644
--- a/lib/libv4l2/libv4l2.c
+++ b/lib/libv4l2/libv4l2.c
@@ -1,3 +1,4 @@
+/* -*- c-file-style: "linux" -*- */
 /*
 # (C) 2008 Hans de Goede 
 
@@ -789,18 +790,25 @@ no_capture:
 
/* Note we always tell v4lconvert to optimize src fmt selection for
   our default fps, the only exception is the app explicitly selecting
-  a fram erate using the S_PARM ioctl after a S_FMT */
+  a frame rate using the S_PARM ioctl after a S_FMT */
if (devices[index].convert)
v4lconvert_set_fps(devices[index].convert, V4L2_DEFAULT_FPS);
v4l2_update_fps(index, );
 
+   devices[index].subdev_fds[0] = SYS_OPEN("/dev/video_sensor", O_RDWR, 0);
+   devices[index].subdev_fds[1] = SYS_OPEN("/dev/video_focus", O_RDWR, 0);
+   devices[index].subdev_fds[2] = -1;
+
+   printf("Sensor: %d, focus: %d\n", devices[index].subdev_fds[0], 
+  devices[index].subdev_fds[1]);
+
V4L2_LOG("open: %d\n", fd);
 
return fd;
 }
 
 /* Is this an fd for which we are emulating v4l1 ? */
-static int v4l2_get_index(int fd)
+int v4l2_get_index(int fd)
 {
int index;
 

commit 1d6a9ce121f53e8f2e38549eed597a3c3dea5233
Author: Pavel 
Date:   Wed Apr 26 12:34:04 2017 +0200

Enable ioctl propagation.

diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
index edc9642..6dab661 100644
--- a/lib/libv4l2/libv4l2.c
+++ b/lib/libv4l2/libv4l2.c
@@ -1064,6 +1064,23 @@ static int v4l2_s_fmt(int index, struct v4l2_format 
*dest_fmt)
return 0;
 }
 
+static int v4l2_propagate_ioctl(int index, unsigned long request, void *arg)
+{
+   int i = 0;
+   int result;
+   while (1) {
+   if (devices[index].subdev_fds[i] == -1)
+   return -1;
+   printf("g_ctrl failed, trying...\n");
+   result = SYS_IOCTL(devices[index].subdev_fds[i], request, arg);
+   printf("subdev %d result %d\n", i, result);
+   if (result == 0)
+   return 0;
+   i++;
+   }
+   return -1;
+}
+
 int v4l2_ioctl(int fd, unsigned long int request, ...)
 {
void *arg;
@@ -1193,14 +1210,20 @@ no_capture_request:
switch (request) {
case VIDIOC_QUERYCTRL:
result = v4lconvert_vidioc_queryctrl(devices[index].convert, 
arg);
+   if (result == -1)
+   result = v4l2_propagate_ioctl(index, request, arg);
break;
 
case VIDIOC_G_CTRL:
result =

Re: [PATCH] dma-buf: avoid scheduling on fence status query

2017-04-26 Thread Christian König


Am 26.04.2017 um 11:59 schrieb Dave Airlie:

On 26 April 2017 at 17:20, Christian König  wrote:

NAK, I'm wondering how often I have to reject that change. We should
probably add a comment here.

Even with a zero timeout we still need to enable signaling, otherwise some
fence will never signal if userspace just polls on them.

If a caller is only interested in the fence status without enabling the
signaling it should call dma_fence_is_signaled() instead.

Can we not move the return 0 (with spin unlock) down after we enabling
signalling, but before
we enter the schedule_timeout(1)?


Yes, that would be an option.

Christian.



Dave.

Re: [PATCH] dma-buf: avoid scheduling on fence status query

2017-04-26 Thread Dave Airlie

On 26 April 2017 at 17:20, Christian König  wrote:
> NAK, I'm wondering how often I have to reject that change. We should
> probably add a comment here.
>
> Even with a zero timeout we still need to enable signaling, otherwise some
> fence will never signal if userspace just polls on them.
>
> If a caller is only interested in the fence status without enabling the
> signaling it should call dma_fence_is_signaled() instead.

Can we not move the return 0 (with spin unlock) down after we enabling
signalling, but before
we enter the schedule_timeout(1)?

Dave.

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

2017-04-26 Thread Simon Horman

On Wed, Apr 26, 2017 at 11:00:30AM +0200, Niklas Söderlund wrote:
> Hi Simon,
> 
> Thanks for your feedback.
> 
> On 2017-04-26 09:23:20 +0200, Simon Horman wrote:
> > Hi Kieran,
> > 
> > On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
> > > From: Kieran Bingham 
> > > 
> > > The rvin_digital_notify_bound() call dereferences the subdev->dev
> > > pointer to obtain the of_node. On some error paths, this dev node can be
> > > set as NULL. The of_node is mapped into the subdevice structure on
> > > initialisation, so this is a safer source to compare the nodes.
> > > 
> > > Dereference the of_node from the subdev structure instead of the dev
> > > structure.
> > > 
> > > Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
> > >   bound")
> > > Signed-off-by: Kieran Bingham 
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index 5861ab281150..a530dc388b95 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct 
> > > v4l2_async_notifier *notifier,
> > >  
> > >   v4l2_set_subdev_hostdata(subdev, vin);
> > >  
> > > - if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> > > + if (vin->digital.asd.match.of.node == subdev->of_node) {
> > >   /* Find surce and sink pad of remote subdevice */
> > >  
> > >   ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> > 
> > I see two different accesses to subdev->dev->of_node in the version of
> > rcar-core.c in linux-next. So I'm unsure if the following comment makes
> > sense in the context of the version you are working on. It is that
> > I wonder if all accesses to subdev->dev->of_node should be updated.
> 
> Are you sure you checked linux-next and not renesas-drivers? I checked 
> next-20170424.
> 
> $ git grep "dev->of_node" -- drivers/media/platform/rcar-vin/
> drivers/media/platform/rcar-vin/rcar-core.c:107:if 
> (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> drivers/media/platform/rcar-vin/rcar-core.c:161:ep = 
> of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> 
> Here vin->dev->of_node is correct and subdev->dev->of_node should be 
> fixed by Kieran patch. I'm only asking to be sure I did not miss 
> anything. In renesas-drivers the Gen3 patches are included and more 
> references to subdev->dev->of_node exists, but as Kieran sates these 
> fixes will be squashed into those patches since they are not yet picked 
> up.

I think we are seeing the same thing, sorry for the noise.

git show next-20170424:drivers/media/platform/rcar-vin/rcar-core.c | grep -A 3 
"dev->of_node"
if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
vin_dbg(vin, "bound digital subdev %s\n", subdev->name);
vin->digital.subdev = subdev;
return 0;
--
ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
if (!ep)
return 0;

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

2017-04-26 Thread Niklas Söderlund

Hi Simon,

Thanks for your feedback.

On 2017-04-26 09:23:20 +0200, Simon Horman wrote:
> Hi Kieran,
> 
> On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
> > From: Kieran Bingham 
> > 
> > The rvin_digital_notify_bound() call dereferences the subdev->dev
> > pointer to obtain the of_node. On some error paths, this dev node can be
> > set as NULL. The of_node is mapped into the subdevice structure on
> > initialisation, so this is a safer source to compare the nodes.
> > 
> > Dereference the of_node from the subdev structure instead of the dev
> > structure.
> > 
> > Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
> > bound")
> > Signed-off-by: Kieran Bingham 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> > b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 5861ab281150..a530dc388b95 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct 
> > v4l2_async_notifier *notifier,
> >  
> > v4l2_set_subdev_hostdata(subdev, vin);
> >  
> > -   if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> > +   if (vin->digital.asd.match.of.node == subdev->of_node) {
> > /* Find surce and sink pad of remote subdevice */
> >  
> > ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> 
> I see two different accesses to subdev->dev->of_node in the version of
> rcar-core.c in linux-next. So I'm unsure if the following comment makes
> sense in the context of the version you are working on. It is that
> I wonder if all accesses to subdev->dev->of_node should be updated.

Are you sure you checked linux-next and not renesas-drivers? I checked 
next-20170424.

$ git grep "dev->of_node" -- drivers/media/platform/rcar-vin/
drivers/media/platform/rcar-vin/rcar-core.c:107:if 
(vin->digital.asd.match.of.node == subdev->dev->of_node) {
drivers/media/platform/rcar-vin/rcar-core.c:161:ep = 
of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);

Here vin->dev->of_node is correct and subdev->dev->of_node should be 
fixed by Kieran patch. I'm only asking to be sure I did not miss 
anything. In renesas-drivers the Gen3 patches are included and more 
references to subdev->dev->of_node exists, but as Kieran sates these 
fixes will be squashed into those patches since they are not yet picked 
up.

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 01/21] scatterlist: Introduce sg_map helper functions

2017-04-26 Thread Christian König


Am 25.04.2017 um 20:20 schrieb Logan Gunthorpe:

This patch introduces functions which kmap the pages inside an sgl.
These functions replace a common pattern of kmap(sg_page(sg)) that is
used in more than 50 places within the kernel.

The motivation for this work is to eventually safely support sgls that
contain io memory. In order for that to work, any access to the contents
of an iomem SGL will need to be done with iomemcpy or hit some warning.
(The exact details of how this will work have yet to be worked out.)
Having all the kmaps in one place is just a first step in that
direction. Additionally, seeing this helps cut down the users of sg_page,
it should make any effort to go to struct-page-less DMAs a little
easier (should that idea ever swing back into favour again).

A flags option is added to select between a regular or atomic mapping so
these functions can replace kmap(sg_page or kmap_atomic(sg_page.
Future work may expand this to have flags for using page_address or
vmap. We include a flag to require the function not to fail to
support legacy code that has no easy error path. Much further in the
future, there may be a flag to allocate memory and copy the data
from/to iomem.

We also add the semantic that sg_map can fail to create a mapping,
despite the fact that the current code this is replacing is assumed to
never fail and the current version of these functions cannot fail. This
is to support iomem which may either have to fail to create the mapping or
allocate memory as a bounce buffer which itself can fail.

Also, in terms of cleanup, a few of the existing kmap(sg_page) users
play things a bit loose in terms of whether they apply sg->offset
so using these helper functions should help avoid such issues.

Signed-off-by: Logan Gunthorpe 
---


Good to know that somebody is working on this. Those problems troubled 
us as well.


Patch is Acked-by: Christian König .

Regards,
Christian.


  include/linux/scatterlist.h | 85 +
  1 file changed, 85 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index cb3c8fe..fad170b 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -5,6 +5,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  
  struct scatterlist {

@@ -126,6 +127,90 @@ static inline struct page *sg_page(struct scatterlist *sg)
return (struct page *)((sg)->page_link & ~0x3);
  }
  
+#define SG_KMAP		 (1 << 0)	/* create a mapping with kmap */

+#define SG_KMAP_ATOMIC  (1 << 1) /* create a mapping with kmap_atomic 
*/
+#define SG_MAP_MUST_NOT_FAIL (1 << 2)/* indicate sg_map should not fail */
+
+/**
+ * sg_map - kmap a page inside an sgl
+ * @sg:SG entry
+ * @offset:Offset into entry
+ * @flags: Flags for creating the mapping
+ *
+ * Description:
+ *   Use this function to map a page in the scatterlist at the specified
+ *   offset. sg->offset is already added for you. Note: the semantics of
+ *   this function are that it may fail. Thus, its output should be checked
+ *   with IS_ERR and PTR_ERR. Otherwise, a pointer to the specified offset
+ *   in the mapped page is returned.
+ *
+ *   Flags can be any of:
+ * * SG_KMAP   - Use kmap to create the mapping
+ * * SG_KMAP_ATOMIC- Use kmap_atomic to map the page atommically.
+ *   Thus, the rules of that function apply: the
+ *   cpu may not sleep until it is unmaped.
+ * * SG_MAP_MUST_NOT_FAIL  - Indicate that sg_map must not fail.
+ *   If it does, it will issue a BUG_ON instead.
+ *   This is intended for legacy code only, it
+ *   is not to be used in new code.
+ *
+ *   Also, consider carefully whether this function is appropriate. It is
+ *   largely not recommended for new code and if the sgl came from another
+ *   subsystem and you don't know what kind of memory might be in the list
+ *   then you definitely should not call it. Non-mappable memory may be in
+ *   the sgl and thus this function may fail unexpectedly. Consider using
+ *   sg_copy_to_buffer instead.
+ **/
+static inline void *sg_map(struct scatterlist *sg, size_t offset, int flags)
+{
+   struct page *pg;
+   unsigned int pg_off;
+   void *ret;
+
+   offset += sg->offset;
+   pg = nth_page(sg_page(sg), offset >> PAGE_SHIFT);
+   pg_off = offset_in_page(offset);
+
+   if (flags & SG_KMAP_ATOMIC)
+   ret = kmap_atomic(pg) + pg_off;
+   else if (flags & SG_KMAP)
+   ret = kmap(pg) + pg_off;
+   else
+   ret = ERR_PTR(-EINVAL);
+
+   /*
+* In theory, this can't happen yet. Once we start adding
+* unmapable memory, it also shouldn't happen unless developers
+* start putting unmappable struct pages in

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

2017-04-26 Thread Simon Horman

On Wed, Apr 26, 2017 at 08:48:25AM +0100, Kieran Bingham wrote:
> Hi Simon,
> 
> On 26/04/17 08:23, Simon Horman wrote:
> > Hi Kieran,
> > 
> > On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
> >> From: Kieran Bingham 
> >>
> >> The rvin_digital_notify_bound() call dereferences the subdev->dev
> >> pointer to obtain the of_node. On some error paths, this dev node can be
> >> set as NULL. The of_node is mapped into the subdevice structure on
> >> initialisation, so this is a safer source to compare the nodes.
> >>
> >> Dereference the of_node from the subdev structure instead of the dev
> >> structure.
> >>
> >> Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
> >>bound")
> >> Signed-off-by: Kieran Bingham 
> >> ---
> >>  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> >> b/drivers/media/platform/rcar-vin/rcar-core.c
> >> index 5861ab281150..a530dc388b95 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> >> @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct 
> >> v4l2_async_notifier *notifier,
> >>  
> >>v4l2_set_subdev_hostdata(subdev, vin);
> >>  
> >> -  if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> >> +  if (vin->digital.asd.match.of.node == subdev->of_node) {
> >>/* Find surce and sink pad of remote subdevice */
> >>  
> >>ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> > 
> > I see two different accesses to subdev->dev->of_node in the version of
> > rcar-core.c in linux-next. So I'm unsure if the following comment makes
> > sense in the context of the version you are working on. It is that
> > I wonder if all accesses to subdev->dev->of_node should be updated.
> 
> Yes, all uses in rcar-core should be updated, This patch is targeted directly 
> at
> mainline, in which only one reference occurs.
> 
> I presume(?) the references in linux-next, relate to the previous version of
> this patch which I posted as a reply to Niklas' patch series:
>  "[PATCH v3 00/27] rcar-vin: Add Gen3 with media controller support"
> 
> The first version of this patch (which was titled differently) covered three
> uses, but two of them were not yet in mainline.
> 
> The 'fixes' for those references are going to be squashed in to Niklas' next
> version of his patchset.

Understood, sounds good to me.

Re: [PATCH v2 02/21] libiscsi: Add an internal error code

2017-04-26 Thread Christoph Hellwig

On Tue, Apr 25, 2017 at 12:20:49PM -0600, Logan Gunthorpe wrote:
> This is a prep patch to add a new error code to libiscsi. We want to
> rework some kmap calls to be able to fail. When we do, we'd like to
> use this error code.

The kmap case in iscsi_tcp_segment_map can already fail.  Please add
handling of that failure to this patch, and we should get it merged
ASAP.

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

2017-04-26 Thread Kieran Bingham

Hi Simon,

On 26/04/17 08:23, Simon Horman wrote:
> Hi Kieran,
> 
> On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
>> From: Kieran Bingham 
>>
>> The rvin_digital_notify_bound() call dereferences the subdev->dev
>> pointer to obtain the of_node. On some error paths, this dev node can be
>> set as NULL. The of_node is mapped into the subdevice structure on
>> initialisation, so this is a safer source to compare the nodes.
>>
>> Dereference the of_node from the subdev structure instead of the dev
>> structure.
>>
>> Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
>>  bound")
>> Signed-off-by: Kieran Bingham 
>> ---
>>  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
>> b/drivers/media/platform/rcar-vin/rcar-core.c
>> index 5861ab281150..a530dc388b95 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
>> @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct 
>> v4l2_async_notifier *notifier,
>>  
>>  v4l2_set_subdev_hostdata(subdev, vin);
>>  
>> -if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
>> +if (vin->digital.asd.match.of.node == subdev->of_node) {
>>  /* Find surce and sink pad of remote subdevice */
>>  
>>  ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> 
> I see two different accesses to subdev->dev->of_node in the version of
> rcar-core.c in linux-next. So I'm unsure if the following comment makes
> sense in the context of the version you are working on. It is that
> I wonder if all accesses to subdev->dev->of_node should be updated.

Yes, all uses in rcar-core should be updated, This patch is targeted directly at
mainline, in which only one reference occurs.

I presume(?) the references in linux-next, relate to the previous version of
this patch which I posted as a reply to Niklas' patch series:
 "[PATCH v3 00/27] rcar-vin: Add Gen3 with media controller support"

The first version of this patch (which was titled differently) covered three
uses, but two of them were not yet in mainline.

The 'fixes' for those references are going to be squashed in to Niklas' next
version of his patchset.

--
Regards

Kieran

Re: [PATCH v2 01/21] scatterlist: Introduce sg_map helper functions

2017-04-26 Thread Christoph Hellwig

On Tue, Apr 25, 2017 at 12:20:48PM -0600, Logan Gunthorpe wrote:
> This patch introduces functions which kmap the pages inside an sgl.
> These functions replace a common pattern of kmap(sg_page(sg)) that is
> used in more than 50 places within the kernel.
> 
> The motivation for this work is to eventually safely support sgls that
> contain io memory. In order for that to work, any access to the contents
> of an iomem SGL will need to be done with iomemcpy or hit some warning.
> (The exact details of how this will work have yet to be worked out.)

I think we'll at least need a draft of those to make sense of these
patches.  Otherwise they just look very clumsy.

> + *   Use this function to map a page in the scatterlist at the specified
> + *   offset. sg->offset is already added for you. Note: the semantics of
> + *   this function are that it may fail. Thus, its output should be checked
> + *   with IS_ERR and PTR_ERR. Otherwise, a pointer to the specified offset
> + *   in the mapped page is returned.
> + *
> + *   Flags can be any of:
> + *   * SG_KMAP   - Use kmap to create the mapping
> + *   * SG_KMAP_ATOMIC- Use kmap_atomic to map the page atommically.
> + * Thus, the rules of that function apply: the
> + * cpu may not sleep until it is unmaped.
> + *   * SG_MAP_MUST_NOT_FAIL  - Indicate that sg_map must not fail.
> + * If it does, it will issue a BUG_ON instead.
> + * This is intended for legacy code only, it
> + * is not to be used in new code.

I'm sorry but this API is just a trainwreck.  Right now we have the
nice little kmap_atomic API, which never fails and has a very nice
calling convention where we just pass back the return address, but does
not support sleeping inside the critical section.

And kmap, whіch may fail and requires the original page to be passed
back.  Anything that mixes these two concepts up is simply a non-starter.

Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function

2017-04-26 Thread Roger Pau Monné

On Tue, Apr 25, 2017 at 12:21:02PM -0600, Logan Gunthorpe wrote:
> Straightforward conversion to the new helper, except due to the lack
> of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in
> certain cases in the future.
> 
> Signed-off-by: Logan Gunthorpe 
> Cc: Boris Ostrovsky 
> Cc: Juergen Gross 
> Cc: Konrad Rzeszutek Wilk 
> Cc: "Roger Pau Monné" 
> ---
>  drivers/block/xen-blkfront.c | 20 +++-
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 3945963..ed62175 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req, struct 
> blkfront_ring_info *ri
>   BUG_ON(sg->offset + sg->length > PAGE_SIZE);
>  
>   if (setup.need_copy) {
> - setup.bvec_off = sg->offset;
> - setup.bvec_data = kmap_atomic(sg_page(sg));
> + setup.bvec_off = 0;
> + setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC |
> +  SG_MAP_MUST_NOT_FAIL);

I assume that sg_map already adds sg->offset to the address?

Also wondering whether we can get rid of bvec_off and just increment bvec_data,
adding Julien who IIRC added this code.

>   }
>  
>   gnttab_foreach_grant_in_range(sg_page(sg),
> @@ -827,7 +828,7 @@ static int blkif_queue_rw_req(struct request *req, struct 
> blkfront_ring_info *ri
> );
>  
>   if (setup.need_copy)
> - kunmap_atomic(setup.bvec_data);
> + sg_unmap(sg, setup.bvec_data, 0, SG_KMAP_ATOMIC);
>   }
>   if (setup.segments)
>   kunmap_atomic(setup.segments);
> @@ -1053,7 +1054,7 @@ static int xen_translate_vdev(int vdevice, int *minor, 
> unsigned int *offset)
>   case XEN_SCSI_DISK5_MAJOR:
>   case XEN_SCSI_DISK6_MAJOR:
>   case XEN_SCSI_DISK7_MAJOR:
> - *offset = (*minor / PARTS_PER_DISK) + 
> + *offset = (*minor / PARTS_PER_DISK) +
>   ((major - XEN_SCSI_DISK1_MAJOR + 1) * 16) +
>   EMULATED_SD_DISK_NAME_OFFSET;
>   *minor = *minor +
> @@ -1068,7 +1069,7 @@ static int xen_translate_vdev(int vdevice, int *minor, 
> unsigned int *offset)
>   case XEN_SCSI_DISK13_MAJOR:
>   case XEN_SCSI_DISK14_MAJOR:
>   case XEN_SCSI_DISK15_MAJOR:
> - *offset = (*minor / PARTS_PER_DISK) + 
> + *offset = (*minor / PARTS_PER_DISK) +
>   ((major - XEN_SCSI_DISK8_MAJOR + 8) * 16) +
>   EMULATED_SD_DISK_NAME_OFFSET;
>   *minor = *minor +
> @@ -1119,7 +1120,7 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
>   if (!VDEV_IS_EXTENDED(info->vdevice)) {
>   err = xen_translate_vdev(info->vdevice, , );
>   if (err)
> - return err; 
> + return err;

Cosmetic changes should go in a separate patch please.

Roger.

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

2017-04-26 Thread Simon Horman

Hi Kieran,

On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> The rvin_digital_notify_bound() call dereferences the subdev->dev
> pointer to obtain the of_node. On some error paths, this dev node can be
> set as NULL. The of_node is mapped into the subdevice structure on
> initialisation, so this is a safer source to compare the nodes.
> 
> Dereference the of_node from the subdev structure instead of the dev
> structure.
> 
> Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
>   bound")
> Signed-off-by: Kieran Bingham 
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index 5861ab281150..a530dc388b95 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct 
> v4l2_async_notifier *notifier,
>  
>   v4l2_set_subdev_hostdata(subdev, vin);
>  
> - if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> + if (vin->digital.asd.match.of.node == subdev->of_node) {
>   /* Find surce and sink pad of remote subdevice */
>  
>   ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);

I see two different accesses to subdev->dev->of_node in the version of
rcar-core.c in linux-next. So I'm unsure if the following comment makes
sense in the context of the version you are working on. It is that
I wonder if all accesses to subdev->dev->of_node should be updated.

Re: [PATCH] dma-buf: avoid scheduling on fence status query

2017-04-26 Thread Christian König

NAK, I'm wondering how often I have to reject that change. We should 
probably add a comment here.


Even with a zero timeout we still need to enable signaling, otherwise 
some fence will never signal if userspace just polls on them.


If a caller is only interested in the fence status without enabling the 
signaling it should call dma_fence_is_signaled() instead.


Regards,
Christian.

Am 26.04.2017 um 04:50 schrieb Andres Rodriguez:

CC a few extra lists I missed.

Regards,
Andres

On 2017-04-25 09:36 PM, Andres Rodriguez wrote:

When a timeout of zero is specified, the caller is only interested in
the fence status.

In the current implementation, dma_fence_default_wait will always call
schedule_timeout() at least once for an unsignaled fence. This adds a
significant overhead to a fence status query.

Avoid this overhead by returning early if a zero timeout is specified.

Signed-off-by: Andres Rodriguez 
---

This heavily affects the performance of the Source2 engine running on
radv.

This patch improves dota2(radv) perf on a i7-6700k+RX480 system from
72fps->81fps.

 drivers/dma-buf/dma-fence.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 0918d3f..348e9e2 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -380,6 +380,9 @@ dma_fence_default_wait(struct dma_fence *fence, 
bool intr, signed long timeout)

 if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >flags))
 return ret;

+if (!timeout)
+return 0;
+
 spin_lock_irqsave(fence->lock, flags);

 if (intr && signal_pending(current)) {

40 matches

Mail list logo