Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Sakari Ailus]

On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
> Sakari Ailus  writes:
> 
> > Hi Kevin,
> >
> > On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >> Hi Sakari,
> >> 
> >> Sakari Ailus  writes:
> >> 
> >> > On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
> >> >> Allow getting of subdevs from DT ports and endpoints.
> >> >> 
> >> >> The _get_pdata() function was larely inspired by (i.e. stolen from)
> >> >
> >> > vpif_capture_get_pdata and "largely"?
> >> 
> >> Yes, thanks.
> >> 
> >> >> am437x-vpfe.c
> >> >> 
> >> >> Signed-off-by: Kevin Hilman 
> >> >> ---
> >> >>  drivers/media/platform/davinci/vpif_capture.c | 130 
> >> >> +-
> >> >>  include/media/davinci/vpif_types.h|   9 +-
> >> >>  2 files changed, 133 insertions(+), 6 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/media/platform/davinci/vpif_capture.c 
> >> >> b/drivers/media/platform/davinci/vpif_capture.c
> >> >> index 94ee6cf03f02..47a4699157e7 100644
> >> >> --- a/drivers/media/platform/davinci/vpif_capture.c
> >> >> +++ b/drivers/media/platform/davinci/vpif_capture.c
> >> >> @@ -26,6 +26,8 @@
> >> >>  #include 
> >> >>  
> >> >>  #include 
> >> >> +#include 
> >> >> +#include 
> >> >
> >> > Do you need this header?
> >> >
> >> 
> >> Yes, based on discussion with Hans, since there is no DT binding for
> >> selecting the input pins of the TVP514x, I have to select it in the
> >> driver, so I need the defines from this header.  More on this below...
> >> 
> >> >>  
> >> >>  #include "vpif.h"
> >> >>  #include "vpif_capture.h"
> >> >> @@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
> >> >>  
> >> >> vpif_dbg(2, debug, "vpif_input_to_subdev\n");
> >> >>  
> >> >> +   if (!chan_cfg)
> >> >> +   return -1;
> >> >> +   if (input_index >= chan_cfg->input_count)
> >> >> +   return -1;
> >> >> subdev_name = chan_cfg->inputs[input_index].subdev_name;
> >> >> if (subdev_name == NULL)
> >> >> return -1;
> >> >> @@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
> >> >> /* loop through the sub device list to get the sub device info 
> >> >> */
> >> >> for (i = 0; i < vpif_cfg->subdev_count; i++) {
> >> >> subdev_info = _cfg->subdev_info[i];
> >> >> -   if (!strcmp(subdev_info->name, subdev_name))
> >> >> +   if (subdev_info && !strcmp(subdev_info->name, 
> >> >> subdev_name))
> >> >> return i;
> >> >> }
> >> >> return -1;
> >> >> @@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct 
> >> >> v4l2_async_notifier *notifier,
> >> >>  {
> >> >> int i;
> >> >>  
> >> >> +   for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
> >> >> +   struct v4l2_async_subdev *_asd = 
> >> >> vpif_obj.config->asd[i];
> >> >> +   const struct device_node *node = _asd->match.of.node;
> >> >> +
> >> >> +   if (node == subdev->of_node) {
> >> >> +   vpif_obj.sd[i] = subdev;
> >> >> +   
> >> >> vpif_obj.config->chan_config->inputs[i].subdev_name =
> >> >> +   (char *)subdev->of_node->full_name;
> >> >> +   vpif_dbg(2, debug,
> >> >> +"%s: setting input %d subdev_name = 
> >> >> %s\n",
> >> >> +__func__, i, 
> >> >> subdev->of_node->full_name);
> >> >> +   return 0;
> >> >> +   }
> >> >> +   }
> >> >> +
> >> >> for (i = 0; i < vpif_obj.config->subdev_count; i++)
> >> >> if (!strcmp(vpif_obj.config->subdev_info[i].name,
> >> >> subdev->name)) {
> >> >> @@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct 
> >> >> v4l2_async_notifier *notifier)
> >> >> return vpif_probe_complete();
> >> >>  }
> >> >>  
> >> >> +static struct vpif_capture_config *
> >> >> +vpif_capture_get_pdata(struct platform_device *pdev)
> >> >> +{
> >> >> +   struct device_node *endpoint = NULL;
> >> >> +   struct v4l2_of_endpoint bus_cfg;
> >> >> +   struct vpif_capture_config *pdata;
> >> >> +   struct vpif_subdev_info *sdinfo;
> >> >> +   struct vpif_capture_chan_config *chan;
> >> >> +   unsigned int i;
> >> >> +
> >> >> +   dev_dbg(>dev, "vpif_get_pdata\n");
> >> >> +
> >> >> +   if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
> >> >> +   return pdev->dev.platform_data;
> >> >> +
> >> >> +   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> >> >> +   if (!pdata)
> >> >> +   return NULL;
> >> >> +   pdata->subdev_info =
> >> >> +   devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
> >> >> +VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
> >> >> +
> >> >> +   if 

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 Dec  1 05:00:17 CET 2016
media-tree git hash:003611334d5592984e319e08c6b66825aca00290
media_build git hash:   1606032398b1d79149c1507be2029e1a00d8dff0
v4l-utils git hash: f6ecbc90656815d91dc6ba90aac0ad8193a14b38
gcc version:i686-linux-gcc (GCC) 6.2.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.8.0-164

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.2.37-i686: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: OK
linux-3.12.67-i686: OK
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.16.7-i686: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: OK
linux-4.9-rc5-i686: OK
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.37-x86_64: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-x86_64: WARNINGS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: OK
linux-3.12.67-x86_64: OK
linux-3.13.11-x86_64: WARNINGS
linux-3.14.9-x86_64: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.7-x86_64: WARNINGS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: OK
linux-4.9-rc5-x86_64: OK
apps: WARNINGS
spec-git: OK
smatch: ERRORS
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Platform: Sti: Bdisp: Clean up file handle in open() error path.

[Shailendra Verma]

The File handle is not yet added in the vdev list.So no need to call 
v4l2_fh_del(>fh)if it fails to create control.

Signed-off-by: Shailendra Verma 
---
 drivers/media/platform/sti/bdisp/bdisp-v4l2.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c 
b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
index 45f82b5..fbf302f 100644
--- a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
+++ b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
@@ -632,8 +632,8 @@ static int bdisp_open(struct file *file)
 
 error_ctrls:
bdisp_ctrls_delete(ctx);
-error_fh:
v4l2_fh_del(>fh);
+error_fh:
v4l2_fh_exit(>fh);
bdisp_hw_free_nodes(ctx);
 mem_ctx:
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] V4l: omap4iss: Clean up file handle in open() and release().

[Shailendra Verma]

Both functions initialize the file handle with v4l2_fh_init()
and thus need to call clean up with v4l2_fh_exit() as appropriate.

Signed-off-by: Shailendra Verma 
---
 drivers/staging/media/omap4iss/iss_video.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/media/omap4iss/iss_video.c 
b/drivers/staging/media/omap4iss/iss_video.c
index c16927a..077c9f8 100644
--- a/drivers/staging/media/omap4iss/iss_video.c
+++ b/drivers/staging/media/omap4iss/iss_video.c
@@ -1141,6 +1141,7 @@ static int iss_video_open(struct file *file)
 done:
if (ret < 0) {
v4l2_fh_del(>vfh);
+   v4l2_fh_exit(>vfh);
kfree(handle);
}
 
@@ -1162,6 +1163,7 @@ static int iss_video_release(struct file *file)
vb2_queue_release(>queue);
 
v4l2_fh_del(vfh);
+   v4l2_fh_exit(vfh);
kfree(handle);
file->private_data = NULL;
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Platform: vsp1: Clean up file handle in open() error path.

[Shailendra Verma]

v4l2_fh_init is already done.So call the v4l2_fh_exit in error condition
before returing from the function.

Signed-off-by: Shailendra Verma 
---
 drivers/media/platform/vsp1/vsp1_video.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/vsp1/vsp1_video.c 
b/drivers/media/platform/vsp1/vsp1_video.c
index d351b9c..cc58163 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -1044,6 +1044,7 @@ static int vsp1_video_open(struct file *file)
ret = vsp1_device_get(video->vsp1);
if (ret < 0) {
v4l2_fh_del(vfh);
+   v4l2_fh_exit(vfh);
kfree(vfh);
}
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] V4l: omap3isp: Clean up file handle in open() and release().

[Shailendra Verma]

Both functions initialize the file handle with v4l2_fh_init()
and thus need to call clean up with v4l2_fh_exit() as appropriate.

Signed-off-by: Shailendra Verma 
---
 drivers/media/platform/omap3isp/ispvideo.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c 
b/drivers/media/platform/omap3isp/ispvideo.c
index 7354469..9f966e8 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1350,6 +1350,7 @@ static int isp_video_open(struct file *file)
 done:
if (ret < 0) {
v4l2_fh_del(>vfh);
+   v4l2_fh_exit(>vfh);
kfree(handle);
}
 
@@ -1373,6 +1374,7 @@ static int isp_video_release(struct file *file)
 
/* Release the file handle. */
v4l2_fh_del(vfh);
+   v4l2_fh_exit(vfh);
kfree(handle);
file->private_data = NULL;
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Platform: Exynos-gsc: Clean up file handle in open() error path.

[Shailendra Verma]

The File handle is not yet added in the vfd list.So no need to call
v4l2_fh_del(>fh) if it fails to create control.

Signed-off-by: Shailendra Verma 
---
 drivers/media/platform/exynos-gsc/gsc-m2m.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c 
b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 9f03b79..5ea97c1 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -664,8 +664,8 @@ static int gsc_m2m_open(struct file *file)
 
 error_ctrls:
gsc_ctrls_delete(ctx);
-error_fh:
v4l2_fh_del(>fh);
+error_fh:
v4l2_fh_exit(>fh);
kfree(ctx);
 unlock:
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Platform: Exynos4-is: Clean up file handle in open() error path.

[Shailendra Verma]

The File handle is not yet added in the vfd list.So no need to call 
v4l2_fh_del(>fh) if it fails to create control.

Signed-off-by: Shailendra Verma 
---
 drivers/media/platform/exynos4-is/fimc-m2m.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c 
b/drivers/media/platform/exynos4-is/fimc-m2m.c
index 6028e4f..d8724fe 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -663,8 +663,8 @@ static int fimc_m2m_open(struct file *file)
v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
 error_c:
fimc_ctrls_delete(ctx);
-error_fh:
v4l2_fh_del(>fh);
+error_fh:
v4l2_fh_exit(>fh);
kfree(ctx);
 unlock:
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GIT PULL STABLE] mn88472 & mn88473 chip id bug on probe

[Antti Palosaari]

I already send these 2 simple patches to stable list, but not sure how 
to get those mainline so here is pull-request too. Both are same bug, 
which nacks driver probe when chip is already on warm state.


regards
Antti

The following changes since commit 003611334d5592984e319e08c6b66825aca00290:

  [media] s5p-mfc: Add support for MFC v8 available in Exynos 5433 SoCs 
(2016-11-30 09:22:07 -0200)


are available in the git repository at:

  git://linuxtv.org/anttip/media_tree.git mn88473

for you to fetch changes up to c4367e9f774e85da1cd00c7f48da5684b57b9b06:

  mn88472: fix chip id check on probe (2016-12-01 02:27:11 +0200)


Antti Palosaari (2):
  mn88473: fix chip id check on probe
  mn88472: fix chip id check on probe

 drivers/media/dvb-frontends/mn88472.c | 24 
 drivers/media/dvb-frontends/mn88473.c | 24 
 2 files changed, 24 insertions(+), 24 deletions(-)

--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] mn88473: fix chip id check on probe

[Antti Palosaari]

A register used to identify chip during probe was overwritten during
firmware download and due to that later probe's for warm chip were
failing. Detect chip from the another register, which is located on
different register bank 2.

Fixes: 7908fad99a6c ("[media] mn88473: finalize driver")
Cc:  # v4.8+
Signed-off-by: Antti Palosaari 
---
 drivers/media/dvb-frontends/mn88473.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/media/dvb-frontends/mn88473.c 
b/drivers/media/dvb-frontends/mn88473.c
index f3b59a5..c221c7d 100644
--- a/drivers/media/dvb-frontends/mn88473.c
+++ b/drivers/media/dvb-frontends/mn88473.c
@@ -648,18 +648,6 @@ static int mn88473_probe(struct i2c_client *client,
goto err_kfree;
}
 
-   /* Check demod answers with correct chip id */
-   ret = regmap_read(dev->regmap[0], 0xff, );
-   if (ret)
-   goto err_regmap_0_regmap_exit;
-
-   dev_dbg(>dev, "chip id=%02x\n", uitmp);
-
-   if (uitmp != 0x03) {
-   ret = -ENODEV;
-   goto err_regmap_0_regmap_exit;
-   }
-
/*
 * Chip has three I2C addresses for different register banks. Used
 * addresses are 0x18, 0x1a and 0x1c. We register two dummy clients,
@@ -696,6 +684,18 @@ static int mn88473_probe(struct i2c_client *client,
}
i2c_set_clientdata(dev->client[2], dev);
 
+   /* Check demod answers with correct chip id */
+   ret = regmap_read(dev->regmap[2], 0xff, );
+   if (ret)
+   goto err_regmap_2_regmap_exit;
+
+   dev_dbg(>dev, "chip id=%02x\n", uitmp);
+
+   if (uitmp != 0x03) {
+   ret = -ENODEV;
+   goto err_regmap_2_regmap_exit;
+   }
+
/* Sleep because chip is active by default */
ret = regmap_write(dev->regmap[2], 0x05, 0x3e);
if (ret)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Kevin Hilman]

Sakari Ailus  writes:

> Hi Kevin,
>
> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>> Hi Sakari,
>> 
>> Sakari Ailus  writes:
>> 
>> > On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>> >> Allow getting of subdevs from DT ports and endpoints.
>> >> 
>> >> The _get_pdata() function was larely inspired by (i.e. stolen from)
>> >
>> > vpif_capture_get_pdata and "largely"?
>> 
>> Yes, thanks.
>> 
>> >> am437x-vpfe.c
>> >> 
>> >> Signed-off-by: Kevin Hilman 
>> >> ---
>> >>  drivers/media/platform/davinci/vpif_capture.c | 130 
>> >> +-
>> >>  include/media/davinci/vpif_types.h|   9 +-
>> >>  2 files changed, 133 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/drivers/media/platform/davinci/vpif_capture.c 
>> >> b/drivers/media/platform/davinci/vpif_capture.c
>> >> index 94ee6cf03f02..47a4699157e7 100644
>> >> --- a/drivers/media/platform/davinci/vpif_capture.c
>> >> +++ b/drivers/media/platform/davinci/vpif_capture.c
>> >> @@ -26,6 +26,8 @@
>> >>  #include 
>> >>  
>> >>  #include 
>> >> +#include 
>> >> +#include 
>> >
>> > Do you need this header?
>> >
>> 
>> Yes, based on discussion with Hans, since there is no DT binding for
>> selecting the input pins of the TVP514x, I have to select it in the
>> driver, so I need the defines from this header.  More on this below...
>> 
>> >>  
>> >>  #include "vpif.h"
>> >>  #include "vpif_capture.h"
>> >> @@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
>> >>  
>> >>   vpif_dbg(2, debug, "vpif_input_to_subdev\n");
>> >>  
>> >> + if (!chan_cfg)
>> >> + return -1;
>> >> + if (input_index >= chan_cfg->input_count)
>> >> + return -1;
>> >>   subdev_name = chan_cfg->inputs[input_index].subdev_name;
>> >>   if (subdev_name == NULL)
>> >>   return -1;
>> >> @@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
>> >>   /* loop through the sub device list to get the sub device info */
>> >>   for (i = 0; i < vpif_cfg->subdev_count; i++) {
>> >>   subdev_info = _cfg->subdev_info[i];
>> >> - if (!strcmp(subdev_info->name, subdev_name))
>> >> + if (subdev_info && !strcmp(subdev_info->name, subdev_name))
>> >>   return i;
>> >>   }
>> >>   return -1;
>> >> @@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct 
>> >> v4l2_async_notifier *notifier,
>> >>  {
>> >>   int i;
>> >>  
>> >> + for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
>> >> + struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
>> >> + const struct device_node *node = _asd->match.of.node;
>> >> +
>> >> + if (node == subdev->of_node) {
>> >> + vpif_obj.sd[i] = subdev;
>> >> + vpif_obj.config->chan_config->inputs[i].subdev_name =
>> >> + (char *)subdev->of_node->full_name;
>> >> + vpif_dbg(2, debug,
>> >> +  "%s: setting input %d subdev_name = %s\n",
>> >> +  __func__, i, subdev->of_node->full_name);
>> >> + return 0;
>> >> + }
>> >> + }
>> >> +
>> >>   for (i = 0; i < vpif_obj.config->subdev_count; i++)
>> >>   if (!strcmp(vpif_obj.config->subdev_info[i].name,
>> >>   subdev->name)) {
>> >> @@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct 
>> >> v4l2_async_notifier *notifier)
>> >>   return vpif_probe_complete();
>> >>  }
>> >>  
>> >> +static struct vpif_capture_config *
>> >> +vpif_capture_get_pdata(struct platform_device *pdev)
>> >> +{
>> >> + struct device_node *endpoint = NULL;
>> >> + struct v4l2_of_endpoint bus_cfg;
>> >> + struct vpif_capture_config *pdata;
>> >> + struct vpif_subdev_info *sdinfo;
>> >> + struct vpif_capture_chan_config *chan;
>> >> + unsigned int i;
>> >> +
>> >> + dev_dbg(>dev, "vpif_get_pdata\n");
>> >> +
>> >> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>> >> + return pdev->dev.platform_data;
>> >> +
>> >> + pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
>> >> + if (!pdata)
>> >> + return NULL;
>> >> + pdata->subdev_info =
>> >> + devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
>> >> +  VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
>> >> +
>> >> + if (!pdata->subdev_info)
>> >> + return NULL;
>> >> + dev_dbg(>dev, "%s\n", __func__);
>> >> +
>> >> + for (i = 0; ; i++) {
>> >> + struct device_node *rem;
>> >> + unsigned int flags;
>> >> + int err;
>> >> +
>> >> + endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
>> >> +   endpoint);
>> >> + if (!endpoint)
>> >> + break;
>> >> +
>> >> + sdinfo = >subdev_info[i];
>> >
>> > subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.
>> >
>> 
>> Right, I need to make the loop only go for a max of
>> VPIF_CAPTURE_MAX_CHANNELS iterations.
>> 
>> 

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Kevin Hilman]

Sakari Ailus  writes:

> Hi Rob and Kevin,
>
> On Tue, Nov 29, 2016 at 08:41:44AM -0600, Rob Herring wrote:
>> On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman  wrote:
>> > Hi Rob,
>> >
>> > Rob Herring  writes:
>> >
>> >> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>> >>> Signed-off-by: Kevin Hilman 
>> >>> ---
>> >>>  .../bindings/media/ti,da850-vpif-capture.txt   | 65 
>> >>> ++
>> >>>  .../devicetree/bindings/media/ti,da850-vpif.txt|  8 +++
>> >>>  2 files changed, 73 insertions(+)
>> >>>  create mode 100644 
>> >>> Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> >>>  create mode 100644 
>> >>> Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> >>>
>> >>> diff --git 
>> >>> a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt 
>> >>> b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> >>> new file mode 100644
>> >>> index ..c447ac482c1d
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> >>> @@ -0,0 +1,65 @@
>> >>> +Texas Instruments VPIF Capture
>> >>> +--
>> >>> +
>> >>> +The TI Video Port InterFace (VPIF) capture component is the primary
>> >>> +component for video capture on the DA850 family of TI DaVinci SoCs.
>> >>> +
>> >>> +TI Document number reference: SPRUH82C
>> >>> +
>> >>> +Required properties:
>> >>> +- compatible: must be "ti,da850-vpif-capture"
>> >>> +- reg: physical base address and length of the registers set for the 
>> >>> device;
>> >>> +- interrupts: should contain IRQ line for the VPIF
>> >>> +
>> >>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
>> >>> +channels or a single 16-bit channel.  It should contain at least one
>> >>> +port child node with child 'endpoint' node. Please refer to the
>> >>> +bindings defined in
>> >>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> >>> +
>> >>> +Example using 2 8-bit input channels, one of which is connected to an
>> >>> +I2C-connected TVP5147 decoder:
>> >>> +
>> >>> +vpif_capture: video-capture@0x00217000 {
>> >>> +reg = <0x00217000 0x1000>;
>> >>> +interrupts = <92>;
>> >>> +
>> >>> +port {
>> >>> +vpif_ch0: endpoint@0 {
>> >>> +  reg = <0>;
>> >>> +  bus-width = <8>;
>> >>> +  remote-endpoint = <>;
>> >>> +};
>> >>> +
>> >>> +vpif_ch1: endpoint@1 {
>> >>
>> >> I think probably channels here should be ports rather than endpoints.
>> >> AIUI, having multiple endpoints is for cases like a mux or 1 to many
>> >> connections. There's only one data flow, but multiple sources or sinks.
>> >
>> > Looking at this closer... , I used an endpoint because it's bascially a
>> > 16-bit parallel bus, that can be configured as (up to) 2 8-bit
>> > "channels.  So, based on the video-interfaces.txt doc, I configured this
>> > as a single port, with (up to) 2 endpoints.  That also allows me to
>> > connect output of the decoder directly, using the remote-endpoint
>> > property.
>> >
>> > So I guess I'm not fully understanding your suggestion.
>> 
>> NM, looks like video-interfaces.txt actually spells out this case and
>> defines doing it as you did.
>
> It's actually the first time I read that portion (at least so that I could
> remember) of video-interfaces.txt. I'm not sure if anyone has implemented
> that previously, nor how we ended up with the text. The list archive could
> probably tell. Cc Guennadi who wrote it. :-) I couldn't immediately find DT
> source with this arrangement.
>
> In case of splitting the port into two parallel interfaces, how do you
> determine which wires belong to which endpoint? I guess they'd be particular
> sets of wires but as there's just a single port it isn't defined by the
> port.

Isn't that the point of data-shift?

e.g. it's a single 16-bit parallel bus, where the lower 8 bits are for
channel 0 and the upper 8 bits are for channel 1.  Alternately, the port
can also be configured as a single 16-bit channel (e.g. for raw
capture.)

If you want more details on this hardware, it's pretty well described in
Chapter 35 of http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf.

FWIW, I'm not really picky about how to do this.  I'm trying to learn
"the right way" and am happy to do that, but the feedback so far has
been confusing (at least for someone relatively new to the DT side of
the media framework.)

Kevin

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Sakari Ailus]

Hi Kevin,

On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> Hi Sakari,
> 
> Sakari Ailus  writes:
> 
> > On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
> >> Allow getting of subdevs from DT ports and endpoints.
> >> 
> >> The _get_pdata() function was larely inspired by (i.e. stolen from)
> >
> > vpif_capture_get_pdata and "largely"?
> 
> Yes, thanks.
> 
> >> am437x-vpfe.c
> >> 
> >> Signed-off-by: Kevin Hilman 
> >> ---
> >>  drivers/media/platform/davinci/vpif_capture.c | 130 
> >> +-
> >>  include/media/davinci/vpif_types.h|   9 +-
> >>  2 files changed, 133 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/davinci/vpif_capture.c 
> >> b/drivers/media/platform/davinci/vpif_capture.c
> >> index 94ee6cf03f02..47a4699157e7 100644
> >> --- a/drivers/media/platform/davinci/vpif_capture.c
> >> +++ b/drivers/media/platform/davinci/vpif_capture.c
> >> @@ -26,6 +26,8 @@
> >>  #include 
> >>  
> >>  #include 
> >> +#include 
> >> +#include 
> >
> > Do you need this header?
> >
> 
> Yes, based on discussion with Hans, since there is no DT binding for
> selecting the input pins of the TVP514x, I have to select it in the
> driver, so I need the defines from this header.  More on this below...
> 
> >>  
> >>  #include "vpif.h"
> >>  #include "vpif_capture.h"
> >> @@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
> >>  
> >>vpif_dbg(2, debug, "vpif_input_to_subdev\n");
> >>  
> >> +  if (!chan_cfg)
> >> +  return -1;
> >> +  if (input_index >= chan_cfg->input_count)
> >> +  return -1;
> >>subdev_name = chan_cfg->inputs[input_index].subdev_name;
> >>if (subdev_name == NULL)
> >>return -1;
> >> @@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
> >>/* loop through the sub device list to get the sub device info */
> >>for (i = 0; i < vpif_cfg->subdev_count; i++) {
> >>subdev_info = _cfg->subdev_info[i];
> >> -  if (!strcmp(subdev_info->name, subdev_name))
> >> +  if (subdev_info && !strcmp(subdev_info->name, subdev_name))
> >>return i;
> >>}
> >>return -1;
> >> @@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct 
> >> v4l2_async_notifier *notifier,
> >>  {
> >>int i;
> >>  
> >> +  for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
> >> +  struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
> >> +  const struct device_node *node = _asd->match.of.node;
> >> +
> >> +  if (node == subdev->of_node) {
> >> +  vpif_obj.sd[i] = subdev;
> >> +  vpif_obj.config->chan_config->inputs[i].subdev_name =
> >> +  (char *)subdev->of_node->full_name;
> >> +  vpif_dbg(2, debug,
> >> +   "%s: setting input %d subdev_name = %s\n",
> >> +   __func__, i, subdev->of_node->full_name);
> >> +  return 0;
> >> +  }
> >> +  }
> >> +
> >>for (i = 0; i < vpif_obj.config->subdev_count; i++)
> >>if (!strcmp(vpif_obj.config->subdev_info[i].name,
> >>subdev->name)) {
> >> @@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct 
> >> v4l2_async_notifier *notifier)
> >>return vpif_probe_complete();
> >>  }
> >>  
> >> +static struct vpif_capture_config *
> >> +vpif_capture_get_pdata(struct platform_device *pdev)
> >> +{
> >> +  struct device_node *endpoint = NULL;
> >> +  struct v4l2_of_endpoint bus_cfg;
> >> +  struct vpif_capture_config *pdata;
> >> +  struct vpif_subdev_info *sdinfo;
> >> +  struct vpif_capture_chan_config *chan;
> >> +  unsigned int i;
> >> +
> >> +  dev_dbg(>dev, "vpif_get_pdata\n");
> >> +
> >> +  if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
> >> +  return pdev->dev.platform_data;
> >> +
> >> +  pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> >> +  if (!pdata)
> >> +  return NULL;
> >> +  pdata->subdev_info =
> >> +  devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
> >> +   VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
> >> +
> >> +  if (!pdata->subdev_info)
> >> +  return NULL;
> >> +  dev_dbg(>dev, "%s\n", __func__);
> >> +
> >> +  for (i = 0; ; i++) {
> >> +  struct device_node *rem;
> >> +  unsigned int flags;
> >> +  int err;
> >> +
> >> +  endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
> >> +endpoint);
> >> +  if (!endpoint)
> >> +  break;
> >> +
> >> +  sdinfo = >subdev_info[i];
> >
> > subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.
> >
> 
> Right, I need to make the loop only go for a max of
> VPIF_CAPTURE_MAX_CHANNELS iterations.
> 
> >> +  chan = >chan_config[i];
> >> +  chan->inputs = devm_kzalloc(>dev,
> >> +   

Re: [PATCH 01/10] doc: DT: camss: Binding document for Qualcomm Camera subsystem driver

[Rob Herring]

On Fri, Nov 25, 2016 at 04:56:53PM +0200, Todor Tomov wrote:
> Add DT binding document for Qualcomm Camera subsystem driver.
> 
> Signed-off-by: Todor Tomov 
> ---
>  .../devicetree/bindings/media/qcom,camss.txt   | 196 
> +
>  1 file changed, 196 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/qcom,camss.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,camss.txt 
> b/Documentation/devicetree/bindings/media/qcom,camss.txt
> new file mode 100644
> index 000..76ad89a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,camss.txt
> @@ -0,0 +1,196 @@
> +Qualcomm Camera Subsystem
> +
> +* Properties
> +
> +- compatible:
> + Usage: required
> + Value type: 
> + Definition: Should contain:
> + - "qcom,8x16-camss"

Don't use wildcards in compatible strings. One string per SoC.

> +- reg:
> + Usage: required
> + Value type: 
> + Definition: Register ranges as listed in the reg-names property.
> +- reg-names:
> + Usage: required
> + Value type: 
> + Definition: Should contain the following entries:
> + - "csiphy0"
> + - "csiphy0_clk_mux"
> + - "csiphy1"
> + - "csiphy1_clk_mux"
> + - "csid0"
> + - "csid1"
> + - "ispif"
> + - "csi_clk_mux"
> + - "vfe0"

Kind of looks like the phy's should be separate nodes since each phy has 
its own register range, irq, clocks, etc.

> +- interrupts:
> + Usage: required
> + Value type: 
> + Definition: Interrupts as listed in the interrupt-names property.
> +- interrupt-names:
> + Usage: required
> + Value type: 
> + Definition: Should contain the following entries:
> + - "csiphy0"
> + - "csiphy1"
> + - "csid0"
> + - "csid1"
> + - "ispif"
> + - "vfe0"
> +- power-domains:
> + Usage: required
> + Value type: 
> + Definition: A phandle and power domain specifier pairs to the
> + power domain which is responsible for collapsing
> + and restoring power to the peripheral.
> +- clocks:
> + Usage: required
> + Value type: 
> + Definition: A list of phandle and clock specifier pairs as listed
> + in clock-names property.
> +- clock-names:
> + Usage: required
> + Value type: 
> + Definition: Should contain the following entries:
> + - "camss_top_ahb_clk"
> + - "ispif_ahb_clk"
> + - "csiphy0_timer_clk"
> + - "csiphy1_timer_clk"
> + - "csi0_ahb_clk"
> + - "csi0_clk"
> + - "csi0_phy_clk"
> + - "csi0_pix_clk"
> + - "csi0_rdi_clk"
> + - "csi1_ahb_clk"
> + - "csi1_clk"
> + - "csi1_phy_clk"
> + - "csi1_pix_clk"
> + - "csi1_rdi_clk"
> + - "camss_ahb_clk"
> + - "camss_vfe_vfe_clk"
> + - "camss_csi_vfe_clk"
> + - "iface_clk"
> + - "bus_clk"
> +- vdda-supply:
> + Usage: required
> + Value type: 
> + Definition: A phandle to voltage supply for CSI2.
> +- iommus:
> + Usage: required
> + Value type: 
> + Definition: A list of phandle and IOMMU specifier pairs.
> +
> +* Nodes
> +
> +- ports:
> + Usage: required
> + Definition: As described in video-interfaces.txt in same directory.
> + Properties:
> + - reg:
> + Usage: required
> + Value type: 
> + Definition: Selects CSI2 PHY interface - PHY0 or PHY1.
> + Endpoint node properties:
> + - clock-lanes:
> + Usage: required
> + Value type: 
> + Definition: The clock lane.
> + - data-lanes:
> + Usage: required
> + Value type: 
> + Definition: An array of data lanes.
> + - qcom,settle-cnt:

This should go in phy node ideally.

> + Usage: required
> + Value type: 
> + Definition: The settle count parameter for CSI PHY.
> +
> +* An Example
> +
> + camss: camss@1b0 {
> + compatible = "qcom,8x16-camss";
> + reg = <0x1b0ac00 0x200>,
> + <0x1b00030 0x4>,
> + <0x1b0b000 0x200>,
> + <0x1b00038 0x4>,
> + <0x1b08000 0x100>,
> + <0x1b08400 0x100>,
> + <0x1b0a000 0x500>,
> + <0x1b00020 0x10>,
> + <0x1b1 0x1000>;
> + reg-names = "csiphy0",
> + "csiphy0_clk_mux",
> + "csiphy1",
> + "csiphy1_clk_mux",
> + 

[PATCH v6 1/3] media: Media Device Allocator API

[Shuah Khan]

Media Device Allocator API to allows multiple drivers share a media device.
Using this API, drivers can allocate a media device with the shared struct
device as the key. Once the media device is allocated by a driver, other
drivers can get a reference to it. The media device is released when all
the references are released.

Signed-off-by: Shuah Khan 
Reviewed-by: Hans Verkuil 
---
Changes to patch 0001 since v5: (comments from Mauro and Sakari)
- Removed struct device from media_device_instance. mdev.dev is used instead.
- Added documentation.

 Documentation/media/kapi/mc-core.rst |  37 ++
 drivers/media/Makefile   |   3 +-
 drivers/media/media-dev-allocator.c  | 133 +++
 include/media/media-dev-allocator.h  |  54 ++
 4 files changed, 226 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/media-dev-allocator.c
 create mode 100644 include/media/media-dev-allocator.h

diff --git a/Documentation/media/kapi/mc-core.rst 
b/Documentation/media/kapi/mc-core.rst
index 1a738e5..4942cdd 100644
--- a/Documentation/media/kapi/mc-core.rst
+++ b/Documentation/media/kapi/mc-core.rst
@@ -257,8 +257,45 @@ Subsystems should facilitate link validation by providing 
subsystem specific
 helper functions to provide easy access for commonly needed information, and
 in the end provide a way to use driver-specific callbacks.
 
+Media Controller Device Allocator API
+^
+
+When media device belongs to more than one driver, the shared media device
+is allocated with the shared struct device as the key for look ups.
+
+Shared media device should stay in registered state until the last driver
+unregisters it. In addition, media device should be released when all the
+references are released. Each driver gets a reference to the media device
+during probe, when it allocates the media device. If media device is already
+allocated, allocate API bumps up the refcount and return the existing media
+device. Driver puts the reference back from its disconnect routine when it
+calls :c:func:`media_device_delete()`.
+
+Media device is unregistered and cleaned up from the kref put handler to
+ensure that the media device stays in registered state until the last driver
+unregisters the media device.
+
+**Driver Usage**
+
+Drivers should use the media-core routines to get register reference and
+call :c:func:`media_device_delete()` routine to make sure the shared media
+device delete is handled correctly.
+
+**driver probe:**
+Call :c:func:`media_device_usb_allocate()` to allocate or get a reference
+Call :c:func:`media_device_register()`, if media devnode isn't registered
+
+**driver disconnect:**
+Call :c:func:`media_device_delete()` to free the media_device. Free'ing is
+handled by the kref put handler.
+
+API Definitions
+^^^
+
 .. kernel-doc:: include/media/media-device.h
 
 .. kernel-doc:: include/media/media-devnode.h
 
 .. kernel-doc:: include/media/media-entity.h
+
+.. kernel-doc:: include/media/media-dev-allocator.h
diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index 0deaa93..7c0701d 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -6,7 +6,8 @@ ifeq ($(CONFIG_MEDIA_CEC_EDID),y)
   obj-$(CONFIG_MEDIA_SUPPORT) += cec-edid.o
 endif
 
-media-objs := media-device.o media-devnode.o media-entity.o
+media-objs := media-device.o media-devnode.o media-entity.o \
+  media-dev-allocator.o
 
 #
 # I2C drivers should come before other drivers, otherwise they'll fail
diff --git a/drivers/media/media-dev-allocator.c 
b/drivers/media/media-dev-allocator.c
new file mode 100644
index 000..0ecf152
--- /dev/null
+++ b/drivers/media/media-dev-allocator.c
@@ -0,0 +1,133 @@
+/*
+ * media-dev-allocator.c - Media Controller Device Allocator API
+ *
+ * Copyright (c) 2016 Shuah Khan 
+ * Copyright (c) 2016 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ * Credits: Suggested by Laurent Pinchart 
+ */
+
+/*
+ * This file adds a global refcounted Media Controller Device Instance API.
+ * A system wide global media device list is managed and each media device
+ * includes a kref count. The last put on the media device releases the media
+ * device instance.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+static LIST_HEAD(media_device_list);
+static DEFINE_MUTEX(media_device_lock);
+
+struct media_device_instance {
+   struct media_device mdev;
+   struct module *owner;
+   struct list_head list;
+   struct kref refcount;
+};
+
+static inline struct media_device_instance *
+to_media_device_instance(struct media_device *mdev)
+{
+   return container_of(mdev, struct media_device_instance, mdev);
+}
+
+static void media_device_instance_release(struct kref *kref)
+{
+   struct media_device_instance *mdi =

[PATCH v6 2/3] media: change au0828 to use Media Device Allocator API

[Shuah Khan]

Change au0828 to use Media Device Allocator API to allocate media device
with the parent usb struct device as the key, so it can be shared with the
snd_usb_audio driver.

Signed-off-by: Shuah Khan 
---
Changes to patch 0002:
- No changes since patch v2, applies cleanly on top of the following:
media: Protect enable_source and disable_source handler code paths
https://lkml.org/lkml/2016/11/29/1001

 drivers/media/usb/au0828/au0828-core.c | 12 
 drivers/media/usb/au0828/au0828.h  |  1 +
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c 
b/drivers/media/usb/au0828/au0828-core.c
index bfd6482..ff1928e 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -159,9 +159,7 @@ static void au0828_unregister_media_device(struct 
au0828_dev *dev)
dev->media_dev->disable_source = NULL;
mutex_unlock(>graph_mutex);
 
-   media_device_unregister(dev->media_dev);
-   media_device_cleanup(dev->media_dev);
-   kfree(dev->media_dev);
+   media_device_delete(dev->media_dev, KBUILD_MODNAME);
dev->media_dev = NULL;
 #endif
 }
@@ -214,14 +212,10 @@ static int au0828_media_device_init(struct au0828_dev 
*dev,
 #ifdef CONFIG_MEDIA_CONTROLLER
struct media_device *mdev;
 
-   mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+   mdev = media_device_usb_allocate(udev, KBUILD_MODNAME);
if (!mdev)
return -ENOMEM;
 
-   /* check if media device is already initialized */
-   if (!mdev->dev)
-   media_device_usb_init(mdev, udev, udev->product);
-
dev->media_dev = mdev;
 #endif
return 0;
@@ -482,6 +476,8 @@ static int au0828_media_device_register(struct au0828_dev 
*dev,
/* register media device */
ret = media_device_register(dev->media_dev);
if (ret) {
+   media_device_delete(dev->media_dev, KBUILD_MODNAME);
+   dev->media_dev = NULL;
dev_err(>dev,
"Media Device Register Error: %d\n", ret);
return ret;
diff --git a/drivers/media/usb/au0828/au0828.h 
b/drivers/media/usb/au0828/au0828.h
index dd7b378..4bf1b0c 100644
--- a/drivers/media/usb/au0828/au0828.h
+++ b/drivers/media/usb/au0828/au0828.h
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* DVB */
 #include "demux.h"
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 3/3] sound/usb: Use Media Controller API to share media resources

[Shuah Khan]

Change ALSA driver to use Media Controller API to share media resources
with DVB, and V4L2 drivers on a AU0828 media device.

Media Controller specific initialization is done after sound card is
registered. ALSA creates Media interface and entity function graph
nodes for Control, Mixer, PCM Playback, and PCM Capture devices.

snd_usb_hw_params() will call Media Controller enable source handler
interface to request the media resource. If resource request is granted,
it will release it from snd_usb_hw_free(). If resource is busy, -EBUSY is
returned.

Media specific cleanup is done in usb_audio_disconnect().

Signed-off-by: Shuah Khan 
---
Changes to patch 0003:
- Changed to hold graph_mutex to check and call enable_source and
  disable_source handlers - to match au0828 doing the same in:
media: Protect enable_source and disable_source handler code paths 
https://lkml.org/lkml/2016/11/29/1001

 sound/usb/Kconfig|   4 +
 sound/usb/Makefile   |   2 +
 sound/usb/card.c |  14 ++
 sound/usb/card.h |   3 +
 sound/usb/media.c| 326 +++
 sound/usb/media.h|  73 +++
 sound/usb/mixer.h|   3 +
 sound/usb/pcm.c  |  28 +++-
 sound/usb/quirks-table.h |   1 +
 sound/usb/stream.c   |   2 +
 sound/usb/usbaudio.h |   6 +
 11 files changed, 457 insertions(+), 5 deletions(-)
 create mode 100644 sound/usb/media.c
 create mode 100644 sound/usb/media.h

diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig
index a452ad7..d14bf41 100644
--- a/sound/usb/Kconfig
+++ b/sound/usb/Kconfig
@@ -15,6 +15,7 @@ config SND_USB_AUDIO
select SND_RAWMIDI
select SND_PCM
select BITREVERSE
+   select SND_USB_AUDIO_USE_MEDIA_CONTROLLER if MEDIA_CONTROLLER && 
(MEDIA_SUPPORT=y || MEDIA_SUPPORT=SND_USB_AUDIO)
help
  Say Y here to include support for USB audio and USB MIDI
  devices.
@@ -22,6 +23,9 @@ config SND_USB_AUDIO
  To compile this driver as a module, choose M here: the module
  will be called snd-usb-audio.
 
+config SND_USB_AUDIO_USE_MEDIA_CONTROLLER
+   bool
+
 config SND_USB_UA101
tristate "Edirol UA-101/UA-1000 driver"
select SND_PCM
diff --git a/sound/usb/Makefile b/sound/usb/Makefile
index 2d2d122..8dca3c4 100644
--- a/sound/usb/Makefile
+++ b/sound/usb/Makefile
@@ -15,6 +15,8 @@ snd-usb-audio-objs := card.o \
quirks.o \
stream.o
 
+snd-usb-audio-$(CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER) += media.o
+
 snd-usbmidi-lib-objs := midi.o
 
 # Toplevel Module Dependency
diff --git a/sound/usb/card.c b/sound/usb/card.c
index 2ddc034..570ff00 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -66,6 +66,7 @@
 #include "format.h"
 #include "power.h"
 #include "stream.h"
+#include "media.h"
 
 MODULE_AUTHOR("Takashi Iwai ");
 MODULE_DESCRIPTION("USB Audio");
@@ -616,6 +617,11 @@ static int usb_audio_probe(struct usb_interface *intf,
if (err < 0)
goto __error;
 
+   if (quirk && quirk->media_device) {
+   /* don't want to fail when media_snd_device_create() fails */
+   media_snd_device_create(chip, intf);
+   }
+
usb_chip[chip->index] = chip;
chip->num_interfaces++;
usb_set_intfdata(intf, chip);
@@ -672,6 +678,14 @@ static void usb_audio_disconnect(struct usb_interface 
*intf)
list_for_each(p, >midi_list) {
snd_usbmidi_disconnect(p);
}
+   /*
+* Nice to check quirk && quirk->media_device and
+* then call media_snd_device_delete(). Don't have
+* access to quirk here. media_snd_device_delete()
+* acceses mixer_list
+*/
+   media_snd_device_delete(chip);
+
/* release mixer resources */
list_for_each_entry(mixer, >mixer_list, list) {
snd_usb_mixer_disconnect(mixer);
diff --git a/sound/usb/card.h b/sound/usb/card.h
index 111b0f0..6ef8e88 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -105,6 +105,8 @@ struct snd_usb_endpoint {
struct list_head list;
 };
 
+struct media_ctl;
+
 struct snd_usb_substream {
struct snd_usb_stream *stream;
struct usb_device *dev;
@@ -156,6 +158,7 @@ struct snd_usb_substream {
} dsd_dop;
 
bool trigger_tstamp_pending_update; /* trigger timestamp being updated 
from initial estimate */
+   struct media_ctl *media_ctl;
 };
 
 struct snd_usb_stream {
diff --git a/sound/usb/media.c b/sound/usb/media.c
new file mode 100644
index 000..40c24d3f
--- /dev/null
+++ b/sound/usb/media.c
@@ -0,0 +1,326 @@
+/*
+ * media.c - Media Controller specific ALSA driver code
+ *
+ * Copyright (c) 2016 Shuah Khan 
+ * Copyright (c) 2016 Samsung Electronics Co., Ltd.
+ *
+ * This 

[PATCH v6 0/3] Media Device Allocator API

[Shuah Khan]

Media Device Allocator API to allows multiple drivers share a media device.
Using this API, drivers can allocate a media device with the shared struct
device as the key. Once the media device is allocated by a driver, other
drivers can get a reference to it. The media device is released when all
the references are released.

Patches 0001 and 0002 are rebased to 4.9-rc7. Patch 0003 for snd-usb-audio
is a rebase of the patch that was tested with the original Media Device
Allocator patch series.

snd-usb-audio patch includes the fixes found during 4.7-rc1 time in the
original snd-usb-audio patch.

Changes to patches in this series:
Changes to patch 0001 since v5: (comments from Mauro and Sakari)
- Removed struct device from media_device_instance. mdev.dev is used instead.
- Added documentation.

Changes to patch 0002:
- No changes since patch v2, applies cleanly on top of the following:
media: Protect enable_source and disable_source handler code paths
https://lkml.org/lkml/2016/11/29/1001
 
Changes to patch 0003:
- Changed to hold graph_mutex to check and call enable_source and
  disable_source handlers - to match au0828 doing the same in:
media: Protect enable_source and disable_source handler code paths 
https://lkml.org/lkml/2016/11/29/1001

Changes to patch 0001 since v4:
- Addressed Sakari's review comments with the exception of
  opting to not introduce media_device_usb_allocate() macro,
  and to not add a new routine to find media device instance
  to avoid a one line check.

Changes to patch 0001 since v3:
- Fixed undefined reference to `__media_device_usb_init compile error when
  CONFIG_USB is disabled.
- Fixed kernel paging error when accessing /dev/mediaX after rmmod of the
  module that owns the media_device. The fix bumps the reference count for
  the owner when second driver comes along to share the media_device. If
  au0828 owns the media_device, then snd_usb_audio will bump the refcount
  for au0828, so it won't get deleted and vice versa.

Changes to patch 0002 since v2:
- Updated media_device_delete() to pass in module name.

Changes to patch 0003 since the last version in 4.7-rc1:
- Included fixes to bugs found during testing. 
- Updated to use the Media Allocator API.

This patch series has been tested with au0828 and snd-usb-audio drivers.
Ran bind and unbind loop tests on each driver with mc_nextgen_test and
media_device_test app loop tests while checking lsmod and dmesg.

Please refer to tools/testing/selftests/media_tests/regression_test.txt
for testing done on this series.

Shuah Khan (3):
  media: Media Device Allocator API
  media: change au0828 to use Media Device Allocator API
  sound/usb: Use Media Controller API to share media resources

 Documentation/media/kapi/mc-core.rst   |  37 
 drivers/media/Makefile |   3 +-
 drivers/media/media-dev-allocator.c| 133 ++
 drivers/media/usb/au0828/au0828-core.c |  12 +-
 drivers/media/usb/au0828/au0828.h  |   1 +
 include/media/media-dev-allocator.h|  54 ++
 sound/usb/Kconfig  |   4 +
 sound/usb/Makefile |   2 +
 sound/usb/card.c   |  14 ++
 sound/usb/card.h   |   3 +
 sound/usb/media.c  | 326 +
 sound/usb/media.h  |  73 
 sound/usb/mixer.h  |   3 +
 sound/usb/pcm.c|  28 ++-
 sound/usb/quirks-table.h   |   1 +
 sound/usb/stream.c |   2 +
 sound/usb/usbaudio.h   |   6 +
 17 files changed, 688 insertions(+), 14 deletions(-)
 create mode 100644 drivers/media/media-dev-allocator.c
 create mode 100644 include/media/media-dev-allocator.h
 create mode 100644 sound/usb/media.c
 create mode 100644 sound/usb/media.h

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Sakari Ailus]

Hi Rob and Kevin,

On Tue, Nov 29, 2016 at 08:41:44AM -0600, Rob Herring wrote:
> On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman  wrote:
> > Hi Rob,
> >
> > Rob Herring  writes:
> >
> >> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
> >>> Signed-off-by: Kevin Hilman 
> >>> ---
> >>>  .../bindings/media/ti,da850-vpif-capture.txt   | 65 
> >>> ++
> >>>  .../devicetree/bindings/media/ti,da850-vpif.txt|  8 +++
> >>>  2 files changed, 73 insertions(+)
> >>>  create mode 100644 
> >>> Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >>>  create mode 100644 
> >>> Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> >>>
> >>> diff --git 
> >>> a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt 
> >>> b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >>> new file mode 100644
> >>> index ..c447ac482c1d
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >>> @@ -0,0 +1,65 @@
> >>> +Texas Instruments VPIF Capture
> >>> +--
> >>> +
> >>> +The TI Video Port InterFace (VPIF) capture component is the primary
> >>> +component for video capture on the DA850 family of TI DaVinci SoCs.
> >>> +
> >>> +TI Document number reference: SPRUH82C
> >>> +
> >>> +Required properties:
> >>> +- compatible: must be "ti,da850-vpif-capture"
> >>> +- reg: physical base address and length of the registers set for the 
> >>> device;
> >>> +- interrupts: should contain IRQ line for the VPIF
> >>> +
> >>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
> >>> +channels or a single 16-bit channel.  It should contain at least one
> >>> +port child node with child 'endpoint' node. Please refer to the
> >>> +bindings defined in
> >>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>> +
> >>> +Example using 2 8-bit input channels, one of which is connected to an
> >>> +I2C-connected TVP5147 decoder:
> >>> +
> >>> +vpif_capture: video-capture@0x00217000 {
> >>> +reg = <0x00217000 0x1000>;
> >>> +interrupts = <92>;
> >>> +
> >>> +port {
> >>> +vpif_ch0: endpoint@0 {
> >>> +  reg = <0>;
> >>> +  bus-width = <8>;
> >>> +  remote-endpoint = <>;
> >>> +};
> >>> +
> >>> +vpif_ch1: endpoint@1 {
> >>
> >> I think probably channels here should be ports rather than endpoints.
> >> AIUI, having multiple endpoints is for cases like a mux or 1 to many
> >> connections. There's only one data flow, but multiple sources or sinks.
> >
> > Looking at this closer... , I used an endpoint because it's bascially a
> > 16-bit parallel bus, that can be configured as (up to) 2 8-bit
> > "channels.  So, based on the video-interfaces.txt doc, I configured this
> > as a single port, with (up to) 2 endpoints.  That also allows me to
> > connect output of the decoder directly, using the remote-endpoint
> > property.
> >
> > So I guess I'm not fully understanding your suggestion.
> 
> NM, looks like video-interfaces.txt actually spells out this case and
> defines doing it as you did.

It's actually the first time I read that portion (at least so that I could
remember) of video-interfaces.txt. I'm not sure if anyone has implemented
that previously, nor how we ended up with the text. The list archive could
probably tell. Cc Guennadi who wrote it. :-) I couldn't immediately find DT
source with this arrangement.

In case of splitting the port into two parallel interfaces, how do you
determine which wires belong to which endpoint? I guess they'd be particular
sets of wires but as there's just a single port it isn't defined by the
port.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] [media] lmedm04: change some static variables to automatic

[Rasmus Villemoes]

ibuf and rbuf in lme2510_int_response are always assigned to before they
are read, and their addresses do not escape the function, so they have
no reason to be static.

Signed-off-by: Rasmus Villemoes 
---
 drivers/media/usb/dvb-usb-v2/lmedm04.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/dvb-usb-v2/lmedm04.c 
b/drivers/media/usb/dvb-usb-v2/lmedm04.c
index 7692701878ba..cd463f09ebc7 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -315,7 +315,7 @@ static void lme2510_int_response(struct urb *lme_urb)
 {
struct dvb_usb_adapter *adap = lme_urb->context;
struct lme2510_state *st = adap_to_priv(adap);
-   static u8 *ibuf, *rbuf;
+   u8 *ibuf, *rbuf;
int i = 0, offset;
u32 key;
u8 signal_lock = 0;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] [media] lmedm04: make some string arrays static

[Rasmus Villemoes]

It takes more .text to initialize these on the stack than they occupy
in .rodata, so just make them static const.

Signed-off-by: Rasmus Villemoes 
---
 drivers/media/usb/dvb-usb-v2/lmedm04.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/lmedm04.c 
b/drivers/media/usb/dvb-usb-v2/lmedm04.c
index cd463f09ebc7..bf5bc36a6ed9 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -1002,8 +1002,9 @@ static int lme_name(struct dvb_usb_adapter *adap)
struct dvb_usb_device *d = adap_to_d(adap);
struct lme2510_state *st = adap_to_priv(adap);
const char *desc = d->name;
-   char *fe_name[] = {"", " LG TDQY-P001F", " SHARP:BS2F7HZ7395",
-   " SHARP:BS2F7HZ0194", " RS2000"};
+   static const char * const fe_name[] = {
+   "", " LG TDQY-P001F", " SHARP:BS2F7HZ7395",
+   " SHARP:BS2F7HZ0194", " RS2000"};
char *name = adap->fe[0]->ops.info.name;
 
strlcpy(name, desc, 128);
@@ -1124,7 +1125,7 @@ static int dm04_lme2510_tuner(struct dvb_usb_adapter 
*adap)
 {
struct dvb_usb_device *d = adap_to_d(adap);
struct lme2510_state *st = adap_to_priv(adap);
-   char *tun_msg[] = {"", "TDA8263", "IX2505V", "DVB_PLL_OPERA", "RS2000"};
+   static const char * const tun_msg[] = {"", "TDA8263", "IX2505V", 
"DVB_PLL_OPERA", "RS2000"};
int ret = 0;
 
switch (st->tuner_config) {
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] [media] lmedm04: use %phN for hex dump

[Rasmus Villemoes]

Using the %ph printf extension for hex dumps like this makes the
generated code quite a bit smaller.

Signed-off-by: Rasmus Villemoes 
---
 drivers/media/usb/dvb-usb-v2/lmedm04.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/lmedm04.c 
b/drivers/media/usb/dvb-usb-v2/lmedm04.c
index 0e8fb89896c4..7692701878ba 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -99,9 +99,7 @@ static int dvb_usb_lme2510_debug;
 } while (0)
 #define deb_info(level, args...) lme_debug(dvb_usb_lme2510_debug, level, args)
 #define debug_data_snipet(level, name, p) \
-deb_info(level, name" (%02x%02x%02x%02x%02x%02x%02x%02x)", \
-   *p, *(p+1), *(p+2), *(p+3), *(p+4), \
-   *(p+5), *(p+6), *(p+7));
+deb_info(level, name" (%8phN)", p);
 #define info(args...) pr_info(DVB_USB_LOG_PREFIX": "args)
 
 module_param_named(debug, dvb_usb_lme2510_debug, int, 0644);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] [media] lmedm04: make lme2510_powerup a little smaller

[Rasmus Villemoes]

gcc isn't smart enough to realize it can share most of the argument
buildup and the actual function call between the two branches, so help
it a little.

Signed-off-by: Rasmus Villemoes 
---
 drivers/media/usb/dvb-usb-v2/lmedm04.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/lmedm04.c 
b/drivers/media/usb/dvb-usb-v2/lmedm04.c
index bf5bc36a6ed9..7462207f4fd7 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -1179,10 +1179,7 @@ static int lme2510_powerup(struct dvb_usb_device *d, int 
onoff)
 
mutex_lock(>i2c_mutex);
 
-   if (onoff)
-   ret = lme2510_usb_talk(d, lnb_on, len, rbuf, rlen);
-   else
-   ret = lme2510_usb_talk(d, lnb_off, len, rbuf, rlen);
+   ret = lme2510_usb_talk(d, onoff ? lnb_on : lnb_off, len, rbuf, rlen);
 
st->i2c_talk_onoff = 1;
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Enabling peer to peer device transactions for PCIe devices

[Deucher, Alexander]

> -Original Message-
> From: Haggai Eran [mailto:hagg...@mellanox.com]
> Sent: Wednesday, November 30, 2016 5:46 AM
> To: Jason Gunthorpe
> Cc: linux-ker...@vger.kernel.org; linux-r...@vger.kernel.org; linux-
> nvd...@ml01.01.org; Koenig, Christian; Suthikulpanit, Suravee; Bridgman,
> John; Deucher, Alexander; Linux-media@vger.kernel.org;
> dan.j.willi...@intel.com; log...@deltatee.com; dri-
> de...@lists.freedesktop.org; Max Gurtovoy; linux-...@vger.kernel.org;
> Sagalovitch, Serguei; Blinzer, Paul; Kuehling, Felix; Sander, Ben
> Subject: Re: Enabling peer to peer device transactions for PCIe devices
> 
> On 11/28/2016 9:02 PM, Jason Gunthorpe wrote:
> > On Mon, Nov 28, 2016 at 06:19:40PM +, Haggai Eran wrote:
>  GPU memory. We create a non-ODP MR pointing to VRAM but rely on
>  user-space and the GPU not to migrate it. If they do, the MR gets
>  destroyed immediately.
> >>> That sounds horrible. How can that possibly work? What if the MR is
> >>> being used when the GPU decides to migrate?
> >> Naturally this doesn't support migration. The GPU is expected to pin
> >> these pages as long as the MR lives. The MR invalidation is done only as
> >> a last resort to keep system correctness.
> >
> > That just forces applications to handle horrible unexpected
> > failures. If this sort of thing is needed for correctness then OOM
> > kill the offending process, don't corrupt its operation.
> Yes, that sounds fine. Can we simply kill the process from the GPU driver?
> Or do we need to extend the OOM killer to manage GPU pages?

Christian sent out an RFC patch a while back that extended the OOM to cover 
memory allocated for the GPU:
https://lists.freedesktop.org/archives/dri-devel/2015-September/089778.html

Alex

> 
> >
> >> I think it is similar to how non-ODP MRs rely on user-space today to
> >> keep them correct. If you do something like madvise(MADV_DONTNEED)
> on a
> >> non-ODP MR's pages, you can still get yourself into a data corruption
> >> situation (HCA sees one page and the process sees another for the same
> >> virtual address). The pinning that we use only guarentees the HCA's page
> >> won't be reused.
> >
> > That is not really data corruption - the data still goes where it was
> > originally destined. That is an application violating the
> > requirements of a MR.
> I guess it is a matter of terminology. If you compare it to the ODP case
> or the CPU case then you usually expect a single virtual address to map to
> a single physical page. Violating this cause some of your writes to be dropped
> which is a data corruption in my book, even if the application caused it.
> 
> > An application cannot munmap/mremap a VMA
> > while a non ODP MR points to it and then keep using the MR.
> Right. And it is perfectly fine to have some similar requirements from the
> application
> when doing peer to peer with a non-ODP MR.
> 
> > That is totally different from a GPU driver wanthing to mess with
> > translation to physical pages.
> >
> >>> From what I understand we are not really talking about kernel p2p,
> >>> everything proposed so far is being mediated by a userspace VMA, so
> >>> I'd focus on making that work.
> >
> >> Fair enough, although we will need both eventually, and I hope the
> >> infrastructure can be shared to some degree.
> >
> > What use case do you see for in kernel?
> Two cases I can think of are RDMA access to an NVMe device's controller
> memory buffer, and O_DIRECT operations that access GPU memory.
> Also, HMM's migration between two GPUs could use peer to peer in the
> kernel,
> although that is intended to be handled by the GPU driver if I understand
> correctly.
> 
> > Presumably in-kernel could use a vmap or something and the same basic
> > flow?
> I think we can achieve the kernel's needs with ZONE_DEVICE and DMA-API
> support
> for peer to peer. I'm not sure we need vmap. We need a way to have a
> scatterlist
> of MMIO pfns, and ZONE_DEVICE allows that.
> 
> Haggai
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Enabling peer to peer device transactions for PCIe devices

[Logan Gunthorpe]

On 30/11/16 09:23 AM, Jason Gunthorpe wrote:
>> Two cases I can think of are RDMA access to an NVMe device's controller
>> memory buffer,
> 
> I'm not sure on the use model there..

The NVMe fabrics stuff could probably make use of this. It's an
in-kernel system to allow remote access to an NVMe device over RDMA. So
they ought to be able to optimize their transfers by DMAing directly to
the NVMe's CMB -- no userspace interface would be required but there
would need some kernel infrastructure.

Logan
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Enabling peer to peer device transactions for PCIe devices

[Serguei Sagalovitch]

On 2016-11-30 11:23 AM, Jason Gunthorpe wrote:

Yes, that sounds fine. Can we simply kill the process from the GPU driver?
Or do we need to extend the OOM killer to manage GPU pages?

I don't know..
We could use send_sig_info to send signal from  kernel  to user space. 
So theoretically GPU driver

could issue KILL signal to some process.


On Wed, Nov 30, 2016 at 12:45:58PM +0200, Haggai Eran wrote:

I think we can achieve the kernel's needs with ZONE_DEVICE and DMA-API support
for peer to peer. I'm not sure we need vmap. We need a way to have a scatterlist
of MMIO pfns, and ZONE_DEVICE allows that.
I do not think that using DMA-API as it is is the best solution (at 
least in the current form):


-  It deals with handles/fd for the whole allocation but client 
could/will use sub-allocation as
well as theoretically possible to "merge" several allocations in one 
from GPU perspective.
-  It require knowledge to export but because "sharing" is controlled 
from user space it

means that we must "export" all allocation by default
- It deals with 'fd'/handles but user application may work with 
addresses/pointers.


Also current  DMA-API force each time to do all DMA table programming 
unrelated if
location was changed or not. With  vma / mmu  we are  able to install 
notifier to intercept
changes in location and update  translation tables only as needed (we do 
not need to keep

get_user_pages()  lock).
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Enabling peer to peer device transactions for PCIe devices

[Jason Gunthorpe]

On Wed, Nov 30, 2016 at 12:45:58PM +0200, Haggai Eran wrote:

> > That just forces applications to handle horrible unexpected
> > failures. If this sort of thing is needed for correctness then OOM
> > kill the offending process, don't corrupt its operation.

> Yes, that sounds fine. Can we simply kill the process from the GPU driver?
> Or do we need to extend the OOM killer to manage GPU pages?

I don't know..

> >>> From what I understand we are not really talking about kernel p2p,
> >>> everything proposed so far is being mediated by a userspace VMA, so
> >>> I'd focus on making that work.
> > 
> >> Fair enough, although we will need both eventually, and I hope the
> >> infrastructure can be shared to some degree.
> > 
> > What use case do you see for in kernel?

> Two cases I can think of are RDMA access to an NVMe device's controller
> memory buffer,

I'm not sure on the use model there..

> and O_DIRECT operations that access GPU memory.

This goes through user space so there is still a VMA..

> Also, HMM's migration between two GPUs could use peer to peer in the
> kernel, although that is intended to be handled by the GPU driver if
> I understand correctly.

Hum, presumably these migrations are VMA backed as well...

> > Presumably in-kernel could use a vmap or something and the same basic
> > flow?
> I think we can achieve the kernel's needs with ZONE_DEVICE and DMA-API support
> for peer to peer. I'm not sure we need vmap. We need a way to have a 
> scatterlist
> of MMIO pfns, and ZONE_DEVICE allows that.

Well, if there is no virtual map then we are back to how do you do
migrations and other things people seem to want to do on these
pages. Maybe the loose 'struct page' flow is not for those users.

But I think if you want kGPU or similar then you probably need vmaps
or something similar to represent the GPU pages in kernel memory.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2.2 1/2] smiapp: Implement power-on and power-off sequences without runtime PM

[Sakari Ailus]

Power on the sensor when the module is loaded and power it off when it is
removed.

Signed-off-by: Sakari Ailus 
---
since v2.1:

- Power off conditionally, i.e. only if the device was powered.
  pm_runtime_status_suspended() returns false if CONFIG_PM is not set.

 drivers/media/i2c/smiapp/smiapp-core.c | 29 ++---
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
b/drivers/media/i2c/smiapp/smiapp-core.c
index 59872b3..620f8ce 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops 
smiapp_internal_ops = {
  * I2C Driver
  */
 
-#ifdef CONFIG_PM
-
 static int smiapp_suspend(struct device *dev)
 {
struct i2c_client *client = to_i2c_client(dev);
@@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev)
return rval;
 }
 
-#else
-
-#define smiapp_suspend NULL
-#define smiapp_resume  NULL
-
-#endif /* CONFIG_PM */
-
 static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
 {
struct smiapp_hwconfig *hwcfg;
@@ -2913,13 +2904,9 @@ static int smiapp_probe(struct i2c_client *client,
if (IS_ERR(sensor->xshutdown))
return PTR_ERR(sensor->xshutdown);
 
-   pm_runtime_enable(>dev);
-
-   rval = pm_runtime_get_sync(>dev);
-   if (rval < 0) {
-   rval = -ENODEV;
-   goto out_power_off;
-   }
+   rval = smiapp_power_on(>dev);
+   if (rval < 0)
+   return rval;
 
rval = smiapp_identify_module(sensor);
if (rval) {
@@ -3100,6 +3087,9 @@ static int smiapp_probe(struct i2c_client *client,
if (rval < 0)
goto out_media_entity_cleanup;
 
+   pm_runtime_set_active(>dev);
+   pm_runtime_get_noresume(>dev);
+   pm_runtime_enable(>dev);
pm_runtime_set_autosuspend_delay(>dev, 1000);
pm_runtime_use_autosuspend(>dev);
pm_runtime_put_autosuspend(>dev);
@@ -3113,8 +3103,7 @@ static int smiapp_probe(struct i2c_client *client,
smiapp_cleanup(sensor);
 
 out_power_off:
-   pm_runtime_put(>dev);
-   pm_runtime_disable(>dev);
+   smiapp_power_off(>dev);
 
return rval;
 }
@@ -3127,8 +3116,10 @@ static int smiapp_remove(struct i2c_client *client)
 
v4l2_async_unregister_subdev(subdev);
 
-   pm_runtime_suspend(>dev);
pm_runtime_disable(>dev);
+   if (!pm_runtime_status_suspended(>dev))
+   smiapp_power_off(>dev);
+   pm_runtime_set_suspended(>dev);
 
for (i = 0; i < sensor->ssds_used; i++) {
v4l2_device_unregister_subdev(>ssds[i].sd);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Wed, Nov 30, 2016 at 03:53:03PM +0200, Laurent Pinchart wrote:
> But then you get the following patch (which, apart from being totally made 
> up, 
> probably shows what I've watched yesterday evening).
> 
> @@ ... @@
>   return -ENOMEM;
>   }
>  
> + ret = check_time_vortex();
> + if (ret < 0)
> + goto power_off_tardis;
> +
>   matt_smith = alloc_regeneration();
>   if (math_smith->status != OK) {
>   ret = -E_NEEDS_FISH_FINGERS_AND_CUSTARD;
> 


I don't get it.  Did we power on the tardis on the lines before?  That's
all the state that you need to keep in your head is just the most
recently allocated thing.

> >From that code only you can't tell whether the jump label is the right one. 
> >If 
> a single jump label is used with an unwinding code block that supports non-
> allocated resources, you don't have to ask yourself any question.
> 

You absolutely do have to ask that question, you just can't answer it
without jumping back and forth.  Doing everything at once is logically
more complicated than doing them one thing at a time, and empirically
just from looking at which code has the most bugs, then single exit
labels are the most buggy.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Dan,

On Wednesday 30 Nov 2016 15:33:26 Dan Carpenter wrote:
> On Mon, Nov 28, 2016 at 04:49:36PM +0200, Laurent Pinchart wrote:
> > On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> >> On Mon, 28 Nov 2016, Dan Carpenter wrote:
> >>> I understand the comparison, but I just think it's better if people
> >>> always keep track of what has been allocated and what has not.  I
> >>> tried so hard to get Markus to stop sending those hundreds of patches
> >>> where he's like "this function has a sanity check so we can pass
> >>> pointers that weren't allocated"...  It's garbage code.
> >>> 
> >>> But I understand that other people don't agree.
> >> 
> >> In my opinion, it is good for code understanding to only do what is
> >> useful to do.  It's not a hard and fast rule, but I think it is
> >> something to take into account.
> > 
> > On the other hand it complicates the error handling code by increasing the
> > number of goto labels, and it then becomes pretty easy when reworking code
> > to goto the wrong label. This is even more of an issue when the rework
> > doesn't touch the error handling code, in which case the reviewers can
> > easily miss the issue if they don't open the original source file to
> > check the goto labels.
>
> It's really not.  I've looked at a lot of error handling in the past
> five years and sent hundreds of fixes for error paths, more than any
> other kernel developer during that time.  Although it seems obvious in
> retrospect, it took me years to realize this but the canonical way of
> doing error handling is the least error prone.
> 
> Counting the labels is the wrong way to measure complexity.  That's like
> counting the number of functions.  Code with lots of functions is not
> necessarily more complicated than if it's just one big function.
> 
> Part of the key to unwinding correctly is using good label names.  It
> should say what the label does.  Some people use come-from labels names
> like "goto kmalloc_failed".  Those are totally useless.  It's like
> naming your functions "called_from_foo()".  If there is only one goto
> then come-from label names are useless and if there are more than one
> goto which go to the same label then it's useless *and* misleading.

Yes, label naming is (or at least should be) largely agreed upon, they should 
name the unwinding action, not the cause of the failure.

> Functions should be written so you can read them from top to bottom
> without scrolling back and forth.
> 
>   a = alloc();
>   if (!a)
>   return -ENOMEM;
> 
>   b = alloc();
>   if (!b) {
>   ret = -ENOMEM;
>   goto free_a;
>   }

But then you get the following patch (which, apart from being totally made up, 
probably shows what I've watched yesterday evening).

@@ ... @@
return -ENOMEM;
}
 
+   ret = check_time_vortex();
+   if (ret < 0)
+   goto power_off_tardis;
+
matt_smith = alloc_regeneration();
if (math_smith->status != OK) {
ret = -E_NEEDS_FISH_FINGERS_AND_CUSTARD;

>From that code only you can't tell whether the jump label is the right one. If 
a single jump label is used with an unwinding code block that supports non-
allocated resources, you don't have to ask yourself any question.

> That code tells a complete story without any scrolling.  It's future
> proof too.  You can tell the goto is correct just from the name.  But
> when it's:
> 
>   a = alloc();
>   if (!a)
>   goto out;
>   b = alloc();
>   goto out;
> 
> That code doesn't have enough information to be understandable on it's
> own.  It's way more bug prone than the first sample.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Mon, Nov 28, 2016 at 04:49:36PM +0200, Laurent Pinchart wrote:
> Hi Julia and Dan,
> 
> On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> > On Mon, 28 Nov 2016, Dan Carpenter wrote:
> > > I understand the comparison, but I just think it's better if people
> > > always keep track of what has been allocated and what has not.  I tried
> > > so hard to get Markus to stop sending those hundreds of patches where
> > > he's like "this function has a sanity check so we can pass pointers
> > > that weren't allocated"...  It's garbage code.
> > > 
> > > But I understand that other people don't agree.
> > 
> > In my opinion, it is good for code understanding to only do what is useful
> > to do.  It's not a hard and fast rule, but I think it is something to take
> > into account.
> 
> On the other hand it complicates the error handling code by increasing the 
> number of goto labels, and it then becomes pretty easy when reworking code to 
> goto the wrong label. This is even more of an issue when the rework doesn't 
> touch the error handling code, in which case the reviewers can easily miss 
> the 
> issue if they don't open the original source file to check the goto labels.
> 

It's really not.  I've looked at a lot of error handling in the past
five years and sent hundreds of fixes for error paths, more than any
other kernel developer during that time.  Although it seems obvious in
retrospect, it took me years to realize this but the canonical way of
doing error handling is the least error prone.

Counting the labels is the wrong way to measure complexity.  That's like
counting the number of functions.  Code with lots of functions is not
necessarily more complicated than if it's just one big function.

Part of the key to unwinding correctly is using good label names.  It
should say what the label does.  Some people use come-from labels names
like "goto kmalloc_failed".  Those are totally useless.  It's like
naming your functions "called_from_foo()".  If there is only one goto
then come-from label names are useless and if there are more than one
goto which go to the same label then it's useless *and* misleading.

Functions should be written so you can read them from top to bottom
without scrolling back and forth.

a = alloc();
if (!a)
return -ENOMEM;

b = alloc();
if (!b) {
ret = -ENOMEM;
goto free_a;
}

That code tells a complete story without any scrolling.  It's future
proof too.  You can tell the goto is correct just from the name.  But
when it's:

a = alloc();
if (!a)
goto out;
b = alloc();
goto out;

That code doesn't have enough information to be understandable on it's
own.  It's way more bug prone than the first sample.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [media] vivid: support for contiguous DMA buffers

[Hans Verkuil]

On 11/22/16 14:18, Vincent ABRIOU wrote:

Hi Hans,

Is there any issue so that those 2 patches cannot be merged?
[media] vivid: support for contiguous DMA buffer
[media] uvcvideo: support for contiguous DMA buffers


Lack of time, really. I'll see if I can take a look at these soonish.

Regards,

Hans



They both have same approach and have been tested against ARM and X86
platform.

Thanks.
BR
Vincent

On 09/12/2016 05:56 PM, Javier Martinez Canillas wrote:

Hello Vincent,

On Mon, Sep 12, 2016 at 4:47 AM, Vincent Abriou  wrote:

It allows to simulate the behavior of hardware with such limitations or
to connect vivid to real hardware with such limitations.

Add the "allocators" module parameter option to let vivid use the
dma-contig instead of vmalloc.

Signed-off-by: Philipp Zabel 
Signed-off-by: Hans Verkuil 
Signed-off-by: Vincent Abriou 

Cc: Philipp Zabel 
Cc: Hans Verkuil 
---


The patch looks good to me.

Reviewed-by: Javier Martinez Canillas 

I've also tested on an Exynos5 board to share DMA buffers between a
vivid capture device and the Exynos DRM driver, so:

Tested-by: Javier Martinez Canillas 

Before $SUBJECT, when vivid was always using the vb2 vmalloc memory
allocator, the Exynos DRM driver wasn't able to import the dma-buf
because the GEM buffers are non-contiguous:

$ gst-launch-1.0 v4l2src device=/dev/video7 io-mode=dmabuf ! kmssink
Setting pipeline to PAUSED ...
Pipeline is live and does not need PREROLL ...
Setting pipeline to PLAYING ...
New clock: GstSystemClock
0:00:00.853895814  29570xd6260 ERROR   kmsallocator
gstkmsallocator.c:334:gst_kms_allocator_add_fb:
Failed to bind to framebuffer: Invalid argument (-22)

[ 1757.390564] [drm:exynos_drm_framebuffer_init] *ERROR* cannot use
this gem memory type for fb.

The issue goes away when using the the vb2 DMA contig memory allocator.

Best regards,
Javier
N�r��y���b�X��ǧv�^�)޺{.n�+{���bj)���w*jg����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a�����G���h��j:+v���w�٥

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2.1 1/2] smiapp: Implement power-on and power-off sequences without runtime PM

[Sakari Ailus]

Hi Laurent,

On Tue, Nov 29, 2016 at 07:35:17PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Tuesday 29 Nov 2016 19:28:53 Sakari Ailus wrote:
> > Power on the sensor when the module is loaded and power it off when it is
> > removed.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> > Hi Alan and Laurent,
> > 
> > I hope this should be good then. I'm only enabling runtime PM at the end
> > of probe() when all is well, which reduces need for error handling.
> > 
> > Regards,
> > Sakari
> > 
> >  drivers/media/i2c/smiapp/smiapp-core.c | 28 +---
> >  1 file changed, 9 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> > b/drivers/media/i2c/smiapp/smiapp-core.c index 59872b3..683a3e0 100644
> > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > @@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops
> > smiapp_internal_ops = { * I2C Driver
> >   */
> > 
> > -#ifdef CONFIG_PM
> > -
> >  static int smiapp_suspend(struct device *dev)
> >  {
> > struct i2c_client *client = to_i2c_client(dev);
> > @@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev)
> > return rval;
> >  }
> > 
> > -#else
> > -
> > -#define smiapp_suspend NULL
> > -#define smiapp_resume  NULL
> > -
> > -#endif /* CONFIG_PM */
> > -
> >  static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
> >  {
> > struct smiapp_hwconfig *hwcfg;
> > @@ -2913,13 +2904,9 @@ static int smiapp_probe(struct i2c_client *client,
> > if (IS_ERR(sensor->xshutdown))
> > return PTR_ERR(sensor->xshutdown);
> > 
> > -   pm_runtime_enable(>dev);
> > -
> > -   rval = pm_runtime_get_sync(>dev);
> > -   if (rval < 0) {
> > -   rval = -ENODEV;
> > -   goto out_power_off;
> > -   }
> > +   rval = smiapp_power_on(>dev);
> > +   if (rval < 0)
> > +   return rval;
> > 
> > rval = smiapp_identify_module(sensor);
> > if (rval) {
> > @@ -3100,6 +3087,9 @@ static int smiapp_probe(struct i2c_client *client,
> > if (rval < 0)
> > goto out_media_entity_cleanup;
> > 
> > +   pm_runtime_set_active(>dev);
> > +   pm_runtime_get_noresume(>dev);
> > +   pm_runtime_enable(>dev);
> > pm_runtime_set_autosuspend_delay(>dev, 1000);
> > pm_runtime_use_autosuspend(>dev);
> > pm_runtime_put_autosuspend(>dev);
> 
> This looks better to me, although these 6 lines really call for a new helper 
> function.

They're somewhat unrelated, especially the autosuspend part. Few drivers use
it.

> 
> However, I still believe a helper that calls the runtime PM handlers directly 
> when CONFIG_PM=n and rely on runtime PM when CONFIG_PM=y would be the 
> cleanest 
> solution from a driver point of view.
> 
> > @@ -3113,8 +3103,7 @@ static int smiapp_probe(struct i2c_client *client,
> > smiapp_cleanup(sensor);
> > 
> >  out_power_off:
> > -   pm_runtime_put(>dev);
> > -   pm_runtime_disable(>dev);
> > +   smiapp_power_off(>dev);
> > 
> > return rval;
> >  }
> > @@ -3127,8 +3116,9 @@ static int smiapp_remove(struct i2c_client *client)
> > 
> > v4l2_async_unregister_subdev(subdev);
> > 
> > -   pm_runtime_suspend(>dev);
> > pm_runtime_disable(>dev);
> > +   pm_runtime_set_suspended(>dev);
> > +   smiapp_power_off(>dev);
> 
> The device could be powered off already.

Good point.

The autosuspend must be explicitly disabled as well which wasn't previously
done. I'll add another patch for that.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Enabling peer to peer device transactions for PCIe devices

[Haggai Eran]

On 11/28/2016 9:02 PM, Jason Gunthorpe wrote:
> On Mon, Nov 28, 2016 at 06:19:40PM +, Haggai Eran wrote:
 GPU memory. We create a non-ODP MR pointing to VRAM but rely on
 user-space and the GPU not to migrate it. If they do, the MR gets
 destroyed immediately.
>>> That sounds horrible. How can that possibly work? What if the MR is
>>> being used when the GPU decides to migrate? 
>> Naturally this doesn't support migration. The GPU is expected to pin
>> these pages as long as the MR lives. The MR invalidation is done only as
>> a last resort to keep system correctness.
> 
> That just forces applications to handle horrible unexpected
> failures. If this sort of thing is needed for correctness then OOM
> kill the offending process, don't corrupt its operation.
Yes, that sounds fine. Can we simply kill the process from the GPU driver?
Or do we need to extend the OOM killer to manage GPU pages?

> 
>> I think it is similar to how non-ODP MRs rely on user-space today to
>> keep them correct. If you do something like madvise(MADV_DONTNEED) on a
>> non-ODP MR's pages, you can still get yourself into a data corruption
>> situation (HCA sees one page and the process sees another for the same
>> virtual address). The pinning that we use only guarentees the HCA's page
>> won't be reused.
> 
> That is not really data corruption - the data still goes where it was
> originally destined. That is an application violating the
> requirements of a MR. 
I guess it is a matter of terminology. If you compare it to the ODP case 
or the CPU case then you usually expect a single virtual address to map to
a single physical page. Violating this cause some of your writes to be dropped
which is a data corruption in my book, even if the application caused it.

> An application cannot munmap/mremap a VMA
> while a non ODP MR points to it and then keep using the MR.
Right. And it is perfectly fine to have some similar requirements from the 
application
when doing peer to peer with a non-ODP MR. 

> That is totally different from a GPU driver wanthing to mess with
> translation to physical pages.
> 
>>> From what I understand we are not really talking about kernel p2p,
>>> everything proposed so far is being mediated by a userspace VMA, so
>>> I'd focus on making that work.
> 
>> Fair enough, although we will need both eventually, and I hope the
>> infrastructure can be shared to some degree.
> 
> What use case do you see for in kernel?
Two cases I can think of are RDMA access to an NVMe device's controller 
memory buffer, and O_DIRECT operations that access GPU memory.
Also, HMM's migration between two GPUs could use peer to peer in the kernel,
although that is intended to be handled by the GPU driver if I understand
correctly.

> Presumably in-kernel could use a vmap or something and the same basic
> flow?
I think we can achieve the kernel's needs with ZONE_DEVICE and DMA-API support
for peer to peer. I'm not sure we need vmap. We need a way to have a scatterlist
of MMIO pfns, and ZONE_DEVICE allows that.

Haggai
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] adv7604: Initialize drive strength to default when using DT

[Niklas Söderlund]

Hi Lars,

On 2016-11-29 12:23:48 +0100, Lars-Peter Clausen wrote:
> The adv7604 driver platform data contains fields for configuring the drive
> strength of the output pins. When probing the driver through DT these
> fields are not explicitly initialized, which means they are left at 0. This
> is a reserved setting for the drive strength configuration though and can
> cause signal integrity issues.
> 
> Whether these signal integrity issues are visible depends on the PCB
> specifics (e.g. the higher the load capacitance for the output the more
> visible the issue). But it has been observed on existing solutions at high
> pixel clock rates.
> 
> Initialize the drive strength settings to the power-on-reset value of the
> device when probing through devicetree to avoid this issue.
> 
> Fixes: 0e158be0162b ("adv7604: Add DT support")
> Signed-off-by: Lars-Peter Clausen 

Tested on Renesas Koelsch board which uses a adv7612 for HDMI input.

Tested-by: Niklas Söderlund 

> ---
>  drivers/media/i2c/adv7604.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 5630eb2..a4dc64a 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -3132,6 +3132,9 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
>   state->pdata.blank_data = 1;
>   state->pdata.op_format_mode_sel = ADV7604_OP_FORMAT_MODE0;
>   state->pdata.bus_order = ADV7604_BUS_ORDER_RGB;
> + state->pdata.dr_str_data = ADV76XX_DR_STR_MEDIUM_HIGH;
> + state->pdata.dr_str_clk = ADV76XX_DR_STR_MEDIUM_HIGH;
> + state->pdata.dr_str_sync = ADV76XX_DR_STR_MEDIUM_HIGH;
>  
>   return 0;
>  }
> -- 
> 2.1.4
> 

-- 
Regards,
Niklas Söderlund
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ir-keytable: infinite loops, segfaults

[Vincent McIntyre]

On Sun, Nov 27, 2016 at 07:35:10PM +, Sean Young wrote:
>  
> > I wanted to mention that the IR protocol is still showing as unknown.
> > Is there anything that can be done to sort that out?
> 
> It would be nice if that could be sorted out, although that would be 
> a separate patch.
> 
> So all we know right now is what scancode the IR receiver hardware
> produces but we have no idea what IR protocol is being used. In order to
> figure this out we need a recording of the IR the remote sends, for which
> a different IR receiver is needed. Neither your imon nor your 
> dvb_usb_af9035 can do this, something like a mce usb IR receiver would
> be best. Do you have access to one? One with an IR emitter would be
> best.
> 
> So with that we can have a recording of the IR the remote sends, and
> with the emitter we can see which IR protocols the IR receiver 
> understands.

Haven't been able to find anything suitable. I would order something
but I won't be able to follow up for several weeks.
I'll ask on the myth list to see if anyone is up for trying this.

Thanks again for your help with this
Vince
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] [media] davinci: vpif_capture: don't lock over s_stream

[Laurent Pinchart]

Hi Kevin,

Thank you for the patch.

On Tuesday 29 Nov 2016 15:57:09 Kevin Hilman wrote:
> Video capture subdevs may be over I2C and may sleep during xfer, so we
> cannot do IRQ-disabled locking when calling the subdev.
> 
> Signed-off-by: Kevin Hilman 
> ---
>  drivers/media/platform/davinci/vpif_capture.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/davinci/vpif_capture.c
> b/drivers/media/platform/davinci/vpif_capture.c index
> 5104cc0ee40e..9f8f41c0f251 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -193,7 +193,10 @@ static int vpif_start_streaming(struct vb2_queue *vq,
> unsigned int count) }
>   }
> 
> + spin_unlock_irqrestore(>irqlock, flags);
>   ret = v4l2_subdev_call(ch->sd, video, s_stream, 1);
> + spin_lock_irqsave(>irqlock, flags);

I always get anxious when I see a spinlock being released randomly with an 
operation in the middle of a protected section. Looking at the code it looks 
like the spinlock is abused here. irqlock should only protect the dma_queue 
and should thus only be taken around the following code:

spin_lock_irqsave(>irqlock, flags);
/* Get the next frame from the buffer queue */
common->cur_frm = common->next_frm = list_entry(common->dma_queue.next,
struct vpif_cap_buffer, list);
/* Remove buffer from the buffer queue */
list_del(>cur_frm->list);
spin_unlock_irqrestore(>irqlock, flags);

The code that is currently protected by the lock in the start and stop 
streaming functions should be protected by a mutex instead.

> +
>   if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) {
>   vpif_dbg(1, debug, "stream on failed in subdev\n");
>   goto err;

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html