cron job: media_tree daily build: ERRORS

[Hans Verkuil]

This message is generated daily by a cron job that builds media_tree for
the kernels and architectures in the list below.

Results of the daily build of media_tree:

date:   Thu Apr 12 05:00:18 CEST 2018
media-tree git hash:b284d4d5a6785f8cd07eda2646a95782373cd01e
media_build git hash:   541653bb52fcf7c34b8b83a4c8cc66b09c68ac37
v4l-utils git hash: 47d43b130dc6e9e0edc900759fb37649208371e4
gcc version:i686-linux-gcc (GCC) 7.3.0
sparse version: v0.5.2-rc1
smatch version: v0.5.0-4419-g3b5bf5c9
host hardware:  x86_64
host os:4.14.0-3-amd64

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

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

[PATCH] media: vcodec: fix ptr_ret.cocci warnings

[kbuild test robot]

From: Fengguang Wu 

drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1087:1-3: WARNING: 
PTR_ERR_OR_ZERO can be used


 Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR

Generated by: scripts/coccinelle/api/ptr_ret.cocci

Fixes: 648a9576932a ("media: vcodec: fix error return value from 
mtk_jpeg_clk_init()")
CC: Ryder Lee 
Signed-off-by: Fengguang Wu 
---

 mtk_jpeg_core.c |5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -1084,10 +1084,7 @@ static int mtk_jpeg_clk_init(struct mtk_
return PTR_ERR(jpeg->clk_jdec);
 
jpeg->clk_jdec_smi = devm_clk_get(jpeg->dev, "jpgdec-smi");
-   if (IS_ERR(jpeg->clk_jdec_smi))
-   return PTR_ERR(jpeg->clk_jdec_smi);
-
-   return 0;
+   return PTR_ERR_OR_ZERO(jpeg->clk_jdec_smi);
 }
 
 static int mtk_jpeg_probe(struct platform_device *pdev)

drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1087:1-3: WARNING: PTR_ERR_OR_ZERO can be used

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   8837c70d531a1788f975c366c254a5cb973a5291
commit: 648a9576932a26e1c6a157b4c9345204de975957 media: vcodec: fix error 
return value from mtk_jpeg_clk_init()
date:   7 days ago


coccinelle warnings: (new ones prefixed by >>)

>> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1087:1-3: WARNING: 
>> PTR_ERR_OR_ZERO can be used

Please review and possibly fold the followup patch.

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

[PATCH] media: cx88: enable IR transmitter on HVR-1300

[Sean Young]

The HVR 1300 has a Z8F0811 IR device, which can do both IR transmit
and receive. The transmit part was not probed.

Signed-off-by: Sean Young 
---
 drivers/media/pci/cx88/cx88-input.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/cx88/cx88-input.c 
b/drivers/media/pci/cx88/cx88-input.c
index 6f4e6923a91a..16233e837fcc 100644
--- a/drivers/media/pci/cx88/cx88-input.c
+++ b/drivers/media/pci/cx88/cx88-input.c
@@ -632,8 +632,9 @@ void cx88_i2c_init_ir(struct cx88_core *core)
memset(>init_data, 0, sizeof(core->init_data));
 
if (*addrp == 0x71) {
-   /* Hauppauge XVR */
-   core->init_data.name = "cx88 Hauppauge XVR remote";
+   /* Hauppauge Z8F0811 */
+   strlcpy(info.type, "ir_z8f0811_haup", I2C_NAME_SIZE);
+   core->init_data.name = core->board.name;
core->init_data.ir_codes = RC_MAP_HAUPPAUGE;
core->init_data.type = RC_PROTO_BIT_RC5 |
RC_PROTO_BIT_RC6_MCE | RC_PROTO_BIT_RC6_6A_32;
-- 
2.14.3

Emulated formats do not allow to use highest FPS of some cameras

[Ochi]

Hello,

I'm using a Logitech Brio webcam which is able to capture video at 60+ 
FPS when using MJPEG input format at e.g. 1280x720 pixels, but only up 
to 30 FPS when using other formats such as YUYV.


When using the cam with OBS Studio, the MJPEG input format is not 
currently supported by OBS so that I tried to choose one of the emulated 
formats. However, when using e.g. emulated YU12 or YV12 input formats, I 
can only choose up to 30 FPS.


I think that the reason for this is that the automatic ranking of 
available input formats by the function "v4lconvert_get_rank" in 
"libv4lconvert.c" uses a heuristic to rank formats which disregards 
characteristics of a format such as the maximum available FPS. Indeed, 
if I lower the rank (in this case lower == better) manually for MJPEG in 
said function, I can choose 60 or even 90 FPS in OBS when using one of 
the emulated formats.


Do you think a change of the heuristic used in "v4lconvert_get_rank" 
would be in order here, or do you have any other advice?


Best Regards
Sebastian

Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion

[Guennadi Liakhovetski]

On Wed, 11 Apr 2018, Kieran Bingham wrote:

> Hi Guennadi,
> 
> On 11/04/18 18:06, Guennadi Liakhovetski wrote:
> 
>  
> 
> 
>  Just figured out this commit
> 
>  From: Edgar Thier 
>  Date: Thu, 12 Oct 2017 03:54:17 -0400
>  Subject: [PATCH] media: uvcvideo: Apply flags from device to actual 
>  properties
> 
>  as the culprit. Without it everything is back to normal.
> >>>
> >>> I've already investigated and fixed this:
> >>>
> >>> Please apply:
> >>>   https://patchwork.kernel.org/patch/10299735/
> > 
> > Great, thanks! That seems to fix my problem.
> 
> Fantastic. I'm glad it helped.
> 
>  - Can I call that a Tested-by: ? :-D

Now you officially can - I replied to the patch.

Thanks
Guennadi

> 
> Regards
> 
> Kieran
> 

Re: [PATCH] media: uvcvideo: Prevent setting unavailable flags

[Guennadi Liakhovetski]

Hi Kieran,

Thanks for the patch, it fixed a problem I was having with media master, 
working with a Logitech UVC 1.5 camera.

On Wed, 21 Mar 2018, Kieran Bingham wrote:

> The addition of an extra operation to use the GET_INFO command
> overwrites all existing flags from the uvc_ctrls table. This includes
> setting all controls as supporting  GET_MIN, GET_MAX, GET_RES, and
> GET_DEF regardless of whether they do or not.
> 
> Move the initialisation of these control capabilities directly to the
> uvc_ctrl_fill_xu_info() call where they were originally located in that
> use case, and ensure that the new functionality in uvc_ctrl_get_flags()
> will only set flags based on their reported capability from the GET_INFO
> call.
> 
> Fixes: 859086ae3636 ("media: uvcvideo: Apply flags from device to actual
> properties")
> 
> Signed-off-by: Kieran Bingham 

Tested-by: Guennadi Liakhovetski 

Thanks
Guennadi

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c 
> b/drivers/media/usb/uvc/uvc_ctrl.c
> index 1daf444371be..4042cbdb721b 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1607,14 +1607,12 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
>   ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
>info->selector, data, 1);
>   if (!ret)
> - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> - | (data[0] & UVC_CONTROL_CAP_GET ?
> -UVC_CTRL_FLAG_GET_CUR : 0)
> - | (data[0] & UVC_CONTROL_CAP_SET ?
> -UVC_CTRL_FLAG_SET_CUR : 0)
> - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> -UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> + info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
> + UVC_CTRL_FLAG_GET_CUR : 0)
> + |  (data[0] & UVC_CONTROL_CAP_SET ?
> + UVC_CTRL_FLAG_SET_CUR : 0)
> + |  (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> + UVC_CTRL_FLAG_AUTO_UPDATE : 0);
>  
>   kfree(data);
>   return ret;
> @@ -1689,6 +1687,9 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>  
>   info->size = le16_to_cpup((__le16 *)data);
>  
> + info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF;
> +
>   ret = uvc_ctrl_get_flags(dev, ctrl, info);
>   if (ret < 0) {
>   uvc_trace(UVC_TRACE_CONTROL,
> -- 
> 2.7.4
> 

Re: [PATCH] media: ddbridge: better handle optional spin locks at the code

[Ralph Metzler]

Mauro Carvalho Chehab writes:
 > Em Wed, 11 Apr 2018 18:03:15 +0200
 > Daniel Scheller  escreveu:
 > 
 > > Am Wed, 11 Apr 2018 08:03:37 -0400
 > > schrieb Mauro Carvalho Chehab :
 > > 
 > > > Currently, ddbridge produces 4 warnings on sparse:
 > > >  drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context 
 > > > imbalance in 'ddb_output_start' - different lock contexts for basic block
 > > >  drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context 
 > > > imbalance in 'ddb_output_stop' - different lock contexts for basic block
 > > >  drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context 
 > > > imbalance in 'ddb_input_stop' - different lock contexts for basic block
 > > >  drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context 
 > > > imbalance in 'ddb_input_start' - different lock contexts for basic block
 > > > 
 > > > Those are all false positives, but they result from the fact that
 > > > there could potentially have some troubles at the locking schema,
 > > > because the lock depends on a var (output->dma).
 > > > 

Yeah, there were false positives on other parts of the driver where it was 
obvious to anybody
that the bad case cannot happen ...


 > > > I discussed that in priv with Sparse author and with the current
 > > > maintainer. Both believe that sparse is doing the right thing, and
 > > > that the proper fix would be to change the code to make it clearer
 > > > that, when spin_lock_irq() is called, spin_unlock_irq() will be
 > > > also called.
 > > > 
 > > > That help not only static analyzers to better understand the code,
 > > > but also humans that could need to take a look at the code.
 > > > 
 > > > It was also pointed that gcc would likely be smart enough to
 > > > optimize the code and produce the same result. I double
 > > > checked: indeed, the size of the driver didn't change after
 > > > this patch.
 > > > 
 > > > Signed-off-by: Mauro Carvalho Chehab 
 > > > ---
 > > >  drivers/media/pci/ddbridge/ddbridge-core.c | 43 
 > > > --
 > > >  1 file changed, 29 insertions(+), 14 deletions(-)
 > > > 
 > > > diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c 
 > > > b/drivers/media/pci/ddbridge/ddbridge-core.c
 > > > index 4a2819d3e225..080e2189ca7f 100644
 > > > --- a/drivers/media/pci/ddbridge/ddbridge-core.c
 > > > +++ b/drivers/media/pci/ddbridge/ddbridge-core.c
 > > > @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, 
 > > > u32 *con, u32 *con2, u32 flags)
 > > >  *con2 = (nco << 16) | gap;
 > > >  }
 > > >  
 > > > -static void ddb_output_start(struct ddb_output *output)
 > > > +static void __ddb_output_start(struct ddb_output *output)
 > > >  {
 > > >  struct ddb *dev = output->port->dev;
 > > >  u32 con = 0x11c, con2 = 0;
 > > >  
 > > >  if (output->dma) {
 > > > -spin_lock_irq(>dma->lock);
 > > >  output->dma->cbuf = 0;
 > > >  output->dma->coff = 0;
 > > >  output->dma->stat = 0;
 > > > @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output 
 > > > *output)
 > > >  
 > > >  ddbwritel(dev, con | 1, TS_CONTROL(output));
 > > >  
 > > > -if (output->dma) {
 > > > +if (output->dma)
 > > >  output->dma->running = 1;
 > > > +}
 > > > +
 > > > +static void ddb_output_start(struct ddb_output *output)
 > > > +{
 > > > +if (output->dma) {
 > > > +spin_lock_irq(>dma->lock);
 > > > +__ddb_output_start(output);
 > > >  spin_unlock_irq(>dma->lock);
 > > > +} else {
 > > > +__ddb_output_start(output);
 > > >  }
 > > >  }  

What does it say if you do:

struct ddb_dma *dma = output->dma;

if (dma) 
 lock;

...

if (dma)
 unlock;

?

If that does not work, what if you make dma const?

 > > 
 > > This makes things look rather strange (at least to my eyes), especially
 > > when simply trying to satisfy automated checkers, which in this case is
 > > useless since both lock and unlock will always happen based on the same
 > > condition ([input|output]->dma != NULL). Though I agree having the
 > > locking inside a condition in it's current form isn't optimal, too, and
 > > I also already thought about this in the past.
 > > 
 > > I'd rather try to fix this by checking for the dma ptrs at the
 > > beginning of the four functions and immediately return if the ptr is
 > > invalid. Though I don't know if this may cause side effects as there's
 > > data written to the regs pointed by the TS_CONTROL() macros even if
 > > there's no dma ptr present.

TS_CONTROL only controls the output itself and does not need to have DMA.


 > > 
 > > I'd like to hear Ralph's opinion on this, and also like to have this
 > > changed (in whatever way) in the upstream (dddvb) repository, too.
 > > 
 > > Please 

Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion

[Kieran Bingham]

Hi Guennadi,

On 11/04/18 18:06, Guennadi Liakhovetski wrote:

 


 Just figured out this commit

 From: Edgar Thier 
 Date: Thu, 12 Oct 2017 03:54:17 -0400
 Subject: [PATCH] media: uvcvideo: Apply flags from device to actual 
 properties

 as the culprit. Without it everything is back to normal.
>>>
>>> I've already investigated and fixed this:
>>>
>>> Please apply:
>>> https://patchwork.kernel.org/patch/10299735/
> 
> Great, thanks! That seems to fix my problem.

Fantastic. I'm glad it helped.

 - Can I call that a Tested-by: ? :-D

Regards

Kieran

Re: [PATCH] media: ddbridge: better handle optional spin locks at the code

[Daniel Scheller]

Am Wed, 11 Apr 2018 13:33:02 -0300
schrieb Mauro Carvalho Chehab :

> Em Wed, 11 Apr 2018 18:03:15 +0200
> Daniel Scheller  escreveu:
> 
> > Am Wed, 11 Apr 2018 08:03:37 -0400
> > schrieb Mauro Carvalho Chehab :
> >   
> > > Currently, ddbridge produces 4 warnings on sparse:
> > >   drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context 
> > > imbalance in 'ddb_output_start' - different lock contexts for basic block
> > >   drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context 
> > > imbalance in 'ddb_output_stop' - different lock contexts for basic block
> > >   drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context 
> > > imbalance in 'ddb_input_stop' - different lock contexts for basic block
> > >   drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context 
> > > imbalance in 'ddb_input_start' - different lock contexts for basic block
> > > 
> > > Those are all false positives, but they result from the fact that
> > > there could potentially have some troubles at the locking schema,
> > > because the lock depends on a var (output->dma).
> > > 
> > > I discussed that in priv with Sparse author and with the current
> > > maintainer. Both believe that sparse is doing the right thing, and
> > > that the proper fix would be to change the code to make it clearer
> > > that, when spin_lock_irq() is called, spin_unlock_irq() will be
> > > also called.
> > > 
> > > That help not only static analyzers to better understand the code,
> > > but also humans that could need to take a look at the code.
> > > 
> > > It was also pointed that gcc would likely be smart enough to
> > > optimize the code and produce the same result. I double
> > > checked: indeed, the size of the driver didn't change after
> > > this patch.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab 
> > > ---
> > >  drivers/media/pci/ddbridge/ddbridge-core.c | 43 
> > > --
> > >  1 file changed, 29 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c 
> > > b/drivers/media/pci/ddbridge/ddbridge-core.c
> > > index 4a2819d3e225..080e2189ca7f 100644
> > > --- a/drivers/media/pci/ddbridge/ddbridge-core.c
> > > +++ b/drivers/media/pci/ddbridge/ddbridge-core.c
> > > @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, u32 
> > > *con, u32 *con2, u32 flags)
> > >   *con2 = (nco << 16) | gap;
> > >  }
> > >  
> > > -static void ddb_output_start(struct ddb_output *output)
> > > +static void __ddb_output_start(struct ddb_output *output)
> > >  {
> > >   struct ddb *dev = output->port->dev;
> > >   u32 con = 0x11c, con2 = 0;
> > >  
> > >   if (output->dma) {
> > > - spin_lock_irq(>dma->lock);
> > >   output->dma->cbuf = 0;
> > >   output->dma->coff = 0;
> > >   output->dma->stat = 0;
> > > @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output 
> > > *output)
> > >  
> > >   ddbwritel(dev, con | 1, TS_CONTROL(output));
> > >  
> > > - if (output->dma) {
> > > + if (output->dma)
> > >   output->dma->running = 1;
> > > +}
> > > +
> > > +static void ddb_output_start(struct ddb_output *output)
> > > +{
> > > + if (output->dma) {
> > > + spin_lock_irq(>dma->lock);
> > > + __ddb_output_start(output);
> > >   spin_unlock_irq(>dma->lock);
> > > + } else {
> > > + __ddb_output_start(output);
> > >   }
> > >  }
> > 
> > This makes things look rather strange (at least to my eyes), especially
> > when simply trying to satisfy automated checkers, which in this case is
> > useless since both lock and unlock will always happen based on the same
> > condition ([input|output]->dma != NULL). Though I agree having the
> > locking inside a condition in it's current form isn't optimal, too, and
> > I also already thought about this in the past.
> > 
> > I'd rather try to fix this by checking for the dma ptrs at the
> > beginning of the four functions and immediately return if the ptr is
> > invalid. Though I don't know if this may cause side effects as there's
> > data written to the regs pointed by the TS_CONTROL() macros even if
> > there's no dma ptr present.
> > 
> > I'd like to hear Ralph's opinion on this, and also like to have this
> > changed (in whatever way) in the upstream (dddvb) repository, too.
> > 
> > Please refrain from applying this patch until we agreed on a proper
> > solution that works for everyone.  
> 
> Yeah, sure. 
> 
> Btw, does ddbridge driver works without DMA? On a quick look, it
> seems that it is enabled all the times.

DMA (and only this way of transportation) is used for all TS stream
input/output from/to demods and CI adapters (and the modulator cards in
the upstream driver) when driven by any of the PCIe bridges.

After another quick glance, [in|out]put->dma should really always be
set. In the end, they are pointers to 

Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion

[Guennadi Liakhovetski]

Hi Kieran,

On Wed, 11 Apr 2018, Kieran Bingham wrote:

> Hi Guennadi,
> 
> On 11/04/18 17:17, Kieran Bingham wrote:
> > Hi Guennadi,
> > 
> > On 11/04/18 10:56, Guennadi Liakhovetski wrote:
> >> Hi Laurent,
> >>
> >> Not sure whether that's a kernel or a user-space problem, but UVC related 
> >> anyway. I've got a UVC 1.5 (!) Logitech camera, that used to work fine 
> >> with earlier kernels. I now installed "media 4.16" and saw, that the 
> >> kernel log was filling with messages like
> >>
> >> uvcvideo: Failed to query (GET_MIN) UVC control 2 on unit 1: -32 (exp. 1).
> >>
> >> The expected /dev/video[01] nodes were not created correctly, and the 
> >> hard-drive was getting full very quickly. The latter was happening because 
> >> the the /var/log/uvcdynctrl-udev.log file was growing. A truncated sample 
> >> is attached. At its bottom you see messages
> >>
> >> [libwebcam] Warning: The driver behind device video0 has a slightly buggy 
> >> implementation
> >>   of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher
> >>   control ID if a control query fails. A workaround has been enabled.
> >>
> >> repeating, which continues even if the camera is unplugged. The kernel is 
> >> the head of the master branch of git://linuxtv.org/media_tree.git
> >>
> >> Just figured out this commit
> >>
> >> From: Edgar Thier 
> >> Date: Thu, 12 Oct 2017 03:54:17 -0400
> >> Subject: [PATCH] media: uvcvideo: Apply flags from device to actual 
> >> properties
> >>
> >> as the culprit. Without it everything is back to normal.
> > 
> > I've already investigated and fixed this:
> > 
> > Please apply:
> > https://patchwork.kernel.org/patch/10299735/

Great, thanks! That seems to fix my problem.

Regards
Guennadi

> > 
> > You stated that this is showing up on a v4.16 kernel ... but as far as I'm 
> > aware
> > - this feature shouldn't make it in until v4.17. Are you using linux-next 
> > or a
> > media/master or such ?
> 
> Aha - never mind - I just re-read your message.
> 
> I expect the fix to make it into the media mainline when Mauro pulls from
> Laurent? So I'm sure it will find its way soon.
> 
> --
> Cheers
> 
> Kieran
> 
> 
> > 
> > Regards
> > 
> > Kieran
> > 
> >> Thanks
> >> Guennadi
> >>
> 

Re: [PATCH] media: ddbridge: better handle optional spin locks at the code

[Mauro Carvalho Chehab]

Em Wed, 11 Apr 2018 18:03:15 +0200
Daniel Scheller  escreveu:

> Am Wed, 11 Apr 2018 08:03:37 -0400
> schrieb Mauro Carvalho Chehab :
> 
> > Currently, ddbridge produces 4 warnings on sparse:
> > drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context 
> > imbalance in 'ddb_output_start' - different lock contexts for basic block
> > drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context 
> > imbalance in 'ddb_output_stop' - different lock contexts for basic block
> > drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context 
> > imbalance in 'ddb_input_stop' - different lock contexts for basic block
> > drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context 
> > imbalance in 'ddb_input_start' - different lock contexts for basic block
> > 
> > Those are all false positives, but they result from the fact that
> > there could potentially have some troubles at the locking schema,
> > because the lock depends on a var (output->dma).
> > 
> > I discussed that in priv with Sparse author and with the current
> > maintainer. Both believe that sparse is doing the right thing, and
> > that the proper fix would be to change the code to make it clearer
> > that, when spin_lock_irq() is called, spin_unlock_irq() will be
> > also called.
> > 
> > That help not only static analyzers to better understand the code,
> > but also humans that could need to take a look at the code.
> > 
> > It was also pointed that gcc would likely be smart enough to
> > optimize the code and produce the same result. I double
> > checked: indeed, the size of the driver didn't change after
> > this patch.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >  drivers/media/pci/ddbridge/ddbridge-core.c | 43 
> > --
> >  1 file changed, 29 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c 
> > b/drivers/media/pci/ddbridge/ddbridge-core.c
> > index 4a2819d3e225..080e2189ca7f 100644
> > --- a/drivers/media/pci/ddbridge/ddbridge-core.c
> > +++ b/drivers/media/pci/ddbridge/ddbridge-core.c
> > @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, u32 
> > *con, u32 *con2, u32 flags)
> > *con2 = (nco << 16) | gap;
> >  }
> >  
> > -static void ddb_output_start(struct ddb_output *output)
> > +static void __ddb_output_start(struct ddb_output *output)
> >  {
> > struct ddb *dev = output->port->dev;
> > u32 con = 0x11c, con2 = 0;
> >  
> > if (output->dma) {
> > -   spin_lock_irq(>dma->lock);
> > output->dma->cbuf = 0;
> > output->dma->coff = 0;
> > output->dma->stat = 0;
> > @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output *output)
> >  
> > ddbwritel(dev, con | 1, TS_CONTROL(output));
> >  
> > -   if (output->dma) {
> > +   if (output->dma)
> > output->dma->running = 1;
> > +}
> > +
> > +static void ddb_output_start(struct ddb_output *output)
> > +{
> > +   if (output->dma) {
> > +   spin_lock_irq(>dma->lock);
> > +   __ddb_output_start(output);
> > spin_unlock_irq(>dma->lock);
> > +   } else {
> > +   __ddb_output_start(output);
> > }
> >  }  
> 
> This makes things look rather strange (at least to my eyes), especially
> when simply trying to satisfy automated checkers, which in this case is
> useless since both lock and unlock will always happen based on the same
> condition ([input|output]->dma != NULL). Though I agree having the
> locking inside a condition in it's current form isn't optimal, too, and
> I also already thought about this in the past.
> 
> I'd rather try to fix this by checking for the dma ptrs at the
> beginning of the four functions and immediately return if the ptr is
> invalid. Though I don't know if this may cause side effects as there's
> data written to the regs pointed by the TS_CONTROL() macros even if
> there's no dma ptr present.
> 
> I'd like to hear Ralph's opinion on this, and also like to have this
> changed (in whatever way) in the upstream (dddvb) repository, too.
> 
> Please refrain from applying this patch until we agreed on a proper
> solution that works for everyone.

Yeah, sure. 

Btw, does ddbridge driver works without DMA? On a quick look, it
seems that it is enabled all the times.


> 
> Best regards,
> Daniel Scheller



Thanks,
Mauro

Re: [PATCH v2] media: omap3isp: fix unbalanced dma_iommu_mapping

[Sakari Ailus]

On Mon, Mar 26, 2018 at 11:40:58AM -0500, Suman Anna wrote:
> Hi Mauro,
> 
> On 03/21/2018 05:26 AM, Laurent Pinchart wrote:
> > Hi Suman,
> > 
> > Thank you for the patch.
> > 
> > On Wednesday, 14 March 2018 17:41:36 EET Suman Anna wrote:
> >> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> >> ARM DMA backend. The current code creates a dma_iommu_mapping and
> >> attaches this to the ISP device, but never detaches the mapping in
> >> either the probe failure paths or the driver remove path resulting
> >> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> >>
> >> Reported-by: Pavel Machek 
> >> Signed-off-by: Suman Anna 
> >> Acked-by: Sakari Ailus 
> > 
> > Reviewed-by: Laurent Pinchart 
> 
> I don't see this patch in -next yet, can you pick up this patch at your
> earliest?

Here:



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

[GIT FIXES for 4.17] omap3isp IOMMU error handling fix

[Sakari Ailus]

Hi Mauro,

Here's a fix for the omap3isp driver's IOMMU error handling in probe.

Please pull.


The following changes since commit a95845ba184b854106972f5d8f50354c2d272c06:

  media: v4l2-core: fix size of devnode_nums[] bitarray (2018-04-05 06:41:30 
-0400)

are available in the git repository at:

  ssh://linuxtv.org/git/sailus/media_tree.git fixes-4.17-1

for you to fetch changes up to d4e0f478573131d424b7116cdb63f91e65164963:

  media: omap3isp: fix unbalanced dma_iommu_mapping (2018-04-11 18:07:37 +0300)


Suman Anna (1):
  media: omap3isp: fix unbalanced dma_iommu_mapping

 drivers/media/platform/omap3isp/isp.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
Kind regards,

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

Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion

[Kieran Bingham]

Hi Guennadi,

On 11/04/18 17:17, Kieran Bingham wrote:
> Hi Guennadi,
> 
> On 11/04/18 10:56, Guennadi Liakhovetski wrote:
>> Hi Laurent,
>>
>> Not sure whether that's a kernel or a user-space problem, but UVC related 
>> anyway. I've got a UVC 1.5 (!) Logitech camera, that used to work fine 
>> with earlier kernels. I now installed "media 4.16" and saw, that the 
>> kernel log was filling with messages like
>>
>> uvcvideo: Failed to query (GET_MIN) UVC control 2 on unit 1: -32 (exp. 1).
>>
>> The expected /dev/video[01] nodes were not created correctly, and the 
>> hard-drive was getting full very quickly. The latter was happening because 
>> the the /var/log/uvcdynctrl-udev.log file was growing. A truncated sample 
>> is attached. At its bottom you see messages
>>
>> [libwebcam] Warning: The driver behind device video0 has a slightly buggy 
>> implementation
>>   of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher
>>   control ID if a control query fails. A workaround has been enabled.
>>
>> repeating, which continues even if the camera is unplugged. The kernel is 
>> the head of the master branch of git://linuxtv.org/media_tree.git
>>
>> Just figured out this commit
>>
>> From: Edgar Thier 
>> Date: Thu, 12 Oct 2017 03:54:17 -0400
>> Subject: [PATCH] media: uvcvideo: Apply flags from device to actual 
>> properties
>>
>> as the culprit. Without it everything is back to normal.
> 
> I've already investigated and fixed this:
> 
> Please apply:
>   https://patchwork.kernel.org/patch/10299735/
> 
> You stated that this is showing up on a v4.16 kernel ... but as far as I'm 
> aware
> - this feature shouldn't make it in until v4.17. Are you using linux-next or a
> media/master or such ?

Aha - never mind - I just re-read your message.

I expect the fix to make it into the media mainline when Mauro pulls from
Laurent? So I'm sure it will find its way soon.

--
Cheers

Kieran


> 
> Regards
> 
> Kieran
> 
>> Thanks
>> Guennadi
>>

Re: a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion

[Kieran Bingham]

Hi Guennadi,

On 11/04/18 10:56, Guennadi Liakhovetski wrote:
> Hi Laurent,
> 
> Not sure whether that's a kernel or a user-space problem, but UVC related 
> anyway. I've got a UVC 1.5 (!) Logitech camera, that used to work fine 
> with earlier kernels. I now installed "media 4.16" and saw, that the 
> kernel log was filling with messages like
> 
> uvcvideo: Failed to query (GET_MIN) UVC control 2 on unit 1: -32 (exp. 1).
> 
> The expected /dev/video[01] nodes were not created correctly, and the 
> hard-drive was getting full very quickly. The latter was happening because 
> the the /var/log/uvcdynctrl-udev.log file was growing. A truncated sample 
> is attached. At its bottom you see messages
> 
> [libwebcam] Warning: The driver behind device video0 has a slightly buggy 
> implementation
>   of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher
>   control ID if a control query fails. A workaround has been enabled.
> 
> repeating, which continues even if the camera is unplugged. The kernel is 
> the head of the master branch of git://linuxtv.org/media_tree.git
> 
> Just figured out this commit
> 
> From: Edgar Thier 
> Date: Thu, 12 Oct 2017 03:54:17 -0400
> Subject: [PATCH] media: uvcvideo: Apply flags from device to actual properties
> 
> as the culprit. Without it everything is back to normal.

I've already investigated and fixed this:

Please apply:
https://patchwork.kernel.org/patch/10299735/

You stated that this is showing up on a v4.16 kernel ... but as far as I'm aware
- this feature shouldn't make it in until v4.17. Are you using linux-next or a
media/master or such ?

Regards

Kieran

> Thanks
> Guennadi
> 

Re: [RFCv11 PATCH 04/29] media-request: core request support

[Mauro Carvalho Chehab]

Em Wed, 11 Apr 2018 18:35:14 +0300
Sakari Ailus  escreveu:

> On Wed, Apr 11, 2018 at 12:17:27PM -0300, Mauro Carvalho Chehab wrote:
> > Em Wed, 11 Apr 2018 18:02:19 +0300
> > Sakari Ailus  escreveu:
> >   
> > > On Wed, Apr 11, 2018 at 10:49:35AM -0300, Mauro Carvalho Chehab wrote:  
> > > > Em Wed, 11 Apr 2018 16:21:16 +0300
> > > > Sakari Ailus  escreveu:
> > > > 
> > > > 
> > > > > > > > Btw, this is a very good reason why you should define the ioctl 
> > > > > > > > to
> > > > > > > > have an integer argument instead of a struct with a __s32 field
> > > > > > > > on it, as per my comment to patch 02/29:
> > > > > > > > 
> > > > > > > > #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int)
> > > > > > > > 
> > > > > > > > At 64 bit architectures, you're truncating the file descriptor! 
> > > > > > > >
> > > > > > > 
> > > > > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit 
> > > > > > > systems as
> > > > > > > well.  
> > > > > > 
> > > > > > Hmm.. you're right. I was thinking that it could be 64 bits on some
> > > > > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but,
> > > > > > according with:
> > > > > > 
> > > > > > https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html
> > > > > > 
> > > > > > This is not the case on gcc.  
> > > > > 
> > > > > Ok. The reasoning back then was that what "int" means varies across
> > > > > compilers and languages. And the intent was to codify this to __s32 
> > > > > which
> > > > > is what the kernel effectively uses.
> > > > 
> > > > ...
> > > > 
> > > > > The rest of the kernel uses int rather liberally in the uAPI so I'm 
> > > > > not
> > > > > sure in the end whether something desirable was achieved. Perhaps 
> > > > > it'd be
> > > > > good to go back to the original discussion to find out for sure.
> > > > > 
> > > > > Still binaries compiled with Tru64 C compiler wouldn't work on Linux 
> > > > > anyway
> > > > > due to that difference.
> > > > > 
> > > > > Well, I stop here for this begins to be off-topic. :-)
> > > > 
> > > > Yes. Let's keep it as s32 as originally proposed. Just ignore my 
> > > > comments
> > > > about that :-)
> > > > 
> > > > > > > > > + get_task_comm(comm, current);
> > > > > > > > > + snprintf(req->debug_str, sizeof(req->debug_str), 
> > > > > > > > > "%s:%d",
> > > > > > > > > +  comm, fd);
> > > > > > > > 
> > > > > > > > Not sure if it is a good idea to store the task that allocated
> > > > > > > > the request. While it makes sense for the dev_dbg() below, it
> > > > > > > > may not make sense anymore on other dev_dbg() you would be
> > > > > > > > using it.
> > > > > > > 
> > > > > > > The lifetime of the file handle roughly matches that of the 
> > > > > > > request. It's
> > > > > > > for debug only anyway.
> > > > > > > 
> > > > > > > Better proposals are always welcome of course. But I think we 
> > > > > > > should have
> > > > > > > something here that helps debugging by meaningfully making the 
> > > > > > > requests
> > > > > > > identifiable from logs.  
> > > > > > 
> > > > > > What I meant to say is that one PID could be allocating the
> > > > > > request, while some other one could be actually doing Q/DQ_BUF.
> > > > > > On such scenario, the debug string could provide mislead prints.
> > > > > >   
> > > > > 
> > > > > Um, yes, indeed it would no longer match the process. But the request 
> > > > > is
> > > > > still the same. That's actually a positive thing since it allows you 
> > > > > to
> > > > > identify the request.
> > > > > 
> > > > > With a global ID space this was trivial; you could just print the 
> > > > > request
> > > > > ID and that was all that was ever needed. (I'm not proposing to 
> > > > > consider
> > > > > that though.)
> > > > > 
> > > > 
> > > > IMO, a global ID number would work better than get_task_comm().
> > > > 
> > > > Just add a static int monotonic counter and use it for the debug 
> > > > purposes,
> > > > e. g.:
> > > > 
> > > > {
> > > > static unsigned int req_count = 0;
> > > > 
> > > > snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
> > > > req_count++, fd);
> > > > 
> > > > Ok, eventually, it will overflow, but, it will be unique within
> > > > a reasonable timeframe to be good enough for debugging purposes.
> > > 
> > > Yes, but you can't figure out which process allocated it anymore, making
> > > associating kernel debug logs with user space process logs harder.
> > > 
> > > How about process id + file handle? That still doesn't tell which process
> > > operated on the request though, but I'm not sure whether that's really a
> > > crucial piece of information.  
> > 
> > You don't need that. With dev_dbg() - and other *_dbg() macros - you can
> > enable process ID for all debug messages.  
> 
> With this, the problem again 

Re: [PATCH] media: ddbridge: better handle optional spin locks at the code

[Daniel Scheller]

Am Wed, 11 Apr 2018 08:03:37 -0400
schrieb Mauro Carvalho Chehab :

> Currently, ddbridge produces 4 warnings on sparse:
>   drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context 
> imbalance in 'ddb_output_start' - different lock contexts for basic block
>   drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context 
> imbalance in 'ddb_output_stop' - different lock contexts for basic block
>   drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context 
> imbalance in 'ddb_input_stop' - different lock contexts for basic block
>   drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context 
> imbalance in 'ddb_input_start' - different lock contexts for basic block
> 
> Those are all false positives, but they result from the fact that
> there could potentially have some troubles at the locking schema,
> because the lock depends on a var (output->dma).
> 
> I discussed that in priv with Sparse author and with the current
> maintainer. Both believe that sparse is doing the right thing, and
> that the proper fix would be to change the code to make it clearer
> that, when spin_lock_irq() is called, spin_unlock_irq() will be
> also called.
> 
> That help not only static analyzers to better understand the code,
> but also humans that could need to take a look at the code.
> 
> It was also pointed that gcc would likely be smart enough to
> optimize the code and produce the same result. I double
> checked: indeed, the size of the driver didn't change after
> this patch.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/pci/ddbridge/ddbridge-core.c | 43 
> --
>  1 file changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c 
> b/drivers/media/pci/ddbridge/ddbridge-core.c
> index 4a2819d3e225..080e2189ca7f 100644
> --- a/drivers/media/pci/ddbridge/ddbridge-core.c
> +++ b/drivers/media/pci/ddbridge/ddbridge-core.c
> @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, u32 
> *con, u32 *con2, u32 flags)
>   *con2 = (nco << 16) | gap;
>  }
>  
> -static void ddb_output_start(struct ddb_output *output)
> +static void __ddb_output_start(struct ddb_output *output)
>  {
>   struct ddb *dev = output->port->dev;
>   u32 con = 0x11c, con2 = 0;
>  
>   if (output->dma) {
> - spin_lock_irq(>dma->lock);
>   output->dma->cbuf = 0;
>   output->dma->coff = 0;
>   output->dma->stat = 0;
> @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output *output)
>  
>   ddbwritel(dev, con | 1, TS_CONTROL(output));
>  
> - if (output->dma) {
> + if (output->dma)
>   output->dma->running = 1;
> +}
> +
> +static void ddb_output_start(struct ddb_output *output)
> +{
> + if (output->dma) {
> + spin_lock_irq(>dma->lock);
> + __ddb_output_start(output);
>   spin_unlock_irq(>dma->lock);
> + } else {
> + __ddb_output_start(output);
>   }
>  }

This makes things look rather strange (at least to my eyes), especially
when simply trying to satisfy automated checkers, which in this case is
useless since both lock and unlock will always happen based on the same
condition ([input|output]->dma != NULL). Though I agree having the
locking inside a condition in it's current form isn't optimal, too, and
I also already thought about this in the past.

I'd rather try to fix this by checking for the dma ptrs at the
beginning of the four functions and immediately return if the ptr is
invalid. Though I don't know if this may cause side effects as there's
data written to the regs pointed by the TS_CONTROL() macros even if
there's no dma ptr present.

I'd like to hear Ralph's opinion on this, and also like to have this
changed (in whatever way) in the upstream (dddvb) repository, too.

Please refrain from applying this patch until we agreed on a proper
solution that works for everyone.

Best regards,
Daniel Scheller
-- 
https://github.com/herrnst

Re: [RFCv11 PATCH 04/29] media-request: core request support

[Sakari Ailus]

On Wed, Apr 11, 2018 at 12:17:27PM -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 11 Apr 2018 18:02:19 +0300
> Sakari Ailus  escreveu:
> 
> > On Wed, Apr 11, 2018 at 10:49:35AM -0300, Mauro Carvalho Chehab wrote:
> > > Em Wed, 11 Apr 2018 16:21:16 +0300
> > > Sakari Ailus  escreveu:
> > > 
> > >   
> > > > > > > Btw, this is a very good reason why you should define the ioctl to
> > > > > > > have an integer argument instead of a struct with a __s32 field
> > > > > > > on it, as per my comment to patch 02/29:
> > > > > > > 
> > > > > > >   #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int)
> > > > > > > 
> > > > > > > At 64 bit architectures, you're truncating the file descriptor!   
> > > > > > >
> > > > > > 
> > > > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit 
> > > > > > systems as
> > > > > > well.
> > > > > 
> > > > > Hmm.. you're right. I was thinking that it could be 64 bits on some
> > > > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but,
> > > > > according with:
> > > > > 
> > > > >   https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html
> > > > > 
> > > > > This is not the case on gcc.
> > > > 
> > > > Ok. The reasoning back then was that what "int" means varies across
> > > > compilers and languages. And the intent was to codify this to __s32 
> > > > which
> > > > is what the kernel effectively uses.  
> > > 
> > > ...
> > >   
> > > > The rest of the kernel uses int rather liberally in the uAPI so I'm not
> > > > sure in the end whether something desirable was achieved. Perhaps it'd 
> > > > be
> > > > good to go back to the original discussion to find out for sure.
> > > > 
> > > > Still binaries compiled with Tru64 C compiler wouldn't work on Linux 
> > > > anyway
> > > > due to that difference.
> > > > 
> > > > Well, I stop here for this begins to be off-topic. :-)  
> > > 
> > > Yes. Let's keep it as s32 as originally proposed. Just ignore my comments
> > > about that :-)
> > >   
> > > > > > > > +   get_task_comm(comm, current);
> > > > > > > > +   snprintf(req->debug_str, sizeof(req->debug_str), 
> > > > > > > > "%s:%d",
> > > > > > > > +comm, fd);  
> > > > > > > 
> > > > > > > Not sure if it is a good idea to store the task that allocated
> > > > > > > the request. While it makes sense for the dev_dbg() below, it
> > > > > > > may not make sense anymore on other dev_dbg() you would be
> > > > > > > using it.  
> > > > > > 
> > > > > > The lifetime of the file handle roughly matches that of the 
> > > > > > request. It's
> > > > > > for debug only anyway.
> > > > > > 
> > > > > > Better proposals are always welcome of course. But I think we 
> > > > > > should have
> > > > > > something here that helps debugging by meaningfully making the 
> > > > > > requests
> > > > > > identifiable from logs.
> > > > > 
> > > > > What I meant to say is that one PID could be allocating the
> > > > > request, while some other one could be actually doing Q/DQ_BUF.
> > > > > On such scenario, the debug string could provide mislead prints.
> > > > 
> > > > Um, yes, indeed it would no longer match the process. But the request is
> > > > still the same. That's actually a positive thing since it allows you to
> > > > identify the request.
> > > > 
> > > > With a global ID space this was trivial; you could just print the 
> > > > request
> > > > ID and that was all that was ever needed. (I'm not proposing to consider
> > > > that though.)
> > > >   
> > > 
> > > IMO, a global ID number would work better than get_task_comm().
> > > 
> > > Just add a static int monotonic counter and use it for the debug purposes,
> > > e. g.:
> > > 
> > > {
> > >   static unsigned int req_count = 0;
> > > 
> > >   snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
> > >   req_count++, fd);
> > > 
> > > Ok, eventually, it will overflow, but, it will be unique within
> > > a reasonable timeframe to be good enough for debugging purposes.  
> > 
> > Yes, but you can't figure out which process allocated it anymore, making
> > associating kernel debug logs with user space process logs harder.
> > 
> > How about process id + file handle? That still doesn't tell which process
> > operated on the request though, but I'm not sure whether that's really a
> > crucial piece of information.
> 
> You don't need that. With dev_dbg() - and other *_dbg() macros - you can
> enable process ID for all debug messages.

With this, the problem again is that it does not uniquely identify the
request: the request is the same request independently of which process
would operate on it. Or whether it is being processed in an interrupt
context.

AFAICT, the allocator PID (or process name) + file handle are both required
to match a request between user and kernel space logs.

> 
> Basically, if the user needs the PID, all it needs is to use "+pt",
> e. g. something like:
> 
>   echo "file 

Smatch and sparse errors

[Mauro Carvalho Chehab]

Hi Hans/Jasmin,

There is a regression with sparse and upstream Kernels, with also affect
smatch. Due to that, both will produce hundreds of new errors on all places
that directly or indirectly use min() or max().

Those were caused by those upstream patches:

3c8ba0d61d04 ("kernel.h: Retain constant expression output for 
max()/min()")
e9092d0d9796 ("Fix subtle macro variable shadowing in min_not_zero()")

There is already an upstream patch for hidding it:
https://patchwork.kernel.org/patch/10334353/

While upstream doesn't fix it (either by applying this patch or with some
other fixup patch), it should be applied on both sparse and smatch trees, 
in order to get rid of those false positives.

I suggest applying it to the logic with does daily compilations,
in order to avoid generating useless reports for smatch/sparse.

Thanks,
Mauro

Re: [RFCv11 PATCH 04/29] media-request: core request support

[Mauro Carvalho Chehab]

Em Wed, 11 Apr 2018 18:02:19 +0300
Sakari Ailus  escreveu:

> On Wed, Apr 11, 2018 at 10:49:35AM -0300, Mauro Carvalho Chehab wrote:
> > Em Wed, 11 Apr 2018 16:21:16 +0300
> > Sakari Ailus  escreveu:
> > 
> >   
> > > > > > Btw, this is a very good reason why you should define the ioctl to
> > > > > > have an integer argument instead of a struct with a __s32 field
> > > > > > on it, as per my comment to patch 02/29:
> > > > > > 
> > > > > > #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int)
> > > > > > 
> > > > > > At 64 bit architectures, you're truncating the file descriptor! 
> > > > > >  
> > > > > 
> > > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit systems 
> > > > > as
> > > > > well.
> > > > 
> > > > Hmm.. you're right. I was thinking that it could be 64 bits on some
> > > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but,
> > > > according with:
> > > > 
> > > > https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html
> > > > 
> > > > This is not the case on gcc.
> > > 
> > > Ok. The reasoning back then was that what "int" means varies across
> > > compilers and languages. And the intent was to codify this to __s32 which
> > > is what the kernel effectively uses.  
> > 
> > ...
> >   
> > > The rest of the kernel uses int rather liberally in the uAPI so I'm not
> > > sure in the end whether something desirable was achieved. Perhaps it'd be
> > > good to go back to the original discussion to find out for sure.
> > > 
> > > Still binaries compiled with Tru64 C compiler wouldn't work on Linux 
> > > anyway
> > > due to that difference.
> > > 
> > > Well, I stop here for this begins to be off-topic. :-)  
> > 
> > Yes. Let's keep it as s32 as originally proposed. Just ignore my comments
> > about that :-)
> >   
> > > > > > > + get_task_comm(comm, current);
> > > > > > > + snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d",
> > > > > > > +  comm, fd);  
> > > > > > 
> > > > > > Not sure if it is a good idea to store the task that allocated
> > > > > > the request. While it makes sense for the dev_dbg() below, it
> > > > > > may not make sense anymore on other dev_dbg() you would be
> > > > > > using it.  
> > > > > 
> > > > > The lifetime of the file handle roughly matches that of the request. 
> > > > > It's
> > > > > for debug only anyway.
> > > > > 
> > > > > Better proposals are always welcome of course. But I think we should 
> > > > > have
> > > > > something here that helps debugging by meaningfully making the 
> > > > > requests
> > > > > identifiable from logs.
> > > > 
> > > > What I meant to say is that one PID could be allocating the
> > > > request, while some other one could be actually doing Q/DQ_BUF.
> > > > On such scenario, the debug string could provide mislead prints.
> > > 
> > > Um, yes, indeed it would no longer match the process. But the request is
> > > still the same. That's actually a positive thing since it allows you to
> > > identify the request.
> > > 
> > > With a global ID space this was trivial; you could just print the request
> > > ID and that was all that was ever needed. (I'm not proposing to consider
> > > that though.)
> > >   
> > 
> > IMO, a global ID number would work better than get_task_comm().
> > 
> > Just add a static int monotonic counter and use it for the debug purposes,
> > e. g.:
> > 
> > {
> > static unsigned int req_count = 0;
> > 
> > snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
> > req_count++, fd);
> > 
> > Ok, eventually, it will overflow, but, it will be unique within
> > a reasonable timeframe to be good enough for debugging purposes.  
> 
> Yes, but you can't figure out which process allocated it anymore, making
> associating kernel debug logs with user space process logs harder.
> 
> How about process id + file handle? That still doesn't tell which process
> operated on the request though, but I'm not sure whether that's really a
> crucial piece of information.

You don't need that. With dev_dbg() - and other *_dbg() macros - you can
enable process ID for all debug messages.

Basically, if the user needs the PID, all it needs is to use "+pt",
e. g. something like:

echo "file drivers/media/* +pt" > 
/sys/kernel/debug/dynamic_debug/control


[1] see:

https://www.kernel.org/doc/html/v4.11/admin-guide/dynamic-debug-howto.html

Thanks,
Mauro

Re: [RFCv11 PATCH 04/29] media-request: core request support

[Sakari Ailus]

On Wed, Apr 11, 2018 at 10:49:35AM -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 11 Apr 2018 16:21:16 +0300
> Sakari Ailus  escreveu:
> 
> 
> > > > > Btw, this is a very good reason why you should define the ioctl to
> > > > > have an integer argument instead of a struct with a __s32 field
> > > > > on it, as per my comment to patch 02/29:
> > > > > 
> > > > >   #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int)
> > > > > 
> > > > > At 64 bit architectures, you're truncating the file descriptor!
> > > > 
> > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit systems as
> > > > well.  
> > > 
> > > Hmm.. you're right. I was thinking that it could be 64 bits on some
> > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but,
> > > according with:
> > > 
> > >   https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html
> > > 
> > > This is not the case on gcc.  
> > 
> > Ok. The reasoning back then was that what "int" means varies across
> > compilers and languages. And the intent was to codify this to __s32 which
> > is what the kernel effectively uses.
> 
> ...
> 
> > The rest of the kernel uses int rather liberally in the uAPI so I'm not
> > sure in the end whether something desirable was achieved. Perhaps it'd be
> > good to go back to the original discussion to find out for sure.
> > 
> > Still binaries compiled with Tru64 C compiler wouldn't work on Linux anyway
> > due to that difference.
> > 
> > Well, I stop here for this begins to be off-topic. :-)
> 
> Yes. Let's keep it as s32 as originally proposed. Just ignore my comments
> about that :-)
> 
> > > > > > +   get_task_comm(comm, current);
> > > > > > +   snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d",
> > > > > > +comm, fd);
> > > > > 
> > > > > Not sure if it is a good idea to store the task that allocated
> > > > > the request. While it makes sense for the dev_dbg() below, it
> > > > > may not make sense anymore on other dev_dbg() you would be
> > > > > using it.
> > > > 
> > > > The lifetime of the file handle roughly matches that of the request. 
> > > > It's
> > > > for debug only anyway.
> > > > 
> > > > Better proposals are always welcome of course. But I think we should 
> > > > have
> > > > something here that helps debugging by meaningfully making the requests
> > > > identifiable from logs.  
> > > 
> > > What I meant to say is that one PID could be allocating the
> > > request, while some other one could be actually doing Q/DQ_BUF.
> > > On such scenario, the debug string could provide mislead prints.  
> > 
> > Um, yes, indeed it would no longer match the process. But the request is
> > still the same. That's actually a positive thing since it allows you to
> > identify the request.
> > 
> > With a global ID space this was trivial; you could just print the request
> > ID and that was all that was ever needed. (I'm not proposing to consider
> > that though.)
> > 
> 
> IMO, a global ID number would work better than get_task_comm().
> 
> Just add a static int monotonic counter and use it for the debug purposes,
> e. g.:
> 
> {
>   static unsigned int req_count = 0;
> 
>   snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
>   req_count++, fd);
> 
> Ok, eventually, it will overflow, but, it will be unique within
> a reasonable timeframe to be good enough for debugging purposes.

Yes, but you can't figure out which process allocated it anymore, making
associating kernel debug logs with user space process logs harder.

How about process id + file handle? That still doesn't tell which process
operated on the request though, but I'm not sure whether that's really a
crucial piece of information.

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

Re: [RFCv11 PATCH 26/29] vim2m: use workqueue

[Paul Kocialkowski]

Hi,

On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in
> interrupt context. Switch to a workqueue instead.

See one comment below.

> Signed-off-by: Hans Verkuil 
> ---
>  drivers/media/platform/vim2m.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vim2m.c
> b/drivers/media/platform/vim2m.c
> index ef970434af13..9b18b32c255d 100644
> --- a/drivers/media/platform/vim2m.c
> +++ b/drivers/media/platform/vim2m.c
> @@ -150,6 +150,7 @@ struct vim2m_dev {
>   spinlock_t  irqlock;
>  
>   struct timer_list   timer;
> + struct work_struct  work_run;

Wouldn't it make more sense to move this to vim2m_ctx instead (since
this is heavily m2m-specific)?

>   struct v4l2_m2m_dev *m2m_dev;
>  };
> @@ -392,9 +393,10 @@ static void device_run(void *priv)
>   schedule_irq(dev, ctx->transtime);
>  }
>  
> -static void device_isr(struct timer_list *t)
> +static void device_work(struct work_struct *w)
>  {
> - struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t,
> timer);
> + struct vim2m_dev *vim2m_dev =
> + container_of(w, struct vim2m_dev, work_run);
>   struct vim2m_ctx *curr_ctx;
>   struct vb2_v4l2_buffer *src_vb, *dst_vb;
>   unsigned long flags;
> @@ -426,6 +428,13 @@ static void device_isr(struct timer_list *t)
>   }
>  }
>  
> +static void device_isr(struct timer_list *t)
> +{
> + struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t,
> timer);
> +
> + schedule_work(_dev->work_run);
> +}
> +
>  /*
>   * video ioctls
>   */
> @@ -806,6 +815,7 @@ static void vim2m_stop_streaming(struct vb2_queue
> *q)
>   struct vb2_v4l2_buffer *vbuf;
>   unsigned long flags;
>  
> + flush_scheduled_work();
>   for (;;) {
>   if (V4L2_TYPE_IS_OUTPUT(q->type))
>   vbuf = v4l2_m2m_src_buf_remove(ctx-
> >fh.m2m_ctx);
> @@ -1011,6 +1021,7 @@ static int vim2m_probe(struct platform_device
> *pdev)
>   vfd = >vfd;
>   vfd->lock = >dev_mutex;
>   vfd->v4l2_dev = >v4l2_dev;
> + INIT_WORK(>work_run, device_work);
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>   dev->mdev.dev = >dev;
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [RFCv11 PATCH 27/29] vim2m: support requests

[Paul Kocialkowski]

Hi,

On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Add support for requests to vim2m.

Depending on where STREAMON happens in the sequence, there is a
possibility that v4l2_m2m_try_schedule is never called and thus that
device_run never runs when submitting the request.

More specifically, the m2m fashion of the STREAMON ioctl handler will
normally call v4l2_m2m_try_schedule. Hence, there is no issue if it's
called after submitting the request. On the other hand, if the request
was not submitted when calling STREAMON (which is a valid use case),
buffers are not available and v4l2_m2m_try_schedule won't schedule
anything. Once the request is submitted, the internal plumbing will
detect that STREAMON was requested but did not take effect, so it will
call vb2_streamon. And of course, vb2_streamon has no provision for
trying to schedule a m2m device run.

One way to fix this is to call v4l2_m2m_try_schedule from a workqueue
triggered in the driver's start_streaming qop. The same is also probably
necessary in buf_queue and buf_prepare since those qops are not called
from the m2m ioctl handler either.

What do you think?

> Signed-off-by: Hans Verkuil 
> ---
>  drivers/media/platform/vim2m.c | 25 +
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/media/platform/vim2m.c
> b/drivers/media/platform/vim2m.c
> index 9b18b32c255d..2dcf0ea85705 100644
> --- a/drivers/media/platform/vim2m.c
> +++ b/drivers/media/platform/vim2m.c
> @@ -387,8 +387,26 @@ static void device_run(void *priv)
>   src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>   dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>  
> + /* Apply request if needed */
> + if (src_buf->vb2_buf.req_obj.req)
> + v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> + >hdl);
> + if (dst_buf->vb2_buf.req_obj.req &&
> + dst_buf->vb2_buf.req_obj.req != src_buf-
> >vb2_buf.req_obj.req)
> + v4l2_ctrl_request_setup(dst_buf->vb2_buf.req_obj.req,
> + >hdl);
> +
>   device_process(ctx, src_buf, dst_buf);
>  
> + /* Complete request if needed */
> + if (src_buf->vb2_buf.req_obj.req)
> + v4l2_ctrl_request_complete(src_buf-
> >vb2_buf.req_obj.req,
> + >hdl);
> + if (dst_buf->vb2_buf.req_obj.req &&
> + dst_buf->vb2_buf.req_obj.req != src_buf-
> >vb2_buf.req_obj.req)
> + v4l2_ctrl_request_complete(dst_buf-
> >vb2_buf.req_obj.req,
> + >hdl);
> +
>   /* Run a timer, which simulates a hardware irq  */
>   schedule_irq(dev, ctx->transtime);
>  }
> @@ -823,6 +841,8 @@ static void vim2m_stop_streaming(struct vb2_queue
> *q)
>   vbuf = v4l2_m2m_dst_buf_remove(ctx-
> >fh.m2m_ctx);
>   if (vbuf == NULL)
>   return;
> + v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req,
> +>hdl);
>   spin_lock_irqsave(>dev->irqlock, flags);
>   v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
>   spin_unlock_irqrestore(>dev->irqlock, flags);
> @@ -1003,6 +1023,10 @@ static const struct v4l2_m2m_ops m2m_ops = {
>   .job_abort  = job_abort,
>  };
>  
> +static const struct media_device_ops m2m_media_ops = {
> + .req_queue = vb2_request_queue,
> +};
> +
>  static int vim2m_probe(struct platform_device *pdev)
>  {
>   struct vim2m_dev *dev;
> @@ -1027,6 +1051,7 @@ static int vim2m_probe(struct platform_device
> *pdev)
>   dev->mdev.dev = >dev;
>   strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model));
>   media_device_init(>mdev);
> + dev->mdev.ops = _media_ops;
>   dev->v4l2_dev.mdev = >mdev;
>   dev->pad[0].flags = MEDIA_PAD_FL_SINK;
>   dev->pad[1].flags = MEDIA_PAD_FL_SOURCE;
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [RFCv11 PATCH 04/29] media-request: core request support

[Mauro Carvalho Chehab]

Em Wed, 11 Apr 2018 16:21:16 +0300
Sakari Ailus  escreveu:


> > > > Btw, this is a very good reason why you should define the ioctl to
> > > > have an integer argument instead of a struct with a __s32 field
> > > > on it, as per my comment to patch 02/29:
> > > > 
> > > > #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int)
> > > > 
> > > > At 64 bit architectures, you're truncating the file descriptor!
> > > 
> > > I'm not quite sure what do you mean. int is 32 bits on 64-bit systems as
> > > well.  
> > 
> > Hmm.. you're right. I was thinking that it could be 64 bits on some
> > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but,
> > according with:
> > 
> > https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html
> > 
> > This is not the case on gcc.  
> 
> Ok. The reasoning back then was that what "int" means varies across
> compilers and languages. And the intent was to codify this to __s32 which
> is what the kernel effectively uses.

...

> The rest of the kernel uses int rather liberally in the uAPI so I'm not
> sure in the end whether something desirable was achieved. Perhaps it'd be
> good to go back to the original discussion to find out for sure.
> 
> Still binaries compiled with Tru64 C compiler wouldn't work on Linux anyway
> due to that difference.
> 
> Well, I stop here for this begins to be off-topic. :-)

Yes. Let's keep it as s32 as originally proposed. Just ignore my comments
about that :-)

> > > > > + get_task_comm(comm, current);
> > > > > + snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d",
> > > > > +  comm, fd);
> > > > 
> > > > Not sure if it is a good idea to store the task that allocated
> > > > the request. While it makes sense for the dev_dbg() below, it
> > > > may not make sense anymore on other dev_dbg() you would be
> > > > using it.
> > > 
> > > The lifetime of the file handle roughly matches that of the request. It's
> > > for debug only anyway.
> > > 
> > > Better proposals are always welcome of course. But I think we should have
> > > something here that helps debugging by meaningfully making the requests
> > > identifiable from logs.  
> > 
> > What I meant to say is that one PID could be allocating the
> > request, while some other one could be actually doing Q/DQ_BUF.
> > On such scenario, the debug string could provide mislead prints.  
> 
> Um, yes, indeed it would no longer match the process. But the request is
> still the same. That's actually a positive thing since it allows you to
> identify the request.
> 
> With a global ID space this was trivial; you could just print the request
> ID and that was all that was ever needed. (I'm not proposing to consider
> that though.)
> 

IMO, a global ID number would work better than get_task_comm().

Just add a static int monotonic counter and use it for the debug purposes,
e. g.:

{
static unsigned int req_count = 0;

snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
req_count++, fd);

Ok, eventually, it will overflow, but, it will be unique within
a reasonable timeframe to be good enough for debugging purposes.


Thanks,
Mauro

[PATCH 5/5] drm/amdgpu: add independent DMA-buf import v3

[Christian König]

Instead of relying on the DRM functions just implement our own import
functions. This adds support for taking care of unpinned DMA-buf.

v2: enable for all exporters, not just amdgpu, fix invalidation
handling, lock reservation object while setting callback
v3: change to new dma_buf attach interface

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 72 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   | 32 +++---
 2 files changed, 89 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 7fef95f0fed1..58dcfba0225a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -86,28 +86,24 @@ int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, 
struct vm_area_struct *vma
return ret;
 }
 
-struct drm_gem_object *
-amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
-struct dma_buf_attachment *attach,
-struct sg_table *sg)
+static struct drm_gem_object *
+amdgpu_gem_prime_create_obj(struct drm_device *dev, struct dma_buf *dma_buf)
 {
-   struct reservation_object *resv = attach->dmabuf->resv;
+   struct reservation_object *resv = dma_buf->resv;
struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_bo *bo;
int ret;
 
ww_mutex_lock(>lock, NULL);
-   ret = amdgpu_bo_create(adev, attach->dmabuf->size, PAGE_SIZE,
+   ret = amdgpu_bo_create(adev, dma_buf->size, PAGE_SIZE,
   AMDGPU_GEM_DOMAIN_CPU, 0, ttm_bo_type_sg,
   resv, );
if (ret)
goto error;
 
-   bo->tbo.sg = sg;
-   bo->tbo.ttm->sg = sg;
bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
-   if (attach->dmabuf->ops != _dmabuf_ops)
+   if (dma_buf->ops != _dmabuf_ops)
bo->prime_shared_count = 1;
 
ww_mutex_unlock(>lock);
@@ -118,6 +114,26 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
return ERR_PTR(ret);
 }
 
+struct drm_gem_object *
+amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
+struct dma_buf_attachment *attach,
+struct sg_table *sg)
+{
+   struct drm_gem_object *obj;
+   struct amdgpu_bo *bo;
+
+   obj = amdgpu_gem_prime_create_obj(dev, attach->dmabuf);
+   if (IS_ERR(obj))
+   return obj;
+
+   bo = gem_to_amdgpu_bo(obj);
+   amdgpu_bo_reserve(bo, true);
+   bo->tbo.sg = sg;
+   bo->tbo.ttm->sg = sg;
+   amdgpu_bo_unreserve(bo);
+   return obj;
+}
+
 static struct sg_table *
 amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach,
   enum dma_data_direction dir)
@@ -293,9 +309,29 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device 
*dev,
return buf;
 }
 
+static void
+amdgpu_gem_prime_invalidate_mappings(struct dma_buf_attachment *attach)
+{
+   struct ttm_operation_ctx ctx = { false, false };
+   struct drm_gem_object *obj = attach->priv;
+   struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
+   struct ttm_placement placement = {};
+   int r;
+
+   r = ttm_bo_validate(>tbo, , );
+   if (r)
+   DRM_ERROR("Failed to unmap DMA-buf import (%d))\n", r);
+}
+
 struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
struct dma_buf *dma_buf)
 {
+   struct dma_buf_attach_info attach_info = {
+   .dev = dev->dev,
+   .dmabuf = dma_buf,
+   .invalidate = amdgpu_gem_prime_invalidate_mappings
+   };
+   struct dma_buf_attachment *attach;
struct drm_gem_object *obj;
 
if (dma_buf->ops == _dmabuf_ops) {
@@ -310,5 +346,21 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct 
drm_device *dev,
}
}
 
-   return drm_gem_prime_import(dev, dma_buf);
+   if (!dma_buf->invalidation_supported)
+   return drm_gem_prime_import(dev, dma_buf);
+
+   obj = amdgpu_gem_prime_create_obj(dev, dma_buf);
+   if (IS_ERR(obj))
+   return obj;
+
+   attach_info.importer_priv = obj;
+   attach = dma_buf_attach(_info);
+   if (IS_ERR(attach)) {
+   drm_gem_object_put(obj);
+   return ERR_CAST(attach);
+   }
+
+   get_dma_buf(dma_buf);
+   obj->import_attach = attach;
+   return obj;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index ab73300e6c7f..2a8f328918cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "amdgpu.h"
 #include 

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

[Christian König]

Move the parameters into a structure to make it simpler to extend it in
follow up patches.

This also adds the importer private as parameter so that we can directly
work with a completely filled in attachment structure.

Signed-off-by: Christian König 
---
 drivers/dma-buf/dma-buf.c | 15 ---
 drivers/gpu/drm/armada/armada_gem.c   |  6 +-
 drivers/gpu/drm/drm_prime.c   |  6 +-
 drivers/gpu/drm/i915/i915_gem_dmabuf.c|  6 +-
 drivers/gpu/drm/tegra/gem.c   |  6 +-
 drivers/gpu/drm/udl/udl_dmabuf.c  |  6 +-
 drivers/media/common/videobuf2/videobuf2-dma-contig.c |  6 +-
 drivers/media/common/videobuf2/videobuf2-dma-sg.c |  6 +-
 drivers/staging/media/tegra-vde/tegra-vde.c   |  6 +-
 include/linux/dma-buf.h   | 17 +++--
 10 files changed, 63 insertions(+), 17 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index d78d5fc173dc..4b46982c6d9c 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -534,8 +534,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
 /**
  * dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
  * calls attach() of dma_buf_ops to allow device-specific attach functionality
- * @dmabuf:[in]buffer to attach device to.
- * @dev:   [in]device to be attached.
+ * @info:  [in]holds all the attach related information provided
+ * by the importer. see  dma_buf_attach_info
+ * for further details.
  *
  * Returns struct dma_buf_attachment pointer for this attachment. Attachments
  * must be cleaned up by calling dma_buf_detach().
@@ -549,26 +550,26 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
  * accessible to @dev, and cannot be moved to a more suitable place. This is
  * indicated with the error code -EBUSY.
  */
-struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
- struct device *dev)
+struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info 
*info)
 {
+   struct dma_buf *dmabuf = info->dmabuf;
struct dma_buf_attachment *attach;
int ret;
 
-   if (WARN_ON(!dmabuf || !dev))
+   if (WARN_ON(!dmabuf || !info->dev))
return ERR_PTR(-EINVAL);
 
attach = kzalloc(sizeof(*attach), GFP_KERNEL);
if (!attach)
return ERR_PTR(-ENOMEM);
 
-   attach->dev = dev;
+   attach->dev = info->dev;
attach->dmabuf = dmabuf;
 
mutex_lock(>lock);
 
if (dmabuf->ops->attach) {
-   ret = dmabuf->ops->attach(dmabuf, dev, attach);
+   ret = dmabuf->ops->attach(dmabuf, info->dev, attach);
if (ret)
goto err_attach;
}
diff --git a/drivers/gpu/drm/armada/armada_gem.c 
b/drivers/gpu/drm/armada/armada_gem.c
index a97f509743a5..f4d1c11f57ea 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -514,6 +514,10 @@ armada_gem_prime_export(struct drm_device *dev, struct 
drm_gem_object *obj,
 struct drm_gem_object *
 armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
 {
+   struct dma_buf_attach_info attach_info = {
+   .dev = dev->dev,
+   .dmabuf = buf
+   };
struct dma_buf_attachment *attach;
struct armada_gem_object *dobj;
 
@@ -529,7 +533,7 @@ armada_gem_prime_import(struct drm_device *dev, struct 
dma_buf *buf)
}
}
 
-   attach = dma_buf_attach(buf, dev->dev);
+   attach = dma_buf_attach(_info);
if (IS_ERR(attach))
return ERR_CAST(attach);
 
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7856a9b3f8a8..4da242de51c2 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -707,6 +707,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct 
drm_device *dev,
struct dma_buf *dma_buf,
struct device *attach_dev)
 {
+   struct dma_buf_attach_info attach_info = {
+   .dev = attach_dev,
+   .dmabuf = dma_buf
+   };
struct dma_buf_attachment *attach;
struct sg_table *sgt;
struct drm_gem_object *obj;
@@ -727,7 +731,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct 
drm_device *dev,
if (!dev->driver->gem_prime_import_sg_table)
return ERR_PTR(-EINVAL);
 
-   attach = dma_buf_attach(dma_buf, attach_dev);
+   attach = dma_buf_attach(_info);
if (IS_ERR(attach))
return ERR_CAST(attach);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c 
b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 69a7aec49e84..7b737a883106 100644
--- 

[PATCH 3/5] drm/ttm: remove the backing store if no placement is given

[Christian König]

Pipeline removal of the BOs backing store when the placement is given
during validation.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_bo.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 98e06f8bf23b..17e821f01d0a 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1078,6 +1078,18 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
uint32_t new_flags;
 
reservation_object_assert_held(bo->resv);
+
+   /*
+* Remove the backing store if no placement is given.
+*/
+   if (!placement->num_placement && !placement->num_busy_placement) {
+   ret = ttm_bo_pipeline_gutting(bo);
+   if (ret)
+   return ret;
+
+   return ttm_tt_create(bo, false);
+   }
+
/*
 * Check whether we need to move buffer.
 */
-- 
2.14.1

[PATCH 4/5] drm/amdgpu: add independent DMA-buf export v2

[Christian König]

Instead of relying on the DRM functions just implement our own export
functions. This adds support for taking care of unpinned DMA-buf.

v2: fix unintended recursion, remove debugging leftovers

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h|   1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c|   1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |   5 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c  | 108 ++---
 4 files changed, 72 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 2babfad1fd7f..3e1727251edc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -371,7 +371,6 @@ int amdgpu_gem_object_open(struct drm_gem_object *obj,
 void amdgpu_gem_object_close(struct drm_gem_object *obj,
struct drm_file *file_priv);
 unsigned long amdgpu_gem_timeout(uint64_t timeout_ns);
-struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj);
 struct drm_gem_object *
 amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 struct dma_buf_attachment *attach,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 0b19482b36b8..6107b845d8a8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -890,7 +890,6 @@ static struct drm_driver kms_driver = {
.gem_prime_export = amdgpu_gem_prime_export,
.gem_prime_import = amdgpu_gem_prime_import,
.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
-   .gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table,
.gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table,
.gem_prime_vmap = amdgpu_gem_prime_vmap,
.gem_prime_vunmap = amdgpu_gem_prime_vunmap,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 9e23d6f6f3f3..15bb48cebbc4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -31,6 +31,7 @@
  */
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -953,6 +954,10 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
 
amdgpu_bo_kunmap(abo);
 
+   if (abo->gem_base.dma_buf && !abo->gem_base.import_attach &&
+   bo->mem.mem_type != TTM_PL_SYSTEM)
+   dma_buf_invalidate_mappings(abo->gem_base.dma_buf);
+
/* remember the eviction */
if (evict)
atomic64_inc(>num_evictions);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 4b584cb75bf4..7fef95f0fed1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -32,14 +32,6 @@
 
 static const struct dma_buf_ops amdgpu_dmabuf_ops;
 
-struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj)
-{
-   struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
-   int npages = bo->tbo.num_pages;
-
-   return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
-}
-
 void *amdgpu_gem_prime_vmap(struct drm_gem_object *obj)
 {
struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
@@ -126,23 +118,22 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
return ERR_PTR(ret);
 }
 
-static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
-struct device *target_dev,
-struct dma_buf_attachment *attach)
+static struct sg_table *
+amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach,
+  enum dma_data_direction dir)
 {
+   struct dma_buf *dma_buf = attach->dmabuf;
struct drm_gem_object *obj = dma_buf->priv;
struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
+   struct sg_table *sgt;
long r;
 
-   r = drm_gem_map_attach(dma_buf, target_dev, attach);
-   if (r)
-   return r;
-
-   r = amdgpu_bo_reserve(bo, false);
-   if (unlikely(r != 0))
-   goto error_detach;
-
+   if (!attach->invalidate) {
+   r = amdgpu_bo_reserve(bo, false);
+   if (unlikely(r != 0))
+   return ERR_PTR(r);
+   }
 
if (attach->dev->driver != adev->dev->driver) {
/*
@@ -158,42 +149,77 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
}
}
 
-   /* pin buffer into GTT */
-   r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL);
-   if (r)
+   if (!attach->invalidate) {
+   /* pin buffer into GTT */
+   r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL);
+   if (r)
+   goto error_unreserve;
+
+   } else {
+   /* move buffer into GTT */
+ 

[PATCH 2/5] dma-buf: add optional invalidate_mappings callback v3

[Christian König]

Each importer can now provide an invalidate_mappings callback.

This allows the exporter to provide the mappings without the need to pin
the backing store.

v2: don't try to invalidate mappings when the callback is NULL,
lock the reservation obj while using the attachments,
add helper to set the callback
v3: move flag for invalidation support into the DMA-buf,
use new attach_info structure to set the callback
v4: use importer_priv field instead of mangling exporter priv.

Signed-off-by: Christian König 
---
 drivers/dma-buf/dma-buf.c | 44 
 include/linux/dma-buf.h   | 36 ++--
 2 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 4b46982c6d9c..ffdaab10e2f2 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -565,6 +565,8 @@ struct dma_buf_attachment *dma_buf_attach(const struct 
dma_buf_attach_info *info
 
attach->dev = info->dev;
attach->dmabuf = dmabuf;
+   attach->importer_priv = info->importer_priv;
+   attach->invalidate = info->invalidate;
 
mutex_lock(>lock);
 
@@ -573,7 +575,9 @@ struct dma_buf_attachment *dma_buf_attach(const struct 
dma_buf_attach_info *info
if (ret)
goto err_attach;
}
+   reservation_object_lock(dmabuf->resv, NULL);
list_add(>node, >attachments);
+   reservation_object_unlock(dmabuf->resv);
 
mutex_unlock(>lock);
return attach;
@@ -599,7 +603,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct 
dma_buf_attachment *attach)
return;
 
mutex_lock(>lock);
+   reservation_object_lock(dmabuf->resv, NULL);
list_del(>node);
+   reservation_object_unlock(dmabuf->resv);
if (dmabuf->ops->detach)
dmabuf->ops->detach(dmabuf, attach);
 
@@ -633,10 +639,23 @@ struct sg_table *dma_buf_map_attachment(struct 
dma_buf_attachment *attach,
if (WARN_ON(!attach || !attach->dmabuf))
return ERR_PTR(-EINVAL);
 
+   /*
+* Mapping a DMA-buf can trigger its invalidation, prevent sending this
+* event to the caller by temporary removing this attachment from the
+* list.
+*/
+   if (attach->invalidate) {
+   reservation_object_assert_held(attach->dmabuf->resv);
+   list_del(>node);
+   }
+
sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
if (!sg_table)
sg_table = ERR_PTR(-ENOMEM);
 
+   if (attach->invalidate)
+   list_add(>node, >dmabuf->attachments);
+
return sg_table;
 }
 EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
@@ -657,6 +676,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment 
*attach,
 {
might_sleep();
 
+   if (attach->invalidate)
+   reservation_object_assert_held(attach->dmabuf->resv);
+
if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
return;
 
@@ -665,6 +687,26 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment 
*attach,
 }
 EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
 
+/**
+ * dma_buf_invalidate_mappings - invalidate all mappings of this dma_buf
+ *
+ * @dmabuf:[in]buffer which mappings should be invalidated
+ *
+ * Informs all attachmenst that they need to destroy and recreated all their
+ * mappings.
+ */
+void dma_buf_invalidate_mappings(struct dma_buf *dmabuf)
+{
+   struct dma_buf_attachment *attach;
+
+   reservation_object_assert_held(dmabuf->resv);
+
+   list_for_each_entry(attach, >attachments, node)
+   if (attach->invalidate)
+   attach->invalidate(attach);
+}
+EXPORT_SYMBOL_GPL(dma_buf_invalidate_mappings);
+
 /**
  * DOC: cpu access
  *
@@ -1122,10 +1164,12 @@ static int dma_buf_debug_show(struct seq_file *s, void 
*unused)
seq_puts(s, "\tAttached Devices:\n");
attach_count = 0;
 
+   reservation_object_lock(buf_obj->resv, NULL);
list_for_each_entry(attach_obj, _obj->attachments, node) {
seq_printf(s, "\t%s\n", dev_name(attach_obj->dev));
attach_count++;
}
+   reservation_object_unlock(buf_obj->resv);
 
seq_printf(s, "Total %d devices attached\n\n",
attach_count);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 414b4dde5eb7..566503dd2b4f 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -270,6 +270,8 @@ struct dma_buf_ops {
  * @poll: for userspace poll support
  * @cb_excl: for userspace poll support
  * @cb_shared: for userspace poll support
+ * @invalidation_supported: True when the exporter supports unpinned operation
+ *  using the reservation lock.
  *
  * This represents 

Re: [RFCv11 PATCH 04/29] media-request: core request support

[Sakari Ailus]

Hi Mauro,

On Tue, Apr 10, 2018 at 11:51:43AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 10 Apr 2018 15:32:34 +0300
> Sakari Ailus  escreveu:
> 
> > Hi Mauro and Hans,
> > 
> > On Tue, Apr 10, 2018 at 07:32:06AM -0300, Mauro Carvalho Chehab wrote:
> > ...
> > > > +static void media_request_release(struct kref *kref)
> > > > +{
> > > > +   struct media_request *req =
> > > > +   container_of(kref, struct media_request, kref);
> > > > +   struct media_device *mdev = req->mdev;
> > > > +   unsigned long flags;
> > > > +
> > > > +   dev_dbg(mdev->dev, "request: release %s\n", req->debug_str);
> > > > +
> > > > +   spin_lock_irqsave(>lock, flags);
> > > > +   req->state = MEDIA_REQUEST_STATE_CLEANING;
> > > > +   spin_unlock_irqrestore(>lock, flags);
> > > > +
> > > > +   media_request_clean(req);
> > > > +
> > > > +   if (mdev->ops->req_free)
> > > > +   mdev->ops->req_free(req);
> > > > +   else
> > > > +   kfree(req);  
> > > 
> > > Without looking at req_free() implementation, I would actually prefer
> > > to always do a kfree(req) here. So, a req_free() function would only
> > > free "extra" allocations, and not the request itself. e. g.:
> > > 
> > > ...
> > >   if (mdev->ops->req_free)
> > >   mdev->ops->req_free(req);
> > > 
> > >   kfree(req);
> > > }  
> > 
> > The idea is that you can embed the request object in a driver specific
> > struct. The drivers can store information related to that request rather
> > easily that way, without requiring to be aware of two objects with
> > references pointing to each other. I rather prefer the current
> > implementation. It same pattern is actually used on videobuf2 buffers.
> 
> Ok, then document it so.
> 
> Btw, one of the things it is missing is a kAPI documentation patch,
> describing things like that.

I agree. Documentation will need to be added and improved.

> 
> > 
> > ...
> > 
> > > > +static unsigned int media_request_poll(struct file *filp,
> > > > +  struct poll_table_struct *wait)
> > > > +{
> > > > +   struct media_request *req = filp->private_data;
> > > > +   unsigned long flags;
> > > > +   enum media_request_state state;
> > > > +
> > > > +   if (!(poll_requested_events(wait) & POLLPRI))
> > > > +   return 0;
> > > > +
> > > > +   spin_lock_irqsave(>lock, flags);
> > > > +   state = req->state;
> > > > +   spin_unlock_irqrestore(>lock, flags);  
> > > 
> > > IMO, it would be better to use an atomic var for state, having a
> > > lockless access to it.  
> > 
> > The lock serialises access to the whole request, not just to its state.
> 
> From what I understood at the code is that the spin lock
> is used *sometimes* to protect just the state. I didn't
> see it used to protect the hole data struct.
> 
> Instead, the mutex is used for that purpose, but, again,
> it is *sometimes* used, but on several parts, neither the
> spin lock nor the mutex is used.
> 
> It should be noticed that the data, itself, should be protected
> by *either* mutex or spin lock.
> 
> > While it doesn't matter here as you're just reading the state, writing it
> > would still require taking the lock. Using atomic_t might suggest
> > otherwise, and could end up being a source of bugs.
> 
> There are already a lot of bugs at the locking, from what I noticed.
> 
> We should do it right: it should use *just one* kind of memory
> protection for struct media_request. On the places where it is
> safe to just read the status without locking, atomic_t() should
> be used. Where it doesn't, probably the entire code should be
> protected by the lock.

If done that way, it needs to be documented because it isn't obvious.
Keeping the code simple helps a lot; I don't see the core code growing a
lot so this looks good.

> 
> > >   
> > > > +
> > > > +   if (state == MEDIA_REQUEST_STATE_COMPLETE)
> > > > +   return POLLPRI;
> > > > +   if (state == MEDIA_REQUEST_STATE_IDLE)
> > > > +   return POLLERR;
> > > > +
> > > > +   poll_wait(filp, >poll_wait, wait);
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static long media_request_ioctl(struct file *filp, unsigned int cmd,
> > > > +   unsigned long __arg)
> > > > +{
> > > > +   return -ENOIOCTLCMD;
> > > > +}
> > > > +
> > > > +static const struct file_operations request_fops = {
> > > > +   .owner = THIS_MODULE,
> > > > +   .poll = media_request_poll,
> > > > +   .unlocked_ioctl = media_request_ioctl,
> > > > +   .release = media_request_close,
> > > > +};
> > > > +
> > > >  int media_request_alloc(struct media_device *mdev,
> > > > struct media_request_alloc *alloc)
> > > >  {
> > > > -   return -ENOMEM;
> > > > +   struct media_request *req;
> > > > +   struct file *filp;
> > > > +   char comm[TASK_COMM_LEN];
> > > > +   

Re: [PATCH v7 2/2] uvcvideo: handle control pipe protocol STALLs

[Guennadi Liakhovetski]

Hi Laurent,

First just answers to your questions:

On Sat, 7 Apr 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> On Friday, 23 March 2018 11:24:01 EEST Laurent Pinchart wrote:
> > From: Guennadi Liakhovetski 
> > 
> > When a command ends up in a STALL on the control pipe, use the Request
> > Error Code control to provide a more precise error information to the
> > user.
> 
> This is the kind of change that I believe is completely right, but that 
> nonetheless worries me. All the years I've spent working with UVC webcams 
> taught me that lots of cameras have buggy firmware, and that any change in 
> how 
> the host driver issues requests can have dire consequences for users. This is 
> especially true when the host driver issues a request that was never issued 
> before.
> 
> The UVC specification also doesn't clearly tell whether the request error 
> code 
> control is mandatory for a device to implement. My interpretation of the 
> specification is that it is, but it would have been nice for the 
> specification 
> to be explicit on this topic. Have you encountered devices that don't 
> implement this control ?

No, I haven't. But I haven't explicitly tested too many either. This patch 
would only issue that control if a STALL condition is detected, and 
normally that doesn't happen.

> This being said, I don't claim that would be a reason not to use the request 
> error code control as proposed by this patch, but I would feel less worried 
> if 
> I knew whether the Windows driver used that control as well. If it does 
> there's a high chance that cameras will implement it correctly, while if it 
> doesn't we will most certainly hit bugs with several cameras. I was thus 
> wondering if in the course of your UVC developments you would have happened 
> to 
> find out whether this control is used by Windows.

No, sorry, I never tried to analyse the behaviour of the Windows UVC 
driver.

> I would also like to know a bit more about the purpose of this patch. Logging 
> the error code is certainly useful for diagnosis purpose. Have you also found 
> it useful to report different errors back to userspace ? If so, could you 
> explain your use cases ?

Yes, with this patch the user-space can with certainty identify the reason 
of a stall, specifically you would know, when the camera is in a "not 
ready" state. With the previous patch in this series the driver shouldn't 
be sending a second SET_CUR command to the same control, before the first 
one has completed, but on some cameras different controls can also be 
interrelated. In such a case trying to set a different control, while a 
previous one is still being processed, can also cause a STALL with a "Not 
ready" error state.

> > Signed-off-by: Guennadi Liakhovetski 
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 59 
> >  1 file changed, 53 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index aa0082fe5833..eb9e04a59427 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -34,15 +34,59 @@ static int __uvc_query_ctrl(struct uvc_device *dev, u8
> > query, u8 unit, u8 intfnum, u8 cs, void *data, u16 size,
> > int timeout)
> >  {
> > -   u8 type = USB_TYPE_CLASS | USB_RECIP_INTERFACE;
> > +   u8 type = USB_TYPE_CLASS | USB_RECIP_INTERFACE, tmp, error;
> 
> Would you mind declaring one variable per line to match the style of the rest 
> of the driver ?

In fact I would, but well... ;-)

> > unsigned int pipe;
> > +   int ret;
> > 
> > pipe = (query & 0x80) ? usb_rcvctrlpipe(dev->udev, 0)
> >   : usb_sndctrlpipe(dev->udev, 0);
> > 
> > type |= (query & 0x80) ? USB_DIR_IN : USB_DIR_OUT;
> > 
> > -   return usb_control_msg(dev->udev, pipe, query, type, cs << 8,
> > +   ret = usb_control_msg(dev->udev, pipe, query, type, cs << 8,
> > unit << 8 | intfnum, data, size, timeout);
> > +
> 
> Nitpicking, you can remove the blank line here.
> 
> > +   if (ret != -EPIPE)
> > +   return ret;
> > +
> > +   tmp = *(u8 *)data;
> > +
> > +   pipe = usb_rcvctrlpipe(dev->udev, 0);
> > +   type = USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN;
> > +   ret = usb_control_msg(dev->udev, pipe, UVC_GET_CUR, type,
> > + UVC_VC_REQUEST_ERROR_CODE_CONTROL << 8,
> > + unit << 8 | intfnum, data, 1, timeout);
> 
> Unless I'm mistaken the wIndex value should be "Zero and Interface" according 
> to both the UVC 1.1 and UVC 1.5 specifications. This should thus be
> 
>   ret = usb_control_msg(dev->udev, pipe, UVC_GET_CUR, type,
> UVC_VC_REQUEST_ERROR_CODE_CONTROL << 8,
> intfnum, data, 1, timeout);

Hm, the 1.5 spec says:


This read-only control 

[PATCH] media: ddbridge: better handle optional spin locks at the code

[Mauro Carvalho Chehab]

Currently, ddbridge produces 4 warnings on sparse:
drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context 
imbalance in 'ddb_output_start' - different lock contexts for basic block
drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context 
imbalance in 'ddb_output_stop' - different lock contexts for basic block
drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context 
imbalance in 'ddb_input_stop' - different lock contexts for basic block
drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context 
imbalance in 'ddb_input_start' - different lock contexts for basic block

Those are all false positives, but they result from the fact that
there could potentially have some troubles at the locking schema,
because the lock depends on a var (output->dma).

I discussed that in priv with Sparse author and with the current
maintainer. Both believe that sparse is doing the right thing, and
that the proper fix would be to change the code to make it clearer
that, when spin_lock_irq() is called, spin_unlock_irq() will be
also called.

That help not only static analyzers to better understand the code,
but also humans that could need to take a look at the code.

It was also pointed that gcc would likely be smart enough to
optimize the code and produce the same result. I double
checked: indeed, the size of the driver didn't change after
this patch.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/pci/ddbridge/ddbridge-core.c | 43 --
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c 
b/drivers/media/pci/ddbridge/ddbridge-core.c
index 4a2819d3e225..080e2189ca7f 100644
--- a/drivers/media/pci/ddbridge/ddbridge-core.c
+++ b/drivers/media/pci/ddbridge/ddbridge-core.c
@@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, u32 *con, 
u32 *con2, u32 flags)
*con2 = (nco << 16) | gap;
 }
 
-static void ddb_output_start(struct ddb_output *output)
+static void __ddb_output_start(struct ddb_output *output)
 {
struct ddb *dev = output->port->dev;
u32 con = 0x11c, con2 = 0;
 
if (output->dma) {
-   spin_lock_irq(>dma->lock);
output->dma->cbuf = 0;
output->dma->coff = 0;
output->dma->stat = 0;
@@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output *output)
 
ddbwritel(dev, con | 1, TS_CONTROL(output));
 
-   if (output->dma) {
+   if (output->dma)
output->dma->running = 1;
+}
+
+static void ddb_output_start(struct ddb_output *output)
+{
+   if (output->dma) {
+   spin_lock_irq(>dma->lock);
+   __ddb_output_start(output);
spin_unlock_irq(>dma->lock);
+   } else {
+   __ddb_output_start(output);
}
 }
 
@@ -502,15 +510,13 @@ static void ddb_output_stop(struct ddb_output *output)
 {
struct ddb *dev = output->port->dev;
 
-   if (output->dma)
-   spin_lock_irq(>dma->lock);
-
-   ddbwritel(dev, 0, TS_CONTROL(output));
-
if (output->dma) {
+   spin_lock_irq(>dma->lock);
ddbwritel(dev, 0, DMA_BUFFER_CONTROL(output->dma));
output->dma->running = 0;
spin_unlock_irq(>dma->lock);
+   } else {
+   ddbwritel(dev, 0, TS_CONTROL(output));
}
 }
 
@@ -519,22 +525,21 @@ static void ddb_input_stop(struct ddb_input *input)
struct ddb *dev = input->port->dev;
u32 tag = DDB_LINK_TAG(input->port->lnr);
 
-   if (input->dma)
-   spin_lock_irq(>dma->lock);
-   ddbwritel(dev, 0, tag | TS_CONTROL(input));
if (input->dma) {
+   spin_lock_irq(>dma->lock);
ddbwritel(dev, 0, DMA_BUFFER_CONTROL(input->dma));
input->dma->running = 0;
spin_unlock_irq(>dma->lock);
+   } else {
+   ddbwritel(dev, 0, tag | TS_CONTROL(input));
}
 }
 
-static void ddb_input_start(struct ddb_input *input)
+static void __ddb_input_start(struct ddb_input *input)
 {
struct ddb *dev = input->port->dev;
 
if (input->dma) {
-   spin_lock_irq(>dma->lock);
input->dma->cbuf = 0;
input->dma->coff = 0;
input->dma->stat = 0;
@@ -557,9 +562,19 @@ static void ddb_input_start(struct ddb_input *input)
if (input->port->type == DDB_TUNER_DUMMY)
ddbwritel(dev, 0x000fff01, TS_CONTROL2(input));
 
-   if (input->dma) {
+   if (input->dma)
input->dma->running = 1;
+}
+
+static void ddb_input_start(struct ddb_input *input)
+{
+
+   if (input->dma) {
+   spin_lock_irq(>dma->lock);
+   __ddb_input_start(input);
spin_unlock_irq(>dma->lock);
+   } else {
+   __ddb_input_start(input);
}
 

Re: sensor driver - v4l2 - MEDIA_BUS_FMT

[Sakari Ailus]

On Wed, Apr 11, 2018 at 03:33:50PM +0530, asadpt iqroot wrote:
> Hi All,
> 
> We are trying develop a sensor driver code for hdmi2csi adapter.
> Reguired data format is RGB888. But in media format header file, we
> could see three macros related to RGB888. Hardware connection is mipi
> csi2.
> 
> #define MEDIA_BUS_FMT_RGB888_1X24 0x100a
> #define MEDIA_BUS_FMT_RGB888_2X12_BE 0x100b
> #define MEDIA_BUS_FMT_RGB888_2X12_LE 0x100c
> 
> How to decide whether we go for RGB888_1X24 or RGB888_2X12 macros.

Originally when support was added for serial busses, we did not create new
formats for these busses but instead re-used the ones intended for parallel
busses. Please use the single-sample variant of the format.

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

Re: [RFCv11 PATCH 03/29] media-request: allocate media requests

[Mauro Carvalho Chehab]

Em Tue, 10 Apr 2018 14:14:30 +0300
Sakari Ailus  escreveu:

> > >  /**
> > > @@ -88,6 +96,8 @@ struct media_device_ops {
> > >   * @disable_source: Disable Source Handler function pointer
> > >   *
> > >   * @ops: Operation handler callbacks
> > > + * @req_lock:Serialise access to requests
> > > + * @req_queue_mutex: Serialise validating and queueing requests  
> > 
> > IMHO, this would better describe it:
> > Serialise validate and queue requests
> > 
> > Yet, IMO, it doesn't let it clear when the spin lock should be
> > used and when the mutex should be used.
> > 
> > I mean, what of them protect what variable?  
> 
> It might not be obvious, but the purpose of this mutex is to prevent
> queueing multiple requests simultaneously in order to serialise access to
> the top of the queue.

It is not obvious at all. The purpose of a lock is to prevent
multiple acesses to the content of a data structure.

>From what I see, here we have a single structure with two locks.
To make it worse, you're introducing first the lock and then,
on patch 04/29, the actual structs to be protected.

Well, a house with two doors is only closed if both doors are
closed. The same happens with a data structure protected by
two locks. It is only locked if both locks are locked.

So, every time I see two locks meant to protect the same struct, it
sounds poorly designed or wrong. 

There's one exception, though: if you have an independent
guest house and a main house, you could have two locks at the
same place, one for the main house and another one for the
guest house, but in this case, you should clearly tag with lock
protects each house.

So, in this case, two new locks that are being proposed to be added
at for struct media_device. So, I would be expecting a comment
like:

@req_lock: serialize access to  media_request 
@req_queue_mutex: serialize access to  media_request_object

Clearly stating what data structures each lock protects.

That was my concern when I pointed it. After looking at the entire
patchset, what I saw was a non-consistent locking model.

It sounded to me that the original design to be:

1) req_queue_mutex was designed to protect struct media_request
   instances;

2) req_lock was designed to protect just one field inside
   struct media_request (the state field).

There is a big issue on that:

As state is part of media_request, assuming that the data struct
won't disappear while in use (with is warranted by kref), before
changing its value and touching other fields at req, the code should 
be locking both req_queue_mutex and req_lock, but I didn't see that
behavior on several places.

Also, I noticed several locking inconsistencies, as, on several places,
the content of a media_request instance and/or its state was 
accessed/altered without either locks.

> How about this instead:
> 
>   Serialise access to accessing device state on the tail of the
>   request queue.

It still doesn't mention what struct each of the new locks protect.
IMHO, this patch should either be bound with patch 04/29 or come after
that, and explicitly mention what data is protected by each lock.

Thanks,
Mauro

sensor driver - v4l2 - MEDIA_BUS_FMT

[asadpt iqroot]

Hi All,

We are trying develop a sensor driver code for hdmi2csi adapter.
Reguired data format is RGB888. But in media format header file, we
could see three macros related to RGB888. Hardware connection is mipi
csi2.

#define MEDIA_BUS_FMT_RGB888_1X24 0x100a
#define MEDIA_BUS_FMT_RGB888_2X12_BE 0x100b
#define MEDIA_BUS_FMT_RGB888_2X12_LE 0x100c

How to decide whether we go for RGB888_1X24 or RGB888_2X12 macros.


Thanks & Regards
- Asad

a 4.16 kernel with Debian 9.4 "stretch" causes a log explosion

[Guennadi Liakhovetski]

Hi Laurent,

Not sure whether that's a kernel or a user-space problem, but UVC related 
anyway. I've got a UVC 1.5 (!) Logitech camera, that used to work fine 
with earlier kernels. I now installed "media 4.16" and saw, that the 
kernel log was filling with messages like

uvcvideo: Failed to query (GET_MIN) UVC control 2 on unit 1: -32 (exp. 1).

The expected /dev/video[01] nodes were not created correctly, and the 
hard-drive was getting full very quickly. The latter was happening because 
the the /var/log/uvcdynctrl-udev.log file was growing. A truncated sample 
is attached. At its bottom you see messages

[libwebcam] Warning: The driver behind device video0 has a slightly buggy 
implementation
  of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher
  control ID if a control query fails. A workaround has been enabled.

repeating, which continues even if the camera is unplugged. The kernel is 
the head of the master branch of git://linuxtv.org/media_tree.git

Just figured out this commit

From: Edgar Thier 
Date: Thu, 12 Oct 2017 03:54:17 -0400
Subject: [PATCH] media: uvcvideo: Apply flags from device to actual properties

as the culprit. Without it everything is back to normal.

Thanks
Guennadi


==
uvcdynctrl script version 0.3 running from '/lib/udev/uvcdynctrl'
==
uvcdynctrl script version 0.3 running from '/lib/udev/uvcdynctrl'
Triggered at Wed Apr 11 09:08:26 CEST 2018

ACTION='add'
DEVLINKS='/dev/v4l/by-path/pci-:00:14.0-usb-0:1:1.0-video-index0 
/dev/v4l/by-id/usb-046d_Logitech_Webcam_C930e_816415EE-video-index0'
DEVNAME='/dev/video0'
DEVPATH='/devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/video4linux/video0'
ID_BUS='usb'
ID_FOR_SEAT='video4linux-pci-_00_14_0-usb-0_1_1_0'
ID_MODEL='Logitech_Webcam_C930e'
ID_MODEL_ENC='Logitech\x20Webcam\x20C930e'
ID_MODEL_ID='0843'
ID_PATH='pci-:00:14.0-usb-0:1:1.0'
ID_PATH_TAG='pci-_00_14_0-usb-0_1_1_0'
ID_REVISION='0013'
ID_SERIAL='046d_Logitech_Webcam_C930e_816415EE'
ID_SERIAL_SHORT='816415EE'
ID_TYPE='video'
ID_USB_DRIVER='uvcvideo'
ID_USB_INTERFACES=':0e0100:0e0200:010100:010200:'
ID_USB_INTERFACE_NUM='00'
ID_V4L_CAPABILITIES=':capture:'
ID_V4L_PRODUCT='Logitech Webcam C930e'
ID_V4L_VERSION='2'
ID_VENDOR='046d'
ID_VENDOR_ENC='046d'
ID_VENDOR_ID='046d'
IFS='   
'
MAJOR='81'
MINOR='0'
OPTIND='1'
PATH='/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'
PPID='853'
PS1='# '
PS2='> '
PS4='+ '
PWD='/'
SEQNUM='1895'
SUBSYSTEM='video4linux'
TAGS=':uaccess:seat:'
USEC_INITIALIZED='175470181'
debug='1'
logfile='/var/log/uvcdynctrl-udev.log'
uvcdynctrlpath='uvcdynctrl'
version='0.3'

Triggered at Wed Apr 11 09:08:26 CEST 2018
VID of new device: '046d'
PID of new device: '0843'

Executing command: 'uvcdynctrl -d /dev/video0 --addctrl=046d:0843'
ACTION='add'
DEVLINKS='/dev/v4l/by-id/usb-046d_Logitech_Webcam_C930e_816415EE-video-index1 
/dev/v4l/by-path/pci-:00:14.0-usb-0:1:1.0-video-index1'
DEVNAME='/dev/video1'
DEVPATH='/devices/pci:00/:00:14.0/usb2/2-1/2-1:1.0/video4linux/video1'
ID_BUS='usb'
ID_FOR_SEAT='video4linux-pci-_00_14_0-usb-0_1_1_0'
ID_MODEL='Logitech_Webcam_C930e'
ID_MODEL_ENC='Logitech\x20Webcam\x20C930e'
ID_MODEL_ID='0843'
ID_PATH='pci-:00:14.0-usb-0:1:1.0'
ID_PATH_TAG='pci-_00_14_0-usb-0_1_1_0'
ID_REVISION='0013'
ID_SERIAL='046d_Logitech_Webcam_C930e_816415EE'
ID_SERIAL_SHORT='816415EE'
ID_TYPE='video'
ID_USB_DRIVER='uvcvideo'
ID_USB_INTERFACES=':0e0100:0e0200:010100:010200:'
ID_USB_INTERFACE_NUM='00'
ID_V4L_CAPABILITIES=':capture:'
ID_V4L_PRODUCT='Logitech Webcam C930e'
ID_V4L_VERSION='2'
ID_VENDOR='046d'
ID_VENDOR_ENC='046d'
ID_VENDOR_ID='046d'
IFS='   
'
MAJOR='81'
MINOR='1'
OPTIND='1'
PATH='/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'
PPID='858'
PS1='# '
PS2='> '
PS4='+ '
PWD='/'
SEQNUM='1896'
SUBSYSTEM='video4linux'
TAGS=':uaccess:seat:'
USEC_INITIALIZED='175470183'
debug='1'
logfile='/var/log/uvcdynctrl-udev.log'
uvcdynctrlpath='uvcdynctrl'
version='0.3'

VID of new device: '046d'
PID of new device: '0843'
Executing command: 'uvcdynctrl -d /dev/video1 --addctrl=046d:0843'
[libwebcam] Warning: The driver behind device video0 has a slightly buggy 
implementation
  of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher
  control ID if a control query fails. A workaround has been enabled.
[libwebcam] Warning: The driver behind device video0 has a slightly buggy 
implementation
  of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher
  control ID if a control query fails. A workaround has been enabled.
[libwebcam] Warning: The driver behind device video0 has a slightly buggy 
implementation
  of the V4L2_CTRL_FLAG_NEXT_CTRL flag. It does not return the next higher
  control ID if a control query fails. A workaround has been enabled.
[libwebcam] Warning: The 

Re: [RFCv11 PATCH 14/29] v4l2-ctrls: add core request support

[Tomasz Figa]

Hi Hans,

On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil  wrote:

> From: Hans Verkuil 

> Integrate the request support. This adds the v4l2_ctrl_request_complete
> and v4l2_ctrl_request_setup functions to complete a request and (as a
> helper function) to apply a request to the hardware.

Please see my comments inline.

[snip]
> +static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
> +  const struct v4l2_ctrl_handler *from)
> +{
> +   struct v4l2_ctrl_ref *ref;
> +   int err;
> +
> +   if (WARN_ON(!hdl || hdl == from))
> +   return -EINVAL;
> +
> +   if (hdl->error)
> +   return hdl->error;
> +
> +   WARN_ON(hdl->lock != >_lock);
> +
> +   mutex_lock(from->lock);
> +   list_for_each_entry(ref, >ctrl_refs, node) {
> +   struct v4l2_ctrl *ctrl = ref->ctrl;
> +   struct v4l2_ctrl_ref *new_ref;
> +
> +   /* Skip refs inherited from other devices */
> +   if (ref->from_other_dev)
> +   continue;
> +   /* And buttons */
> +   if (ctrl->type == V4L2_CTRL_TYPE_BUTTON)
> +   continue;
> +   err = handler_new_ref(hdl, ctrl, _ref, false, true);
> +   if (err) {
> +   printk("%s: handler_new_ref on control %x (%s)
returned %d\n", __func__, ctrl->id, ctrl->name, err);

Perhaps pr_err()?

> +   err = 0;
> +   continue;

Hmm, is it really fine to ignore the error and continue here?

> +   }
> +   if (err)
> +   break;
> +   }
> +   mutex_unlock(from->lock);
> +   return err;
> +}
> +
> +static void v4l2_ctrl_request_queue(struct media_request_object *obj)
> +{
> +   struct v4l2_ctrl_handler *hdl =
> +   container_of(obj, struct v4l2_ctrl_handler, req_obj);
> +   struct v4l2_ctrl_handler *main_hdl = obj->priv;
> +   struct v4l2_ctrl_handler *prev = NULL;
> +   struct v4l2_ctrl_ref *ref_hdl, *ref_prev = NULL;
> +
> +   if (list_empty(_hdl->requests_queued))
> +   goto queue;
> +
> +   prev = list_last_entry(_hdl->requests_queued,
> +  struct v4l2_ctrl_handler, requests_queued);
> +   mutex_lock(prev->lock);
> +   ref_prev = list_first_entry(>ctrl_refs,
> +   struct v4l2_ctrl_ref, node);
> +   list_for_each_entry(ref_hdl, >ctrl_refs, node) {
> +   if (ref_hdl->req)
> +   continue;
> +   while (ref_prev->ctrl->id < ref_hdl->ctrl->id)
> +   ref_prev = list_next_entry(ref_prev, node);

Is this really safe? The only stop condition here is the control id.
Perhaps the code below could be better?

list_for_each_entry_from(ref_prev, >ctrl_refs, node)
 if (ref_prev->ctrl->id >= ref_hdl->ctrl->id)
 break;

> +   if (WARN_ON(ref_prev->ctrl->id != ref_hdl->ctrl->id))
> +   break;
> +   ref_hdl->req = ref_prev->req;
> +   }
> +   mutex_unlock(prev->lock);
> +queue:
> +   list_add_tail(>requests_queued, _hdl->requests_queued);
> +   hdl->request_is_queued = true;
> +}
> +

[snip]
> +void v4l2_ctrl_request_complete(struct media_request *req,
> +   struct v4l2_ctrl_handler *main_hdl)
> +{
> +   struct media_request_object *obj;
> +   struct v4l2_ctrl_handler *hdl;
> +   struct v4l2_ctrl_ref *ref;
> +
> +   if (!req || !main_hdl)
> +   return;

Can this happen normally? Perhaps WARN_ON() would make sense?

[snip]
> +void v4l2_ctrl_request_setup(struct media_request *req,
> +struct v4l2_ctrl_handler *main_hdl)
> +{
> +   struct media_request_object *obj;
> +   struct v4l2_ctrl_handler *hdl;
> +   struct v4l2_ctrl_ref *ref;
> +
> +   if (!req || !main_hdl)

Can this happen normally? Perhaps WARN_ON() would make sense?

> +   return;
> +
> +   obj = media_request_object_find(req, _ops, main_hdl);
> +   if (!obj)
> +   return;
> +   if (obj->completed) {
> +   media_request_object_put(obj);
> +   return;
> +   }
> +   hdl = container_of(obj, struct v4l2_ctrl_handler, req_obj);
> +
> +   mutex_lock(hdl->lock);
> +
> +   list_for_each_entry(ref, >ctrl_refs, node)
> +   ref->done = false;
> +
> +   list_for_each_entry(ref, >ctrl_refs, node) {
> +   struct v4l2_ctrl *ctrl = ref->ctrl;
> +   struct v4l2_ctrl *master = ctrl->cluster[0];
> +   bool have_new_data = false;
> +   int i;
> +
> +   /* Skip if this control was already handled by a cluster.
*/
> +   /* Skip button controls and read-only controls. */
> +   if (ref->done || 

Re: [RFCv11 PATCH 14/29] v4l2-ctrls: add core request support

[Paul Kocialkowski]

Hi,

On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Integrate the request support. This adds the
> v4l2_ctrl_request_complete
> and v4l2_ctrl_request_setup functions to complete a request and (as a
> helper function) to apply a request to the hardware.
> 
> It takes care of queuing requests and correctly chaining control
> values
> in the request queue.
> 
> Note that when a request is marked completed it will copy control
> values
> to the internal request state. This can be optimized in the future
> since
> this is sub-optimal when dealing with large compound and/or array
> controls.
> 
> For the initial 'stateless codec' use-case the current implementation
> is
> sufficient.

See one comment about a missing unlock below.

> Signed-off-by: Hans Verkuil 
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 325
> ++-
>  include/media/v4l2-ctrls.h   |  23 +++
>  2 files changed, 342 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index da4cc1485dc4..6e2c5e24734f 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1647,6 +1647,13 @@ static int new_to_user(struct v4l2_ext_control
> *c,
>   return ptr_to_user(c, ctrl, ctrl->p_new);
>  }
>  
> +/* Helper function: copy the request value back to the caller */
> +static int req_to_user(struct v4l2_ext_control *c,
> +struct v4l2_ctrl_ref *ref)
> +{
> + return ptr_to_user(c, ref->ctrl, ref->p_req);
> +}
> +
>  /* Helper function: copy the initial control value back to the caller
> */
>  static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl
> *ctrl)
>  {
> @@ -1766,6 +1773,26 @@ static void cur_to_new(struct v4l2_ctrl *ctrl)
>   ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new);
>  }
>  
> +/* Copy the new value to the request value */
> +static void new_to_req(struct v4l2_ctrl_ref *ref)
> +{
> + if (!ref)
> + return;
> + ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
> + ref->req = ref;
> +}
> +
> +/* Copy the request value to the new value */
> +static void req_to_new(struct v4l2_ctrl_ref *ref)
> +{
> + if (!ref)
> + return;
> + if (ref->req)
> + ptr_to_ptr(ref->ctrl, ref->req->p_req, ref->ctrl-
> >p_new);
> + else
> + ptr_to_ptr(ref->ctrl, ref->ctrl->p_cur, ref->ctrl-
> >p_new);
> +}
> +
>  /* Return non-zero if one or more of the controls in the cluster has
> a new
> value that differs from the current value. */
>  static int cluster_changed(struct v4l2_ctrl *master)
> @@ -1875,6 +1902,9 @@ int v4l2_ctrl_handler_init_class(struct
> v4l2_ctrl_handler *hdl,
>   lockdep_set_class_and_name(hdl->lock, key, name);
>   INIT_LIST_HEAD(>ctrls);
>   INIT_LIST_HEAD(>ctrl_refs);
> + INIT_LIST_HEAD(>requests);
> + INIT_LIST_HEAD(>requests_queued);
> + hdl->request_is_queued = false;
>   hdl->nr_of_buckets = 1 + nr_of_controls_hint / 8;
>   hdl->buckets = kvmalloc_array(hdl->nr_of_buckets,
> sizeof(hdl->buckets[0]),
> @@ -1895,6 +1925,14 @@ void v4l2_ctrl_handler_free(struct
> v4l2_ctrl_handler *hdl)
>   if (hdl == NULL || hdl->buckets == NULL)
>   return;
>  
> + if (!hdl->req_obj.req && !list_empty(>requests)) {
> + struct v4l2_ctrl_handler *req, *next_req;
> +
> + list_for_each_entry_safe(req, next_req, 
> >requests, requests) {
> + media_request_object_unbind(>req_obj);
> + media_request_object_put(>req_obj);
> + }
> + }
>   mutex_lock(hdl->lock);
>   /* Free all nodes */
>   list_for_each_entry_safe(ref, next_ref, >ctrl_refs,
> node) {
> @@ -2816,6 +2854,126 @@ int v4l2_querymenu(struct v4l2_ctrl_handler
> *hdl, struct v4l2_querymenu *qm)
>  }
>  EXPORT_SYMBOL(v4l2_querymenu);
>  
> +static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
> +const struct v4l2_ctrl_handler
> *from)
> +{
> + struct v4l2_ctrl_ref *ref;
> + int err;
> +
> + if (WARN_ON(!hdl || hdl == from))
> + return -EINVAL;
> +
> + if (hdl->error)
> + return hdl->error;
> +
> + WARN_ON(hdl->lock != >_lock);
> +
> + mutex_lock(from->lock);
> + list_for_each_entry(ref, >ctrl_refs, node) {
> + struct v4l2_ctrl *ctrl = ref->ctrl;
> + struct v4l2_ctrl_ref *new_ref;
> +
> + /* Skip refs inherited from other devices */
> + if (ref->from_other_dev)
> + continue;
> + /* And buttons */
> + if (ctrl->type == V4L2_CTRL_TYPE_BUTTON)
> + continue;
> + err = handler_new_ref(hdl, ctrl, _ref, false,
> true);
> + if (err) {
> + 

Re: [PATCH 1/6] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[jacopo mondi]

Hi Akinobu,

On Wed, Apr 11, 2018 at 01:37:03AM +0900, Akinobu Mita wrote:
> 2018-04-09 15:58 GMT+09:00 jacopo mondi :
> > Hello Akinobu,
> > thank you for the patch.
> >
> > On which platform have you tested the series (just curious) ?
>
> I use Zynq-7000 development board with Xilinx Video IP driver and
> custom video pipeline design based on the example reference project.
>

That's interesting! I was just wondering if there were other
development boards around that ship with this sensor...

> > On Sun, Apr 08, 2018 at 12:48:05AM +0900, Akinobu Mita wrote:
> >> The ov772x driver only works when the i2c controller have
> >> I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
> >> support it.
> >>
> >> The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
> >> it doesn't support repeated starts.
> >>
> >> This change adds an alternative method for reading from ov772x register
> >> which uses two separated i2c messages for the i2c controllers without
> >> I2C_FUNC_PROTOCOL_MANGLING.
> >
> > Actually, and please correct me if I'm wrong, what I see here is that
> > an i2c_master_send+i2c_master_recv sequence is equivalent to a mangled
> > smbus transaction:
> >
> > i2c_smbus_read_byte_data | I2C_CLIENT_SCCB:
> > S Addr Wr [A] Comm [A] P S Addr Rd [A] [Data] NA P
> >
> > i2c_master_send() + i2c_master_recv():
> > S Addr Wr [A] Data [A] P
> > S Addr Rd [A] [Data] NA P
> >
> > I wonder if it is not worth to ditch the existing read() function in
> > favour of your new proposed one completely.
> >
> > I have tested it on the Migo-R board where I have an ov772x installed
> > and it works fine.
>
> I'll replace the read() function to the new implementation in the next
> version of this patchset. Although handling in the I2C core is fascinating,
> I feel the area of influence is a bit large.
>

Yep, i2c is huge and complex but fascinating, and the SCCB use case
should probably be handled with a master_send+master_read instead of
relying on the i2c adapter ability to send an additional stop bit
in between an smbus transaction, as Laurent suggested. I wonder why it
has not been implemented this way from day 1. I'm sure there are
reasons :)

I'll wait for v2.
Thanks
   j

> > Although I would like to have a confirmation this is fine by people
> > how has seen more i2c adapters in action than me :)
> >
> > Thanks
> >j
> >
> >>
> >> Cc: Jacopo Mondi 
> >> Cc: Laurent Pinchart 
> >> Cc: Hans Verkuil 
> >> Cc: Sakari Ailus 
> >> Cc: Mauro Carvalho Chehab 
> >> Signed-off-by: Akinobu Mita 
> >> ---
> >>  drivers/media/i2c/ov772x.c | 42 +-
> >>  1 file changed, 33 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> >> index b62860c..283ae2c 100644
> >> --- a/drivers/media/i2c/ov772x.c
> >> +++ b/drivers/media/i2c/ov772x.c
> >> @@ -424,6 +424,7 @@ struct ov772x_priv {
> >>   /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
> >>   unsigned shortband_filter;
> >>   unsigned int  fps;
> >> + int (*reg_read)(struct i2c_client *client, u8 addr);
> >>  };
> >>
> >>  /*
> >> @@ -542,11 +543,34 @@ static struct ov772x_priv *to_ov772x(struct 
> >> v4l2_subdev *sd)
> >>   return container_of(sd, struct ov772x_priv, subdev);
> >>  }
> >>
> >> -static inline int ov772x_read(struct i2c_client *client, u8 addr)
> >> +static int ov772x_read(struct i2c_client *client, u8 addr)
> >> +{
> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> + struct ov772x_priv *priv = to_ov772x(sd);
> >> +
> >> + return priv->reg_read(client, addr);
> >> +}
> >> +
> >> +static int ov772x_reg_read(struct i2c_client *client, u8 addr)
> >>  {
> >>   return i2c_smbus_read_byte_data(client, addr);
> >>  }
> >>
> >> +static int ov772x_reg_read_fallback(struct i2c_client *client, u8 addr)
> >> +{
> >> + int ret;
> >> + u8 val;
> >> +
> >> + ret = i2c_master_send(client, , 1);
> >> + if (ret < 0)
> >> + return ret;
> >> + ret = i2c_master_recv(client, , 1);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return val;
> >> +}
> >> +
> >>  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 
> >> value)
> >>  {
> >>   return i2c_smbus_write_byte_data(client, addr, value);
> >> @@ -1255,20 +1279,20 @@ static int ov772x_probe(struct i2c_client *client,
> >>   return -EINVAL;
> >>   }
> >>
> >> - if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> >> -   I2C_FUNC_PROTOCOL_MANGLING)) {
> >> - dev_err(>dev,
> >> - "I2C-Adapter doesn't support SMBUS_BYTE_DATA or 
> >> 

Re: [RfC PATCH] Add udmabuf misc device

[Oleksandr Andrushchenko]

On 04/10/2018 08:26 PM, Dongwon Kim wrote:

On Tue, Apr 10, 2018 at 09:37:53AM +0300, Oleksandr Andrushchenko wrote:

On 04/06/2018 09:57 PM, Dongwon Kim wrote:

On Fri, Apr 06, 2018 at 03:36:03PM +0300, Oleksandr Andrushchenko wrote:

On 04/06/2018 02:57 PM, Gerd Hoffmann wrote:

   Hi,


I fail to see any common ground for xen-zcopy and udmabuf ...

Does the above mean you can assume that xen-zcopy and udmabuf
can co-exist as two different solutions?

Well, udmabuf route isn't fully clear yet, but yes.

See also gvt (intel vgpu), where the hypervisor interface is abstracted
away into a separate kernel modules even though most of the actual vgpu
emulation code is common.

Thank you for your input, I'm just trying to figure out
which of the three z-copy solutions intersect and how much

And what about hyper-dmabuf?

xen z-copy solution is pretty similar fundamentally to hyper_dmabuf
in terms of these core sharing feature:

1. the sharing process - import prime/dmabuf from the producer -> extract
underlying pages and get those shared -> return references for shared pages

Another thing is danvet was kind of against to the idea of importing existing
dmabuf/prime buffer and forward it to the other domain due to synchronization
issues. He proposed to make hyper_dmabuf only work as an exporter so that it
can have a full control over the buffer. I think we need to talk about this
further as well.

Yes, I saw this. But this limits the use-cases so much.
For instance, running Android as a Guest (which uses ION to allocate
buffers) means that finally HW composer will import dma-buf into
the DRM driver. Then, in case of xen-front for example, it needs to be
shared with the backend (Host side). Of course, we can change user-space
to make xen-front allocate the buffers (make it exporter), but what we try
to avoid is to change user-space which in normal world would have remain
unchanged otherwise.
So, I do think we have to support this use-case and just have to understand
the complexity.



danvet, can you comment on this topic?


2. the page sharing mechanism - it uses Xen-grant-table.

And to give you a quick summary of differences as far as I understand
between two implementations (please correct me if I am wrong, Oleksandr.)

1. xen-zcopy is DRM specific - can import only DRM prime buffer
while hyper_dmabuf can export any dmabuf regardless of originator

Well, this is true. And at the same time this is just a matter
of extending the API: xen-zcopy is a helper driver designed for
xen-front/back use-case, so this is why it only has DRM PRIME API

2. xen-zcopy doesn't seem to have dma-buf synchronization between two VMs
while (as danvet called it as remote dmabuf api sharing) hyper_dmabuf sends
out synchronization message to the exporting VM for synchronization.

This is true. Again, this is because of the use-cases it covers.
But having synchronization for a generic solution seems to be a good idea.

Yeah, understood xen-zcopy works ok with your use case. But I am just curious
if it is ok not to have any inter-domain synchronization in this sharing model.

The synchronization is done with displif protocol [1]

The buffer being shared is technically dma-buf and originator needs to be able
to keep track of it.

As I am working in DRM terms the tracking is done by the DRM core
for me for free. (This might be one of the reasons Daniel sees DRM
based implementation fit very good from code-reuse POV).



3. 1-level references - when using grant-table for sharing pages, there will
be same # of refs (each 8 byte)

To be precise, grant ref is 4 bytes

You are right. Thanks for correction.;)


as # of shared pages, which is passed to
the userspace to be shared with importing VM in case of xen-zcopy.

The reason for that is that xen-zcopy is a helper driver, e.g.
the grant references come from the display backend [1], which implements
Xen display protocol [2]. So, effectively the backend extracts references
from frontend's requests and passes those to xen-zcopy as an array
of refs.

  Compared
to this, hyper_dmabuf does multiple level addressing to generate only one
reference id that represents all shared pages.

In the protocol [2] only one reference to the gref directory is passed
between VMs
(and the gref directory is a single-linked list of shared pages containing
all
of the grefs of the buffer).

ok, good to know. I will look into its implementation in more details but is
this gref directory (chained grefs) something that can be used for any general
memory sharing use case or is it jsut for xen-display (in current code base)?

Not to mislead you: one grant ref is passed via displif protocol,
but the page it's referencing contains the rest of the grant refs.

As to if this can be used for any memory: yes. It is the same for
sndif and displif Xen protocols, but defined twice as strictly speaking
sndif and displif are two separate protocols.

While reviewing your RFC v2 one of the comments I had [2] was that if we
can start