cron job: media_tree daily build: WARNINGS

[Hans Verkuil]

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

Results of the daily build of media_tree:

date:   Sat Dec 17 05:00:19 CET 2016
media-tree git hash:d183e4efcae8d88a2f252e546978658ca6d273cc
media_build git hash:   1606032398b1d79149c1507be2029e1a00d8dff0
v4l-utils git hash: 1252d0de6a4ae3a7c8c55765e82c988be52cc729
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-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-x86_64: OK
apps: WARNINGS
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

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

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Saturday.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 2/2] [media] em28xx: support for Hauppauge WinTV-dualHD 01595 ATSC/QAM

[Kevin Cheng]

Hauppauge WinTV-dualHD model 01595 is a USB 2.0 dual ATSC/QAM tuner with
the following components:

USB bridge: Empia em28274
Demodulator: 2x LG LGDT3306a at addresses 0xb2 and 0x1c
Tuner: 2x Silicon Labs si2157 at addresses 0xc0 and 0xc4

This patch enables only the first tuner.

Signed-off-by: Kevin Cheng 
---
 drivers/media/usb/em28xx/em28xx-cards.c | 19 
 drivers/media/usb/em28xx/em28xx-dvb.c   | 78 +
 drivers/media/usb/em28xx/em28xx.h   |  1 +
 3 files changed, 98 insertions(+)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c 
b/drivers/media/usb/em28xx/em28xx-cards.c
index 23c6749..5f90d08 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -509,6 +509,7 @@ static struct em28xx_reg_seq plex_px_bcud[] = {
 
 /*
  * 2040:0265 Hauppauge WinTV-dualHD DVB
+ * 2040:026d Hauppauge WinTV-dualHD ATSC/QAM
  * reg 0x80/0x84:
  * GPIO_0: Yellow LED tuner 1, 0=on, 1=off
  * GPIO_1: Green LED tuner 1, 0=on, 1=off
@@ -2389,6 +2390,21 @@ struct em28xx_board em28xx_boards[] = {
.ir_codes  = RC_MAP_HAUPPAUGE,
.leds  = hauppauge_dualhd_leds,
},
+   /*
+* 2040:026d Hauppauge WinTV-dualHD (model 01595 - ATSC/QAM).
+* Empia EM28274, 2x LG LGDT3306A, 2x Silicon Labs Si2157
+*/
+   [EM28174_BOARD_HAUPPAUGE_WINTV_DUALHD_01595] = {
+   .name  = "Hauppauge WinTV-dualHD 01595 ATSC/QAM",
+   .def_i2c_bus   = 1,
+   .i2c_speed = EM28XX_I2C_CLK_WAIT_ENABLE |
+EM28XX_I2C_FREQ_400_KHZ,
+   .tuner_type= TUNER_ABSENT,
+   .tuner_gpio= hauppauge_dualhd_dvb,
+   .has_dvb   = 1,
+   .ir_codes  = RC_MAP_HAUPPAUGE,
+   .leds  = hauppauge_dualhd_leds,
+   },
 };
 EXPORT_SYMBOL_GPL(em28xx_boards);
 
@@ -2514,6 +2530,8 @@ struct usb_device_id em28xx_id_table[] = {
.driver_info = EM2883_BOARD_HAUPPAUGE_WINTV_HVR_850 },
{ USB_DEVICE(0x2040, 0x0265),
.driver_info = EM28174_BOARD_HAUPPAUGE_WINTV_DUALHD_DVB 
},
+   { USB_DEVICE(0x2040, 0x026d),
+   .driver_info = 
EM28174_BOARD_HAUPPAUGE_WINTV_DUALHD_01595 },
{ USB_DEVICE(0x0438, 0xb002),
.driver_info = EM2880_BOARD_AMD_ATI_TV_WONDER_HD_600 },
{ USB_DEVICE(0x2001, 0xf112),
@@ -2945,6 +2963,7 @@ static void em28xx_card_setup(struct em28xx *dev)
case EM2883_BOARD_HAUPPAUGE_WINTV_HVR_950:
case EM2884_BOARD_HAUPPAUGE_WINTV_HVR_930C:
case EM28174_BOARD_HAUPPAUGE_WINTV_DUALHD_DVB:
+   case EM28174_BOARD_HAUPPAUGE_WINTV_DUALHD_01595:
{
struct tveeprom tv;
 
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c 
b/drivers/media/usb/em28xx/em28xx-dvb.c
index 75a75da..35c186e 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -37,6 +37,7 @@
 
 #include "lgdt330x.h"
 #include "lgdt3305.h"
+#include "lgdt3306a.h"
 #include "zl10353.h"
 #include "s5h1409.h"
 #include "mt2060.h"
@@ -920,6 +921,17 @@ static struct tda18271_config pinnacle_80e_dvb_config = {
.role= TDA18271_MASTER,
 };
 
+static struct lgdt3306a_config hauppauge_01595_lgdt3306a_config = {
+   .qam_if_khz = 4000,
+   .vsb_if_khz = 3250,
+   .spectral_inversion = 0,
+   .deny_i2c_rptr  = 0,
+   .mpeg_mode  = LGDT3306A_MPEG_SERIAL,
+   .tpclk_edge = LGDT3306A_TPCLK_RISING_EDGE,
+   .tpvalid_polarity   = LGDT3306A_TP_VALID_HIGH,
+   .xtalMHz= 25,
+};
+
 /* -- */
 
 static int em28xx_attach_xc3028(u8 addr, struct em28xx *dev)
@@ -1950,6 +1962,72 @@ static int em28xx_dvb_init(struct em28xx *dev)
 
}
break;
+   case EM28174_BOARD_HAUPPAUGE_WINTV_DUALHD_01595:
+   {
+   struct i2c_adapter *adapter;
+   struct i2c_client *client;
+   struct i2c_board_info info;
+   struct lgdt3306a_config lgdt3306a_config;
+   struct si2157_config si2157_config;
+
+   /* attach demod */
+   memcpy(_config,
+   _01595_lgdt3306a_config,
+   sizeof(struct lgdt3306a_config));
+   lgdt3306a_config.fe = >fe[0];
+   lgdt3306a_config.i2c_adapter = 
+   memset(, 0, sizeof(struct i2c_board_info));
+   strlcpy(info.type, "lgdt3306a", I2C_NAME_SIZE);
+   info.addr = 0x59;
+   info.platform_data = _config;
+   request_module(info.type);
+   

[PATCH 1/2] [media] lgdt3306a: support i2c mux for use by em28xx

[Kevin Cheng]

Adds an i2c mux to the lgdt3306a demodulator.  This was done to support
the Hauppauge WinTV-dualHD 01595 USB TV tuner (em28xx), which utilizes two
si2157 tuners behind gate control.

Signed-off-by: Kevin Cheng 
---
 drivers/media/dvb-frontends/lgdt3306a.c | 108 
 drivers/media/dvb-frontends/lgdt3306a.h |   4 ++
 2 files changed, 112 insertions(+)

diff --git a/drivers/media/dvb-frontends/lgdt3306a.c 
b/drivers/media/dvb-frontends/lgdt3306a.c
index 19dca46..b191e01 100644
--- a/drivers/media/dvb-frontends/lgdt3306a.c
+++ b/drivers/media/dvb-frontends/lgdt3306a.c
@@ -22,6 +22,7 @@
 #include 
 #include "dvb_math.h"
 #include "lgdt3306a.h"
+#include 
 
 
 static int debug;
@@ -65,6 +66,8 @@ struct lgdt3306a_state {
enum fe_modulation current_modulation;
u32 current_frequency;
u32 snr;
+
+   struct i2c_mux_core *muxc;
 };
 
 /*
@@ -2131,6 +2134,111 @@ static const struct dvb_frontend_ops lgdt3306a_ops = {
.search   = lgdt3306a_search,
 };
 
+static int lgdt3306a_select(struct i2c_mux_core *muxc, u32 chan)
+{
+   struct i2c_client *client = i2c_mux_priv(muxc);
+   struct lgdt3306a_state *state = i2c_get_clientdata(client);
+
+   return lgdt3306a_i2c_gate_ctrl(>frontend, 1);
+}
+
+static int lgdt3306a_deselect(struct i2c_mux_core *muxc, u32 chan)
+{
+   struct i2c_client *client = i2c_mux_priv(muxc);
+   struct lgdt3306a_state *state = i2c_get_clientdata(client);
+
+   return lgdt3306a_i2c_gate_ctrl(>frontend, 0);
+}
+
+static int lgdt3306a_probe(struct i2c_client *client,
+   const struct i2c_device_id *id)
+{
+   struct lgdt3306a_config *config = client->dev.platform_data;
+   struct lgdt3306a_state *state;
+   struct dvb_frontend *fe;
+   int ret;
+
+   config = kzalloc(sizeof(struct lgdt3306a_config), GFP_KERNEL);
+   if (config == NULL) {
+   ret = -ENOMEM;
+   goto fail;
+   }
+
+   memcpy(config, client->dev.platform_data,
+   sizeof(struct lgdt3306a_config));
+
+   config->i2c_addr = client->addr;
+   fe = lgdt3306a_attach(config, client->adapter);
+   if (fe == NULL) {
+   ret = -ENODEV;
+   goto err_fe;
+   }
+
+   i2c_set_clientdata(client, fe->demodulator_priv);
+   state = fe->demodulator_priv;
+
+   /* create mux i2c adapter for tuner */
+   state->muxc = i2c_mux_alloc(client->adapter, >dev,
+ 1, 0, I2C_MUX_LOCKED,
+ lgdt3306a_select, lgdt3306a_deselect);
+   if (!state->muxc) {
+   ret = -ENOMEM;
+   goto err_kfree;
+   }
+   state->muxc->priv = client;
+   ret = i2c_mux_add_adapter(state->muxc, 0, 0, 0);
+   if (ret)
+   goto err_kfree;
+
+   /* create dvb_frontend */
+   fe->ops.i2c_gate_ctrl = NULL;
+   *config->i2c_adapter = state->muxc->adapter[0];
+   *config->fe = fe;
+
+   return 0;
+
+err_kfree:
+   kfree(state);
+err_fe:
+   kfree(config);
+fail:
+   dev_dbg(>dev, "failed=%d\n", ret);
+   return ret;
+}
+
+static int lgdt3306a_remove(struct i2c_client *client)
+{
+   struct lgdt3306a_state *state = i2c_get_clientdata(client);
+
+   i2c_mux_del_adapters(state->muxc);
+
+   state->frontend.ops.release = NULL;
+   state->frontend.demodulator_priv = NULL;
+
+   kfree(state->cfg);
+   kfree(state);
+
+   return 0;
+}
+
+static const struct i2c_device_id lgdt3306a_id_table[] = {
+   {"lgdt3306a", 0},
+   {}
+};
+MODULE_DEVICE_TABLE(i2c, lgdt3306a_id_table);
+
+static struct i2c_driver lgdt3306a_driver = {
+   .driver = {
+   .name= "lgdt3306a",
+   .suppress_bind_attrs = true,
+   },
+   .probe  = lgdt3306a_probe,
+   .remove = lgdt3306a_remove,
+   .id_table   = lgdt3306a_id_table,
+};
+
+module_i2c_driver(lgdt3306a_driver);
+
 MODULE_DESCRIPTION("LG Electronics LGDT3306A ATSC/QAM-B Demodulator Driver");
 MODULE_AUTHOR("Fred Richter ");
 MODULE_LICENSE("GPL");
diff --git a/drivers/media/dvb-frontends/lgdt3306a.h 
b/drivers/media/dvb-frontends/lgdt3306a.h
index 9dbb2dc..6ce337e 100644
--- a/drivers/media/dvb-frontends/lgdt3306a.h
+++ b/drivers/media/dvb-frontends/lgdt3306a.h
@@ -56,6 +56,10 @@ struct lgdt3306a_config {
 
/* demod clock freq in MHz; 24 or 25 supported */
int  xtalMHz;
+
+   /* returned by driver if using i2c bus multiplexing */
+   struct dvb_frontend **fe;
+   struct i2c_adapter **i2c_adapter;
 };
 
 #if IS_REACHABLE(CONFIG_DVB_LGDT3306A)
-- 
2.9.3

--
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] solo6x10: use designated initializers

[Kees Cook]

Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.

Signed-off-by: Kees Cook 
---
 drivers/media/pci/solo6x10/solo6x10-g723.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-g723.c 
b/drivers/media/pci/solo6x10/solo6x10-g723.c
index 6a35107aca25..36e93540bb49 100644
--- a/drivers/media/pci/solo6x10/solo6x10-g723.c
+++ b/drivers/media/pci/solo6x10/solo6x10-g723.c
@@ -350,7 +350,7 @@ static int solo_snd_pcm_init(struct solo_dev *solo_dev)
 
 int solo_g723_init(struct solo_dev *solo_dev)
 {
-   static struct snd_device_ops ops = { NULL };
+   static struct snd_device_ops ops = { };
struct snd_card *card;
struct snd_kcontrol_new kctl;
char name[32];
-- 
2.7.4


-- 
Kees Cook
Nexus Security
--
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] mtk-vcodec: use designated initializers

[Kees Cook]

Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.

Signed-off-by: Kees Cook 
---
 drivers/media/platform/mtk-vcodec/venc/venc_h264_if.c | 8 
 drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c  | 8 
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/venc/venc_h264_if.c 
b/drivers/media/platform/mtk-vcodec/venc/venc_h264_if.c
index b76c80bdf30b..4eb3be37ba14 100644
--- a/drivers/media/platform/mtk-vcodec/venc/venc_h264_if.c
+++ b/drivers/media/platform/mtk-vcodec/venc/venc_h264_if.c
@@ -665,10 +665,10 @@ static int h264_enc_deinit(unsigned long handle)
 }
 
 static const struct venc_common_if venc_h264_if = {
-   h264_enc_init,
-   h264_enc_encode,
-   h264_enc_set_param,
-   h264_enc_deinit,
+   .init = h264_enc_init,
+   .encode = h264_enc_encode,
+   .set_param = h264_enc_set_param,
+   .deinit = h264_enc_deinit,
 };
 
 const struct venc_common_if *get_h264_enc_comm_if(void);
diff --git a/drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c 
b/drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c
index 544f57186243..a6fa145f2c54 100644
--- a/drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c
+++ b/drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c
@@ -470,10 +470,10 @@ static int vp8_enc_deinit(unsigned long handle)
 }
 
 static const struct venc_common_if venc_vp8_if = {
-   vp8_enc_init,
-   vp8_enc_encode,
-   vp8_enc_set_param,
-   vp8_enc_deinit,
+   .init = vp8_enc_init,
+   .encode = vp8_enc_encode,
+   .set_param = vp8_enc_set_param,
+   .deinit = vp8_enc_deinit,
 };
 
 const struct venc_common_if *get_vp8_enc_comm_if(void);
-- 
2.7.4


-- 
Kees Cook
Nexus Security
--
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: [RFC RESEND 11/11] vb2: dma-contig: Add WARN_ON_ONCE() to check for potential bugs

[Sakari Ailus]

On Thu, Dec 15, 2016 at 11:57:54PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday 11 Sep 2015 14:50:34 Sakari Ailus wrote:
> > The scatterlist should always be present when the cache would need to be
> > flushed. Each buffer type has its own means to provide that. Add
> > WARN_ON_ONCE() to check the scatterist exists.
> 
> Do you think such a check is really needed ? Have you run into this before ?

I think I may have, but the reason was that the code is non-trivial and
letting the user know what went wrong and where is nice. I guess this one
could be dropped, too.

> 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/videobuf2-dma-contig.c | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 65ee122..58c35c5
> > 100644
> > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > @@ -145,6 +145,9 @@ static void vb2_dc_prepare(void *buf_priv)
> > !dma_get_attr(DMA_ATTR_NON_CONSISTENT, buf->attrs))
> > return;
> > 
> > +   if (WARN_ON_ONCE(!sgt))
> > +   return;
> > +
> > dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> >  }
> > 
> > @@ -161,6 +164,9 @@ static void vb2_dc_finish(void *buf_priv)
> > !dma_get_attr(DMA_ATTR_NON_CONSISTENT, buf->attrs))
> > return;
> > 
> > +   if (WARN_ON_ONCE(!sgt))
> > +   return;
> > +
> > dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> >  }
> 

-- 
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: [PATCH v6 0/5] davinci: VPIF: add DT support

[Kevin Hilman]

Hans Verkuil  writes:

> On 07/12/16 19:30, Kevin Hilman wrote:
>> Prepare the groundwork for adding DT support for davinci VPIF drivers.
>> This series does some fixups/cleanups and then adds the DT binding and
>> DT compatible string matching for DT probing.
>>
>> The controversial part from previous versions around async subdev
>> parsing, and specifically hard-coding the input/output routing of
>> subdevs, has been left out of this series.  That part can be done as a
>> follow-on step after agreement has been reached on the path forward.
>> With this version, platforms can still use the VPIF capture/display
>> drivers, but must provide platform_data for the subdevs and subdev
>> routing.
>>
>> Tested video capture to memory on da850-lcdk board using composite
>> input.
>
> Other than the comment for the first patch this series looks good.
>
> So once that's addressed I'll queue it up for 4.11.

I've fixed that issue, and sent an update for just that patch in reply
to the original.

Thanks for the review,

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

[PATCH v6.1 1/5] [media] davinci: VPIF: fix module loading, init errors

[Kevin Hilman]

Fix problems with automatic module loading by adding MODULE_ALIAS.  Also
fix various load-time errors cause by incorrect or not present
platform_data.

Acked-by: Sakari Ailus 
Signed-off-by: Kevin Hilman 
---
Minor tweaks since v6
- added ack from Sakari
- droped an extraneous change for NULL subdev_info

 drivers/media/platform/davinci/vpif.c |  5 -
 drivers/media/platform/davinci/vpif_capture.c | 13 +
 drivers/media/platform/davinci/vpif_display.c |  6 ++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/davinci/vpif.c 
b/drivers/media/platform/davinci/vpif.c
index 0380cf2e5775..f50148dcba64 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -32,6 +32,9 @@
 MODULE_DESCRIPTION("TI DaVinci Video Port Interface driver");
 MODULE_LICENSE("GPL");
 
+#define VPIF_DRIVER_NAME   "vpif"
+MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
+
 #define VPIF_CH0_MAX_MODES 22
 #define VPIF_CH1_MAX_MODES 2
 #define VPIF_CH2_MAX_MODES 15
@@ -466,7 +469,7 @@ static const struct dev_pm_ops vpif_pm = {
 
 static struct platform_driver vpif_driver = {
.driver = {
-   .name   = "vpif",
+   .name   = VPIF_DRIVER_NAME,
.pm = vpif_pm_ops,
},
.remove = vpif_remove,
diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index 5104cc0ee40e..892a26f3c5b4 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -45,6 +45,7 @@ module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level 0-1");
 
 #define VPIF_DRIVER_NAME   "vpif_capture"
+MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
 
 /* global variables */
 static struct vpif_device vpif_obj = { {NULL} };
@@ -647,6 +648,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;
@@ -685,6 +690,9 @@ static int vpif_set_input(
if (sd_index >= 0) {
sd = vpif_obj.sd[sd_index];
subdev_info = _cfg->subdev_info[sd_index];
+   } else {
+   /* no subdevice, no input to setup */
+   return 0;
}
 
/* first setup input path from sub device to vpif */
@@ -1435,6 +1443,11 @@ static __init int vpif_probe(struct platform_device 
*pdev)
int res_idx = 0;
int i, err;
 
+   if (!pdev->dev.platform_data) {
+   dev_warn(>dev, "Missing platform data.  Giving up.\n");
+   return -EINVAL;
+   }
+
vpif_dev = >dev;
 
err = initialize_vpif();
diff --git a/drivers/media/platform/davinci/vpif_display.c 
b/drivers/media/platform/davinci/vpif_display.c
index 75b27233ec2f..7f632b757d32 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -42,6 +42,7 @@ module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level 0-1");
 
 #define VPIF_DRIVER_NAME   "vpif_display"
+MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
 
 /* Is set to 1 in case of SDTV formats, 2 in case of HDTV formats. */
 static int ycmux_mode;
@@ -1249,6 +1250,11 @@ static __init int vpif_probe(struct platform_device 
*pdev)
int res_idx = 0;
int i, err;
 
+   if (!pdev->dev.platform_data) {
+   dev_warn(>dev, "Missing platform data.  Giving up.\n");
+   return -EINVAL;
+   }
+
vpif_dev = >dev;
err = initialize_vpif();
 
-- 
2.9.3

--
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: [RFC RESEND 07/11] vb2: dma-contig: Remove redundant sgt_base field

[Sakari Ailus]

On Thu, Dec 15, 2016 at 11:08:37PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday 11 Sep 2015 14:50:30 Sakari Ailus wrote:
> > The struct vb2_dc_buf contains two struct sg_table fields: sgt_base and
> > dma_sgt. The former is used by DMA-BUF buffers whereas the latter is used
> > by USERPTR.
> > 
> > Unify the two, leaving dma_sgt.
> 
> Looks good to me. I initially thought this would prevent exporting a USERPTR 
> buffer through dmabuf, but that's forbidden by the videobuf2 core (and 
> rightly 
> so).
> 
> > MMAP buffers do not need cache flushing since they have been allocated
> > using dma_alloc_coherent().
> 
> I had understood this as meaning that the patch changes the behaviour for 
> MMAP 
> buffers. How about
> 
> "The prepare and finish implementation currently skip cache sync for MMAP 
> buffers and identify them based on dma_sgt being NULL. Now that dma_sgt is 
> used to export the MMAP buffers the condition must be updated."
> 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/videobuf2-dma-contig.c | 21 ++---
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 94c1e64..26a0a0f
> > 100644
> > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> > @@ -36,7 +36,6 @@ struct vb2_dc_buf {
> > /* MMAP related */
> > struct vb2_vmarea_handler   handler;
> > atomic_trefcount;
> > -   struct sg_table *sgt_base;
> > 
> > /* USERPTR related */
> > struct vm_area_struct   *vma;
> > @@ -117,7 +116,7 @@ static void vb2_dc_prepare(void *buf_priv)
> > struct sg_table *sgt = buf->dma_sgt;
> > 
> > /* DMABUF exporter will flush the cache for us */
> 
> How about updating the comment to
> 
>   /*
>* Skip cache sync for MMAP buffers (they don't need it) and DMABUF
>* buffers (the exporter will sync the cache for us).
>*/

Both are fine for me.

> 
> > -   if (!sgt || buf->db_attach)
> > +   if (!buf->vma)
> > return;
> > 
> > dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > @@ -129,7 +128,7 @@ static void vb2_dc_finish(void *buf_priv)
> > struct sg_table *sgt = buf->dma_sgt;
> > 
> > /* DMABUF exporter will flush the cache for us */
> > -   if (!sgt || buf->db_attach)
> > +   if (!buf->vma)
> > return;
> > 
> > dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> > @@ -146,9 +145,9 @@ static void vb2_dc_put(void *buf_priv)
> > if (!atomic_dec_and_test(>refcount))
> > return;
> > 
> > -   if (buf->sgt_base) {
> > -   sg_free_table(buf->sgt_base);
> > -   kfree(buf->sgt_base);
> > +   if (buf->dma_sgt) {
> > +   sg_free_table(buf->dma_sgt);
> > +   kfree(buf->dma_sgt);
> > }
> > dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
> > put_device(buf->dev);
> > @@ -252,13 +251,13 @@ static int vb2_dc_dmabuf_ops_attach(struct dma_buf
> > *dbuf, struct device *dev, /* Copy the buf->base_sgt scatter list to the
> > attachment, as we can't * map the same scatter list to multiple attachments
> > at the same time. */
> > -   ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
> > +   ret = sg_alloc_table(sgt, buf->dma_sgt->orig_nents, GFP_KERNEL);
> > if (ret) {
> > kfree(attach);
> > return -ENOMEM;
> > }
> > 
> > -   rd = buf->sgt_base->sgl;
> > +   rd = buf->dma_sgt->sgl;
> > wr = sgt->sgl;
> > for (i = 0; i < sgt->orig_nents; ++i) {
> > sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
> > @@ -409,10 +408,10 @@ static struct dma_buf *vb2_dc_get_dmabuf(void
> > *buf_priv, unsigned long flags) exp_info.flags = flags;
> > exp_info.priv = buf;
> > 
> > -   if (!buf->sgt_base)
> > -   buf->sgt_base = vb2_dc_get_base_sgt(buf);
> > +   if (!buf->dma_sgt)
> > +   buf->dma_sgt = vb2_dc_get_base_sgt(buf);
> > 
> > -   if (WARN_ON(!buf->sgt_base))
> > +   if (WARN_ON(!buf->dma_sgt))
> > return NULL;
> > 
> > dbuf = dma_buf_export(_info);
> 

-- 
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: [RFC RESEND 05/11] v4l2-core: Don't sync cache for a buffer if so requested

[Sakari Ailus]

Hi Laurent,

On Thu, Dec 15, 2016 at 10:37:20PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday 11 Sep 2015 14:50:28 Sakari Ailus wrote:
> > From: Samu Onkalo 
> > 
> > The user may request to the driver (vb2) to skip the cache maintenance
> > operations in case the buffer does not need cache synchronisation, e.g. in
> > cases where the buffer is passed between hardware blocks without it being
> > touched by the CPU.
> > 
> > Also document that the prepare and finish vb2_mem_ops might not get called
> > every time the buffer ownership changes between the kernel and the user
> > space.
> > 
> > Signed-off-by: Samu Onkalo 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c | 52 ++---
> >  include/media/videobuf2-core.h   | 12 +---
> >  2 files changed, 49 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> > b/drivers/media/v4l2-core/videobuf2-core.c index c5c0707a..b664024 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -187,6 +187,28 @@ static void __vb2_queue_cancel(struct vb2_queue *q);
> >  static void __enqueue_in_driver(struct vb2_buffer *vb);
> > 
> >  /**
> > + * __mem_prepare_planes() - call finish mem op for all planes of the buffer
> > + */
> > +static void __mem_prepare_planes(struct vb2_buffer *vb)
> > +{
> > +   unsigned int plane;
> > +
> > +   for (plane = 0; plane < vb->num_planes; ++plane)
> > +   call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
> > +}
> > +
> > +/**
> > + * __mem_finish_planes() - call finish mem op for all planes of the buffer
> > + */
> > +static void __mem_finish_planes(struct vb2_buffer *vb)
> > +{
> > +   unsigned int plane;
> > +
> > +   for (plane = 0; plane < vb->num_planes; ++plane)
> > +   call_void_memop(vb, finish, vb->planes[plane].mem_priv);
> > +}
> > +
> > +/**
> >   * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
> >   */
> >  static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> > @@ -1391,6 +1413,10 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb,
> > const struct v4l2_buffer *b static int __prepare_mmap(struct vb2_buffer
> > *vb, const struct v4l2_buffer *b) {
> > __fill_vb2_buffer(vb, b, vb->v4l2_planes);
> > +
> > +   if (!(b->flags & V4L2_BUF_FLAG_NO_CACHE_SYNC))
> > +   __mem_prepare_planes(vb);
> > +
> > return call_vb_qop(vb, buf_prepare, vb);
> >  }
> > 
> > @@ -1476,6 +1502,11 @@ static int __prepare_userptr(struct vb2_buffer *vb,
> > dprintk(1, "buffer initialization failed\n");
> > goto err;
> > }
> > +
> > +   /* This is new buffer memory --- always synchronise cache. */
> > +   __mem_prepare_planes(vb);
> > +   } else if (!(b->flags & V4L2_BUF_FLAG_NO_CACHE_SYNC)) {
> > +   __mem_prepare_planes(vb);
> > }
> > 
> > ret = call_vb_qop(vb, buf_prepare, vb);
> > @@ -1601,6 +1632,11 @@ static int __prepare_dmabuf(struct vb2_buffer *vb,
> > const struct v4l2_buffer *b) dprintk(1, "buffer initialization failed\n");
> > goto err;
> > }
> > +
> > +   /* This is new buffer memory --- always synchronise cache. */
> > +   __mem_prepare_planes(vb);
> > +   } else if (!(b->flags & V4L2_BUF_FLAG_NO_CACHE_SYNC)) {
> > +   __mem_prepare_planes(vb);
> > }
> > 
> > ret = call_vb_qop(vb, buf_prepare, vb);
> > @@ -1624,7 +1660,6 @@ err:
> >  static void __enqueue_in_driver(struct vb2_buffer *vb)
> >  {
> > struct vb2_queue *q = vb->vb2_queue;
> > -   unsigned int plane;
> > 
> > vb->state = VB2_BUF_STATE_ACTIVE;
> > atomic_inc(>owned_by_drv_count);
> > @@ -1691,10 +1726,6 @@ static int __buf_prepare(struct vb2_buffer *vb, const
> > struct v4l2_buffer *b) return ret;
> > }
> > 
> > -   /* sync buffers */
> > -   for (plane = 0; plane < vb->num_planes; ++plane)
> > -   call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
> > -
> > vb->state = VB2_BUF_STATE_PREPARED;
> > 
> > return 0;
> > @@ -2078,7 +2109,7 @@ EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
> >  /**
> >   * __vb2_dqbuf() - bring back the buffer to the DEQUEUED state
> >   */
> > -static void __vb2_dqbuf(struct vb2_buffer *vb)
> > +static void __vb2_dqbuf(struct vb2_buffer *vb, bool no_cache_sync)
> >  {
> > struct vb2_queue *q = vb->vb2_queue;
> > unsigned int plane;
> > @@ -2089,9 +2120,8 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
> > 
> > vb->state = VB2_BUF_STATE_DEQUEUED;
> > 
> > -   /* sync buffers */
> > -   for (plane = 0; plane < vb->num_planes; plane++)
> > -   call_void_memop(vb, finish, vb->planes[plane].mem_priv);
> > +   if (!no_cache_sync)
> > +   __mem_finish_planes(vb);
> > 
> > /* unmap DMABUF 

Re: [RFC RESEND 04/11] v4l: Unify cache management hint buffer flags

[Sakari Ailus]

Hi Laurent,

Thank you for the review.

On Thu, Dec 15, 2016 at 10:15:39PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday 11 Sep 2015 14:50:27 Sakari Ailus wrote:
> > The V4L2_BUF_FLAG_NO_CACHE_INVALIDATE and V4L2_BUF_FLAG_NO_CACHE_CLEAN
> > buffer flags are currently not used by the kernel. Replace the definitions
> > by a single V4L2_BUF_FLAG_NO_CACHE_SYNC flag to be used by further
> > patches.
> > 
> > Different cache architectures should not be visible to the user space
> > which can make no meaningful use of the differences anyway. In case a
> > device can make use of non-coherent memory accesses, the necessary cache
> > operations depend on the CPU architecture and the buffer type, not the
> > requests of the user. The cache operation itself may be skipped on the
> > user's request which was the purpose of the two flags.
> > 
> > On ARM the invalidate and clean are separate operations whereas on
> > x86(-64) the two are a single operation (flush). Whether the hardware uses
> > the buffer for reading (V4L2_BUF_TYPE_*_OUTPUT*) or writing
> > (V4L2_BUF_TYPE_*CAPTURE*) already defines the required cache operation
> > (clean and invalidate, respectively). No user input is required.
> 
> We need to perform the following operations.
> 
>   | QBUF  | DQBUF
> ---
> CAPTURE   | Invalidate| Invalidate (*)
> OUTPUT| Clean | -
> 
> (*) for systems using speculative pre-fetching only.
> 
> The following optimizations are possible:
> 
> 1. CAPTURE, the CPU has not written to the buffer before QBUF
> 
> Cache invalidation can be skipped at QBUF time, but becomes required at DQBUF 
> time on all systems, regardless of whether they use speculative prefetching.
> 
> 2. CAPTURE, the CPU will not read from the buffer after DQBUF
> 
> Cache invalidation can be skipped at DQBUF time.
> 
> 3. CAPTURE, combination of (1) and (2)
> 
> Cache invalidation can be skipped at both QBUF and DQBUF time.
> 
> 4. OUTPUT, the CPU has not written to the buffer before QBUF
> 
> Cache clean can be skipped at QBUF time.

Ack.

> 
> 
> A single flag can cover all cases, provided we keep track of the flag being 
> set at QBUF time to force cache invalidation at DQBUF time for case (1) if 
> the 
> flag isn't set at DQBUF time.
> 
> One issue is that cache invalidation at DQBUF time for CAPTURE buffers isn't 
> fully under the control of videobuf. We can instruct the DMA mapping API to 
> skip cache handling, but we can't ask it to force cache invalidation in the 
> sync_for_cpu operation for non speculative prefetching systems. On ARM32 the 
> implementation currently always invalidates the cache in 
> __dma_page_dev_to_cpu() for CAPTURE buffers so we're currently safe, but 
> there's a FIXME comment that might lead to someone fixing the implementation 
> in the future. I believe we'll have to fix this in the DMA mapping level, 
> userspace shouldn't be affected.
> 
> Feel free to capture (part of) this explanation in the commit message to 
> clarify your last paragraph.
> 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  Documentation/DocBook/media/v4l/io.xml | 25 +++--
> >  include/trace/events/v4l2.h|  3 +--
> >  include/uapi/linux/videodev2.h |  7 +--
> >  3 files changed, 17 insertions(+), 18 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/io.xml
> > b/Documentation/DocBook/media/v4l/io.xml index 7bbc2a4..4facd63 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -1112,21 +1112,18 @@ application. Drivers set or clear this flag when the
> > linkend="vidioc-qbuf">VIDIOC_DQBUF ioctl is called. 
> >   
> > -   
> V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
> > +   V4L2_BUF_FLAG_NO_CACHE_SYNC
> > 0x0800
> > -   Caches do not have to be invalidated for this buffer.
> > -Typically applications shall use this flag if the data captured in the
> > buffer -is not going to be touched by the CPU, instead the buffer will,
> > probably, be -passed on to a DMA-capable hardware unit for further
> > processing or output. -
> > - 
> > - 
> > -   V4L2_BUF_FLAG_NO_CACHE_CLEAN
> > -   0x1000
> > -   Caches do not have to be cleaned for this buffer.
> > -Typically applications shall use this flag for output buffers if the data
> > -in this buffer has not been created by the CPU but by some DMA-capable
> > unit, -in which case caches have not been used.
> > +   Do not perform CPU cache synchronisation operations
> > +   when the buffer is queued or dequeued. The user is
> > +   responsible for the correct use of this flag. It should be
> > +   only used when the buffer is not accessed using the CPU,
> > +   e.g. the buffer is written to by a hardware block and then
> > +   read by another one, in which case the flag should be 

Re: [PATCH v6 1/5] [media] davinci: VPIF: fix module loading, init errors

[Kevin Hilman]

Hans Verkuil  writes:

> On 07/12/16 19:30, Kevin Hilman wrote:
>> Fix problems with automatic module loading by adding MODULE_ALIAS.  Also
>> fix various load-time errors cause by incorrect or not present
>> platform_data.
>>
>> Signed-off-by: Kevin Hilman 
>> ---
>>  drivers/media/platform/davinci/vpif.c |  5 -
>>  drivers/media/platform/davinci/vpif_capture.c | 15 ++-
>>  drivers/media/platform/davinci/vpif_display.c |  6 ++
>>  3 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/davinci/vpif.c 
>> b/drivers/media/platform/davinci/vpif.c
>> index 0380cf2e5775..f50148dcba64 100644
>> --- a/drivers/media/platform/davinci/vpif.c
>> +++ b/drivers/media/platform/davinci/vpif.c
>> @@ -32,6 +32,9 @@
>>  MODULE_DESCRIPTION("TI DaVinci Video Port Interface driver");
>>  MODULE_LICENSE("GPL");
>>
>> +#define VPIF_DRIVER_NAME"vpif"
>> +MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
>> +
>>  #define VPIF_CH0_MAX_MODES  22
>>  #define VPIF_CH1_MAX_MODES  2
>>  #define VPIF_CH2_MAX_MODES  15
>> @@ -466,7 +469,7 @@ static const struct dev_pm_ops vpif_pm = {
>>
>>  static struct platform_driver vpif_driver = {
>>  .driver = {
>> -.name   = "vpif",
>> +.name   = VPIF_DRIVER_NAME,
>>  .pm = vpif_pm_ops,
>>  },
>>  .remove = vpif_remove,
>> diff --git a/drivers/media/platform/davinci/vpif_capture.c 
>> b/drivers/media/platform/davinci/vpif_capture.c
>> index 5104cc0ee40e..20c4344ed118 100644
>> --- a/drivers/media/platform/davinci/vpif_capture.c
>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>> @@ -45,6 +45,7 @@ module_param(debug, int, 0644);
>>  MODULE_PARM_DESC(debug, "Debug level 0-1");
>>
>>  #define VPIF_DRIVER_NAME"vpif_capture"
>> +MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
>>
>>  /* global variables */
>>  static struct vpif_device vpif_obj = { {NULL} };
>> @@ -647,6 +648,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;
>> @@ -654,7 +659,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))
>
> Why this change? subdev_info can never be NULL.

A debugging leftover I guess.  Will remove and resend.

Thanks for the review,

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: [TSN RFC v2 5/9] Add TSN header for the driver

[Richard Cochran]

On Fri, Dec 16, 2016 at 06:59:09PM +0100, hen...@austad.us wrote:
> +/*
> + * List of current subtype fields in the common header of AVTPDU
> + *
> + * Note: AVTPDU is a remnant of the standards from when it was AVB.
> + *
> + * The list has been updated with the recent values from IEEE 1722, draft 16.
> + */
> +enum avtp_subtype {
> + TSN_61883_IIDC = 0, /* IEC 61883/IIDC Format */
> + TSN_MMA_STREAM, /* MMA Streams */
> + TSN_AAF,/* AVTP Audio Format */
> + TSN_CVF,/* Compressed Video Format */
> + TSN_CRF,/* Clock Reference Format */
> + TSN_TSCF,   /* Time-Synchronous Control Format */
> + TSN_SVF,/* SDI Video Format */
> + TSN_RVF,/* Raw Video Format */
> + /* 0x08 - 0x6D reserved */
> + TSN_AEF_CONTINOUS = 0x6e, /* AES Encrypted Format Continous */
> + TSN_VSF_STREAM, /* Vendor Specific Format Stream */
> + /* 0x70 - 0x7e reserved */
> + TSN_EF_STREAM = 0x7f,   /* Experimental Format Stream */
> + /* 0x80 - 0x81 reserved */
> + TSN_NTSCF = 0x82,   /* Non Time-Synchronous Control Format */
> + /* 0x83 - 0xed reserved */
> + TSN_ESCF = 0xec,/* ECC Signed Control Format */
> + TSN_EECF,   /* ECC Encrypted Control Format */
> + TSN_AEF_DISCRETE,   /* AES Encrypted Format Discrete */
> + /* 0xef - 0xf9 reserved */
> + TSN_ADP = 0xfa, /* AVDECC Discovery Protocol */
> + TSN_AECP,   /* AVDECC Enumeration and Control Protocol */
> + TSN_ACMP,   /* AVDECC Connection Management Protocol */
> + /* 0xfd reserved */
> + TSN_MAAP = 0xfe,/* MAAP Protocol */
> + TSN_EF_CONTROL, /* Experimental Format Control */
> +};

The kernel shouldn't be in the business of assembling media packets.

Thanks,
Richard
--
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: Problem with uvcvideo timestamps

[Guennadi Liakhovetski]

Hi Niels,

Sorry for a late reply.

On Mon, 31 Oct 2016, Niels Möller wrote:

> Hi,
> 
> I'm tracking down a problem in Chrome, where video streams captured
> from a Logitech c930e camera get bogus timestamps. Chrome started
> using camera timestamps on linux a few months ago. I've noted commit
> 
>   
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5d0fd3c806b9e932010931ae67dbb482020e0882
> 
>   "[media] uvcvideo: Disable hardware timestamps by default"
> 
> but I'm running with a kernel which doesn't have that change.
> 
> First, let me say that for our purposes, the hairy syncing to the
> "SOF" clock done by uvc_video_clock_update is not that useful.
> Ideally, I would prefer if the v4l2_buffer of a captured frame
> included both
> 
>   * untranslated pts timestamp from the camera device (if I've
> understood this correctly, and there is a pts sent over the wire),
> and
> 
>   * the value of system monotonic clock at the point when the frame
> was received by the kernel.
> 
> Is there any reasonable way to get this information out from the
> driver? We could then do estimation of the camera's epoch and clock
> drift in the application. The raw pts is the most important piece of
> information.

I think these patches can help you;

http://www.mail-archive.com/linux-media@vger.kernel.org/msg106077.html

Note, that they require an additional patch from Laurent:

https://patchwork.linuxtv.org/patch/36810/

Thanks
Guennadi

> 
> Second, I'd like to try to provide some logs to help track down the
> bug. To reproduce, I'm using the example program at
> https://gist.github.com/maxlapshin/1253534, modified to print out
> camera timestamp and gettimeofday for each frame. Log attached as
> time-2.log.
> 
> I also enabled tracing of the clock translation logic using
> 
>   echo 4096 > /sys/module/uvcvideo/parameters/trace
> 
> The corresponding kernel log messages are attached as trace-2.log.
> 
> In time-2.log (i.e., the application log), I see that camera
> timestamps move backwards in time,
> 
>   TIMESTAMP_MONOTONIC
>  cam: 2321521.085372
>  sys: 1477913910.983620
>   TIMESTAMP_MONOTONIC
>  cam: 2321520.879272
>  sys: 1477913911.051628
> 
> In trace-2.log (i.e., kernel log messages) I see
> 
>   uvcvideo: Logitech Webcam C930e: PTS 219483992 y 4084.798004 SOF
> 4084.798004 (x1 2064310082 x2 2148397132 y1 218759168 y2 268238848 SOF
> offset 170)
>   uvcvideo: Logitech Webcam C930e: SOF 4084.798004 y 3105900702 ts
> 2321520.879272 buf ts 2321521.153372 (x1 218759168/1546/1290 x2
> 274071552/1878/2045 y1 10 y2 3380001263)
>   uvcvideo: Logitech Webcam C930e: PTS 221480532 y 4156.709564 SOF
> 4156.709564 (x1 2079524156 x2 2148397450 y1 256376832 y2 272629760 SOF
> offset 170)
>   uvcvideo: Logitech Webcam C930e: SOF 4156.709564 y 2453257742 ts
> 2321520.378627 buf ts 2321521.217373 (x1 262275072/1698/1864 x2
> 278265856/1942/64 y1 10 y2 3292003672)
>   uvcvideo: Logitech Webcam C930e: PTS 223477044 y 4223.428085 SOF
> 4223.428085 (x1 2081269216 x2 2148397122 y1 264568832 y2 276955136 SOF
> offset 170)
>   uvcvideo: Logitech Webcam C930e: SOF 2175.428085 y 2158773894 ts
> 2321520.208143 buf ts 2321521.285373 (x1 136183808/1822/1989 x2
> 148504576/2010/130 y1 10 y2 3236003012)
> 
> I don't know the details of the usb protocol, but it looks like the
> "SOF" value is usually increasing. But close to the bogus output
> timestamp of 2321520.879272, it goes through some kind of wraparound,
> with the sequence of values
> 
>   4156.709564
>   4223.428085
>   2175.428085# 2048 less than previous value
>   2243.169921
> 
> I hope the attached logs provide enough information to analyze where
> uvc_video_clock_update gets this wrong.
> 
> Best regards,
> /Niels Möller
> 
--
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: [TSN RFC v2 0/9] TSN driver for the kernel

[Richard Cochran]

On Fri, Dec 16, 2016 at 06:59:04PM +0100, hen...@austad.us wrote:
> The driver is directed via ConfigFS as we need userspace to handle
> stream-reservation (MSRP), discovery and enumeration (IEEE 1722.1) and
> whatever other management is needed.

I complained about configfs before, but you didn't listen.

> 2 new fields in netdev_ops have been introduced, and the Intel
> igb-driver has been updated (as this an AVB-capable NIC which is
> available as a PCI-e card).

The igb hacks show that you are on the wrong track.  We can and should
be able to support TSN without resorting to driver specific hacks and
module parameters.

> Before reading on - this is not even beta, but I'd really appreciate if
> people would comment on the overall architecture and perhaps provide
> some pointers to where I should improve/fix/update

As I said before about V1, this architecture stinks.  You appear to
have continued hacking along and posted the same design again.  Did
you even address any of the points I raised back then?

You are trying to put tons of code into the kernel that really belongs
in user space, and at the same time, you omit critical functions that
only the kernel can provide.

> There are at least one AVB-driver (the AV-part of TSN) in the kernel
> already.

And which driver is that?

Thanks,
Richard
--
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] [media] bt8xx: fix memory leak

[Sudip Mukherjee]

If dvb_attach() fails then we were just printing an error message and
exiting but the memory allocated to state was not released.

Signed-off-by: Sudip Mukherjee 
---
 drivers/media/pci/bt8xx/dvb-bt8xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/pci/bt8xx/dvb-bt8xx.c 
b/drivers/media/pci/bt8xx/dvb-bt8xx.c
index 6100fa7..e247638 100644
--- a/drivers/media/pci/bt8xx/dvb-bt8xx.c
+++ b/drivers/media/pci/bt8xx/dvb-bt8xx.c
@@ -683,6 +683,7 @@ static void frontend_init(struct dvb_bt8xx_card *card, u32 
type)
/*  DST is not a frontend, attaching the ASIC   */
if (dvb_attach(dst_attach, state, >dvb_adapter) == NULL) {
pr_err("%s: Could not find a Twinhan DST\n", __func__);
+   kfree(state);
break;
}
/*  Attach other DST peripherals if any */
-- 
1.9.1

--
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: [TSN RFC v2 0/9] TSN driver for the kernel

[Henrik Austad]

On Fri, Dec 16, 2016 at 01:20:57PM -0500, David Miller wrote:
> From: Greg 
> Date: Fri, 16 Dec 2016 10:12:44 -0800
> 
> > On Fri, 2016-12-16 at 18:59 +0100, hen...@austad.us wrote:
> >> From: Henrik Austad 
> >> 
> >> 
> >> The driver is directed via ConfigFS as we need userspace to handle
> >> stream-reservation (MSRP), discovery and enumeration (IEEE 1722.1) and
> >> whatever other management is needed. This also includes running an
> >> appropriate PTP daemon (TSN favors gPTP).
> > 
> > I suggest using a generic netlink interface to communicate with the
> > driver to set up and/or configure your drivers.
> > 
> > I think configfs is frowned upon for network drivers.  YMMV.
> 
> Agreed.

Ok - thanks!

I will have look at netlink and see if I can wrap my head around it and if 
I can apply it to how to bring the media-devices up once the TSN-link has 
been configured.

Thanks! :)

-- 
Henrik Austad


signature.asc
Description: PGP signature

Re: [TSN RFC v2 0/9] TSN driver for the kernel

[David Miller]

From: Greg 
Date: Fri, 16 Dec 2016 10:12:44 -0800

> On Fri, 2016-12-16 at 18:59 +0100, hen...@austad.us wrote:
>> From: Henrik Austad 
>> 
>> 
>> The driver is directed via ConfigFS as we need userspace to handle
>> stream-reservation (MSRP), discovery and enumeration (IEEE 1722.1) and
>> whatever other management is needed. This also includes running an
>> appropriate PTP daemon (TSN favors gPTP).
> 
> I suggest using a generic netlink interface to communicate with the
> driver to set up and/or configure your drivers.
> 
> I think configfs is frowned upon for network drivers.  YMMV.

Agreed.
--
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: [TSN RFC v2 0/9] TSN driver for the kernel

[Greg]

On Fri, 2016-12-16 at 18:59 +0100, hen...@austad.us wrote:
> From: Henrik Austad 
> 
> 
> The driver is directed via ConfigFS as we need userspace to handle
> stream-reservation (MSRP), discovery and enumeration (IEEE 1722.1) and
> whatever other management is needed. This also includes running an
> appropriate PTP daemon (TSN favors gPTP).

I suggest using a generic netlink interface to communicate with the
driver to set up and/or configure your drivers.

I think configfs is frowned upon for network drivers.  YMMV.

- Greg

> 
> Once we have all the required attributes, we can create link using
> mkdir, and use write() to set the attributes. Once ready, specify the
> 'shim' (basically a thin wrapper between TSN and another subsystem) and
> we start pushing out frames.
> 
> The network part: it ties directly into the Rx-handler for receive and
> writes skb's using dev_queue_xmit(). This could probably be improved.
> 
> 2 new fields in netdev_ops have been introduced, and the Intel
> igb-driver has been updated (as this an AVB-capable NIC which is
> available as a PCI-e card).
> 
> What remains (tl;dr: a lot) a.k.a "Known problems" or "working on it!"
> - tie to (g)PTP properly, currently using ktime_get() for presentation
>   time
> - get time from shim into TSN
> - let shim create/manage buffer
> - redo parts of the link-stuff using RCUs, the current setup is a bit
>   clumsy.
> - The igb-driver does not work properly when compiled with IGB_TSN, some
>   details in setting the register values needs to be figured out. I am
>   working on this, but as it stands, the best bet is to load tsn using
>   in_debug=1 to bypass the capability-check. I have had e1000 and sky2
>   running for several days without crashing, igb crashes and burns
>   violently.
> - The ALSA driver does not handle multiple devices very well and is a
>   work in progress.
> 
> * v2: changes since v1
> 
> Changes since v1
> - updated to latest upstream kernel (v4.8)
> - set dedicated enabled-attribute and let shim be stored in own (support
>   future plan for enabling per-shim attributes)
> - fixed endianess issue in bitfields used in tsn-structs
> - Updated some of the trace-events to use trace_class
> - Fix various silly typos
> - Handle disabling of link from hrtimer a bit more gracefully (that
>   actually works-ish).
> - use old skb and size of skb when that is set (Reporte by Nikita)
> - Move PCP-codes to NIC and not in the link itself
> - Allow TSN-capable card to be loaded even when in debug-mode (and do
>   not enforce TSN behaviour)
> - Start hooking into ALSA's get_time_info hooks (very much incomplete)
> - use threads for sending frames, wake from hrtimer-callback.
>   This also queues up awaiting timers if we fail to complete the
>   transmit before another timer arrives, it will immediately execute
>   another iteration, so no events should be lost. That being said,
>   should this happen, it is a clear bug as we really should complete
>   well before the next interval.
> - Cleanup link-locking and differentiate between Talker and Listener (as
>   Listener grab link-lock from IRQ context)
> - Change list-lock to spinlock as we may need to take a link-lock whilst
>   holding the master list-lock.
> - Do a more careful dance holding the spinlocks to regions only doing
>   actual update.
> 
> Network driver (I210 only)
> - bring up all Tx-/Rx-queues when igb is in TSN-mode regardless of how
>   many CPUs the system has for I210
> - Correctly calculate the idle_slope in I210's configure hook
> - Update igb-driver with queue-select and return correct queue when
>   sending TSN-frames
> - add IGB_FLAG_QAV_PRIO flag to igb_adapter (to handle proper config of
>   tx-ring when adapter is brought up.
> - add TXDCTL logic (part of preparatory work for TSN) to igb-driver
> - Improve SR(A|B) accountingo
> 
> ALSA Shim
> - Allow userspace to grab much smaller chunks of data (down to a single
>   Class A frame for S16_LE 2ch 48kHz).
> - Create the card with index/id pattern to avoid collision with other
>   cards.
> * v1
> 
> Before reading on - this is not even beta, but I'd really appreciate if
> people would comment on the overall architecture and perhaps provide
> some pointers to where I should improve/fix/update
> - thanks!
> 
> This is a very early RFC for a TSN-driver in the kernel. It has been
> floating around in my repo for a while and I would appreciate some
> feedback on the overall design to avoid doing some major blunders.
> 
> There are at least one AVB-driver (the AV-part of TSN) in the kernel
> already. This driver aims to solve a wider scope as TSN can do much more
> than just audio. A very basic ALSA-driver is added to the end that
> allows you to play music between 2 machines using aplay in one end and
> arecord | aplay on the other (some fiddling required) We have plans for
> doing the same for v4l2 eventually (but there are other fishes to fry
> first). The same goes for a TSN_SOCK type approach 

[TSN RFC v2 6/9] Add TSN machinery to drive the traffic from a shim over the network

[henrik]

From: Henrik Austad 

In short summary:

* tsn_core.c is the main driver of tsn, all new links go through
  here and all data to/form the shims are handled here
  core also manages the shim-interface.

* tsn_configfs.c is the API to userspace. TSN is driven from userspace
  and a link is created, configured, enabled, disabled and removed
  purely from userspace. All attributes requried must be determined by
  userspace, preferrably via IEEE 1722.1 (discovery and enumeration).

  New is that setting a shim will not automatically enable it, this is to
  allow shims to expose own attributes via ConfigFS. It will also make the
  steps a bit more obvious.

* tsn_header.c small part that handles the actual header of the frames
  we send. Kept out of core for cleanliness.

* tsn_net.c handles operations towards the networking layer. A *very*
  simple hook for handling backpressure in the tx-queue is added, but this
  is currently nowhere near sufficient.

The current driver is under development. This means that from the moment it
is enabled (with a registered shim), it will send traffic, either 0-traffic
(frames of reserved length but with payload 0) or actual traffic. This will
change once the driver stabilizes.

We also use a kthread to handle the lifting when transmitting frames. This
should remove some of the old timeouts and issues we had when doing all of
this via the hrtimer callback. Should a new timer fire before we are done,
it will be queued up and handled immediately. Note that this is a bug (we
*really* should be done before the next 1ms tick happens.

For more detail, see Documentation/networking/tsn/

Cc: "David S. Miller" 
Signed-off-by: Henrik Austad 
---
 net/Makefile   |1 +
 net/tsn/Makefile   |6 +
 net/tsn/tsn_configfs.c |  673 +++
 net/tsn/tsn_core.c | 1189 
 net/tsn/tsn_header.c   |  162 +++
 net/tsn/tsn_internal.h |  397 
 net/tsn/tsn_net.c  |  392 
 7 files changed, 2820 insertions(+)
 create mode 100644 net/tsn/Makefile
 create mode 100644 net/tsn/tsn_configfs.c
 create mode 100644 net/tsn/tsn_core.c
 create mode 100644 net/tsn/tsn_header.c
 create mode 100644 net/tsn/tsn_internal.h
 create mode 100644 net/tsn/tsn_net.c

diff --git a/net/Makefile b/net/Makefile
index 4cafaa2..a0f7d41 100644
--- a/net/Makefile
+++ b/net/Makefile
@@ -81,3 +81,4 @@ obj-y += l3mdev/
 endif
 obj-$(CONFIG_QRTR) += qrtr/
 obj-$(CONFIG_NET_NCSI) += ncsi/
+obj-$(CONFIG_TSN)  += tsn/
diff --git a/net/tsn/Makefile b/net/tsn/Makefile
new file mode 100644
index 000..0d87687
--- /dev/null
+++ b/net/tsn/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for the Linux TSN subsystem
+#
+
+obj-$(CONFIG_TSN) += tsn.o
+tsn-objs :=tsn_core.o tsn_configfs.o tsn_net.o tsn_header.o
diff --git a/net/tsn/tsn_configfs.c b/net/tsn/tsn_configfs.c
new file mode 100644
index 000..9ace1aa
--- /dev/null
+++ b/net/tsn/tsn_configfs.c
@@ -0,0 +1,673 @@
+/*
+ *   ConfigFS interface to TSN
+ *   Copyright (C) 2015- Henrik Austad 
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "tsn_internal.h"
+
+static inline struct tsn_link *to_tsn_link(struct config_item *item)
+{
+   /* this line causes checkpatch to WARN. making checkpatch happy,
+* makes code messy..
+*/
+   return item ? container_of(to_config_group(item), struct tsn_link, 
group) : NULL;
+}
+
+static inline struct tsn_nic *to_tsn_nic(struct config_group *group)
+{
+   return group ? container_of(group, struct tsn_nic, group) : NULL;
+}
+
+static inline struct tsn_nic *item_to_tsn_nic(struct config_item *item)
+{
+   return item ? container_of(to_config_group(item), struct tsn_nic, 
group) : NULL;
+}
+
+/* ---
+ * Tier2 attributes
+ *
+ * The content of the links userspace can see/modify
+ * ---
+*/
+static ssize_t _tsn_max_payload_size_show(struct config_item *item,
+ char *page)
+{
+   struct tsn_link *link = to_tsn_link(item);
+
+   if (!link)
+   return -EINVAL;
+   return sprintf(page, "%u\n", (u32)link->max_payload_size);
+}
+
+static ssize_t 

[TSN RFC v2 9/9] MAINTAINERS: add TSN/AVB-entries

[henrik]

From: Henrik Austad 

Not sure how relevant this is other than making a point about
maintaining it.

Signed-off-by: Henrik Austad 
---
 MAINTAINERS | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 63cefa6..7c5afd2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12295,6 +12295,20 @@ T: git git://linuxtv.org/anttip/media_tree.git
 S: Maintained
 F: drivers/media/tuners/tua9001*
 
+TSN CORE DRIVER
+M: Henrik Austad 
+L: linux-ker...@vger.kernel.org
+S: Supported
+F: drivers/net/tsn/
+F: include/linux/tsn.h
+F: include/trace/events/tsn.h
+
+TSN_AVB_DRIVER
+M: Henrik Austad 
+L: alsa-de...@alsa-project.org (moderated for non-subscribers)
+S: Supported
+F: drivers/media/avb/
+
 TULIP NETWORK DRIVERS
 L: net...@vger.kernel.org
 L: linux-par...@vger.kernel.org
-- 
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

[TSN RFC v2 7/9] Add TSN event-tracing

[henrik]

From: Henrik Austad 

Provide a fair debug-window into TSN. It tries to use TRACE_CLASS as much
as possible and moves as much as possible of the logic into TP_printk() to
minimize tracing overhead.

Cc: "David S. Miller" 
Cc: Steven Rostedt  (maintainer:TRACING)
Cc: Ingo Molnar  (maintainer:TRACING)
Signed-off-by: Henrik Austad 
---
 include/trace/events/tsn.h | 333 +
 1 file changed, 333 insertions(+)
 create mode 100644 include/trace/events/tsn.h

diff --git a/include/trace/events/tsn.h b/include/trace/events/tsn.h
new file mode 100644
index 000..59c
--- /dev/null
+++ b/include/trace/events/tsn.h
@@ -0,0 +1,333 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM tsn
+
+#if !defined(_TRACE_TSN_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_TSN_H
+
+#include 
+#include 
+
+#include 
+#include 
+/* #include  */
+DECLARE_EVENT_CLASS(tsn_buffer_template,
+
+   TP_PROTO(struct tsn_link *link,
+   size_t bytes),
+
+   TP_ARGS(link, bytes),
+
+   TP_STRUCT__entry(
+   __field(u64, stream_id)
+   __field(size_t, size)
+   __field(size_t, bsize)
+   __field(void *, buffer)
+   __field(void *, head)
+   __field(void *, tail)
+   __field(void *, end)
+   ),
+
+   TP_fast_assign(
+   __entry->stream_id = link->stream_id;
+   __entry->size = bytes;
+   __entry->bsize = link->used_buffer_size;
+   __entry->buffer = link->buffer;
+   __entry->head = link->head;
+   __entry->tail = link->tail;
+   __entry->end = link->end;
+   ),
+
+   TP_printk("stream_id=%llu, copy=%zd, buffer: %zd, avail=%zd, 
[buffer=%p, head=%p, tail=%p, end=%p]",
+   __entry->stream_id, __entry->size, __entry->bsize,
+   (__entry->head - __entry->tail) % __entry->bsize,
+   __entry->buffer,__entry->head, __entry->tail, __entry->end)
+);
+
+DEFINE_EVENT(tsn_buffer_template, tsn_buffer_write,
+   TP_PROTO(struct tsn_link *link, size_t bytes),
+   TP_ARGS(link, bytes)
+);
+
+DEFINE_EVENT(tsn_buffer_template, tsn_buffer_write_net,
+   TP_PROTO(struct tsn_link *link, size_t bytes),
+   TP_ARGS(link, bytes)
+);
+
+DEFINE_EVENT(tsn_buffer_template, tsn_buffer_read,
+   TP_PROTO(struct tsn_link *link, size_t bytes),
+   TP_ARGS(link, bytes)
+);
+
+DEFINE_EVENT(tsn_buffer_template, tsn_buffer_read_net,
+   TP_PROTO(struct tsn_link *link, size_t bytes),
+   TP_ARGS(link, bytes)
+);
+
+
+DECLARE_EVENT_CLASS(tsn_buffer_update,
+
+   TP_PROTO(struct tsn_link *link,
+   size_t reported_avail),
+
+   TP_ARGS(link, reported_avail),
+
+   TP_STRUCT__entry(
+   __field(u64, stream_id)
+   __field(size_t, bsize)
+   __field(void *, head)
+   __field(void *, tail)
+   __field(size_t, reported_left)
+   __field(size_t, low_water)
+   ),
+
+   TP_fast_assign(
+   __entry->stream_id = link->stream_id;
+   __entry->bsize = link->used_buffer_size;
+   __entry->head = link->head;
+   __entry->tail = link->tail;
+   __entry->reported_left = reported_avail;
+   __entry->low_water = link->low_water_mark;
+   ),
+
+   TP_printk("stream_id=%llu, buffer_size=%zd, avail=%zd, reported=%zd, 
low_water=%zd",
+   __entry->stream_id, __entry->bsize,
+   (__entry->head - __entry->tail) % __entry->bsize,
+   __entry->reported_left, __entry->low_water)
+);
+
+/* Bytes will be "reported left", i.e. how much more space we have in
+ * the buffer before we wrap.
+ */
+DEFINE_EVENT(tsn_buffer_update, tsn_refill,
+   TP_PROTO(struct tsn_link *link, size_t bytes),
+   TP_ARGS(link, bytes)
+);
+
+DEFINE_EVENT(tsn_buffer_update, tsn_buffer_drain,
+   TP_PROTO(struct tsn_link *link, size_t bytes),
+   TP_ARGS(link, bytes)
+);
+
+TRACE_EVENT(tsn_send_batch,
+
+   TP_PROTO(struct tsn_link *link,
+   int num_send,
+   u64 ts_base_ns,
+   u64 ts_delta_ns),
+
+   TP_ARGS(link, num_send, ts_base_ns, ts_delta_ns),
+
+   TP_STRUCT__entry(
+   __field(u64, stream_id)
+   __field(int, seqnr)
+   __field(int, num_send)
+   __field(u64, ts_base_ns)
+   __field(u64, ts_delta_ns)
+   ),
+
+   TP_fast_assign(
+   __entry->stream_id   = link->stream_id;
+   __entry->seqnr   = (int)link->last_seqnr;
+   __entry->ts_base_ns  = ts_base_ns;
+   __entry->ts_delta_ns = ts_delta_ns;
+   __entry->num_send= num_send;
+   ),
+
+   

[TSN RFC v2 1/9] igb: add missing fields to TXDCTL-register

[henrik]

From: Henrik Austad 

The current list of E1000_TXDCTL-registers is incomplete. This adds the
missing parts for the Transmit Descriptor Control (TXDCTL) register.

The rest of these values (threshold for descriptor read/write) for
TXDCTL seems to be defined in igb/igb.h, not sure why this is split
though.

It seems that this was left out in the commit that added support for
82575 Gigabit Ethernet driver 9d5c8243 (igb: PCI-Express 82575 Gigabit
Ethernet driver).

Cc: linux-ker...@vger.kernel.org
Cc: Jeff Kirsher 
Cc: intel-wired-...@lists.osuosl.org
Signed-off-by: Henrik Austad 
---
 drivers/net/ethernet/intel/igb/e1000_82575.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.h 
b/drivers/net/ethernet/intel/igb/e1000_82575.h
index acf0605..7faa482 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.h
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.h
@@ -158,7 +158,11 @@ struct e1000_adv_tx_context_desc {
 
 /* Additional Transmit Descriptor Control definitions */
 #define E1000_TXDCTL_QUEUE_ENABLE  0x0200 /* Enable specific Tx Queue */
+
+/* Transmit Software Flush, sw-triggered desc writeback */
+#define E1000_TXDCTL_SWFLSH0x0400
 /* Tx Queue Arbitration Priority 0=low, 1=high */
+#define E1000_TXDCTL_PRIORITY  0x0800
 
 /* Additional Receive Descriptor Control definitions */
 #define E1000_RXDCTL_QUEUE_ENABLE  0x0200 /* Enable specific Rx Queue */
-- 
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

[TSN RFC v2 2/9] TSN: add documentation

[henrik]

From: Henrik Austad 

Describe the overall design behind the TSN standard, the TSN-driver,
requirements to userspace and new functionality introduced.

Cc: "David S. Miller" 
Signed-off-by: Henrik Austad 
---
 Documentation/TSN/tsn.txt | 345 ++
 1 file changed, 345 insertions(+)
 create mode 100644 Documentation/TSN/tsn.txt

diff --git a/Documentation/TSN/tsn.txt b/Documentation/TSN/tsn.txt
new file mode 100644
index 000..540246f
--- /dev/null
+++ b/Documentation/TSN/tsn.txt
@@ -0,0 +1,345 @@
+Time Sensitive Networking (TSN)
+---
+
+[work in progress]
+
+1. Motivation
+=
+
+TSN is a set of open standards, formerly known as 'AVB' (Audio/Video
+Bridging). It was renamed to TSN to better reflect that it can do much
+more than just media transport and extended to handle more types of
+traffic.
+
+TSN is a way to create reliable, deterministic streams across a network
+without loss of frames due to congestion in the network. By using gPTP
+(a specialized IEEE-1588v2 PTP profile), the time can be synchronized
+with sub-us granularity across all the connected devices in the AVB
+domain.
+
+In its current version, this driver only supports L2 traffic (i.e
+etherframes only), but later version is planned to handle L3. L2-L3
+traversing is currently being worked on by the IETF detnet working
+group.
+
+2. Intro to AVB/TSN
+===
+
+The original standards were written with Audio/Video in mind, so the
+initial standards refer to this as 'AVB'. In later standards, this has
+changed to TSN, and AVB now refers to a service you can add on top of
+TSN. In some parts of the driver, this naming shines through, in
+particular for AVTP (AVB Transport Protocol), and this is to reflect the
+naming in the standards.
+
+In this document, we refer to the infrastructure part as TSN, and AVB to
+the ALSA/V4L2 shim which can be added on top of TSN to provide a
+media-service.
+
+TSN operates with 'streams', and one stream can contain pretty much
+whatever you like. An AVB stream carrying audio can carry multiple
+channels. The current revision of AVTP (defined in IEEE 1722 d16)
+defines many more types than media.
+
+A stream flows through the network from a Talker to a Listener. A Talker
+is a single End-station in the network, a Listener can be a single
+End-station (unicast) or a group of end-stdations (multicast).
+
+2.1 Domains
+
+2.1.1 SRP Domain
+
+An SRP domain is the set of entities in the network that support the
+Stream Reservation Protocol (IEEE 802.1Q-2014 Sec 35) and where all
+entities agree on the priority code points (PCP). A bridge will mark
+each port as either SRP capable or not capable.
+
+PCP is used to map a specific priority to a given traffic-class,
+typically class A or B.
+
+2.1.2 gPTP domain
+
+This is the set of all connected bridges and end-stations that support
+the gPTP protocol. gPTO is a PTPv2 profile.
+
+2.1.3 AVB Domain
+
+An AVB domain is the intersection of an SRP Domain and gPTP domain.
+
+
+2.2 End Station (ES)
+
+An TSN ES is where a stream either originates or ends -what others would
+call sources (Talkers) and sinks (Listeners). Looking back at pre-TSN
+when this was called AVB, these names make a bit more sense.
+
+Common for both types, they need to be PTPv2 capable, i.e. you need to
+timestamp gPTP frames upon ingress/egress to improve the accuracy of
+PTP.
+
+2.2.1 Talkers
+
+A Talker must be single ES in the AVB Domain.
+
+Hardware requirements:
+- Multiple Tx-queues
+- Credit based shaper on at least one of the queues for pacing the
+  frames onto the network
+- VLAN capable
+
+2.2.2 Listener
+
+A Listener does not have the same requirements as a Talker as it cannot
+control the pace of the incoming frames anyway. It is beneficial if the
+NIC understands VLANs and has a few Rx-queues so that you can steer all
+TSN-frames to a dedicated queue, but this is not a hard requirement.
+
+If the Listener receives audio, having an adjustable PL/L is a clear
+benefit to avoid resampling.
+
+2.3 Bridges
+
+A Bridge is what TSN calls switches that are TSN-capable. They must be
+able to prioritize TSN-streams, have the credit-based shaper available
+for that class, support SRP, support gPTP and so on. The requirements is
+laid down in "Forwardin and Queueing of Time Sensitive Streams" (IEEE
+802.1Q-2014 sec. 34).
+
+2.4 Relevant standards
+
+* IEEE 802.1BA-2011 Audio Video Bridging (AVB) Systems
+
+* IEEE 802.1Q-2014 sec 34 and 35
+
+  What is referred to as:
+  IEEE 802.1Qav (Forwarding and Queueing for Time-sensitive Streams)
+  IEEE 802.1Qat (Stream Registration protocol)
+
+* IEEE 802.1AS gPTP
+
+  A PTPv2 profile (from IEEE 1588) tailored for this domain. Notable
+  changes include the requirement that all nodes in the network must be
+  gPTP capable (i.e. no traversing non-PTP entities), and it allows
+ 

[TSN RFC v2 3/9] TSN: Add the standard formerly known as AVB to the kernel

[henrik]

From: Henrik Austad 

TSN provides a mechanism to create reliable, jitter-free, low latency
guaranteed bandwidth links over a local network. It does this by
reserving a path through the network. Support for TSN must be found in
both the NIC as well as in the network itself.

This adds required hooks into netdev_ops so that the core TSN driver can
use this when configuring a new NIC or setting up a new link. It also
provides hook for removing a link and reducing the idle_slope parameter on
the NIC.

(We need to set the PCP values when we first configure the link. This
 value should not change as long as we have valid streams running, and in
 most cases, the PCP for the domain will not change.)

Cc: "David S. Miller" 
Signed-off-by: Henrik Austad 
---
 include/linux/netdevice.h | 44 
 net/Kconfig   |  1 +
 net/tsn/Kconfig   | 32 
 3 files changed, 77 insertions(+)
 create mode 100644 net/tsn/Kconfig

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e16a2a9..0d758aa 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -112,6 +112,15 @@ enum netdev_tx {
 };
 typedef enum netdev_tx netdev_tx_t;
 
+#if IS_ENABLED(CONFIG_TSN)
+enum sr_class {
+   SR_CLASS_A = 1,
+   SR_CLASS_B = 2,
+   SR_CLASS_LAST,
+   SR_CLASS_ERR,
+};
+#endif
+
 /*
  * Current order: NETDEV_TX_MASK > NET_XMIT_MASK >= 0 is significant;
  * hard_start_xmit() return < NET_XMIT_MASK means skb was consumed.
@@ -944,6 +953,31 @@ struct netdev_xdp {
  *
  * void (*ndo_poll_controller)(struct net_device *dev);
  *
+ * TSN functions (if CONFIG_TSN)
+ *
+ * int (*ndo_tsn_capable)(struct net_device *dev);
+ * If a particular device is capable of sustaining TSN traffic
+ * provided current configuration
+ *
+ * int (*ndo_tsn_link_configure)(struct net_device *dev,
+ *  enum sr_class class,
+ *  u16 framesize,
+ *  u16 vid,
+ *  u8 add_link,
+ *  u8 pcp_hi,
+ *  u8 pcp_lo)
+);
+ * Configure a NIC to handle TSN-streams
+ * - Update the bandwidth for the particular stream-class.
+ * - The framesize is the size of the _entire_ frame (not just the payload)
+ *   since the full size is required to allocate bandwidth through
+ *   the credit based shaper in the NIC
+ * - the vlan_id is the configured vlan for TSN in this session.
+ * - add_link: if the link should be added or subtracted from the current
+ *   budget.
+ *- u8 pcp_hi: 802.1Q priority value for high-class traffic (class A)
+ *- u8 pcp_lo: 802.1Q priority value for low-class traffic (class B)
+ *
  * SR-IOV management functions.
  * int (*ndo_set_vf_mac)(struct net_device *dev, int vf, u8* mac);
  * int (*ndo_set_vf_vlan)(struct net_device *dev, int vf, u16 vlan,
@@ -1185,6 +1219,16 @@ struct net_device_ops {
 #ifdef CONFIG_NET_RX_BUSY_POLL
int (*ndo_busy_poll)(struct napi_struct *dev);
 #endif
+
+#if IS_ENABLED(CONFIG_TSN)
+   int (*ndo_tsn_capable)(struct net_device *dev);
+   int (*ndo_tsn_link_configure)(struct net_device 
*dev,
+ enum sr_class class,
+ u16 framesize,
+ u16 vid, u8 add_link,
+ u8 pcp_hi, u8 pcp_lo);
+#endif /* CONFIG_TSN */
+
int (*ndo_set_vf_mac)(struct net_device *dev,
  int queue, u8 *mac);
int (*ndo_set_vf_vlan)(struct net_device *dev,
diff --git a/net/Kconfig b/net/Kconfig
index 7b6cd34..19b8f9a 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -215,6 +215,7 @@ source "net/802/Kconfig"
 source "net/bridge/Kconfig"
 source "net/dsa/Kconfig"
 source "net/8021q/Kconfig"
+source "net/tsn/Kconfig"
 source "net/decnet/Kconfig"
 source "net/llc/Kconfig"
 source "net/ipx/Kconfig"
diff --git a/net/tsn/Kconfig b/net/tsn/Kconfig
new file mode 100644
index 000..1fc3c1d
--- /dev/null
+++ b/net/tsn/Kconfig
@@ -0,0 +1,32 @@
+#
+# Configuration for 802.1 Time Sensitive Networking (TSN)
+#
+
+config TSN
+   tristate "802.1 TSN Support"
+   depends on VLAN_8021Q && PTP_1588_CLOCK && CONFIGFS_FS
+   ---help---
+ Select this if you want to enable TSN on capable interfaces.
+
+ TSN allows you to set up deterministic links on your LAN (only
+ L2 is currently supported). Once loaded, the driver will probe
+ all available interfaces if they are capable of supporting TSN
+ links.
+
+ Once loaded, a directory in 

[TSN RFC v2 5/9] Add TSN header for the driver

[henrik]

From: Henrik Austad 

This defines the general TSN headers for network packets, the
shim-interface and the central 'tsn_list' structure.

Cc: "David S. Miller" 
Signed-off-by: Henrik Austad 
---
 include/linux/tsn.h | 952 
 1 file changed, 952 insertions(+)
 create mode 100644 include/linux/tsn.h

diff --git a/include/linux/tsn.h b/include/linux/tsn.h
new file mode 100644
index 000..9123b25
--- /dev/null
+++ b/include/linux/tsn.h
@@ -0,0 +1,952 @@
+/*   TSN - Time Sensitive Networking
+ *
+ *   Copyright (C) 2016- Henrik Austad 
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ */
+#ifndef _TSN_H
+#define _TSN_H
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * List of current subtype fields in the common header of AVTPDU
+ *
+ * Note: AVTPDU is a remnant of the standards from when it was AVB.
+ *
+ * The list has been updated with the recent values from IEEE 1722, draft 16.
+ */
+enum avtp_subtype {
+   TSN_61883_IIDC = 0, /* IEC 61883/IIDC Format */
+   TSN_MMA_STREAM, /* MMA Streams */
+   TSN_AAF,/* AVTP Audio Format */
+   TSN_CVF,/* Compressed Video Format */
+   TSN_CRF,/* Clock Reference Format */
+   TSN_TSCF,   /* Time-Synchronous Control Format */
+   TSN_SVF,/* SDI Video Format */
+   TSN_RVF,/* Raw Video Format */
+   /* 0x08 - 0x6D reserved */
+   TSN_AEF_CONTINOUS = 0x6e, /* AES Encrypted Format Continous */
+   TSN_VSF_STREAM, /* Vendor Specific Format Stream */
+   /* 0x70 - 0x7e reserved */
+   TSN_EF_STREAM = 0x7f,   /* Experimental Format Stream */
+   /* 0x80 - 0x81 reserved */
+   TSN_NTSCF = 0x82,   /* Non Time-Synchronous Control Format */
+   /* 0x83 - 0xed reserved */
+   TSN_ESCF = 0xec,/* ECC Signed Control Format */
+   TSN_EECF,   /* ECC Encrypted Control Format */
+   TSN_AEF_DISCRETE,   /* AES Encrypted Format Discrete */
+   /* 0xef - 0xf9 reserved */
+   TSN_ADP = 0xfa, /* AVDECC Discovery Protocol */
+   TSN_AECP,   /* AVDECC Enumeration and Control Protocol */
+   TSN_ACMP,   /* AVDECC Connection Management Protocol */
+   /* 0xfd reserved */
+   TSN_MAAP = 0xfe,/* MAAP Protocol */
+   TSN_EF_CONTROL, /* Experimental Format Control */
+};
+
+/* Link-states to help error-recovery detected from irq context.
+ */
+enum link_states {
+   LINK_OFF = 0,
+   LINK_RUNNING,
+   LINK_ERROR,
+};
+
+
+/* NOTE NOTE NOTE !!
+ * The headers below use bitfields extensively and verifications
+ * are needed when using little-endian vs big-endian systems.
+ */
+
+/* Common part of avtph header
+ *
+ * AVB Transport Protocol Common Header
+ *
+ * Defined in 1722-2011 Sec. 5.2
+ */
+struct avtp_ch {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+   /* use avtp_subtype enum.
+*/
+   u8 subtype:7;
+
+   /* Controlframe: 1
+* Dataframe   : 0
+*/
+   u8 cd:1;
+
+   /* Type specific data, part 1 */
+   u8 tsd_1:4;
+
+   /* In current version of AVB, only 0 is valid, all other values
+* are reserved for future versions.
+*/
+   u8 version:3;
+
+   /* Valid StreamID in frame
+*
+* ControlData not related to a specific stream should clear
+* this (and have stream_id = 0), _all_ other values should set
+* this to 1.
+*/
+   u8 sv:1;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+   u8 cd:1;
+   u8 subtype:7;
+   u8 sv:1;
+   u8 version:3;
+   u8 tsd_1:4;
+#else
+#error "Unknown Endianness, cannot determine bitfield ordering"
+#endif
+   /* Type specific data (adjacent to tsd_1, but split due to bitfield) */
+   u16 tsd_2;
+   u64 stream_id;
+
+   /*
+* payload by subtype
+*/
+   u8 pbs[0];
+} __packed;
+
+/* AVTPDU Common Control header format
+ * IEEE 1722#5.3
+ */
+struct avtpc_header {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+   u8 subtype:7;
+   u8 cd:1;
+   u8 control_data:4;
+   u8 version:3;
+   u8 sv:1;
+   u16 control_data_length:11;
+   u16 status:5;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+   u8 cd:1;
+   u8 subtype:7;
+   u8 sv:1;
+   u8 

[TSN RFC v2 4/9] Adding TSN-driver to Intel I210 controller

[henrik]

From: Henrik Austad 

This adds support for loading the igb.ko module with tsn
capabilities. This requires a 2-step approach. First enabling TSN in
.config, then load the module with use_tsn=1.

Once enabled and loaded, the controller will be placed in "Qav-mode"
which is when the credit-based shaper is available, 3 of the queues are
removed from regular traffic, max payload is set to 1522 octets (no
jumboframes allowed).

It dumps the registers of interest before and after, so this clutters
kern.log a bit if it is loaded with debug_tsn=1.

Regardless of number of online CPUs, it will enable *all* for Tx-queues as
2 is required for Qav traffic. This has not been tested extensively, so
there may be some instabilities in this.

Improved SR(A|B) accounting:
Use the idleslope-bins to keep track of how much time is reserved for
each class. This can then be used to strip the vlan-tag on the NIC when
the last stream goes (and also allow for reconfiguration of PCP when the
NIC is not sending TSN traffic).

Note: currently this driver is *not* stable, it is still a work in
progress, some points to keep tabs on:
- Set hicred to unlim (for testing this is ok and we avoid some nasty
  calculations)
- once we configure it for TSN, enable credit shaping, do not wait for
  first link to be configured (nobody else should use these queues after
  being configured).
- enable all Tx-/Rx-queues in TSN-mode regardless of num_online_cpus()
- Add 802.1Qav Prio-bit in adapter->flags
Cc: Jeff Kirsher 
Cc: Jesse Brandeburg 
Cc: intel-wired-...@lists.osuosl.org
Cc: "David S. Miller" 
Signed-off-by: Henrik Austad 
---
 drivers/net/ethernet/intel/Kconfig|  18 ++
 drivers/net/ethernet/intel/igb/Makefile   |   2 +-
 drivers/net/ethernet/intel/igb/igb.h  |  26 ++
 drivers/net/ethernet/intel/igb/igb_main.c |  39 ++-
 drivers/net/ethernet/intel/igb/igb_tsn.c  | 468 ++
 5 files changed, 550 insertions(+), 3 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igb/igb_tsn.c

diff --git a/drivers/net/ethernet/intel/Kconfig 
b/drivers/net/ethernet/intel/Kconfig
index c0e1743..d4382b4 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -99,6 +99,24 @@ config IGB
  To compile this driver as a module, choose M here. The module
  will be called igb.
 
+config IGB_TSN
+   tristate "TSN Support for Intel(R) 82575/82576 i210 Network Controller"
+   depends on IGB && TSN
+   ---help---
+ This driver supports TSN (AVB) on Intel I210 network controllers.
+
+ When enabled, it will allow the module to be loaded with
+ "use_tsn" which will initialize the controller to A/V-mode
+ instead of legacy-mode. This will take 3 of the tx-queues and
+ place them in 802.1Q QoS mode and enable the credit-based
+ shaper for 2 of the queues.
+
+ If built with this option, but not loaded with use_tsn, the
+ only difference is a slightly larger module, no extra
+ code paths are called.
+
+ If unsure, say No
+
 config IGB_HWMON
bool "Intel(R) PCI-Express Gigabit adapters HWMON support"
default y
diff --git a/drivers/net/ethernet/intel/igb/Makefile 
b/drivers/net/ethernet/intel/igb/Makefile
index 5bcb2de..1a9b776 100644
--- a/drivers/net/ethernet/intel/igb/Makefile
+++ b/drivers/net/ethernet/intel/igb/Makefile
@@ -33,4 +33,4 @@ obj-$(CONFIG_IGB) += igb.o
 
 igb-objs := igb_main.o igb_ethtool.o e1000_82575.o \
e1000_mac.o e1000_nvm.o e1000_phy.o e1000_mbx.o \
-   e1000_i210.o igb_ptp.o igb_hwmon.o
+   e1000_i210.o igb_ptp.o igb_hwmon.o igb_tsn.o
diff --git a/drivers/net/ethernet/intel/igb/igb.h 
b/drivers/net/ethernet/intel/igb/igb.h
index d11093d..474a5b4 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -394,6 +394,7 @@ struct igb_nfc_filter {
 };
 
 /* board specific private data structure */
+
 struct igb_adapter {
unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
 
@@ -519,6 +520,17 @@ struct igb_adapter {
/* lock for RX network flow classification filter */
spinlock_t nfc_lock;
bool etype_bitmap[MAX_ETYPE_FILTER];
+
+#if IS_ENABLED(CONFIG_IGB_TSN)
+   /* Reserved BW for class A and B */
+   s32 sra_idleslope_res;
+   s32 srb_idleslope_res;
+   u8 pcp_hi;
+   u8 pcp_lo;
+   u8 tsn_ready:1;
+   u8 tsn_vlan_added:1;
+   u8 res:6;
+#endif /* IGB_TSN */
 };
 
 /* flags controlling PTP/1588 function */
@@ -540,6 +552,7 @@ struct igb_adapter {
 #define IGB_FLAG_HAS_MSIX  BIT(13)
 #define IGB_FLAG_EEE   BIT(14)
 #define IGB_FLAG_VLAN_PROMISC  BIT(15)
+#define IGB_FLAG_QAV_PRIO  BIT(16)
 
 /* Media Auto Sense */
 #define IGB_MAS_ENABLE_0   0X0001
@@ -603,6 

[TSN RFC v2 0/9] TSN driver for the kernel

[henrik]

From: Henrik Austad 


The driver is directed via ConfigFS as we need userspace to handle
stream-reservation (MSRP), discovery and enumeration (IEEE 1722.1) and
whatever other management is needed. This also includes running an
appropriate PTP daemon (TSN favors gPTP).

Once we have all the required attributes, we can create link using
mkdir, and use write() to set the attributes. Once ready, specify the
'shim' (basically a thin wrapper between TSN and another subsystem) and
we start pushing out frames.

The network part: it ties directly into the Rx-handler for receive and
writes skb's using dev_queue_xmit(). This could probably be improved.

2 new fields in netdev_ops have been introduced, and the Intel
igb-driver has been updated (as this an AVB-capable NIC which is
available as a PCI-e card).

What remains (tl;dr: a lot) a.k.a "Known problems" or "working on it!"
- tie to (g)PTP properly, currently using ktime_get() for presentation
  time
- get time from shim into TSN
- let shim create/manage buffer
- redo parts of the link-stuff using RCUs, the current setup is a bit
  clumsy.
- The igb-driver does not work properly when compiled with IGB_TSN, some
  details in setting the register values needs to be figured out. I am
  working on this, but as it stands, the best bet is to load tsn using
  in_debug=1 to bypass the capability-check. I have had e1000 and sky2
  running for several days without crashing, igb crashes and burns
  violently.
- The ALSA driver does not handle multiple devices very well and is a
  work in progress.

* v2: changes since v1

Changes since v1
- updated to latest upstream kernel (v4.8)
- set dedicated enabled-attribute and let shim be stored in own (support
  future plan for enabling per-shim attributes)
- fixed endianess issue in bitfields used in tsn-structs
- Updated some of the trace-events to use trace_class
- Fix various silly typos
- Handle disabling of link from hrtimer a bit more gracefully (that
  actually works-ish).
- use old skb and size of skb when that is set (Reporte by Nikita)
- Move PCP-codes to NIC and not in the link itself
- Allow TSN-capable card to be loaded even when in debug-mode (and do
  not enforce TSN behaviour)
- Start hooking into ALSA's get_time_info hooks (very much incomplete)
- use threads for sending frames, wake from hrtimer-callback.
  This also queues up awaiting timers if we fail to complete the
  transmit before another timer arrives, it will immediately execute
  another iteration, so no events should be lost. That being said,
  should this happen, it is a clear bug as we really should complete
  well before the next interval.
- Cleanup link-locking and differentiate between Talker and Listener (as
  Listener grab link-lock from IRQ context)
- Change list-lock to spinlock as we may need to take a link-lock whilst
  holding the master list-lock.
- Do a more careful dance holding the spinlocks to regions only doing
  actual update.

Network driver (I210 only)
- bring up all Tx-/Rx-queues when igb is in TSN-mode regardless of how
  many CPUs the system has for I210
- Correctly calculate the idle_slope in I210's configure hook
- Update igb-driver with queue-select and return correct queue when
  sending TSN-frames
- add IGB_FLAG_QAV_PRIO flag to igb_adapter (to handle proper config of
  tx-ring when adapter is brought up.
- add TXDCTL logic (part of preparatory work for TSN) to igb-driver
- Improve SR(A|B) accountingo

ALSA Shim
- Allow userspace to grab much smaller chunks of data (down to a single
  Class A frame for S16_LE 2ch 48kHz).
- Create the card with index/id pattern to avoid collision with other
  cards.
* v1

Before reading on - this is not even beta, but I'd really appreciate if
people would comment on the overall architecture and perhaps provide
some pointers to where I should improve/fix/update
- thanks!

This is a very early RFC for a TSN-driver in the kernel. It has been
floating around in my repo for a while and I would appreciate some
feedback on the overall design to avoid doing some major blunders.

There are at least one AVB-driver (the AV-part of TSN) in the kernel
already. This driver aims to solve a wider scope as TSN can do much more
than just audio. A very basic ALSA-driver is added to the end that
allows you to play music between 2 machines using aplay in one end and
arecord | aplay on the other (some fiddling required) We have plans for
doing the same for v4l2 eventually (but there are other fishes to fry
first). The same goes for a TSN_SOCK type approach as well.

Henrik Austad (9):
  igb: add missing fields to TXDCTL-register
  TSN: add documentation
  TSN: Add the standard formerly known as AVB to the kernel
  Adding TSN-driver to Intel I210 controller
  Add TSN header for the driver
  Add TSN machinery to drive the traffic from a shim over the network
  Add TSN event-tracing
  AVB ALSA - Add ALSA shim for TSN
  MAINTAINERS: add TSN/AVB-entries

 Documentation/TSN/tsn.txt

[TSN RFC v2 8/9] AVB ALSA - Add ALSA shim for TSN

[henrik]

From: Henrik Austad 

This exposes a *very* rudimentary and simplistic ALSA driver that hooks
into TSN to create a device for userspace.

It currently only supports 44.1/48kHz sampling, 2ch, S16_LE

Userspace is supposed to reserve bandwidth, find StreamID etc.

To use as a Talker:

mkdir /config/tsn/test/eth0/talker
cd /config/tsn/test/eth0/talker
echo 65535 > buffer_size
echo 08:00:27:08:9f:c3 > remote_mac
echo 42 > stream_id
echo alsa > shim
echo on > enabled

 aplay -Ddefault:CARD=avb music.wav

  or

 arecord -r48000 -c2 -f S16_LE | aplay -Ddefault:CARD=avb

alternatively, if the device is set as Listener;

 arecord -r48000 -c2 -f S16_LE -Ddefault:CARD=avb > file.wav

Cc: Mauro Carvalho Chehab 
Cc: Takashi Iwai 
Cc: Mark Brown 
Signed-off-by: Henrik Austad 
---
 drivers/media/Kconfig|  15 +
 drivers/media/Makefile   |   2 +-
 drivers/media/avb/Makefile   |   5 +
 drivers/media/avb/avb_alsa.c | 793 +++
 drivers/media/avb/tsn_iec61883.h | 152 
 5 files changed, 966 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/avb/Makefile
 create mode 100644 drivers/media/avb/avb_alsa.c
 create mode 100644 drivers/media/avb/tsn_iec61883.h

diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index 7b85402..8250aff 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -221,3 +221,18 @@ source "drivers/media/tuners/Kconfig"
 source "drivers/media/dvb-frontends/Kconfig"
 
 endif # MEDIA_SUPPORT
+
+config MEDIA_AVB_ALSA
+   tristate "ALSA part of AVB over TSN"
+   depends on TSN
+   help
+
+ Enable the ALSA device that hoooks into TSN and allows the
+ computer to send ethernet frames over the network carrying
+ audio-data to selected hosts.
+
+This must be configured by userspace as MSRP and IEEE 1722.1
+(discovery and enumeration) is not implemented within the
+kernel.
+
+If unsure, say N
\ No newline at end of file
diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index 0deaa93..9dfee62 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -34,4 +34,4 @@ obj-y += rc/
 
 obj-y += common/ platform/ pci/ usb/ mmc/ firewire/ spi/
 obj-$(CONFIG_VIDEO_DEV) += radio/
-
+obj-$(CONFIG_MEDIA_AVB_ALSA) += avb/
diff --git a/drivers/media/avb/Makefile b/drivers/media/avb/Makefile
new file mode 100644
index 000..5d6302c
--- /dev/null
+++ b/drivers/media/avb/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for the ALSA shim in AVB/TSN
+#
+
+obj-$(CONFIG_MEDIA_AVB_ALSA) += avb_alsa.o
diff --git a/drivers/media/avb/avb_alsa.c b/drivers/media/avb/avb_alsa.c
new file mode 100644
index 000..bd202f5
--- /dev/null
+++ b/drivers/media/avb/avb_alsa.c
@@ -0,0 +1,793 @@
+/* Copyright 2016 Cisco Systems, Inc. and/or its affiliates. All rights
+ * reserved.
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "tsn_iec61883.h"
+
+static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
+static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
+module_param_array(index, int, NULL, 0444);
+MODULE_PARM_DESC(index, "Index value for AVB soundcard.");
+module_param_array(id, charp, NULL, 0444);
+MODULE_PARM_DESC(id, "ID string for AVB soundcard.");
+
+struct avb_chip {
+   struct snd_card *card;
+   struct tsn_link *link;
+   struct snd_pcm *pcm;
+   struct snd_pcm_substream *substream;
+
+   /* Need a reference to this when we unregister the platform
+* driver.
+*/
+   struct platform_device *device;
+
+   /* on first copy, we set a few values, use this to make sure we
+* only do this once.
+*/
+   u8 first_copy;
+
+   u8 sample_size;
+   u8 channels;
+
+   /* current idx in 10ms set of frames
+* class A: 80
+* class B: 40
+*
+* This is mostly relevant for 44.1kHz samplefreq
+*/
+   u8 num_10ms_series;
+
+   u32 sample_freq;
+};
+
+/* currently, only playback is implemented in TSN layer
+ *
+
+ * FIXMEs: (should be set according to the active TSN link)
+ * - format
+ * - rates
+ * - channels
+ *
+ * 

Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Laurent Pinchart]

Hi Shuah,

On Thursday 15 Dec 2016 07:56:55 Shuah Khan wrote:
> On 12/15/2016 03:39 AM, Laurent Pinchart wrote:
> > On Tuesday 13 Dec 2016 15:23:53 Shuah Khan wrote:

[snip]

> >> Please don't pursue this RFC series that makes mc-core changes until
> >> ompa3 driver problems are addressed. There is no need to change the
> >> core unless it is necessary.
> > 
> > It is necessary as has been explained countless times, and will become
> > more and more necessary as media_device instances get shared between
> > multiple drivers, which is currently attempted *without* reference
> > counting.
> 
> You are probably forgetting the Media Device Allocator API work I did
> to make media_device sharable across media and audio drivers.

I haven't. How could I forget it ? :-) Media device sharing is important, and 
will become even more so in the future.

> Sakari's patches don't address the sharable need.

That's correct.

> I have been asking Sakari to use Media Device Allocator API in his patch
> series for allocating media device.

That's where I disagree. The more we dig the more we realize that the current 
infrastructure is broken. Adding anything on top of a construction that is on 
the verge of collapsing isn't a good idea until the foundations have been 
fixed and consolidated.

> I discussed the conflicts between the work I am doing and Sakari's series
> to find a common ground. But it doesn't look like we are going to get there.

-- 
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: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Laurent Pinchart]

Hi Mauro,

On Thursday 15 Dec 2016 12:32:07 Mauro Carvalho Chehab wrote:
> Em Thu, 15 Dec 2016 15:03:36 +0100 Hans Verkuil escreveu:
> > On 15/12/16 13:56, Laurent Pinchart wrote:
> >> On Thursday 15 Dec 2016 13:30:41 Sakari Ailus wrote:
> >>> On Tue, Dec 13, 2016 at 10:24:47AM -0200, Mauro Carvalho Chehab wrote:
>  Em Tue, 13 Dec 2016 12:53:05 +0200 Sakari Ailus escreveu:
> > On Tue, Nov 29, 2016 at 09:13:05AM -0200, Mauro Carvalho Chehab wrote:
> >> Hi Sakari,
> >> 
> >> There's plenty of way to try and work around the problem in drivers,
> >> some more racy than others, but if we require changes to all platform
> >> drivers to fix this we need to ensure that we get it right, not as
> >> half-baked hacks spread around the whole subsystem.
> > 
> > Why on earth do we want this for the omap3 driver? It is not a
> > hot-pluggable device and I see no reason whatsoever to start modifying
> > platform drivers just because you can do an unbind. I know there are real
> > hot-pluggable devices, and getting this right for those is of course
> > important.
> 
> That's indeed a very good point. If unbind is not needed by any usecase,
> the better fix for OMAP3 would be to just prevent it to happen in the first
> place.

There are several reasons to implement proper unbind support in the omap3isp 
driver. Sakari has outlined them in another e-mail in this thread, I won't 
copy them here to avoid splitting the discussion.

>  The USB subsystem has a a .disconnect() callback that notifies
>  the drivers that a device was unbound (likely physically removed).
>  The way USB media drivers handle it is by returning -ENODEV to any
>  V4L2 call that would try to touch at the hardware after unbound.
> > 
> > In my view the main problem is that the media core is bound to a struct
> > device set by the driver that creates the MC. But since the MC gives an
> > overview of lots of other (sub)devices the refcount of the media device
> > should be increased for any (sub)device that adds itself to the MC and
> > decreased for any (sub)device that is removed. Only when the very last
> > user goes away can the MC memory be released.
> > 
> > The memory/refcounting associated with device nodes is unrelated to this:
> > once a devnode is unregistered it will be removed in /dev, and once the
> > last open fh closes any memory associated with the devnode can be
> > released. That will also decrease the refcount to its parent device.
> > 
> > This also means that it is a bad idea to embed devnodes in a larger
> > struct. They should be allocated and freed when the devnode is
> > unregistered and the last open filehandle is closed.
> > 
> > Then the parent's device refcount is decreased, and that may now call its
> > release callback if the refcount reaches 0.
> > 
> > For the media controller's device: any other device driver that needs
> > access to it needs to increase that device's refcount, and only when
> > those devices are released will they decrease the MC device's refcount.
> > 
> > And when that refcount goes to 0 can we finally free everything.
> > 
> > With regards to the opposition to reverting those initial patches, I'm
> > siding with Greg KH. Just revert the bloody patches. It worked most of the
> > time before those patches, so reverting really won't cause bisect
> > problems.
> 
> You're contradicting yourself here ;)
> 
> The patches that this patch series is reverting are the ones that
> de-embeeds devnode struct and fixes its lifecycle.
>
> Reverting those patches will cause regressions on hot-pluggable drivers,
> preventing them to be unplugged. So, if we're willing to revert, then we
> should also revert MC support on them.
> 
> > Just revert and build up things as they should.
> > 
> > Note that v4l2-dev.c doesn't do things correctly (it doesn't set the cdev
> > parent pointer for example) and many drivers (including omap3isp) embed
> > video_device, which is wrong and can lead to complications.
> > 
> > I'm to blame for the embedding since I thought that was a good idea at one
> > time. I now realized that it isn't. Sorry about that...
> > 
> > And because the cdev of the video_device doesn't know about the parent
> > device it is (I think) possible that the parent device is released before
> > the cdev is released. Which can result in major problems.
> 
> I agree with you here. IMHO, de-embeeding cdev's struct from video_device
> seems to be the right thing to do at V4L2 side too.

I believe Hans' comment about embedded devnodes in larger structures referred 
to embedded video_device and media_device inside driver private structures. 
And even in that case I'm not convinced. I've replied to that in another part 
of the mail thread, let's keep the discussion there.

-- 
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  

Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Laurent Pinchart]

On Thursday 15 Dec 2016 13:45:52 Mauro Carvalho Chehab wrote:
> Em Thu, 15 Dec 2016 15:45:22 +0100
> 
> Hans Verkuil  escreveu:
> > On 15/12/16 15:32, Mauro Carvalho Chehab wrote:
> > > Em Thu, 15 Dec 2016 15:03:36 +0100
> > > 
> > > Hans Verkuil  escreveu:
> > >> On 15/12/16 13:56, Laurent Pinchart wrote:
> > >>> Hi Sakari,
> > >>> 
> > >>> On Thursday 15 Dec 2016 13:30:41 Sakari Ailus wrote:
> >  On Tue, Dec 13, 2016 at 10:24:47AM -0200, Mauro Carvalho Chehab 
wrote:
> > > Em Tue, 13 Dec 2016 12:53:05 +0200 Sakari Ailus escreveu:
> > >> On Tue, Nov 29, 2016 at 09:13:05AM -0200, Mauro Carvalho Chehab 
wrote:
> > >>> Hi Sakari,
> > >>> 
> > >>> There's plenty of way to try and work around the problem in drivers,
> > >>> some more racy than others, but if we require changes to all platform
> > >>> drivers to fix this we need to ensure that we get it right, not as
> > >>> half-baked hacks spread around the whole subsystem.
> > >> 
> > >> Why on earth do we want this for the omap3 driver? It is not a
> > >> hot-pluggable device and I see no reason whatsoever to start modifying
> > >> platform drivers just because you can do an unbind. I know there are
> > >> real hot-pluggable devices, and getting this right for those is of
> > >> course important.
> > > 
> > > That's indeed a very good point. If unbind is not needed by any usecase,
> > > the better fix for OMAP3 would be to just prevent it to happen in the
> > > first
> > > place.
> > > 
> > > The USB subsystem has a a .disconnect() callback that notifies
> > > the drivers that a device was unbound (likely physically removed).
> > > The way USB media drivers handle it is by returning -ENODEV to any
> > > V4L2 call that would try to touch at the hardware after unbound.
> > >> 
> > >> In my view the main problem is that the media core is bound to a struct
> > >> device set by the driver that creates the MC. But since the MC gives an
> > >> overview of lots of other (sub)devices the refcount of the media device
> > >> should be increased for any (sub)device that adds itself to the MC and
> > >> decreased for any (sub)device that is removed. Only when the very last
> > >> user goes away can the MC memory be released.
> > >> 
> > >> The memory/refcounting associated with device nodes is unrelated to
> > >> this:
> > >> once a devnode is unregistered it will be removed in /dev, and once the
> > >> last open fh closes any memory associated with the devnode can be
> > >> released.
> > >> That will also decrease the refcount to its parent device.
> > >> 
> > >> This also means that it is a bad idea to embed devnodes in a larger
> > >> struct.
> > >> They should be allocated and freed when the devnode is unregistered and
> > >> the last open filehandle is closed.
> > >> 
> > >> Then the parent's device refcount is decreased, and that may now call
> > >> its
> > >> release callback if the refcount reaches 0.
> > >> 
> > >> For the media controller's device: any other device driver that needs
> > >> access to it needs to increase that device's refcount, and only when
> > >> those devices are released will they decrease the MC device's
> > >> refcount.
> > >> 
> > >> And when that refcount goes to 0 can we finally free everything.
> > >> 
> > >> With regards to the opposition to reverting those initial patches, I'm
> > >> siding with Greg KH. Just revert the bloody patches. It worked most of
> > >> the
> > >> time before those patches, so reverting really won't cause bisect
> > >> problems.
> > > 
> > > You're contradicting yourself here ;)
> > > 
> > > The patches that this patch series is reverting are the ones that
> > > de-embeeds devnode struct and fixes its lifecycle.
> > > 
> > > Reverting those patches will cause regressions on hot-pluggable drivers,
> > > preventing them to be unplugged. So, if we're willing to revert, then we
> > > should also revert MC support on them.
> > 
> > Two options:
> > 
> > 1) Revert, then build up a proper solution.
> 
> Reverting is a regression, as we'll strip off the MC support from the
> existing devices. We would also need to revert a lot more than just those
> 3 patches.

It's not a regression for all the drivers that were already broken before. It 
can be considered as a regression for the drivers that have been broken 
afterwards (as far as I understand that's several USB drivers that you and 
Shuah have migrated to MC in the past few months, but I haven't followed 
driver work closely enough to name them), and I would certainly not oppose to 
additional patches being reverted for those drivers.

> > 2) Do a big-bang patch switching directly over to the new solution, but
> > that's very hard to review.
> > 2a) Post the patch series in small chunks on the mailinglist (starting
> > with the reverts), but once we're all happy merge that patch series into
> > a single big-bang patch and apply that.
> 
> We could do that, but so far, what has been submitted 

Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Laurent Pinchart]

Hi Hans,

On Friday 16 Dec 2016 17:07:23 Sakari Ailus wrote:
> On Thu, Dec 15, 2016 at 03:03:36PM +0100, Hans Verkuil wrote:
> > On 15/12/16 13:56, Laurent Pinchart wrote:
> >> On Thursday 15 Dec 2016 13:30:41 Sakari Ailus wrote:
> >>> On Tue, Dec 13, 2016 at 10:24:47AM -0200, Mauro Carvalho Chehab wrote:

[snip]

>  It is not clear from the logs what the driver tried to do, but
>  that sounds like a driver's bug, with was not prepared to properly
>  handle unbinds.
> 
>  The problem here is that isp_video_release() is called by V4L2
>  release logic, and not by the MC one:
> 
>  static const struct v4l2_file_operations isp_video_fops = {
>   .owner  = THIS_MODULE,
>   .open   = isp_video_open,
>   .release= isp_video_release,
>   .poll   = vb2_fop_poll,
>   .unlocked_ioctl = video_ioctl2,
>   .mmap   = vb2_fop_mmap,
> };
> 
>  It seems that the driver's logic allows it to be called before or
>  after destroying the MC.
> 
>  Assuming that, if the OMAP3 driver is not used it works,
>  it means that, if the isp_video_release() is called
>  first, no errors will happen, but if MC is destroyed before
>  V4L2 call to its .release() callback, as there's no logic at the
>  driver that would detect it, isp_video_release() will be calling
>  isp_video_streamoff(), with depends on the MC to work.
> 
>  On a first glance, I can see two ways of fixing it:
> 
>  1) to increment devnode's device kobject refcount at OMAP3 .probe(),
>  decrementing it only at isp_video_release(). That will ensure that
>  MC will only be removed after V4L2 removal.
> >>
> >> As soon as you have to dig deep in a structure to find a reference
> >> counter and increment it, bypassing all the API layers, you can be
> >> entirely sure that the solution is wrong.
> > 
> > Indeed.
> > 
>  2) to call isp_video_streamoff() before removing the MC stuff, e. g.
>  inside the MC .release() callback.
> >>>
> >>> This is a fair suggestion, indeed. Let me see what could be done there.
> >>> Albeit this is just *one* of the existing issues. It will not address
> >>> all problems fixed by the patchset.
> >>
> >> We need to stop the hardware at .remove() time. That should not be linked
> >> to a videodev, v4l2_device or media_device .release() callback. When the
> >> .remove() callback returns the driver is not allowed to touch the
> >> hardware anymore. In particular, power domains might clocks or power
> >> supplies, leading to invalid access faults if we try to access hardware
> >> registers.
> > 
> > Correct.
> > 
> >> USB devices get help from the USB core that cancels all USB operations
> >> in progress when they're disconnected. Platform devices don't have it as
> >> easy, and need to implement everything themselves. We thus need to stop
> >> the hardware, but I'm not sure it makes sense to fake a VIDIOC_STREAMOFF
> >> ioctl at .remove() time.
> > 
> > Please don't. This shouldn't be done automatically.
> > 
> >> That could introduce other races between .remove() and the
> >> userspace API. A better solution is to make sure the objects that are
> >> needed at .release() time of the device node are all reference-counted
> >> and only released when the last reference goes away.
> >>
> >> There's plenty of way to try and work around the problem in drivers, some
> >> more racy than others, but if we require changes to all platform drivers
> >> to fix this we need to ensure that we get it right, not as half-baked
> >> hacks spread around the whole subsystem.
> > 
> > Why on earth do we want this for the omap3 driver? It is not a
> > hot-pluggable device and I see no reason whatsoever to start modifying
> > platform drivers just because you can do an unbind. I know there are real
> > hot-pluggable devices, and getting this right for those is of course
> > important.
> > 
> > If the omap3 is used as a testbed, then that's fine by me, but even then I
> > probably wouldn't want the omap3 code that makes this possible in the
> > kernel. It's just additional code for no purpose.
> 
> The same problems exist on other devices, whether platform, pci or USB, as
> the problems are in the core frameworks rather than (only) in the drivers.
> 
> On platform devices, this happens also when removing the module.
> 
> I've used omap3isp as an example since it demonstrates well the problems and
> a lot of people have the hardware as well. Also, Mauro has requested all
> drivers to be converted to the new API. I'm fine doing that for the
> actually hot-pluggable hardware.
> 
> One additional reason is that as the omap3isp driver has been used as an
> example to write a number of other drivers, people do see what's the right
> way to do these things, instead of copying code from a driver doing it
> wrong.

That's a very important reason in my opinion. If we design core code properly 

Re: [PATCH RFC] omap3isp: prevent releasing MC too early

[Laurent Pinchart]

Hi Mauro,

On Friday 16 Dec 2016 09:18:50 Mauro Carvalho Chehab wrote:
> Em Thu, 15 Dec 2016 16:04:51 +0200 Laurent Pinchart escreveu:
> 
> We have now two threads discussing the same subject, which is bad, as
> we'll end repeating the same arguments on different threads...
> 
> Let's use the "[PATCH RFC 00/21]" for those discussions, as it seems we're
> reaching to somewhere there.
> 
> > Even if you're not entirely convinced by the reasons
> > explained in this mail thread, remember that we will need sooner or later
> > to implement support for media graph update at runtime. Refcounting will
> > be needed, let's design it in the cleanest possible way.
> 
> As I said, I'm not against using some other approach and even
> adding refcounting to each graph object.
> 
> What I am against is on a patchset that starts by breaking
> the USB drivers that use the media controller.

So, what you're essentially saying, is that you noticed we have a problem in 
the core when trying to add MC support to a bunch of USB driver. Instead of 
fixing the problem properly, you've merged 3 patches that work around part of 
the issue, despite negative comments received by the original authors of the 
code, and then added a bunch of code to the USB drivers that make them subject 
to the race condition. And you're then claiming that we can't revert the 
patches that we know from the start were broken because you piled additional 
patches on top of them, making the end result worse ? Sorry, I can't buy that. 
If you really insist we can also revert the series that add MC support to the 
USB drivers, but there's no way that your decision to ignore known issues can 
ever be considered as an excuse to not revert broken changes.

This discussion is over as far as I'm concerned. The 3 patches in question are 
wrong. I want the proper fixes to be merged, and we thus all need to work in 
that direction, which means reviewing them. Once we agree on what the end 
result should be we'll see whether we could possibly rework the code in a way 
that doesn't require a revert. If that's not possible, we'll revert what is 
broken. It's as simple as that. Now, let's get technical and fix this crap. If 
I had wanted a show I would have bought tickets to the circus.

> Btw, I'm starting to suspect that getting rid of devm_*alloc()
> on OMAP3, as proposed by the 00/21 thread is addressing a symptom of
> the problem and not a cause, and that using get_device()/put_device()
> may help fixing such issues. See Hans comments on that thread.

-- 
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: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Sakari Ailus]

Hi Hans,

On Thu, Dec 15, 2016 at 03:03:36PM +0100, Hans Verkuil wrote:
> On 15/12/16 13:56, Laurent Pinchart wrote:
> >Hi Sakari,
> >
> >On Thursday 15 Dec 2016 13:30:41 Sakari Ailus wrote:
> >>On Tue, Dec 13, 2016 at 10:24:47AM -0200, Mauro Carvalho Chehab wrote:
> >>>Em Tue, 13 Dec 2016 12:53:05 +0200 Sakari Ailus escreveu:
> On Tue, Nov 29, 2016 at 09:13:05AM -0200, Mauro Carvalho Chehab wrote:
> >Hi Sakari,
> >
> >I answered you point to point below, but I suspect that you missed how
> >the current approach works. So, I decided to write a quick summary
> >here.
> >
> >The character devices /dev/media? are created via cdev, with relies on
> >a kobject per device, with has an embedded struct kref inside.
> >
> >Also, each kobj at /dev/media0, /dev/media1, ... is associated with a
> >struct device, when the code does:
> >  devnode->cdev.kobj.parent = >dev.kobj;
> >
> >before calling cdev_add().
> >
> >The current lifetime management is actually based on cdev's kobject's
> >refcount, provided by its embedded kref.
> >
> >The kref warrants that any data associated with /dev/media0 won't be
> >freed if there are any pending system call. In other words, when
> >cdev_del() is called, it will remove /dev/media0 from the filesystem,
> >and will call kobject_put().
> >
> >If the refcount is zero, it will call devnode->dev.release(). If the
> >kobject refcount is not zero, the data won't be freed.
> >
> >So, in the best case scenario, there's no opened file descriptors
> >by the time media device node is unregistered. So, it will free
> >everything.
> >
> >In the worse case scenario, e. g. when the driver is removed or
> >unbind while /dev/media0 has some opened file descriptor(s),
> >the cdev logic will do the proper lifetime management.
> >
> >On such case, /dev/media0 disappears from the file system, so another
> >open is not possible anymore. The data structures will remain
> >allocated until all associated file descriptors are not closed.
> >
> >When all file descriptors are closed, the data will be freed.
> >
> >On that time, it will call an optional dev.release() callback,
> >responsible to free any other data struct that the driver allocated.
> 
> The patchset does not change this. It's not a question of the
> media_devnode struct either. That's not an issue.
> 
> The issue is rather what else can be accessed through the media device
> and other interfaces. As IOCTLs are not serialised with device removal
> (which now releases much of the data structures)
> >>>
> >>>Huh? ioctls are serialized with struct device removal. The Driver core
> >>>warrants that.
> >>
> >>How?
> >>
> >>As far as I can tell, there's nothing in the way of an IOCTL being in
> >>progress on a character device which is registered by the driver for a
> >>hardware device which is being removed.
> >>
> >>vfs_ioctl() directly calls the unlocked_ioctl() file operation which is, in
> >>case of MC, media_ioctl() in media-devnode.c. No mutexes (or other locks)
> >>are taken during that path, which I believe is by design.
> 
> chrdev_open in fs/char_dev.c increases the refcount on open() and decreases it
> on release(). Thus ensuring that the cdev can never be removed while in an
> ioctl.

It does, but it does not affect memory which is allocated separately of that.

See this:



> 
> >>
> there's a high chance of accessing
> released memory (or mutexes that have been already destroyed). An
> example of that is here, stopping a running pipeline after unbinding
> the device. What happens there is that the media device is released
> whilst it's in use through the video device.
> 
> 
> >>>
> >>>It is not clear from the logs what the driver tried to do, but
> >>>that sounds like a driver's bug, with was not prepared to properly
> >>>handle unbinds.
> >>>
> >>>The problem here is that isp_video_release() is called by V4L2
> >>>release logic, and not by the MC one:
> >>>
> >>>static const struct v4l2_file_operations isp_video_fops = {
> >>>  .owner  = THIS_MODULE,
> >>>  .open   = isp_video_open,
> >>>  .release= isp_video_release,
> >>>  .poll   = vb2_fop_poll,
> >>>  .unlocked_ioctl = video_ioctl2,
> >>>  .mmap   = vb2_fop_mmap,
> >>>};
> >>>
> >>>It seems that the driver's logic allows it to be called before or
> >>>after destroying the MC.
> >>>
> >>>Assuming that, if the OMAP3 driver is not used it works,
> >>>it means that, if the isp_video_release() is called
> >>>first, no errors will happen, but if MC is destroyed before
> >>>V4L2 call to its .release() callback, as there's no logic at the
> >>>driver that would 

Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Hans Verkuil]

On 16/12/16 13:00, Mauro Carvalho Chehab wrote:

Em
 escreveu:


On 16/12/16 11:57, Mauro Carvalho Chehab wrote:

Em Fri, 16 Dec 2016 11:11:25 +0100
Hans Verkuil  escreveu:


Would it make sense to enforce that dependency. Can we tie /dev/media usecount
to /dev/video etc. usecount? In other words:

/dev/video is opened, then open /dev/media.


When a device node is registered it should increase the refcount on the 
media_device
(as I proposed, that would be mdev->dev). When a device node is unregistered 
and the
last user disappeared, then it can decrease the media_device refcount.

So as long as anyone is using a device node, the media_device will stick around 
as
well.

No need to take refcounts on open/close.


That makes sense. You're meaning something like the enclosed (untested)
patch?


One note: as I mentioned, the video_device does not set the cdev parent 
correctly,
so that bug needs to be fixed first for this to work.


Actually, __video_register_device() seems to be setting the parent
properly:

if (vdev->dev_parent == NULL)
vdev->dev_parent = vdev->v4l2_dev->dev;


No, I mean this code (from cec-core.c):


/* Part 2: Initialize and register the character device */
 cdev_init(>cdev, _devnode_fops);
 devnode->cdev.kobj.parent = >dev.kobj;
 devnode->cdev.owner = owner;

 ret = cdev_add(>cdev, devnode->dev.devt, 1);
 if (ret < 0) {
 pr_err("%s: cdev_add failed\n", __func__);
 goto clr_bit;
 }

 ret = device_add(>dev);
 if (ret)
 goto cdev_del;

which sets cdev.kobj.parent. And that would indeed be vdev->dev_parent.


Ah! So, you're basically proposing to have a separate struct for
V4L2 devnode as well, right?

Makes sense.


No need for that, that's already struct video_device.







Thanks,
Mauro

[PATCH] Be sure that the media_device won't be freed too early

This code snippet is untested.

Signed-off-by: Mauro Carvalho chehab 

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 8756275e9fc4..5fdeab382069 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -706,7 +706,7 @@ int __must_check __media_device_register(struct 
media_device *mdev,
struct media_devnode *devnode;
int ret;

-   devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
+   devnode = devm_kzalloc(mdev->dev, sizeof(*devnode), GFP_KERNEL);


I'm not sure about this change. I *think* this would work, but *only* if all
the refcounting is 100% correct, and we know it isn't at the moment. So I
think this should be postponed until we are confident everything is correct.


Yes, such change will require first to be sure that drivers would be
doing the right thing.


So devm_ resources are released right after remove() exits, not when the last 
reference
goes to 0. In other words, devm_ typically can't be used for these complex scenarios, 
certainly not for memory. See discussion with Laurent on irc.







if (!devnode)
return -ENOMEM;

diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
b/drivers/media/v4l2-core/v4l2-dev.c
index 8be561ab2615..14a3c56dbcac 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -196,6 +196,7 @@ static void v4l2_device_release(struct device *cd)
 #if defined(CONFIG_MEDIA_CONTROLLER)
if (v4l2_dev->mdev) {
/* Remove interfaces and interface links */
+   put_device(v4l2_dev->mdev->dev);
media_devnode_remove(vdev->intf_devnode);
if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
media_device_unregister_entity(>entity);


I think this is the wrong order: put_device should go after 
media_device_unregister_entity().


OK.




@@ -810,6 +811,7 @@ static int video_register_media_controller(struct 
video_device *vdev, int type)
return -ENOMEM;
}
}
+   get_device(vdev->v4l2_dev->dev);


You mean v4l2_dev->mdev->dev?


Yes, that's right (vdev->v4l2_dev->mdev->dev).


Laurent helped me realize that this won't work either: mdev->dev is typically a 
platform/pci/usb device, and that won't go away when you rmmod the driver.


So while taking a refcount on that device doesn't hurt, we also need to take a 
refcount
on a kref inside the mdev. Just like v4l2_device this struct has an unfortunate 
name.
It's not a device, but a root structure for media devices.

We really need a whiteboard for this :-(

Regards,

Hans







/* FIXME: how to create the other interface links? */

@@ -1015,6 +1017,11 @@ void video_unregister_device(struct video_device *vdev)
if (!vdev || !video_is_registered(vdev))
return;

+#if defined(CONFIG_MEDIA_CONTROLLER)
+   if (vdev->v4l2_dev->dev)
+   put_device(vdev->v4l2_dev->dev);

Re: [RFC v3 21/21] omap3isp: Don't rely on devm for memory resource management

[Shuah Khan]

On 12/16/2016 06:32 AM, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Thu, Dec 15, 2016 at 01:23:50PM +0200, Laurent Pinchart wrote:
>>> @@ -1596,7 +1604,6 @@ static void isp_unregister_entities(struct isp_device
>>> *isp) omap3isp_stat_unregister_entities(>isp_af);
>>> omap3isp_stat_unregister_entities(>isp_hist);
>>>
>>> -   v4l2_device_unregister(>v4l2_dev);
>>
>> This isn't correct. The v4l2_device instance should be unregistered here, to 
>> make sure that the subdev nodes are unregistered too. And even if moving the 
>> function call was correct, it should be done in a separate patch as it's 
>> unrelated to $SUBJECT.
> 
> I think I tried to fix another problem here we haven't considered before,
> which is that v4l2_device_unregister() also unregisters the entities through
> media_device_unregister_entity(). This will set the media device of the
> graph objects NULL.
> 
> I'll see whether something could be done to that.
> 

Right That is what I was pointing out, when I said the cleanup routines are
done in the wrong place. Entity registration and unregistration are distributed
in nature. v4l2 register will register entities and unregister will unregister
its entities. dvb will do the same.

So essentially entities get added and removed when any of these drivers get
unbound. Please see the following I posted on

[RFC v3 00/21] Make use of kref in media device, grab references as needed

> v4l2-core registers and unregisters entities and so does dvb-core. So when a
> driver unregisters video and dvb, these entities get deleted. So we have a
> distributed mode of registering and unregistering entities. We also have
> ioctls (video, dvb, and media) accessing these entities. So where do we make
> changes to ensure entities stick around until all users exit?
>
> Ref-counting entities won't work if they are embedded - like in the case of
> struct video_device which embeds the media entity. When struct video goes
> away then entity will disappear. So we do have a complex lifetime model here
> that we need to figure out how to fix.

thanks,
-- Shuah
--
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 v5 2/6] [media] rc-main: split setup and unregister functions

[Sean Young]

Hi Andi,

On Fri, Dec 16, 2016 at 12:10:26PM +, Sean Young wrote:
> Sorry to add to your woes, but there are some checkpatch warnings and
> errors. Please can you correct these. One is below.

Actually, the changes are pretty minor, I can fix them up before sending
them to Mauro. Sorry for bothering you.


Sean
--
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 v1] [media] v4l2-common: fix aligned value calculation

[Sakari Ailus]

Hi Jean-Christophe,

On Fri, Dec 16, 2016 at 02:32:15PM +0100, Jean-Christophe Trotin wrote:
> Correct the calculation of the rounding to nearest aligned value in
> the clamp_align() function. For example, clamp_align(1277, 1, 9600, 2)
> returns 1276, while it should return 1280.

Why should the function return 1280 instead of 1276, which is closer to
1277?

> 
> Signed-off-by: Jean-Christophe Trotin 
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c 
> b/drivers/media/v4l2-core/v4l2-common.c
> index 57cfe26a..2970ce7 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -315,7 +315,7 @@ static unsigned int clamp_align(unsigned int x, unsigned 
> int min,
>  
>   /* Round to nearest aligned value */
>   if (align)
> - x = (x + (1 << (align - 1))) & mask;
> + x = (x + ((1 << align) - 1)) & mask;
>  
>   return x;
>  }

-- 
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

Re: [RFC v3 21/21] omap3isp: Don't rely on devm for memory resource management

[Sakari Ailus]

Hi Laurent,

On Thu, Dec 15, 2016 at 01:23:50PM +0200, Laurent Pinchart wrote:
> > @@ -1596,7 +1604,6 @@ static void isp_unregister_entities(struct isp_device
> > *isp) omap3isp_stat_unregister_entities(>isp_af);
> > omap3isp_stat_unregister_entities(>isp_hist);
> > 
> > -   v4l2_device_unregister(>v4l2_dev);
> 
> This isn't correct. The v4l2_device instance should be unregistered here, to 
> make sure that the subdev nodes are unregistered too. And even if moving the 
> function call was correct, it should be done in a separate patch as it's 
> unrelated to $SUBJECT.

I think I tried to fix another problem here we haven't considered before,
which is that v4l2_device_unregister() also unregisters the entities through
media_device_unregister_entity(). This will set the media device of the
graph objects NULL.

I'll see whether something could be done to that.

-- 
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 v1] [media] v4l2-common: fix aligned value calculation

[Jean-Christophe Trotin]

Correct the calculation of the rounding to nearest aligned value in
the clamp_align() function. For example, clamp_align(1277, 1, 9600, 2)
returns 1276, while it should return 1280.

Signed-off-by: Jean-Christophe Trotin 
---
 drivers/media/v4l2-core/v4l2-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c 
b/drivers/media/v4l2-core/v4l2-common.c
index 57cfe26a..2970ce7 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -315,7 +315,7 @@ static unsigned int clamp_align(unsigned int x, unsigned 
int min,
 
/* Round to nearest aligned value */
if (align)
-   x = (x + (1 << (align - 1))) & mask;
+   x = (x + ((1 << align) - 1)) & mask;
 
return x;
 }
-- 
1.9.1

--
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 2/2] media: omap3isp change to devm for resources

[Hans Verkuil]

On 16/12/16 13:19, Laurent Pinchart wrote:

Hi Hans,

On Friday 16 Dec 2016 12:39:49 Hans Verkuil wrote:

On 15/12/16 20:40, Shuah Khan wrote:

Using devm resources that have external dependencies such as a dev
for a file handler could result in devm resources getting released
durin unbind while an application has the file open holding pointer
to the devm resource. This results in use-after-free errors when the
application exits.


That's solving the wrong problem.

The real problem is that when registering a video_device it should do
this:

devnode->cdev.kobj.parent = >dev.kobj;

(taken from cec-core.c)

This will prevent isp->dev from being released as long as there is a
filehandle still open.


But it won't be enough, devm_* resources are released at unbind time, not at
device release time. Right after the unbind (.remove() for platform devices)
handler returns, devm_kzalloc allocated memory goes away.


You're completely right, I keep forgetting about that.

Sorry for the noise.

Hans




After that change I believe that this will work correctly, but this
has to be tested first!




--
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] vim2m: Clean up file handle in open() error path.

[Laurent Pinchart]

On Friday 16 Dec 2016 14:31:15 santosh kumar singh wrote:
> Dear Mr. laurent,
> 
> Can you please check the patch submitted by me.

No, because I haven't received it :-) You've sent it as an HTML message it it 
thus got dropped by the mailing list. Please don't send HTML e-mails.

While at it, when submitting a new version of a patch, please indicate that in 
the subject line. It should have been [PATCH v2]. Otherwise it's difficult for 
us to figure out if the patch is a duplicate or a new version, or to find the 
latest version.

-- 
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 2/2] media: omap3isp change to devm for resources

[Laurent Pinchart]

Hi Hans,

On Friday 16 Dec 2016 12:39:49 Hans Verkuil wrote:
> On 15/12/16 20:40, Shuah Khan wrote:
> > Using devm resources that have external dependencies such as a dev
> > for a file handler could result in devm resources getting released
> > durin unbind while an application has the file open holding pointer
> > to the devm resource. This results in use-after-free errors when the
> > application exits.
> 
> That's solving the wrong problem.
> 
> The real problem is that when registering a video_device it should do
> this:
> 
> devnode->cdev.kobj.parent = >dev.kobj;
> 
> (taken from cec-core.c)
> 
> This will prevent isp->dev from being released as long as there is a
> filehandle still open.

But it won't be enough, devm_* resources are released at unbind time, not at 
device release time. Right after the unbind (.remove() for platform devices) 
handler returns, devm_kzalloc allocated memory goes away.

> After that change I believe that this will work correctly, but this
> has to be tested first!

-- 
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 v5 2/6] [media] rc-main: split setup and unregister functions

[Sean Young]

Hi Andi,

Sorry to add to your woes, but there are some checkpatch warnings and
errors. Please can you correct these. One is below.

Thanks
Sean

On Fri, Dec 16, 2016 at 03:12:14PM +0900, Andi Shyti wrote:
> Move the input device allocation, map and protocol handling to
> different functions.
> 
> Signed-off-by: Andi Shyti 
> Reviewed-by: Sean Young 
> ---
>  drivers/media/rc/rc-main.c | 143 
> +
>  1 file changed, 81 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index a6bbceb..7cc700d 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1436,16 +1436,12 @@ struct rc_dev *devm_rc_allocate_device(struct device 
> *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
>  
> -int rc_register_device(struct rc_dev *dev)
> +static int rc_setup_rx_device(struct rc_dev *dev)
>  {
> - static bool raw_init = false; /* raw decoders loaded? */
> - struct rc_map *rc_map;
> - const char *path;
> - int attr = 0;
> - int minor;
>   int rc;
> + struct rc_map *rc_map;
>  
> - if (!dev || !dev->map_name)
> + if (!dev->map_name)
>   return -EINVAL;
>  
>   rc_map = rc_map_get(dev->map_name);
> @@ -1454,6 +1450,19 @@ int rc_register_device(struct rc_dev *dev)
>   if (!rc_map || !rc_map->scan || rc_map->size == 0)
>   return -EINVAL;
>  
> + rc = ir_setkeytable(dev, rc_map);
> + if (rc)
> + return rc;
> +
> + if (dev->change_protocol) {
> + u64 rc_type = (1ll << rc_map->rc_type);
> +
> + rc = dev->change_protocol(dev, _type);
> + if (rc < 0)
> + goto out_table;
> + dev->enabled_protocols = rc_type;
> + }
> +
>   set_bit(EV_KEY, dev->input_dev->evbit);
>   set_bit(EV_REP, dev->input_dev->evbit);
>   set_bit(EV_MSC, dev->input_dev->evbit);
> @@ -1463,6 +1472,61 @@ int rc_register_device(struct rc_dev *dev)
>   if (dev->close)
>   dev->input_dev->close = ir_close;
>  
> + /*
> +  * Default delay of 250ms is too short for some protocols, especially
> +  * since the timeout is currently set to 250ms. Increase it to 500ms,
> +  * to avoid wrong repetition of the keycodes. Note that this must be
> +  * set after the call to input_register_device().
> +  */
> + dev->input_dev->rep[REP_DELAY] = 500;
> +
> + /*
> +  * As a repeat event on protocols like RC-5 and NEC take as long as
> +  * 110/114ms, using 33ms as a repeat period is not the right thing
> +  * to do.
> +  */
> + dev->input_dev->rep[REP_PERIOD] = 125;
> +
> + dev->input_dev->dev.parent = >dev;
> + memcpy(>input_dev->id, >input_id, sizeof(dev->input_id));
> + dev->input_dev->phys = dev->input_phys;
> + dev->input_dev->name = dev->input_name;
> +
> + /* rc_open will be called here */
> + rc = input_register_device(dev->input_dev);
> + if (rc)
> + goto out_table;
> +
> + return 0;
> +
> +out_table:
> + ir_free_table(>rc_map);
> +
> + return rc;
> +}
> +
> +static void rc_free_rx_device(struct rc_dev *dev)
> +{
> + if (!dev)
> + return;
> +
> + ir_free_table(>rc_map);
> +
> + input_unregister_device(dev->input_dev);
> + dev->input_dev = NULL;
> +}
> +
> +int rc_register_device(struct rc_dev *dev)
> +{
> + static bool raw_init = false; /* raw decoders loaded? */

ERROR: do not initialise statics to false
#110: FILE: drivers/media/rc/rc-main.c:1741:
+   static bool raw_init = false; /* raw decoders loaded? */

> + const char *path;
> + int attr = 0;
> + int minor;
> + int rc;
> +
> + if (!dev)
> + return -EINVAL;
> +
>   minor = ida_simple_get(_ida, 0, RC_DEV_MAX, GFP_KERNEL);
>   if (minor < 0)
>   return minor;
> @@ -1486,39 +1550,15 @@ int rc_register_device(struct rc_dev *dev)
>   if (rc)
>   goto out_unlock;
>  
> - rc = ir_setkeytable(dev, rc_map);
> - if (rc)
> - goto out_dev;
> -
> - dev->input_dev->dev.parent = >dev;
> - memcpy(>input_dev->id, >input_id, sizeof(dev->input_id));
> - dev->input_dev->phys = dev->input_phys;
> - dev->input_dev->name = dev->input_name;
> -
> - rc = input_register_device(dev->input_dev);
> - if (rc)
> - goto out_table;
> -
> - /*
> -  * Default delay of 250ms is too short for some protocols, especially
> -  * since the timeout is currently set to 250ms. Increase it to 500ms,
> -  * to avoid wrong repetition of the keycodes. Note that this must be
> -  * set after the call to input_register_device().
> -  */
> - dev->input_dev->rep[REP_DELAY] = 500;
> -
> - /*
> -  * As a repeat event on protocols like RC-5 and NEC take as long as
> -  * 110/114ms, using 33ms as a repeat period is 

Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Mauro Carvalho Chehab]

Em 
 escreveu:

> On 16/12/16 11:57, Mauro Carvalho Chehab wrote:
> > Em Fri, 16 Dec 2016 11:11:25 +0100
> > Hans Verkuil  escreveu:
> >  
> >>> Would it make sense to enforce that dependency. Can we tie /dev/media 
> >>> usecount
> >>> to /dev/video etc. usecount? In other words:
> >>>
> >>> /dev/video is opened, then open /dev/media.  
> >>
> >> When a device node is registered it should increase the refcount on the 
> >> media_device
> >> (as I proposed, that would be mdev->dev). When a device node is 
> >> unregistered and the
> >> last user disappeared, then it can decrease the media_device refcount.
> >>
> >> So as long as anyone is using a device node, the media_device will stick 
> >> around as
> >> well.
> >>
> >> No need to take refcounts on open/close.  
> >
> > That makes sense. You're meaning something like the enclosed (untested)
> > patch?
> >  
> >> One note: as I mentioned, the video_device does not set the cdev parent 
> >> correctly,
> >> so that bug needs to be fixed first for this to work.  
> >
> > Actually, __video_register_device() seems to be setting the parent
> > properly:
> >
> > if (vdev->dev_parent == NULL)
> > vdev->dev_parent = vdev->v4l2_dev->dev;  
> 
> No, I mean this code (from cec-core.c):
> 
> 
> /* Part 2: Initialize and register the character device */
>  cdev_init(>cdev, _devnode_fops);
>  devnode->cdev.kobj.parent = >dev.kobj;
>  devnode->cdev.owner = owner;
> 
>  ret = cdev_add(>cdev, devnode->dev.devt, 1);
>  if (ret < 0) {
>  pr_err("%s: cdev_add failed\n", __func__);
>  goto clr_bit;
>  }
> 
>  ret = device_add(>dev);
>  if (ret)
>  goto cdev_del;
> 
> which sets cdev.kobj.parent. And that would indeed be vdev->dev_parent.

Ah! So, you're basically proposing to have a separate struct for
V4L2 devnode as well, right?

Makes sense.

> 
> >
> > Thanks,
> > Mauro
> >
> > [PATCH] Be sure that the media_device won't be freed too early
> >
> > This code snippet is untested.
> >
> > Signed-off-by: Mauro Carvalho chehab 
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 8756275e9fc4..5fdeab382069 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -706,7 +706,7 @@ int __must_check __media_device_register(struct 
> > media_device *mdev,
> > struct media_devnode *devnode;
> > int ret;
> >
> > -   devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
> > +   devnode = devm_kzalloc(mdev->dev, sizeof(*devnode), GFP_KERNEL);  
> 
> I'm not sure about this change. I *think* this would work, but *only* if all
> the refcounting is 100% correct, and we know it isn't at the moment. So I
> think this should be postponed until we are confident everything is correct.

Yes, such change will require first to be sure that drivers would be
doing the right thing.

> 
> > if (!devnode)
> > return -ENOMEM;
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
> > b/drivers/media/v4l2-core/v4l2-dev.c
> > index 8be561ab2615..14a3c56dbcac 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -196,6 +196,7 @@ static void v4l2_device_release(struct device *cd)
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> > if (v4l2_dev->mdev) {
> > /* Remove interfaces and interface links */
> > +   put_device(v4l2_dev->mdev->dev);
> > media_devnode_remove(vdev->intf_devnode);
> > if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
> > media_device_unregister_entity(>entity);  
> 
> I think this is the wrong order: put_device should go after 
> media_device_unregister_entity().

OK.

> 
> > @@ -810,6 +811,7 @@ static int video_register_media_controller(struct 
> > video_device *vdev, int type)
> > return -ENOMEM;
> > }
> > }
> > +   get_device(vdev->v4l2_dev->dev);  
> 
> You mean v4l2_dev->mdev->dev?

Yes, that's right (vdev->v4l2_dev->mdev->dev).

> 
> >
> > /* FIXME: how to create the other interface links? */
> >
> > @@ -1015,6 +1017,11 @@ void video_unregister_device(struct video_device 
> > *vdev)
> > if (!vdev || !video_is_registered(vdev))
> > return;
> >
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +   if (vdev->v4l2_dev->dev)
> > +   put_device(vdev->v4l2_dev->dev);  
> 
> Ditto.
> 
> > +#endif
> > +
> > mutex_lock(_lock);
> > /* This must be in a critical section to prevent a race with v4l2_open.
> >  * Once this bit has been cleared video_get may never be called again.
> > diff --git a/drivers/media/v4l2-core/v4l2-device.c 
> > b/drivers/media/v4l2-core/v4l2-device.c
> > index 62bbed76dbbc..53f42090c762 100644
> > --- a/drivers/media/v4l2-core/v4l2-device.c
> > +++ b/drivers/media/v4l2-core/v4l2-device.c
> > @@ -188,6 +188,7 @@ int 

Re: [PATCH RFC] omap3isp: prevent releasing MC too early

[Sakari Ailus]

Hi folks,

On Fri, Dec 16, 2016 at 10:21:25AM +0200, Sakari Ailus wrote:
...
> At least some of the issues the patches claim to fix are really not fixed
> either. They are just made slightly less likely to accidentally stumble
> upon.

I couldn't immediately trigger this with the current mainline kernel as the
remaining time window isn't very large. However making this trivial-looking
change:

diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index f2772ba..6ec4125 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -32,6 +32,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include 
 #include 
 #include 
 #include 
@@ -128,6 +129,8 @@ __media_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg,
if (!media_devnode_is_registered(devnode))
return -EIO;
 
+   msleep(1000);
+
return ioctl_func(filp, cmd, arg);
 }
 
And calling MEDIA_IOC_ENUM_ENTITIES (for instance) in a loop (v4l-utils):

diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c
index 1fd6525..5be3cea 100644
--- a/utils/media-ctl/libmediactl.c
+++ b/utils/media-ctl/libmediactl.c
@@ -522,7 +522,7 @@ static int media_enum_entities(struct media_device *media)
entity->fd = -1;
entity->info.id = id | MEDIA_ENT_ID_FLAG_NEXT;
entity->media = media;
-
+while (1)
ret = ioctl(media->fd, MEDIA_IOC_ENUM_ENTITIES, >info);
if (ret < 0) {
ret = errno != EINVAL ? -errno : 0;


And then unbinding the omap3-isp driver will immediately produce this. The
same would take place on any driver implementing the Media controller
interface. What happens there is that the memory backing the media device
has been released while the IOCTL call was in progress. This can happen
because device removal and IOCTL calls are not serialised nor the media
device is reference counted --- the latter of which I believe is the right
thing to do:

[  105.756408] Unable to handle kernel NULL pointer dereference at virtual 
address 0140
[  105.756561] pgd = eda94000
[  105.756591] [0140] *pgd=adaf8831, *pte=, *ppte=
[  105.756683] Internal error: Oops: 17 [#1] PREEMPT ARM
[  105.756744] Modules linked in: smiapp smiapp_pll omap3_isp 
videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_core v4l2_common 
videodev media
[  105.756927] CPU: 0 PID: 2276 Comm: media-ctl Not tainted 
4.9.0-rc6-00494-g9244e38-dirty #830
[  105.757019] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[  105.757080] task: ed970380 task.stack: eda5e000
[  105.757141] PC is at __lock_acquire+0x94/0x1868
[  105.757202] LR is at lock_acquire+0x70/0x90
[  105.757263] pc : []lr : []psr: 2093
   sp : eda5fcc0  ip : eda5e000  fp : 
[  105.757354] r10: 0001  r9 :   r8 : 0002715c
[  105.757415] r7 : 0140  r6 : 0001  r5 : 6013  r4 : ed970380
[  105.757476] r3 : 0001  r2 : 0001  r1 :   r0 : 0140
[  105.757537] Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment 
none
[  105.757629] Control: 10c5387d  Table: ada94019  DAC: 0051
[  105.757690] Process media-ctl (pid: 2276, stack limit = 0xeda5e208)
[  105.757751] Stack: (0xeda5fcc0 to 0xeda6)
[  105.757812] fcc0: ed970868 0001 a409df10 c355d08b c0c75714 c01602f4 
6013 0001
[  105.757904] fce0: ee1c9514 c0839b48 ee1c9518 effed360 ed970868 6013 
c0833e74 c0839b48
[  105.757995] fd00: effed360 effed360 c0e9166c ee1c951c ee1c9518  
6013 0001
[  105.758087] fd20:  0002715c eda5e000   c01601ec 
0001 
[  105.758178] fd40:  bf000e78   010c bf000e78 
ed970380 c04ed214
[  105.758270] fd60: 0001  bf000e78 c015aa50 ed9703b0 ed9703b0 
c0830d50 
[  105.758361] fd80: ed970848 c0830d60 ed970380 ed970380 c0830d60 c01602f4 
c0e76744 0051
[  105.758483] fda0: eda5fdd0  0002715c eda5fdd0 bf002e28 c1007c01 
 0002715c
[  105.758575] fdc0: eda5e000   bf000e78 0001 50414d4f 
53492033 43432050
[  105.758666] fde0: 3250     0002 
 
[  105.758758] fe00:  00010002     
0051 0007
[  105.758850] fe20:       
 
[  105.758941] fe40:       
 
[  105.759033] fe60:       
 
[  105.759124] fe80:       
 
[  105.759216] fea0:       
 
[  105.759307] fec0:     ed967c80 bf000ddc 
ed967c80 0002715c
[  105.759399] fee0: c1007c01 0003  bf001808 

Re: [RFC v2 00/11] vb2: Handle user cache hints, allow drivers to choose cache coherency

[Hans Verkuil]

On 16/12/16 02:24, Laurent Pinchart wrote:

Hello,

This is a rebased version of the vb2 cache hints support patch series posted
by Sakari more than a year ago. The patches have been modified as needed by
the upstream changes and received the occasional small odd fix but are
otherwise not modified. Please see the individual commit messages for more
information.

The videobuf2 memory managers use the DMA mapping API to handle cache
synchronization on systems that require them transparently for drivers. As
cache operations are expensive, system performances can be impacted. Cache
synchronization can't be skipped altogether if we want to retain correct
behaviour, but optimizations are possible in cases related to buffer sharing
between multiple devices without CPU access to the memory.

The first optimization covers cases where the memory never needs to be
accessed by the CPU (neither in kernelspace nor in userspace). In those cases,
as no CPU memory mappings exist, cache synchronization can be skipped. The
situation could be detected in the kernel as we have enough information to
determine whether CPU mappings for kernelspace or userspace exist (in the
first case because drivers should request them explicitly, in the second case
because the mmap() handler hasn't been invoked). This optimization is not
implemented currently but should at least be prototyped as it could improve
performances automatically in a large number of cases.

The second class of optimizations cover cases where the memory sometimes needs
to be accessed by the CPU. In those cases memory mapping must be created and
cache handled, but cache synchronization could be skipped for buffer that are
not touched by the CPU.

By default the following cache synchronization operations need to be performed
related to the buffer management ioctls. For simplicity means of QBUF below
apply to buf VIDIOC_QBUF and VIDIOC_PREPARE_BUF.

| QBUF  | DQBUF

CAPTURE | Invalidate| Invalidate (*)
OUTPUT  | Clean | -

(*) for systems using speculative pre-fetching only

The following cases can be optimized.

1. CAPTURE, the CPU has not written to the buffer before QBUF

   Cache invalidation can be skipped at QBUF time, but becomes required at
   DQBUF time on all systems, regardless of whether they use speculative
   prefetching.

2. CAPTURE, the CPU will not read from the buffer after DQBUF

   Cache invalidation can be skipped at DQBUF time.

3. CAPTURE, combination of (1) and (2)

   Cache invalidation can be skipped at both QBUF and DQBUF time.

4. OUTPUT, the CPU has not written to the buffer before QBUF

   Cache clean can be skipped at QBUF time.


The kernel can't detect thoses situations automatically and thus requires
hints from userspace to decide whether cache synchronization can be skipped.
It should be noted that those hints might not be honoured. In particular, if
userspace hints that it hasn't touched the buffer with the CPU, drivers might
need to perform memory accesses themselves (adding JPEG or MPEG headers to
buffers is a common case where CPU access could be needed in the kernel), in
which case the userspace hints will be ignored.

Getting the hints wrong will result in data corruption. Userspace applications
are allowed to shoot themselves in the foot, but driver are responsible for
deciding whether data corruption can pose a risk to the system in general. For
instance if the device could be made to crash, or behave in a way that would
jeopardize system security, reliability or performances, when fed with invalid
data, cache synchronization shall not be skipped solely due to possibly
incorrect userspace hints.

The V4L2 API defines two flags, V4L2-BUF-FLAG-NO-CACHE-INVALIDATE and
V4L2_BUF_FLAG_NO_CACHE_SYNC, that can be used to provide cache-related hints
to the kernel. However, no kernel has ever implemented support for those flags
that are thus most likely unused.

A single flag is enough to cover all the optimization cases described above,
provided we keep track of the flag being set at QBUF time to force cache
invalidation at DQBUF time for case (1) if the  flag isn't set at DQBUF time.
This patch series thus cleans up the userspace API and merges both flags into
a single one.

One potential issue with case (1) is that cache invalidation at DQBUF time for
CAPTURE buffers isn't fully under the control of videobuf2. We can instruct
the DMA mapping API to skip cache handling, but we can't force it to
invalidate the cache in the sync_for_cpu operation for non speculative
prefetching systems. Luckily, on ARM32 the current implementation always
invalidates the cache in __dma_page_dev_to_cpu() for CAPTURE buffers so we are
safe fot now. However, this is documented by a FIXME comment that might lead
to someone fixing the implementation in the future. I believe we will have to
the problem at the DMA mapping level, the userspace hint API shouldn't be

[GIT PULL for v4.10-rc1] media updates

[Mauro Carvalho Chehab]

Hi Linus,

Please pull from:
  git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media 
tags/media/v4.10-1

For:

  - New Mediatek drivers: mtk-mdp and mtk-vcodec;
  - Some additions at the media documentation;
  - the CEC core and drivers were promoted from staging to mainstream;
  - Some cleanups at the DVB core;
  - The LIRC serial driver got promoted from staging to mainstream;
  - Added a driver for Renesas R-Car FDP1 driver;
  - add DVBv5 statistics support to mn88473 driver;
  - several fixes related to printk continuation lines;
  - add support for HSV encoding formats;
  - Lots of other cleanups, fixups and driver improvements.

Thanks!
Mauro

-

The following changes since commit e7aa8c2eb11ba69b1b69099c3c7bd6be3087b0ba:

  Merge tag 'docs-4.10' of git://git.lwn.net/linux (2016-12-12 21:58:13 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media 
tags/media/v4.10-1

for you to fetch changes up to 65390ea01ce678379da32b01f39fcfac4903f256:

  Merge branch 'patchwork' into v4l_for_linus (2016-12-15 08:38:35 -0200)


media updates for v4.10-rc1


Andi Shyti (1):
  [media] lirc_dev: remove compat_ioctl assignment

Andrea Gelmini (1):
  [media] extended-controls.rst: fix typo

Andrew-CT Chen (1):
  [media] VPU: mediatek: Add decode support

Andrey Utkin (3):
  [media] tw5864: crop picture width to 704
  [media] media: solo6x10: fix lockup by avoiding delayed register write
  [media] saa7146: Fix for while releasing video buffers

Andrzej Hajda (1):
  [media] s5p-mfc: Correct scratch buffer size of H.263 decoder

Antti Palosaari (3):
  [media] mn88473: refactor and fix statistics
  [media] mn88473: fix chip id check on probe
  [media] mn88472: fix chip id check on probe

Archit Taneja (1):
  [media] media: ti-vpe: Use line average de-interlacing for first 2 frames

Arnd Bergmann (11):
  [media] platform: pxa_camera: add VIDEO_V4L2 dependency
  [media] s5p-cec: mark PM functions as __maybe_unused again
  [media] media: mtk-mdp: mark PM functions as __maybe_unused
  [media] dvb: remove unused systime() function
  [media] go7007: add MEDIA_CAMERA_SUPPORT dependency
  [media] em28xx: only use mt9v011 if camera support is enabled
  [media] staging: media: davinci_vfpe: allow modular build
  [media] staging: media: davinci_vpfe: fix W=1 build warnings
  [media] v4l: rcar_fdp1: mark PM functions as __maybe_unused
  [media] v4l: rcar_fdp1: add FCP dependency
  [media] DaVinci-VPFE-Capture: fix error handling

Baoyou Xie (1):
  [media] coda: add missing header dependencies

Benoit Parrot (17):
  [media] media: i2c: tvp514x: Reported mbus format should be 
MEDIA_BUS_FMT_UYVY8_2X8
  [media] media: ti-vpe: vpdma: Make vpdma library into its own module
  [media] media: ti-vpe: vpdma: Add multi-instance and multi-client support
  [media] media: ti-vpe: vpdma: Add helper to set a background color
  [media] media: ti-vpe: vpdma: Fix bus error when vpdma is writing a 
descriptor
  [media] media: ti-vpe: vpe: Added MODULE_DEVICE_TABLE hint
  [media] media: ti-vpe: vpdma: Corrected YUV422 data type label
  [media] media: ti-vpe: vpdma: RGB data type yield inverted data
  [media] media: ti-vpe: vpe: Fix vb2 buffer cleanup
  [media] media: ti-vpe: vpe: Enable DMABUF export
  [media] media: ti-vpe: Make scaler library into its own module
  [media] media: ti-vpe: scaler: Add debug support for multi-instance
  [media] media: ti-vpe: vpe: Make sure frame size dont exceed scaler 
capacity
  [media] media: ti-vpe: vpdma: Add RAW8 and RAW16 data types
  [media] media: ti-vpe: Make colorspace converter library into its own 
module
  [media] media: ti-vpe: csc: Add debug support for multi-instance
  [media] media: ti-vpe: vpe: Add proper support single and multi-plane 
buffer

CIJOML CIJOMLovic (1):
  [media] Add support for EVOLVEO XtraTV stick

Christophe JAILLET (1):
  [media] VPU: mediatek: Fix return value in case of error

Colin Ian King (5):
  [media] VPU: mediatek: fix null pointer dereference on pdev
  [media] cx24120: do not allow an invalid delivery system types
  [media] variable name is never null, so remove null check
  [media] st-hva: fix a copy-and-paste variable name error
  [media] zoran: fix spelling mistake in dprintk message

CrazyCat (1):
  [media] dvb-usb-cxusb: Geniatech T230 - resync TS FIFO after lock

Dan Carpenter (5):
  [media] st-hva: fix some error handling in hva_hw_probe()
  [media] blackfin: check devm_pinctrl_get() for errors
  [media] stk-webcam: fix an endian bug in stk_camera_read_reg()
  [media] staging: media: davinci_vpfe: unlock on error in vpfe_reqbufs()
  [media] uvcvideo: freeing an 

[PATCH 1/2] media: s5p-mfc: convert drivers to use the new vb2_queue dev field

[Pankaj Dubey]

From: Smitha T Murthy 

commit 2548fee63d9e ("[media] media/platform: convert drivers to use the new
vb2_queue dev field") has missed to set dev pointer of vb2_queue which will be
used in reqbufs of mfc driver. Without this change following error is observed:

---
V4L2 Codec decoding example application
Kamil Debski 
Copyright 2012 Samsung Electronics Co., Ltd.

Opening MFC.
(mfc.c:mfc_open:58): MFC Info (/dev/video0): driver="s5p-mfc" \
bus_info="platform:12c3.mfc0" card="s5p-mfc-dec" fd=0x4[
42.339165] Remapping memory failed, error: -6

MFC Open Success.
(main.c:main:711): Successfully opened all necessary files and devices
(mfc.c:mfc_dec_setup_output:103): Setup MFC decoding OUTPUT buffer \
size=4194304 (requested=4194304)
(mfc.c:mfc_dec_setup_output:120): Number of MFC OUTPUT buffers is 2 \
(requested 2)

[App] Out buf phy : 0x, virt : 0x
Output Length is = 0x30
Error (mfc.c:mfc_dec_setup_output:145): Failed to MMAP MFC OUTPUT buffer
---

Signed-off-by: Smitha T Murthy 
[pankaj.dubey: debugging issue and formatting commit message]
Signed-off-by: Pankaj Dubey 
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 0a5b8f5..6ea8246 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -838,6 +838,7 @@ static int s5p_mfc_open(struct file *file)
q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
q->drv_priv = >fh;
q->lock = >mfc_mutex;
+   q->dev = >plat_dev->dev;
if (vdev == dev->vfd_dec) {
q->io_modes = VB2_MMAP;
q->ops = get_dec_queue_ops();
@@ -861,6 +862,7 @@ static int s5p_mfc_open(struct file *file)
q->io_modes = VB2_MMAP;
q->drv_priv = >fh;
q->lock = >mfc_mutex;
+   q->dev = >plat_dev->dev;
if (vdev == dev->vfd_dec) {
q->io_modes = VB2_MMAP;
q->ops = get_dec_queue_ops();
-- 
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 0/2] s5p-mfc fix for using reserved memory

[Pankaj Dubey]

It has been observed on ARM64 based Exynos SoC, if IOMMU is not enabled
and we try to use reserved memory for MFC, reqbufs fails with below
mentioned error
---
V4L2 Codec decoding example application
Kamil Debski 
Copyright 2012 Samsung Electronics Co., Ltd.

Opening MFC.
(mfc.c:mfc_open:58): MFC Info (/dev/video0): driver="s5p-mfc" \
bus_info="platform:12c3.mfc0" card="s5p-mfc-dec" fd=0x4[
42.339165] Remapping memory failed, error: -6

MFC Open Success.
(main.c:main:711): Successfully opened all necessary files and devices
(mfc.c:mfc_dec_setup_output:103): Setup MFC decoding OUTPUT buffer \
size=4194304 (requested=4194304)
(mfc.c:mfc_dec_setup_output:120): Number of MFC OUTPUT buffers is 2 \
(requested 2)

[App] Out buf phy : 0x, virt : 0x
Output Length is = 0x30
Error (mfc.c:mfc_dec_setup_output:145): Failed to MMAP MFC OUTPUT buffer
-
This is because the device requesting for memory is mfc0.left not the parent 
mfc0.
Hence setting of alloc_devs need to be done only if IOMMU is enabled
and in that case both the left and right device is treated as mfc0 only.
Also we need to populate vb2_queue's dev pointer with mfc dev pointer.

Smitha T Murthy (2):
  media: s5p-mfc: convert drivers to use the new vb2_queue dev field
  media: s5p-mfc: fix MMAP of mfc buffer during reqbufs

 drivers/media/platform/s5p-mfc/s5p_mfc.c |  2 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 17 ++---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 18 +++---
 3 files changed, 23 insertions(+), 14 deletions(-)

-- 
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 2/2] media: s5p-mfc: fix MMAP of mfc buffer during reqbufs

[Pankaj Dubey]

From: Smitha T Murthy 

It has been observed on ARM64 based Exynos SoC, if IOMMU is not enabled
and we try to use reserved memory for MFC, reqbufs fails with below
mentioned error
---
V4L2 Codec decoding example application
Kamil Debski 
Copyright 2012 Samsung Electronics Co., Ltd.

Opening MFC.
(mfc.c:mfc_open:58): MFC Info (/dev/video0): driver="s5p-mfc" \
bus_info="platform:12c3.mfc0" card="s5p-mfc-dec" fd=0x4[
42.339165] Remapping memory failed, error: -6

MFC Open Success.
(main.c:main:711): Successfully opened all necessary files and devices
(mfc.c:mfc_dec_setup_output:103): Setup MFC decoding OUTPUT buffer \
size=4194304 (requested=4194304)
(mfc.c:mfc_dec_setup_output:120): Number of MFC OUTPUT buffers is 2 \
(requested 2)

[App] Out buf phy : 0x, virt : 0x
Output Length is = 0x30
Error (mfc.c:mfc_dec_setup_output:145): Failed to MMAP MFC OUTPUT buffer
-
This is because the device requesting for memory is mfc0.left not the parent 
mfc0.
Hence setting of alloc_devs need to be done only if IOMMU is enabled
and in that case both the left and right device is treated as mfc0 only.

Signed-off-by: Smitha T Murthy 
[pankaj.dubey: debugging issue and formatting commit message]
Signed-off-by: Pankaj Dubey 
---
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 17 ++---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 18 +++---
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 52081dd..9cfca5d 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -30,6 +30,7 @@
 #include "s5p_mfc_intr.h"
 #include "s5p_mfc_opr.h"
 #include "s5p_mfc_pm.h"
+#include "s5p_mfc_iommu.h"
 
 static struct s5p_mfc_fmt formats[] = {
{
@@ -930,16 +931,18 @@ static int s5p_mfc_queue_setup(struct vb2_queue *vq,
vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
psize[0] = ctx->luma_size;
psize[1] = ctx->chroma_size;
-
-   if (IS_MFCV6_PLUS(dev))
-   alloc_devs[0] = ctx->dev->mem_dev_l;
-   else
-   alloc_devs[0] = ctx->dev->mem_dev_r;
-   alloc_devs[1] = ctx->dev->mem_dev_l;
+   if (exynos_is_iommu_available(>plat_dev->dev)) {
+   if (IS_MFCV6_PLUS(dev))
+   alloc_devs[0] = ctx->dev->mem_dev_l;
+   else
+   alloc_devs[0] = ctx->dev->mem_dev_r;
+   alloc_devs[1] = ctx->dev->mem_dev_l;
+   }
} else if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
   ctx->state == MFCINST_INIT) {
psize[0] = ctx->dec_src_buf_size;
-   alloc_devs[0] = ctx->dev->mem_dev_l;
+   if (exynos_is_iommu_available(>plat_dev->dev))
+   alloc_devs[0] = ctx->dev->mem_dev_l;
} else {
mfc_err("This video node is dedicated to decoding. Decoding not 
initialized\n");
return -EINVAL;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index fcc2e05..eb8f06d 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -30,6 +30,7 @@
 #include "s5p_mfc_enc.h"
 #include "s5p_mfc_intr.h"
 #include "s5p_mfc_opr.h"
+#include "s5p_mfc_iommu.h"
 
 #define DEF_SRC_FMT_ENCV4L2_PIX_FMT_NV12M
 #define DEF_DST_FMT_ENCV4L2_PIX_FMT_H264
@@ -1832,7 +1833,8 @@ static int s5p_mfc_queue_setup(struct vb2_queue *vq,
if (*buf_count > MFC_MAX_BUFFERS)
*buf_count = MFC_MAX_BUFFERS;
psize[0] = ctx->enc_dst_buf_size;
-   alloc_devs[0] = ctx->dev->mem_dev_l;
+   if (exynos_is_iommu_available(>plat_dev->dev))
+   alloc_devs[0] = ctx->dev->mem_dev_l;
} else if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
if (ctx->src_fmt)
*plane_count = ctx->src_fmt->num_planes;
@@ -1847,12 +1849,14 @@ static int s5p_mfc_queue_setup(struct vb2_queue *vq,
psize[0] = ctx->luma_size;
psize[1] = ctx->chroma_size;
 
-   if (IS_MFCV6_PLUS(dev)) {
-   alloc_devs[0] = ctx->dev->mem_dev_l;
-   alloc_devs[1] = ctx->dev->mem_dev_l;
-   } else {
-   alloc_devs[0] = ctx->dev->mem_dev_r;
-   alloc_devs[1] = ctx->dev->mem_dev_r;
+   if (exynos_is_iommu_available(>plat_dev->dev)) {
+

Re: [PATCH v5 3/6] [media] rc-core: add support for IR raw transmitters

[kbuild test robot]

Hi Andi,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on next-20161216]
[cannot apply to v4.9]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Andi-Shyti/Add-support-for-IR-transmitters/20161216-144204
base:   git://linuxtv.org/media_tree.git master
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make 
rule.
   include/linux/init.h:1: warning: no structured comments found
   include/linux/workqueue.h:392: warning: No description found for parameter 
'...'
   include/linux/workqueue.h:392: warning: Excess function parameter 'args' 
description in 'alloc_workqueue'
   include/linux/workqueue.h:413: warning: No description found for parameter 
'...'
   include/linux/workqueue.h:413: warning: Excess function parameter 'args' 
description in 'alloc_ordered_workqueue'
   include/linux/kthread.h:26: warning: No description found for parameter '...'
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/linux/fence-array.h:61: warning: No description found for parameter 
'fence'
   include/sound/core.h:324: warning: No description found for parameter '...'
   include/sound/core.h:335: warning: No description found for parameter '...'
   include/sound/core.h:388: warning: No description found for parameter '...'
   drivers/media/dvb-core/dvb_frontend.h:677: warning: No description found for 
parameter 'refcount'
   include/media/media-entity.h:1054: warning: No description found for 
parameter '...'
>> include/media/rc-core.h:39: warning: bad line:driver 
>> requires pulse/space data sequence.
   include/net/mac80211.h:3207: ERROR: Unexpected indentation.
   include/net/mac80211.h:3210: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   include/net/mac80211.h:3212: ERROR: Unexpected indentation.
   include/net/mac80211.h:3213: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   include/net/mac80211.h:1772: ERROR: Unexpected indentation.
   include/net/mac80211.h:1776: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without 
end-string.
   kernel/time/timer.c:1240: ERROR: Unexpected indentation.
   kernel/time/timer.c:1242: ERROR: Unexpected indentation.
   kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   include/linux/wait.h:121: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   include/linux/wait.h:124: ERROR: Unexpected indentation.
   include/linux/wait.h:126: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   kernel/signal.c:317: WARNING: Inline literal start-string without end-string.
   drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a 
blank line; unexpected unindent.
   drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without 
a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1893: WARNING: Definition list ends without 
a blank line; unexpected unindent.
   include/linux/spi/spi.h:369: ERROR: Unexpected indentation.
   WARNING: dvipng command 'dvipng' cannot be run (needed for math display), 
check the imgmath_dvipng setting

vim +39 include/media/rc-core.h

23  #include 
24  
25  extern int rc_core_debug;
26  #define IR_dprintk(level, fmt, ...) \
27  do {\
28  if (rc_core_debug >= level) \
29  printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);  \
30  } while (0)
31  
32  /**
33   * enum rc_driver_type - type of the RC output
34   *
35   * @RC_DRIVER_SCANCODE:  Driver or hardware generates a scancode
36   * @RC_DRIVER_IR_RAW:Driver or hardware generates pulse/space 
sequences.
37   *   It needs a Infra-Red pulse/space decoder
38   * @RC_DRIVER_IR_RAW_TX: Device transmitter only,
  > 39   driver requires pulse/space data sequence.
40   */
41  enum rc_driver_type {
42  RC_DRIVER_SCANCODE = 0,
43  RC_DRIVER_IR_RAW,
44  RC_DRIVER_IR_RAW_TX,
45  };
46  
47  /**

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


.config.gz
Description: application/gzip

Re: [PATCH 2/2] media: omap3isp change to devm for resources

[Hans Verkuil]

On 15/12/16 20:40, Shuah Khan wrote:

Using devm resources that have external dependencies such as a dev
for a file handler could result in devm resources getting released
durin unbind while an application has the file open holding pointer
to the devm resource. This results in use-after-free errors when the
application exits.


That's solving the wrong problem.

The real problem is that when registering a video_device it should do
this:

devnode->cdev.kobj.parent = >dev.kobj;

(taken from cec-core.c)

This will prevent isp->dev from being released as long as there is a
filehandle still open.

After that change I believe that this will work correctly, but this
has to be tested first!

Regards,

Hans



Signed-off-by: Shuah Khan 
---
 drivers/media/platform/omap3isp/isp.c | 71 +++
 drivers/media/platform/omap3isp/ispccp2.c | 10 +++-
 drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +---
 drivers/media/platform/omap3isp/isph3a_af.c   | 21 +---
 drivers/media/platform/omap3isp/isphist.c |  5 +-
 5 files changed, 92 insertions(+), 36 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index 0321d84..a11c509 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1374,7 +1374,7 @@ static int isp_get_clocks(struct isp_device *isp)
unsigned int i;

for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) {
-   clk = devm_clk_get(isp->dev, isp_clocks[i]);
+   clk = clk_get(isp->dev, isp_clocks[i]);
if (IS_ERR(clk)) {
dev_err(isp->dev, "clk_get %s failed\n", isp_clocks[i]);
return PTR_ERR(clk);
@@ -1386,6 +1386,14 @@ static int isp_get_clocks(struct isp_device *isp)
return 0;
 }

+static void isp_put_clocks(struct isp_device *isp)
+{
+   unsigned int i;
+
+   for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i)
+   clk_put(isp->clock[i]);
+}
+
 /*
  * omap3isp_get - Acquire the ISP resource.
  *
@@ -2015,6 +2023,11 @@ static int isp_remove(struct platform_device *pdev)

media_entity_enum_cleanup(>crashed);

+   regulator_put(isp->isp_csiphy2.vdd);
+   regulator_put(isp->isp_csiphy1.vdd);
+
+   isp_put_clocks(isp);
+   kfree(isp);
return 0;
 }

@@ -2107,8 +2120,8 @@ static int isp_of_parse_nodes(struct device *dev,
 {
struct device_node *node = NULL;

-   notifier->subdevs = devm_kcalloc(
-   dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
+   notifier->subdevs = kcalloc(
+   ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
if (!notifier->subdevs)
return -ENOMEM;

@@ -2116,11 +2129,9 @@ static int isp_of_parse_nodes(struct device *dev,
   (node = of_graph_get_next_endpoint(dev->of_node, node))) {
struct isp_async_subdev *isd;

-   isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
-   if (!isd) {
-   of_node_put(node);
+   isd = kzalloc(sizeof(*isd), GFP_KERNEL);
+   if (!isd)
return -ENOMEM;
-   }

notifier->subdevs[notifier->num_subdevs] = >asd;

@@ -2204,7 +2215,7 @@ static int isp_probe(struct platform_device *pdev)
int ret;
int i, m;

-   isp = devm_kzalloc(>dev, sizeof(*isp), GFP_KERNEL);
+   isp = kzalloc(sizeof(*isp), GFP_KERNEL);
if (!isp) {
dev_err(>dev, "could not allocate memory\n");
return -ENOMEM;
@@ -2213,21 +2224,23 @@ static int isp_probe(struct platform_device *pdev)
ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type",
   >phy_type);
if (ret)
-   return ret;
+   goto error_release_isp;

isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
  "syscon");
-   if (IS_ERR(isp->syscon))
-   return PTR_ERR(isp->syscon);
+   if (IS_ERR(isp->syscon)) {
+   ret = PTR_ERR(isp->syscon);
+   goto error_release_isp;
+   }

ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1,
 >syscon_offset);
if (ret)
-   return ret;
+   goto error_release_isp;

ret = isp_of_parse_nodes(>dev, >notifier);
if (ret < 0)
-   return ret;
+   goto error_release_isp;

isp->autoidle = autoidle;

@@ -2244,8 +2257,18 @@ static int isp_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, isp);

/* Regulators */
-   isp->isp_csiphy1.vdd = devm_regulator_get(>dev, "vdd-csiphy1");
-   isp->isp_csiphy2.vdd = devm_regulator_get(>dev, "vdd-csiphy2");
+   

Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Hans Verkuil]

On 16/12/16 11:57, Mauro Carvalho Chehab wrote:

Em Fri, 16 Dec 2016 11:11:25 +0100
Hans Verkuil  escreveu:


Would it make sense to enforce that dependency. Can we tie /dev/media usecount
to /dev/video etc. usecount? In other words:

/dev/video is opened, then open /dev/media.


When a device node is registered it should increase the refcount on the 
media_device
(as I proposed, that would be mdev->dev). When a device node is unregistered 
and the
last user disappeared, then it can decrease the media_device refcount.

So as long as anyone is using a device node, the media_device will stick around 
as
well.

No need to take refcounts on open/close.


That makes sense. You're meaning something like the enclosed (untested)
patch?


One note: as I mentioned, the video_device does not set the cdev parent 
correctly,
so that bug needs to be fixed first for this to work.


Actually, __video_register_device() seems to be setting the parent
properly:

if (vdev->dev_parent == NULL)
vdev->dev_parent = vdev->v4l2_dev->dev;


No, I mean this code (from cec-core.c):


   /* Part 2: Initialize and register the character device */
cdev_init(>cdev, _devnode_fops);
devnode->cdev.kobj.parent = >dev.kobj;
devnode->cdev.owner = owner;

ret = cdev_add(>cdev, devnode->dev.devt, 1);
if (ret < 0) {
pr_err("%s: cdev_add failed\n", __func__);
goto clr_bit;
}

ret = device_add(>dev);
if (ret)
goto cdev_del;

which sets cdev.kobj.parent. And that would indeed be vdev->dev_parent.



Thanks,
Mauro

[PATCH] Be sure that the media_device won't be freed too early

This code snippet is untested.

Signed-off-by: Mauro Carvalho chehab 

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 8756275e9fc4..5fdeab382069 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -706,7 +706,7 @@ int __must_check __media_device_register(struct 
media_device *mdev,
struct media_devnode *devnode;
int ret;

-   devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
+   devnode = devm_kzalloc(mdev->dev, sizeof(*devnode), GFP_KERNEL);


I'm not sure about this change. I *think* this would work, but *only* if all
the refcounting is 100% correct, and we know it isn't at the moment. So I
think this should be postponed until we are confident everything is correct.


if (!devnode)
return -ENOMEM;

diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
b/drivers/media/v4l2-core/v4l2-dev.c
index 8be561ab2615..14a3c56dbcac 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -196,6 +196,7 @@ static void v4l2_device_release(struct device *cd)
 #if defined(CONFIG_MEDIA_CONTROLLER)
if (v4l2_dev->mdev) {
/* Remove interfaces and interface links */
+   put_device(v4l2_dev->mdev->dev);
media_devnode_remove(vdev->intf_devnode);
if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
media_device_unregister_entity(>entity);


I think this is the wrong order: put_device should go after 
media_device_unregister_entity().


@@ -810,6 +811,7 @@ static int video_register_media_controller(struct 
video_device *vdev, int type)
return -ENOMEM;
}
}
+   get_device(vdev->v4l2_dev->dev);


You mean v4l2_dev->mdev->dev?



/* FIXME: how to create the other interface links? */

@@ -1015,6 +1017,11 @@ void video_unregister_device(struct video_device *vdev)
if (!vdev || !video_is_registered(vdev))
return;

+#if defined(CONFIG_MEDIA_CONTROLLER)
+   if (vdev->v4l2_dev->dev)
+   put_device(vdev->v4l2_dev->dev);


Ditto.


+#endif
+
mutex_lock(_lock);
/* This must be in a critical section to prevent a race with v4l2_open.
 * Once this bit has been cleared video_get may never be called again.
diff --git a/drivers/media/v4l2-core/v4l2-device.c 
b/drivers/media/v4l2-core/v4l2-device.c
index 62bbed76dbbc..53f42090c762 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -188,6 +188,7 @@ int v4l2_device_register_subdev(struct v4l2_device 
*v4l2_dev,
err = media_device_register_entity(v4l2_dev->mdev, entity);
if (err < 0)
goto error_module;
+   get_device(v4l2_dev->mdev->dev);
}
 #endif

@@ -205,6 +206,8 @@ int v4l2_device_register_subdev(struct v4l2_device 
*v4l2_dev,

 error_unregister:
 #if defined(CONFIG_MEDIA_CONTROLLER)
+   if (v4l2_dev->mdev)
+   put_device(v4l2_dev->mdev->dev);
media_device_unregister_entity(entity);
 #endif
 error_module:
@@ -310,6 +313,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 

Re: [PATCH RFC] omap3isp: prevent releasing MC too early

[Mauro Carvalho Chehab]

Em Thu, 15 Dec 2016 16:04:51 +0200
Laurent Pinchart  escreveu:

We have now two threads discussing the same subject, which is bad, as
we'll end repeating the same arguments on different threads...

Let's use the "[PATCH RFC 00/21]" for those discussions, as it seems we're
reaching to somewhere there.

> Even if you're not entirely convinced by the reasons 
> explained in this mail thread, remember that we will need sooner or later to 
> implement support for media graph update at runtime. Refcounting will be 
> needed, let's design it in the cleanest possible way.

As I said, I'm not against using some other approach and even
adding refcounting to each graph object.

What I am against is on a patchset that starts by breaking 
the USB drivers that use the media controller.

Btw, I'm starting to suspect that getting rid of devm_*alloc()
on OMAP3, as proposed by the 00/21 thread is addressing a symptom of
the problem and not a cause, and that using get_device()/put_device()
may help fixing such issues. See Hans comments on that thread.

Thanks,
Mauro
--
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: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Mauro Carvalho Chehab]

Em Fri, 16 Dec 2016 11:11:25 +0100
Hans Verkuil  escreveu:

> > Would it make sense to enforce that dependency. Can we tie /dev/media 
> > usecount
> > to /dev/video etc. usecount? In other words:
> >
> > /dev/video is opened, then open /dev/media.  
> 
> When a device node is registered it should increase the refcount on the 
> media_device
> (as I proposed, that would be mdev->dev). When a device node is unregistered 
> and the
> last user disappeared, then it can decrease the media_device refcount.
> 
> So as long as anyone is using a device node, the media_device will stick 
> around as
> well.
> 
> No need to take refcounts on open/close.

That makes sense. You're meaning something like the enclosed (untested)
patch?

> One note: as I mentioned, the video_device does not set the cdev parent 
> correctly,
> so that bug needs to be fixed first for this to work.

Actually, __video_register_device() seems to be setting the parent
properly:

if (vdev->dev_parent == NULL)
vdev->dev_parent = vdev->v4l2_dev->dev;

Thanks,
Mauro

[PATCH] Be sure that the media_device won't be freed too early

This code snippet is untested.

Signed-off-by: Mauro Carvalho chehab 

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 8756275e9fc4..5fdeab382069 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -706,7 +706,7 @@ int __must_check __media_device_register(struct 
media_device *mdev,
struct media_devnode *devnode;
int ret;
 
-   devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
+   devnode = devm_kzalloc(mdev->dev, sizeof(*devnode), GFP_KERNEL);
if (!devnode)
return -ENOMEM;
 
diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
b/drivers/media/v4l2-core/v4l2-dev.c
index 8be561ab2615..14a3c56dbcac 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -196,6 +196,7 @@ static void v4l2_device_release(struct device *cd)
 #if defined(CONFIG_MEDIA_CONTROLLER)
if (v4l2_dev->mdev) {
/* Remove interfaces and interface links */
+   put_device(v4l2_dev->mdev->dev);
media_devnode_remove(vdev->intf_devnode);
if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
media_device_unregister_entity(>entity);
@@ -810,6 +811,7 @@ static int video_register_media_controller(struct 
video_device *vdev, int type)
return -ENOMEM;
}
}
+   get_device(vdev->v4l2_dev->dev);
 
/* FIXME: how to create the other interface links? */
 
@@ -1015,6 +1017,11 @@ void video_unregister_device(struct video_device *vdev)
if (!vdev || !video_is_registered(vdev))
return;
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+   if (vdev->v4l2_dev->dev)
+   put_device(vdev->v4l2_dev->dev);
+#endif
+
mutex_lock(_lock);
/* This must be in a critical section to prevent a race with v4l2_open.
 * Once this bit has been cleared video_get may never be called again.
diff --git a/drivers/media/v4l2-core/v4l2-device.c 
b/drivers/media/v4l2-core/v4l2-device.c
index 62bbed76dbbc..53f42090c762 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -188,6 +188,7 @@ int v4l2_device_register_subdev(struct v4l2_device 
*v4l2_dev,
err = media_device_register_entity(v4l2_dev->mdev, entity);
if (err < 0)
goto error_module;
+   get_device(v4l2_dev->mdev->dev);
}
 #endif
 
@@ -205,6 +206,8 @@ int v4l2_device_register_subdev(struct v4l2_device 
*v4l2_dev,
 
 error_unregister:
 #if defined(CONFIG_MEDIA_CONTROLLER)
+   if (v4l2_dev->mdev)
+   put_device(v4l2_dev->mdev->dev);
media_device_unregister_entity(entity);
 #endif
 error_module:
@@ -310,6 +313,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 * links are removed by the function below, in the right order
 */
media_device_unregister_entity(>entity);
+   put_device(v4l2_dev->mdev->dev);
}
 #endif
video_unregister_device(sd->devnode);




--
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] gen-errors.rst: document EIO

[Sakari Ailus]

On Fri, Dec 16, 2016 at 10:22:08AM +0100, Hans Verkuil wrote:
> Document the EIO error since this can happen anywhere anytime and applications
> should be aware of this.
> 
> Signed-off-by: Hans Verkuil 
> ---
> diff --git a/Documentation/media/uapi/gen-errors.rst 
> b/Documentation/media/uapi/gen-errors.rst
> index 6e983b9..ee224b9 100644
> --- a/Documentation/media/uapi/gen-errors.rst
> +++ b/Documentation/media/uapi/gen-errors.rst
> @@ -94,6 +94,14 @@ Generic Error Codes
> -  Permission denied. Can be returned if the device needs write
> permission, or some special capabilities is needed (e. g. root)
> 
> +-  .. row 11
> +
> +   -  ``EIO``
> +
> +   -  I/O error. Typically used when there are problems communicating 
> with
> +  a hardware device. This could indicate broken or flaky hardware.
> +   It's a 'Something is wrong, I give up!' type of error.
> +
>  .. note::
> 
>#. This list is not exaustive; ioctls may return other error codes.

Acked-by: Sakari Ailus 

-- 
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: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Mauro Carvalho Chehab]

Em Fri, 16 Dec 2016 11:03:09 +0100
Hans Verkuil  escreveu:

> So:
> 
> 1) subdev drivers should disallow unbind
> 2) interface entities should call media_device_unregister_entity() when they
> are unregistered (if that doesn't already happen)

Sounds like a plan to me.

Thanks,
Mauro
--
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: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Hans Verkuil]

On 15/12/16 18:51, Shuah Khan wrote:

On 12/15/2016 10:25 AM, Mauro Carvalho Chehab wrote:

Em Thu, 15 Dec 2016 10:09:53 -0700
Shuah Khan  escreveu:


On 12/15/2016 09:28 AM, Hans Verkuil wrote:

On 15/12/16 17:06, Shuah Khan wrote:




I think this will work for interface entities, but for subdev entities this
certainly won't work. Unbinding subdevs should be blocked (just set
suppress_bind_attrs to true in all subdev drivers). Most top-level drivers
have pointers to subdev data, so unbinding them will just fail horribly.



Yes that is an option. I did something similar for au0828 and snd_usb_audio
case, so the module that registers the media_device can't unbound until the
other driver. If au0828 registers media_device, it becomes the owner and if
it gets unbound ioctls will start to see problems.


Sorry I meant to say rmmod'ed not unbound. Unbound will work just fine. If the
modules that owns the media_devnode goes away, there will be problems with
cdev trying to load module when application closes the device file and exits.
In this case, Media Device Allocator API takes module reference, so its use
count goes up.



What this means though is that drivers can't be unbound easily. But that is
a small price to pay compared to the problems we will see if a driver is
unbound when its entities are still in use. Also, unsetting bind_attrs has
to be done as well, otherwise we can never unbind any driver.


I don't think suppress_bind_attrs will work on USB drivers, as the
device can be physically removed.



Yeah setting suppress_bind_attrs would cause problems. On one hand keeping
all entities until all references are gone sound like a good option, however
this would cause problems coordinating removal especially in the case of
embedded entities. Can this be done in a simpler way? The way I see it, we
have /dev/video, /dev/dvb, /dev/snd/* etc. that depend on /dev/media for
graph nodes. Any one of these devices could be open when any of the drivers
is unbound (physical removal is a simpler case).

Would it make sense to enforce that dependency. Can we tie /dev/media usecount
to /dev/video etc. usecount? In other words:

/dev/video is opened, then open /dev/media.


When a device node is registered it should increase the refcount on the 
media_device
(as I proposed, that would be mdev->dev). When a device node is unregistered 
and the
last user disappeared, then it can decrease the media_device refcount.

So as long as anyone is using a device node, the media_device will stick around 
as
well.

No need to take refcounts on open/close.

One note: as I mentioned, the video_device does not set the cdev parent 
correctly,
so that bug needs to be fixed first for this to work.


prevent entities being removed if /dev/media is open.

Would that help. The above could be done in a generic way possibly. Would it
help if /dev/media is kept open when streaming is active? That is just one


Again, it's not about the device nodes, it's about the media_device.

Regards,

Hans


use-case, there might be others.

thanks,
-- Shuah


thanks,
-- Shuah

--
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



--
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: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Hans Verkuil]

On 15/12/16 18:09, Shuah Khan wrote:

On 12/15/2016 09:28 AM, Hans Verkuil wrote:

On 15/12/16 17:06, Shuah Khan wrote:

On 12/15/2016 08:26 AM, Hans Verkuil wrote:

On 15/12/16 15:45, Shuah Khan wrote:

On 12/15/2016 07:03 AM, Hans Verkuil wrote:

On 15/12/16 13:56, Laurent Pinchart wrote:

Hi Sakari,

On Thursday 15 Dec 2016 13:30:41 Sakari Ailus wrote:

On Tue, Dec 13, 2016 at 10:24:47AM -0200, Mauro Carvalho Chehab wrote:

Em Tue, 13 Dec 2016 12:53:05 +0200 Sakari Ailus escreveu:

On Tue, Nov 29, 2016 at 09:13:05AM -0200, Mauro Carvalho Chehab wrote:

Hi Sakari,

I answered you point to point below, but I suspect that you missed how
the current approach works. So, I decided to write a quick summary
here.

The character devices /dev/media? are created via cdev, with relies on
a kobject per device, with has an embedded struct kref inside.

Also, each kobj at /dev/media0, /dev/media1, ... is associated with a
struct device, when the code does:
  devnode->cdev.kobj.parent = >dev.kobj;

before calling cdev_add().

The current lifetime management is actually based on cdev's kobject's
refcount, provided by its embedded kref.

The kref warrants that any data associated with /dev/media0 won't be
freed if there are any pending system call. In other words, when
cdev_del() is called, it will remove /dev/media0 from the filesystem,
and will call kobject_put().

If the refcount is zero, it will call devnode->dev.release(). If the
kobject refcount is not zero, the data won't be freed.

So, in the best case scenario, there's no opened file descriptors
by the time media device node is unregistered. So, it will free
everything.

In the worse case scenario, e. g. when the driver is removed or
unbind while /dev/media0 has some opened file descriptor(s),
the cdev logic will do the proper lifetime management.

On such case, /dev/media0 disappears from the file system, so another
open is not possible anymore. The data structures will remain
allocated until all associated file descriptors are not closed.

When all file descriptors are closed, the data will be freed.

On that time, it will call an optional dev.release() callback,
responsible to free any other data struct that the driver allocated.


The patchset does not change this. It's not a question of the
media_devnode struct either. That's not an issue.

The issue is rather what else can be accessed through the media device
and other interfaces. As IOCTLs are not serialised with device removal
(which now releases much of the data structures)


Huh? ioctls are serialized with struct device removal. The Driver core
warrants that.


How?

As far as I can tell, there's nothing in the way of an IOCTL being in
progress on a character device which is registered by the driver for a
hardware device which is being removed.

vfs_ioctl() directly calls the unlocked_ioctl() file operation which is, in
case of MC, media_ioctl() in media-devnode.c. No mutexes (or other locks)
are taken during that path, which I believe is by design.


chrdev_open in fs/char_dev.c increases the refcount on open() and decreases it
on release(). Thus ensuring that the cdev can never be removed while in an
ioctl.




there's a high chance of accessing
released memory (or mutexes that have been already destroyed). An
example of that is here, stopping a running pipeline after unbinding
the device. What happens there is that the media device is released
whilst it's in use through the video device.




It is not clear from the logs what the driver tried to do, but
that sounds like a driver's bug, with was not prepared to properly
handle unbinds.

The problem here is that isp_video_release() is called by V4L2
release logic, and not by the MC one:

static const struct v4l2_file_operations isp_video_fops = {
  .owner  = THIS_MODULE,
  .open   = isp_video_open,
  .release= isp_video_release,
  .poll   = vb2_fop_poll,
  .unlocked_ioctl = video_ioctl2,
  .mmap   = vb2_fop_mmap,
};

It seems that the driver's logic allows it to be called before or
after destroying the MC.

Assuming that, if the OMAP3 driver is not used it works,
it means that, if the isp_video_release() is called
first, no errors will happen, but if MC is destroyed before
V4L2 call to its .release() callback, as there's no logic at the
driver that would detect it, isp_video_release() will be calling
isp_video_streamoff(), with depends on the MC to work.

On a first glance, I can see two ways of fixing it:

1) to increment devnode's device kobject refcount at OMAP3 .probe(),
decrementing it only at isp_video_release(). That will ensure that
MC will only be removed after V4L2 removal.


As soon as you have to dig deep in a structure to find a reference counter and
increment it, bypassing all the API layers, you can be entirely sure that the
solution is wrong.


Indeed.




2) to call isp_video_streamoff() before removing the MC 

Re: [PATCH v6 0/5] davinci: VPIF: add DT support

[Hans Verkuil]

On 07/12/16 19:30, Kevin Hilman wrote:

Prepare the groundwork for adding DT support for davinci VPIF drivers.
This series does some fixups/cleanups and then adds the DT binding and
DT compatible string matching for DT probing.

The controversial part from previous versions around async subdev
parsing, and specifically hard-coding the input/output routing of
subdevs, has been left out of this series.  That part can be done as a
follow-on step after agreement has been reached on the path forward.
With this version, platforms can still use the VPIF capture/display
drivers, but must provide platform_data for the subdevs and subdev
routing.

Tested video capture to memory on da850-lcdk board using composite
input.


Other than the comment for the first patch this series looks good.

So once that's addressed I'll queue it up for 4.11.

Regards,

Hans



Changes since v5:
- locking fix: updated comment around lock variable
- binding doc: added example for
- added reviewed-by tags from Laurent (thanks!)

Changes since v4:
- dropped controversial async subdev parsing support.  That can be
  done as a follow-up step after the discussions have finalized on the
right approach.
- DT binding Acked by DT maintainer (Rob H.)
- reworked locking fix (suggested by Laurent)

Changes since v3:
- move to a single VPIF node, DT binding updated accordingly
- misc fixes/updates based on reviews from Sakari

Changes since v2:
- DT binding doc: fix example to use correct compatible

Changes since v1:
- more specific compatible strings, based on SoC: ti,da850-vpif*
- fix locking bug when unlocking over subdev s_stream


Kevin Hilman (5):
  [media] davinci: VPIF: fix module loading, init errors
  [media] davinci: vpif_capture: remove hard-coded I2C adapter id
  [media] davinci: vpif_capture: fix start/stop streaming locking
  [media] dt-bindings: add TI VPIF documentation
  [media] davinci: VPIF: add basic support for DT init

 .../devicetree/bindings/media/ti,da850-vpif.txt| 83 ++
 drivers/media/platform/davinci/vpif.c  | 14 +++-
 drivers/media/platform/davinci/vpif_capture.c  | 26 +--
 drivers/media/platform/davinci/vpif_capture.h  |  2 +-
 drivers/media/platform/davinci/vpif_display.c  |  6 ++
 include/media/davinci/vpif_types.h |  1 +
 6 files changed, 125 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt



--
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 v6 1/5] [media] davinci: VPIF: fix module loading, init errors

[Hans Verkuil]

On 07/12/16 19:30, Kevin Hilman wrote:

Fix problems with automatic module loading by adding MODULE_ALIAS.  Also
fix various load-time errors cause by incorrect or not present
platform_data.

Signed-off-by: Kevin Hilman 
---
 drivers/media/platform/davinci/vpif.c |  5 -
 drivers/media/platform/davinci/vpif_capture.c | 15 ++-
 drivers/media/platform/davinci/vpif_display.c |  6 ++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif.c 
b/drivers/media/platform/davinci/vpif.c
index 0380cf2e5775..f50148dcba64 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -32,6 +32,9 @@
 MODULE_DESCRIPTION("TI DaVinci Video Port Interface driver");
 MODULE_LICENSE("GPL");

+#define VPIF_DRIVER_NAME   "vpif"
+MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
+
 #define VPIF_CH0_MAX_MODES 22
 #define VPIF_CH1_MAX_MODES 2
 #define VPIF_CH2_MAX_MODES 15
@@ -466,7 +469,7 @@ static const struct dev_pm_ops vpif_pm = {

 static struct platform_driver vpif_driver = {
.driver = {
-   .name   = "vpif",
+   .name   = VPIF_DRIVER_NAME,
.pm = vpif_pm_ops,
},
.remove = vpif_remove,
diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index 5104cc0ee40e..20c4344ed118 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -45,6 +45,7 @@ module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level 0-1");

 #define VPIF_DRIVER_NAME   "vpif_capture"
+MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);

 /* global variables */
 static struct vpif_device vpif_obj = { {NULL} };
@@ -647,6 +648,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;
@@ -654,7 +659,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))


Why this change? subdev_info can never be NULL.

Regards,

Hans


return i;
}
return -1;
@@ -685,6 +690,9 @@ static int vpif_set_input(
if (sd_index >= 0) {
sd = vpif_obj.sd[sd_index];
subdev_info = _cfg->subdev_info[sd_index];
+   } else {
+   /* no subdevice, no input to setup */
+   return 0;
}

/* first setup input path from sub device to vpif */
@@ -1435,6 +1443,11 @@ static __init int vpif_probe(struct platform_device 
*pdev)
int res_idx = 0;
int i, err;

+   if (!pdev->dev.platform_data) {
+   dev_warn(>dev, "Missing platform data.  Giving up.\n");
+   return -EINVAL;
+   }
+
vpif_dev = >dev;

err = initialize_vpif();
diff --git a/drivers/media/platform/davinci/vpif_display.c 
b/drivers/media/platform/davinci/vpif_display.c
index 75b27233ec2f..7f632b757d32 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -42,6 +42,7 @@ module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level 0-1");

 #define VPIF_DRIVER_NAME   "vpif_display"
+MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);

 /* Is set to 1 in case of SDTV formats, 2 in case of HDTV formats. */
 static int ycmux_mode;
@@ -1249,6 +1250,11 @@ static __init int vpif_probe(struct platform_device 
*pdev)
int res_idx = 0;
int i, err;

+   if (!pdev->dev.platform_data) {
+   dev_warn(>dev, "Missing platform data.  Giving up.\n");
+   return -EINVAL;
+   }
+
vpif_dev = >dev;
err = initialize_vpif();




--
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] gen-errors.rst: document EIO

[Hans Verkuil]

Document the EIO error since this can happen anywhere anytime and applications
should be aware of this.

Signed-off-by: Hans Verkuil 
---
diff --git a/Documentation/media/uapi/gen-errors.rst 
b/Documentation/media/uapi/gen-errors.rst
index 6e983b9..ee224b9 100644
--- a/Documentation/media/uapi/gen-errors.rst
+++ b/Documentation/media/uapi/gen-errors.rst
@@ -94,6 +94,14 @@ Generic Error Codes
-  Permission denied. Can be returned if the device needs write
  permission, or some special capabilities is needed (e. g. root)

+-  .. row 11
+
+   -  ``EIO``
+
+   -  I/O error. Typically used when there are problems communicating with
+  a hardware device. This could indicate broken or flaky hardware.
+ It's a 'Something is wrong, I give up!' type of error.
+
 .. note::

   #. This list is not exaustive; ioctls may return other error codes.
--
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 1/6] [media] rc-main: assign driver type during allocation

[Andi Shyti]

The driver type can be assigned immediately when an RC device
requests to the framework to allocate the device.

This is an 'enum rc_driver_type' data type and specifies whether
the device is a raw receiver or scancode receiver. The type will
be given as parameter to the rc_allocate_device device.

Change accordingly all the drivers calling rc_allocate_device()
so that the device type is specified during the rc device
allocation. Whenever the device type is not specified, it will be
set as RC_DRIVER_SCANCODE which was the default '0' value.

Suggested-by: Sean Young 
Signed-off-by: Andi Shyti 
Reviewed-by: Sean Young 
---
Hi,

Sorry for further spamming, but these drivers change so fast and
I'm taking too long to finalize this patch series.

This is the correct patch which fixes the compile error from the
kbuild test robot.

The whole patch series, anyway, is rebased on next-161216 and all
the files compile.

Thanks,
Andi

 drivers/hid/hid-picolcd_cir.c   |  3 +--
 drivers/media/cec/cec-core.c|  3 +--
 drivers/media/common/siano/smsir.c  |  3 +--
 drivers/media/i2c/ir-kbd-i2c.c  |  2 +-
 drivers/media/pci/bt8xx/bttv-input.c|  2 +-
 drivers/media/pci/cx23885/cx23885-input.c   | 11 +--
 drivers/media/pci/cx88/cx88-input.c |  3 +--
 drivers/media/pci/dm1105/dm1105.c   |  3 +--
 drivers/media/pci/mantis/mantis_input.c |  2 +-
 drivers/media/pci/saa7134/saa7134-input.c   |  2 +-
 drivers/media/pci/smipcie/smipcie-ir.c  |  3 +--
 drivers/media/pci/ttpci/budget-ci.c |  2 +-
 drivers/media/rc/ati_remote.c   |  3 +--
 drivers/media/rc/ene_ir.c   |  3 +--
 drivers/media/rc/fintek-cir.c   |  3 +--
 drivers/media/rc/gpio-ir-recv.c |  3 +--
 drivers/media/rc/igorplugusb.c  |  3 +--
 drivers/media/rc/iguanair.c |  3 +--
 drivers/media/rc/img-ir/img-ir-hw.c |  2 +-
 drivers/media/rc/img-ir/img-ir-raw.c|  3 +--
 drivers/media/rc/imon.c |  3 +--
 drivers/media/rc/ir-hix5hd2.c   |  3 +--
 drivers/media/rc/ite-cir.c  |  3 +--
 drivers/media/rc/mceusb.c   |  3 +--
 drivers/media/rc/meson-ir.c |  3 +--
 drivers/media/rc/nuvoton-cir.c  |  3 +--
 drivers/media/rc/rc-loopback.c  |  3 +--
 drivers/media/rc/rc-main.c  |  9 ++---
 drivers/media/rc/redrat3.c  |  3 +--
 drivers/media/rc/serial_ir.c|  3 +--
 drivers/media/rc/st_rc.c|  3 +--
 drivers/media/rc/streamzap.c|  3 +--
 drivers/media/rc/sunxi-cir.c|  3 +--
 drivers/media/rc/ttusbir.c  |  3 +--
 drivers/media/rc/winbond-cir.c  |  3 +--
 drivers/media/usb/au0828/au0828-input.c |  3 +--
 drivers/media/usb/cx231xx/cx231xx-input.c   |  2 +-
 drivers/media/usb/dvb-usb-v2/dvb_usb_core.c |  3 +--
 drivers/media/usb/dvb-usb/dvb-usb-remote.c  |  3 +--
 drivers/media/usb/em28xx/em28xx-input.c |  2 +-
 drivers/media/usb/tm6000/tm6000-input.c |  3 +--
 include/media/rc-core.h |  6 --
 42 files changed, 50 insertions(+), 85 deletions(-)

diff --git a/drivers/hid/hid-picolcd_cir.c b/drivers/hid/hid-picolcd_cir.c
index 9628651..38b0ea8 100644
--- a/drivers/hid/hid-picolcd_cir.c
+++ b/drivers/hid/hid-picolcd_cir.c
@@ -108,12 +108,11 @@ int picolcd_init_cir(struct picolcd_data *data, struct 
hid_report *report)
struct rc_dev *rdev;
int ret = 0;
 
-   rdev = rc_allocate_device();
+   rdev = rc_allocate_device(RC_DRIVER_IR_RAW);
if (!rdev)
return -ENOMEM;
 
rdev->priv = data;
-   rdev->driver_type  = RC_DRIVER_IR_RAW;
rdev->allowed_protocols = RC_BIT_ALL;
rdev->open = picolcd_cir_open;
rdev->close= picolcd_cir_close;
diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index aca3ab8..37217e2 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -239,7 +239,7 @@ struct cec_adapter *cec_allocate_adapter(const struct 
cec_adap_ops *ops,
 
 #if IS_REACHABLE(CONFIG_RC_CORE)
/* Prepare the RC input device */
-   adap->rc = rc_allocate_device();
+   adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
if (!adap->rc) {
pr_err("cec-%s: failed to allocate memory for rc_dev\n",
   name);
@@ -259,7 +259,6 @@ struct cec_adapter *cec_allocate_adapter(const struct 
cec_adap_ops *ops,
adap->rc->input_id.vendor = 0;
adap->rc->input_id.product = 0;
adap->rc->input_id.version = 1;
-   adap->rc->driver_type = RC_DRIVER_SCANCODE;
adap->rc->driver_name = CEC_NAME;
adap->rc->allowed_protocols = RC_BIT_CEC;
adap->rc->priv = adap;
diff 

Re: [PATCH RFC] omap3isp: prevent releasing MC too early

[Sakari Ailus]

Hi Mauro and Greg,

On Thu, Dec 15, 2016 at 12:17:58PM -0200, Mauro Carvalho Chehab wrote:
> Greg,
> 
> Em Thu, 15 Dec 2016 05:44:38 -0800
> Greg KH  escreveu:
> 
> > On Thu, Dec 15, 2016 at 10:57:16AM -0200, Mauro Carvalho Chehab wrote:
> > > Em Thu, 15 Dec 2016 09:42:35 -0300
> > > Javier Martinez Canillas  escreveu:
> > >   
> > > > Hello Mauro,
> > > > 
> > > > On 12/15/2016 09:37 AM, Mauro Carvalho Chehab wrote:
> > > > 
> > > > [snip]
> > > >   
> > > > > 
> > > > > What happens is that omap3isp driver calls media_device_unregister()
> > > > > too early. Right now, it is called at omap3isp_video_device_release(),
> > > > > with happens when a driver unbind is ordered by userspace, and not 
> > > > > after
> > > > > the last usage of all /dev/video?? devices.
> > > > > 
> > > > > There are two possible fixes:
> > > > > 
> > > > > 1) at omap3isp_video_device_release(), streamoff all streams and mark
> > > > > that the media device will be gone.  
> > > 
> > > I actually meant to say: isp_unregister_entities() here.
> > >   
> > > > > 
> > > > > 2) instead of using video_device_release_empty for the 
> > > > > video->video.release,
> > > > > create a omap3isp_video_device_release() that will call
> > > > > media_device_unregister() when destroying the last /dev/video?? 
> > > > > devnode.
> > > > >
> > > > 
> > > > There's also option (3), to have a proper refcounting to make sure that
> > > > the media device node is not freed until all references to it are gone. 
> > > >  
> > > 
> > > Yes, that's another alternative.
> > >   
> > > > I understand that's what Sakari's RFC patches do. I'll try to make some
> > > > time tomorrow to test and review his patches.  
> > > 
> > > The biggest problem with Sakari's patches is that it starts by 
> > > reverting 3 patches, and this will cause regressions on existing
> > > devices.
> > > 
> > > Development should be incremental.  
> > 
> > How can reverting patches cause regressions?  If a patch that is applied
> > breaks something else, it needs to be reverted, end of story.  If that
> > patch happened to have fixed a different issue, that's fine, we are back
> > to the original issue, it's not a "regression" at all, the patch was
> > wrong in the first place.
> > 
> > So please, just revert them now.  That's the correct thing to do, as we
> > will be back to the previous release's behavior.
> 
> The patches that Sakari want to revert are fixes. Before those patches, the
> cdev logic was broken. So, it used to generate OOPS when the media
> character device is removed.

I'd rather call them workarounds. They cause other issues (see below the
reply) and there are unaddressed review comments. They were still applied
because there was no good solution available at the time.








At least some of the issues the patches claim to fix are really not fixed
either. They are just made slightly less likely to accidentally stumble
upon.

> 
> So, when PCI/USB drivers gained media controller support, a regression
> was introduced. Those patches fix it, as without them, unbinding
> (or physically removing) PCI/USB devices cause OOPS because the cdev logic
> there are broken.
> 
> What happens is that Sakari is proposing a different approach to
> solve it on this 21 RFC patch series. Instead of applying his solution
> on the top of upstream code, he decided to start it by removing the
> regression fix patches.

The problems always existed, we just did not pay much attention to them, for
the reason you stated above. Instead of trying to fend off user space
accessing the device through IOCTL calls (which are _not_ serialised with
releasing of the data structures the IOCTL calls operate on), one really
must make sure the device is not being accessed from the user space through
e.g. IOCTLs while its memory is being released.

On V4L2 there's a release() callback to do that, on Media controller we
still have nothing --- something that's added by my RFC patchset. And yes,
it's still RFC and requires modifications, but it's not reasonable to
outright reject it (or not to review it) on the grounds that an unagreed
Media controller core API change isn't yet implemented for all potentially
affected drivers. Existing drivers still work without API changes, with the
caveat that they do have the same object lifetime issues which they did
before.

> 
> Also, the way his solution was designed, every single media driver using 
> the media controller needs to be modified in order for it to not cause OOPS
> at device unbind. However, the series only do such changes on OMAP3
> driver (and 

Re: [PATCH 2/2] media: omap3isp change to devm for resources

[kbuild test robot]

Hi Shuah,

[auto build test WARNING on v4.9-rc8]
[cannot apply to linuxtv-media/master next-20161215]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Shuah-Khan/omap3-devm-usage-removal/20161216-111439
config: arm-omap2plus_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   drivers/media/platform/omap3isp/isp.c: In function 'isp_probe':
>> drivers/media/platform/omap3isp/isp.c:2289:3: warning: this 'if' clause does 
>> not guard... [-Wmisleading-indentation]
  if (IS_ERR(isp->mmio_base[map_idx]))
  ^~
   drivers/media/platform/omap3isp/isp.c:2291:4: note: ...this statement, but 
the latter is misleadingly indented as if it is guarded by the 'if'
   goto error_put_vdd_csiphy2;
   ^~~~

vim +/if +2289 drivers/media/platform/omap3isp/isp.c

d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  
2273/* Clocks
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  
2274 *
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  
2275 * The ISP clock tree is revision-dependent. We thus need to enable 
ICLK
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  
2276 * manually to read the revision before calling __omap3isp_get().
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2277 *
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2278 * Start by mapping the ISP MMIO area, which is in two pieces.
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2279 * The ISP IOMMU is in between. Map both now, and fill in the
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2280 * ISP revision specific portions a little later in the
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2281 * function.
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27  
2282 */
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2283for (i = 0; i < 2; i++) {
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2284unsigned int map_idx = i ? OMAP3_ISP_IOMEM_CSI2A_REGS1 : 0;
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2285  
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2286mem = platform_get_resource(pdev, IORESOURCE_MEM, i);
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2287isp->mmio_base[map_idx] =
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2288devm_ioremap_resource(isp->dev, mem);
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 
@2289if (IS_ERR(isp->mmio_base[map_idx]))
27e86e5f drivers/media/platform/omap3isp/isp.c Shuah Khan   2016-12-15  
2290ret = PTR_ERR(isp->mmio_base[map_idx]);
27e86e5f drivers/media/platform/omap3isp/isp.c Shuah Khan   2016-12-15  
2291goto error_put_vdd_csiphy2;
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25  
2292}
448de7e7 drivers/media/video/omap3isp/isp.cSakari Ailus 2011-02-12  
2293  
448de7e7 drivers/media/video/omap3isp/isp.cSakari Ailus 2011-02-12  
2294ret = isp_get_clocks(isp);
448de7e7 drivers/media/video/omap3isp/isp.cSakari Ailus 2011-02-12  
2295if (ret < 0)
27e86e5f drivers/media/platform/omap3isp/isp.c Shuah Khan   2016-12-15  
2296goto error_put_vdd_csiphy2;
448de7e7 drivers/media/video/omap3isp/isp.cSakari Ailus 2011-02-12  
2297  

:: The code at line 2289 was first introduced by commit
:: 8644cdf972dd6dfebf98161025900f6a9d1ad58a [media] omap3isp: Replace many 
MMIO regions by two

:: TO: Sakari Ailus <sakari.ai...@iki.fi>
:: CC: Mauro Carvalho Chehab <mche...@osg.samsung.com>

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


.config.gz
Description: application/gzip