Re: [PATCH 6/7] si2168: Announce frontend creation failure

[Antti Palosaari]

hmmm, IIRC driver core even prints some error when driver probe fails? 
After that you could enable module debug logging to see more 
information. So I don't see point for that change.


regards
Antti

On 01/12/2018 06:19 PM, Brad Love wrote:

The driver outputs on success, but is silent on failure. Give
one message that probe failed.

Signed-off-by: Brad Love 
---
  drivers/media/dvb-frontends/si2168.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/si2168.c 
b/drivers/media/dvb-frontends/si2168.c
index 429c03a..c1a638c 100644
--- a/drivers/media/dvb-frontends/si2168.c
+++ b/drivers/media/dvb-frontends/si2168.c
@@ -810,7 +810,7 @@ static int si2168_probe(struct i2c_client *client,
  err_kfree:
kfree(dev);
  err:
-   dev_dbg(>dev, "failed=%d\n", ret);
+   dev_warn(>dev, "probe failed = %d\n", ret);
return ret;
  }
  



--
http://palosaari.fi/

Re: [PATCH 4/7] si2168: Add ts bus coontrol, turn off bus on sleep

[Antti Palosaari]

Hello
And what is rationale here, is there some use case demod must be active 
and ts set to tristate (disabled)? Just put demod sleep when you don't 
use it.


regards
Antti

On 01/12/2018 06:19 PM, Brad Love wrote:

Includes a function to set TS MODE property os si2168. The function
either disables the TS output bus, or sets mode to config option.

When going to sleep the TS bus is turned off, this makes the driver
compatible with multiple frontend usage.

Signed-off-by: Brad Love 
---
  drivers/media/dvb-frontends/si2168.c | 38 
  drivers/media/dvb-frontends/si2168.h |  1 +
  2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/media/dvb-frontends/si2168.c 
b/drivers/media/dvb-frontends/si2168.c
index 539399d..429c03a 100644
--- a/drivers/media/dvb-frontends/si2168.c
+++ b/drivers/media/dvb-frontends/si2168.c
@@ -409,6 +409,30 @@ static int si2168_set_frontend(struct dvb_frontend *fe)
return ret;
  }
  
+static int si2168_ts_bus_ctrl(struct dvb_frontend *fe, int acquire)

+{
+   struct i2c_client *client = fe->demodulator_priv;
+   struct si2168_dev *dev = i2c_get_clientdata(client);
+   struct si2168_cmd cmd;
+   int ret = 0;
+
+   dev_dbg(>dev, "%s acquire: %d\n", __func__, acquire);
+
+   /* set TS_MODE property */
+   memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6);
+   if (acquire)
+   cmd.args[4] |= dev->ts_mode;
+   else
+   cmd.args[4] |= SI2168_TS_TRISTATE;
+   if (dev->ts_clock_gapped)
+   cmd.args[4] |= 0x40;
+   cmd.wlen = 6;
+   cmd.rlen = 4;
+   ret = si2168_cmd_execute(client, );
+
+   return ret;
+}
+
  static int si2168_init(struct dvb_frontend *fe)
  {
struct i2c_client *client = fe->demodulator_priv;
@@ -540,14 +564,7 @@ static int si2168_init(struct dvb_frontend *fe)
 dev->version >> 24 & 0xff, dev->version >> 16 & 0xff,
 dev->version >> 8 & 0xff, dev->version >> 0 & 0xff);
  
-	/* set ts mode */

-   memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6);
-   cmd.args[4] |= dev->ts_mode;
-   if (dev->ts_clock_gapped)
-   cmd.args[4] |= 0x40;
-   cmd.wlen = 6;
-   cmd.rlen = 4;
-   ret = si2168_cmd_execute(client, );
+   ret = si2168_ts_bus_ctrl(fe, 1);
if (ret)
goto err;
  
@@ -584,6 +601,9 @@ static int si2168_sleep(struct dvb_frontend *fe)
  
  	dev->active = false;
  
+	/* tri-state data bus */

+   si2168_ts_bus_ctrl(fe, 0);
+
/* Firmware B 4.0-11 or later loses warm state during sleep */
if (dev->version > ('B' << 24 | 4 << 16 | 0 << 8 | 11 << 0))
dev->warm = false;
@@ -681,6 +701,8 @@ static const struct dvb_frontend_ops si2168_ops = {
.init = si2168_init,
.sleep = si2168_sleep,
  
+	.ts_bus_ctrl  = si2168_ts_bus_ctrl,

+
.set_frontend = si2168_set_frontend,
  
  	.read_status = si2168_read_status,

diff --git a/drivers/media/dvb-frontends/si2168.h 
b/drivers/media/dvb-frontends/si2168.h
index 3225d0c..f48f0fb 100644
--- a/drivers/media/dvb-frontends/si2168.h
+++ b/drivers/media/dvb-frontends/si2168.h
@@ -38,6 +38,7 @@ struct si2168_config {
/* TS mode */
  #define SI2168_TS_PARALLEL0x06
  #define SI2168_TS_SERIAL  0x03
+#define SI2168_TS_TRISTATE 0x00
u8 ts_mode;
  
  	/* TS clock inverted */




--
http://palosaari.fi/

Re: [PATCH 3/7] si2157: Add hybrid tuner support

[Antti Palosaari]

Hello
So the use case is to share single tuner with multiple demodulators? Why 
don't just register single tuner and pass that info to multiple demods?


Antti

On 01/12/2018 06:19 PM, Brad Love wrote:

Add ability to share a tuner amongst demodulators. Addtional
demods are attached using hybrid_tuner_instance_list.

The changes are equivalent to moving all of probe to _attach.
Results are backwards compatible with current usage.

If the tuner is acquired via attach, then .release cleans state.
if the tuner is an i2c driver, then .release is set to NULL, and
.remove cleans remaining state.

The following file contains a static si2157_attach:
- drivers/media/pci/saa7164/saa7164-dvb.c
The function name has been appended with _priv to appease
the compiler.

Signed-off-by: Brad Love 
---
  drivers/media/pci/saa7164/saa7164-dvb.c |  11 +-
  drivers/media/tuners/si2157.c   | 232 +++-
  drivers/media/tuners/si2157.h   |  14 ++
  drivers/media/tuners/si2157_priv.h  |   5 +
  4 files changed, 192 insertions(+), 70 deletions(-)

diff --git a/drivers/media/pci/saa7164/saa7164-dvb.c 
b/drivers/media/pci/saa7164/saa7164-dvb.c
index e76d3ba..9522c6c 100644
--- a/drivers/media/pci/saa7164/saa7164-dvb.c
+++ b/drivers/media/pci/saa7164/saa7164-dvb.c
@@ -110,8 +110,9 @@ static struct si2157_config hauppauge_hvr2255_tuner_config 
= {
.if_port = 1,
  };
  
-static int si2157_attach(struct saa7164_port *port, struct i2c_adapter *adapter,

-   struct dvb_frontend *fe, u8 addr8bit, struct si2157_config *cfg)
+static int si2157_attach_priv(struct saa7164_port *port,
+   struct i2c_adapter *adapter, struct dvb_frontend *fe,
+   u8 addr8bit, struct si2157_config *cfg)
  {
struct i2c_board_info bi;
struct i2c_client *tuner;
@@ -624,11 +625,13 @@ int saa7164_dvb_register(struct saa7164_port *port)
if (port->dvb.frontend != NULL) {
  
  			if (port->nr == 0) {

-   si2157_attach(port, >i2c_bus[0].i2c_adap,
+   si2157_attach_priv(port,
+ >i2c_bus[0].i2c_adap,
  port->dvb.frontend, 0xc0,
  _hvr2255_tuner_config);
} else {
-   si2157_attach(port, >i2c_bus[1].i2c_adap,
+   si2157_attach_priv(port,
+ >i2c_bus[1].i2c_adap,
  port->dvb.frontend, 0xc0,
  _hvr2255_tuner_config);
}
diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index e35b1fa..9121361 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -18,6 +18,11 @@
  
  static const struct dvb_tuner_ops si2157_ops;
  
+static DEFINE_MUTEX(si2157_list_mutex);

+static LIST_HEAD(hybrid_tuner_instance_list);
+
+/*-*/
+
  /* execute firmware command */
  static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd 
*cmd)
  {
@@ -385,6 +390,31 @@ static int si2157_get_if_frequency(struct dvb_frontend 
*fe, u32 *frequency)
return 0;
  }
  
+static void si2157_release(struct dvb_frontend *fe)

+{
+   struct i2c_client *client = fe->tuner_priv;
+   struct si2157_dev *dev = i2c_get_clientdata(client);
+
+   dev_dbg(>dev, "%s()\n", __func__);
+
+   /* only do full cleanup on final instance */
+   if (hybrid_tuner_report_instance_count(dev) == 1) {
+   /* stop statistics polling */
+   cancel_delayed_work_sync(>stat_work);
+#ifdef CONFIG_MEDIA_CONTROLLER_DVB
+   if (dev->mdev)
+   media_device_unregister_entity(>ent);
+#endif
+   i2c_set_clientdata(client, NULL);
+   }
+
+   mutex_lock(_list_mutex);
+   hybrid_tuner_release_state(dev);
+   mutex_unlock(_list_mutex);
+
+   fe->tuner_priv = NULL;
+}
+
  static const struct dvb_tuner_ops si2157_ops = {
.info = {
.name   = "Silicon Labs 
Si2141/Si2146/2147/2148/2157/2158",
@@ -396,6 +426,7 @@ static const struct dvb_tuner_ops si2157_ops = {
.sleep = si2157_sleep,
.set_params = si2157_set_params,
.get_if_frequency = si2157_get_if_frequency,
+   .release = si2157_release,
  };
  
  static void si2157_stat_work(struct work_struct *work)

@@ -431,72 +462,30 @@ static int si2157_probe(struct i2c_client *client,
  {
struct si2157_config *cfg = client->dev.platform_data;
struct dvb_frontend *fe = cfg->fe;
-   struct si2157_dev *dev;
-   struct si2157_cmd cmd;
-   int ret;
-
-   dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-   if (!dev) {
-   ret = -ENOMEM;
-   

cron job: media_tree daily build: ERRORS

[Hans Verkuil]

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

Results of the daily build of media_tree:

date:   Tue Jan 16 05:00:17 CET 2018
media-tree git hash:e3ee691dbf24096ea51b3200946b11d68ce75361
media_build git hash:   2bd1f1623fbadfdc1026712b3d55141ba164c403
v4l-utils git hash: 2c46e73aa82d913183eb4e83a8f2c9262718f2e2
gcc version:i686-linux-gcc (GCC) 7.1.0
sparse version: v0.5.0-3911-g6f737e1f
smatch version: v0.5.0-3911-g6f737e1f
host hardware:  x86_64
host os:4.13.0-164

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-arm-stm32: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: ERRORS
linux-2.6.37.6-i686: ERRORS
linux-2.6.38.8-i686: ERRORS
linux-2.6.39.4-i686: ERRORS
linux-3.0.60-i686: ERRORS
linux-3.1.10-i686: ERRORS
linux-3.2.37-i686: ERRORS
linux-3.3.8-i686: ERRORS
linux-3.4.27-i686: ERRORS
linux-3.5.7-i686: ERRORS
linux-3.6.11-i686: ERRORS
linux-3.7.4-i686: ERRORS
linux-3.8-i686: ERRORS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: ERRORS
linux-3.12.67-i686: ERRORS
linux-3.13.11-i686: ERRORS
linux-3.14.9-i686: ERRORS
linux-3.15.2-i686: ERRORS
linux-3.16.7-i686: ERRORS
linux-3.17.8-i686: ERRORS
linux-3.18.7-i686: ERRORS
linux-3.19-i686: ERRORS
linux-4.0.9-i686: ERRORS
linux-4.1.33-i686: ERRORS
linux-4.2.8-i686: ERRORS
linux-4.3.6-i686: ERRORS
linux-4.4.22-i686: ERRORS
linux-4.5.7-i686: ERRORS
linux-4.6.7-i686: ERRORS
linux-4.7.5-i686: ERRORS
linux-4.8-i686: ERRORS
linux-4.9.26-i686: ERRORS
linux-4.10.14-i686: WARNINGS
linux-4.11-i686: WARNINGS
linux-4.12.1-i686: WARNINGS
linux-4.13-i686: WARNINGS
linux-4.14-i686: WARNINGS
linux-2.6.36.4-x86_64: ERRORS
linux-2.6.37.6-x86_64: ERRORS
linux-2.6.38.8-x86_64: ERRORS
linux-2.6.39.4-x86_64: ERRORS
linux-3.0.60-x86_64: ERRORS
linux-3.1.10-x86_64: ERRORS
linux-3.2.37-x86_64: ERRORS
linux-3.3.8-x86_64: ERRORS
linux-3.4.27-x86_64: ERRORS
linux-3.5.7-x86_64: ERRORS
linux-3.6.11-x86_64: ERRORS
linux-3.7.4-x86_64: ERRORS
linux-3.8-x86_64: ERRORS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: ERRORS
linux-3.12.67-x86_64: ERRORS
linux-3.13.11-x86_64: ERRORS
linux-3.14.9-x86_64: ERRORS
linux-3.15.2-x86_64: ERRORS
linux-3.16.7-x86_64: ERRORS
linux-3.17.8-x86_64: ERRORS
linux-3.18.7-x86_64: ERRORS
linux-3.19-x86_64: ERRORS
linux-4.0.9-x86_64: ERRORS
linux-4.1.33-x86_64: ERRORS
linux-4.2.8-x86_64: ERRORS
linux-4.3.6-x86_64: ERRORS
linux-4.4.22-x86_64: ERRORS
linux-4.5.7-x86_64: ERRORS
linux-4.6.7-x86_64: ERRORS
linux-4.7.5-x86_64: ERRORS
linux-4.8-x86_64: ERRORS
linux-4.9.26-x86_64: ERRORS
linux-4.10.14-x86_64: WARNINGS
linux-4.11-x86_64: WARNINGS
linux-4.12.1-x86_64: WARNINGS
linux-4.13-x86_64: WARNINGS
linux-4.14-x86_64: WARNINGS
apps: OK
spec-git: OK
smatch: OK

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Tomasz Figa]

Hi Bingbu,

On Tue, Jan 16, 2018 at 1:05 PM, Cao, Bingbu  wrote:
> I think if set the pages as the DIV_ROUND_UP(vb->planes[0].length, 
> CIO2_PAGE_SIZE) + 1, the ' if (!pages--)' in loop is not correct.
> should be 'if (!--pages)'.
> The last page from sg list is the last valid page.
>

Yes, that's exactly what I meant.

By the way, please avoid top-posting to mailing lists, it isn't well
received by recipients.

Best regards,
Tomasz

>
> __
> BRs,
> Cao, Bingbu
>
>
>
>> -Original Message-
>> From: Tomasz Figa [mailto:tf...@chromium.org]
>> Sent: Tuesday, January 16, 2018 10:40 AM
>> To: Zhi, Yong 
>> Cc: Linux Media Mailing List ; Sakari Ailus
>> ; Mani, Rajmohan ;
>> Cao, Bingbu 
>> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-
>> of-bounds access
>>
>> Hi Yong,
>>
>> On Tue, Jan 16, 2018 at 2:05 AM, Zhi, Yong  wrote:
>> > Hi, Tomasz,
>> >
>> > Thanks for the patch review.
>> >
>> >> -Original Message-
>> >> From: Tomasz Figa [mailto:tf...@chromium.org]
>> >> Sent: Friday, January 12, 2018 12:17 AM
>> >> To: Zhi, Yong 
>> >> Cc: Linux Media Mailing List ; Sakari
>> >> Ailus ; Mani, Rajmohan
>> >> ; Cao, Bingbu 
>> >> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with
>> >> out-of- bounds access
>> >>
>> >> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi  wrote:
>> >> > When dmabuf is used for BLOB type frame, the frame buffers
>> >> > allocated by gralloc will hold more pages than the valid frame data
>> >> > due to height alignment.
>> >> >
>> >> > In this case, the page numbers in sg list could exceed the FBPT
>> >> > upper limit value - max_lops(8)*1024 to cause crash.
>> >> >
>> >> > Limit the LOP access to the valid data length to avoid FBPT
>> >> > sub-entries overflow.
>> >> >
>> >> > Signed-off-by: Yong Zhi 
>> >> > Signed-off-by: Cao Bing Bu 
>> >> > ---
>> >> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +--
>> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> >> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> >> > index 941caa987dab..949f43d206ad 100644
>> >> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> >> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> >> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer
>> *vb)
>> >> > container_of(vb, struct cio2_buffer, vbb.vb2_buf);
>> >> > static const unsigned int entries_per_page =
>> >> > CIO2_PAGE_SIZE / sizeof(u32);
>> >> > -   unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
>> >> CIO2_PAGE_SIZE);
>> >> > -   unsigned int lops = DIV_ROUND_UP(pages + 1,
>> entries_per_page);
>> >> > +   unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
>> >> > + CIO2_PAGE_SIZE) + 1;
>> >>
>> >> Why + 1? This would still overflow the buffer, wouldn't it?
>> >
>> > The "pages" variable is used to calculate lops which has one extra
>> page at the end that points to dummy page.
>> >
>> >>
>> >> > +   unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
>> >> > struct sg_table *sg;
>> >> > struct sg_page_iter sg_iter;
>> >> > int i, j;
>> >> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer
>> >> > *vb)
>> >> >
>> >> > i = j = 0;
>> >> > for_each_sg_page(sg->sgl, _iter, sg->nents, 0) {
>> >> > +   if (!pages--)
>> >> > +   break;
>> >>
>> >> Or perhaps we should check here for (pages > 1)?
>> >
>> > This is so that the end of lop is set to the dummy_page.
>>
>> How about this simple example:
>>
>> vb->planes[0].length = 1023 * 4096
>> pages = 1023 + 1 = 1024
>> lops  = 1
>>
>> If sg->sgl includes more than 1023 pages, the for_each_sg_page() loop
>> will iterate for pages from 1024 to 1 inclusive and ends up overflowing
>> the dummy page to next lop (i == 1 and j == 0), but we only allocated 1
>> lop.
>>
>> Best regards,
>> Tomasz

RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Cao, Bingbu]

I think if set the pages as the DIV_ROUND_UP(vb->planes[0].length, 
CIO2_PAGE_SIZE) + 1, the ' if (!pages--)' in loop is not correct.
should be 'if (!--pages)'.
The last page from sg list is the last valid page.


__
BRs,
Cao, Bingbu



> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Tuesday, January 16, 2018 10:40 AM
> To: Zhi, Yong 
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan ;
> Cao, Bingbu 
> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-
> of-bounds access
> 
> Hi Yong,
> 
> On Tue, Jan 16, 2018 at 2:05 AM, Zhi, Yong  wrote:
> > Hi, Tomasz,
> >
> > Thanks for the patch review.
> >
> >> -Original Message-
> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> Sent: Friday, January 12, 2018 12:17 AM
> >> To: Zhi, Yong 
> >> Cc: Linux Media Mailing List ; Sakari
> >> Ailus ; Mani, Rajmohan
> >> ; Cao, Bingbu 
> >> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with
> >> out-of- bounds access
> >>
> >> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi  wrote:
> >> > When dmabuf is used for BLOB type frame, the frame buffers
> >> > allocated by gralloc will hold more pages than the valid frame data
> >> > due to height alignment.
> >> >
> >> > In this case, the page numbers in sg list could exceed the FBPT
> >> > upper limit value - max_lops(8)*1024 to cause crash.
> >> >
> >> > Limit the LOP access to the valid data length to avoid FBPT
> >> > sub-entries overflow.
> >> >
> >> > Signed-off-by: Yong Zhi 
> >> > Signed-off-by: Cao Bing Bu 
> >> > ---
> >> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +--
> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> >> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> >> > index 941caa987dab..949f43d206ad 100644
> >> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> >> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> >> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer
> *vb)
> >> > container_of(vb, struct cio2_buffer, vbb.vb2_buf);
> >> > static const unsigned int entries_per_page =
> >> > CIO2_PAGE_SIZE / sizeof(u32);
> >> > -   unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> >> CIO2_PAGE_SIZE);
> >> > -   unsigned int lops = DIV_ROUND_UP(pages + 1,
> entries_per_page);
> >> > +   unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> >> > + CIO2_PAGE_SIZE) + 1;
> >>
> >> Why + 1? This would still overflow the buffer, wouldn't it?
> >
> > The "pages" variable is used to calculate lops which has one extra
> page at the end that points to dummy page.
> >
> >>
> >> > +   unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
> >> > struct sg_table *sg;
> >> > struct sg_page_iter sg_iter;
> >> > int i, j;
> >> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer
> >> > *vb)
> >> >
> >> > i = j = 0;
> >> > for_each_sg_page(sg->sgl, _iter, sg->nents, 0) {
> >> > +   if (!pages--)
> >> > +   break;
> >>
> >> Or perhaps we should check here for (pages > 1)?
> >
> > This is so that the end of lop is set to the dummy_page.
> 
> How about this simple example:
> 
> vb->planes[0].length = 1023 * 4096
> pages = 1023 + 1 = 1024
> lops  = 1
> 
> If sg->sgl includes more than 1023 pages, the for_each_sg_page() loop
> will iterate for pages from 1024 to 1 inclusive and ends up overflowing
> the dummy page to next lop (i == 1 and j == 0), but we only allocated 1
> lop.
> 
> Best regards,
> Tomasz

Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings

[Tomasz Figa]

On Tue, Jan 16, 2018 at 2:07 AM, Zhi, Yong  wrote:
> Hi, Tomasz,
>
> Thanks for reviewing the patch.
>
>> -Original Message-
>> From: Tomasz Figa [mailto:tf...@chromium.org]
>> Sent: Friday, January 12, 2018 12:19 AM
>> To: Zhi, Yong 
>> Cc: Linux Media Mailing List ; Sakari Ailus
>> ; Mani, Rajmohan
>> ; Cao, Bingbu 
>> Subject: Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state
>> warnings
>>
>> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi  wrote:
>> > cio2 driver should release buffer with QUEUED state when start_stream
>> > op failed, wrong buffer state will cause vb2 core throw a warning.
>> >
>> > Signed-off-by: Yong Zhi 
>> > Signed-off-by: Cao Bing Bu 
>> > ---
>> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +
>> >  1 file changed, 5 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > index 949f43d206ad..106d04306372 100644
>> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > @@ -785,7 +785,8 @@ static irqreturn_t cio2_irq(int irq, void
>> > *cio2_ptr)
>> >
>> >  / Videobuf2 interface /
>> >
>> > -static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
>> > +static void cio2_vb2_return_all_buffers(struct cio2_queue *q,
>> > +   enum vb2_buffer_state state)
>> >  {
>> > unsigned int i;
>> >
>> > @@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct
>> cio2_queue *q)
>> > if (q->bufs[i]) {
>> > atomic_dec(>bufs_queued);
>> > vb2_buffer_done(>bufs[i]->vbb.vb2_buf,
>> > -   VB2_BUF_STATE_ERROR);
>> > +   state);
>>
>> nit: Does it really exceed 80 characters after folding into previous line?
>>
>
> Thanks for catching this, seems this patch was merged, may I fix it in future 
> patch?

It's just a nit that I was hoping to be fixed at patch applying time.
Otherwise just never mind.

Best regards,
Tomasz

Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Tomasz Figa]

Hi Yong,

On Tue, Jan 16, 2018 at 2:05 AM, Zhi, Yong  wrote:
> Hi, Tomasz,
>
> Thanks for the patch review.
>
>> -Original Message-
>> From: Tomasz Figa [mailto:tf...@chromium.org]
>> Sent: Friday, January 12, 2018 12:17 AM
>> To: Zhi, Yong 
>> Cc: Linux Media Mailing List ; Sakari Ailus
>> ; Mani, Rajmohan
>> ; Cao, Bingbu 
>> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-
>> bounds access
>>
>> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi  wrote:
>> > When dmabuf is used for BLOB type frame, the frame buffers allocated
>> > by gralloc will hold more pages than the valid frame data due to
>> > height alignment.
>> >
>> > In this case, the page numbers in sg list could exceed the FBPT upper
>> > limit value - max_lops(8)*1024 to cause crash.
>> >
>> > Limit the LOP access to the valid data length to avoid FBPT
>> > sub-entries overflow.
>> >
>> > Signed-off-by: Yong Zhi 
>> > Signed-off-by: Cao Bing Bu 
>> > ---
>> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > index 941caa987dab..949f43d206ad 100644
>> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>> > container_of(vb, struct cio2_buffer, vbb.vb2_buf);
>> > static const unsigned int entries_per_page =
>> > CIO2_PAGE_SIZE / sizeof(u32);
>> > -   unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
>> CIO2_PAGE_SIZE);
>> > -   unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
>> > +   unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
>> > + CIO2_PAGE_SIZE) + 1;
>>
>> Why + 1? This would still overflow the buffer, wouldn't it?
>
> The "pages" variable is used to calculate lops which has one extra page at 
> the end that points to dummy page.
>
>>
>> > +   unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
>> > struct sg_table *sg;
>> > struct sg_page_iter sg_iter;
>> > int i, j;
>> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer
>> > *vb)
>> >
>> > i = j = 0;
>> > for_each_sg_page(sg->sgl, _iter, sg->nents, 0) {
>> > +   if (!pages--)
>> > +   break;
>>
>> Or perhaps we should check here for (pages > 1)?
>
> This is so that the end of lop is set to the dummy_page.

How about this simple example:

vb->planes[0].length = 1023 * 4096
pages = 1023 + 1 = 1024
lops  = 1

If sg->sgl includes more than 1023 pages, the for_each_sg_page() loop
will iterate for pages from 1024 to 1 inclusive and ends up
overflowing the dummy page to next lop (i == 1 and j == 0), but we
only allocated 1 lop.

Best regards,
Tomasz

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Alexandre Courbot]

On Mon, Jan 15, 2018 at 9:01 PM, Gustavo Padovan  wrote:
> 2018-01-15 Alexandre Courbot :
>
>> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan  wrote:
>> > From: Gustavo Padovan 
>> >
>> > Explicit synchronization benefits a lot from ordered queues, they fit
>> > better in a pipeline with DRM for example so create a opt-in way for
>> > drivers notify videobuf2 that the queue is unordered.
>> >
>> > Drivers don't need implement it if the queue is ordered.
>>
>> This is going to make user-space believe that *all* vb2 drivers use
>> ordered queues by default, at least until non-ordered drivers catch up
>> with this change. Wouldn't it be less dangerous to do the opposite
>> (make queues non-ordered by default)?
>
> The rational behind this decision was because most formats/drivers are
> ordered so only a small amount of drivers need to changed. I think this
> was proposed by Hans on the Media Summit.

As long as all concerned drivers are updated we should be on the safe
side. At first I was surprised that we expose the ordering feature in
a negative tense, but if the vast majority of devices are ordered this
probably makes sense.

Re: [PATCH v2] vsp1: fix video output on R8A77970

[Laurent Pinchart]

Hi Sergei,

On Monday, 15 January 2018 18:06:48 EET Sergei Shtylyov wrote:
> On 01/15/2018 03:51 PM, Laurent Pinchart wrote:
> > On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote:
> >> Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970)
> >> but
> > 
> > I'm not sure there's a need to state my name in the commit message.
> 
> You were the author of the patch this one has in the Fixes: tag, so I
> thought that was appropriate. I can remove that if you don't want you name
> mentioned...

It's just that I thought it was more important to refer to the patch than to 
my name, developers reading the log will be more interested in the technical 
details than in my person :-)

> >> the video  output that VSP2-D sends to DU has a greenish garbage-like
> >> line
> > 
> > Why does the text in your patches (commit message, comments, ...) sometime
> > have double spaces between words ?
> 
> The text looks more pleasant (at least to me). Can remove them if you
> want...

I don't think we try to do typesetting in kernel code or commit messages, so 
I'd stick to single spaces if you don't mind.

> >> repeated every 8 or so screen rows.
> > 
> > Is it every "8 or so" rows, or exactly every 8 rows ?
> 
> You really want me to count the pixels?

I'd like to know a bit more about the systems, whether the green lines appear 
at the exact same interval, whether they bounce up and down or are always at 
the same place, ...

> >> It turns out that V3M has a teeny LIF register (at least it's
> >> documented!) that you need to set to some kind of a  magic value for the
> >> LIF to work correctly...
> >> 
> >> Based on the original (and large) patch by Daisuke Matsushita
> >> .
> > 
> > What else is in the big patch ? Is it available somewhere ?
> 
> Assorted changes gathered together only because they all bring the
> support for R8A77970. If you're really curious, here you are:
> 
> https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/reci
> pes-kernel/linux/linux-renesas/0030-arm64-renesas-r8a7797-Add-Renesas-R8A779
> 7-SoC-suppor.patch

Thank you.

> >> Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and
> >> VSP2-D instances")
> >> Signed-off-by: Sergei Shtylyov 
> >> 
> >> ---
> >> This patch is against the 'media_tree.git' repo's 'master' branch.
> >> 
> >> Changes in version 2:
> >> - added a  comment before the V3M SoC check;
> >> - fixed indetation in that check;
> >> - reformatted  the patch description.
> >> 
> >>   drivers/media/platform/vsp1/vsp1_lif.c  |   12 
> >>   drivers/media/platform/vsp1/vsp1_regs.h |5 +
> >>   2 files changed, 17 insertions(+)
> >> 
> >> Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c
> >> ===
> >> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c
> >> +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c
> >> @@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en
> >>(obth << VI6_LIF_CTRL_OBTH_SHIFT) |
> >>(format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) |
> >>VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
> >> +
> >> +  /*
> >> +   * R-Car V3M has the buffer attribute register you absolutely need
> >> +   * to write kinda magic value to  for the LIF to work correctly...
> >> +   */
> > 
> > I'm not sure about the "kinda" magic value. 1536 is very likely a buffer
> > size.
> 
> Well, that's only guessing. The manual doesn't say anything about what the
> number is.

Yes it's just an educated guess.

> > How about the following text ?
> > 
> > /*
> > 
> >  * On V3M the LBA register has to be set to a non-default value to
> 
> I'd then spell its full name, LIF0 Buffer Attribute register.

Sounds good to me.

> >  * guarantee proper operation (otherwise artifacts may appear on the
> >  * output). The value required by the datasheet is not documented but
> >  * is likely a buffer size or threshold.
> >  */
> 
> OK,  can change that (modulo the name).
> 
> > The commit message should also be updated to feel a bit less magic.
> 
> I'll see about it.
> 
> >> +  if ((entity->vsp1->version &
> >> +   (VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) ==
> >> +  (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) {
> >> +  vsp1_lif_write(lif, dl, VI6_LIF_LBA,
> >> + VI6_LIF_LBA_LBA0 |
> >> + (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> >> +  }
> >> 
> >>   }
> > 
> > The datasheet documents the register as being present on both V3M and M3-W
> > (and the test I've just run on H3 shows that the register is present there
> > as well). Should we program it on M3-W or leave it to the default value
> > that should be what is recommended by the datasheet for that SoC ?
> 
> If the default value matches what's 

[PATCH] [v4l-utils] buildsystem: Fix not reporting if libjpeg is not being used

[Chris Mayo]

Signed-off-by: Chris Mayo 
---

If configured --without-jpeg, currently see:

compile time options summary


Host OS: linux-gnu
X11: yes
GL : yes
glu: yes
libjpeg: 

 configure.ac | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index dc1e9cbf5..cfbdffd99 100644
--- a/configure.ac
+++ b/configure.ac
@@ -195,7 +195,8 @@ AS_IF([test "x$with_jpeg" != xno],
  [have_jpeg=no
   AC_MSG_WARN(cannot find libjpeg (v6 or 
later required))])],
[have_jpeg=no
-AC_MSG_WARN(cannot find libjpeg)])])
+AC_MSG_WARN(cannot find libjpeg)])],
+  [have_jpeg=no])
 
 AM_CONDITIONAL([HAVE_JPEG], [test x$have_jpeg = xyes])
 
-- 
2.13.6

Re: [RFT PATCH v3 0/6] Asynchronous UVC

[Philipp Zabel]

Hi Kieran,

On Fri, Jan 12, 2018 at 10:19 AM, Kieran Bingham
 wrote:
> This series has been tested on both the ZED and BRIO cameras on arm64
> platforms, however due to the intrinsic changes in the driver I would like to
> see it tested with other devices and other platforms, so I'd appreciate if
> anyone can test this on a range of USB cameras.

FWIW,

Tested-by: Philipp Zabel 

with a Lite-On internal Laptop Webcam, Logitech C910 (USB2 isoc),
Oculus Sensor (USB3 isoc), and Microsoft HoloLens Sensors (USB3 bulk).

regards
Philipp

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Gustavo Padovan]

2018-01-15 Hans Verkuil :

> On 01/15/2018 01:01 PM, Gustavo Padovan wrote:
> > 2018-01-15 Alexandre Courbot :
> > 
> >> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan  
> >> wrote:
> >>> From: Gustavo Padovan 
> >>>
> >>> Explicit synchronization benefits a lot from ordered queues, they fit
> >>> better in a pipeline with DRM for example so create a opt-in way for
> >>> drivers notify videobuf2 that the queue is unordered.
> >>>
> >>> Drivers don't need implement it if the queue is ordered.
> >>
> >> This is going to make user-space believe that *all* vb2 drivers use
> >> ordered queues by default, at least until non-ordered drivers catch up
> >> with this change. Wouldn't it be less dangerous to do the opposite
> >> (make queues non-ordered by default)?
> > 
> > The rational behind this decision was because most formats/drivers are
> > ordered so only a small amount of drivers need to changed. I think this
> > was proposed by Hans on the Media Summit.
> > 
> > I understand your concern. My question is how dangerous will it be. If
> > you are building a product you will make the changes in the driver if
> > they are not there yet, or if it is a distribution you'd never know
> > which driver/format you are using so you should be prepared for
> > everything.
> > 
> > AFAIK all Capture drivers are ordered and that is where I think fences
> > is most useful.
> 
> Right. What could be done is to mark all codec drivers as unordered initially
> ask the driver authors to verify this. All capture drivers using vb2 and not
> using REQUEUE are ordered.

That is a good way out.

> 
> One thing we haven't looked at is what to do with drivers that do not use vb2.
> Those won't support fences, but how will userspace know that fences are not
> supported? I'm not sure what the best method is for that.
> 
> I am leaning towards a new capability since this has to be advertised clearly.

The capability flag makes sense to me, I'll incorporate it as part of my
next patchset.

Gustavo

RE: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings

[Zhi, Yong]

Hi, Tomasz,

Thanks for reviewing the patch.

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Friday, January 12, 2018 12:19 AM
> To: Zhi, Yong 
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan
> ; Cao, Bingbu 
> Subject: Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state
> warnings
> 
> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi  wrote:
> > cio2 driver should release buffer with QUEUED state when start_stream
> > op failed, wrong buffer state will cause vb2 core throw a warning.
> >
> > Signed-off-by: Yong Zhi 
> > Signed-off-by: Cao Bing Bu 
> > ---
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > index 949f43d206ad..106d04306372 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > @@ -785,7 +785,8 @@ static irqreturn_t cio2_irq(int irq, void
> > *cio2_ptr)
> >
> >  / Videobuf2 interface /
> >
> > -static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
> > +static void cio2_vb2_return_all_buffers(struct cio2_queue *q,
> > +   enum vb2_buffer_state state)
> >  {
> > unsigned int i;
> >
> > @@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct
> cio2_queue *q)
> > if (q->bufs[i]) {
> > atomic_dec(>bufs_queued);
> > vb2_buffer_done(>bufs[i]->vbb.vb2_buf,
> > -   VB2_BUF_STATE_ERROR);
> > +   state);
> 
> nit: Does it really exceed 80 characters after folding into previous line?
> 

Thanks for catching this, seems this patch was merged, may I fix it in future 
patch?

> With the nit fixed:
> Reviewed-by: Tomasz Figa 
> 
> Best regards,
> Tomasz

RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Zhi, Yong]

Hi, Tomasz,

Thanks for the patch review.

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Friday, January 12, 2018 12:17 AM
> To: Zhi, Yong 
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan
> ; Cao, Bingbu 
> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-
> bounds access
> 
> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi  wrote:
> > When dmabuf is used for BLOB type frame, the frame buffers allocated
> > by gralloc will hold more pages than the valid frame data due to
> > height alignment.
> >
> > In this case, the page numbers in sg list could exceed the FBPT upper
> > limit value - max_lops(8)*1024 to cause crash.
> >
> > Limit the LOP access to the valid data length to avoid FBPT
> > sub-entries overflow.
> >
> > Signed-off-by: Yong Zhi 
> > Signed-off-by: Cao Bing Bu 
> > ---
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > index 941caa987dab..949f43d206ad 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
> > container_of(vb, struct cio2_buffer, vbb.vb2_buf);
> > static const unsigned int entries_per_page =
> > CIO2_PAGE_SIZE / sizeof(u32);
> > -   unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> CIO2_PAGE_SIZE);
> > -   unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
> > +   unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> > + CIO2_PAGE_SIZE) + 1;
> 
> Why + 1? This would still overflow the buffer, wouldn't it?

The "pages" variable is used to calculate lops which has one extra page at the 
end that points to dummy page.

> 
> > +   unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
> > struct sg_table *sg;
> > struct sg_page_iter sg_iter;
> > int i, j;
> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer
> > *vb)
> >
> > i = j = 0;
> > for_each_sg_page(sg->sgl, _iter, sg->nents, 0) {
> > +   if (!pages--)
> > +   break;
> 
> Or perhaps we should check here for (pages > 1)?

This is so that the end of lop is set to the dummy_page.

> 
> Best regards,
> Tomasz

[PATCH v5 5/9] v4l: vsp1: Use reference counting for bodies

[Kieran Bingham]

Extend the display list body with a reference count, allowing bodies to
be kept as long as a reference is maintained. This provides the ability
to keep a cached copy of bodies which will not change, so that they can
be re-applied to multiple display lists.

Signed-off-by: Kieran Bingham 

---
This could be squashed into the body update code, but it's not a
straightforward squash as the refcounts will affect both:
  v4l: vsp1: Provide a body pool
and
  v4l: vsp1: Convert display lists to use new body pool
therefore, I have kept this separate to prevent breaking bisectability
of the vsp-tests.

v3:
 - 's/fragment/body/'

v4:
 - Fix up reference handling comments.
---
 drivers/media/platform/vsp1/vsp1_clu.c |  7 ++-
 drivers/media/platform/vsp1/vsp1_dl.c  | 15 ++-
 drivers/media/platform/vsp1/vsp1_lut.c |  7 ++-
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_clu.c 
b/drivers/media/platform/vsp1/vsp1_clu.c
index a765d56c4118..1142d004e238 100644
--- a/drivers/media/platform/vsp1/vsp1_clu.c
+++ b/drivers/media/platform/vsp1/vsp1_clu.c
@@ -216,8 +216,13 @@ static void clu_configure(struct vsp1_entity *entity,
clu->clu = NULL;
spin_unlock_irqrestore(>lock, flags);
 
-   if (dlb)
+   if (dlb) {
vsp1_dl_list_add_body(dl, dlb);
+
+   /* release our local reference */
+   vsp1_dl_body_put(dlb);
+   }
+
break;
}
 }
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index 961c3fd2ff1c..2784d3b48b02 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -58,6 +59,8 @@ struct vsp1_dl_body {
struct list_head list;
struct list_head free;
 
+   refcount_t refcnt;
+
struct vsp1_dl_body_pool *pool;
struct vsp1_device *vsp1;
 
@@ -259,6 +262,7 @@ struct vsp1_dl_body *vsp1_dl_body_get(struct 
vsp1_dl_body_pool *pool)
if (!list_empty(>free)) {
dlb = list_first_entry(>free, struct vsp1_dl_body, free);
list_del(>free);
+   refcount_set(>refcnt, 1);
}
 
spin_unlock_irqrestore(>lock, flags);
@@ -279,6 +283,9 @@ void vsp1_dl_body_put(struct vsp1_dl_body *dlb)
if (!dlb)
return;
 
+   if (!refcount_dec_and_test(>refcnt))
+   return;
+
dlb->num_entries = 0;
 
spin_lock_irqsave(>pool->lock, flags);
@@ -465,7 +472,11 @@ void vsp1_dl_list_write(struct vsp1_dl_list *dl, u32 reg, 
u32 data)
  * in the order in which bodies are added.
  *
  * Adding a body to a display list passes ownership of the body to the list. 
The
- * caller must not touch the body after this call.
+ * caller retains its reference to the fragment when adding it to the display
+ * list, but is not allowed to add new entries to the body.
+ *
+ * The reference must be explicitly released by a call to vsp1_dl_body_put()
+ * when the body isn't needed anymore.
  *
  * Additional bodies are only usable for display lists in header mode.
  * Attempting to add a body to a header-less display list will return an error.
@@ -477,6 +488,8 @@ int vsp1_dl_list_add_body(struct vsp1_dl_list *dl,
if (dl->dlm->mode != VSP1_DL_MODE_HEADER)
return -EINVAL;
 
+   refcount_inc(>refcnt);
+
list_add_tail(>list, >bodies);
return 0;
 }
diff --git a/drivers/media/platform/vsp1/vsp1_lut.c 
b/drivers/media/platform/vsp1/vsp1_lut.c
index a9cf874312c1..7643f18b1ea6 100644
--- a/drivers/media/platform/vsp1/vsp1_lut.c
+++ b/drivers/media/platform/vsp1/vsp1_lut.c
@@ -172,8 +172,13 @@ static void lut_configure(struct vsp1_entity *entity,
lut->lut = NULL;
spin_unlock_irqrestore(>lock, flags);
 
-   if (dlb)
+   if (dlb) {
vsp1_dl_list_add_body(dl, dlb);
+
+   /* release our local reference */
+   vsp1_dl_body_put(dlb);
+   }
+
break;
}
 }
-- 
git-series 0.9.1

[PATCH v5 6/9] v4l: vsp1: Refactor display list configure operations

[Kieran Bingham]

The entities provide a single .configure operation which configures the
object into the target display list, based on the vsp1_entity_params
selection.

This restricts us to a single function prototype for both static
configuration (the pre-stream INIT stage) and the dynamic runtime stages
for both each frame - and each partition therein.

Split the configure function into two parts, '.prepare()' and
'.configure()', merging both the VSP1_ENTITY_PARAMS_RUNTIME and
VSP1_ENTITY_PARAMS_PARTITION stages into a single call through the
.configure(). The configuration for individual partitions is handled by
passing the partition number to the configure call, and processing any
runtime stage actions on the first partition only.

Signed-off-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_bru.c|  12 +-
 drivers/media/platform/vsp1/vsp1_clu.c|  42 +--
 drivers/media/platform/vsp1/vsp1_dl.h |   1 +-
 drivers/media/platform/vsp1/vsp1_drm.c|  21 +--
 drivers/media/platform/vsp1/vsp1_entity.c |  15 +-
 drivers/media/platform/vsp1/vsp1_entity.h |  27 +--
 drivers/media/platform/vsp1/vsp1_hgo.c|  12 +-
 drivers/media/platform/vsp1/vsp1_hgt.c|  12 +-
 drivers/media/platform/vsp1/vsp1_hsit.c   |  12 +-
 drivers/media/platform/vsp1/vsp1_lif.c|  12 +-
 drivers/media/platform/vsp1/vsp1_lut.c|  24 +-
 drivers/media/platform/vsp1/vsp1_rpf.c| 162 ++---
 drivers/media/platform/vsp1/vsp1_sru.c|  12 +-
 drivers/media/platform/vsp1/vsp1_uds.c|  55 ++--
 drivers/media/platform/vsp1/vsp1_uif.c|  20 +--
 drivers/media/platform/vsp1/vsp1_video.c  |  24 +--
 drivers/media/platform/vsp1/vsp1_wpf.c| 297 ---
 17 files changed, 368 insertions(+), 392 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_bru.c 
b/drivers/media/platform/vsp1/vsp1_bru.c
index e8fd2ae3b3eb..b9ff96f76b3e 100644
--- a/drivers/media/platform/vsp1/vsp1_bru.c
+++ b/drivers/media/platform/vsp1/vsp1_bru.c
@@ -285,19 +285,15 @@ static const struct v4l2_subdev_ops bru_ops = {
  * VSP1 Entity Operations
  */
 
-static void bru_configure(struct vsp1_entity *entity,
- struct vsp1_pipeline *pipe,
- struct vsp1_dl_list *dl,
- enum vsp1_entity_params params)
+static void bru_prepare(struct vsp1_entity *entity,
+   struct vsp1_pipeline *pipe,
+   struct vsp1_dl_list *dl)
 {
struct vsp1_bru *bru = to_bru(>subdev);
struct v4l2_mbus_framefmt *format;
unsigned int flags;
unsigned int i;
 
-   if (params != VSP1_ENTITY_PARAMS_INIT)
-   return;
-
format = vsp1_entity_get_pad_format(>entity, bru->entity.config,
bru->entity.source_pad);
 
@@ -404,7 +400,7 @@ static void bru_configure(struct vsp1_entity *entity,
 }
 
 static const struct vsp1_entity_operations bru_entity_ops = {
-   .configure = bru_configure,
+   .prepare = bru_prepare,
 };
 
 /* 
-
diff --git a/drivers/media/platform/vsp1/vsp1_clu.c 
b/drivers/media/platform/vsp1/vsp1_clu.c
index 1142d004e238..006ad94bbe57 100644
--- a/drivers/media/platform/vsp1/vsp1_clu.c
+++ b/drivers/media/platform/vsp1/vsp1_clu.c
@@ -172,37 +172,36 @@ static const struct v4l2_subdev_ops clu_ops = {
 /* 
-
  * VSP1 Entity Operations
  */
+static void clu_prepare(struct vsp1_entity *entity,
+   struct vsp1_pipeline *pipe,
+   struct vsp1_dl_list *dl)
+{
+   struct vsp1_clu *clu = to_clu(>subdev);
+
+   /*
+* The yuv_mode can't be changed during streaming. Cache it internally
+* for future runtime configuration calls.
+*/
+   struct v4l2_mbus_framefmt *format;
+
+   format = vsp1_entity_get_pad_format(>entity,
+   clu->entity.config,
+   CLU_PAD_SINK);
+   clu->yuv_mode = format->code == MEDIA_BUS_FMT_AYUV8_1X32;
+}
 
 static void clu_configure(struct vsp1_entity *entity,
  struct vsp1_pipeline *pipe,
  struct vsp1_dl_list *dl,
- enum vsp1_entity_params params)
+ unsigned int partition)
 {
struct vsp1_clu *clu = to_clu(>subdev);
struct vsp1_dl_body *dlb;
unsigned long flags;
u32 ctrl = VI6_CLU_CTRL_AAI | VI6_CLU_CTRL_MVS | VI6_CLU_CTRL_EN;
 
-   switch (params) {
-   case VSP1_ENTITY_PARAMS_INIT: {
-   /*
-* The format can't be changed during streaming, only verify it
-* at setup time and store the information internally for future
-* runtime configuration calls.
-*/
-   

[PATCH v5 8/9] v4l: vsp1: Move video configuration to a cached dlb

[Kieran Bingham]

We are now able to configure a pipeline directly into a local display
list body. Take advantage of this fact, and create a cacheable body to
store the configuration of the pipeline in the video object.

vsp1_video_pipeline_run() is now the last user of the pipe->dl object.
Convert this function to use the cached video->config body and obtain a
local display list reference.

Attach the video->config body to the display list when needed before
committing to hardware.

The pipe object is marked as un-configured when resuming from a suspend.
This ensures that when the hardware is reset - our cached configuration
will be re-attached to the next committed DL.

Signed-off-by: Kieran Bingham 
---

v3:
 - 's/fragment/body/', 's/fragments/bodies/'
 - video dlb cache allocation increased from 2 to 3 dlbs

Our video DL usage now looks like the below output:

dl->body0 contains our disposable runtime configuration. Max 41.
dl_child->body0 is our partition specific configuration. Max 12.
dl->bodies shows our constant configuration and LUTs.

  These two are LUT/CLU:
 * dl->bodies[x]->num_entries 256 / max 256
 * dl->bodies[x]->num_entries 4914 / max 4914

Which shows that our 'constant' configuration cache is currently
utilised to a maximum of 64 entries.

trace-cmd report | \
grep max | sed 's/.*vsp1_dl_list_commit://g' | sort | uniq;

  dl->body0->num_entries 13 / max 128
  dl->body0->num_entries 14 / max 128
  dl->body0->num_entries 16 / max 128
  dl->body0->num_entries 20 / max 128
  dl->body0->num_entries 27 / max 128
  dl->body0->num_entries 34 / max 128
  dl->body0->num_entries 41 / max 128
  dl_child->body0->num_entries 10 / max 128
  dl_child->body0->num_entries 12 / max 128
  dl->bodies[x]->num_entries 15 / max 128
  dl->bodies[x]->num_entries 16 / max 128
  dl->bodies[x]->num_entries 17 / max 128
  dl->bodies[x]->num_entries 18 / max 128
  dl->bodies[x]->num_entries 20 / max 128
  dl->bodies[x]->num_entries 21 / max 128
  dl->bodies[x]->num_entries 256 / max 256
  dl->bodies[x]->num_entries 31 / max 128
  dl->bodies[x]->num_entries 32 / max 128
  dl->bodies[x]->num_entries 39 / max 128
  dl->bodies[x]->num_entries 40 / max 128
  dl->bodies[x]->num_entries 47 / max 128
  dl->bodies[x]->num_entries 48 / max 128
  dl->bodies[x]->num_entries 4914 / max 4914
  dl->bodies[x]->num_entries 55 / max 128
  dl->bodies[x]->num_entries 56 / max 128
  dl->bodies[x]->num_entries 63 / max 128
  dl->bodies[x]->num_entries 64 / max 128

v4:
 - Adjust pipe configured flag to be reset on resume rather than suspend
 - rename dl_child, dl_next
---
 drivers/media/platform/vsp1/vsp1_pipe.c  |  7 +++-
 drivers/media/platform/vsp1/vsp1_pipe.h  |  4 +-
 drivers/media/platform/vsp1/vsp1_video.c | 67 -
 drivers/media/platform/vsp1/vsp1_video.h |  2 +-
 4 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c 
b/drivers/media/platform/vsp1/vsp1_pipe.c
index 5012643583b6..fa445b1a2e38 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -249,6 +249,7 @@ void vsp1_pipeline_run(struct vsp1_pipeline *pipe)
vsp1_write(vsp1, VI6_CMD(pipe->output->entity.index),
   VI6_CMD_STRCMD);
pipe->state = VSP1_PIPELINE_RUNNING;
+   pipe->configured = true;
}
 
pipe->buffers_ready = 0;
@@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1)
continue;
 
spin_lock_irqsave(>irqlock, flags);
+   /*
+* The hardware may have been reset during a suspend and will
+* need a full reconfiguration
+*/
+   pipe->configured = false;
+
if (vsp1_pipeline_ready(pipe))
vsp1_pipeline_run(pipe);
spin_unlock_irqrestore(>irqlock, flags);
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h 
b/drivers/media/platform/vsp1/vsp1_pipe.h
index 90d29492b9b9..e7ad6211b4d0 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -90,6 +90,7 @@ struct vsp1_partition {
  * @irqlock: protects the pipeline state
  * @state: current state
  * @wq: wait queue to wait for state change completion
+ * @configured: flag determining if the hardware has run since reset
  * @frame_end: frame end interrupt handler
  * @lock: protects the pipeline use count and stream count
  * @kref: pipeline reference count
@@ -117,6 +118,7 @@ struct vsp1_pipeline {
spinlock_t irqlock;
enum vsp1_pipeline_state state;
wait_queue_head_t wq;
+   bool configured;
 
void (*frame_end)(struct vsp1_pipeline *pipe, bool completed);
 
@@ -143,8 +145,6 @@ struct vsp1_pipeline {
 */
struct list_head entities;
 
-   struct vsp1_dl_list *dl;
-
unsigned int partitions;
struct 

[PATCH v5 9/9] v4l: vsp1: Reduce display list body size

[Kieran Bingham]

The display list originally allocated a body of 256 entries to store all
of the register lists required for each frame.

This has now been separated into fragments for constant stream setup, and
runtime updates.

Empirical testing shows that the body0 now uses a maximum of 41
registers for each frame, for both DRM and Video API pipelines thus a
rounded 64 entries provides a suitable allocation.

Signed-off-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_dl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index 1fc52496fc13..ce315821b60c 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -21,7 +21,7 @@
 #include "vsp1.h"
 #include "vsp1_dl.h"
 
-#define VSP1_DL_NUM_ENTRIES256
+#define VSP1_DL_NUM_ENTRIES64
 
 #define VSP1_DLH_INT_ENABLE(1 << 1)
 #define VSP1_DLH_AUTO_START(1 << 0)
-- 
git-series 0.9.1

[PATCH v5 7/9] v4l: vsp1: Adapt entities to configure into a body

[Kieran Bingham]

Currently the entities store their configurations into a display list.
Adapt this such that the code can be configured into a body directly,
allowing greater flexibility and control of the content.

All users of vsp1_dl_list_write() are removed in this process, thus it
too is removed.

A helper, vsp1_dl_list_get_body0() is provided to access the internal body0
from the display list.

Signed-off-by: Kieran Bingham 

---

v4:
 - Rename vsp1_dl_list_get_body() to vsp1_dl_list_get_body0()
   The similarities between vsp1_dl_list_get_body and
   vsp1_dl_list_body_get() were too close

 - body0 could be removed later when the default body is no longer
   needed.

v5:
 - Support DRM/UIF changes
---
 drivers/media/platform/vsp1/vsp1_bru.c| 22 +--
 drivers/media/platform/vsp1/vsp1_clu.c| 22 +--
 drivers/media/platform/vsp1/vsp1_dl.c | 12 ++
 drivers/media/platform/vsp1/vsp1_dl.h |  2 +-
 drivers/media/platform/vsp1/vsp1_drm.c| 24 +++-
 drivers/media/platform/vsp1/vsp1_entity.c | 16 
 drivers/media/platform/vsp1/vsp1_entity.h | 12 +++---
 drivers/media/platform/vsp1/vsp1_hgo.c| 16 
 drivers/media/platform/vsp1/vsp1_hgt.c| 18 -
 drivers/media/platform/vsp1/vsp1_hsit.c   | 10 ++---
 drivers/media/platform/vsp1/vsp1_lif.c| 13 +++
 drivers/media/platform/vsp1/vsp1_lut.c| 21 +--
 drivers/media/platform/vsp1/vsp1_pipe.c   |  4 +-
 drivers/media/platform/vsp1/vsp1_pipe.h   |  3 +-
 drivers/media/platform/vsp1/vsp1_rpf.c| 47 +++-
 drivers/media/platform/vsp1/vsp1_sru.c| 14 +++
 drivers/media/platform/vsp1/vsp1_uds.c| 24 ++--
 drivers/media/platform/vsp1/vsp1_uds.h|  2 +-
 drivers/media/platform/vsp1/vsp1_uif.c| 18 -
 drivers/media/platform/vsp1/vsp1_video.c  | 11 --
 drivers/media/platform/vsp1/vsp1_wpf.c| 42 +++--
 21 files changed, 185 insertions(+), 168 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_bru.c 
b/drivers/media/platform/vsp1/vsp1_bru.c
index b9ff96f76b3e..60d449d7b135 100644
--- a/drivers/media/platform/vsp1/vsp1_bru.c
+++ b/drivers/media/platform/vsp1/vsp1_bru.c
@@ -30,10 +30,10 @@
  * Device Access
  */
 
-static inline void vsp1_bru_write(struct vsp1_bru *bru, struct vsp1_dl_list 
*dl,
- u32 reg, u32 data)
+static inline void vsp1_bru_write(struct vsp1_bru *bru,
+ struct vsp1_dl_body *dlb, u32 reg, u32 data)
 {
-   vsp1_dl_list_write(dl, bru->base + reg, data);
+   vsp1_dl_body_write(dlb, bru->base + reg, data);
 }
 
 /* 
-
@@ -287,7 +287,7 @@ static const struct v4l2_subdev_ops bru_ops = {
 
 static void bru_prepare(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
-   struct vsp1_dl_list *dl)
+   struct vsp1_dl_body *dlb)
 {
struct vsp1_bru *bru = to_bru(>subdev);
struct v4l2_mbus_framefmt *format;
@@ -309,7 +309,7 @@ static void bru_prepare(struct vsp1_entity *entity,
 * format at the pipeline output is premultiplied.
 */
flags = pipe->output ? pipe->output->format.flags : 0;
-   vsp1_bru_write(bru, dl, VI6_BRU_INCTRL,
+   vsp1_bru_write(bru, dlb, VI6_BRU_INCTRL,
   flags & V4L2_PIX_FMT_FLAG_PREMUL_ALPHA ?
   0 : VI6_BRU_INCTRL_NRM);
 
@@ -317,12 +317,12 @@ static void bru_prepare(struct vsp1_entity *entity,
 * Set the background position to cover the whole output image and
 * configure its color.
 */
-   vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_SIZE,
+   vsp1_bru_write(bru, dlb, VI6_BRU_VIRRPF_SIZE,
   (format->width << VI6_BRU_VIRRPF_SIZE_HSIZE_SHIFT) |
   (format->height << VI6_BRU_VIRRPF_SIZE_VSIZE_SHIFT));
-   vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_LOC, 0);
+   vsp1_bru_write(bru, dlb, VI6_BRU_VIRRPF_LOC, 0);
 
-   vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_COL, bru->bgcolor |
+   vsp1_bru_write(bru, dlb, VI6_BRU_VIRRPF_COL, bru->bgcolor |
   (0xff << VI6_BRU_VIRRPF_COL_A_SHIFT));
 
/*
@@ -332,7 +332,7 @@ static void bru_prepare(struct vsp1_entity *entity,
 * unit.
 */
if (entity->type == VSP1_ENTITY_BRU)
-   vsp1_bru_write(bru, dl, VI6_BRU_ROP,
+   vsp1_bru_write(bru, dlb, VI6_BRU_ROP,
   VI6_BRU_ROP_DSTSEL_BRUIN(1) |
   VI6_BRU_ROP_CROP(VI6_ROP_NOP) |
   VI6_BRU_ROP_AROP(VI6_ROP_NOP));
@@ -374,7 +374,7 @@ static void bru_prepare(struct vsp1_entity *entity,
if (!(entity->type == VSP1_ENTITY_BRU && i == 1))
ctrl |= VI6_BRU_CTRL_SRCSEL_BRUIN(i);
 
-   vsp1_bru_write(bru, dl, 

[PATCH v5 4/9] v4l: vsp1: Convert display lists to use new body pool

[Kieran Bingham]

Adapt the dl->body0 object to use an object from the body pool. This
greatly reduces the pressure on the TLB for IPMMU use cases, as all of
the lists use a single allocation for the main body.

The CLU and LUT objects pre-allocate a pool containing three bodies,
allowing a userspace update before the hardware has committed a previous
set of tables.

Bodies are no longer 'freed' in interrupt context, but instead released
back to their respective pools. This allows us to remove the garbage
collector in the DLM.

Signed-off-by: Kieran Bingham 

---
v3:
 - 's/fragment/body', 's/fragments/bodies/'
 - CLU/LUT now allocate 3 bodies
 - vsp1_dl_list_fragments_free -> vsp1_dl_list_bodies_put

v2:
 - Use dl->body0->max_entries to determine header offset, instead of the
   global constant VSP1_DL_NUM_ENTRIES which is incorrect.
 - squash updates for LUT, CLU, and fragment cleanup into single patch.
   (Not fully bisectable when separated)
---
 drivers/media/platform/vsp1/vsp1_clu.c |  27 ++-
 drivers/media/platform/vsp1/vsp1_clu.h |   1 +-
 drivers/media/platform/vsp1/vsp1_dl.c  | 223 ++
 drivers/media/platform/vsp1/vsp1_dl.h  |   3 +-
 drivers/media/platform/vsp1/vsp1_lut.c |  27 ++-
 drivers/media/platform/vsp1/vsp1_lut.h |   1 +-
 6 files changed, 101 insertions(+), 181 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_clu.c 
b/drivers/media/platform/vsp1/vsp1_clu.c
index 246dd595c978..a765d56c4118 100644
--- a/drivers/media/platform/vsp1/vsp1_clu.c
+++ b/drivers/media/platform/vsp1/vsp1_clu.c
@@ -23,6 +23,8 @@
 #define CLU_MIN_SIZE   4U
 #define CLU_MAX_SIZE   8190U
 
+#define CLU_SIZE   (17 * 17 * 17)
+
 /* 
-
  * Device Access
  */
@@ -47,19 +49,19 @@ static int clu_set_table(struct vsp1_clu *clu, struct 
v4l2_ctrl *ctrl)
struct vsp1_dl_body *dlb;
unsigned int i;
 
-   dlb = vsp1_dl_body_alloc(clu->entity.vsp1, 1 + 17 * 17 * 17);
+   dlb = vsp1_dl_body_get(clu->pool);
if (!dlb)
return -ENOMEM;
 
vsp1_dl_body_write(dlb, VI6_CLU_ADDR, 0);
-   for (i = 0; i < 17 * 17 * 17; ++i)
+   for (i = 0; i < CLU_SIZE; ++i)
vsp1_dl_body_write(dlb, VI6_CLU_DATA, ctrl->p_new.p_u32[i]);
 
spin_lock_irq(>lock);
swap(clu->clu, dlb);
spin_unlock_irq(>lock);
 
-   vsp1_dl_body_free(dlb);
+   vsp1_dl_body_put(dlb);
return 0;
 }
 
@@ -220,8 +222,16 @@ static void clu_configure(struct vsp1_entity *entity,
}
 }
 
+static void clu_destroy(struct vsp1_entity *entity)
+{
+   struct vsp1_clu *clu = to_clu(>subdev);
+
+   vsp1_dl_body_pool_destroy(clu->pool);
+}
+
 static const struct vsp1_entity_operations clu_entity_ops = {
.configure = clu_configure,
+   .destroy = clu_destroy,
 };
 
 /* 
-
@@ -247,6 +257,17 @@ struct vsp1_clu *vsp1_clu_create(struct vsp1_device *vsp1)
if (ret < 0)
return ERR_PTR(ret);
 
+   /*
+* Pre-allocate a body pool, with 3 bodies allowing a userspace update
+* before the hardware has committed a previous set of tables, handling
+* both the queued and pending dl entries. One extra entry is added to
+* the CLU_SIZE to allow for the VI6_CLU_ADDR header.
+*/
+   clu->pool = vsp1_dl_body_pool_create(clu->entity.vsp1, 3, CLU_SIZE + 1,
+0);
+   if (!clu->pool)
+   return ERR_PTR(-ENOMEM);
+
/* Initialize the control handler. */
v4l2_ctrl_handler_init(>ctrls, 2);
v4l2_ctrl_new_custom(>ctrls, _table_control, NULL);
diff --git a/drivers/media/platform/vsp1/vsp1_clu.h 
b/drivers/media/platform/vsp1/vsp1_clu.h
index 036e0a2f1a42..fa3fe856725b 100644
--- a/drivers/media/platform/vsp1/vsp1_clu.h
+++ b/drivers/media/platform/vsp1/vsp1_clu.h
@@ -36,6 +36,7 @@ struct vsp1_clu {
spinlock_t lock;
unsigned int mode;
struct vsp1_dl_body *clu;
+   struct vsp1_dl_body_pool *pool;
 };
 
 static inline struct vsp1_clu *to_clu(struct v4l2_subdev *subdev)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index 4e71792d183c..961c3fd2ff1c 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -111,7 +111,7 @@ struct vsp1_dl_list {
struct vsp1_dl_header *header;
dma_addr_t dma;
 
-   struct vsp1_dl_body body0;
+   struct vsp1_dl_body *body0;
struct list_head bodies;
 
bool has_chain;
@@ -135,8 +135,6 @@ enum vsp1_dl_mode {
  * @queued: list queued to the hardware (written to the DL registers)
  * @pending: list waiting to be queued to the hardware
  * @pool: body pool for the display list bodies
- * 

[PATCH v5 1/9] v4l: vsp1: Reword uses of 'fragment' as 'body'

[Kieran Bingham]

Throughout the codebase, the term 'fragment' is used to represent a
display list body. This term duplicates the 'body' which is already in
use.

The datasheet references these objects as a body, therefore replace all
mentions of a fragment with a body, along with the corresponding
pluralised terms.

Signed-off-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_clu.c |  10 +-
 drivers/media/platform/vsp1/vsp1_dl.c  | 107 --
 drivers/media/platform/vsp1/vsp1_dl.h  |  14 +--
 drivers/media/platform/vsp1/vsp1_lut.c |   8 +-
 4 files changed, 69 insertions(+), 70 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_clu.c 
b/drivers/media/platform/vsp1/vsp1_clu.c
index bc931a3ab498..246dd595c978 100644
--- a/drivers/media/platform/vsp1/vsp1_clu.c
+++ b/drivers/media/platform/vsp1/vsp1_clu.c
@@ -47,19 +47,19 @@ static int clu_set_table(struct vsp1_clu *clu, struct 
v4l2_ctrl *ctrl)
struct vsp1_dl_body *dlb;
unsigned int i;
 
-   dlb = vsp1_dl_fragment_alloc(clu->entity.vsp1, 1 + 17 * 17 * 17);
+   dlb = vsp1_dl_body_alloc(clu->entity.vsp1, 1 + 17 * 17 * 17);
if (!dlb)
return -ENOMEM;
 
-   vsp1_dl_fragment_write(dlb, VI6_CLU_ADDR, 0);
+   vsp1_dl_body_write(dlb, VI6_CLU_ADDR, 0);
for (i = 0; i < 17 * 17 * 17; ++i)
-   vsp1_dl_fragment_write(dlb, VI6_CLU_DATA, ctrl->p_new.p_u32[i]);
+   vsp1_dl_body_write(dlb, VI6_CLU_DATA, ctrl->p_new.p_u32[i]);
 
spin_lock_irq(>lock);
swap(clu->clu, dlb);
spin_unlock_irq(>lock);
 
-   vsp1_dl_fragment_free(dlb);
+   vsp1_dl_body_free(dlb);
return 0;
 }
 
@@ -215,7 +215,7 @@ static void clu_configure(struct vsp1_entity *entity,
spin_unlock_irqrestore(>lock, flags);
 
if (dlb)
-   vsp1_dl_list_add_fragment(dl, dlb);
+   vsp1_dl_list_add_body(dl, dlb);
break;
}
 }
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index 4257451f1bd8..90e972a75c62 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -69,7 +69,7 @@ struct vsp1_dl_body {
  * @header: display list header, NULL for headerless lists
  * @dma: DMA address for the header
  * @body0: first display list body
- * @fragments: list of extra display list bodies
+ * @bodies: list of extra display list bodies
  * @has_chain: if true, indicates that there's a partition chain
  * @chain: entry in the display list partition chain
  */
@@ -81,7 +81,7 @@ struct vsp1_dl_list {
dma_addr_t dma;
 
struct vsp1_dl_body body0;
-   struct list_head fragments;
+   struct list_head bodies;
 
bool has_chain;
struct list_head chain;
@@ -98,13 +98,13 @@ enum vsp1_dl_mode {
  * @mode: display list operation mode (header or headerless)
  * @singleshot: execute the display list in single-shot mode
  * @vsp1: the VSP1 device
- * @lock: protects the free, active, queued, pending and gc_fragments lists
+ * @lock: protects the free, active, queued, pending and gc_bodies lists
  * @free: array of all free display lists
  * @active: list currently being processed (loaded) by hardware
  * @queued: list queued to the hardware (written to the DL registers)
  * @pending: list waiting to be queued to the hardware
- * @gc_work: fragments garbage collector work struct
- * @gc_fragments: array of display list fragments waiting to be freed
+ * @gc_work: bodies garbage collector work struct
+ * @gc_bodies: array of display list bodies waiting to be freed
  */
 struct vsp1_dl_manager {
unsigned int index;
@@ -119,7 +119,7 @@ struct vsp1_dl_manager {
struct vsp1_dl_list *pending;
 
struct work_struct gc_work;
-   struct list_head gc_fragments;
+   struct list_head gc_bodies;
 };
 
 /* 
-
@@ -157,17 +157,16 @@ static void vsp1_dl_body_cleanup(struct vsp1_dl_body *dlb)
 }
 
 /**
- * vsp1_dl_fragment_alloc - Allocate a display list fragment
+ * vsp1_dl_body_alloc - Allocate a display list body
  * @vsp1: The VSP1 device
- * @num_entries: The maximum number of entries that the fragment can contain
+ * @num_entries: The maximum number of entries that the body can contain
  *
- * Allocate a display list fragment with enough memory to contain the requested
+ * Allocate a display list body with enough memory to contain the requested
  * number of entries.
  *
- * Return a pointer to a fragment on success or NULL if memory can't be
- * allocated.
+ * Return a pointer to a body on success or NULL if memory can't be allocated.
  */
-struct vsp1_dl_body *vsp1_dl_fragment_alloc(struct vsp1_device *vsp1,
+struct vsp1_dl_body *vsp1_dl_body_alloc(struct vsp1_device *vsp1,
unsigned int num_entries)
 {

[PATCH v5 2/9] v4l: vsp1: Protect bodies against overflow

[Kieran Bingham]

The body write function relies on the code never asking it to write more
than the entries available in the list.

Currently with each list body containing 256 entries, this is fine, but
we can reduce this number greatly saving memory. In preparation of this
add a level of protection to catch any buffer overflows.

Signed-off-by: Kieran Bingham 
Reviewed-by: Laurent Pinchart 

---

v3:
 - adapt for new 'body' terminology
 - simplify WARN_ON macro usage
---
 drivers/media/platform/vsp1/vsp1_dl.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index 90e972a75c62..ecc3659a7884 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -50,6 +50,7 @@ struct vsp1_dl_entry {
  * @dma: DMA address of the entries
  * @size: size of the DMA memory in bytes
  * @num_entries: number of stored entries
+ * @max_entries: number of entries available
  */
 struct vsp1_dl_body {
struct list_head list;
@@ -60,6 +61,7 @@ struct vsp1_dl_body {
size_t size;
 
unsigned int num_entries;
+   unsigned int max_entries;
 };
 
 /**
@@ -139,6 +141,7 @@ static int vsp1_dl_body_init(struct vsp1_device *vsp1,
 
dlb->vsp1 = vsp1;
dlb->size = size;
+   dlb->max_entries = num_entries;
 
dlb->entries = dma_alloc_wc(vsp1->bus_master, dlb->size, >dma,
GFP_KERNEL);
@@ -220,6 +223,10 @@ void vsp1_dl_body_free(struct vsp1_dl_body *dlb)
  */
 void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 reg, u32 data)
 {
+   if (WARN_ONCE(dlb->num_entries >= dlb->max_entries,
+ "DLB size exceeded (max %u)", dlb->max_entries))
+   return;
+
dlb->entries[dlb->num_entries].addr = reg;
dlb->entries[dlb->num_entries].data = data;
dlb->num_entries++;
-- 
git-series 0.9.1

[PATCH v5 0/9] vsp1: TLB optimisation and DL caching

[Kieran Bingham]

Each display list currently allocates an area of DMA memory to store register
settings for the VSP1 to process. Each of these allocations adds pressure to
the IPMMU TLB entries.

We can reduce the pressure by pre-allocating larger areas and dividing the area
across multiple bodies represented as a pool.

With this reconfiguration of bodies, we can adapt the configuration code to
separate out constant hardware configuration and cache it for re-use.

--

The patches provided in this series can be found at:
  git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git  
tags/vsp1/tlb-optimise/v5

This series is temporarily based on the renesas-drivers-2018-01-09-v4.15-rc7 
release tag,
until changes for the DRM and UIF make it upstream.

Changelog:
--

v5:
 - Rebased on to renesas-drivers-2018-01-09-v4.15-rc7 to fix conflicts
   with DRM and UIF updates on VSP1 driver

v4:
 - Rebased to v4.14
 * v4l: vsp1: Use reference counting for bodies
   - Fix up reference handling comments

 * v4l: vsp1: Provide a body pool
   - Provide comment explaining extra allocation on body pool
 highlighting area for optimisation later.

 * v4l: vsp1: Refactor display list configure operations
   - Fix up comment to describe yuv_mode caching rather than format

 * vsp1: Adapt entities to configure into a body
   - Rename vsp1_dl_list_get_body() to vsp1_dl_list_get_body0()

 * v4l: vsp1: Move video configuration to a cached dlb
   - Adjust pipe configured flag to be reset on resume rather than suspend
   - rename dl_child, dl_next

Testing:

The VSP unit tests have been run on this patch set with the following results:

- vsp-unit-test-.sh
Test Conditions:
  Platform  Renesas Salvator-X 2nd version board based on r8a7795 ES2.0+
  Kernel release4.15.0-rc7-arm64-renesas-9-gfa58cc7e4788
  yavta /usr/bin/yavta
  convert   /usr/bin/convert
  compare   /usr/bin/compare
  killall   /usr/bin/killall
  raw2rgbpnm/usr/bin/raw2rgbpnm
- vsp-unit-test-0001.sh
Testing WPF packing in RGB332: pass
Testing WPF packing in ARGB555: pass
Testing WPF packing in XRGB555: pass
Testing WPF packing in RGB565: pass
Testing WPF packing in BGR24: pass
Testing WPF packing in RGB24: pass
Testing WPF packing in ABGR32: pass
Testing WPF packing in ARGB32: pass
Testing WPF packing in XBGR32: pass
Testing WPF packing in XRGB32: pass
- vsp-unit-test-0002.sh
Testing WPF packing in NV12M: pass
Testing WPF packing in NV16M: pass
Testing WPF packing in NV21M: pass
Testing WPF packing in NV61M: pass
Testing WPF packing in UYVY: pass
Testing WPF packing in VYUY: skip
Testing WPF packing in YUV420M: pass
Testing WPF packing in YUV422M: pass
Testing WPF packing in YUV444M: pass
Testing WPF packing in YVU420M: pass
Testing WPF packing in YVU422M: pass
Testing WPF packing in YVU444M: pass
Testing WPF packing in YUYV: pass
Testing WPF packing in YVYU: pass
- vsp-unit-test-0003.sh
Testing scaling from 640x640 to 640x480 in RGB24: pass
Testing scaling from 1024x768 to 640x480 in RGB24: pass
Testing scaling from 640x480 to 1024x768 in RGB24: pass
Testing scaling from 640x640 to 640x480 in YUV444M: pass
Testing scaling from 1024x768 to 640x480 in YUV444M: pass
Testing scaling from 640x480 to 1024x768 in YUV444M: pass
- vsp-unit-test-0004.sh
Testing histogram in RGB24: pass
Testing histogram in YUV444M: pass
- vsp-unit-test-0005.sh
Testing RPF.0: pass
Testing RPF.1: pass
Testing RPF.2: pass
Testing RPF.3: pass
Testing RPF.4: pass
- vsp-unit-test-0006.sh
Testing invalid pipeline with no RPF: pass
Testing invalid pipeline with no WPF: pass
- vsp-unit-test-0007.sh
Testing BRU in RGB24 with 1 inputs: pass
Testing BRU in RGB24 with 2 inputs: pass
Testing BRU in RGB24 with 3 inputs: pass
Testing BRU in RGB24 with 4 inputs: pass
Testing BRU in RGB24 with 5 inputs: pass
Testing BRU in YUV444M with 1 inputs: pass
Testing BRU in YUV444M with 2 inputs: pass
Testing BRU in YUV444M with 3 inputs: pass
Testing BRU in YUV444M with 4 inputs: pass
Testing BRU in YUV444M with 5 inputs: pass
- vsp-unit-test-0008.sh
Test requires unavailable feature set `bru rpf.0 uds wpf.0': skipped
- vsp-unit-test-0009.sh
Test requires unavailable feature set `rpf.0 wpf.0 wpf.1': skipped
- vsp-unit-test-0010.sh
Testing CLU in RGB24 with zero configuration: pass
Testing CLU in RGB24 with identity configuration: pass
Testing CLU in RGB24 with wave configuration: pass
Testing CLU in YUV444M with zero configuration: pass
Testing CLU in YUV444M with identity configuration: pass
Testing CLU in YUV444M with wave configuration: pass
Testing LUT in RGB24 with zero configuration: pass
Testing LUT in RGB24 with identity configuration: pass
Testing LUT in RGB24 with gamma configuration: pass
Testing LUT in YUV444M with zero configuration: pass
Testing LUT in YUV444M with identity configuration: pass
Testing LUT in YUV444M with gamma configuration: pass
- vsp-unit-test-0011.sh
Testing  hflip=0 vflip=0 rotate=0: pass
Testing  hflip=1 vflip=0 

[PATCH v5 3/9] v4l: vsp1: Provide a body pool

[Kieran Bingham]

Each display list allocates a body to store register values in a dma
accessible buffer from a dma_alloc_wc() allocation. Each of these
results in an entry in the TLB, and a large number of display list
allocations adds pressure to this resource.

Reduce TLB pressure on the IPMMUs by allocating multiple display list
bodies in a single allocation, and providing these to the display list
through a 'body pool'. A pool can be allocated by the display list
manager or entities which require their own body allocations.

Signed-off-by: Kieran Bingham 

---
v4:
 - Provide comment explaining extra allocation on body pool
   highlighting area for optimisation later.

v3:
 - s/fragment/body/, s/fragments/bodies/
 - qty -> num_bodies
 - indentation fix
 - s/vsp1_dl_body_pool_{alloc,free}/vsp1_dl_body_pool_{create,destroy}/'
 - Add kerneldoc to non-static functions

v2:
 - assign dlb->dma correctly
---
 drivers/media/platform/vsp1/vsp1_dl.c | 163 +++-
 drivers/media/platform/vsp1/vsp1_dl.h |   8 +-
 2 files changed, 171 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index ecc3659a7884..4e71792d183c 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -45,6 +45,8 @@ struct vsp1_dl_entry {
 /**
  * struct vsp1_dl_body - Display list body
  * @list: entry in the display list list of bodies
+ * @free: entry in the pool free body list
+ * @pool: pool to which this body belongs
  * @vsp1: the VSP1 device
  * @entries: array of entries
  * @dma: DMA address of the entries
@@ -54,6 +56,9 @@ struct vsp1_dl_entry {
  */
 struct vsp1_dl_body {
struct list_head list;
+   struct list_head free;
+
+   struct vsp1_dl_body_pool *pool;
struct vsp1_device *vsp1;
 
struct vsp1_dl_entry *entries;
@@ -65,6 +70,30 @@ struct vsp1_dl_body {
 };
 
 /**
+ * struct vsp1_dl_body_pool - display list body pool
+ * @dma: DMA address of the entries
+ * @size: size of the full DMA memory pool in bytes
+ * @mem: CPU memory pointer for the pool
+ * @bodies: Array of DLB structures for the pool
+ * @free: List of free DLB entries
+ * @lock: Protects the pool and free list
+ * @vsp1: the VSP1 device
+ */
+struct vsp1_dl_body_pool {
+   /* DMA allocation */
+   dma_addr_t dma;
+   size_t size;
+   void *mem;
+
+   /* Body management */
+   struct vsp1_dl_body *bodies;
+   struct list_head free;
+   spinlock_t lock;
+
+   struct vsp1_device *vsp1;
+};
+
+/**
  * struct vsp1_dl_list - Display list
  * @list: entry in the display list manager lists
  * @dlm: the display list manager
@@ -105,6 +134,7 @@ enum vsp1_dl_mode {
  * @active: list currently being processed (loaded) by hardware
  * @queued: list queued to the hardware (written to the DL registers)
  * @pending: list waiting to be queued to the hardware
+ * @pool: body pool for the display list bodies
  * @gc_work: bodies garbage collector work struct
  * @gc_bodies: array of display list bodies waiting to be freed
  */
@@ -120,6 +150,8 @@ struct vsp1_dl_manager {
struct vsp1_dl_list *queued;
struct vsp1_dl_list *pending;
 
+   struct vsp1_dl_body_pool *pool;
+
struct work_struct gc_work;
struct list_head gc_bodies;
 };
@@ -128,6 +160,137 @@ struct vsp1_dl_manager {
  * Display List Body Management
  */
 
+/**
+ * vsp1_dl_body_pool_create - Create a pool of bodies from a single allocation
+ * @vsp1: The VSP1 device
+ * @num_bodies: The quantity of bodies to allocate
+ * @num_entries: The maximum number of entries that the body can contain
+ * @extra_size: Extra allocation provided for the bodies
+ *
+ * Allocate a pool of display list bodies each with enough memory to contain 
the
+ * requested number of entries.
+ *
+ * Return a pointer to a pool on success or NULL if memory can't be allocated.
+ */
+struct vsp1_dl_body_pool *
+vsp1_dl_body_pool_create(struct vsp1_device *vsp1, unsigned int num_bodies,
+unsigned int num_entries, size_t extra_size)
+{
+   struct vsp1_dl_body_pool *pool;
+   size_t dlb_size;
+   unsigned int i;
+
+   pool = kzalloc(sizeof(*pool), GFP_KERNEL);
+   if (!pool)
+   return NULL;
+
+   pool->vsp1 = vsp1;
+
+   /*
+* Todo: 'extra_size' is only used by vsp1_dlm_create(), to allocate
+* extra memory for the display list header. We need only one header per
+* display list, not per display list body, thus this allocation is
+* extraneous and should be reworked in the future.
+*/
+   dlb_size = num_entries * sizeof(struct vsp1_dl_entry) + extra_size;
+   pool->size = dlb_size * num_bodies;
+
+   pool->bodies = kcalloc(num_bodies, sizeof(*pool->bodies), GFP_KERNEL);
+   if (!pool->bodies) {
+   kfree(pool);
+   return NULL;
+   }
+
+   pool->mem = 

Re: [PATCH] media: staging/imx: Checking the right variable in vdic_get_ipu_resources()

[Steve Longerbeam]

Acked-by: Steve Longerbeam 


On 01/15/2018 12:11 AM, Dan Carpenter wrote:

We recently changed this error handling around but missed this error
pointer check.  We're testing "priv->vdi_in_ch_n" instead of "ch" so the
error handling can't be triggered.

Fixes: 0b2e9e7947e7 ("media: staging/imx: remove confusing IS_ERR_OR_NULL 
usage")
Signed-off-by: Dan Carpenter 

diff --git a/drivers/staging/media/imx/imx-media-vdic.c 
b/drivers/staging/media/imx/imx-media-vdic.c
index 433474d58e3e..ed356844cdf6 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -177,7 +177,7 @@ static int vdic_get_ipu_resources(struct vdic_priv *priv)
priv->vdi_in_ch = ch;
  
  		ch = ipu_idmac_get(priv->ipu, IPUV3_CHANNEL_MEM_VDI_NEXT);

-   if (IS_ERR(priv->vdi_in_ch_n)) {
+   if (IS_ERR(ch)) {
err_chan = IPUV3_CHANNEL_MEM_VDI_NEXT;
ret = PTR_ERR(ch);
goto out_err_chan;

Re: [PATCH v2] vsp1: fix video output on R8A77970

[Sergei Shtylyov]

Hello!

On 01/15/2018 03:51 PM, Laurent Pinchart wrote:


On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote:

Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but


I'm not sure there's a need to state my name in the commit message.


   You were the author of the patch this one has in the Fixes: tag, so I 
thought that was appropriate. I can remove that if you don't want you name 
mentioned...



the video  output that VSP2-D sends to DU has a greenish garbage-like line


Why does the text in your patches (commit message, comments, ...) sometime
have double spaces between words ?


   The text looks more pleasant (at least to me). Can remove them if you want...


repeated every 8 or so screen rows.


Is it every "8 or so" rows, or exactly every 8 rows ?


   You really want me to count the pixels?


It turns out that V3M has a teeny LIF register (at least it's documented!)
that you need to set to some kind of a  magic value for the LIF to work
correctly...

Based on the original (and large) patch by Daisuke Matsushita
.


What else is in the big patch ? Is it available somewhere ?


   Assorted changes gathered together only because they all bring the support 
for R8A77970. If you're really curious, here you are:


https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0030-arm64-renesas-r8a7797-Add-Renesas-R8A7797-SoC-suppor.patch


Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and
VSP2-D instances")
Signed-off-by: Sergei Shtylyov 

---
This patch is against the 'media_tree.git' repo's 'master' branch.

Changes in version 2:
- added a  comment before the V3M SoC check;
- fixed indetation in that check;
- reformatted  the patch description.

  drivers/media/platform/vsp1/vsp1_lif.c  |   12 
  drivers/media/platform/vsp1/vsp1_regs.h |5 +
  2 files changed, 17 insertions(+)

Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c
===
--- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c
+++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c
@@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en
(obth << VI6_LIF_CTRL_OBTH_SHIFT) |
(format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) |
VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
+
+   /*
+* R-Car V3M has the buffer attribute register you absolutely need
+* to write kinda magic value to  for the LIF to work correctly...
+*/


I'm not sure about the "kinda" magic value. 1536 is very likely a buffer size.


   Well, that's only guessing. The manual doesn't say anything about what the 
number is.



How about the following text ?

/*
 * On V3M the LBA register has to be set to a non-default value to


   I'd then spell its full name, LIF0 Buffer Attribute register.


 * guarantee proper operation (otherwise artifacts may appear on the
 * output). The value required by the datasheet is not documented but
 * is likely a buffer size or threshold.
 */

>

   OK,  can change that (modulo the name).


The commit message should also be updated to feel a bit less magic.


   I'll see about it.


+   if ((entity->vsp1->version &
+(VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) ==
+   (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) {
+   vsp1_lif_write(lif, dl, VI6_LIF_LBA,
+  VI6_LIF_LBA_LBA0 |
+  (1536 << VI6_LIF_LBA_LBA1_SHIFT));
+   }
  }


The datasheet documents the register as being present on both V3M and M3-W
(and the test I've just run on H3 shows that the register is present there as
well). Should we program it on M3-W or leave it to the default value that
should be what is recommended by the datasheet for that SoC ?


   If the default value matches what's recommended by the manual, then I'd 
leave the register alone. But my task was R8A77970 support only anyway...



[..]

MBR, Sergei

Re: [PATCH 3/4] tsi108_eth: use dma API properly

[Christoph Hellwig]

On Wed, Jan 10, 2018 at 10:09:20PM +0200, Andy Shevchenko wrote:
> > +   struct platform_device *pdev;
> 
> Do you really need platform_defice reference?
> 
> Perhaps
> 
> struct device *hdev; // hardware device
> 
> 
> data->hdev = >dev;
> 
> Another idea
> 
> dev->dev.parent = >dev;
> 
> No new member needed.

Maybe.  But what I've done is the simplest change in a long obsolete
driver that I don't understand at all.  I'd rather keep it simple.

Re: MT9M131 on I.MX6DL CSI color issue

[Florian Boor]

Hi Philipp,

On 15.01.2018 13:49, Philipp Zabel wrote:
> media-ctl propagates video formats downstream, can you try reversing the
> order?

I did but it does not make a difference.

> Also, while the external format is UYVY2X8, internally the IPU only
> supports AYUV32, so the last call should be 
> 
> media-ctl -d /dev/media0 -v -V "'ipu1_csi0':2 [fmt:AYUV32/${GEOM} field:none]"
> not that it should make a visible difference.
> And setting a format on 'ipu1_csi0 capture' is not necessary.

Changed this as well. What I do now is the following:

SF="UYVY2X8"
IF="AYUV32"
GEOM="1280x1024"

media-ctl -r
media-ctl -l "'mt9m111 2-0048':0 -> 'ipu1_csi0_mux':4[1]"
media-ctl -l "'ipu1_csi0_mux':5 -> 'ipu1_csi0':0[1]"
media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]"

media-ctl -d /dev/media0 -v -V "'mt9m111 2-0048':0 [fmt:${SF}/${GEOM} field: 
none]"
media-ctl -d /dev/media0 -v -V "'ipu1_csi0_mux':4 [fmt:${SF}/${GEOM} field: 
none]"
media-ctl -d /dev/media0 -v -V "'ipu1_csi0_mux':5 [fmt:${SF}/${GEOM} field: 
none]"
media-ctl -d /dev/media0 -v -V "'ipu1_csi0':2 [fmt:${IF}/${GEOM} field:none]"


> The new picture looks a little like there is 10-bit sensor data and only
> the lower 8-bit arrive in memory, given the number of wraparounds.

I will take a look at the sensor configuration. Maybe there is some issue or a
difference among all th MT9M1x1 semsors the driver does not support.

> Can you show the output of "media-ctl -p" (or "media-ctl --get-v4l2" for
> each pad in the pipeline)?

> media-ctl --get-v4l2 "'mt9m111 2-0048':0"
[fmt:UYVY2X8/1280x1024 field:none]
 crop.bounds:(26,8)/1280x1024
 crop:(26,8)/1280x1024]
> media-ctl --get-v4l2 "'ipu1_csi0_mux':4"
[fmt:UYVY2X8/1280x1024 field:none]
> media-ctl --get-v4l2 "'ipu1_csi0_mux':5"
[fmt:UYVY2X8/1280x1024 field:none]
> media-ctl --get-v4l2 "'ipu1_csi0':0"
[fmt:UYVY2X8/1280x1024 field:none
 crop.bounds:(0,0)/1280x1024
 crop:(0,0)/1280x1024
 compose.bounds:(0,0)/1280x1024
 compose:(0,0)/1280x1024]
> media-ctl --get-v4l2 "'ipu1_csi0':2"
[fmt:AYUV32/1280x1024 field:none]

I uploaded the complete topology output from media-ctrl -p as well [1].

Greetings

Florian


[1] http://www.kernelconcepts.de/~florian/media-ctl-topology.txt



-- 
The dream of yesterday  Florian Boor
is the hope of todayTel: +49 271-771091-15
and the reality of tomorrow.Fax: +49 271-338857-29
[Robert Hutchings Goddard, 1904]florian.b...@kernelconcepts.de
http://www.kernelconcepts.de/en

kernel concepts GmbH
Hauptstraße 16
D-57074 Siegen
Geschäftsführer: Ole Reinhardt
HR Siegen, HR B 9613

Re: MT9M131 on I.MX6DL CSI color issue

[Philipp Zabel]

Hi Florian,

On Fri, 2018-01-12 at 01:16 +0100, Florian Boor wrote:
> Hello all,
> 
> I have a Phytec VM-009 camera based on MT9M131 connected to CSI0 of a I.MX6DL
> based board running mainline 4.13.0 + custom devicetree. Its using the 
> parallel
> interface, 8 bit bus width on pins 12 to 19.
> 
> Basically it works pretty well apart from the really strange colors. I guess 
> its
> some YUV vs. RGB issue or similar. Here [1] is an example generated with the
> following command.
> 
> gst-launch v4l2src device=/dev/video4 num-buffers=1 ! jpegenc ! filesink
> location=capture1.jpeg
> 
> Apart from the colors everything is fine.
> I'm pretty sure I have not seen such an effect before - what might be wrong 
> here?
> 
> The current setup looks like this:
> 
> IF=UYVY2X8
> GEOM="1280x1024"
> media-ctl -l "'mt9m111 2-0048':0 -> 'ipu1_csi0_mux':4[1]"
> media-ctl -l "'ipu1_csi0_mux':5 -> 'ipu1_csi0':0[1]"
> media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]"
> 
> media-ctl -d /dev/media0 -v -V "'ipu1_csi0':2 [fmt:${IF}/${GEOM} field:none]"
> media-ctl -d /dev/media0 -v -V "'ipu1_csi0 capture':0 [fmt:${IF}/${GEOM}
> field:none]"
> media-ctl -d /dev/media0 -v -V "'ipu1_csi0_mux':4 [fmt:${IF}/${GEOM} field: 
> none]"
> media-ctl -d /dev/media0 -v -V "'ipu1_csi0_mux':5 [fmt:${IF}/${GEOM} field: 
> none]"
> media-ctl -d /dev/media0 -v -V "'mt9m111 2-0048':0 [fmt:${IF}/${GEOM} field: 
> none]"

media-ctl propagates video formats downstream, can you try reversing the
order?
Also, while the external format is UYVY2X8, internally the IPU only
supports AYUV32, so the last call should be 

media-ctl -d /dev/media0 -v -V "'ipu1_csi0':2 [fmt:AYUV32/${GEOM} field:none]"

not that it should make a difference.
And setting a format on 'ipu1_csi0 capture' is not necessary.

The new picture looks a little like there is 10-bit sensor data and only
the lower 8-bit arrive in memory, given the number of wraparounds.

Can you show the output of "media-ctl -p" (or "media-ctl --get-v4l2" for
each pad in the pipeline)?

media-ctl --get-v4l2 "'mt9m111 2-0048':0"
media-ctl --get-v4l2 "'ipu1_csi0_mux':4"
media-ctl --get-v4l2 "'ipu1_csi0_mux':5"
media-ctl --get-v4l2 "'ipu1_csi0':0"
media-ctl --get-v4l2 "'ipu1_csi0':2"

regards
Philipp

Re: [PATCH v2] vsp1: fix video output on R8A77970

[Laurent Pinchart]

Hi Sergei,

Thank you for the patch.

On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote:
> Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but

I'm not sure there's a need to state my name in the commit message.

> the video  output that VSP2-D sends to DU has a greenish garbage-like line

Why does the text in your patches (commit message, comments, ...) sometime 
have double spaces between words ?

> repeated every 8 or so screen rows.

Is it every "8 or so" rows, or exactly every 8 rows ?

> It turns out that V3M has a teeny LIF register (at least it's documented!)
> that you need to set to some kind of a  magic value for the LIF to work
> correctly...
> 
> Based on the original (and large) patch by Daisuke Matsushita
> .

What else is in the big patch ? Is it available somewhere ?

> Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and
> VSP2-D instances")
> Signed-off-by: Sergei Shtylyov 
> 
> ---
> This patch is against the 'media_tree.git' repo's 'master' branch.
> 
> Changes in version 2:
> - added a  comment before the V3M SoC check;
> - fixed indetation in that check;
> - reformatted  the patch description.
> 
>  drivers/media/platform/vsp1/vsp1_lif.c  |   12 
>  drivers/media/platform/vsp1/vsp1_regs.h |5 +
>  2 files changed, 17 insertions(+)
> 
> Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c
> ===
> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c
> +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c
> @@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en
>   (obth << VI6_LIF_CTRL_OBTH_SHIFT) |
>   (format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) |
>   VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
> +
> + /*
> +  * R-Car V3M has the buffer attribute register you absolutely need
> +  * to write kinda magic value to  for the LIF to work correctly...
> +  */

I'm not sure about the "kinda" magic value. 1536 is very likely a buffer size. 
How about the following text ?

/*
 * On V3M the LBA register has to be set to a non-default value to
 * guarantee proper operation (otherwise artifacts may appear on the
 * output). The value required by the datasheet is not documented but
 * is likely a buffer size or threshold.
 */

The commit message should also be updated to feel a bit less magic.

> + if ((entity->vsp1->version &
> +  (VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) ==
> + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) {
> + vsp1_lif_write(lif, dl, VI6_LIF_LBA,
> +VI6_LIF_LBA_LBA0 |
> +(1536 << VI6_LIF_LBA_LBA1_SHIFT));
> + }
>  }

The datasheet documents the register as being present on both V3M and M3-W 
(and the test I've just run on H3 shows that the register is present there as 
well). Should we program it on M3-W or leave it to the default value that 
should be what is recommended by the datasheet for that SoC ?

>  static const struct vsp1_entity_operations lif_entity_ops = {
> Index: media_tree/drivers/media/platform/vsp1/vsp1_regs.h
> ===
> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_regs.h
> +++ media_tree/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -693,6 +693,11 @@
>  #define VI6_LIF_CSBTH_LBTH_MASK  (0x7ff << 0)
>  #define VI6_LIF_CSBTH_LBTH_SHIFT 0
> 
> +#define VI6_LIF_LBA  0x3b0c
> +#define VI6_LIF_LBA_LBA0 (1 << 31)
> +#define VI6_LIF_LBA_LBA1_MASK(0xfff << 16)
> +#define VI6_LIF_LBA_LBA1_SHIFT   16
> +
>  /* 
>   * Security Control Registers
>   */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 4/6] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

On 12/28/2017 09:09 PM, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.
> 
> Cc: Hans Verkuil 
> Signed-off-by: Tim Harvey 
> ---
> v6:
>  - fix return on regulator enablei in tda1997x_set_power() (Fabio)
>  - replace copyright with SPDX tag (Philippe)
>  - fix colorspace handling (Hans)
> 
> v5:
>  - uppercase string constants
>  - use v4l2_hdmi_rx_coloriemtry to fill format
>  - fix V4L2_CID_DV_RX_RGB_RANGE
>  - fix interlaced mode format
> 
> v4:
>  - move include/dt-bindings/media/tda1997x.h to bindings patch
>  - fix typos
>  - fix default quant range for VGA
>  - fix quant range handling and conv matrix
>  - add additional standards and capabilities to timings_cap
> 
> v3:
>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>  - fixed missing break
>  - use only hdmi_infoframe_log for infoframe logging
>  - simplify tda1997x_s_stream error handling
>  - add delayed work proc to handle hotplug enable/disable
>  - fix set_edid (disable HPD before writing, enable after)
>  - remove enabling edid by default
>  - initialize timings
>  - take quant range into account in colorspace conversion
>  - remove vendor/product tracking (we provide this in log_status via 
> infoframes)
>  - add v4l_controls
>  - add more detail to log_status
>  - calculate vhref generator timings
>  - timing detection fixes (rounding errors, hswidth errors)
>  - rename configure_input/configure_conv functions
> 
> v2:
>  - implement dv timings enum/cap
>  - remove deprecated g_mbus_config op
>  - fix dv_query_timings
>  - add EDID get/set handling
>  - remove max-pixel-rate support
>  - add audio codec DAI support
>  - change audio bindings
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3476 
> ++
>  include/media/i2c/tda1997x.h |   41 +
>  4 files changed, 3527 insertions(+)
>  create mode 100644 drivers/media/i2c/tda1997x.c
>  create mode 100644 include/media/i2c/tda1997x.h
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 3c6d642..abf24b9 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -56,6 +56,15 @@ config VIDEO_TDA9840
> To compile this driver as a module, choose M here: the
> module will be called tda9840.
>  
> +config VIDEO_TDA1997X
> + tristate "NXP TDA1997x HDMI receiver"
> + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> + ---help---
> +   V4L2 subdevice driver for the NXP TDA1997x HDMI receivers.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called tda1997x.
> +
>  config VIDEO_TEA6415C
>   tristate "Philips TEA6415C audio processor"
>   depends on I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 548a9ef..adfcae9 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_VIDEO_TVAUDIO) += tvaudio.o
>  obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o
>  obj-$(CONFIG_VIDEO_SAA6588) += saa6588.o
>  obj-$(CONFIG_VIDEO_TDA9840) += tda9840.o
> +obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
>  obj-$(CONFIG_VIDEO_TEA6415C) += tea6415c.o
>  obj-$(CONFIG_VIDEO_TEA6420) += tea6420.o
>  obj-$(CONFIG_VIDEO_SAA7110) += saa7110.o
> diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
> new file mode 100644
> index 000..0993c13
> --- /dev/null
> +++ b/drivers/media/i2c/tda1997x.c
> @@ -0,0 +1,3476 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 Gateworks Corporation
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/* debug level */
> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "debug level (0-2)");
> +
> +/* Page 0x00 - General Control */
> +#define REG_VERSION  0x
> +#define REG_INPUT_SEL0x0001
> +#define REG_SVC_MODE 0x0002
> +#define REG_HPD_MAN_CTRL 0x0003
> +#define REG_RT_MAN_CTRL  0x0004
> +#define REG_STANDBY_SOFT_RST 0x000A
> +#define REG_HDMI_SOFT_RST0x000B
> +#define REG_HDMI_INFO_RST0x000C
> +#define REG_INT_FLG_CLR_TOP  0x000E
> +#define REG_INT_FLG_CLR_SUS  0x000F
> +#define REG_INT_FLG_CLR_DDC  0x0010
> +#define REG_INT_FLG_CLR_RATE 0x0011
> +#define REG_INT_FLG_CLR_MODE 0x0012
> +#define REG_INT_FLG_CLR_INFO 0x0013
> +#define REG_INT_FLG_CLR_AUDIO0x0014
> +#define REG_INT_FLG_CLR_HDCP 0x0015
> +#define REG_INT_FLG_CLR_AFE  0x0016
> +#define REG_INT_MASK_TOP 0x0017
> +#define REG_INT_MASK_SUS 0x0018
> +#define REG_INT_MASK_DDC 0x0019
> +#define REG_INT_MASK_RATE0x001A
> 

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Hans Verkuil]

On 01/15/2018 01:01 PM, Gustavo Padovan wrote:
> 2018-01-15 Alexandre Courbot :
> 
>> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan  wrote:
>>> From: Gustavo Padovan 
>>>
>>> Explicit synchronization benefits a lot from ordered queues, they fit
>>> better in a pipeline with DRM for example so create a opt-in way for
>>> drivers notify videobuf2 that the queue is unordered.
>>>
>>> Drivers don't need implement it if the queue is ordered.
>>
>> This is going to make user-space believe that *all* vb2 drivers use
>> ordered queues by default, at least until non-ordered drivers catch up
>> with this change. Wouldn't it be less dangerous to do the opposite
>> (make queues non-ordered by default)?
> 
> The rational behind this decision was because most formats/drivers are
> ordered so only a small amount of drivers need to changed. I think this
> was proposed by Hans on the Media Summit.
> 
> I understand your concern. My question is how dangerous will it be. If
> you are building a product you will make the changes in the driver if
> they are not there yet, or if it is a distribution you'd never know
> which driver/format you are using so you should be prepared for
> everything.
> 
> AFAIK all Capture drivers are ordered and that is where I think fences
> is most useful.

Right. What could be done is to mark all codec drivers as unordered initially
ask the driver authors to verify this. All capture drivers using vb2 and not
using REQUEUE are ordered.

One thing we haven't looked at is what to do with drivers that do not use vb2.
Those won't support fences, but how will userspace know that fences are not
supported? I'm not sure what the best method is for that.

I am leaning towards a new capability since this has to be advertised clearly.

Regards,

Hans

Re: [PATCH v7 1/6] [media] vb2: add is_unordered callback for drivers

[Gustavo Padovan]

2018-01-15 Alexandre Courbot :

> On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan  wrote:
> > From: Gustavo Padovan 
> >
> > Explicit synchronization benefits a lot from ordered queues, they fit
> > better in a pipeline with DRM for example so create a opt-in way for
> > drivers notify videobuf2 that the queue is unordered.
> >
> > Drivers don't need implement it if the queue is ordered.
> 
> This is going to make user-space believe that *all* vb2 drivers use
> ordered queues by default, at least until non-ordered drivers catch up
> with this change. Wouldn't it be less dangerous to do the opposite
> (make queues non-ordered by default)?

The rational behind this decision was because most formats/drivers are
ordered so only a small amount of drivers need to changed. I think this
was proposed by Hans on the Media Summit.

I understand your concern. My question is how dangerous will it be. If
you are building a product you will make the changes in the driver if
they are not there yet, or if it is a distribution you'd never know
which driver/format you are using so you should be prepared for
everything.

AFAIK all Capture drivers are ordered and that is where I think fences
is most useful.

Gustavo

Re: MT9M131 on I.MX6DL CSI color issue

[Florian Boor]

Hello Anatolij,

many thanks for explaining. It changed something at least - see below.

On 12.01.2018 11:06, Anatolij Gustschin wrote:
> On Fri, 12 Jan 2018 10:58:40 +0100
> Anatolij Gustschin ag...@denx.de wrote:
> ...
> I forgot the videoconvert, sorry. Try
> 
>   gst-launch v4l2src device=/dev/video4 num-buffers=1 ! \
>  filesink location=frame.raw
> 
>   gst-launch filesrc num-buffers=1 Lotion=frame.raw ! \
>  videoparse format=5 width=1280 height=1024 framerate=25/1 ! \
>  videoconvert ! jpegenc ! filesink location=capture1.jpeg

Capturing like this the colors turn a little bit less psychedelic green and
purple. Looks like this:
http://www.kernelconcepts.de/~florian/capture2.jpeg
The dark area is in fact a very bright one. So maybe the format I read from the
sensor is not exactly what it is supposed to be or similar...

Greetings

Florian

-- 
The dream of yesterday  Florian Boor
is the hope of todayTel: +49 271-771091-15
and the reality of tomorrow.Fax: +49 271-338857-29
[Robert Hutchings Goddard, 1904]florian.b...@kernelconcepts.de
http://www.kernelconcepts.de/en

kernel concepts GmbH
Hauptstraße 16
D-57074 Siegen
Geschäftsführer: Ole Reinhardt
HR Siegen, HR B 9613

[PATCH 3/5] auxdisplay: charlcd: add escape sequence for brightness on NEC µPD16314

[Sean Young]

The NEC µPD16314 can alter the the brightness of the LCD. Make it possible
to set this via escape sequence Y0 - Y3. B and R were already taken, so
I picked Y for luminance.

Signed-off-by: Sean Young 
---
 drivers/auxdisplay/charlcd.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index a16c72779722..7a671ad959d1 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -39,6 +39,8 @@
 #define LCD_FLAG_F 0x0020  /* Large font mode */
 #define LCD_FLAG_N 0x0040  /* 2-rows mode */
 #define LCD_FLAG_L 0x0080  /* Backlight enabled */
+#define LCD_BRIGHTNESS_MASK0x0300  /* Brightness */
+#define LCD_BRIGHTNESS_SHIFT   8
 
 /* LCD commands */
 #define LCD_CMD_DISPLAY_CLEAR  0x01/* Clear entire display */
@@ -490,6 +492,17 @@ static inline int handle_lcd_special_code(struct charlcd 
*lcd)
charlcd_gotoxy(lcd);
processed = 1;
break;
+   case 'Y':   /* brightness (luma) */
+   switch (esc[1]) {
+   case '0':   /* 25% */
+   case '1':   /* 50% */
+   case '2':   /* 75% */
+   case '3':   /* 100% */
+   priv->flags = (priv->flags & ~(LCD_BRIGHTNESS_MASK)) |
+   (('3' - esc[1]) << LCD_BRIGHTNESS_SHIFT);
+   processed =  1;
+   break;
+   }
}
 
/* TODO: This indent party here got ugly, clean it! */
@@ -507,12 +520,15 @@ static inline int handle_lcd_special_code(struct charlcd 
*lcd)
((priv->flags & LCD_FLAG_C) ? LCD_CMD_CURSOR_ON : 0) |
((priv->flags & LCD_FLAG_B) ? LCD_CMD_BLINK_ON : 0));
/* check whether one of F,N flags was changed */
-   else if ((oldflags ^ priv->flags) & (LCD_FLAG_F | LCD_FLAG_N))
+   else if ((oldflags ^ priv->flags) & (LCD_FLAG_F | LCD_FLAG_N |
+LCD_BRIGHTNESS_MASK))
lcd->ops->write_cmd(lcd,
LCD_CMD_FUNCTION_SET |
((lcd->ifwidth == 8) ? LCD_CMD_DATA_LEN_8BITS : 0) |
((priv->flags & LCD_FLAG_F) ? LCD_CMD_FONT_5X10_DOTS : 
0) |
-   ((priv->flags & LCD_FLAG_N) ? LCD_CMD_TWO_LINES : 0));
+   ((priv->flags & LCD_FLAG_N) ? LCD_CMD_TWO_LINES : 0) |
+   ((priv->flags & LCD_BRIGHTNESS_MASK) >>
+   LCD_BRIGHTNESS_SHIFT));
/* check whether L flag was changed */
else if ((oldflags ^ priv->flags) & LCD_FLAG_L)
charlcd_backlight(lcd, !!(priv->flags & LCD_FLAG_L));
-- 
2.14.3

[PATCH 5/5] media: rc: new driver for Sasem Remote Controller VFD/IR

[Sean Young]

This device is built into the Ahanix D.Vine 5 HTPC case. It has an LCD
device, and an IR receiver.

The LCD can be controlled via the charlcd driver. Unfortunately the device
does not seem to provide a method for accessing the character generator
ram.

Signed-off-by: Sean Young 
---
 MAINTAINERS |   6 +
 drivers/media/rc/Kconfig|  16 +++
 drivers/media/rc/Makefile   |   1 +
 drivers/media/rc/sasem_ir.c | 297 
 4 files changed, 320 insertions(+)
 create mode 100644 drivers/media/rc/sasem_ir.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 58797b83dd8d..a06801032ee3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12066,6 +12066,12 @@ F: drivers/phy/samsung/phy-s5pv210-usb2.c
 F: drivers/phy/samsung/phy-samsung-usb2.c
 F: drivers/phy/samsung/phy-samsung-usb2.h
 
+SASEM REMOTE CONTROLLER
+M: Sean Young 
+L: linux-media@vger.kernel.org
+S: Maintained
+F: drivers/media/rc/sasem_ir.c
+
 SC1200 WDT DRIVER
 M: Zwane Mwaikambo 
 S: Maintained
diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 7919f4a36ad2..bffa39e06a68 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -457,6 +457,22 @@ config IR_SERIAL
   To compile this driver as a module, choose M here: the module will
   be called serial-ir.
 
+config IR_SASEM
+   tristate "Sasem Remote Controller"
+   depends on USB_ARCH_HAS_HCD
+   depends on RC_CORE
+   select USB
+   select CHARLCD
+   ---help---
+  Driver for the Sasem OnAir Remocon-V or Dign HV5 HTPC IR/VFD Module
+
+  The LCD can be controlled via the charlcd driver. Unfortunately the
+  device does not seem to provide a method for accessing the
+  character generator ram.
+
+  To compile this driver as a module, choose M here: the module will
+  be called sesam_ir.
+
 config IR_SERIAL_TRANSMITTER
bool "Serial Port Transmitter"
default y
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index e098e127b26a..9b474c8b49dc 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o
 obj-$(CONFIG_RC_ST) += st_rc.o
 obj-$(CONFIG_IR_SUNXI) += sunxi-cir.o
 obj-$(CONFIG_IR_IMG) += img-ir/
+obj-$(CONFIG_IR_SASEM) += sasem_ir.o
 obj-$(CONFIG_IR_SERIAL) += serial_ir.o
 obj-$(CONFIG_IR_SIR) += sir_ir.o
 obj-$(CONFIG_IR_MTK) += mtk-cir.o
diff --git a/drivers/media/rc/sasem_ir.c b/drivers/media/rc/sasem_ir.c
new file mode 100644
index ..33d3b8bdb56d
--- /dev/null
+++ b/drivers/media/rc/sasem_ir.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright (C) 2018 Sean Young 
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+struct sasem {
+   struct device *dev;
+   struct urb *ir_urb;
+   struct urb *vfd_urb;
+   struct rc_dev *rcdev;
+   struct completion completion;
+   u8 ir_buf[8];
+   u8 vfd_buf[8];
+   unsigned int offset;
+   char phys[64];
+};
+
+static void sasem_ir_rx(struct urb *urb)
+{
+   struct sasem *sasem = urb->context;
+   enum rc_proto proto;
+   u32 code;
+   int ret;
+
+   switch (urb->status) {
+   case 0:
+   dev_dbg(sasem->dev, "data: %*ph", 8, sasem->ir_buf);
+   /*
+* Note that sanyo and jvc protocols are also supported,
+* but the scancode seems garbled. More testing needed.
+*/
+   switch (sasem->ir_buf[0]) {
+   case 0xc:
+   code = ir_nec_bytes_to_scancode(sasem->ir_buf[1],
+   sasem->ir_buf[2], sasem->ir_buf[3],
+   sasem->ir_buf[4], );
+   rc_keydown(sasem->rcdev, proto, code, 0);
+   break;
+   case 0x8:
+   rc_repeat(sasem->rcdev);
+   break;
+   }
+   break;
+   case -ECONNRESET:
+   case -ENOENT:
+   case -ESHUTDOWN:
+   usb_unlink_urb(urb);
+   return;
+   case -EPIPE:
+   default:
+   dev_dbg(sasem->dev, "error: urb status = %d", urb->status);
+   break;
+   }
+
+   ret = usb_submit_urb(urb, GFP_ATOMIC);
+   if (ret && ret != -ENODEV)
+   dev_warn(sasem->dev, "failed to resubmit urb: %d", ret);
+}
+
+static void sasem_vfd_complete(struct urb *urb)
+{
+   struct sasem *sasem = urb->context;
+
+   if (urb->status)
+   dev_info(sasem->dev, "error: vfd urb status = %d", urb->status);
+
+   complete(>completion);
+}
+
+static int sasem_vfd_send(struct sasem *sasem)
+{
+   int ret;
+
+   reinit_completion(>completion);
+
+   ret = usb_submit_urb(sasem->vfd_urb, GFP_KERNEL);
+   if (ret)
+

[PATCH 4/5] media: rc: add keymap for Dign Remote

[Sean Young]

This is the remote which comes with the Ahanix D.Vine 5 HTPC case.

Signed-off-by: Sean Young 
---
 drivers/media/rc/keymaps/Makefile  |  1 +
 drivers/media/rc/keymaps/rc-dign.c | 70 ++
 include/media/rc-map.h |  1 +
 3 files changed, 72 insertions(+)
 create mode 100644 drivers/media/rc/keymaps/rc-dign.c

diff --git a/drivers/media/rc/keymaps/Makefile 
b/drivers/media/rc/keymaps/Makefile
index 50b319355edf..5a7aebe12285 100644
--- a/drivers/media/rc/keymaps/Makefile
+++ b/drivers/media/rc/keymaps/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_RC_MAP) += rc-adstech-dvb-t-pci.o \
rc-dib0700-rc5.o \
rc-digitalnow-tinytwin.o \
rc-digittrade.o \
+   rc-dign.o \
rc-dm1105-nec.o \
rc-dntv-live-dvb-t.o \
rc-dntv-live-dvbt-pro.o \
diff --git a/drivers/media/rc/keymaps/rc-dign.c 
b/drivers/media/rc/keymaps/rc-dign.c
new file mode 100644
index ..a70bb5f89278
--- /dev/null
+++ b/drivers/media/rc/keymaps/rc-dign.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Keymap for Dign Remote for Ahanix D.Vine 5 Case
+//
+// Copyright (C) 2018 by Sean Young 
+
+#include 
+#include 
+
+static struct rc_map_table dign[] = {
+   { 0x8000, KEY_POWER },
+   { 0x8002, KEY_EJECTCD },
+   { 0x8008, KEY_1 },
+   { 0x8009, KEY_2 },
+   { 0x800a, KEY_3 },
+   { 0x8010, KEY_4 },
+   { 0x8011, KEY_5 },
+   { 0x8012, KEY_6 },
+   { 0x8018, KEY_7 },
+   { 0x8019, KEY_8 },
+   { 0x801a, KEY_9 },
+
+   { 0x8040, KEY_ENTER }, /* Start/Windows */
+   { 0x8041, KEY_0 },
+   { 0x8006, KEY_MENU },
+
+   { 0x800f, KEY_VOLUMEDOWN },
+   { 0x800e, KEY_VOLUMEUP },
+
+   { 0x8005, KEY_ESC },
+   { 0x8007, KEY_CLOSE },
+   { 0x800b, KEY_UP },
+   { 0x8003, KEY_LEFT },
+   { 0x801b, KEY_RIGHT },
+   { 0x8043, KEY_DOWN },
+   { 0x8013, KEY_ENTER },
+
+   { 0x800c, KEY_PREVIOUSSONG },
+   { 0x8001, KEY_NEXTSONG },
+
+   { 0x800d, KEY_MUTE },
+   { 0x8004, KEY_FRAMEFORWARD }, /* Step */
+   { 0x8049, KEY_PLAYPAUSE },
+   { 0x8048, KEY_STOP },
+};
+
+static struct rc_map_list dign_map = {
+   .map = {
+   .scan = dign,
+   .size = ARRAY_SIZE(dign),
+   .rc_proto = RC_PROTO_NEC,
+   .name = RC_MAP_DIGN,
+   }
+};
+
+static int __init init_rc_map_dign(void)
+{
+   return rc_map_register(_map);
+}
+
+static void __exit exit_rc_map_dign(void)
+{
+   rc_map_unregister(_map);
+}
+
+module_init(init_rc_map_dign)
+module_exit(exit_rc_map_dign)
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sean Young ");
diff --git a/include/media/rc-map.h b/include/media/rc-map.h
index 7046734b3895..1c0016af45a1 100644
--- a/include/media/rc-map.h
+++ b/include/media/rc-map.h
@@ -185,6 +185,7 @@ struct rc_map *rc_map_get(const char *name);
 #define RC_MAP_DIB0700_RC5_TABLE "rc-dib0700-rc5"
 #define RC_MAP_DIGITALNOW_TINYTWIN   "rc-digitalnow-tinytwin"
 #define RC_MAP_DIGITTRADE"rc-digittrade"
+#define RC_MAP_DIGN "rc-dign"
 #define RC_MAP_DM1105_NEC"rc-dm1105-nec"
 #define RC_MAP_DNTV_LIVE_DVBT_PRO"rc-dntv-live-dvbt-pro"
 #define RC_MAP_DNTV_LIVE_DVB_T   "rc-dntv-live-dvb-t"
-- 
2.14.3

[PATCH 2/5] auxdisplay: charlcd: add flush function

[Sean Young]

The Sasem Remote Controller has an LCD, which is connnected via usb.
Multiple write reg or write data commands can be combined into one usb
packet.

The latency of usb is such that if we send commands one by one, we get
very obvious tearing on the LCD.

By adding a flush function, we can buffer all commands until either
the usb packet is full or the lcd changes are complete.

Signed-off-by: Sean Young 
---
 drivers/auxdisplay/charlcd.c | 6 ++
 include/misc/charlcd.h   | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 45ec5ce697c4..a16c72779722 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -642,6 +642,9 @@ static ssize_t charlcd_write(struct file *file, const char 
__user *buf,
charlcd_write_char(the_charlcd, c);
}
 
+   if (the_charlcd->ops->flush)
+   the_charlcd->ops->flush(the_charlcd);
+
return tmp - buf;
 }
 
@@ -703,6 +706,9 @@ static void charlcd_puts(struct charlcd *lcd, const char *s)
 
charlcd_write_char(lcd, *tmp);
}
+
+   if (lcd->ops->flush)
+   lcd->ops->flush(lcd);
 }
 
 /* initialize the LCD driver */
diff --git a/include/misc/charlcd.h b/include/misc/charlcd.h
index 23f61850f363..ff8fd456018e 100644
--- a/include/misc/charlcd.h
+++ b/include/misc/charlcd.h
@@ -32,6 +32,7 @@ struct charlcd_ops {
void (*write_cmd_raw4)(struct charlcd *lcd, int cmd);   /* 4-bit only */
void (*clear_fast)(struct charlcd *lcd);
void (*backlight)(struct charlcd *lcd, int on);
+   void (*flush)(struct charlcd *lcd);
 };
 
 struct charlcd *charlcd_alloc(unsigned int drvdata_size);
-- 
2.14.3

[PATCH 0/5] new driver for Ahanix D.Vine 5 IR/VFD

[Sean Young]

This is a newer driver for this device. It originally supported by the
lirc_sasem.c staging driver, which was removed in kernel v4.12.

Here a some more information about the hardware and my attempts to
understand it:

http://www.mess.org/2018/01/17/Ahanix-D-Vine-5-IR-VFD-module/

Sean Young (5):
  auxdisplay: charlcd: no need to call charlcd_gotoxy() if nothing
changes
  auxdisplay: charlcd: add flush function
  auxdisplay: charlcd: add escape sequence for brightness on NEC
µPD16314
  media: rc: add keymap for Dign Remote
  media: rc: new driver for Sasem Remote Controller VFD/IR

 MAINTAINERS|   6 +
 drivers/auxdisplay/charlcd.c   |  33 -
 drivers/media/rc/Kconfig   |  16 ++
 drivers/media/rc/Makefile  |   1 +
 drivers/media/rc/keymaps/Makefile  |   1 +
 drivers/media/rc/keymaps/rc-dign.c |  70 +
 drivers/media/rc/sasem_ir.c| 297 +
 include/media/rc-map.h |   1 +
 include/misc/charlcd.h |   1 +
 9 files changed, 421 insertions(+), 5 deletions(-)
 create mode 100644 drivers/media/rc/keymaps/rc-dign.c
 create mode 100644 drivers/media/rc/sasem_ir.c

-- 
2.14.3

[PATCH 1/5] auxdisplay: charlcd: no need to call charlcd_gotoxy() if nothing changes

[Sean Young]

If the line extends beyond the width to the screen, nothing changes. The
existing code will call charlcd_gotoxy every time for this case.

Signed-off-by: Sean Young 
---
 drivers/auxdisplay/charlcd.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 642afd88870b..45ec5ce697c4 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -192,10 +192,11 @@ static void charlcd_print(struct charlcd *lcd, char c)
c = lcd->char_conv[(unsigned char)c];
lcd->ops->write_data(lcd, c);
priv->addr.x++;
+
+   /* prevents the cursor from wrapping onto the next line */
+   if (priv->addr.x == lcd->bwidth)
+   charlcd_gotoxy(lcd);
}
-   /* prevents the cursor from wrapping onto the next line */
-   if (priv->addr.x == lcd->bwidth)
-   charlcd_gotoxy(lcd);
 }
 
 static void charlcd_clear_fast(struct charlcd *lcd)
-- 
2.14.3

Re: [PATCH] dma-buf/sw_sync: fix document of sw_sync_create_fence_data

[Daniel Vetter]

On Mon, Jan 15, 2018 at 11:47:59AM +0800, Shawn Guo wrote:
> The structure should really be sw_sync_create_fence_data rather than
> sw_sync_ioctl_create_fence which is the function name.
> 
> Signed-off-by: Shawn Guo 

Applied, thanks for your patch.
-Daniel

> ---
>  drivers/dma-buf/sw_sync.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 24f83f9eeaed..7779bdbd18d1 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -43,14 +43,14 @@
>   * timelines.
>   *
>   * Fences can be created with SW_SYNC_IOC_CREATE_FENCE ioctl with struct
> - * sw_sync_ioctl_create_fence as parameter.
> + * sw_sync_create_fence_data as parameter.
>   *
>   * To increment the timeline counter, SW_SYNC_IOC_INC ioctl should be used
>   * with the increment as u32. This will update the last signaled value
>   * from the timeline and signal any fence that has a seqno smaller or equal
>   * to it.
>   *
> - * struct sw_sync_ioctl_create_fence
> + * struct sw_sync_create_fence_data
>   * @value:   the seqno to initialise the fence with
>   * @name:the name of the new sync point
>   * @fence:   return the fd of the new sync_file with the created fence
> -- 
> 1.9.1
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH 6/9] media: vb2: add support for requests in QBUF ioctl

[Hans Verkuil]

On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
> On Fri, Jan 12, 2018 at 8:37 PM, Hans Verkuil  wrote:
>> On 12/15/17 08:56, Alexandre Courbot wrote:
>>> Support the request argument of the QBUF ioctl.
>>>
>>> Signed-off-by: Alexandre Courbot 
>>> ---
>>>  drivers/media/v4l2-core/v4l2-ioctl.c | 93 
>>> +++-
>>>  1 file changed, 92 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
>>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 8d041247e97f..28f9c368563e 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -29,6 +29,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include 
>>>
>>> @@ -965,6 +966,81 @@ static int check_fmt(struct file *file, enum 
>>> v4l2_buf_type type)
>>>   return -EINVAL;
>>>  }
>>>
>>> +/*
>>> + * Validate that a given request can be used during an ioctl.
>>> + *
>>> + * When using the request API, request file descriptors must be matched 
>>> against
>>> + * the actual request object. User-space can pass any file descriptor, so 
>>> we
>>> + * need to make sure the call is valid before going further.
>>> + *
>>> + * This function looks up the request and associated data and performs the
>>> + * following sanity checks:
>>> + *
>>> + * * Make sure that the entity supports requests,
>>> + * * Make sure that the entity belongs to the media_device managing the 
>>> passed
>>> + *   request,
>>> + * * Make sure that the entity data (if any) is associated to the current 
>>> file
>>> + *   handler.
>>> + *
>>> + * This function returns a pointer to the valid request, or and error code 
>>> in
>>> + * case of failure. When successful, a reference to the request is 
>>> acquired and
>>> + * must be properly released.
>>> + */
>>> +#ifdef CONFIG_MEDIA_CONTROLLER
>>> +static struct media_request *
>>> +check_request(int request, struct file *file, void *fh)
>>> +{
>>> + struct media_request *req = NULL;
>>> + struct video_device *vfd = video_devdata(file);
>>> + struct v4l2_fh *vfh =
>>> + test_bit(V4L2_FL_USES_V4L2_FH, >flags) ? fh : NULL;
>>> + struct media_entity *entity = >entity;
>>> + const struct media_entity *ent;
>>> + struct media_request_entity_data *data;
>>> + bool found = false;
>>> +
>>> + if (!entity)
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> + /* Check that the entity supports requests */
>>> + if (!entity->req_ops)
>>> + return ERR_PTR(-ENOTSUPP);
>>> +
>>> + req = media_request_get_from_fd(request);
>>
>> You can get the media_device from vfd->v4l2_dev->mdev. So it is much easier
>> to just pass the media_device as an argument to 
>> media_request_get_from_fd()...
>>
>>> + if (!req)
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> + /* Validate that the entity belongs to the media_device managing
>>> +  * the request queue */
>>> + media_device_for_each_entity(ent, req->queue->mdev) {
>>> + if (entity == ent) {
>>> + found = true;
>>> + break;
>>> + }
>>> + }
>>> + if (!found) {
>>> + media_request_put(req);
>>> + return ERR_PTR(-EINVAL);
>>> + }
>>
>> ...and then you don't need to do this ^^^ extra validation check.
> 
> Ah right, all you need to do is check that req->queue->mdev ==
> vfd->v4l2_dev->mdev and you can get rid of this whole block.

Correct.

 I don't
> think we can do that in media_request_get_from_fd() though, since it
> is called from other places where (IIUC) we don't have access to the
> media_device.

Yes, sorry about that. Ignore my comment about media_request_get_from_fd().
I misunderstood what that function did.

> 
>>
>>> +
>>> + /* Validate that the entity's data belongs to the correct fh */
>>> + data = media_request_get_entity_data(req, entity, vfh);
>>> + if (IS_ERR(data)) {
>>> + media_request_put(req);
>>> + return ERR_PTR(PTR_ERR(data));
>>> + }
>>
>> This assumes that each filehandle has its own state. That's true for codecs,
>> but not for most (all?) other devices. There the state is per device 
>> instance.
>>
>> I'm not sure if we have a unique identifying mark for such drivers. The 
>> closest
>> is checking if fh->m2m_ctx is non-NULL, but I don't know if all drivers with
>> per-filehandle state use that field. An alternative might be to check if
>> fh->ctrl_handler is non-NULL. But again, I'm not sure if that's a 100% valid
>> check.
> 
> I think the current code already takes that case into account: if the
> device does not uses v4l2_fh, then the fh argument passed to
> media_request_get_entity_data() will be null, and so will be data->fh.
> 

Using v4l2_fh doesn't mean it has per-filehandle state. Most drivers only have 
global
state and they still use v4l2_fh.

This is why you should also look at 

Re: [RFC PATCH 5/9] media: vb2: add support for requests

[Hans Verkuil]

On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
> On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil  wrote:
>> On 12/15/17 08:56, Alexandre Courbot wrote:
>>> Add throttling support for buffers when requests are in use on a given
>>> queue. Buffers associated to a request are kept into the vb2 queue until
>>> the request becomes active, at which point all the buffers are passed to
>>> the driver. The queue can also signal that is has processed all of a
>>> request's buffers.
>>>
>>> Also add support for the request parameter when handling the QBUF ioctl.
>>>
>>> Signed-off-by: Alexandre Courbot 
>>> ---
>>>  drivers/media/v4l2-core/videobuf2-core.c | 59 
>>> 
>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
>>>  include/media/videobuf2-core.h   | 25 +-
>>>  3 files changed, 104 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
>>> b/drivers/media/v4l2-core/videobuf2-core.c
>>> index cb115ba6a1d2..c01038b7962a 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>>> vb2_buffer_state state)
>>>   state != VB2_BUF_STATE_REQUEUEING))
>>>   state = VB2_BUF_STATE_ERROR;
>>>
>>> + WARN_ON(vb->request != q->cur_req);
>>
>> What's the reason for this WARN_ON? It's not immediately obvious to me.
> 
> This is a safeguard against driver bugs: a buffer should not complete
> unless it is part of the request being currently processed.
> 
>>
>>> +
>>>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>>>   /*
>>>* Although this is not a callback, it still does have to balance
>>> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>>> vb2_buffer_state state)
>>>   /* Add the buffer to the done buffers list */
>>>   list_add_tail(>done_entry, >done_list);
>>>   vb->state = state;
>>> +
>>> + if (q->cur_req) {
>>> + WARN_ON(q->req_buf_cnt < 1);
>>> +
>>> + if (--q->req_buf_cnt == 0)
>>> + q->cur_req = NULL;
>>> + }
>>>   }
>>>   atomic_dec(>owned_by_drv_count);
>>>   spin_unlock_irqrestore(>done_lock, flags);
>>> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, 
>>> unsigned int index, void *pb)
>>>  }
>>>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>>
>>> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
>>> +{
>>> + struct vb2_buffer *vb;
>>> +
>>> + list_for_each_entry(vb, >queued_list, queued_entry) {
>>> + if (vb->request == q->cur_req)
>>> + __enqueue_in_driver(vb);
>>> + }
>>> +}
>>
>> I think this will clash big time with the v4l2 fence patch series...
> 
> Indeed, but on the other hand I was not a big fan of going through the
> whole list. :) So I welcome the extra throttling introduced by the
> fence series.

There is only throttling if fences are used by userspace. Otherwise there
is no change.

> 
>>
>>> +
>>>  /**
>>>   * vb2_start_streaming() - Attempt to start streaming.
>>>   * @q:   videobuf2 queue
>>> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>>* If any buffers were queued before streamon,
>>>* we can now pass them to driver for processing.
>>>*/
>>> - list_for_each_entry(vb, >queued_list, queued_entry)
>>> - __enqueue_in_driver(vb);
>>> + vb2_queue_enqueue_current_buffers(q);
>>>
>>>   /* Tell the driver to start streaming */
>>>   q->start_streaming_called = 1;
>>> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>>   return ret;
>>>  }
>>>
>>> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>>> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
>>> +   struct media_request *req, void *pb)
>>>  {
>>>   struct vb2_buffer *vb;
>>>   int ret;
>>> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>>> index, void *pb)
>>>   q->queued_count++;
>>>   q->waiting_for_buffers = false;
>>>   vb->state = VB2_BUF_STATE_QUEUED;
>>> + vb->request = req;
>>>
>>>   if (pb)
>>>   call_void_bufop(q, copy_timestamp, vb, pb);
>>> @@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>>> index, void *pb)
>>>   /*
>>>* If already streaming, give the buffer to driver for processing.
>>>* If not, the buffer will be given to driver on next streamon.
>>> +  *
>>> +  * If using the request API, the buffer will be given to the driver
>>> +  * when the request becomes active.
>>>*/
>>> - if (q->start_streaming_called)
>>> + if (q->start_streaming_called && !req)
>>>   

Re: [RFC PATCH 0/9] media: base request API support

[Hans Verkuil]

On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
> Hi Hans,
> 
> On Fri, Jan 12, 2018 at 8:45 PM, Hans Verkuil  wrote:
>> Hi Alexandre,
>>
>> On 12/15/17 08:56, Alexandre Courbot wrote:
>>> Here is a new attempt at the request API, following the UAPI we agreed on in
>>> Prague. Hopefully this can be used as the basis to move forward.
>>>
>>> This series only introduces the very basics of how requests work: allocate a
>>> request, queue buffers to it, queue the request itself, wait for it to 
>>> complete,
>>> reuse it. It does *not* yet use Hans' work with controls setting. I have
>>> preferred to submit it this way for now as it allows us to concentrate on 
>>> the
>>> basic request/buffer flow, which was harder to get properly than I initially
>>> thought. I still have a gut feeling that it can be improved, with less 
>>> back-and-
>>> forth into drivers.
>>>
>>> Plugging in controls support should not be too hard a task (basically just 
>>> apply
>>> the saved controls when the request starts), and I am looking at it now.
>>>
>>> The resulting vim2m driver can be successfully used with requests, and my 
>>> tests
>>> so far have been successful.
>>>
>>> There are still some rougher edges:
>>>
>>> * locking is currently quite coarse-grained
>>> * too many #ifdef CONFIG_MEDIA_CONTROLLER in the code, as the request API
>>>   depends on it - I plan to craft the headers so that it becomes 
>>> unnecessary.
>>>   As it is, some of the code will probably not even compile if
>>>   CONFIG_MEDIA_CONTROLLER is not set
>>>
>>> But all in all I think the request flow should be clear and easy to review, 
>>> and
>>> the possibility of custom queue and entity support implementations should 
>>> give
>>> us the flexibility we need to support more specific use-cases (I expect the
>>> generic implementations to be sufficient most of the time though).
>>>
>>> A very simple test program exercising this API is available here (don't 
>>> forget
>>> to adapt the /dev/media0 hardcoding):
>>> https://gist.github.com/Gnurou/dbc3776ed97ea7d4ce6041ea15eb0438
>>>
>>> Looking forward to your feedback and comments!
>>
>> I think this will become more interesting when the control code is in.
> 
> Definitely.
> 
>> The main thing I've noticed with this patch series is that it is very codec
>> oriented. Which in some ways is OK (after all, that's the first type of HW
>> that we want to support), but the vb2 code in particular should be more
>> generic.
> 
> I don't want to expand too much into use-cases I do not master ; doing
> so would be speculating about how the API will be used. But feel free
> to point out where you think my focus on the codec use-case is not
> future-proof.

The vb2 framework is a core component and it should function properly with
the request API for all drivers using it.

Easiest would be to test this using vivid and vimc. The vb2 code is relatively
simple: all it has to do is keep track of requests and buffers and queue them
up to the driver at the right time. It doesn't really need to know if the driver
is a codec or something else, it's all the same to the framework.

Regards,

Hans

>> I would also recommend that you start preparing documentation patches: we
>> can review that and make sure all the corner-cases are correctly documented.
>>
>> The public API changes are (I think) fairly limited, but the devil is in
>> the details, so getting that reviewed early on will help you later.
> 
> Yeah, I now regret to have submitted this series without
> documentation. Won't do that mistake again.
> 
>> It's a bit unfortunate that the fence patch series is also making vb2 
>> changes,
>> but I hope that will be merged fairly soon so you can develop on top of that
>> series.
> 
> The fence series may actually make things easier. The vb2 code of this
> series is a bit confusing, and fences add a few extra constraints that
> should make things more predictable. So I am looking forward to being
> able to work on top of it.
> 

Re: [RFC PATCH 5/9] media: vb2: add support for requests

[Alexandre Courbot]

On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil  wrote:
> On 12/15/17 08:56, Alexandre Courbot wrote:
>> Add throttling support for buffers when requests are in use on a given
>> queue. Buffers associated to a request are kept into the vb2 queue until
>> the request becomes active, at which point all the buffers are passed to
>> the driver. The queue can also signal that is has processed all of a
>> request's buffers.
>>
>> Also add support for the request parameter when handling the QBUF ioctl.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 59 
>> 
>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
>>  include/media/videobuf2-core.h   | 25 +-
>>  3 files changed, 104 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
>> b/drivers/media/v4l2-core/videobuf2-core.c
>> index cb115ba6a1d2..c01038b7962a 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>> vb2_buffer_state state)
>>   state != VB2_BUF_STATE_REQUEUEING))
>>   state = VB2_BUF_STATE_ERROR;
>>
>> + WARN_ON(vb->request != q->cur_req);
>
> What's the reason for this WARN_ON? It's not immediately obvious to me.

This is a safeguard against driver bugs: a buffer should not complete
unless it is part of the request being currently processed.

>
>> +
>>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>>   /*
>>* Although this is not a callback, it still does have to balance
>> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>> vb2_buffer_state state)
>>   /* Add the buffer to the done buffers list */
>>   list_add_tail(>done_entry, >done_list);
>>   vb->state = state;
>> +
>> + if (q->cur_req) {
>> + WARN_ON(q->req_buf_cnt < 1);
>> +
>> + if (--q->req_buf_cnt == 0)
>> + q->cur_req = NULL;
>> + }
>>   }
>>   atomic_dec(>owned_by_drv_count);
>>   spin_unlock_irqrestore(>done_lock, flags);
>> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, 
>> unsigned int index, void *pb)
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>
>> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
>> +{
>> + struct vb2_buffer *vb;
>> +
>> + list_for_each_entry(vb, >queued_list, queued_entry) {
>> + if (vb->request == q->cur_req)
>> + __enqueue_in_driver(vb);
>> + }
>> +}
>
> I think this will clash big time with the v4l2 fence patch series...

Indeed, but on the other hand I was not a big fan of going through the
whole list. :) So I welcome the extra throttling introduced by the
fence series.

>
>> +
>>  /**
>>   * vb2_start_streaming() - Attempt to start streaming.
>>   * @q:   videobuf2 queue
>> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>* If any buffers were queued before streamon,
>>* we can now pass them to driver for processing.
>>*/
>> - list_for_each_entry(vb, >queued_list, queued_entry)
>> - __enqueue_in_driver(vb);
>> + vb2_queue_enqueue_current_buffers(q);
>>
>>   /* Tell the driver to start streaming */
>>   q->start_streaming_called = 1;
>> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>   return ret;
>>  }
>>
>> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
>> +   struct media_request *req, void *pb)
>>  {
>>   struct vb2_buffer *vb;
>>   int ret;
>> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>> index, void *pb)
>>   q->queued_count++;
>>   q->waiting_for_buffers = false;
>>   vb->state = VB2_BUF_STATE_QUEUED;
>> + vb->request = req;
>>
>>   if (pb)
>>   call_void_bufop(q, copy_timestamp, vb, pb);
>> @@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>> index, void *pb)
>>   /*
>>* If already streaming, give the buffer to driver for processing.
>>* If not, the buffer will be given to driver on next streamon.
>> +  *
>> +  * If using the request API, the buffer will be given to the driver
>> +  * when the request becomes active.
>>*/
>> - if (q->start_streaming_called)
>> + if (q->start_streaming_called && !req)
>>   __enqueue_in_driver(vb);
>>
>>   /* Fill buffer information for the userspace */
>> @@ -1427,6 +1450,28 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>> index, void *pb)
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>>
>> +void vb2_queue_start_request(struct vb2_queue *q, 

Re: [RFC PATCH 4/9] videodev2.h: Add request field to v4l2_buffer

[Alexandre Courbot]

On Fri, Jan 12, 2018 at 7:22 PM, Hans Verkuil  wrote:
> On 12/15/17 08:56, Alexandre Courbot wrote:
>> From: Hans Verkuil 
>>
>> When queuing buffers allow for passing the request ID that
>> should be associated with this buffer.
>>
>> Signed-off-by: Hans Verkuil 
>> [acour...@chromium.org: make request ID 32-bit]
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/usb/cpia2/cpia2_v4l.c   | 2 +-
>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 7 ---
>>  drivers/media/v4l2-core/v4l2-ioctl.c  | 4 ++--
>>  drivers/media/v4l2-core/videobuf2-v4l2.c  | 3 ++-
>>  include/media/videobuf2-v4l2.h| 2 ++
>>  include/uapi/linux/videodev2.h| 3 ++-
>>  6 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c 
>> b/drivers/media/usb/cpia2/cpia2_v4l.c
>> index 3dedd83f0b19..7217dde95a8a 100644
>> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
>> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
>> @@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, 
>> struct v4l2_buffer *buf)
>>   buf->sequence = cam->buffers[buf->index].seq;
>>   buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>>   buf->length = cam->frame_size;
>> - buf->reserved2 = 0;
>> + buf->request = 0;
>>   buf->reserved = 0;
>>   memset(>timecode, 0, sizeof(buf->timecode));
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c 
>> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index 821f2aa299ae..94f07c3b0b53 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -370,7 +370,7 @@ struct v4l2_buffer32 {
>>   __s32   fd;
>>   } m;
>>   __u32   length;
>> - __u32   reserved2;
>> + __u32   request;
>>   __u32   reserved;
>>  };
>>
>> @@ -438,7 +438,8 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, 
>> struct v4l2_buffer32 __user
>>   get_user(kp->type, >type) ||
>>   get_user(kp->flags, >flags) ||
>>   get_user(kp->memory, >memory) ||
>> - get_user(kp->length, >length))
>> + get_user(kp->length, >length) ||
>> + get_user(kp->request, >request))
>>   return -EFAULT;
>>
>>   if (V4L2_TYPE_IS_OUTPUT(kp->type))
>> @@ -533,7 +534,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, 
>> struct v4l2_buffer32 __user
>>   put_user(kp->timestamp.tv_usec, >timestamp.tv_usec) ||
>>   copy_to_user(>timecode, >timecode, sizeof(struct 
>> v4l2_timecode)) ||
>>   put_user(kp->sequence, >sequence) ||
>> - put_user(kp->reserved2, >reserved2) ||
>> + put_user(kp->request, >request) ||
>>   put_user(kp->reserved, >reserved) ||
>>   put_user(kp->length, >length))
>>   return -EFAULT;
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index ec4ecd5aa8bf..8d041247e97f 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -437,13 +437,13 @@ static void v4l_print_buffer(const void *arg, bool 
>> write_only)
>>   const struct v4l2_plane *plane;
>>   int i;
>>
>> - pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, flags=0x%08x, 
>> field=%s, sequence=%d, memory=%s",
>> + pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request=%u, 
>> flags=0x%08x, field=%s, sequence=%d, memory=%s",
>>   p->timestamp.tv_sec / 3600,
>>   (int)(p->timestamp.tv_sec / 60) % 60,
>>   (int)(p->timestamp.tv_sec % 60),
>>   (long)p->timestamp.tv_usec,
>>   p->index,
>> - prt_names(p->type, v4l2_type_names),
>> + prt_names(p->type, v4l2_type_names), p->request,
>>   p->flags, prt_names(p->field, v4l2_field_names),
>>   p->sequence, prt_names(p->memory, v4l2_memory_names));
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c 
>> b/drivers/media/v4l2-core/videobuf2-v4l2.c
>> index 0c0669976bdc..bde7b8a3a303 100644
>> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
>> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
>> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, 
>> void *pb)
>>   b->timestamp = ns_to_timeval(vb->timestamp);
>>   b->timecode = vbuf->timecode;
>>   b->sequence = vbuf->sequence;
>> - b->reserved2 = 0;
>> + b->request = vbuf->request;
>>   b->reserved = 0;
>>
>>   if (q->is_multiplanar) {
>> @@ -320,6 +320,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
>>   }
>>   vb->timestamp = 0;
>>

Re: [RFC PATCH 0/9] media: base request API support

[Alexandre Courbot]

Hi Hans,

On Fri, Jan 12, 2018 at 8:45 PM, Hans Verkuil  wrote:
> Hi Alexandre,
>
> On 12/15/17 08:56, Alexandre Courbot wrote:
>> Here is a new attempt at the request API, following the UAPI we agreed on in
>> Prague. Hopefully this can be used as the basis to move forward.
>>
>> This series only introduces the very basics of how requests work: allocate a
>> request, queue buffers to it, queue the request itself, wait for it to 
>> complete,
>> reuse it. It does *not* yet use Hans' work with controls setting. I have
>> preferred to submit it this way for now as it allows us to concentrate on the
>> basic request/buffer flow, which was harder to get properly than I initially
>> thought. I still have a gut feeling that it can be improved, with less 
>> back-and-
>> forth into drivers.
>>
>> Plugging in controls support should not be too hard a task (basically just 
>> apply
>> the saved controls when the request starts), and I am looking at it now.
>>
>> The resulting vim2m driver can be successfully used with requests, and my 
>> tests
>> so far have been successful.
>>
>> There are still some rougher edges:
>>
>> * locking is currently quite coarse-grained
>> * too many #ifdef CONFIG_MEDIA_CONTROLLER in the code, as the request API
>>   depends on it - I plan to craft the headers so that it becomes unnecessary.
>>   As it is, some of the code will probably not even compile if
>>   CONFIG_MEDIA_CONTROLLER is not set
>>
>> But all in all I think the request flow should be clear and easy to review, 
>> and
>> the possibility of custom queue and entity support implementations should 
>> give
>> us the flexibility we need to support more specific use-cases (I expect the
>> generic implementations to be sufficient most of the time though).
>>
>> A very simple test program exercising this API is available here (don't 
>> forget
>> to adapt the /dev/media0 hardcoding):
>> https://gist.github.com/Gnurou/dbc3776ed97ea7d4ce6041ea15eb0438
>>
>> Looking forward to your feedback and comments!
>
> I think this will become more interesting when the control code is in.

Definitely.

> The main thing I've noticed with this patch series is that it is very codec
> oriented. Which in some ways is OK (after all, that's the first type of HW
> that we want to support), but the vb2 code in particular should be more
> generic.

I don't want to expand too much into use-cases I do not master ; doing
so would be speculating about how the API will be used. But feel free
to point out where you think my focus on the codec use-case is not
future-proof.

> I would also recommend that you start preparing documentation patches: we
> can review that and make sure all the corner-cases are correctly documented.
>
> The public API changes are (I think) fairly limited, but the devil is in
> the details, so getting that reviewed early on will help you later.

Yeah, I now regret to have submitted this series without
documentation. Won't do that mistake again.

> It's a bit unfortunate that the fence patch series is also making vb2 changes,
> but I hope that will be merged fairly soon so you can develop on top of that
> series.

The fence series may actually make things easier. The vb2 code of this
series is a bit confusing, and fences add a few extra constraints that
should make things more predictable. So I am looking forward to being
able to work on top of it.

Re: [RFC PATCH 6/9] media: vb2: add support for requests in QBUF ioctl

[Alexandre Courbot]

On Fri, Jan 12, 2018 at 8:37 PM, Hans Verkuil  wrote:
> On 12/15/17 08:56, Alexandre Courbot wrote:
>> Support the request argument of the QBUF ioctl.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 93 
>> +++-
>>  1 file changed, 92 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 8d041247e97f..28f9c368563e 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -29,6 +29,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>
>> @@ -965,6 +966,81 @@ static int check_fmt(struct file *file, enum 
>> v4l2_buf_type type)
>>   return -EINVAL;
>>  }
>>
>> +/*
>> + * Validate that a given request can be used during an ioctl.
>> + *
>> + * When using the request API, request file descriptors must be matched 
>> against
>> + * the actual request object. User-space can pass any file descriptor, so we
>> + * need to make sure the call is valid before going further.
>> + *
>> + * This function looks up the request and associated data and performs the
>> + * following sanity checks:
>> + *
>> + * * Make sure that the entity supports requests,
>> + * * Make sure that the entity belongs to the media_device managing the 
>> passed
>> + *   request,
>> + * * Make sure that the entity data (if any) is associated to the current 
>> file
>> + *   handler.
>> + *
>> + * This function returns a pointer to the valid request, or and error code 
>> in
>> + * case of failure. When successful, a reference to the request is acquired 
>> and
>> + * must be properly released.
>> + */
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +static struct media_request *
>> +check_request(int request, struct file *file, void *fh)
>> +{
>> + struct media_request *req = NULL;
>> + struct video_device *vfd = video_devdata(file);
>> + struct v4l2_fh *vfh =
>> + test_bit(V4L2_FL_USES_V4L2_FH, >flags) ? fh : NULL;
>> + struct media_entity *entity = >entity;
>> + const struct media_entity *ent;
>> + struct media_request_entity_data *data;
>> + bool found = false;
>> +
>> + if (!entity)
>> + return ERR_PTR(-EINVAL);
>> +
>> + /* Check that the entity supports requests */
>> + if (!entity->req_ops)
>> + return ERR_PTR(-ENOTSUPP);
>> +
>> + req = media_request_get_from_fd(request);
>
> You can get the media_device from vfd->v4l2_dev->mdev. So it is much easier
> to just pass the media_device as an argument to media_request_get_from_fd()...
>
>> + if (!req)
>> + return ERR_PTR(-EINVAL);
>> +
>> + /* Validate that the entity belongs to the media_device managing
>> +  * the request queue */
>> + media_device_for_each_entity(ent, req->queue->mdev) {
>> + if (entity == ent) {
>> + found = true;
>> + break;
>> + }
>> + }
>> + if (!found) {
>> + media_request_put(req);
>> + return ERR_PTR(-EINVAL);
>> + }
>
> ...and then you don't need to do this ^^^ extra validation check.

Ah right, all you need to do is check that req->queue->mdev ==
vfd->v4l2_dev->mdev and you can get rid of this whole block. I don't
think we can do that in media_request_get_from_fd() though, since it
is called from other places where (IIUC) we don't have access to the
media_device.

>
>> +
>> + /* Validate that the entity's data belongs to the correct fh */
>> + data = media_request_get_entity_data(req, entity, vfh);
>> + if (IS_ERR(data)) {
>> + media_request_put(req);
>> + return ERR_PTR(PTR_ERR(data));
>> + }
>
> This assumes that each filehandle has its own state. That's true for codecs,
> but not for most (all?) other devices. There the state is per device instance.
>
> I'm not sure if we have a unique identifying mark for such drivers. The 
> closest
> is checking if fh->m2m_ctx is non-NULL, but I don't know if all drivers with
> per-filehandle state use that field. An alternative might be to check if
> fh->ctrl_handler is non-NULL. But again, I'm not sure if that's a 100% valid
> check.

I think the current code already takes that case into account: if the
device does not uses v4l2_fh, then the fh argument passed to
media_request_get_entity_data() will be null, and so will be data->fh.

[PATCH] media: staging/imx: Checking the right variable in vdic_get_ipu_resources()

[Dan Carpenter]

We recently changed this error handling around but missed this error
pointer check.  We're testing "priv->vdi_in_ch_n" instead of "ch" so the
error handling can't be triggered.

Fixes: 0b2e9e7947e7 ("media: staging/imx: remove confusing IS_ERR_OR_NULL 
usage")
Signed-off-by: Dan Carpenter 

diff --git a/drivers/staging/media/imx/imx-media-vdic.c 
b/drivers/staging/media/imx/imx-media-vdic.c
index 433474d58e3e..ed356844cdf6 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -177,7 +177,7 @@ static int vdic_get_ipu_resources(struct vdic_priv *priv)
priv->vdi_in_ch = ch;
 
ch = ipu_idmac_get(priv->ipu, IPUV3_CHANNEL_MEM_VDI_NEXT);
-   if (IS_ERR(priv->vdi_in_ch_n)) {
+   if (IS_ERR(ch)) {
err_chan = IPUV3_CHANNEL_MEM_VDI_NEXT;
ret = PTR_ERR(ch);
goto out_err_chan;