Re: [PATCH v2] scripts/gdb: add constants.py to .gitignore
On 21/06/16 22:11, Omar Sandoval wrote:
> From: Omar Sandoval
>
> Since scripts/gdb/linux/constants.py is autogenerated, this should have
> been added to .gitignore when it was introduced.
>
> Fixes: f197d75fcad1 ("scripts/gdb: provide linux constants")
> Signed-off-by: Omar Sandoval
> ---
> Thanks, Kieran, I totally missed the comment in top-level .gitignore.
>
> scripts/gdb/linux/.gitignore | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/scripts/gdb/linux/.gitignore b/scripts/gdb/linux/.gitignore
> index 52e4e61140d1..d5abd6ce9425 100644
> --- a/scripts/gdb/linux/.gitignore
> +++ b/scripts/gdb/linux/.gitignore
> @@ -1,2 +1,3 @@
> *.pyc
> *.pyo
> +/constants.py
Does this work with '/' at the beginning of the line?
It looks like all the other sub-tree .gitignores don't use a leading /
on their ignores
If you're happy, I'd like to drop the leading '/'.
(I think I can do this silently as I pick the patch if you approve,
rather than send another patch for a one-byte change)
Other than that:
Signed-off-by: Kieran Bingham
--
Regards
Kieran Bingham
Re: [PATCH] scripts/gdb: add constants.py to .gitignore
Hi Omar,
Thanks for spotting, and submitting this.
On 20/06/16 21:00, Omar Sandoval wrote:
> From: Omar Sandoval
>
> Since scripts/gdb/linux/constants.py is autogenerated, this should have
> been added to .gitignore when it was introduced.
Absolutely! - This never hit me, as I always build out-of-tree
> Fixes: f197d75fcad1 ("scripts/gdb: provide linux constants")
> Signed-off-by: Omar Sandoval
Unfortunately - Nacked on the principle of the comment in the top of the
.gitignore file:
# NOTE! Don't add files that are generated in specific
# subdirectories here. Add them in the ".gitignore" file
# in that subdirectory instead.
Could you fix this up (meaning add to the .gitignore under
scripts/gdb/linux) and resend please?
Then I'll add to my queue to send as fixes for current.
(which I'm hoping to get sent before the weekend, or at the weekend)
> ---
> .gitignore | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/.gitignore b/.gitignore
> index 0c320bf02586..5cdccb07e3a3 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -112,3 +112,6 @@ all.config
>
> # Kdevelop4
> *.kdev4
> +
> +# Generated for CONFIG_GDB_SCRIPTS
> +/scripts/gdb/linux/constants.py
>
--
Regards
Kieran Bingham
Re: [PATCH v2] scripts/gdb: add constants.py to .gitignore
On 21/06/16 22:34, Omar Sandoval wrote:
> $ git status
> On branch master
> Untracked files:
> (use "git add ..." to include in what will be committed)
>
> asdf
>
> nothing added to commit but untracked files present (use "git add" to track)
>
>
> tools/thermal/tmon/.gitignore also uses the leading slash.
Aha yes,
find -name .gitignore -exec grep -Hin '^/' {} \;
./tools/thermal/tmon/.gitignore:1:/tmon
./.gitignore:44:/tags
./.gitignore:45:/TAGS
./.gitignore:46:/linux
./.gitignore:47:/vmlinux
./.gitignore:48:/vmlinux.32
./.gitignore:49:/vmlinux-gdb.py
./.gitignore:50:/vmlinuz
./.gitignore:51:/System.map
./.gitignore:52:/Module.markers
./.gitignore:57:/debian/
./.gitignore:62:/tar-install/
I think we'll call this the exception rather than the norm, and I'd
prefer to match the other 177 .gitignore files in their style.
>
> If you want to drop the /, I'm okay with it.
Thanks for the testing, and checking.
Patch picked, and '/' silently dropped.
--
Regards
Kieran Bingham
Re: [PATCH 7/7] [media] v4l: rcar_fdp1: use proper name for the R-Car SoC
Hi Wolfram Thankyou for the fixup On 28/05/17 18:30, Wolfram Sang wrote: > It is 'R-Car', not 'RCar'. No code or binding changes, only descriptive text. > > Signed-off-by: Wolfram Sang Acked-by: Kieran Bingham Mauro, I'll leave this for you to pick up when you're ready. Thanks Kieran > --- > I suggest this trivial patch should be picked individually per susbsystem. > > drivers/media/platform/rcar_fdp1.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/rcar_fdp1.c > b/drivers/media/platform/rcar_fdp1.c > index 42f25d241edd7c..0da0eba9202cdd 100644 > --- a/drivers/media/platform/rcar_fdp1.c > +++ b/drivers/media/platform/rcar_fdp1.c > @@ -1,5 +1,5 @@ > /* > - * Renesas RCar Fine Display Processor > + * Renesas R-Car Fine Display Processor > * > * Video format converter and frame deinterlacer device. > * >
Re: [PATCH] i2c: return probe deferred status on dev_pm_domain_attach
Hi Wolfram,
On 12 October 2015 at 08:23, Wolfram Sang wrote:
> Hi Kiera,
>
> On Sun, Oct 11, 2015 at 12:39:31PM +0100, Kieran Bingham wrote:
>> A change of return status was introduced in commit 3fffd1283927
>> ("i2c: allow specifying separate wakeup interrupt in device tree")
>
> Thanks for catching this!
NP : Saw it when re-basing my other series.
>> The commit prevents the defer status being passed up the call stack
>> appropriately when dev_pm_domain_attach returns -EPROBE_DEFER.
>>
>> To fix we change this back to the original return status;
>
> What about going to the error path?
Sounds reasonable :D
>> Signed-off-by: Kieran Bingham
>
> Please add a "Fixes: " tag here.
Sure.
>
>Wolfram
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i2c: return probe deferred status on dev_pm_domain_attach
On 12 October 2015 at 08:24, Wolfram Sang wrote: > > And add the original patch author to CC when resending. > Bah - I was sure I'd added him ... must have slipped. Should I add Cc: stable? or are they notified from the Fixes: tag? -- Regards Kieran -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing
Hi Wolfram, On 9 October 2015 at 22:16, Wolfram Sang wrote: > > As said to Kieran personally in Dublin, I want a verification that all > binding methods still work, especially runtime instantiation for drivers > without i2c_device_ids. Ok, I should be able to find some time to look at that this week. > Also, for the last patch, a verification should > be done if the drivers i2c_device_id hasn't been used meanwhile. I'll see what I can do ... > I'd also like to see 'probe_new' instead of 'probe2' for the new function > name. That should be it. Ok, obviously this is only a temporary naming so I don't mind either way, I'll do a rename for the next version I've also just found a compile failure to fix up on !CONFIG_OF, this can make its way into the respin. > > Thanks, > >Wolfram Regards Kieran -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] i2c: return probe deferred status on dev_pm_domain_attach
A change of return status was introduced in commit 3fffd1283927
("i2c: allow specifying separate wakeup interrupt in device tree")
The commit prevents the defer status being passed up the call stack
appropriately when dev_pm_domain_attach returns -EPROBE_DEFER.
Catch the PROBE_DEFER and clear up the IRQ wakeup status
Signed-off-by: Kieran Bingham
Fixes: 3fffd1283927 ("i2c: allow specifying separate wakeup
interrupt in device tree")
---
V2:
Fall through the error path before returning -EPROBE_DEFER
drivers/i2c/i2c-core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 5f89f1e3c2f2..51f20cce1732 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -699,6 +699,8 @@ static int i2c_device_probe(struct device *dev)
client));
if (status)
goto err_detach_pm_domain;
+ } else {
+ goto err_clear_wakeup_irq;
}
return 0;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i2c: return probe deferred status on dev_pm_domain_attach
On 12 October 2015 at 20:40, Wolfram Sang wrote: > >> Should I add Cc: stable? or are they notified from the Fixes: tag? > > My preference is: I'll add stable when I commit to my tree. I am happy > if people tell me when they think this should be done. > > In this case, it should :) > Agreed :) I'm happy to follow your process and preference and let you add. - Patch v2 posted. -- Regards Kieran -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] i2c: return probe deferred status on dev_pm_domain_attach
On 12 October 2015 at 21:17, Dmitry Torokhov wrote:
> On Mon, Oct 12, 2015 at 12:49 PM, Kieran Bingham
> wrote:
>> A change of return status was introduced in commit 3fffd1283927
>> ("i2c: allow specifying separate wakeup interrupt in device tree")
>>
>> The commit prevents the defer status being passed up the call stack
>> appropriately when dev_pm_domain_attach returns -EPROBE_DEFER.
>
> Ah, indeed, sorry about that.
>
>>
>> Catch the PROBE_DEFER and clear up the IRQ wakeup status
>>
>> Signed-off-by: Kieran Bingham
>> Fixes: 3fffd1283927 ("i2c: allow specifying separate wakeup
>> interrupt in device tree")
>>
>> ---
>> V2:
>> Fall through the error path before returning -EPROBE_DEFER
>>
>> drivers/i2c/i2c-core.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 5f89f1e3c2f2..51f20cce1732 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -699,6 +699,8 @@ static int i2c_device_probe(struct device *dev)
>> client));
>> if (status)
>> goto err_detach_pm_domain;
>> + } else {
>> + goto err_clear_wakeup_irq;
>> }
>
> How about we do:
>
> status = dev_pm_domain_attach(&client->dev, true);
> if (status == -EPROBE_DEFER)
> goto err_clear_wakeup_irq;
>
> status = driver->probe(client, i2c_match_id(driver->id_table,
> client));
>
>
> instead of doing "else"?
Well in fact - this is actually what I do in the next set of patches
I'm working on which led me to find this fault.
(i2c: Provide a temporary .probe2() call-back type)
And so ... v3 ... so much for my quick one word bugfix patch :)
I'll send v3 with the if/else removed
--
Kieran
>
> Thanks.
>
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] i2c: return probe deferred status on dev_pm_domain_attach
A change of return status was introduced in commit 3fffd1283927
("i2c: allow specifying separate wakeup interrupt in device tree")
The commit prevents the defer status being passed up the call stack
appropriately when dev_pm_domain_attach returns -EPROBE_DEFER.
Catch the PROBE_DEFER and clear up the IRQ wakeup status
Signed-off-by: Kieran Bingham
Fixes: 3fffd1283927 ("i2c: allow specifying separate wakeup
interrupt in device tree")
---
V2:
Fall through the error path before returning -EPROBE_DEFER
V3:
Drop the if/else for cleaner code path
drivers/i2c/i2c-core.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 5f89f1e3c2f2..a59c3111f7fb 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -694,12 +694,12 @@ static int i2c_device_probe(struct device *dev)
goto err_clear_wakeup_irq;
status = dev_pm_domain_attach(&client->dev, true);
- if (status != -EPROBE_DEFER) {
- status = driver->probe(client, i2c_match_id(driver->id_table,
- client));
- if (status)
- goto err_detach_pm_domain;
- }
+ if (status == -EPROBE_DEFER)
+ goto err_clear_wakeup_irq;
+
+ status = driver->probe(client, i2c_match_id(driver->id_table, client));
+ if (status)
+ goto err_detach_pm_domain;
return 0;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing
Hi Wolfram,
I have picked this patchset [0] up from Lee to rebase it, with an aim to
get this series moving again.
A couple of minor issues were resolved in the rebase. As it stood, Javier
proposed [1] to merge this series, and use a follow up series to make sure
that all I2C drivers are using a MODLE_DEVICE_TABLE(of,...)
I have prepared a Coccinelle patch to work through the bulk of the changes
required for the conversion, which will assist the transition process.
[0] https://lkml.org/lkml/2014/8/28/283
[1] https://lkml.org/lkml/2014/9/12/496
Lee's most recent cover-letter (from 12 months ago) follows:
Hi Wolfram,
Placing this firmly back on your plate. I truly hope we don't miss
another merge-window. This patch-set has the support of some pretty
senior kernel maintainers, so I hope acceptance shouldn't be too
difficult.
As previously discussed I believe it should be okay for an I2C device
driver _not_ supply an I2C ID table to match to. The I2C subsystem
should be able to match via other means, such as via OF tables. The
blocking factor during our previous conversation was to keep
registering via sysfs up and running. This set does that.
After thinking more deeply about the problem, it occurred to me that
any I2C device driver which uses the sysfs method and issues an
of_match_device() would also fail their probe(). Bolted on to this
set is a new, more generic way for these devices to match against
either of the I2C/OF tables.
I hope this ticks all of your boxes.
v4: [Kieran Bingham]
- Rebase to v4.2
- adapt for dev_pm_domain_{attach,detach}
- strnicmp to strncasecmp
v3: [Lee Jones]
- Insist on passing 'struct i2c_client' instead of 'struct device'
- Remove hook from of_match_device()
v2: [Lee Jones]
- Removal of ACPI support (this is really an OF issue).
- Add a new .probe2( with will seamlessly replace
- Supply a warning on devices matching via OF without a suitable compatible
- Remove unified match_device() - bad idea as it subverts type-safe behaviour
- Provide examples of the kind of clean-up possible after this set.
- I already have the full support from the maintainer of these drivers =;-)
Lee Jones (8):
i2c: Add pointer dereference protection to i2c_match_id()
i2c: Add the ability to match device to compatible string without an
of_node
i2c: Match using traditional OF methods, then by vendor-less
compatible strings
i2c: Make I2C ID tables non-mandatory for DT'ed devices
i2c: Export i2c_match_id() for direct use by device drivers
i2c: Provide a temporary .probe2() call-back type
mfd: 88pm860x: Move over to new I2C device .probe() call
mfd: as3722: Rid driver of superfluous I2C device ID structure
drivers/i2c/i2c-core.c | 82 +
drivers/mfd/88pm860x-core.c | 5 ++-
drivers/mfd/as3722.c| 12 ++-
include/linux/i2c.h | 22 +++-
4 files changed, 93 insertions(+), 28 deletions(-)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 5/8] i2c: Export i2c_match_id() for direct use by device drivers
From: Lee Jones
When there was no other way to match a I2C device to driver i2c_match_id()
was exclusively used. However, now there are other types of tables which
are commonly supplied, matching on an i2c_device_id table is used less
frequently. Instead of _always_ calling i2c_match_id() from within the
framework, we only need to do so from drivers which have no other way of
matching. This patch makes i2c_match_id() available to the aforementioned
device drivers.
Acked-by: Grant Likely
Signed-off-by: Lee Jones
---
drivers/i2c/i2c-core.c | 3 ++-
include/linux/i2c.h| 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2ebc64d..0e40136 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -447,7 +447,7 @@ static inline int acpi_i2c_install_space_handler(struct
i2c_adapter *adapter)
/* - */
-static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
+const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
const struct i2c_client *client)
{
if (!(id && client))
@@ -460,6 +460,7 @@ static const struct i2c_device_id *i2c_match_id(const
struct i2c_device_id *id,
}
return NULL;
}
+EXPORT_SYMBOL_GPL(i2c_match_id);
static int i2c_device_match(struct device *dev, struct device_driver *drv)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 48bbbab..126585c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -232,6 +232,8 @@ struct i2c_client {
extern struct i2c_client *i2c_verify_client(struct device *dev);
extern struct i2c_adapter *i2c_verify_adapter(struct device *dev);
+extern const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
+ const struct i2c_client *client);
static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
{
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 2/8] i2c: Add the ability to match device to compatible string without an of_node
From: Lee Jones
A great deal of I2C devices are currently matched via DT node name, and
as such the compatible naming convention of ',' has gone
somewhat awry - some nodes don't supply one, some supply an arbitrary
string and others the correct device name with an arbitrary vendor prefix.
In an effort to correct this problem we have to supply a mechanism to
match a device by compatible string AND by simple device name. This
function strips off the ',' part of a supplied compatible string
and attempts to match without it.
The plan is to remove this function once all of the compatible strings
for each device have been brought into line.
Acked-by: Grant Likely
Signed-off-by: Lee Jones
[Kieran: strnicmp to strncasecmp]
Signed-off-by: Kieran Bingham
---
drivers/i2c/i2c-core.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 30d8a77..0788c1f 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1373,6 +1373,27 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct
device_node *node)
return adapter;
}
EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
+
+static const struct of_device_id*
+i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
+ struct i2c_client *client)
+{
+ const char *name;
+
+ for (; matches->compatible[0]; matches++) {
+ name = strchr(matches->compatible, ',');
+ if (!name)
+ name = matches->compatible;
+ else
+ name++;
+
+ if (!strncasecmp(client->name, name, strlen(client->name)))
+ return matches;
+ }
+
+ return NULL;
+}
+
#else
static void of_i2c_register_devices(struct i2c_adapter *adap) { }
#endif /* CONFIG_OF */
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 8/8] mfd: as3722: Rid driver of superfluous I2C device ID structure
From: Lee Jones
Also remove unused second probe() parameter 'i2c_device_id'.
Acked-by: Grant Likely
Signed-off-by: Lee Jones
---
drivers/mfd/as3722.c | 12 ++--
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/mfd/as3722.c b/drivers/mfd/as3722.c
index 924ea90..6ac4be3 100644
--- a/drivers/mfd/as3722.c
+++ b/drivers/mfd/as3722.c
@@ -354,8 +354,7 @@ static int as3722_i2c_of_probe(struct i2c_client *i2c,
return 0;
}
-static int as3722_i2c_probe(struct i2c_client *i2c,
- const struct i2c_device_id *id)
+static int as3722_i2c_probe(struct i2c_client *i2c)
{
struct as3722 *as3722;
unsigned long irq_flags;
@@ -428,20 +427,13 @@ static const struct of_device_id as3722_of_match[] = {
};
MODULE_DEVICE_TABLE(of, as3722_of_match);
-static const struct i2c_device_id as3722_i2c_id[] = {
- { "as3722", 0 },
- {},
-};
-MODULE_DEVICE_TABLE(i2c, as3722_i2c_id);
-
static struct i2c_driver as3722_i2c_driver = {
.driver = {
.name = "as3722",
.of_match_table = as3722_of_match,
},
- .probe = as3722_i2c_probe,
+ .probe2 = as3722_i2c_probe,
.remove = as3722_i2c_remove,
- .id_table = as3722_i2c_id,
};
module_i2c_driver(as3722_i2c_driver);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 3/8] i2c: Match using traditional OF methods, then by vendor-less compatible strings
From: Lee Jones
This function provides a single call for all I2C devices which need to
match firstly using traditional OF means i.e by of_node, then if that
fails we attempt to match using the supplied I2C client name with a
list of supplied compatible strings with the ',' string
removed. The latter is required due to the unruly naming conventions
used currently by I2C devices.
Acked-by: Grant Likely
Signed-off-by: Lee Jones
---
drivers/i2c/i2c-core.c | 16
include/linux/i2c.h| 12
2 files changed, 28 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 0788c1f..94ca76e 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1394,6 +1394,22 @@ i2c_of_match_device_strip_vendor(const struct
of_device_id *matches,
return NULL;
}
+const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches,
+struct i2c_client *client)
+{
+ const struct of_device_id *match;
+
+ if (!(client && matches))
+ return NULL;
+
+ match = of_match_device(matches, &client->dev);
+ if (match)
+ return match;
+
+ return i2c_of_match_device_strip_vendor(matches, client);
+}
+EXPORT_SYMBOL_GPL(i2c_of_match_device);
#else
static void of_i2c_register_devices(struct i2c_adapter *adap) { }
#endif /* CONFIG_OF */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index e83a738..48bbbab 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -638,6 +638,10 @@ extern struct i2c_client
*of_find_i2c_device_by_node(struct device_node *node);
/* must call put_device() when done with returned i2c_adapter device */
extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
*node);
+extern const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches,
+struct i2c_client *client);
+
#else
static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node
*node)
@@ -649,6 +653,14 @@ static inline struct i2c_adapter
*of_find_i2c_adapter_by_node(struct device_node
{
return NULL;
}
+
+const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches,
+struct i2c_client *client)
+{
+ return NULL;
+}
+
#endif /* CONFIG_OF */
#endif /* _LINUX_I2C_H */
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 7/8] mfd: 88pm860x: Move over to new I2C device .probe() call
From: Lee Jones
As part of an effort to rid the mostly unused second parameter for I2C
related .probe() functions and to conform to other existing frameworks
we're moving over to a temporary replacement .probe() call-back.
Acked-by: Grant Likely
Signed-off-by: Lee Jones
---
drivers/mfd/88pm860x-core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c
index 3269a99..a499d70 100644
--- a/drivers/mfd/88pm860x-core.c
+++ b/drivers/mfd/88pm860x-core.c
@@ -1130,8 +1130,7 @@ static int pm860x_dt_init(struct device_node *np,
return 0;
}
-static int pm860x_probe(struct i2c_client *client,
- const struct i2c_device_id *id)
+static int pm860x_probe(struct i2c_client *client)
{
struct pm860x_platform_data *pdata = dev_get_platdata(&client->dev);
struct device_node *node = client->dev.of_node;
@@ -1257,7 +1256,7 @@ static struct i2c_driver pm860x_driver = {
.pm = &pm860x_pm_ops,
.of_match_table = pm860x_dt_ids,
},
- .probe = pm860x_probe,
+ .probe2 = pm860x_probe,
.remove = pm860x_remove,
.id_table = pm860x_id_table,
};
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 6/8] i2c: Provide a temporary .probe2() call-back type
From: Lee Jones
This will aid the seamless removal of the current probe()'s, more
commonly unused than used second parameter. Most I2C drivers can
simply switch over to the new interface, others which have DT
support can use its own matching instead and others can call
i2c_match_id() themselves. This brings I2C's device probe method
into line with other similar interfaces in the kernel and prevents
the requirement to pass an i2c_device_id table.
Suggested-by: Grant Likely
Signed-off-by: Lee Jones
[Kieran: fix rebase conflicts and adapt for dev_pm_domain_{attach,detach}]
Signed-off-by: Kieran Bingham
---
drivers/i2c/i2c-core.c | 22 ++
include/linux/i2c.h| 8 +++-
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 0e40136..a28b423 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -658,8 +658,6 @@ static int i2c_device_probe(struct device *dev)
}
driver = to_i2c_driver(dev->driver);
- if (!driver->probe)
- return -EINVAL;
/*
* An I2C ID table is not mandatory, if and only if, a suitable Device
@@ -679,12 +677,20 @@ static int i2c_device_probe(struct device *dev)
return status;
status = dev_pm_domain_attach(&client->dev, true);
- if (status != -EPROBE_DEFER) {
- status = driver->probe(client, i2c_match_id(driver->id_table,
- client));
- if (status)
- dev_pm_domain_detach(&client->dev, true);
- }
+ if (status == -EPROBE_DEFER)
+ return status;
+
+ /* When there are no more users of probe(), rename probe2 to probe. */
+ if (driver->probe2)
+ status = driver->probe2(client);
+ else if (driver->probe)
+ status = driver->probe(client,
+ i2c_match_id(driver->id_table, client));
+ else
+ status = -EINVAL;
+
+ if (status)
+ dev_pm_domain_detach(&client->dev, true);
return status;
}
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 126585c..cb25ae0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -127,7 +127,8 @@ extern s32 i2c_smbus_write_i2c_block_data(const struct
i2c_client *client,
* struct i2c_driver - represent an I2C device driver
* @class: What kind of i2c device we instantiate (for detect)
* @attach_adapter: Callback for bus addition (deprecated)
- * @probe: Callback for device binding
+ * @probe: Callback for device binding - soon to be deprecated
+ * @probe2: New callback for device binding
* @remove: Callback for device unbinding
* @shutdown: Callback for device shutdown
* @alert: Alert callback, for example for the SMBus alert protocol
@@ -170,6 +171,11 @@ struct i2c_driver {
int (*probe)(struct i2c_client *, const struct i2c_device_id *);
int (*remove)(struct i2c_client *);
+ /* New driver model interface to aid the seamless removal of the
+* current probe()'s, more commonly unused than used second parameter.
+*/
+ int (*probe2)(struct i2c_client *);
+
/* driver model interfaces that don't relate to enumeration */
void (*shutdown)(struct i2c_client *);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 1/8] i2c: Add pointer dereference protection to i2c_match_id()
From: Lee Jones
Here we're providing dereference protection for i2c_match_id(), which
saves us having to do it each time it's called. We're also stripping
out the (now) needless checks in i2c_device_match(). This patch paves
the way for other, similar code trimming.
Acked-by: Grant Likely
Signed-off-by: Lee Jones
---
drivers/i2c/i2c-core.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c83e4d1..30d8a77 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -450,6 +450,9 @@ static inline int acpi_i2c_install_space_handler(struct
i2c_adapter *adapter)
static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
const struct i2c_client *client)
{
+ if (!(id && client))
+ return NULL;
+
while (id->name[0]) {
if (strcmp(client->name, id->name) == 0)
return id;
@@ -463,8 +466,6 @@ static int i2c_device_match(struct device *dev, struct
device_driver *drv)
struct i2c_client *client = i2c_verify_client(dev);
struct i2c_driver *driver;
- if (!client)
- return 0;
/* Attempt an OF style match */
if (of_driver_match_device(dev, drv))
@@ -475,9 +476,10 @@ static int i2c_device_match(struct device *dev, struct
device_driver *drv)
return 1;
driver = to_i2c_driver(drv);
- /* match on an id table if there is one */
- if (driver->id_table)
- return i2c_match_id(driver->id_table, client) != NULL;
+
+ /* Finally an I2C match */
+ if (i2c_match_id(driver->id_table, client))
+ return 1;
return 0;
}
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 4/8] i2c: Make I2C ID tables non-mandatory for DT'ed devices
From: Lee Jones Currently the I2C framework insists on devices supplying an I2C ID table. Many of the devices which do so unnecessarily adding quite a few wasted lines to kernel code. This patch allows drivers a means to 'not' supply the aforementioned table and match on DT match tables instead. Acked-by: Grant Likely Signed-off-by: Lee Jones --- drivers/i2c/i2c-core.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 94ca76e..2ebc64d 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -468,7 +468,7 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv) /* Attempt an OF style match */ - if (of_driver_match_device(dev, drv)) + if (i2c_of_match_device(drv->of_match_table, client)) return 1; /* Then ACPI style match */ @@ -657,7 +657,15 @@ static int i2c_device_probe(struct device *dev) } driver = to_i2c_driver(dev->driver); - if (!driver->probe || !driver->id_table) + if (!driver->probe) + return -EINVAL; + + /* +* An I2C ID table is not mandatory, if and only if, a suitable Device +* Tree match table entry is supplied for the probing device. +*/ + if (!driver->id_table && + !i2c_of_match_device(dev->driver->of_match_table, client)) return -ENODEV; if (!device_can_wakeup(&client->dev)) -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing
Hi Wolfram,
On 1 October 2015 at 21:50, Wolfram Sang wrote:
>
>> > Yes but that is not true for drivers that support both OF and legacy board
>> > files. For those drivers, there will be a lot of boiler plate code
>> > duplicated
>> > that would look something like:
>> >
>> > unsigned long data;
>> > struct of_device_id *match;
>> > struct i2c_devicd_id *id;
>> >
>> > if (i2c->dev.of_node) {
>> > match = i2c_of_match_device(of_match_table, i2c);
>> > if (!match)
>> >return -EINVAL;
>> >
>> > data = (unsigned long)match->data;
>> > } else {
>> > id = i2c_match_id(id_table, i2c);
>> > if (!id)
>> >return -EINVAL;
>> >
>> > data = id->driver_data;
>> > }
>
> I said this before: It is not only the additional code, I think it is
> quite unelegant to to do the matching again which has already been done.
> (and DT boottime has already increased, partly due to the excessive
> string matching). Also, I wouldn't like to see an I2C specific solution;
> this problem exists for other subsystems, too.
>
>> I'm fine with a new API for this stuff. I'm even happy to go ahead
>> and code it up, but it's important to note that this is work which
>> should be based on this set and not a blocker for this set to be
>> accepted.
>
> Is that a promise? :)
Well if Lee doesn't, then I'll be trying to take it on.
I've already written an spatch to help with the conversion of other
drivers to follow on for this series.
Between us I think we've got motivation to make progress.
>> The correct approach is the former. One of the aims of this set was
>> to bring the I2C .probe() call-back more into line with the majority
>> of the other .probe() calls in the kernel i.e. with only a single
>> parameter. I'm really not a fan of passing some random void pointer
>
> Yes, I like this about this series.
>
>> in. Using a look-up call to fetch ACPI/OF/I2C/etc data is the current
>> norm and is a very viable option.
>
> It is the status quo, but that doesn't make it better IMO.
>
>> Wolfram, please (finally :D) take this set.
>
> I tend to give in ;) Maybe we can talk in Dublin a bit about a possible
> next step after this series?
I don't think Lee is coming to Dublin, but I'll be there all week, if
you find time for a chat.
I'll look out for you in the hallway track, or at least on stage for
the final ELCE games :)
>
> Thanks,
>
>Wolfram
>
--
Regards
Kieran
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/4] MAINTAINERS: VSP1: Add co-maintainer
From: Kieran Bingham Add myself as a co-maintainer for the Renesas VSP driver. Signed-off-by: Kieran Bingham --- Cc: Laurent Pinchart Cc: linux-me...@vger.kernel.org Cc: linux-renesas-...@vger.kernel.org MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index c7cecb9201b3..6a30a5332b18 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8935,6 +8935,7 @@ F:drivers/media/platform/rcar-vin/ MEDIA DRIVERS FOR RENESAS - VSP1 M: Laurent Pinchart +M: Kieran Bingham L: linux-me...@vger.kernel.org L: linux-renesas-...@vger.kernel.org T: git git://linuxtv.org/media_tree.git -- 2.17.1
[PATCH 4/4] MAINTAINERS: GDB: Update e-mail address
From: Kieran Bingham This entry was created with my personal e-mail address. Update this entry to my open-source kernel.org account. Signed-off-by: Kieran Bingham --- Cc: Andrew Morton Cc: jan.kis...@siemens.com MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 174c0c3da67e..c25060e995b1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5931,7 +5931,7 @@ F:Documentation/dev-tools/gcov.rst GDB KERNEL DEBUGGING HELPER SCRIPTS M: Jan Kiszka -M: Kieran Bingham +M: Kieran Bingham S: Supported F: scripts/gdb/ -- 2.17.1
Re: [RFT PATCH v2 6/6] uvcvideo: Move decode processing to process context
Hi Kieran !
Thanking you for the patch might be rather self serving here :D
On 09/01/18 13:09, Kieran Bingham wrote:
> Newer high definition cameras, and cameras with multiple lenses such as
> the range of stereo-vision cameras now available have ever increasing
> data rates.
>
> The inclusion of a variable length packet header in URB packets mean
> that we must memcpy the frame data out to our destination 'manually'.
> This can result in data rates of up to 2 gigabits per second being
> processed.
>
> To improve efficiency, and maximise throughput, handle the URB decode
> processing through a work queue to move it from interrupt context, and
> allow multiple processors to work on URBs in parallel.
>
> Signed-off-by: Kieran Bingham
>
> ---
> v2:
> - Lock full critical section of usb_submit_urb()
>
> drivers/media/usb/uvc/uvc_queue.c | 12 +++-
> drivers/media/usb/uvc/uvc_video.c | 111 +--
> drivers/media/usb/uvc/uvcvideo.h | 24 +++-
> 3 files changed, 129 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c
> index 5a9987e547d3..598087eeb5c2 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -179,10 +179,22 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>
> + /* Prevent new buffers coming in. */
> + spin_lock_irq(&queue->irqlock);
> + queue->flags |= UVC_QUEUE_STOPPING;
> + spin_unlock_irq(&queue->irqlock);
> +
> + /*
> + * All pending work should be completed before disabling the stream, as
> + * all URBs will be free'd during uvc_video_enable(s, 0).
> + */
> + flush_workqueue(stream->async_wq);
> +
> uvc_video_enable(stream, 0);
>
> spin_lock_irq(&queue->irqlock);
> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> + queue->flags &= ~UVC_QUEUE_STOPPING;
> spin_unlock_irq(&queue->irqlock);
> }
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index 3878bec3276e..a9ddc4f27012 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1058,21 +1058,67 @@ static int uvc_video_decode_start(struct
> uvc_streaming *stream,
> return data[0];
> }
>
> -static void uvc_video_decode_data(struct uvc_streaming *stream,
> - struct uvc_buffer *buf, const __u8 *data, int len)
> +/*
> + * uvc_video_decode_data_work: Asynchronous memcpy processing
> + *
> + * Perform memcpy tasks in process context, with completion handlers
> + * to return the URB, and buffer handles.
> + *
> + * The work submitter must pre-determine that the work is safe
> + */
> +static void uvc_video_decode_data_work(struct work_struct *work)
This handles asynchronous memory copy work - which could equally be encode work
- so _decode_ might not be an appropriate name here.
> {
> - unsigned int maxlen, nbytes;
> - void *mem;
> + struct uvc_urb *uvc_urb = container_of(work, struct uvc_urb, work);
> + struct uvc_streaming *stream = uvc_urb->stream;
> + struct uvc_video_queue *queue = &stream->queue;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < uvc_urb->packets; i++) {
> + struct uvc_decode_op *op = &uvc_urb->decodes[i];
> +
> + memcpy(op->dst, op->src, op->len);
> +
> + /* Release reference taken on this buffer */
> + uvc_queue_buffer_release(op->buf);
> + }
> +
> + /*
> + * Prevent resubmitting URBs when shutting down to ensure that no new
> + * work item will be scheduled after uvc_stop_streaming() flushes the
> + * work queue.
> + */
> + spin_lock_irq(&queue->irqlock);
> + if (!(queue->flags & UVC_QUEUE_STOPPING)) {
> + ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
> + if (ret < 0)
> + uvc_printk(KERN_ERR,
> +"Failed to resubmit video URB (%d).\n",
> +ret);
> + }
> + spin_unlock_irq(&queue->irqlock);
> +}
> +
> +static void uvc_video_decode_data(struct uvc_decode_op *decode,
> + struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
> + const __u8 *data, int len)
> +{
> + unsigned int maxlen;
>
> if (len <= 0)
> return;
>
[RFT PATCH v3 5/6] uvcvideo: queue: Support asynchronous buffer handling
The buffer queue interface currently operates sequentially, processing
buffers after they have fully completed.
In preparation for supporting parallel tasks operating on the buffers,
we will need to support buffers being processed on multiple CPUs.
Adapt the uvc_queue_next_buffer() such that a reference count tracks the
active use of the buffer, returning the buffer to the VB2 stack at
completion.
Signed-off-by: Kieran Bingham
---
drivers/media/usb/uvc/uvc_queue.c | 61 ++--
drivers/media/usb/uvc/uvcvideo.h | 4 ++-
2 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_queue.c
b/drivers/media/usb/uvc/uvc_queue.c
index ddac4d89a291..5a9987e547d3 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -131,6 +131,7 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
spin_lock_irqsave(&queue->irqlock, flags);
if (likely(!(queue->flags & UVC_QUEUE_DISCONNECTED))) {
+ kref_init(&buf->ref);
list_add_tail(&buf->queue, &queue->irqqueue);
} else {
/* If the device is disconnected return the buffer to userspace
@@ -424,28 +425,66 @@ struct uvc_buffer *uvc_queue_get_current_buffer(struct
uvc_video_queue *queue)
return nextbuf;
}
-struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
+/*
+ * uvc_queue_requeue: Requeue a buffer on our internal irqqueue
+ *
+ * Reuse a buffer through our internal queue without the need to 'prepare'
+ * The buffer will be returned to userspace through the uvc_buffer_queue call
if
+ * the device has been disconnected
+ */
+static void uvc_queue_requeue(struct uvc_video_queue *queue,
struct uvc_buffer *buf)
{
- struct uvc_buffer *nextbuf;
- unsigned long flags;
+ buf->error = 0;
+ buf->state = UVC_BUF_STATE_QUEUED;
+ buf->bytesused = 0;
+ vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
+
+ uvc_buffer_queue(&buf->buf.vb2_buf);
+}
+
+static void uvc_queue_buffer_complete(struct kref *ref)
+{
+ struct uvc_buffer *buf = container_of(ref, struct uvc_buffer, ref);
+ struct vb2_buffer *vb = &buf->buf.vb2_buf;
+ struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
- buf->error = 0;
- buf->state = UVC_BUF_STATE_QUEUED;
- buf->bytesused = 0;
- vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
- return buf;
+ uvc_queue_requeue(queue, buf);
+ return;
}
+ buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
+ vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
+ vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+/*
+ * Release a reference on the buffer. Complete the buffer when the last
+ * reference is released
+ */
+void uvc_queue_buffer_release(struct uvc_buffer *buf)
+{
+ kref_put(&buf->ref, uvc_queue_buffer_complete);
+}
+
+/*
+ * Remove this buffer from the queue. Lifetime will persist while async actions
+ * are still running (if any), and uvc_queue_buffer_release will give the
buffer
+ * back to VB2 when all users have completed.
+ */
+struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
+ struct uvc_buffer *buf)
+{
+ struct uvc_buffer *nextbuf;
+ unsigned long flags;
+
spin_lock_irqsave(&queue->irqlock, flags);
list_del(&buf->queue);
nextbuf = __uvc_queue_get_current_buffer(queue);
spin_unlock_irqrestore(&queue->irqlock, flags);
- buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
- vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
- vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
+ uvc_queue_buffer_release(buf);
return nextbuf;
}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 5caa1f4de3cb..6a18dbfc3e5b 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -404,6 +404,9 @@ struct uvc_buffer {
unsigned int bytesused;
u32 pts;
+
+ /* asynchronous buffer handling */
+ struct kref ref;
};
#define UVC_QUEUE_DISCONNECTED (1 << 0)
@@ -696,6 +699,7 @@ extern struct uvc_buffer *
uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
extern struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
struct uvc_buffer *buf);
+extern void uvc_queue_buffer_release(struct uvc_buffer *buf);
extern int uvc_queue_mmap(struct uvc_video_queue *queue,
struct vm_area_struct *vma);
extern unsigned int uvc_queue_poll(struct uvc_video_queue *queue,
--
git-series 0.9.1
[RFT PATCH v3 0/6] Asynchronous UVC
The Linux UVC driver has long provided adequate performance capabilities for web-cams and low data rate video devices in Linux while resolutions were low. Modern USB cameras are now capable of high data rates thanks to USB3 with 1080p, and even 4k capture resolutions supported. Cameras such as the Stereolabs ZED (bulk transfers) or the Logitech BRIO (isochronous transfers) can generate more data than an embedded ARM core is able to process on a single core, resulting in frame loss. A large part of this performance impact is from the requirement to ‘memcpy’ frames out from URB packets to destination frames. This unfortunate requirement is due to the UVC protocol allowing a variable length header, and thus it is not possible to provide the target frame buffers directly. Extra throughput is possible by moving the actual memcpy actions to a work queue, and moving the memcpy out of interrupt context thus allowing work tasks to be scheduled across multiple cores. 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. In particular, any iSight devices, or devices which use UVC to encode data (output device) would certainly be great to be tested with these patches. v2: - Fix race reported by Guennadi v3: - Fix similar race reported by Laurent - Only queue work if required (encode/isight do not queue work) - Refactor/Rename variables for clarity Kieran Bingham (6): uvcvideo: Refactor URB descriptors uvcvideo: Convert decode functions to use new context structure uvcvideo: Protect queue internals with helper uvcvideo: queue: Simplify spin-lock usage uvcvideo: queue: Support asynchronous buffer handling uvcvideo: Move decode processing to process context drivers/media/usb/uvc/uvc_isight.c | 4 +- drivers/media/usb/uvc/uvc_queue.c | 114 + drivers/media/usb/uvc/uvc_video.c | 198 ++ drivers/media/usb/uvc/uvcvideo.h | 56 +++- 4 files changed, 296 insertions(+), 76 deletions(-) base-commit: 6f0e5fd39143a59c22d60e7befc4f33f22aeed2f -- git-series 0.9.1
[RFT PATCH v3 6/6] uvcvideo: Move decode processing to process context
Newer high definition cameras, and cameras with multiple lenses such as
the range of stereo-vision cameras now available have ever increasing
data rates.
The inclusion of a variable length packet header in URB packets mean
that we must memcpy the frame data out to our destination 'manually'.
This can result in data rates of up to 2 gigabits per second being
processed.
To improve efficiency, and maximise throughput, handle the URB decode
processing through a work queue to move it from interrupt context, and
allow multiple processors to work on URBs in parallel.
Signed-off-by: Kieran Bingham
---
v2:
- Lock full critical section of usb_submit_urb()
v3:
- Fix race on submitting uvc_video_decode_data_work() to work queue.
- Rename uvc_decode_op -> uvc_copy_op (Generic to encode/decode)
- Rename decodes -> copy_operations
- Don't queue work if there is no async task
- obtain copy op structure directly in uvc_video_decode_data()
- uvc_video_decode_data_work() -> uvc_video_copy_data_work()
---
drivers/media/usb/uvc/uvc_queue.c | 12 +++-
drivers/media/usb/uvc/uvc_video.c | 116 +++
drivers/media/usb/uvc/uvcvideo.h | 24 ++-
3 files changed, 138 insertions(+), 14 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_queue.c
b/drivers/media/usb/uvc/uvc_queue.c
index 5a9987e547d3..598087eeb5c2 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -179,10 +179,22 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
struct uvc_streaming *stream = uvc_queue_to_stream(queue);
+ /* Prevent new buffers coming in. */
+ spin_lock_irq(&queue->irqlock);
+ queue->flags |= UVC_QUEUE_STOPPING;
+ spin_unlock_irq(&queue->irqlock);
+
+ /*
+* All pending work should be completed before disabling the stream, as
+* all URBs will be free'd during uvc_video_enable(s, 0).
+*/
+ flush_workqueue(stream->async_wq);
+
uvc_video_enable(stream, 0);
spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
+ queue->flags &= ~UVC_QUEUE_STOPPING;
spin_unlock_irq(&queue->irqlock);
}
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 3878bec3276e..fb6b5af17380 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1058,21 +1058,74 @@ static int uvc_video_decode_start(struct uvc_streaming
*stream,
return data[0];
}
-static void uvc_video_decode_data(struct uvc_streaming *stream,
+static void uvc_video_copy_packets(struct uvc_urb *uvc_urb)
+{
+ unsigned int i;
+
+ for (i = 0; i < uvc_urb->async_operations; i++) {
+ struct uvc_copy_op *op = &uvc_urb->copy_operations[i];
+
+ memcpy(op->dst, op->src, op->len);
+
+ /* Release reference taken on this buffer */
+ uvc_queue_buffer_release(op->buf);
+ }
+}
+
+/*
+ * uvc_video_decode_data_work: Asynchronous memcpy processing
+ *
+ * Perform memcpy tasks in process context, with completion handlers
+ * to return the URB, and buffer handles.
+ *
+ * The work submitter must pre-determine that the work is safe
+ */
+static void uvc_video_copy_data_work(struct work_struct *work)
+{
+ struct uvc_urb *uvc_urb = container_of(work, struct uvc_urb, work);
+ struct uvc_streaming *stream = uvc_urb->stream;
+ struct uvc_video_queue *queue = &stream->queue;
+ int ret;
+
+ uvc_video_copy_packets(uvc_urb);
+
+ /*
+* Prevent resubmitting URBs when shutting down to ensure that no new
+* work item will be scheduled after uvc_stop_streaming() flushes the
+* work queue.
+*/
+ spin_lock_irq(&queue->irqlock);
+ if (!(queue->flags & UVC_QUEUE_STOPPING)) {
+ ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
+ if (ret < 0)
+ uvc_printk(KERN_ERR,
+ "Failed to resubmit video URB (%d).\n",
+ ret);
+ }
+ spin_unlock_irq(&queue->irqlock);
+}
+
+static void uvc_video_decode_data(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, const __u8 *data, int len)
{
- unsigned int maxlen, nbytes;
- void *mem;
+ unsigned int active_op = uvc_urb->async_operations;
+ struct uvc_copy_op *decode = &uvc_urb->copy_operations[active_op];
+ unsigned int maxlen;
if (len <= 0)
return;
- /* Copy the video data to the buffer. */
maxlen = buf->length - buf->bytesused;
- mem = buf->mem + buf->bytesused;
- nbytes = min((unsigned int)len, maxlen);
-
[RFT PATCH v3 1/6] uvcvideo: Refactor URB descriptors
We currently store three separate arrays for each URB reference we hold.
Objectify the data needed to track URBs into a single uvc_urb structure,
allowing better object management and tracking of the URB.
All accesses to the data pointers through stream, are converted to use a
uvc_urb pointer for consistency.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- Re-describe URB context structure
- Re-name uvc_urb->{urb_buffer,urb_dma}{buffer,dma}
---
drivers/media/usb/uvc/uvc_video.c | 49 +++-
drivers/media/usb/uvc/uvcvideo.h | 18 ++--
2 files changed, 45 insertions(+), 22 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 73cd44e8bd81..e57c5f52c73b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1357,14 +1357,16 @@ static void uvc_free_urb_buffers(struct uvc_streaming
*stream)
unsigned int i;
for (i = 0; i < UVC_URBS; ++i) {
- if (stream->urb_buffer[i]) {
+ struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+
+ if (uvc_urb->buffer) {
#ifndef CONFIG_DMA_NONCOHERENT
usb_free_coherent(stream->dev->udev, stream->urb_size,
- stream->urb_buffer[i], stream->urb_dma[i]);
+ uvc_urb->buffer, uvc_urb->dma);
#else
- kfree(stream->urb_buffer[i]);
+ kfree(uvc_urb->buffer);
#endif
- stream->urb_buffer[i] = NULL;
+ uvc_urb->buffer = NULL;
}
}
@@ -1402,16 +1404,18 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
*stream,
/* Retry allocations until one succeed. */
for (; npackets > 1; npackets /= 2) {
for (i = 0; i < UVC_URBS; ++i) {
+ struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+
stream->urb_size = psize * npackets;
#ifndef CONFIG_DMA_NONCOHERENT
- stream->urb_buffer[i] = usb_alloc_coherent(
+ uvc_urb->buffer = usb_alloc_coherent(
stream->dev->udev, stream->urb_size,
- gfp_flags | __GFP_NOWARN, &stream->urb_dma[i]);
+ gfp_flags | __GFP_NOWARN, &uvc_urb->dma);
#else
- stream->urb_buffer[i] =
+ uvc_urb->buffer =
kmalloc(stream->urb_size, gfp_flags | __GFP_NOWARN);
#endif
- if (!stream->urb_buffer[i]) {
+ if (!uvc_urb->buffer) {
uvc_free_urb_buffers(stream);
break;
}
@@ -1441,13 +1445,15 @@ static void uvc_uninit_video(struct uvc_streaming
*stream, int free_buffers)
uvc_video_stats_stop(stream);
for (i = 0; i < UVC_URBS; ++i) {
- urb = stream->urb[i];
+ struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+
+ urb = uvc_urb->urb;
if (urb == NULL)
continue;
usb_kill_urb(urb);
usb_free_urb(urb);
- stream->urb[i] = NULL;
+ uvc_urb->urb = NULL;
}
if (free_buffers)
@@ -1502,6 +1508,8 @@ static int uvc_init_video_isoc(struct uvc_streaming
*stream,
size = npackets * psize;
for (i = 0; i < UVC_URBS; ++i) {
+ struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+
urb = usb_alloc_urb(npackets, gfp_flags);
if (urb == NULL) {
uvc_uninit_video(stream, 1);
@@ -1514,12 +1522,12 @@ static int uvc_init_video_isoc(struct uvc_streaming
*stream,
ep->desc.bEndpointAddress);
#ifndef CONFIG_DMA_NONCOHERENT
urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
- urb->transfer_dma = stream->urb_dma[i];
+ urb->transfer_dma = uvc_urb->dma;
#else
urb->transfer_flags = URB_ISO_ASAP;
#endif
urb->interval = ep->desc.bInterval;
- urb->transfer_buffer = stream->urb_buffer[i];
+ urb->transfer_buffer = uvc_urb->buffer;
urb->complete = uvc_video_complete;
urb->number_of_packets = npackets;
urb->transfer_buffer_length = size;
@@ -1529,7 +1537,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
*stream,
urb->iso_frame_desc[j].length = psize;
}
- stream->urb[i] = urb;
+ uvc_urb->urb = urb;
}
[RFT PATCH v3 3/6] uvcvideo: Protect queue internals with helper
The URB completion operation obtains the current buffer by reading
directly into the queue internal interface.
Protect this queue abstraction by providing a helper
uvc_queue_get_current_buffer() which can be used by both the decode
task, and the uvc_queue_next_buffer() functions.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- Fix coding style of conditional statements
---
drivers/media/usb/uvc/uvc_queue.c | 33 +++-
drivers/media/usb/uvc/uvc_video.c | 7 +--
drivers/media/usb/uvc/uvcvideo.h | 2 ++-
3 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_queue.c
b/drivers/media/usb/uvc/uvc_queue.c
index c8d78b2f3de4..4a581d631525 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -399,6 +399,33 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int
disconnect)
spin_unlock_irqrestore(&queue->irqlock, flags);
}
+/*
+ * uvc_queue_get_current_buffer: Obtain the current working output buffer
+ *
+ * Buffers may span multiple packets, and even URBs, therefore the active
buffer
+ * remains on the queue until the EOF marker.
+ */
+static struct uvc_buffer *
+__uvc_queue_get_current_buffer(struct uvc_video_queue *queue)
+{
+ if (list_empty(&queue->irqqueue))
+ return NULL;
+
+ return list_first_entry(&queue->irqqueue, struct uvc_buffer, queue);
+}
+
+struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue)
+{
+ struct uvc_buffer *nextbuf;
+ unsigned long flags;
+
+ spin_lock_irqsave(&queue->irqlock, flags);
+ nextbuf = __uvc_queue_get_current_buffer(queue);
+ spin_unlock_irqrestore(&queue->irqlock, flags);
+
+ return nextbuf;
+}
+
struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
struct uvc_buffer *buf)
{
@@ -415,11 +442,7 @@ struct uvc_buffer *uvc_queue_next_buffer(struct
uvc_video_queue *queue,
spin_lock_irqsave(&queue->irqlock, flags);
list_del(&buf->queue);
- if (!list_empty(&queue->irqqueue))
- nextbuf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
- queue);
- else
- nextbuf = NULL;
+ nextbuf = __uvc_queue_get_current_buffer(queue);
spin_unlock_irqrestore(&queue->irqlock, flags);
buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 92bd0952a66e..3878bec3276e 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1322,7 +1322,6 @@ static void uvc_video_complete(struct urb *urb)
struct uvc_streaming *stream = uvc_urb->stream;
struct uvc_video_queue *queue = &stream->queue;
struct uvc_buffer *buf = NULL;
- unsigned long flags;
int ret;
switch (urb->status) {
@@ -1343,11 +1342,7 @@ static void uvc_video_complete(struct urb *urb)
return;
}
- spin_lock_irqsave(&queue->irqlock, flags);
- if (!list_empty(&queue->irqqueue))
- buf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
- queue);
- spin_unlock_irqrestore(&queue->irqlock, flags);
+ buf = uvc_queue_get_current_buffer(queue);
stream->decode(uvc_urb, buf);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 81a9a419a423..5caa1f4de3cb 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -692,6 +692,8 @@ extern int uvc_queue_streamon(struct uvc_video_queue *queue,
extern int uvc_queue_streamoff(struct uvc_video_queue *queue,
enum v4l2_buf_type type);
extern void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect);
+extern struct uvc_buffer *
+ uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
extern struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
struct uvc_buffer *buf);
extern int uvc_queue_mmap(struct uvc_video_queue *queue,
--
git-series 0.9.1
[RFT PATCH v3 4/6] uvcvideo: queue: Simplify spin-lock usage
Both uvc_start_streaming(), and uvc_stop_streaming() are called from
userspace context. As such, they do not need to save the IRQ state, and
can use spin_lock_irq() and spin_unlock_irq() respectively.
Signed-off-by: Kieran Bingham
---
drivers/media/usb/uvc/uvc_queue.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_queue.c
b/drivers/media/usb/uvc/uvc_queue.c
index 4a581d631525..ddac4d89a291 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -158,7 +158,6 @@ static int uvc_start_streaming(struct vb2_queue *vq,
unsigned int count)
{
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
struct uvc_streaming *stream = uvc_queue_to_stream(queue);
- unsigned long flags;
int ret;
queue->buf_used = 0;
@@ -167,9 +166,9 @@ static int uvc_start_streaming(struct vb2_queue *vq,
unsigned int count)
if (ret == 0)
return 0;
- spin_lock_irqsave(&queue->irqlock, flags);
+ spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
- spin_unlock_irqrestore(&queue->irqlock, flags);
+ spin_unlock_irq(&queue->irqlock);
return ret;
}
@@ -178,13 +177,12 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
{
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
struct uvc_streaming *stream = uvc_queue_to_stream(queue);
- unsigned long flags;
uvc_video_enable(stream, 0);
- spin_lock_irqsave(&queue->irqlock, flags);
+ spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
- spin_unlock_irqrestore(&queue->irqlock, flags);
+ spin_unlock_irq(&queue->irqlock);
}
static const struct vb2_ops uvc_queue_qops = {
--
git-series 0.9.1
[RFT PATCH v3 2/6] uvcvideo: Convert decode functions to use new context structure
The URB completion handlers currently reference the stream context.
Now that each URB has its own context structure, convert the decode (and
one encode) functions to utilise this context for URB management.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- fix checkpatch warning (pre-existing in code)
---
drivers/media/usb/uvc/uvc_isight.c | 4 +++-
drivers/media/usb/uvc/uvc_video.c | 32 ---
drivers/media/usb/uvc/uvcvideo.h | 8
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_isight.c
b/drivers/media/usb/uvc/uvc_isight.c
index 8510e7259e76..433b8b4f96e2 100644
--- a/drivers/media/usb/uvc/uvc_isight.c
+++ b/drivers/media/usb/uvc/uvc_isight.c
@@ -99,9 +99,11 @@ static int isight_decode(struct uvc_video_queue *queue,
struct uvc_buffer *buf,
return 0;
}
-void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
+void uvc_video_decode_isight(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf)
{
+ struct urb *urb = uvc_urb->urb;
+ struct uvc_streaming *stream = uvc_urb->stream;
int ret, i;
for (i = 0; i < urb->number_of_packets; ++i) {
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index e57c5f52c73b..92bd0952a66e 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1153,9 +1153,11 @@ static void uvc_video_validate_buffer(const struct
uvc_streaming *stream,
/*
* Completion handler for video URBs.
*/
-static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
*stream,
- struct uvc_buffer *buf)
+static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
+ struct uvc_buffer *buf)
{
+ struct urb *urb = uvc_urb->urb;
+ struct uvc_streaming *stream = uvc_urb->stream;
u8 *mem;
int ret, i;
@@ -1199,9 +1201,11 @@ static void uvc_video_decode_isoc(struct urb *urb,
struct uvc_streaming *stream,
}
}
-static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming
*stream,
- struct uvc_buffer *buf)
+static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
+ struct uvc_buffer *buf)
{
+ struct urb *urb = uvc_urb->urb;
+ struct uvc_streaming *stream = uvc_urb->stream;
u8 *mem;
int len, ret;
@@ -1266,9 +1270,12 @@ static void uvc_video_decode_bulk(struct urb *urb,
struct uvc_streaming *stream,
}
}
-static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming
*stream,
- struct uvc_buffer *buf)
+static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
+ struct uvc_buffer *buf)
{
+ struct urb *urb = uvc_urb->urb;
+ struct uvc_streaming *stream = uvc_urb->stream;
+
u8 *mem = urb->transfer_buffer;
int len = stream->urb_size, ret;
@@ -1311,7 +1318,8 @@ static void uvc_video_encode_bulk(struct urb *urb, struct
uvc_streaming *stream,
static void uvc_video_complete(struct urb *urb)
{
- struct uvc_streaming *stream = urb->context;
+ struct uvc_urb *uvc_urb = urb->context;
+ struct uvc_streaming *stream = uvc_urb->stream;
struct uvc_video_queue *queue = &stream->queue;
struct uvc_buffer *buf = NULL;
unsigned long flags;
@@ -1341,7 +1349,7 @@ static void uvc_video_complete(struct urb *urb)
queue);
spin_unlock_irqrestore(&queue->irqlock, flags);
- stream->decode(urb, stream, buf);
+ stream->decode(uvc_urb, buf);
if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
@@ -1419,6 +1427,8 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
*stream,
uvc_free_urb_buffers(stream);
break;
}
+
+ uvc_urb->stream = stream;
}
if (i == UVC_URBS) {
@@ -1517,7 +1527,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
*stream,
}
urb->dev = stream->dev->udev;
- urb->context = stream;
+ urb->context = uvc_urb;
urb->pipe = usb_rcvisocpipe(stream->dev->udev,
ep->desc.bEndpointAddress);
#ifndef CONFIG_DMA_NONCOHERENT
@@ -1584,8 +1594,8 @@ static int uvc_init_video_bulk(struct uvc_streaming
*stream,
return -ENOMEM;
}
- usb_fill_bulk_urb(urb, stream->dev->udev, pipe, uvc_urb->buffer,
- size, uvc_video_complete, stream);
+ usb_fill_bulk_urb(urb, stream->dev->udev, pipe, uvc_urb->buffer,
+
Re: [RFT PATCH v3 6/6] uvcvideo: Move decode processing to process context
Hi Guennadi,
Thanks for your review and time on this.
I certainly appreciate the extra eyes here!
On 12/01/18 09:37, Guennadi Liakhovetski wrote:
> Hi Kieran,
>
> On Fri, 12 Jan 2018, Kieran Bingham wrote:
>
>> Newer high definition cameras, and cameras with multiple lenses such as
>> the range of stereo-vision cameras now available have ever increasing
>> data rates.
>>
>> The inclusion of a variable length packet header in URB packets mean
>> that we must memcpy the frame data out to our destination 'manually'.
>> This can result in data rates of up to 2 gigabits per second being
>> processed.
>>
>> To improve efficiency, and maximise throughput, handle the URB decode
>> processing through a work queue to move it from interrupt context, and
>> allow multiple processors to work on URBs in parallel.
>>
>> Signed-off-by: Kieran Bingham
>>
>> ---
>> v2:
>> - Lock full critical section of usb_submit_urb()
>>
>> v3:
>> - Fix race on submitting uvc_video_decode_data_work() to work queue.
>> - Rename uvc_decode_op -> uvc_copy_op (Generic to encode/decode)
>> - Rename decodes -> copy_operations
>> - Don't queue work if there is no async task
>> - obtain copy op structure directly in uvc_video_decode_data()
>> - uvc_video_decode_data_work() -> uvc_video_copy_data_work()
>> ---
>> drivers/media/usb/uvc/uvc_queue.c | 12 +++-
>> drivers/media/usb/uvc/uvc_video.c | 116 +++
>> drivers/media/usb/uvc/uvcvideo.h | 24 ++-
>> 3 files changed, 138 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_queue.c
>> b/drivers/media/usb/uvc/uvc_queue.c
>> index 5a9987e547d3..598087eeb5c2 100644
>> --- a/drivers/media/usb/uvc/uvc_queue.c
>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>> @@ -179,10 +179,22 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
>> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>> struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>>
>> +/* Prevent new buffers coming in. */
>> +spin_lock_irq(&queue->irqlock);
>> +queue->flags |= UVC_QUEUE_STOPPING;
>> +spin_unlock_irq(&queue->irqlock);
Q_A:
>> +
>> +/*
>> + * All pending work should be completed before disabling the stream, as
>> + * all URBs will be free'd during uvc_video_enable(s, 0).
>> + */
>> +flush_workqueue(stream->async_wq);
>
> What if we manage to get one last URB here, then...
That will be fine. queue->flags = UVC_QUEUE_STOPPING, and thus no more items can
be added to the workqueue.
>> +
>> uvc_video_enable(stream, 0);
>>
Q_B:
>> spin_lock_irq(&queue->irqlock);
>> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>> +queue->flags &= ~UVC_QUEUE_STOPPING;
>> spin_unlock_irq(&queue->irqlock);
>> }
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c
>> index 3878bec3276e..fb6b5af17380 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>
> [snip]
>
>> +/*
>> + * When the stream is stopped, all URBs are freed as part of the call to
>> + * uvc_stop_streaming() and must not be handled asynchronously. In that
>> + * event we can safely complete the packet work directly in this
>> + * context, without resubmitting the URB.
>> + */
>> +spin_lock_irqsave(&queue->irqlock, flags);
>> +if (!(queue->flags & UVC_QUEUE_STOPPING)) {
>> +INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
>> +queue_work(stream->async_wq, &uvc_urb->work);
>> +} else {
>> +uvc_video_copy_packets(uvc_urb);
>
> Can it not happen, that if the stream is currently being stopped, the
> queue has been flushed, possibly the previous URB or a couple of them
> don't get decoded, but you do decode this one, creating a corrupted frame?
> Wouldn't it be better to just drop this URB too?
I don't think so.
The only time that this uvc_video_copy_packets() can be called directly in this
context is if UVC_QUEUE_STOPPING is set, *AND* we have the lock...
Therefore we must be executing between points Q_A and Q_B above.
The flush_workqueue() will ensure that all queued work is completed.
By calling uvc_video_copy_packets() directly we are ensuring that this last
packet is also completed. The headers have already been processed at this stage
during the call to ->decode() - so all we are actually doing here is the async
memcpy work which has already been promised by the header processing, and
releasing the references on the vb2 buffers if applicable.
Any buffers not fully completed will be returned marked and returned by the call
to :
uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
which happens *after* label Q_B:
--
Regards
Kieran
>
>> }
>> +spin_unlock_irqrestore(&queue->irqlock, flags);
>> }
>>
>> /*
>
> Thanks
> Guennadi
>
Re: [PATCH] scripts/gdb: fix get_thread_info
On 18/01/18 15:43, Jan Kiszka wrote:
> On 2018-01-18 22:01, Xi Kangjie wrote:
>> Since kernel 4.9, the thread_info has been moved into task_struct,
>> no longer locates at the bottom of kernel stack.
>>
>> See commits:
>> - commit c65eacbe290b ("sched/core: Allow putting thread_info into
>> task_struct")
>> - commit 15f4eae70d36 ("x86: Move thread_info into task_struct")
>>
>> Before fix:
>> (gdb) set $current = $lx_current()
>> (gdb) p $lx_thread_info($current)
>> $1 = {flags = 1470918301}
>> (gdb) p $current.thread_info
>> $2 = {flags = 2147483648}
>>
>> After fix:
>> (gdb) p $lx_thread_info($current)
>> $1 = {flags = 2147483648}
>> (gdb) p $current.thread_info
>> $2 = {flags = 2147483648}
>>
>> Signed-off-by: Xi Kangjie
>> ---
>> scripts/gdb/linux/tasks.py | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/scripts/gdb/linux/tasks.py b/scripts/gdb/linux/tasks.py
>> index 1bf949c43b76..f6ab3ccf698f 100644
>> --- a/scripts/gdb/linux/tasks.py
>> +++ b/scripts/gdb/linux/tasks.py
>> @@ -96,6 +96,8 @@ def get_thread_info(task):
>> thread_info_addr = task.address + ia64_task_size
>> thread_info = thread_info_addr.cast(thread_info_ptr_type)
>> else:
>> +if task.type.fields()[0].type == thread_info_type.get_type():
>> +return task['thread_info']
>> thread_info = task['stack'].cast(thread_info_ptr_type)
>> return thread_info.dereference()
>>
>>
>
> Acked-by: Jan Kiszka
>
> Andrew, please pick up.
Acked-by: Kieran Bingham
However, should we mark this as:
Cc: # 4.9+ ?
Regards
--
Kieran
[PATCH 0/2] Add support for i2c_new_secondary_device
Back in 2014, Jean-Michel provided patches [0] to implement a means of describing software defined I2C addresses for devices through the DT nodes. The patch to implement the function "i2c_new_secondary_device()" was integrated, but the corresponding driver update didn't get applied. This short series re-bases Jean-Michel's patch to mainline for the ADV7604 driver in linux-media, and also provides a patch for the ADV7511 DRM Bridge driver taking the same approach. This series allows us to define the I2C address allocations of these devices in the device tree for the Renesas D3 platform where these two devices reside on the same bus and conflict with each other presently.. [0] https://lkml.org/lkml/2014/10/22/468 [1] https://lkml.org/lkml/2014/10/22/469 Jean-Michel Hautbois (1): media: adv7604: Add support for i2c_new_secondary_device Kieran Bingham (1): drm: adv7511: Add support for i2c_new_secondary_device .../bindings/display/bridge/adi,adv7511.txt| 10 +++- .../devicetree/bindings/media/i2c/adv7604.txt | 18 ++- drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 ++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 - drivers/media/i2c/adv7604.c| 60 ++ 5 files changed, 92 insertions(+), 36 deletions(-) -- 2.7.4
[PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device
The ADV7511 has four 256-byte maps that can be accessed via the main I²C
ports. Each map has it own I²C address and acts as a standard slave
device on the I²C bus.
Allow a device tree node to override the default addresses so that
address conflicts with other devices on the same bus may be resolved at
the board description level.
Signed-off-by: Kieran Bingham
---
.../bindings/display/bridge/adi,adv7511.txt| 10 +-
drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++
3 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 0047b1394c70..f6bb9f6d3f48 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -70,6 +70,9 @@ Optional properties:
rather than generate its own timings for HDMI output.
- clocks: from common clock binding: reference to the CEC clock.
- clock-names: from common clock binding: must be "cec".
+- reg-names : Names of maps with programmable addresses.
+ It can contain any map needing a non-default address.
+ Possible maps names are : "main", "edid", "cec", "packet"
Required nodes:
@@ -88,7 +91,12 @@ Example
adv7511w: hdmi@39 {
compatible = "adi,adv7511w";
- reg = <39>;
+ /*
+* The EDID page will be accessible on address 0x66 on the i2c
+* bus. All other maps continue to use their default addresses.
+*/
+ reg = <0x39 0x66>;
+ reg-names = "main", "edid";
interrupt-parent = <&gpio3>;
interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
clocks = <&cec_clock>;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index d034b2cb5eee..7d81ce3808e0 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -53,8 +53,10 @@
#define ADV7511_REG_POWER 0x41
#define ADV7511_REG_STATUS 0x42
#define ADV7511_REG_EDID_I2C_ADDR 0x43
+#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT 0x3f
#define ADV7511_REG_PACKET_ENABLE1 0x44
#define ADV7511_REG_PACKET_I2C_ADDR0x45
+#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT0x38
#define ADV7511_REG_DSD_ENABLE 0x46
#define ADV7511_REG_VIDEO_INPUT_CFG2 0x48
#define ADV7511_REG_INFOFRAME_UPDATE 0x4a
@@ -89,6 +91,7 @@
#define ADV7511_REG_TMDS_CLOCK_INV 0xde
#define ADV7511_REG_ARC_CTRL 0xdf
#define ADV7511_REG_CEC_I2C_ADDR 0xe1
+#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT 0x3c
#define ADV7511_REG_CEC_CTRL 0xe2
#define ADV7511_REG_CHIP_ID_HIGH 0xf5
#define ADV7511_REG_CHIP_ID_LOW0xf6
@@ -322,6 +325,7 @@ struct adv7511 {
struct i2c_client *i2c_main;
struct i2c_client *i2c_edid;
struct i2c_client *i2c_cec;
+ struct i2c_client *i2c_packet;
struct regmap *regmap;
struct regmap *regmap_cec;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index efa29db5fc2b..7ec33837752b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
{
int ret;
- adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
-adv->i2c_main->addr - 1);
+ adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
+ ADV7511_REG_CEC_I2C_ADDR_DEFAULT);
if (!adv->i2c_cec)
return -ENOMEM;
i2c_set_clientdata(adv->i2c_cec, adv);
@@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const
struct i2c_device_id *id)
struct adv7511_link_config link_config;
struct adv7511 *adv7511;
struct device *dev = &i2c->dev;
- unsigned int main_i2c_addr = i2c->addr << 1;
- unsigned int edid_i2c_addr = main_i2c_addr + 4;
unsigned int val;
int ret;
@@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, const
struct i2c_device_id *id)
if (ret)
goto uninit_regulators;
- regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
- regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
-main_i2c_addr - 0x
[PATCH 1/2] media: adv7604: Add support for i2c_new_secondary_device
From: Jean-Michel Hautbois
The ADV7604 has thirteen 256-byte maps that can be accessed via the main
I²C ports. Each map has it own I²C address and acts as a standard slave
device on the I²C bus.
Allow a device tree node to override the default addresses so that
address conflicts with other devices on the same bus may be resolved at
the board description level.
Signed-off-by: Jean-Michel Hautbois
[Kieran: Re-adapted for mainline]
Signed-off-by: Kieran Bingham
---
Based upon the original posting :
https://lkml.org/lkml/2014/10/22/469
---
.../devicetree/bindings/media/i2c/adv7604.txt | 18 ++-
drivers/media/i2c/adv7604.c| 60 ++
2 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index 9cbd92eb5d05..b64e313dcc66 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -13,7 +13,11 @@ Required Properties:
- "adi,adv7611" for the ADV7611
- "adi,adv7612" for the ADV7612
- - reg: I2C slave address
+ - reg: I2C slave addresses
+The ADV76xx has up to thirteen 256-byte maps that can be accessed via the
+main I²C ports. Each map has it own I²C address and acts as a standard
+slave device on the I²C bus. The main address is mandatory, others are
+optional and revert to defaults if not specified.
- hpd-gpios: References to the GPIOs that control the HDMI hot-plug
detection pins, one per HDMI input. The active flag indicates the GPIO
@@ -35,6 +39,11 @@ Optional Properties:
- reset-gpios: Reference to the GPIO connected to the device's reset pin.
- default-input: Select which input is selected after reset.
+ - reg-names : Names of maps with programmable addresses.
+ It can contain any map needing a non-default address.
+ Possible maps names are :
+ "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
+ "rep", "edid", "hdmi", "test", "cp", "vdp"
Optional Endpoint Properties:
@@ -52,7 +61,12 @@ Example:
hdmi_receiver@4c {
compatible = "adi,adv7611";
- reg = <0x4c>;
+ /*
+* The edid page will be accessible @ 0x66 on the i2c bus. All
+* other maps will retain their default addresses.
+*/
+ reg = <0x4c 0x66>;
+ reg-names "main", "edid";
reset-gpios = <&ioexp 0 GPIO_ACTIVE_LOW>;
hpd-gpios = <&ioexp 2 GPIO_ACTIVE_HIGH>;
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 1544920ec52d..c346b9a8fb57 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2734,6 +2734,27 @@ static const struct v4l2_ctrl_config
adv76xx_ctrl_free_run_color = {
/* --- */
+struct adv76xx_register {
+ const char *name;
+ u8 default_addr;
+};
+
+static const struct adv76xx_register adv76xx_secondary_names[] = {
+ [ADV76XX_PAGE_IO] = { "main", 0x4c },
+ [ADV7604_PAGE_AVLINK] = { "avlink", 0x42 },
+ [ADV76XX_PAGE_CEC] = { "cec", 0x40 },
+ [ADV76XX_PAGE_INFOFRAME] = { "infoframe", 0x3e },
+ [ADV7604_PAGE_ESDP] = { "esdp", 0x38 },
+ [ADV7604_PAGE_DPP] = { "dpp", 0x3c },
+ [ADV76XX_PAGE_AFE] = { "afe", 0x26 },
+ [ADV76XX_PAGE_REP] = { "rep", 0x32 },
+ [ADV76XX_PAGE_EDID] = { "edid", 0x36 },
+ [ADV76XX_PAGE_HDMI] = { "hdmi", 0x34 },
+ [ADV76XX_PAGE_TEST] = { "test", 0x30 },
+ [ADV76XX_PAGE_CP] = { "cp", 0x22 },
+ [ADV7604_PAGE_VDP] = { "vdp", 0x24 },
+};
+
static int adv76xx_core_init(struct v4l2_subdev *sd)
{
struct adv76xx_state *state = to_state(sd);
@@ -2834,13 +2855,26 @@ static void adv76xx_unregister_clients(struct
adv76xx_state *state)
}
static struct i2c_client *adv76xx_dummy_client(struct v4l2_subdev *sd,
- u8 addr, u8 io_reg)
+ unsigned int i)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct adv76xx_state *state = to_state(sd);
+ struct adv76xx_platform_data *pdata = &state->pdata;
+ unsigned int io_reg = 0xf2 + i;
+ struct i2c_client *new_client;
+
+ if (pdata && pdata->i2c_addresses[i])
+ new_client = i2c_new_dummy(client->adapter,
+
Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device
On 22/01/18 13:00, Lars-Peter Clausen wrote:
> On 01/22/2018 01:50 PM, Kieran Bingham wrote:
>> The ADV7511 has four 256-byte maps that can be accessed via the main I²C
>> ports. Each map has it own I²C address and acts as a standard slave
>> device on the I²C bus.
>>
>> Allow a device tree node to override the default addresses so that
>> address conflicts with other devices on the same bus may be resolved at
>> the board description level.
>>
>> Signed-off-by: Kieran Bingham
>
> I've been working on the same thing, but you've beat me to it! Patch looks
> mostly OK, but I think you are missing this piece:
>
> https://github.com/analogdevicesinc/linux/commit/ba9b57507cb78724a606eb24104e22fea942437d#diff-2cf1828c644e351adefabe9509410400L553
Ah yes - Thanks, - you're correct - I had missed that bit.
Added locally for a v2.
--
Kieran
>> ---
>> .../bindings/display/bridge/adi,adv7511.txt| 10 +-
>> drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++
>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36
>> ++
>> 3 files changed, 37 insertions(+), 13 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> index 0047b1394c70..f6bb9f6d3f48 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> @@ -70,6 +70,9 @@ Optional properties:
>>rather than generate its own timings for HDMI output.
>> - clocks: from common clock binding: reference to the CEC clock.
>> - clock-names: from common clock binding: must be "cec".
>> +- reg-names : Names of maps with programmable addresses.
>> +It can contain any map needing a non-default address.
>> +Possible maps names are : "main", "edid", "cec", "packet"
>>
>> Required nodes:
>>
>> @@ -88,7 +91,12 @@ Example
>>
>> adv7511w: hdmi@39 {
>> compatible = "adi,adv7511w";
>> -reg = <39>;
>> +/*
>> + * The EDID page will be accessible on address 0x66 on the i2c
>> + * bus. All other maps continue to use their default addresses.
>> + */
>> +reg = <0x39 0x66>;
>> +reg-names = "main", "edid";
>> interrupt-parent = <&gpio3>;
>> interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>> clocks = <&cec_clock>;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index d034b2cb5eee..7d81ce3808e0 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -53,8 +53,10 @@
>> #define ADV7511_REG_POWER 0x41
>> #define ADV7511_REG_STATUS 0x42
>> #define ADV7511_REG_EDID_I2C_ADDR 0x43
>> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT 0x3f
>> #define ADV7511_REG_PACKET_ENABLE1 0x44
>> #define ADV7511_REG_PACKET_I2C_ADDR 0x45
>> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT 0x38
>> #define ADV7511_REG_DSD_ENABLE 0x46
>> #define ADV7511_REG_VIDEO_INPUT_CFG20x48
>> #define ADV7511_REG_INFOFRAME_UPDATE0x4a
>> @@ -89,6 +91,7 @@
>> #define ADV7511_REG_TMDS_CLOCK_INV 0xde
>> #define ADV7511_REG_ARC_CTRL0xdf
>> #define ADV7511_REG_CEC_I2C_ADDR0xe1
>> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT0x3c
>> #define ADV7511_REG_CEC_CTRL0xe2
>> #define ADV7511_REG_CHIP_ID_HIGH0xf5
>> #define ADV7511_REG_CHIP_ID_LOW 0xf6
>> @@ -322,6 +325,7 @@ struct adv7511 {
>> struct i2c_client *i2c_main;
>> struct i2c_client *i2c_edid;
>> struct i2c_client *i2c_cec;
>> +struct i2c_client *i2c_packet;
>>
>> struct regmap *regmap;
>> struct regmap *regmap_cec;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index efa29db5fc2b..7ec33837752b 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>> {
&
Re: [RFC/RFT PATCH 6/6] uvcvideo: Move decode processing to process context
Hi Guennadi,
Thank you for taking the time to review this series,
On 04/01/18 18:54, Guennadi Liakhovetski wrote:
> On Wed, 3 Jan 2018, Kieran Bingham wrote:
>
>> From: Kieran Bingham
>>
>> Newer high definition cameras, and cameras with multiple lenses such as
>> the range of stereovision cameras now available have ever increasing
>> data rates.
>>
>> The inclusion of a variable length packet header in URB packets mean
>> that we must memcpy the frame data out to our destination 'manually'.
>> This can result in data rates of up to 2 gigabits per second being
>> processed.
>>
>> To improve efficiency, and maximise throughput, handle the URB decode
>> processing through a work queue to move it from interrupt context, and
>> allow multiple processors to work on URBs in parallel.
>>
>> Signed-off-by: Kieran Bingham
>> ---
>> drivers/media/usb/uvc/uvc_queue.c | 12 +++-
>> drivers/media/usb/uvc/uvc_video.c | 114 ++-
>> drivers/media/usb/uvc/uvcvideo.h | 24 +++-
>> 3 files changed, 132 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_queue.c
>> b/drivers/media/usb/uvc/uvc_queue.c
>> index 204dd91a8526..07fcbfc132c9 100644
>> --- a/drivers/media/usb/uvc/uvc_queue.c
>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>> @@ -179,10 +179,22 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
>> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>> struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>>
>> +/* Prevent new buffers coming in. */
>> +spin_lock_irq(&queue->irqlock);
>> +queue->flags |= UVC_QUEUE_STOPPING;
>> +spin_unlock_irq(&queue->irqlock);
>> +
>> +/*
>> + * All pending work should be completed before disabling the stream, as
>> + * all URBs will be free'd during uvc_video_enable(s, 0).
>> + */
>> +flush_workqueue(stream->async_wq);
>> +
>> uvc_video_enable(stream, 0);
>>
>> spin_lock_irq(&queue->irqlock);
>> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>> +queue->flags &= ~UVC_QUEUE_STOPPING;
>> spin_unlock_irq(&queue->irqlock);
>> }
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c
>> index 045ac655313c..b7b32a6bc2dc 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1058,21 +1058,70 @@ static int uvc_video_decode_start(struct
>> uvc_streaming *stream,
>> return data[0];
>> }
>>
>> -static void uvc_video_decode_data(struct uvc_streaming *stream,
>> -struct uvc_buffer *buf, const __u8 *data, int len)
>> +/*
>> + * uvc_video_decode_data_work: Asynchronous memcpy processing
>> + *
>> + * Perform memcpy tasks in process context, with completion handlers
>> + * to return the URB, and buffer handles.
>> + *
>> + * The work submitter must pre-determine that the work is safe
>> + */
>> +static void uvc_video_decode_data_work(struct work_struct *work)
>> {
>> -unsigned int maxlen, nbytes;
>> -void *mem;
>> +struct uvc_urb *uvc_urb = container_of(work, struct uvc_urb, work);
>> +struct uvc_streaming *stream = uvc_urb->stream;
>> +struct uvc_video_queue *queue = &stream->queue;
>> +unsigned int i;
>> +bool stopping;
>> +int ret;
>> +
>> +for (i = 0; i < uvc_urb->packets; i++) {
>> +struct uvc_decode_op *op = &uvc_urb->decodes[i];
>> +
>> +memcpy(op->dst, op->src, op->len);
>> +
>> +/* Release reference taken on this buffer */
>> +uvc_queue_buffer_release(op->buf);
>> +}
>> +
>> +/*
>> + * Prevent resubmitting URBs when shutting down to ensure that no new
>> + * work item will be scheduled after uvc_stop_streaming() flushes the
>> + * work queue.
>> + */
>> +spin_lock_irq(&queue->irqlock);
>> +stopping = queue->flags & UVC_QUEUE_STOPPING;
>> +spin_unlock_irq(&queue->irqlock);
>
> Are you sure this locking really helps? What if uvc_stop_streaming() runs
> here?
Quite - there is a race there. It protects the flag though ;-)
I thought I had pulled the lock out to only check the flag, to prevent
surrounding the usb_submit_urb(), but now I look again - I see no reason not to
lock for the whole critical section.
Re: [RFC/RFT PATCH 1/6] uvcvideo: Refactor URB descriptors
Hi Guennadi,
Thanks for your review,
On 04/01/18 18:24, Guennadi Liakhovetski wrote:
> Hi Kieran,
>
> Just minor suggestions below:
>
> On Wed, 3 Jan 2018, Kieran Bingham wrote:
>
>> From: Kieran Bingham
>>
>> We currently store three separate arrays for each URB reference we hold.
>>
>> Objectify the data needed to track URBs into a single uvc_urb structure,
>> allowing better object management and tracking of the URB.
>>
>> All accesses to the data pointers through stream, are converted to use a
>> uvc_urb pointer for consistency.
>>
>> Signed-off-by: Kieran Bingham
>> Reviewed-by: Laurent Pinchart
>> ---
>> drivers/media/usb/uvc/uvc_video.c | 46
>> drivers/media/usb/uvc/uvcvideo.h | 18 ++---
>> 2 files changed, 44 insertions(+), 20 deletions(-)
>
> [snip]
>
>> diff --git a/drivers/media/usb/uvc/uvcvideo.h
>> b/drivers/media/usb/uvc/uvcvideo.h
>> index 19e725e2bda5..4afa8ce13ea7 100644
>> --- a/drivers/media/usb/uvc/uvcvideo.h
>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>> @@ -479,6 +479,20 @@ struct uvc_stats_stream {
>> unsigned int max_sof; /* Maximum STC.SOF value */
>> };
>>
>> +/**
>> + * struct uvc_urb - URB context management structure
>> + *
>> + * @urb: described URB. Must be allocated with usb_alloc_urb()
>
> Didn't you mean "describes?"
Hrm ... I think I meant described as in "This is the URB described by this
uvc_urb structure", rather than "this variable describes the URB"
Perhaps I'll change this to:
@urb: The URB described by this context structure.
I think the 'must be allocated with usb_alloc_urb() is fairly implicit, so could
be dropped in that case.
>
>> + * @urb_buffer: memory storage for the URB
>> + * @urb_dma: DMA coherent addressing for the urb_buffer
>
> The whole struct describes URBs, so, I wouldn't repeat that in these two
> field names, I'd just call them "buffer" and "dma." OTOH, later you add
> more fields like "stream," which aren't per-URB, so, maybe you want to
> keep these prefixes.
These names were kept from the original fields. But actually I think you're
right here - it wouldn't hurt to shorten the names, even with the other fields
added.
> Thanks
> Guennadi
>
>> + */
>> +struct uvc_urb {
>> +struct urb *urb;
>> +
>> +char *urb_buffer;
>> +dma_addr_t urb_dma;
>> +};
>> +
>> struct uvc_streaming {
>> struct list_head list;
>> struct uvc_device *dev;
>> @@ -521,9 +535,7 @@ struct uvc_streaming {
>> __u32 max_payload_size;
>> } bulk;
>>
>> -struct urb *urb[UVC_URBS];
>> -char *urb_buffer[UVC_URBS];
>> -dma_addr_t urb_dma[UVC_URBS];
>> +struct uvc_urb uvc_urb[UVC_URBS];
>> unsigned int urb_size;
>>
>> __u32 sequence;
>> --
>> git-series 0.9.1
>>
--
Kieran
Re: [RFC/RFT PATCH 3/6] uvcvideo: Protect queue internals with helper
Hi Guennadi,
On 04/01/18 18:25, Guennadi Liakhovetski wrote:
> Hi Kieran,
>
> On Wed, 3 Jan 2018, Kieran Bingham wrote:
>
>> From: Kieran Bingham
>>
>> The URB completion operation obtains the current buffer by reading
>> directly into the queue internal interface.
>>
>> Protect this queue abstraction by providing a helper
>> uvc_queue_get_current_buffer() which can be used by both the decode
>> task, and the uvc_queue_next_buffer() functions.
>>
>> Signed-off-by: Kieran Bingham
>> Reviewed-by: Laurent Pinchart
>> ---
>> drivers/media/usb/uvc/uvc_queue.c | 34 +++-
>> drivers/media/usb/uvc/uvc_video.c | 7 +--
>> drivers/media/usb/uvc/uvcvideo.h | 2 ++-
>> 3 files changed, 32 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_queue.c
>> b/drivers/media/usb/uvc/uvc_queue.c
>> index c8d78b2f3de4..0711e3d9ff76 100644
>> --- a/drivers/media/usb/uvc/uvc_queue.c
>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>> @@ -399,6 +399,34 @@ void uvc_queue_cancel(struct uvc_video_queue *queue,
>> int disconnect)
>> spin_unlock_irqrestore(&queue->irqlock, flags);
>> }
>>
>> +/*
>> + * uvc_queue_get_current_buffer: Obtain the current working output buffer
>> + *
>> + * Buffers may span multiple packets, and even URBs, therefore the active
>> buffer
>> + * remains on the queue until the EOF marker.
>> + */
>> +static struct uvc_buffer *
>> +__uvc_queue_get_current_buffer(struct uvc_video_queue *queue)
>> +{
>> +if (!list_empty(&queue->irqqueue))
>> +return list_first_entry(&queue->irqqueue, struct uvc_buffer,
>> +queue);
>> +else
>> +return NULL;
>
> I think the preferred style is not to use "else" in such cases. It might
> even be prettier to write
>
> if (list_empty(...))
> return NULL;
>
> return list_first_entry(...);
Ah yes, I believe you are correct.
Good spot!
Fixed, and looks much neater.
--
Kieran
>
> Thanks
> Guennadi
>
[PATCH] v4l: doc: clarify v4l2_mbus_fmt height definition
The v4l2_mbus_fmt width and height corresponds directly with the v4l2_pix_format definitions, yet the differences in documentation make it ambiguous what to do in the event of field heights. Clarify this by referencing the v4l2_pix_format which is explicit on the matter, and by matching the terminology of 'image height' rather than the misleading 'frame height'. Signed-off-by: Kieran Bingham --- Documentation/media/uapi/v4l/subdev-formats.rst | 6 -- include/uapi/linux/v4l2-mediabus.h | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst b/Documentation/media/uapi/v4l/subdev-formats.rst index b1eea44550e1..a2a00202b430 100644 --- a/Documentation/media/uapi/v4l/subdev-formats.rst +++ b/Documentation/media/uapi/v4l/subdev-formats.rst @@ -16,10 +16,12 @@ Media Bus Formats * - __u32 - ``width`` - - Image width, in pixels. + - Image width in pixels. See struct + :c:type:`v4l2_pix_format`. * - __u32 - ``height`` - - Image height, in pixels. + - Image height in pixels. See struct + :c:type:`v4l2_pix_format`. * - __u32 - ``code`` - Format code, from enum diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h index 6e20de63ec59..6b34108d0338 100644 --- a/include/uapi/linux/v4l2-mediabus.h +++ b/include/uapi/linux/v4l2-mediabus.h @@ -18,8 +18,8 @@ /** * struct v4l2_mbus_framefmt - frame format on the media bus - * @width: frame width - * @height:frame height + * @width: image width + * @height:image height (see struct v4l2_pix_format) * @code: data format code (from enum v4l2_mbus_pixelcode) * @field: used interlacing type (from enum v4l2_field) * @colorspace:colorspace of the data (from enum v4l2_colorspace) -- 2.7.4
[PATCH] media: i2c: adv748x: fix HDMI field heights
The ADV748x handles interlaced media using V4L2_FIELD_ALTERNATE field
types. The correct specification for the height on the mbus is the
image height, in this instance, the field height.
The AFE component already correctly adjusts the height on the mbus, but
the HDMI component got left behind.
Adjust the mbus height to correctly describe the image height of the
fields when processing interlaced video for HDMI pipelines.
Fixes: 3e89586a64df ("media: i2c: adv748x: add adv748x driver")
Signed-off-by: Kieran Bingham
---
drivers/media/i2c/adv748x/adv748x-hdmi.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c
b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index 4da4253553fc..0e2f76f3f029 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -105,6 +105,10 @@ static void adv748x_hdmi_fill_format(struct adv748x_hdmi
*hdmi,
fmt->width = hdmi->timings.bt.width;
fmt->height = hdmi->timings.bt.height;
+
+ /* Propagate field height on the mbus for FIELD_ALTERNATE fmts */
+ if (hdmi->timings.bt.interlaced)
+ fmt->height /= 2;
}
static void adv748x_fill_optional_dv_timings(struct v4l2_dv_timings *timings)
--
2.7.4
[PATCH v2] v4l: doc: Clarify v4l2_mbus_fmt height definition
The v4l2_mbus_fmt width and height corresponds directly with the v4l2_pix_format definitions, yet the differences in documentation make it ambiguous what to do in the event of field heights. Clarify this using the same text as is provided for the v4l2_pix_format which is explicit on the matter, and by matching the terminology of 'image height' rather than the misleading 'frame height'. Signed-off-by: Kieran Bingham --- v2: - Duplicated explicit text from v4l2_pix_format rather than referencing it. Documentation/media/uapi/v4l/subdev-formats.rst | 8 ++-- include/uapi/linux/v4l2-mediabus.h | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst b/Documentation/media/uapi/v4l/subdev-formats.rst index b1eea44550e1..9fcabe7f9367 100644 --- a/Documentation/media/uapi/v4l/subdev-formats.rst +++ b/Documentation/media/uapi/v4l/subdev-formats.rst @@ -16,10 +16,14 @@ Media Bus Formats * - __u32 - ``width`` - - Image width, in pixels. + - Image width in pixels. * - __u32 - ``height`` - - Image height, in pixels. + - Image height in pixels. If ``field`` is one of ``V4L2_FIELD_TOP``, + ``V4L2_FIELD_BOTTOM`` or ``V4L2_FIELD_ALTERNATE`` then height + refers to the number of lines in the field, otherwise it refers to + the number of lines in the frame (which is twice the field height + for interlaced formats). * - __u32 - ``code`` - Format code, from enum diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h index 6e20de63ec59..123a231001a8 100644 --- a/include/uapi/linux/v4l2-mediabus.h +++ b/include/uapi/linux/v4l2-mediabus.h @@ -18,8 +18,8 @@ /** * struct v4l2_mbus_framefmt - frame format on the media bus - * @width: frame width - * @height:frame height + * @width: image width + * @height:image height * @code: data format code (from enum v4l2_mbus_pixelcode) * @field: used interlacing type (from enum v4l2_field) * @colorspace:colorspace of the data (from enum v4l2_colorspace) -- 2.7.4
Re: [PATCH] media: i2c: adv748x: fix HDMI field heights
Hi Niklas,
On 08/01/18 17:56, Niklas Söderlund wrote:
> Hi Kieran,
>
> Thanks for your patch.
>
> On 2018-01-08 17:39:30 +, Kieran Bingham wrote:
>> The ADV748x handles interlaced media using V4L2_FIELD_ALTERNATE field
>> types. The correct specification for the height on the mbus is the
>> image height, in this instance, the field height.
>>
>> The AFE component already correctly adjusts the height on the mbus, but
>> the HDMI component got left behind.
>>
>> Adjust the mbus height to correctly describe the image height of the
>> fields when processing interlaced video for HDMI pipelines.
>>
>> Fixes: 3e89586a64df ("media: i2c: adv748x: add adv748x driver")
>> Signed-off-by: Kieran Bingham
>> ---
>> drivers/media/i2c/adv748x/adv748x-hdmi.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c
>> b/drivers/media/i2c/adv748x/adv748x-hdmi.c
>> index 4da4253553fc..0e2f76f3f029 100644
>> --- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
>> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
>> @@ -105,6 +105,10 @@ static void adv748x_hdmi_fill_format(struct
>> adv748x_hdmi *hdmi,
>>
>> fmt->width = hdmi->timings.bt.width;
>> fmt->height = hdmi->timings.bt.height;
>> +
>> +/* Propagate field height on the mbus for FIELD_ALTERNATE fmts */
>> +if (hdmi->timings.bt.interlaced)
>
> if (V4L2_FIELD_HAS_T_OR_B(fmt->field))
>
> Nit-picking but I would use the field here (which is set just above this
> in the same function) as it makes it more clear why the format is cut in
> half. I looked at the documentation for bt.interlaced and I'm not sure
> if it would be set to true for INTERLACED field formats when the height
> should not be halved? In this case it do not matter as
>
> fmt->field = hdmi->timings.bt.interlaced ?
> V4L2_FIELD_ALTERNATE : V4L2_FIELD_NONE;
>
> So I leave this up to you and feel free to add in either case.
In this instance, I chose to mirror the value(/code path) which explicitly sets
the V4L2_FIELD_ALTERNATE, rather than:
if (fmt->field == V4L2_FIELD_ALTERNATE ||
fmt->field == V4L2_FIELD_TOP ||
fmt->field == V4L2_FIELD_BOTTOM)
But perhaps we would never expect _TOP || _BOTTOM therefore
if (fmt->field == V4L2_FIELD_ALTERNATE)
might be perfectly suitable?
Actually - yes - I now agree. I'll respin.
--
Kieran
> Reviewed-by: Niklas Söderlund
>
>> +fmt->height /= 2;
>> }
>>
>> static void adv748x_fill_optional_dv_timings(struct v4l2_dv_timings
>> *timings)
>> --
>> 2.7.4
>>
>
Re: [PATCH] v4l: doc: clarify v4l2_mbus_fmt height definition
Hi Hans, Niklas, Sakari, Thank you for the very prompt reviews! I fired the patch - disappeared to teach code club, and came back to the answers :-D - very streamlined! On 08/01/18 15:48, Hans Verkuil wrote: > On 01/08/2018 04:32 PM, Niklas Söderlund wrote: >> Hi, >> >> Thanks for your patch. >> >> On 2018-01-08 17:13:53 +0200, Sakari Ailus wrote: >>> Hi Kieran, >>> >>> On Mon, Jan 08, 2018 at 02:45:49PM +, Kieran Bingham wrote: >>>> The v4l2_mbus_fmt width and height corresponds directly with the >>>> v4l2_pix_format definitions, yet the differences in documentation make >>>> it ambiguous what to do in the event of field heights. >>>> >>>> Clarify this by referencing the v4l2_pix_format which is explicit on the >>>> matter, and by matching the terminology of 'image height' rather than >>>> the misleading 'frame height'. >> >> Nice that this relationship is documented as it have contributed to some >> confusion on my side in the past! >> >>>> >>>> Signed-off-by: Kieran Bingham >>>> --- >>>> Documentation/media/uapi/v4l/subdev-formats.rst | 6 -- >>>> include/uapi/linux/v4l2-mediabus.h | 4 ++-- >>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst >>>> b/Documentation/media/uapi/v4l/subdev-formats.rst >>>> index b1eea44550e1..a2a00202b430 100644 >>>> --- a/Documentation/media/uapi/v4l/subdev-formats.rst >>>> +++ b/Documentation/media/uapi/v4l/subdev-formats.rst >>>> @@ -16,10 +16,12 @@ Media Bus Formats >>>> >>>> * - __u32 >>>>- ``width`` >>>> - - Image width, in pixels. >>>> + - Image width in pixels. See struct >>>> + :c:type:`v4l2_pix_format`. >>>> * - __u32 >>>>- ``height`` >>>> - - Image height, in pixels. >>>> + - Image height in pixels. See struct >>>> + :c:type:`v4l2_pix_format`. >>>> * - __u32 >>>>- ``code`` >>>>- Format code, from enum >>>> diff --git a/include/uapi/linux/v4l2-mediabus.h >>>> b/include/uapi/linux/v4l2-mediabus.h >>>> index 6e20de63ec59..6b34108d0338 100644 >>>> --- a/include/uapi/linux/v4l2-mediabus.h >>>> +++ b/include/uapi/linux/v4l2-mediabus.h >>>> @@ -18,8 +18,8 @@ >>>> >>>> /** >>>> * struct v4l2_mbus_framefmt - frame format on the media bus >>>> - * @width:frame width >>>> - * @height: frame height >>>> + * @width:image width >>>> + * @height: image height (see struct v4l2_pix_format) >>> >>> Hmm. This is the media bus format and it has no direct relation to >>> v4l2_pix_format. So no, I can't see what would be the point in making such >>> a reference. >> >> Well we have functions like v4l2_fill_pix_format() that do >> >> pix_fmt->width = mbus_fmt->width; >> pix_fmt->height = mbus_fmt->height; >> >> So I think there at least is an implicit relation between the two >> structs. The issue I think Kieran is trying to address is in the case of >> TOP, BOTTOM and ALTERNATE field formats. From the v4l2_pix_format >> documentation on the height field: Yes, it was this relationship which made me feel it was appropriate to just reference in the same way that the subdevice version did. >>"Image height in pixels. If field is one of V4L2_FIELD_TOP, >>V4L2_FIELD_BOTTOM or V4L2_FIELD_ALTERNATE then height refers to the >>number of lines in the field, otherwise it refers to the number of >>lines in the frame (which is twice the field height for interlaced >>formats)." > > Right, and I'd just copy this text to subdev-formats.rst rather than referring > to it. Ok - Copied for V2. Thanks Kieran
[PATCH v2] media: i2c: adv748x: fix HDMI field heights
The ADV748x handles interlaced media using V4L2_FIELD_ALTERNATE field
types. The correct specification for the height on the mbus is the
image height, in this instance, the field height.
The AFE component already correctly adjusts the height on the mbus, but
the HDMI component got left behind.
Adjust the mbus height to correctly describe the image height of the
fields when processing interlaced video for HDMI pipelines.
Fixes: 3e89586a64df ("media: i2c: adv748x: add adv748x driver")
Reviewed-by: Niklas Söderlund
Signed-off-by: Kieran Bingham
---
v2:
- switch conditional to check the fmt->field, removing the need for
the comment.
drivers/media/i2c/adv748x/adv748x-hdmi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c
b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index 4da4253553fc..10d229a4f088 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -105,6 +105,9 @@ static void adv748x_hdmi_fill_format(struct adv748x_hdmi
*hdmi,
fmt->width = hdmi->timings.bt.width;
fmt->height = hdmi->timings.bt.height;
+
+ if (fmt->field == V4L2_FIELD_ALTERNATE)
+ fmt->height /= 2;
}
static void adv748x_fill_optional_dv_timings(struct v4l2_dv_timings *timings)
--
2.7.4
[RFT PATCH v2 1/6] uvcvideo: Refactor URB descriptors
We currently store three separate arrays for each URB reference we hold.
Objectify the data needed to track URBs into a single uvc_urb structure,
allowing better object management and tracking of the URB.
All accesses to the data pointers through stream, are converted to use a
uvc_urb pointer for consistency.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- Re-describe URB context structure
- Re-name uvc_urb->{urb_buffer,urb_dma}{buffer,dma}
drivers/media/usb/uvc/uvc_video.c | 49 +++-
drivers/media/usb/uvc/uvcvideo.h | 18 ++--
2 files changed, 45 insertions(+), 22 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 73cd44e8bd81..e57c5f52c73b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1357,14 +1357,16 @@ static void uvc_free_urb_buffers(struct uvc_streaming
*stream)
unsigned int i;
for (i = 0; i < UVC_URBS; ++i) {
- if (stream->urb_buffer[i]) {
+ struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+
+ if (uvc_urb->buffer) {
#ifndef CONFIG_DMA_NONCOHERENT
usb_free_coherent(stream->dev->udev, stream->urb_size,
- stream->urb_buffer[i], stream->urb_dma[i]);
+ uvc_urb->buffer, uvc_urb->dma);
#else
- kfree(stream->urb_buffer[i]);
+ kfree(uvc_urb->buffer);
#endif
- stream->urb_buffer[i] = NULL;
+ uvc_urb->buffer = NULL;
}
}
@@ -1402,16 +1404,18 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
*stream,
/* Retry allocations until one succeed. */
for (; npackets > 1; npackets /= 2) {
for (i = 0; i < UVC_URBS; ++i) {
+ struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+
stream->urb_size = psize * npackets;
#ifndef CONFIG_DMA_NONCOHERENT
- stream->urb_buffer[i] = usb_alloc_coherent(
+ uvc_urb->buffer = usb_alloc_coherent(
stream->dev->udev, stream->urb_size,
- gfp_flags | __GFP_NOWARN, &stream->urb_dma[i]);
+ gfp_flags | __GFP_NOWARN, &uvc_urb->dma);
#else
- stream->urb_buffer[i] =
+ uvc_urb->buffer =
kmalloc(stream->urb_size, gfp_flags | __GFP_NOWARN);
#endif
- if (!stream->urb_buffer[i]) {
+ if (!uvc_urb->buffer) {
uvc_free_urb_buffers(stream);
break;
}
@@ -1441,13 +1445,15 @@ static void uvc_uninit_video(struct uvc_streaming
*stream, int free_buffers)
uvc_video_stats_stop(stream);
for (i = 0; i < UVC_URBS; ++i) {
- urb = stream->urb[i];
+ struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+
+ urb = uvc_urb->urb;
if (urb == NULL)
continue;
usb_kill_urb(urb);
usb_free_urb(urb);
- stream->urb[i] = NULL;
+ uvc_urb->urb = NULL;
}
if (free_buffers)
@@ -1502,6 +1508,8 @@ static int uvc_init_video_isoc(struct uvc_streaming
*stream,
size = npackets * psize;
for (i = 0; i < UVC_URBS; ++i) {
+ struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+
urb = usb_alloc_urb(npackets, gfp_flags);
if (urb == NULL) {
uvc_uninit_video(stream, 1);
@@ -1514,12 +1522,12 @@ static int uvc_init_video_isoc(struct uvc_streaming
*stream,
ep->desc.bEndpointAddress);
#ifndef CONFIG_DMA_NONCOHERENT
urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
- urb->transfer_dma = stream->urb_dma[i];
+ urb->transfer_dma = uvc_urb->dma;
#else
urb->transfer_flags = URB_ISO_ASAP;
#endif
urb->interval = ep->desc.bInterval;
- urb->transfer_buffer = stream->urb_buffer[i];
+ urb->transfer_buffer = uvc_urb->buffer;
urb->complete = uvc_video_complete;
urb->number_of_packets = npackets;
urb->transfer_buffer_length = size;
@@ -1529,7 +1537,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
*stream,
urb->iso_frame_desc[j].length = psize;
}
- stream->urb[i] = urb;
+ uvc_urb->urb = urb;
}
[RFT PATCH v2 5/6] uvcvideo: queue: Support asynchronous buffer handling
The buffer queue interface currently operates sequentially, processing
buffers after they have fully completed.
In preparation for supporting parallel tasks operating on the buffers,
we will need to support buffers being processed on multiple CPUs.
Adapt the uvc_queue_next_buffer() such that a reference count tracks the
active use of the buffer, returning the buffer to the VB2 stack at
completion.
Signed-off-by: Kieran Bingham
---
drivers/media/usb/uvc/uvc_queue.c | 61 ++--
drivers/media/usb/uvc/uvcvideo.h | 4 ++-
2 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_queue.c
b/drivers/media/usb/uvc/uvc_queue.c
index ddac4d89a291..5a9987e547d3 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -131,6 +131,7 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
spin_lock_irqsave(&queue->irqlock, flags);
if (likely(!(queue->flags & UVC_QUEUE_DISCONNECTED))) {
+ kref_init(&buf->ref);
list_add_tail(&buf->queue, &queue->irqqueue);
} else {
/* If the device is disconnected return the buffer to userspace
@@ -424,28 +425,66 @@ struct uvc_buffer *uvc_queue_get_current_buffer(struct
uvc_video_queue *queue)
return nextbuf;
}
-struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
+/*
+ * uvc_queue_requeue: Requeue a buffer on our internal irqqueue
+ *
+ * Reuse a buffer through our internal queue without the need to 'prepare'
+ * The buffer will be returned to userspace through the uvc_buffer_queue call
if
+ * the device has been disconnected
+ */
+static void uvc_queue_requeue(struct uvc_video_queue *queue,
struct uvc_buffer *buf)
{
- struct uvc_buffer *nextbuf;
- unsigned long flags;
+ buf->error = 0;
+ buf->state = UVC_BUF_STATE_QUEUED;
+ buf->bytesused = 0;
+ vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
+
+ uvc_buffer_queue(&buf->buf.vb2_buf);
+}
+
+static void uvc_queue_buffer_complete(struct kref *ref)
+{
+ struct uvc_buffer *buf = container_of(ref, struct uvc_buffer, ref);
+ struct vb2_buffer *vb = &buf->buf.vb2_buf;
+ struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
- buf->error = 0;
- buf->state = UVC_BUF_STATE_QUEUED;
- buf->bytesused = 0;
- vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
- return buf;
+ uvc_queue_requeue(queue, buf);
+ return;
}
+ buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
+ vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
+ vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+/*
+ * Release a reference on the buffer. Complete the buffer when the last
+ * reference is released
+ */
+void uvc_queue_buffer_release(struct uvc_buffer *buf)
+{
+ kref_put(&buf->ref, uvc_queue_buffer_complete);
+}
+
+/*
+ * Remove this buffer from the queue. Lifetime will persist while async actions
+ * are still running (if any), and uvc_queue_buffer_release will give the
buffer
+ * back to VB2 when all users have completed.
+ */
+struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
+ struct uvc_buffer *buf)
+{
+ struct uvc_buffer *nextbuf;
+ unsigned long flags;
+
spin_lock_irqsave(&queue->irqlock, flags);
list_del(&buf->queue);
nextbuf = __uvc_queue_get_current_buffer(queue);
spin_unlock_irqrestore(&queue->irqlock, flags);
- buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
- vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
- vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
+ uvc_queue_buffer_release(buf);
return nextbuf;
}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 5caa1f4de3cb..6a18dbfc3e5b 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -404,6 +404,9 @@ struct uvc_buffer {
unsigned int bytesused;
u32 pts;
+
+ /* asynchronous buffer handling */
+ struct kref ref;
};
#define UVC_QUEUE_DISCONNECTED (1 << 0)
@@ -696,6 +699,7 @@ extern struct uvc_buffer *
uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
extern struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
struct uvc_buffer *buf);
+extern void uvc_queue_buffer_release(struct uvc_buffer *buf);
extern int uvc_queue_mmap(struct uvc_video_queue *queue,
struct vm_area_struct *vma);
extern unsigned int uvc_queue_poll(struct uvc_video_queue *queue,
--
git-series 0.9.1
[RFT PATCH v2 6/6] uvcvideo: Move decode processing to process context
Newer high definition cameras, and cameras with multiple lenses such as
the range of stereo-vision cameras now available have ever increasing
data rates.
The inclusion of a variable length packet header in URB packets mean
that we must memcpy the frame data out to our destination 'manually'.
This can result in data rates of up to 2 gigabits per second being
processed.
To improve efficiency, and maximise throughput, handle the URB decode
processing through a work queue to move it from interrupt context, and
allow multiple processors to work on URBs in parallel.
Signed-off-by: Kieran Bingham
---
v2:
- Lock full critical section of usb_submit_urb()
drivers/media/usb/uvc/uvc_queue.c | 12 +++-
drivers/media/usb/uvc/uvc_video.c | 111 +--
drivers/media/usb/uvc/uvcvideo.h | 24 +++-
3 files changed, 129 insertions(+), 18 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_queue.c
b/drivers/media/usb/uvc/uvc_queue.c
index 5a9987e547d3..598087eeb5c2 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -179,10 +179,22 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
struct uvc_streaming *stream = uvc_queue_to_stream(queue);
+ /* Prevent new buffers coming in. */
+ spin_lock_irq(&queue->irqlock);
+ queue->flags |= UVC_QUEUE_STOPPING;
+ spin_unlock_irq(&queue->irqlock);
+
+ /*
+* All pending work should be completed before disabling the stream, as
+* all URBs will be free'd during uvc_video_enable(s, 0).
+*/
+ flush_workqueue(stream->async_wq);
+
uvc_video_enable(stream, 0);
spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
+ queue->flags &= ~UVC_QUEUE_STOPPING;
spin_unlock_irq(&queue->irqlock);
}
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 3878bec3276e..a9ddc4f27012 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1058,21 +1058,67 @@ static int uvc_video_decode_start(struct uvc_streaming
*stream,
return data[0];
}
-static void uvc_video_decode_data(struct uvc_streaming *stream,
- struct uvc_buffer *buf, const __u8 *data, int len)
+/*
+ * uvc_video_decode_data_work: Asynchronous memcpy processing
+ *
+ * Perform memcpy tasks in process context, with completion handlers
+ * to return the URB, and buffer handles.
+ *
+ * The work submitter must pre-determine that the work is safe
+ */
+static void uvc_video_decode_data_work(struct work_struct *work)
{
- unsigned int maxlen, nbytes;
- void *mem;
+ struct uvc_urb *uvc_urb = container_of(work, struct uvc_urb, work);
+ struct uvc_streaming *stream = uvc_urb->stream;
+ struct uvc_video_queue *queue = &stream->queue;
+ unsigned int i;
+ int ret;
+
+ for (i = 0; i < uvc_urb->packets; i++) {
+ struct uvc_decode_op *op = &uvc_urb->decodes[i];
+
+ memcpy(op->dst, op->src, op->len);
+
+ /* Release reference taken on this buffer */
+ uvc_queue_buffer_release(op->buf);
+ }
+
+ /*
+* Prevent resubmitting URBs when shutting down to ensure that no new
+* work item will be scheduled after uvc_stop_streaming() flushes the
+* work queue.
+*/
+ spin_lock_irq(&queue->irqlock);
+ if (!(queue->flags & UVC_QUEUE_STOPPING)) {
+ ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
+ if (ret < 0)
+ uvc_printk(KERN_ERR,
+ "Failed to resubmit video URB (%d).\n",
+ ret);
+ }
+ spin_unlock_irq(&queue->irqlock);
+}
+
+static void uvc_video_decode_data(struct uvc_decode_op *decode,
+ struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
+ const __u8 *data, int len)
+{
+ unsigned int maxlen;
if (len <= 0)
return;
- /* Copy the video data to the buffer. */
maxlen = buf->length - buf->bytesused;
- mem = buf->mem + buf->bytesused;
- nbytes = min((unsigned int)len, maxlen);
- memcpy(mem, data, nbytes);
- buf->bytesused += nbytes;
+
+ /* Take a buffer reference for async work */
+ kref_get(&buf->ref);
+
+ decode->buf = buf;
+ decode->src = data;
+ decode->dst = buf->mem + buf->bytesused;
+ decode->len = min_t(unsigned int, len, maxlen);
+
+ buf->bytesused += decode->len;
/* Complete the current frame if the buffer size was exceeded. */
if (len > maxlen) {
@@ -1080,6 +1126,8 @@ sta
[RFT PATCH v2 3/6] uvcvideo: Protect queue internals with helper
The URB completion operation obtains the current buffer by reading
directly into the queue internal interface.
Protect this queue abstraction by providing a helper
uvc_queue_get_current_buffer() which can be used by both the decode
task, and the uvc_queue_next_buffer() functions.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- Fix coding style of conditional statements
drivers/media/usb/uvc/uvc_queue.c | 33 +++-
drivers/media/usb/uvc/uvc_video.c | 7 +--
drivers/media/usb/uvc/uvcvideo.h | 2 ++-
3 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_queue.c
b/drivers/media/usb/uvc/uvc_queue.c
index c8d78b2f3de4..4a581d631525 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -399,6 +399,33 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int
disconnect)
spin_unlock_irqrestore(&queue->irqlock, flags);
}
+/*
+ * uvc_queue_get_current_buffer: Obtain the current working output buffer
+ *
+ * Buffers may span multiple packets, and even URBs, therefore the active
buffer
+ * remains on the queue until the EOF marker.
+ */
+static struct uvc_buffer *
+__uvc_queue_get_current_buffer(struct uvc_video_queue *queue)
+{
+ if (list_empty(&queue->irqqueue))
+ return NULL;
+
+ return list_first_entry(&queue->irqqueue, struct uvc_buffer, queue);
+}
+
+struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue)
+{
+ struct uvc_buffer *nextbuf;
+ unsigned long flags;
+
+ spin_lock_irqsave(&queue->irqlock, flags);
+ nextbuf = __uvc_queue_get_current_buffer(queue);
+ spin_unlock_irqrestore(&queue->irqlock, flags);
+
+ return nextbuf;
+}
+
struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
struct uvc_buffer *buf)
{
@@ -415,11 +442,7 @@ struct uvc_buffer *uvc_queue_next_buffer(struct
uvc_video_queue *queue,
spin_lock_irqsave(&queue->irqlock, flags);
list_del(&buf->queue);
- if (!list_empty(&queue->irqqueue))
- nextbuf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
- queue);
- else
- nextbuf = NULL;
+ nextbuf = __uvc_queue_get_current_buffer(queue);
spin_unlock_irqrestore(&queue->irqlock, flags);
buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 92bd0952a66e..3878bec3276e 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1322,7 +1322,6 @@ static void uvc_video_complete(struct urb *urb)
struct uvc_streaming *stream = uvc_urb->stream;
struct uvc_video_queue *queue = &stream->queue;
struct uvc_buffer *buf = NULL;
- unsigned long flags;
int ret;
switch (urb->status) {
@@ -1343,11 +1342,7 @@ static void uvc_video_complete(struct urb *urb)
return;
}
- spin_lock_irqsave(&queue->irqlock, flags);
- if (!list_empty(&queue->irqqueue))
- buf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
- queue);
- spin_unlock_irqrestore(&queue->irqlock, flags);
+ buf = uvc_queue_get_current_buffer(queue);
stream->decode(uvc_urb, buf);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 81a9a419a423..5caa1f4de3cb 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -692,6 +692,8 @@ extern int uvc_queue_streamon(struct uvc_video_queue *queue,
extern int uvc_queue_streamoff(struct uvc_video_queue *queue,
enum v4l2_buf_type type);
extern void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect);
+extern struct uvc_buffer *
+ uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
extern struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
struct uvc_buffer *buf);
extern int uvc_queue_mmap(struct uvc_video_queue *queue,
--
git-series 0.9.1
[RFT PATCH v2 2/6] uvcvideo: Convert decode functions to use new context structure
The URB completion handlers currently reference the stream context.
Now that each URB has its own context structure, convert the decode (and
one encode) functions to utilise this context for URB management.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- fix checkpatch warning (pre-existing in code)
drivers/media/usb/uvc/uvc_isight.c | 4 +++-
drivers/media/usb/uvc/uvc_video.c | 32 ---
drivers/media/usb/uvc/uvcvideo.h | 8
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_isight.c
b/drivers/media/usb/uvc/uvc_isight.c
index 8510e7259e76..433b8b4f96e2 100644
--- a/drivers/media/usb/uvc/uvc_isight.c
+++ b/drivers/media/usb/uvc/uvc_isight.c
@@ -99,9 +99,11 @@ static int isight_decode(struct uvc_video_queue *queue,
struct uvc_buffer *buf,
return 0;
}
-void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
+void uvc_video_decode_isight(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf)
{
+ struct urb *urb = uvc_urb->urb;
+ struct uvc_streaming *stream = uvc_urb->stream;
int ret, i;
for (i = 0; i < urb->number_of_packets; ++i) {
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index e57c5f52c73b..92bd0952a66e 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1153,9 +1153,11 @@ static void uvc_video_validate_buffer(const struct
uvc_streaming *stream,
/*
* Completion handler for video URBs.
*/
-static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
*stream,
- struct uvc_buffer *buf)
+static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
+ struct uvc_buffer *buf)
{
+ struct urb *urb = uvc_urb->urb;
+ struct uvc_streaming *stream = uvc_urb->stream;
u8 *mem;
int ret, i;
@@ -1199,9 +1201,11 @@ static void uvc_video_decode_isoc(struct urb *urb,
struct uvc_streaming *stream,
}
}
-static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming
*stream,
- struct uvc_buffer *buf)
+static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
+ struct uvc_buffer *buf)
{
+ struct urb *urb = uvc_urb->urb;
+ struct uvc_streaming *stream = uvc_urb->stream;
u8 *mem;
int len, ret;
@@ -1266,9 +1270,12 @@ static void uvc_video_decode_bulk(struct urb *urb,
struct uvc_streaming *stream,
}
}
-static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming
*stream,
- struct uvc_buffer *buf)
+static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
+ struct uvc_buffer *buf)
{
+ struct urb *urb = uvc_urb->urb;
+ struct uvc_streaming *stream = uvc_urb->stream;
+
u8 *mem = urb->transfer_buffer;
int len = stream->urb_size, ret;
@@ -1311,7 +1318,8 @@ static void uvc_video_encode_bulk(struct urb *urb, struct
uvc_streaming *stream,
static void uvc_video_complete(struct urb *urb)
{
- struct uvc_streaming *stream = urb->context;
+ struct uvc_urb *uvc_urb = urb->context;
+ struct uvc_streaming *stream = uvc_urb->stream;
struct uvc_video_queue *queue = &stream->queue;
struct uvc_buffer *buf = NULL;
unsigned long flags;
@@ -1341,7 +1349,7 @@ static void uvc_video_complete(struct urb *urb)
queue);
spin_unlock_irqrestore(&queue->irqlock, flags);
- stream->decode(urb, stream, buf);
+ stream->decode(uvc_urb, buf);
if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
@@ -1419,6 +1427,8 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
*stream,
uvc_free_urb_buffers(stream);
break;
}
+
+ uvc_urb->stream = stream;
}
if (i == UVC_URBS) {
@@ -1517,7 +1527,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
*stream,
}
urb->dev = stream->dev->udev;
- urb->context = stream;
+ urb->context = uvc_urb;
urb->pipe = usb_rcvisocpipe(stream->dev->udev,
ep->desc.bEndpointAddress);
#ifndef CONFIG_DMA_NONCOHERENT
@@ -1584,8 +1594,8 @@ static int uvc_init_video_bulk(struct uvc_streaming
*stream,
return -ENOMEM;
}
- usb_fill_bulk_urb(urb, stream->dev->udev, pipe, uvc_urb->buffer,
- size, uvc_video_complete, stream);
+ usb_fill_bulk_urb(urb, stream->dev->udev, pipe, uvc_urb->buffer,
+
[RFT PATCH v2 4/6] uvcvideo: queue: Simplify spin-lock usage
Both uvc_start_streaming(), and uvc_stop_streaming() are called from
userspace context. As such, they do not need to save the IRQ state, and
can use spin_lock_irq() and spin_unlock_irq() respectively.
Signed-off-by: Kieran Bingham
---
drivers/media/usb/uvc/uvc_queue.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_queue.c
b/drivers/media/usb/uvc/uvc_queue.c
index 4a581d631525..ddac4d89a291 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -158,7 +158,6 @@ static int uvc_start_streaming(struct vb2_queue *vq,
unsigned int count)
{
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
struct uvc_streaming *stream = uvc_queue_to_stream(queue);
- unsigned long flags;
int ret;
queue->buf_used = 0;
@@ -167,9 +166,9 @@ static int uvc_start_streaming(struct vb2_queue *vq,
unsigned int count)
if (ret == 0)
return 0;
- spin_lock_irqsave(&queue->irqlock, flags);
+ spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
- spin_unlock_irqrestore(&queue->irqlock, flags);
+ spin_unlock_irq(&queue->irqlock);
return ret;
}
@@ -178,13 +177,12 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
{
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
struct uvc_streaming *stream = uvc_queue_to_stream(queue);
- unsigned long flags;
uvc_video_enable(stream, 0);
- spin_lock_irqsave(&queue->irqlock, flags);
+ spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
- spin_unlock_irqrestore(&queue->irqlock, flags);
+ spin_unlock_irq(&queue->irqlock);
}
static const struct vb2_ops uvc_queue_qops = {
--
git-series 0.9.1
[RFT PATCH v2 0/6] Asynchronous UVC
The Linux UVC driver has long provided adequate performance capabilities for web-cams and low data rate video devices in Linux while resolutions were low. Modern USB cameras are now capable of high data rates thanks to USB3 with 1080p, and even 4k capture resolutions supported. Cameras such as the Stereolabs ZED or the Logitech Brio can generate more data than an embedded ARM core is able to process on a single core, resulting in frame loss. A large part of this performance impact is from the requirement to ‘memcpy’ frames out from URB packets to destination frames. This unfortunate requirement is due to the UVC protocol allowing a variable length header, and thus it is not possible to provide the target frame buffers directly. Extra throughput is possible by moving the actual memcpy actions to a work queue, and moving the memcpy out of interrupt context and allowing work tasks to be scheduled across multiple cores. 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. v2: - Fix up comments and issues raised by Guennadi Kieran Bingham (6): uvcvideo: Refactor URB descriptors uvcvideo: Convert decode functions to use new context structure uvcvideo: Protect queue internals with helper uvcvideo: queue: Simplify spin-lock usage uvcvideo: queue: Support asynchronous buffer handling uvcvideo: Move decode processing to process context drivers/media/usb/uvc/uvc_isight.c | 4 +- drivers/media/usb/uvc/uvc_queue.c | 114 ++ drivers/media/usb/uvc/uvc_video.c | 189 ++ drivers/media/usb/uvc/uvcvideo.h | 56 +++-- 4 files changed, 285 insertions(+), 78 deletions(-) base-commit: 6f0e5fd39143a59c22d60e7befc4f33f22aeed2f -- git-series 0.9.1
Re: [RFT PATCH v3 0/6] Asynchronous UVC
Hi Phillip On 15/01/18 19:35, Philipp Zabel wrote: > 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). Thank you, that is very much appreciated! I presume that was on x86_64? -- Regards Kieran
[PATCH 10/11] media: vsp1: Support Interlaced display pipelines
Calculate the top and bottom fields for the interlaced frames and
utilise the extended display list command feature to implement the
auto-field operations. This allows the DU to update the VSP2 registers
dynamically based upon the currently processing field.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 10 +++-
drivers/media/platform/vsp1/vsp1_dl.h | 2 +-
drivers/media/platform/vsp1/vsp1_drm.c | 12 -
drivers/media/platform/vsp1/vsp1_pipe.c | 3 +-
drivers/media/platform/vsp1/vsp1_regs.h | 1 +-
drivers/media/platform/vsp1/vsp1_rpf.c | 72 --
drivers/media/platform/vsp1/vsp1_rwpf.h | 1 +-
include/media/vsp1.h| 1 +-
8 files changed, 96 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 6d17b8bfa21c..4a079060864b 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -886,8 +886,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
* with the frame end interrupt. The function always returns true in header
mode
* as display list processing is then not continuous and races never occur.
*/
-bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
+bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm, bool interlaced)
{
+ struct vsp1_device *vsp1 = dlm->vsp1;
bool completed = false;
spin_lock(&dlm->lock);
@@ -912,6 +913,13 @@ bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
if (vsp1_dl_list_hw_update_pending(dlm))
goto done;
+ if (interlaced) {
+ u32 status = vsp1_read(vsp1, VI6_STATUS);
+
+ if (!(status & VI6_STATUS_FLD_STD(dlm->index)))
+ goto done;
+ }
+
/*
* The device starts processing the queued display list right after the
* frame end interrupt. The display list thus becomes active.
diff --git a/drivers/media/platform/vsp1/vsp1_dl.h
b/drivers/media/platform/vsp1/vsp1_dl.h
index 3009912ddefb..24a9fb53223a 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.h
+++ b/drivers/media/platform/vsp1/vsp1_dl.h
@@ -57,7 +57,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device
*vsp1,
unsigned int prealloc);
void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm);
void vsp1_dlm_reset(struct vsp1_dl_manager *dlm);
-bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
+bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm, bool interlaced);
struct vsp1_dl_ext_cmd *vsp1_dlm_get_autofld_cmd(struct vsp1_dl_list *dl);
struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
b/drivers/media/platform/vsp1/vsp1_drm.c
index 0459b970e9da..d7028de053ae 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -329,6 +329,7 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int
pipe_index,
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
const struct vsp1_format_info *fmtinfo;
+ struct vsp1_rwpf *output = drm_pipe->pipe.output;
struct vsp1_rwpf *rpf;
if (rpf_index >= vsp1->info->rpf_count)
@@ -368,6 +369,17 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int
pipe_index,
return -EINVAL;
}
+ if (!(vsp1_feature(vsp1, VSP1_HAS_EXT_DL)) && cfg->interlaced) {
+ /*
+* Interlaced support requires extended display lists to
+* provide the auto-fld feature with the DU.
+*/
+ dev_dbg(vsp1->dev, "Interlaced unsupported on this output\n");
+ return -EINVAL;
+ }
+
+ rpf->interlaced = output->interlaced = cfg->interlaced;
+
rpf->fmtinfo = fmtinfo;
rpf->format.num_planes = fmtinfo->planes;
rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
b/drivers/media/platform/vsp1/vsp1_pipe.c
index fa445b1a2e38..df674b3bb9a0 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -341,7 +341,8 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
* up being postponed by one frame. @completed represents whether the
* active frame was finished or postponed.
*/
- completed = vsp1_dlm_irq_frame_end(pipe->output->dlm);
+ completed = vsp1_dlm_irq_frame_end(pipe->output->dlm,
+ pipe->output->interlaced);
if (pipe->hgo)
vsp1_hgo_frame_end(pipe->hgo);
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h
b/drivers/media/platform/vsp1/
[PATCH 09/11] media: vsp1: Provide support for extended command pools
VSPD and VSP-DL devices can provide extended display lists supporting
extended command display list objects.
These extended commands require their own dma memory areas for a header
and body specific to the command type.
Implement a command pool to allocate all necessary memory in a single
DMA allocation to reduce pressure on the TLB, and provide convenvient
re-usable command objects for the entities to utilise.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 189 +++-
drivers/media/platform/vsp1/vsp1_dl.h | 3 +-
2 files changed, 192 insertions(+)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 36440a2a2c8b..6d17b8bfa21c 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -121,6 +121,30 @@ struct vsp1_dl_body_pool {
};
/**
+ * struct vsp1_cmd_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_cmd_pool {
+ /* DMA allocation */
+ dma_addr_t dma;
+ size_t size;
+ void *mem;
+
+ struct vsp1_dl_ext_cmd *cmds;
+ 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
@@ -176,6 +200,7 @@ struct vsp1_dl_manager {
struct vsp1_dl_list *pending;
struct vsp1_dl_body_pool *pool;
+ struct vsp1_dl_cmd_pool *autfld_cmds;
};
/*
-
@@ -339,6 +364,139 @@ void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32
reg, u32 data)
}
/*
-
+ * Display List Extended Command Management
+ */
+
+enum vsp1_extcmd_type {
+ VSP1_EXTCMD_AUTODISP,
+ VSP1_EXTCMD_AUTOFLD,
+};
+
+struct vsp1_extended_command_info {
+ u16 opcode;
+ size_t body_size;
+} vsp1_extended_commands[] = {
+ [VSP1_EXTCMD_AUTODISP] = { 0x02, 96 },
+ [VSP1_EXTCMD_AUTOFLD] = { 0x03, 160 },
+};
+
+/**
+ * vsp1_dl_cmd_pool_create - Create a pool of commands from a single allocation
+ * @vsp1: The VSP1 device
+ * @type: The command pool type
+ * @num_commands: The quantity of commands to allocate
+ *
+ * Allocate a pool of commands each with enough memory to contain the private
+ * data of each command. The allocation sizes are dependent upon the command
+ * type.
+ *
+ * Return a pointer to a pool on success or NULL if memory can't be allocated.
+ */
+struct vsp1_dl_cmd_pool *
+vsp1_dl_cmd_pool_create(struct vsp1_device *vsp1, enum vsp1_extcmd_type type,
+ unsigned int num_cmds)
+{
+ struct vsp1_dl_cmd_pool *pool;
+ unsigned int i;
+ size_t cmd_size;
+
+ pool = kzalloc(sizeof(*pool), GFP_KERNEL);
+ if (!pool)
+ return NULL;
+
+ pool->cmds = kcalloc(num_cmds, sizeof(*pool->cmds), GFP_KERNEL);
+ if (!pool->cmds) {
+ kfree(pool);
+ return NULL;
+ }
+
+ cmd_size = sizeof(struct vsp1_dl_ext_cmd_header) +
+ vsp1_extended_commands[type].body_size;
+ cmd_size = ALIGN(cmd_size, 16);
+
+ pool->size = cmd_size * num_cmds;
+ pool->mem = dma_alloc_wc(vsp1->bus_master, pool->size, &pool->dma,
+GFP_KERNEL);
+ if (!pool->mem) {
+ kfree(pool->cmds);
+ kfree(pool);
+ return NULL;
+ }
+
+ spin_lock_init(&pool->lock);
+ INIT_LIST_HEAD(&pool->free);
+
+ for (i = 0; i < num_cmds; ++i) {
+ struct vsp1_dl_ext_cmd *cmd = &pool->cmds[i];
+ size_t cmd_offset = i * cmd_size;
+ size_t data_offset = sizeof(struct vsp1_dl_ext_cmd_header) +
+cmd_offset;
+
+ cmd->pool = pool;
+ cmd->cmd_opcode = vsp1_extended_commands[type].opcode;
+
+ /* TODO: Auto-disp can utilise more than one command per cmd */
+ cmd->num_cmds = 1;
+ cmd->cmds = pool->mem + cmd_offset;
+ cmd->cmd_dma = pool->dma + cmd_offset;
+
+ cmd->data = pool->mem + data_offset;
+ cmd->data_dma = pool->dma + data_offset;
+ cmd->data_size = vsp1_extended_commands[type].body_size;
+
+ list_add_tail(&cmd->free, &pool->free);
+ }
+
+ return pool;
+}
+
+struct vsp1_dl_ext_cmd *vsp1_dl_ext_
[PATCH 11/11] drm: rcar-du: Support interlaced video output through vsp1
Use the newly exposed VSP1 interface to enable interlaced frame support through the VSP1 lif pipelines. Signed-off-by: Kieran Bingham --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 1 + drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5685d5af6998..9854d9deb944 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -248,6 +248,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) /* Signal polarities */ value = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0) | ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? DSMR_HSL : 0) + | ((mode->flags & DRM_MODE_FLAG_INTERLACE) ? DSMR_ODEV : 0) | DSMR_DIPM_DISP | DSMR_CSPM; rcar_du_crtc_write(rcrtc, DSMR, value); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c33840b..5e47daef8bd2 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -178,6 +178,9 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane) }; unsigned int i; + cfg.interlaced = !!(plane->plane.state->crtc->mode.flags + & DRM_MODE_FLAG_INTERLACE); + cfg.src.left = state->state.src.x1 >> 16; cfg.src.top = state->state.src.y1 >> 16; cfg.src.width = drm_rect_width(&state->state.src) >> 16; -- git-series 0.9.1
[PATCH 04/11] media: vsp1: Remove unused display list structure field
The vsp1 reference in the vsp1_dl_body structure is not used.
Remove it.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 057f0f093222..16a2d3c93655 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -48,7 +48,6 @@ struct vsp1_dl_entry {
* @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
* @size: size of the DMA memory in bytes
@@ -62,7 +61,6 @@ struct vsp1_dl_body {
refcount_t refcnt;
struct vsp1_dl_body_pool *pool;
- struct vsp1_device *vsp1;
struct vsp1_dl_entry *entries;
dma_addr_t dma;
--
git-series 0.9.1
[PATCH 05/11] media: vsp1: Clean up DLM objects on error
If there is an error allocating a display list within a DLM object
the existing display lists are not free'd, and neither is the DL body
pool.
Use the existing vsp1_dlm_destroy() function to clean up on error.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 16a2d3c93655..6271bea5e831 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -831,8 +831,10 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device
*vsp1,
struct vsp1_dl_list *dl;
dl = vsp1_dl_list_alloc(dlm, dlm->pool);
- if (!dl)
+ if (!dl) {
+ vsp1_dlm_destroy(dlm);
return NULL;
+ }
list_add_tail(&dl->list, &dlm->free);
}
--
git-series 0.9.1
[PATCH 08/11] media: vsp1: Add support for extended display list headers
Extended display list headers allow pre and post command lists to be
executed by the VSP pipeline. This provides the base support for
features such as AUTO_FLD (for interlaced support) and AUTO_DISP (for
supporting continuous camera preview pipelines.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1.h | 1 +-
drivers/media/platform/vsp1/vsp1_dl.c | 83 +-
drivers/media/platform/vsp1/vsp1_dl.h | 29 -
drivers/media/platform/vsp1/vsp1_drv.c | 7 +-
drivers/media/platform/vsp1/vsp1_regs.h | 5 +-
5 files changed, 116 insertions(+), 9 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1.h
b/drivers/media/platform/vsp1/vsp1.h
index 1c080538c993..bb3b32795206 100644
--- a/drivers/media/platform/vsp1/vsp1.h
+++ b/drivers/media/platform/vsp1/vsp1.h
@@ -55,6 +55,7 @@ struct vsp1_uds;
#define VSP1_HAS_HGO (1 << 7)
#define VSP1_HAS_HGT (1 << 8)
#define VSP1_HAS_BRS (1 << 9)
+#define VSP1_HAS_EXT_DL(1 << 10)
struct vsp1_device_info {
u32 version;
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index dc273e3b4753..36440a2a2c8b 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -26,6 +26,9 @@
#define VSP1_DLH_INT_ENABLE(1 << 1)
#define VSP1_DLH_AUTO_START(1 << 0)
+#define VSP1_DLH_EXT_PRE_CMD_EXEC (1 << 9)
+#define VSP1_DLH_EXT_POST_CMD_EXEC (1 << 8)
+
struct vsp1_dl_header_list {
u32 num_bytes;
u32 addr;
@@ -38,11 +41,34 @@ struct vsp1_dl_header {
u32 flags;
} __packed;
+struct vsp1_dl_ext_header {
+ u32 reserved0; /* alignment padding */
+
+ u16 pre_ext_cmd_qty;
+ u16 flags;
+ u32 pre_ext_cmd_plist;
+
+ u32 post_ext_cmd_qty;
+ u32 post_ext_cmd_plist;
+} __packed;
+
+struct vsp1_dl_header_extended {
+ struct vsp1_dl_header header;
+ struct vsp1_dl_ext_header ext;
+} __packed;
+
struct vsp1_dl_entry {
u32 addr;
u32 data;
} __packed;
+struct vsp1_dl_ext_cmd_header {
+ u32 cmd;
+ u32 flags;
+ u32 data;
+ u32 reserved;
+} __packed;
+
/**
* struct vsp1_dl_body - Display list body
* @list: entry in the display list list of bodies
@@ -99,9 +125,12 @@ struct vsp1_dl_body_pool {
* @list: entry in the display list manager lists
* @dlm: the display list manager
* @header: display list header
+ * @extended: extended display list header. NULL for normal lists
* @dma: DMA address for the header
* @body0: first display list body
* @bodies: list of extra display list bodies
+ * @pre_cmd: pre cmd to be issued through extended dl header
+ * @post_cmd: post cmd to be issued through extended dl header
* @has_chain: if true, indicates that there's a partition chain
* @chain: entry in the display list partition chain
*/
@@ -110,11 +139,15 @@ struct vsp1_dl_list {
struct vsp1_dl_manager *dlm;
struct vsp1_dl_header *header;
+ struct vsp1_dl_ext_header *extended;
dma_addr_t dma;
struct vsp1_dl_body *body0;
struct list_head bodies;
+ struct vsp1_dl_ext_cmd *pre_cmd;
+ struct vsp1_dl_ext_cmd *post_cmd;
+
bool has_chain;
struct list_head chain;
};
@@ -498,6 +531,14 @@ int vsp1_dl_list_add_chain(struct vsp1_dl_list *head,
return 0;
}
+static void vsp1_dl_ext_cmd_fill_header(struct vsp1_dl_ext_cmd *cmd)
+{
+ cmd->cmds[0].cmd = cmd->cmd_opcode;
+ cmd->cmds[0].flags = cmd->flags;
+ cmd->cmds[0].data = cmd->data_dma;
+ cmd->cmds[0].reserved = 0;
+}
+
static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)
{
struct vsp1_dl_manager *dlm = dl->dlm;
@@ -550,6 +591,27 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list
*dl, bool is_last)
*/
dl->header->flags = VSP1_DLH_INT_ENABLE;
}
+
+ if (!dl->extended)
+ return;
+
+ dl->extended->flags = 0;
+
+ if (dl->pre_cmd) {
+ dl->extended->pre_ext_cmd_plist = dl->pre_cmd->cmd_dma;
+ dl->extended->pre_ext_cmd_qty = dl->pre_cmd->num_cmds;
+ dl->extended->flags |= VSP1_DLH_EXT_PRE_CMD_EXEC;
+
+ vsp1_dl_ext_cmd_fill_header(dl->pre_cmd);
+ }
+
+ if (dl->post_cmd) {
+ dl->extended->pre_ext_cmd_plist = dl->post_cmd->cmd_dma;
+ dl->extended->pre_ext_cmd_qty = dl->post_cmd->num_cmds;
+ dl->extended->flags |= VSP1_DLH_EXT_POST_CMD_EXEC;
+
+ vsp1_dl_ext_cmd_fill_header(dl->pre_cmd);
+ }
}
static bool vsp1_dl_list_hw_update_pending(struct vsp1_dl_manager *dlm)
@@ -715,14 +777,20 @@ bool vsp1_dl
[PATCH 07/11] media: vsp1: Use header display lists for all WPF outputs linked to the DU
Header mode display lists are now supported on all WPF outputs. To
support extended headers and auto-fld capabilities for interlaced mode
handling only header mode display lists can be used.
Disable the headerless display list configuration, and remove the dead
code.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 107 ++-
1 file changed, 27 insertions(+), 80 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 6271bea5e831..dc273e3b4753 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -98,7 +98,7 @@ struct vsp1_dl_body_pool {
* struct vsp1_dl_list - Display list
* @list: entry in the display list manager lists
* @dlm: the display list manager
- * @header: display list header, NULL for headerless lists
+ * @header: display list header
* @dma: DMA address for the header
* @body0: first display list body
* @bodies: list of extra display list bodies
@@ -119,15 +119,9 @@ struct vsp1_dl_list {
struct list_head chain;
};
-enum vsp1_dl_mode {
- VSP1_DL_MODE_HEADER,
- VSP1_DL_MODE_HEADERLESS,
-};
-
/**
* struct vsp1_dl_manager - Display List manager
* @index: index of the related WPF
- * @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_bodies lists
@@ -139,7 +133,6 @@ enum vsp1_dl_mode {
*/
struct vsp1_dl_manager {
unsigned int index;
- enum vsp1_dl_mode mode;
bool singleshot;
struct vsp1_device *vsp1;
@@ -320,6 +313,7 @@ static struct vsp1_dl_list *vsp1_dl_list_alloc(struct
vsp1_dl_manager *dlm,
struct vsp1_dl_body_pool *pool)
{
struct vsp1_dl_list *dl;
+ size_t header_offset;
dl = kzalloc(sizeof(*dl), GFP_KERNEL);
if (!dl)
@@ -332,16 +326,15 @@ static struct vsp1_dl_list *vsp1_dl_list_alloc(struct
vsp1_dl_manager *dlm,
dl->body0 = vsp1_dl_body_get(pool);
if (!dl->body0)
return NULL;
- if (dlm->mode == VSP1_DL_MODE_HEADER) {
- size_t header_offset = dl->body0->max_entries
-* sizeof(*dl->body0->entries);
- dl->header = ((void *)dl->body0->entries) + header_offset;
- dl->dma = dl->body0->dma + header_offset;
+ header_offset = dl->body0->max_entries
+* sizeof(*dl->body0->entries);
- memset(dl->header, 0, sizeof(*dl->header));
- dl->header->lists[0].addr = dl->body0->dma;
- }
+ dl->header = ((void *)dl->body0->entries) + header_offset;
+ dl->dma = dl->body0->dma + header_offset;
+
+ memset(dl->header, 0, sizeof(*dl->header));
+ dl->header->lists[0].addr = dl->body0->dma;
return dl;
}
@@ -473,16 +466,9 @@ struct vsp1_dl_body *vsp1_dl_list_get_body0(struct
vsp1_dl_list *dl)
*
* 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.
*/
int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct vsp1_dl_body *dlb)
{
- /* Multi-body lists are only available in header mode. */
- if (dl->dlm->mode != VSP1_DL_MODE_HEADER)
- return -EINVAL;
-
refcount_inc(&dlb->refcnt);
list_add_tail(&dlb->list, &dl->bodies);
@@ -503,17 +489,10 @@ int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct
vsp1_dl_body *dlb)
* Adding a display list to a chain passes ownership of the display list to
* the head display list item. The chain is released when the head dl item is
* put back with __vsp1_dl_list_put().
- *
- * Chained display lists are only usable in header mode. Attempts to add a
- * display list to a chain in header-less mode will return an error.
*/
int vsp1_dl_list_add_chain(struct vsp1_dl_list *head,
struct vsp1_dl_list *dl)
{
- /* Chained lists are only available in header mode. */
- if (head->dlm->mode != VSP1_DL_MODE_HEADER)
- return -EINVAL;
-
head->has_chain = true;
list_add_tail(&dl->chain, &head->chain);
return 0;
@@ -581,17 +560,10 @@ static bool vsp1_dl_list_hw_update_pending(struct
vsp1_dl_manager *dlm)
return false;
/*
-* Check whether the VSP1 has taken the update. In headerless mode the
-* hardware indicates this by clearing the UPD bit in the DL_BODY_SIZE
-* r
[PATCH 06/11] media: vsp1: Provide VSP1 feature helper macro
The VSP1 devices define their specific capabilities through features
marked in their device info structure. Various parts of the code read
this info structure to infer if the features are available.
Wrap this into a more readable vsp1_feature(vsp1, f) macro to ensure
that usage is consistent throughout the driver.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1.h | 2 ++
drivers/media/platform/vsp1/vsp1_drv.c | 16
drivers/media/platform/vsp1/vsp1_wpf.c | 6 +++---
3 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1.h
b/drivers/media/platform/vsp1/vsp1.h
index 78ef838416b3..1c080538c993 100644
--- a/drivers/media/platform/vsp1/vsp1.h
+++ b/drivers/media/platform/vsp1/vsp1.h
@@ -69,6 +69,8 @@ struct vsp1_device_info {
bool uapi;
};
+#define vsp1_feature(vsp1, f) ((vsp1)->info->features & (f))
+
struct vsp1_device {
struct device *dev;
const struct vsp1_device_info *info;
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
b/drivers/media/platform/vsp1/vsp1_drv.c
index eed9516e25e1..6fa0019ffc6e 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -268,7 +268,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
}
/* Instantiate all the entities. */
- if (vsp1->info->features & VSP1_HAS_BRS) {
+ if (vsp1_feature(vsp1, VSP1_HAS_BRS)) {
vsp1->brs = vsp1_bru_create(vsp1, VSP1_ENTITY_BRS);
if (IS_ERR(vsp1->brs)) {
ret = PTR_ERR(vsp1->brs);
@@ -278,7 +278,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
list_add_tail(&vsp1->brs->entity.list_dev, &vsp1->entities);
}
- if (vsp1->info->features & VSP1_HAS_BRU) {
+ if (vsp1_feature(vsp1, VSP1_HAS_BRU)) {
vsp1->bru = vsp1_bru_create(vsp1, VSP1_ENTITY_BRU);
if (IS_ERR(vsp1->bru)) {
ret = PTR_ERR(vsp1->bru);
@@ -288,7 +288,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
list_add_tail(&vsp1->bru->entity.list_dev, &vsp1->entities);
}
- if (vsp1->info->features & VSP1_HAS_CLU) {
+ if (vsp1_feature(vsp1, VSP1_HAS_CLU)) {
vsp1->clu = vsp1_clu_create(vsp1);
if (IS_ERR(vsp1->clu)) {
ret = PTR_ERR(vsp1->clu);
@@ -314,7 +314,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
list_add_tail(&vsp1->hst->entity.list_dev, &vsp1->entities);
- if (vsp1->info->features & VSP1_HAS_HGO && vsp1->info->uapi) {
+ if (vsp1_feature(vsp1, VSP1_HAS_HGO) && vsp1->info->uapi) {
vsp1->hgo = vsp1_hgo_create(vsp1);
if (IS_ERR(vsp1->hgo)) {
ret = PTR_ERR(vsp1->hgo);
@@ -325,7 +325,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
&vsp1->entities);
}
- if (vsp1->info->features & VSP1_HAS_HGT && vsp1->info->uapi) {
+ if (vsp1_feature(vsp1, VSP1_HAS_HGT) && vsp1->info->uapi) {
vsp1->hgt = vsp1_hgt_create(vsp1);
if (IS_ERR(vsp1->hgt)) {
ret = PTR_ERR(vsp1->hgt);
@@ -356,7 +356,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
}
}
- if (vsp1->info->features & VSP1_HAS_LUT) {
+ if (vsp1_feature(vsp1, VSP1_HAS_LUT)) {
vsp1->lut = vsp1_lut_create(vsp1);
if (IS_ERR(vsp1->lut)) {
ret = PTR_ERR(vsp1->lut);
@@ -390,7 +390,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
}
}
- if (vsp1->info->features & VSP1_HAS_SRU) {
+ if (vsp1_feature(vsp1, VSP1_HAS_SRU)) {
vsp1->sru = vsp1_sru_create(vsp1);
if (IS_ERR(vsp1->sru)) {
ret = PTR_ERR(vsp1->sru);
@@ -524,7 +524,7 @@ static int vsp1_device_init(struct vsp1_device *vsp1)
vsp1_write(vsp1, VI6_DPR_HSI_ROUTE, VI6_DPR_NODE_UNUSED);
vsp1_write(vsp1, VI6_DPR_BRU_ROUTE, VI6_DPR_NODE_UNUSED);
- if (vsp1->info->features & VSP1_HAS_BRS)
+ if (vsp1_feature(vsp1, VSP1_HAS_BRS))
vsp1_write(vsp1, VI6_DPR_ILV_BRS_ROUTE, VI6_DPR_NODE_UNUSED);
vsp1_write(vsp1, VI6_DPR_HGO_SMPPT, (7 << VI6_DPR_SMPPT_TGW_SHIFT) |
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c
b/drivers/media/platform/vsp1/vsp1_wpf.c
index 68218625549e..f90e474cf2cc 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -146,13 +146,13 @@ stat
[PATCH 01/11] media: vsp1: drm: Fix minor grammar error
The pixel format is 'unsupported'. Fix the small debug message which
incorrectly declares this.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
b/drivers/media/platform/vsp1/vsp1_drm.c
index 3c8b1952799d..0459b970e9da 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -363,7 +363,7 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int
pipe_index,
*/
fmtinfo = vsp1_get_format_info(vsp1, cfg->pixelformat);
if (!fmtinfo) {
- dev_dbg(vsp1->dev, "Unsupport pixel format %08x for RPF\n",
+ dev_dbg(vsp1->dev, "Unsupported pixel format %08x for RPF\n",
cfg->pixelformat);
return -EINVAL;
}
--
git-series 0.9.1
[PATCH 02/11] media: vsp1: use kernel __packed for structures
The kernel provides a __packed definition to abstract away from the
compiler specific attributes tag.
Convert all packed structures in VSP1 to use it.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 37e2c984fbf3..382e45c2054e 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -29,19 +29,19 @@
struct vsp1_dl_header_list {
u32 num_bytes;
u32 addr;
-} __attribute__((__packed__));
+} __packed;
struct vsp1_dl_header {
u32 num_lists;
struct vsp1_dl_header_list lists[8];
u32 next_header;
u32 flags;
-} __attribute__((__packed__));
+} __packed;
struct vsp1_dl_entry {
u32 addr;
u32 data;
-} __attribute__((__packed__));
+} __packed;
/**
* struct vsp1_dl_body - Display list body
--
git-series 0.9.1
[PATCH 03/11] media: vsp1: Rename dl_child to dl_next
Both vsp1_dl_list_commit() and __vsp1_dl_list_put() walk the display
list chain referencing the nodes as children, when in reality they are
siblings.
Update the terminology to 'dl_next' to be consistent with the
vsp1_video_pipeline_run() usage.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 382e45c2054e..057f0f093222 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -400,7 +400,7 @@ struct vsp1_dl_list *vsp1_dl_list_get(struct
vsp1_dl_manager *dlm)
/* This function must be called with the display list manager lock held.*/
static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
{
- struct vsp1_dl_list *dl_child;
+ struct vsp1_dl_list *dl_next;
if (!dl)
return;
@@ -410,8 +410,8 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
* hardware operation.
*/
if (dl->has_chain) {
- list_for_each_entry(dl_child, &dl->chain, chain)
- __vsp1_dl_list_put(dl_child);
+ list_for_each_entry(dl_next, &dl->chain, chain)
+ __vsp1_dl_list_put(dl_next);
}
dl->has_chain = false;
@@ -667,17 +667,17 @@ static void vsp1_dl_list_commit_singleshot(struct
vsp1_dl_list *dl)
void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
{
struct vsp1_dl_manager *dlm = dl->dlm;
- struct vsp1_dl_list *dl_child;
+ struct vsp1_dl_list *dl_next;
unsigned long flags;
if (dlm->mode == VSP1_DL_MODE_HEADER) {
/* Fill the header for the head and chained display lists. */
vsp1_dl_list_fill_header(dl, list_empty(&dl->chain));
- list_for_each_entry(dl_child, &dl->chain, chain) {
- bool last = list_is_last(&dl_child->chain, &dl->chain);
+ list_for_each_entry(dl_next, &dl->chain, chain) {
+ bool last = list_is_last(&dl_next->chain, &dl->chain);
- vsp1_dl_list_fill_header(dl_child, last);
+ vsp1_dl_list_fill_header(dl_next, last);
}
}
--
git-series 0.9.1
Re: [PATCH 09/11] media: vsp1: Provide support for extended command pools
On 09/03/18 22:04, Kieran Bingham wrote:
> VSPD and VSP-DL devices can provide extended display lists supporting
> extended command display list objects.
>
> These extended commands require their own dma memory areas for a header
> and body specific to the command type.
>
> Implement a command pool to allocate all necessary memory in a single
> DMA allocation to reduce pressure on the TLB, and provide convenvient
s/convenvient/convenient/
> re-usable command objects for the entities to utilise.
> Signed-off-by: Kieran Bingham
> ---
I feel like this adds quite a bit of 'duplication' against the body pool
implementation - and there is scope for re-factoring somehow to make a lot more
of this common.
I think this is still fine to go in as is for now (as an approach that is) - but
I'd like to work out how to make this better as a later task.
Then with a reusable implementation then we can easily move the excess display
list headers (which are currently being allocated 1 for *every dlb* rather than
1 for every dl) to their own pool and allocate as appropriate.
Essentially we have the following 'object's which want to have minimal DMA
allocations (to reduce TLB pressure) - and are all sharing the same size.
- Display list headers (72 or 96 bytes)
- Display list bodys (variable size - multiple per header)
if (VSPD) {
- Extended display list header (16 bytes * number of bodies)
- Extended display list body (autodisp 96 bytes, autofld 160 bytes)
}
The dma_pool API's don't seem to be suitable here because as far as I can tell
it is still calling dma_alloc_coherent for each page.., rather than creating a
large pre-allocated slab and carving from it.
There certainly doesn't seem to be a way to say the number of elements to
pre-allocate... If I'm missing something obvious here - I'd love to hear it as I
don't want to re-invent any wheels!
Surely this similar pattern occurs elsewhere in the kernel ?
--
Kieran
> drivers/media/platform/vsp1/vsp1_dl.c | 189 +++-
> drivers/media/platform/vsp1/vsp1_dl.h | 3 +-
> 2 files changed, 192 insertions(+)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index 36440a2a2c8b..6d17b8bfa21c 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -121,6 +121,30 @@ struct vsp1_dl_body_pool {
> };
>
> /**
> + * struct vsp1_cmd_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_cmd_pool {
> + /* DMA allocation */
> + dma_addr_t dma;
> + size_t size;
> + void *mem;
> +
> + struct vsp1_dl_ext_cmd *cmds;
> + 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
> @@ -176,6 +200,7 @@ struct vsp1_dl_manager {
> struct vsp1_dl_list *pending;
>
> struct vsp1_dl_body_pool *pool;
> + struct vsp1_dl_cmd_pool *autfld_cmds;
> };
>
> /*
> -
> @@ -339,6 +364,139 @@ void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32
> reg, u32 data)
> }
>
> /*
> -
> + * Display List Extended Command Management
> + */
> +
> +enum vsp1_extcmd_type {
> + VSP1_EXTCMD_AUTODISP,
> + VSP1_EXTCMD_AUTOFLD,
> +};
> +
> +struct vsp1_extended_command_info {
> + u16 opcode;
> + size_t body_size;
> +} vsp1_extended_commands[] = {
> + [VSP1_EXTCMD_AUTODISP] = { 0x02, 96 },
> + [VSP1_EXTCMD_AUTOFLD] = { 0x03, 160 },
> +};
> +
> +/**
> + * vsp1_dl_cmd_pool_create - Create a pool of commands from a single
> allocation
> + * @vsp1: The VSP1 device
> + * @type: The command pool type
> + * @num_commands: The quantity of commands to allocate
> + *
> + * Allocate a pool of commands each with enough memory to contain the private
> + * data of each command. The allocation sizes are dependent upon the command
> + * type.
> + *
> + * Return a pointer to a pool on success or NULL if memory can't be
> allocated.
> + */
> +struct vsp1_dl_cmd_pool *
> +vsp1_dl_cmd_pool_create(struct vsp1_device *vsp1, enum vsp1_extcmd_
Re: [PATCH v6] ARM: dts: wheat: Fix ADV7513 address usage
Hi Simon,
On 23/03/18 08:51, Simon Horman wrote:
> On Thu, Mar 22, 2018 at 09:30:40PM +0000, Kieran Bingham wrote:
>> The r8a7792 Wheat board has two ADV7513 devices sharing a single I2C
>> bus, however in low power mode the ADV7513 will reset it's slave maps to
>> use the hardware defined default addresses.
>>
>> The ADV7511 driver was adapted to allow the two devices to be registered
>> correctly - but it did not take into account the fault whereby the
>> devices reset the addresses.
>>
>> This results in an address conflict between the device using the default
>> addresses, and the other device if it is in low-power-mode.
>>
>> Repair this issue by moving both devices away from the default address
>> definitions.
>
> Hi Kierean,
>
> as this is a fix
> a) Does it warrant a fixes tag?
>Fixes: f6eea82a87db ("ARM: dts: wheat: add DU support")
> b) Does it warrant being posted as a fix for v4.16;
> c) or v4.17?
Tricky one, yes it could but this DTS fix, will only actually 'fix' the issue if
the corresponding driver updates to allow secondary addresses to be parsed are
also backported.
It should be safe to back port the dts fix without the driver updates, but the
addresses specified by this patch will simply be ignored.
Thus if this is marked with the fixes tag the corresponding patch "drm: adv7511:
Add support for i2c_new_secondary_device" should also be marked.
It looks like that patch has yet to be picked up by the DRM subsystem, so how
about I bundle both of these two patches together in a repost along with the
fixes tag.
In fact, I don't think the ADV7511 dt-bindings update has made any progress
either. (dt-bindings: adv7511: Extend bindings to allow specifying slave map
addresses). The media tree variants for the adv7604 have already been picked up
by Mauro I believe though.
I presume it would be acceptable for this dts patch (or rather all three patches
mentioned) to get integrated through the DRM tree ?
--
Kieran
Re: [PATCH v6] ARM: dts: wheat: Fix ADV7513 address usage
Hi Simon,
On 26/03/18 09:31, Simon Horman wrote:
> On Fri, Mar 23, 2018 at 09:16:13PM +0000, Kieran Bingham wrote:
>> Hi Simon,
>>
>> On 23/03/18 08:51, Simon Horman wrote:
>>> On Thu, Mar 22, 2018 at 09:30:40PM +, Kieran Bingham wrote:
>>>> The r8a7792 Wheat board has two ADV7513 devices sharing a single I2C
>>>> bus, however in low power mode the ADV7513 will reset it's slave maps to
>>>> use the hardware defined default addresses.
>>>>
>>>> The ADV7511 driver was adapted to allow the two devices to be registered
>>>> correctly - but it did not take into account the fault whereby the
>>>> devices reset the addresses.
>>>>
>>>> This results in an address conflict between the device using the default
>>>> addresses, and the other device if it is in low-power-mode.
>>>>
>>>> Repair this issue by moving both devices away from the default address
>>>> definitions.
>>>
>>> Hi Kierean,
>>>
>>> as this is a fix
>>> a) Does it warrant a fixes tag?
>>>Fixes: f6eea82a87db ("ARM: dts: wheat: add DU support")
>>> b) Does it warrant being posted as a fix for v4.16;
>>> c) or v4.17?
>>
>> Tricky one, yes it could but this DTS fix, will only actually 'fix' the
>> issue if
>> the corresponding driver updates to allow secondary addresses to be parsed
>> are
>> also backported.
>>
>> It should be safe to back port the dts fix without the driver updates, but
>> the
>> addresses specified by this patch will simply be ignored.
>
> In that case I think its safe to add the fixes tag and take the DTS patch
> via the renesas tree. Perhaps applying it for v4.18 and allowing automatic
> backporting to take its course is the cleanest option.
>
>> Thus if this is marked with the fixes tag the corresponding patch "drm:
>> adv7511:
>> Add support for i2c_new_secondary_device" should also be marked.
>>
>> It looks like that patch has yet to be picked up by the DRM subsystem, so how
>> about I bundle both of these two patches together in a repost along with the
>> fixes tag.
>>
>> In fact, I don't think the ADV7511 dt-bindings update has made any progress
>> either. (dt-bindings: adv7511: Extend bindings to allow specifying slave map
>> addresses). The media tree variants for the adv7604 have already been picked
>> up
>> by Mauro I believe though.
>>
>> I presume it would be acceptable for this dts patch (or rather all three
>> patches
>> mentioned) to get integrated through the DRM tree ?
>
> Unless there is a strong reason I would prefer the dts patch to go via
> my tree. The reason is to avoid merge conflicts bubbling up to Linus,
> which really is something best avoided.
That's perfectly fine with me.
Feel free to add:
Fixes: f6eea82a87db ("ARM: dts: wheat: add DU support")
as you suggested when you apply, or alternatively let me know if you need a
repost.
Regards
--
Kieran
[PATCH] media: vsp1: Fix BRx conditional path in WPF
When a BRx is provided by a pipeline, the WPF must determine the master
layer. Currently the condition to check this identifies pipe->bru ||
pipe->num_inputs > 1.
The code then moves on to dereference pipe->bru, thus the check fails
static analysers on the possibility that pipe->num_inputs could be
greater than 1 without pipe->bru being set.
The reality is that the pipeline must have a BRx to support more than
one input, thus this could never cause a fault - however it also
identifies that the num_inputs > 1 check is redundant.
Remove the redundant check - and always configure the master layer
appropriately when we have a BRx configured in our pipeline.
Fixes: 6134148f6098 ("v4l: vsp1: Add support for the BRS entity")
Cc: sta...@vger.kernel.org
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_wpf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c
b/drivers/media/platform/vsp1/vsp1_wpf.c
index f7f3b4b2c2de..8bd6b2f1af15 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -452,7 +452,7 @@ static void wpf_configure(struct vsp1_entity *entity,
: VI6_WPF_SRCRPF_RPF_ACT_SUB(input->entity.index);
}
- if (pipe->bru || pipe->num_inputs > 1)
+ if (pipe->bru)
srcrpf |= pipe->bru->type == VSP1_ENTITY_BRU
? VI6_WPF_SRCRPF_VIRACT_MST
: VI6_WPF_SRCRPF_VIRACT2_MST;
--
2.7.4
Re: [PATCH] media: vsp1: Fix BRx conditional path in WPF
Sorry,
This should have been from my +renesas to support Renesas' statistics and
filtering of course.
On 26/03/18 14:29, Kieran Bingham wrote:
> When a BRx is provided by a pipeline, the WPF must determine the master
> layer. Currently the condition to check this identifies pipe->bru ||
> pipe->num_inputs > 1.
>
> The code then moves on to dereference pipe->bru, thus the check fails
> static analysers on the possibility that pipe->num_inputs could be
> greater than 1 without pipe->bru being set.
>
> The reality is that the pipeline must have a BRx to support more than
> one input, thus this could never cause a fault - however it also
> identifies that the num_inputs > 1 check is redundant.
>
> Remove the redundant check - and always configure the master layer
> appropriately when we have a BRx configured in our pipeline.
>
> Fixes: 6134148f6098 ("v4l: vsp1: Add support for the BRS entity")
> Cc: sta...@vger.kernel.org
>
> Suggested-by: Mauro Carvalho Chehab
> Signed-off-by: Kieran Bingham
And thus:
Signed-off-by: Kieran Bingham
> ---
> drivers/media/platform/vsp1/vsp1_wpf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c
> b/drivers/media/platform/vsp1/vsp1_wpf.c
> index f7f3b4b2c2de..8bd6b2f1af15 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -452,7 +452,7 @@ static void wpf_configure(struct vsp1_entity *entity,
> : VI6_WPF_SRCRPF_RPF_ACT_SUB(input->entity.index);
> }
>
> - if (pipe->bru || pipe->num_inputs > 1)
> + if (pipe->bru)
> srcrpf |= pipe->bru->type == VSP1_ENTITY_BRU
> ? VI6_WPF_SRCRPF_VIRACT_MST
> : VI6_WPF_SRCRPF_VIRACT2_MST;
>
[PATCH] media: uvcvideo: Prevent setting unavailable flags
The addition of an extra operation to use the GET_INFO command
overwrites all existing flags from the uvc_ctrls table. This includes
setting all controls as supporting GET_MIN, GET_MAX, GET_RES, and
GET_DEF regardless of whether they do or not.
Move the initialisation of these control capabilities directly to the
uvc_ctrl_fill_xu_info() call where they were originally located in that
use case, and ensure that the new functionality in uvc_ctrl_get_flags()
will only set flags based on their reported capability from the GET_INFO
call.
Fixes: 859086ae3636 ("media: uvcvideo: Apply flags from device to actual
properties")
Signed-off-by: Kieran Bingham
---
drivers/media/usb/uvc/uvc_ctrl.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 1daf444371be..4042cbdb721b 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1607,14 +1607,12 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
info->selector, data, 1);
if (!ret)
- info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
- | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
- | (data[0] & UVC_CONTROL_CAP_GET ?
- UVC_CTRL_FLAG_GET_CUR : 0)
- | (data[0] & UVC_CONTROL_CAP_SET ?
- UVC_CTRL_FLAG_SET_CUR : 0)
- | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
- UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+ info->flags |= (data[0] & UVC_CONTROL_CAP_GET ?
+ UVC_CTRL_FLAG_GET_CUR : 0)
+ | (data[0] & UVC_CONTROL_CAP_SET ?
+ UVC_CTRL_FLAG_SET_CUR : 0)
+ | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
+ UVC_CTRL_FLAG_AUTO_UPDATE : 0);
kfree(data);
return ret;
@@ -1689,6 +1687,9 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
info->size = le16_to_cpup((__le16 *)data);
+ info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+ | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF;
+
ret = uvc_ctrl_get_flags(dev, ctrl, info);
if (ret < 0) {
uvc_trace(UVC_TRACE_CONTROL,
--
2.7.4
[PATCH v5] ARM: dts: wheat: Fix ADV7513 address usage
The r8a7792 Wheat board has two ADV7513 devices sharing a single I2C
bus, however in low power mode the ADV7513 will reset it's slave maps to
use the hardware defined default addresses.
The ADV7511 driver was adapted to allow the two devices to be registered
correctly - but it did not take into account the fault whereby the
devices reset the addresses.
This results in an address conflict between the device using the default
addresses, and the other device if it is in low-power-mode.
Repair this issue by moving both devices away from the default address
definitions.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- Addition to series
v3:
- Split map register addresses into individual declarations.
v4:
- Normalise I2C usage
v5:
- No change from v4 except to repost and drop the [RFT] now that it's tested
Testing on a wheat board shows the addresses correctly assigned, and the
default addresses (0x38, 0x3e, 0x3f which would otherwise conflict) are
shown as actively returning data in low power mode during the scan.
(they return 0)
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- UU -- -- -- UU -- --
30: -- -- -- -- -- -- -- -- 38 UU -- -- -- UU 3e 3f
40: -- -- -- -- -- -- -- -- -- UU -- -- -- UU -- --
50: -- -- -- -- -- -- -- -- -- UU -- -- -- UU -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --
---
arch/arm/boot/dts/r8a7792-wheat.dts | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/r8a7792-wheat.dts
b/arch/arm/boot/dts/r8a7792-wheat.dts
index 293b9e3b3e70..3e9f70e87a60 100644
--- a/arch/arm/boot/dts/r8a7792-wheat.dts
+++ b/arch/arm/boot/dts/r8a7792-wheat.dts
@@ -245,9 +245,16 @@
status = "okay";
clock-frequency = <40>;
+ /*
+* The adv75xx resets its addresses to defaults during low power power
+* mode. Because we have two ADV7513 devices on the same bus, we must
+* change both of them away from the defaults so that they do not
+* conflict.
+*/
hdmi@3d {
compatible = "adi,adv7513";
- reg = <0x3d>;
+ reg = <0x3d>, <0x2d>, <0x4d>, <0x5d>;
+ reg-names = "main", "cec", "edid", "packet";
adi,input-depth = <8>;
adi,input-colorspace = "rgb";
@@ -277,7 +284,8 @@
hdmi@39 {
compatible = "adi,adv7513";
- reg = <0x39>;
+ reg = <0x39>, <0x29>, <0x49>, <0x59>;
+ reg-names = "main", "cec", "edid", "packet";
adi,input-depth = <8>;
adi,input-colorspace = "rgb";
--
2.7.4
[PATCH v6] ARM: dts: wheat: Fix ADV7513 address usage
The r8a7792 Wheat board has two ADV7513 devices sharing a single I2C
bus, however in low power mode the ADV7513 will reset it's slave maps to
use the hardware defined default addresses.
The ADV7511 driver was adapted to allow the two devices to be registered
correctly - but it did not take into account the fault whereby the
devices reset the addresses.
This results in an address conflict between the device using the default
addresses, and the other device if it is in low-power-mode.
Repair this issue by moving both devices away from the default address
definitions.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- Addition to series
v3:
- Split map register addresses into individual declarations.
v4:
- Normalise I2C usage
v5:
- Repost without [RFT] now that it has been tested
v6:
- s/low power power/low power/ correction from Laurent.
Testing on a wheat board shows the addresses correctly assigned, and the
default addresses (0x38, 0x3e, 0x3f which would otherwise conflict) are
shown as actively returning data in low power mode during the scan.
(they return 0)
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- UU -- -- -- UU -- --
30: -- -- -- -- -- -- -- -- 38 UU -- -- -- UU 3e 3f
40: -- -- -- -- -- -- -- -- -- UU -- -- -- UU -- --
50: -- -- -- -- -- -- -- -- -- UU -- -- -- UU -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --
arch/arm/boot/dts/r8a7792-wheat.dts | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/r8a7792-wheat.dts
b/arch/arm/boot/dts/r8a7792-wheat.dts
index 293b9e3b3e70..db01de7a3811 100644
--- a/arch/arm/boot/dts/r8a7792-wheat.dts
+++ b/arch/arm/boot/dts/r8a7792-wheat.dts
@@ -245,9 +245,15 @@
status = "okay";
clock-frequency = <40>;
+ /*
+* The adv75xx resets its addresses to defaults during low power mode.
+* Because we have two ADV7513 devices on the same bus, we must change
+* both of them away from the defaults so that they do not conflict.
+*/
hdmi@3d {
compatible = "adi,adv7513";
- reg = <0x3d>;
+ reg = <0x3d>, <0x2d>, <0x4d>, <0x5d>;
+ reg-names = "main", "cec", "edid", "packet";
adi,input-depth = <8>;
adi,input-colorspace = "rgb";
@@ -277,7 +283,8 @@
hdmi@39 {
compatible = "adi,adv7513";
- reg = <0x39>;
+ reg = <0x39>, <0x29>, <0x49>, <0x59>;
+ reg-names = "main", "cec", "edid", "packet";
adi,input-depth = <8>;
adi,input-colorspace = "rgb";
--
2.7.4
Re: [PATCH 09/11] media: vsp1: Provide support for extended command pools
Hi Jacopo,
On 12/03/18 16:30, jacopo mondi wrote:
> Hi Kieran,
> just one small thing I noticed below...
>
> On Fri, Mar 09, 2018 at 10:04:07PM +0000, Kieran Bingham wrote:
>> VSPD and VSP-DL devices can provide extended display lists supporting
>> extended command display list objects.
>>
>> These extended commands require their own dma memory areas for a header
>> and body specific to the command type.
>>
>> Implement a command pool to allocate all necessary memory in a single
>> DMA allocation to reduce pressure on the TLB, and provide convenvient
>> re-usable command objects for the entities to utilise.
>>
>> Signed-off-by: Kieran Bingham
>> ---
>> drivers/media/platform/vsp1/vsp1_dl.c | 189 +++-
>> drivers/media/platform/vsp1/vsp1_dl.h | 3 +-
>> 2 files changed, 192 insertions(+)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
>> b/drivers/media/platform/vsp1/vsp1_dl.c
>> index 36440a2a2c8b..6d17b8bfa21c 100644
>> --- a/drivers/media/platform/vsp1/vsp1_dl.c
>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
>> @@ -121,6 +121,30 @@ struct vsp1_dl_body_pool {
>> };
>>
>> /**
>> + * struct vsp1_cmd_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_cmd_pool {
>> +/* DMA allocation */
>> +dma_addr_t dma;
>> +size_t size;
>> +void *mem;
>> +
>> +struct vsp1_dl_ext_cmd *cmds;
>> +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
>> @@ -176,6 +200,7 @@ struct vsp1_dl_manager {
>> struct vsp1_dl_list *pending;
>>
>> struct vsp1_dl_body_pool *pool;
>> +struct vsp1_dl_cmd_pool *autfld_cmds;
>> };
>>
>> /*
>> -
>> @@ -339,6 +364,139 @@ void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32
>> reg, u32 data)
>> }
>>
>> /*
>> -
>> + * Display List Extended Command Management
>> + */
>> +
>> +enum vsp1_extcmd_type {
>> +VSP1_EXTCMD_AUTODISP,
>> +VSP1_EXTCMD_AUTOFLD,
>> +};
>> +
>> +struct vsp1_extended_command_info {
>> +u16 opcode;
>> +size_t body_size;
>> +} vsp1_extended_commands[] = {
>> +[VSP1_EXTCMD_AUTODISP] = { 0x02, 96 },
>> +[VSP1_EXTCMD_AUTOFLD] = { 0x03, 160 },
>> +};
>
> How about making this one static and const, since it does not get
> modified?
Good spot. Certainly. This is just a static descriptor table of the extended
command parameter sizes, so it should not change. (but could be added to in
later hardware operations I presume).
Cheers
Kieran
>
> Thanks
>j
>
Re: [PATCH 02/11] media: vsp1: use kernel __packed for structures
Hi David,
On 13/03/18 11:20, David Laight wrote:
> From: Kieran Bingham
>> Sent: 09 March 2018 22:04
>> The kernel provides a __packed definition to abstract away from the
>> compiler specific attributes tag.
>>
>> Convert all packed structures in VSP1 to use it.
>>
>> Signed-off-by: Kieran Bingham
>> ---
>> drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
>> b/drivers/media/platform/vsp1/vsp1_dl.c
>> index 37e2c984fbf3..382e45c2054e 100644
>> --- a/drivers/media/platform/vsp1/vsp1_dl.c
>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
>> @@ -29,19 +29,19 @@
>> struct vsp1_dl_header_list {
>> u32 num_bytes;
>> u32 addr;
>> -} __attribute__((__packed__));
>> +} __packed;
>>
>> struct vsp1_dl_header {
>> u32 num_lists;
>> struct vsp1_dl_header_list lists[8];
>> u32 next_header;
>> u32 flags;
>> -} __attribute__((__packed__));
>> +} __packed;
>>
>> struct vsp1_dl_entry {
>> u32 addr;
>> u32 data;
>> -} __attribute__((__packed__));
>> +} __packed;
>
> Do these structures ever actually appear in misaligned memory?
> If they don't then they shouldn't be marked 'packed'.
I believe the declaration is to ensure that the struct definition is not altered
by the compiler as these structures specifically define the layout of how the
memory is read by the VSP1 hardware.
Certainly 2 u32's sequentially stored in a struct are unlikely to be moved or
rearranged by the compiler (though I believe it would be free to do so if it
chose without this attribute), but I think the declaration shows the intent of
the memory structure.
Isn't this a common approach throughout the kernel when dealing with hardware
defined memory structures ?
Regards
--
Kieran
>
> David
>
Re: [PATCH 02/11] media: vsp1: use kernel __packed for structures
Hi David,
On 13/03/18 13:38, David Laight wrote:
> From: Kieran Bingham [mailto:kieran.bingham+rene...@ideasonboard.com]
>> On 13/03/18 11:20, David Laight wrote:
>>> From: Kieran Bingham
>>>> Sent: 09 March 2018 22:04
>>>> The kernel provides a __packed definition to abstract away from the
>>>> compiler specific attributes tag.
>>>>
>>>> Convert all packed structures in VSP1 to use it.
>>>>
>>>> Signed-off-by: Kieran Bingham
>>>> ---
>>>> drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
>>>> b/drivers/media/platform/vsp1/vsp1_dl.c
>>>> index 37e2c984fbf3..382e45c2054e 100644
>>>> --- a/drivers/media/platform/vsp1/vsp1_dl.c
>>>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
>>>> @@ -29,19 +29,19 @@
>>>> struct vsp1_dl_header_list {
>>>>u32 num_bytes;
>>>>u32 addr;
>>>> -} __attribute__((__packed__));
>>>> +} __packed;
>>>>
>>>> struct vsp1_dl_header {
>>>>u32 num_lists;
>>>>struct vsp1_dl_header_list lists[8];
>>>>u32 next_header;
>>>>u32 flags;
>>>> -} __attribute__((__packed__));
>>>> +} __packed;
>>>>
>>>> struct vsp1_dl_entry {
>>>>u32 addr;
>>>>u32 data;
>>>> -} __attribute__((__packed__));
>>>> +} __packed;
>>>
>>> Do these structures ever actually appear in misaligned memory?
>>> If they don't then they shouldn't be marked 'packed'.
>>
>> I believe the declaration is to ensure that the struct definition is not
>> altered
>> by the compiler as these structures specifically define the layout of how the
>> memory is read by the VSP1 hardware.
>
> The C language and ABI define structure layouts.
>
>> Certainly 2 u32's sequentially stored in a struct are unlikely to be moved or
>> rearranged by the compiler (though I believe it would be free to do so if it
>> chose without this attribute), but I think the declaration shows the intent
>> of
>> the memory structure.
>
> The language requires the fields be in order and the ABI stops the compiler
> adding 'random' padding.
>
>> Isn't this a common approach throughout the kernel when dealing with hardware
>> defined memory structures ?
>
> Absolutely not.
> __packed tells the compiler that the structure might be on any address
> boundary.
> On many architectures this means the compiler must use byte accesses with
> shifts
> and ors for every access.
> The most a hardware defined structure will have is a compile-time assert
> that it is the correct size (to avoid silly errors from changes).
Ok - interesting, I see what you're saying - and certainly don't want the
compiler to be performing byte accesses on the structures unnecessarily.
I'm trying to distinguish the difference here. Is the single point that
__packed
causes byte-access, where as
__attribute__((__packed__));
does not?
Looking at the GCC docs [0]: I see that __attribute__((__packed__)) tells the
compiler that the "structure or union is placed to minimize the memory
required".
However, the keil compiler docs[1] do certainly declare that __packed causes
byte alignment.
I was confused/thrown off here by the Kernel defining __packed as
__attribute__((packed)) at [2].
Do __attribute__((packed)) and __attribute__((__packed__)) differ ?
In which case, from what I've read so far I wish "__packed" was "__unaligned"...
[0]
https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-packed-type-attribute
[1] http://www.keil.com/support/man/docs/armcc/armcc_chr1359124230195.htm
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler-gcc.h?h=v4.16-rc5#n92
Regards
Kieran
> David
>
[PATCH v2 07/11] media: vsp1: Use header display lists for all WPF outputs linked to the DU
Header mode display lists are now supported on all WPF outputs. To
support extended headers and auto-fld capabilities for interlaced mode
handling only header mode display lists can be used.
Disable the headerless display list configuration, and remove the dead
code.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 107 ++-
1 file changed, 27 insertions(+), 80 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 680eedb6fc9f..5f5706f8a84c 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -98,7 +98,7 @@ struct vsp1_dl_body_pool {
* struct vsp1_dl_list - Display list
* @list: entry in the display list manager lists
* @dlm: the display list manager
- * @header: display list header, NULL for headerless lists
+ * @header: display list header
* @dma: DMA address for the header
* @body0: first display list body
* @bodies: list of extra display list bodies
@@ -119,15 +119,9 @@ struct vsp1_dl_list {
struct list_head chain;
};
-enum vsp1_dl_mode {
- VSP1_DL_MODE_HEADER,
- VSP1_DL_MODE_HEADERLESS,
-};
-
/**
* struct vsp1_dl_manager - Display List manager
* @index: index of the related WPF
- * @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_bodies lists
@@ -139,7 +133,6 @@ enum vsp1_dl_mode {
*/
struct vsp1_dl_manager {
unsigned int index;
- enum vsp1_dl_mode mode;
bool singleshot;
struct vsp1_device *vsp1;
@@ -320,6 +313,7 @@ static struct vsp1_dl_list *vsp1_dl_list_alloc(struct
vsp1_dl_manager *dlm,
struct vsp1_dl_body_pool *pool)
{
struct vsp1_dl_list *dl;
+ size_t header_offset;
dl = kzalloc(sizeof(*dl), GFP_KERNEL);
if (!dl)
@@ -332,16 +326,15 @@ static struct vsp1_dl_list *vsp1_dl_list_alloc(struct
vsp1_dl_manager *dlm,
dl->body0 = vsp1_dl_body_get(pool);
if (!dl->body0)
return NULL;
- if (dlm->mode == VSP1_DL_MODE_HEADER) {
- size_t header_offset = dl->body0->max_entries
-* sizeof(*dl->body0->entries);
- dl->header = ((void *)dl->body0->entries) + header_offset;
- dl->dma = dl->body0->dma + header_offset;
+ header_offset = dl->body0->max_entries
+* sizeof(*dl->body0->entries);
- memset(dl->header, 0, sizeof(*dl->header));
- dl->header->lists[0].addr = dl->body0->dma;
- }
+ dl->header = ((void *)dl->body0->entries) + header_offset;
+ dl->dma = dl->body0->dma + header_offset;
+
+ memset(dl->header, 0, sizeof(*dl->header));
+ dl->header->lists[0].addr = dl->body0->dma;
return dl;
}
@@ -473,16 +466,9 @@ struct vsp1_dl_body *vsp1_dl_list_get_body0(struct
vsp1_dl_list *dl)
*
* 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.
*/
int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct vsp1_dl_body *dlb)
{
- /* Multi-body lists are only available in header mode. */
- if (dl->dlm->mode != VSP1_DL_MODE_HEADER)
- return -EINVAL;
-
refcount_inc(&dlb->refcnt);
list_add_tail(&dlb->list, &dl->bodies);
@@ -503,17 +489,10 @@ int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct
vsp1_dl_body *dlb)
* Adding a display list to a chain passes ownership of the display list to
* the head display list item. The chain is released when the head dl item is
* put back with __vsp1_dl_list_put().
- *
- * Chained display lists are only usable in header mode. Attempts to add a
- * display list to a chain in header-less mode will return an error.
*/
int vsp1_dl_list_add_chain(struct vsp1_dl_list *head,
struct vsp1_dl_list *dl)
{
- /* Chained lists are only available in header mode. */
- if (head->dlm->mode != VSP1_DL_MODE_HEADER)
- return -EINVAL;
-
head->has_chain = true;
list_add_tail(&dl->chain, &head->chain);
return 0;
@@ -581,17 +560,10 @@ static bool vsp1_dl_list_hw_update_pending(struct
vsp1_dl_manager *dlm)
return false;
/*
-* Check whether the VSP1 has taken the update. In headerless mode the
-* hardware indicates this by clearing the UPD bit in the DL_BODY_SIZE
-* r
[PATCH v2 08/11] media: vsp1: Add support for extended display list headers
Extended display list headers allow pre and post command lists to be
executed by the VSP pipeline. This provides the base support for
features such as AUTO_FLD (for interlaced support) and AUTO_DISP (for
supporting continuous camera preview pipelines.
Signed-off-by: Kieran Bingham
---
v2:
- remove __packed attributes
drivers/media/platform/vsp1/vsp1.h | 1 +-
drivers/media/platform/vsp1/vsp1_dl.c | 83 +-
drivers/media/platform/vsp1/vsp1_dl.h | 29 -
drivers/media/platform/vsp1/vsp1_drv.c | 7 +-
drivers/media/platform/vsp1/vsp1_regs.h | 5 +-
5 files changed, 116 insertions(+), 9 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1.h
b/drivers/media/platform/vsp1/vsp1.h
index 1c080538c993..bb3b32795206 100644
--- a/drivers/media/platform/vsp1/vsp1.h
+++ b/drivers/media/platform/vsp1/vsp1.h
@@ -55,6 +55,7 @@ struct vsp1_uds;
#define VSP1_HAS_HGO (1 << 7)
#define VSP1_HAS_HGT (1 << 8)
#define VSP1_HAS_BRS (1 << 9)
+#define VSP1_HAS_EXT_DL(1 << 10)
struct vsp1_device_info {
u32 version;
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 5f5706f8a84c..cd91b50deed1 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -26,6 +26,9 @@
#define VSP1_DLH_INT_ENABLE(1 << 1)
#define VSP1_DLH_AUTO_START(1 << 0)
+#define VSP1_DLH_EXT_PRE_CMD_EXEC (1 << 9)
+#define VSP1_DLH_EXT_POST_CMD_EXEC (1 << 8)
+
struct vsp1_dl_header_list {
u32 num_bytes;
u32 addr;
@@ -38,11 +41,34 @@ struct vsp1_dl_header {
u32 flags;
};
+struct vsp1_dl_ext_header {
+ u32 reserved0; /* alignment padding */
+
+ u16 pre_ext_cmd_qty;
+ u16 flags;
+ u32 pre_ext_cmd_plist;
+
+ u32 post_ext_cmd_qty;
+ u32 post_ext_cmd_plist;
+};
+
+struct vsp1_dl_header_extended {
+ struct vsp1_dl_header header;
+ struct vsp1_dl_ext_header ext;
+};
+
struct vsp1_dl_entry {
u32 addr;
u32 data;
};
+struct vsp1_dl_ext_cmd_header {
+ u32 cmd;
+ u32 flags;
+ u32 data;
+ u32 reserved;
+};
+
/**
* struct vsp1_dl_body - Display list body
* @list: entry in the display list list of bodies
@@ -99,9 +125,12 @@ struct vsp1_dl_body_pool {
* @list: entry in the display list manager lists
* @dlm: the display list manager
* @header: display list header
+ * @extended: extended display list header. NULL for normal lists
* @dma: DMA address for the header
* @body0: first display list body
* @bodies: list of extra display list bodies
+ * @pre_cmd: pre cmd to be issued through extended dl header
+ * @post_cmd: post cmd to be issued through extended dl header
* @has_chain: if true, indicates that there's a partition chain
* @chain: entry in the display list partition chain
*/
@@ -110,11 +139,15 @@ struct vsp1_dl_list {
struct vsp1_dl_manager *dlm;
struct vsp1_dl_header *header;
+ struct vsp1_dl_ext_header *extended;
dma_addr_t dma;
struct vsp1_dl_body *body0;
struct list_head bodies;
+ struct vsp1_dl_ext_cmd *pre_cmd;
+ struct vsp1_dl_ext_cmd *post_cmd;
+
bool has_chain;
struct list_head chain;
};
@@ -498,6 +531,14 @@ int vsp1_dl_list_add_chain(struct vsp1_dl_list *head,
return 0;
}
+static void vsp1_dl_ext_cmd_fill_header(struct vsp1_dl_ext_cmd *cmd)
+{
+ cmd->cmds[0].cmd = cmd->cmd_opcode;
+ cmd->cmds[0].flags = cmd->flags;
+ cmd->cmds[0].data = cmd->data_dma;
+ cmd->cmds[0].reserved = 0;
+}
+
static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)
{
struct vsp1_dl_manager *dlm = dl->dlm;
@@ -550,6 +591,27 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list
*dl, bool is_last)
*/
dl->header->flags = VSP1_DLH_INT_ENABLE;
}
+
+ if (!dl->extended)
+ return;
+
+ dl->extended->flags = 0;
+
+ if (dl->pre_cmd) {
+ dl->extended->pre_ext_cmd_plist = dl->pre_cmd->cmd_dma;
+ dl->extended->pre_ext_cmd_qty = dl->pre_cmd->num_cmds;
+ dl->extended->flags |= VSP1_DLH_EXT_PRE_CMD_EXEC;
+
+ vsp1_dl_ext_cmd_fill_header(dl->pre_cmd);
+ }
+
+ if (dl->post_cmd) {
+ dl->extended->pre_ext_cmd_plist = dl->post_cmd->cmd_dma;
+ dl->extended->pre_ext_cmd_qty = dl->post_cmd->num_cmds;
+ dl->extended->flags |= VSP1_DLH_EXT_POST_CMD_EXEC;
+
+ vsp1_dl_ext_cmd_fill_header(dl->pre_cmd);
+ }
}
static bool vsp1_dl_list_hw_update_pending(struct vsp1_dl_manager *dlm)
@@ -715,14 +777,20 @@ bool vsp1_dlm_irq_frame_en
[PATCH v2 10/11] media: vsp1: Support Interlaced display pipelines
Calculate the top and bottom fields for the interlaced frames and
utilise the extended display list command feature to implement the
auto-field operations. This allows the DU to update the VSP2 registers
dynamically based upon the currently processing field.
Signed-off-by: Kieran Bingham
---
v2:
- fix erroneous BIT value which enabled interlaced
- fix field handling at frame_end interrupt
drivers/media/platform/vsp1/vsp1_dl.c | 10 -
drivers/media/platform/vsp1/vsp1_drm.c | 11 -
drivers/media/platform/vsp1/vsp1_regs.h | 1 +-
drivers/media/platform/vsp1/vsp1_rpf.c | 72 --
drivers/media/platform/vsp1/vsp1_rwpf.h | 1 +-
include/media/vsp1.h| 1 +-
6 files changed, 93 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index d8392bd866f1..08715ce6db1e 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -889,7 +889,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
*/
bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
{
+ struct vsp1_device *vsp1 = dlm->vsp1;
bool completed = false;
+ u32 status = vsp1_read(vsp1, VI6_STATUS);
spin_lock(&dlm->lock);
@@ -914,6 +916,14 @@ bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
goto done;
/*
+* Progressive streams report only TOP fields. If we have a BOTTOM
+* field, we are interlaced, and expect the frame to complete on the
+* next frame end interrupt.
+*/
+ if (status & VI6_STATUS_FLD_STD(dlm->index))
+ goto done;
+
+ /*
* The device starts processing the queued display list right after the
* frame end interrupt. The display list thus becomes active.
*/
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
b/drivers/media/platform/vsp1/vsp1_drm.c
index 0459b970e9da..a714c53601b6 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -368,6 +368,17 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int
pipe_index,
return -EINVAL;
}
+ if (!(vsp1_feature(vsp1, VSP1_HAS_EXT_DL)) && cfg->interlaced) {
+ /*
+* Interlaced support requires extended display lists to
+* provide the auto-fld feature with the DU.
+*/
+ dev_dbg(vsp1->dev, "Interlaced unsupported on this output\n");
+ return -EINVAL;
+ }
+
+ rpf->interlaced = cfg->interlaced;
+
rpf->fmtinfo = fmtinfo;
rpf->format.num_planes = fmtinfo->planes;
rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h
b/drivers/media/platform/vsp1/vsp1_regs.h
index 43ad68ff3167..e2dffbe82809 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -31,6 +31,7 @@
#define VI6_SRESET_SRTS(n) (1 << (n))
#define VI6_STATUS 0x0038
+#define VI6_STATUS_FLD_STD(n) (1 << ((n) + 28))
#define VI6_STATUS_SYS_ACT(n) (1 << ((n) + 8))
#define VI6_WPF_IRQ_ENB(n) (0x0048 + (n) * 12)
diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c
b/drivers/media/platform/vsp1/vsp1_rpf.c
index 67f2fb3e0611..86ac8f488b7a 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -24,6 +24,20 @@
#define RPF_MAX_WIDTH 8190
#define RPF_MAX_HEIGHT 8190
+/* Pre extended display list command data structure */
+struct vsp1_extcmd_auto_fld_body {
+ u32 top_y0;
+ u32 bottom_y0;
+ u32 top_c0;
+ u32 bottom_c0;
+ u32 top_c1;
+ u32 bottom_c1;
+ u32 reserved0;
+ u32 reserved1;
+} __packed;
+
+#define VSP1_DL_EXT_AUTOFLD_INTBIT(0)
+
/*
-
* Device Access
*/
@@ -68,6 +82,14 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
pstride |= format->plane_fmt[1].bytesperline
<< VI6_RPF_SRCM_PSTRIDE_C_SHIFT;
+ /*
+* pstride has both STRIDE_Y and STRIDE_C, but multiplying the whole
+* of pstride by 2 is conveniently OK here as we are multiplying both
+* values.
+*/
+ if (rpf->interlaced)
+ pstride *= 2;
+
vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_PSTRIDE, pstride);
/* Format */
@@ -104,6 +126,9 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
top = compose->top;
}
+ if (rpf->interlaced)
+ top /= 2;
+
vsp1_rpf_write(rpf, dlb, VI6_RPF_LOC,
[PATCH v2 11/11] drm: rcar-du: Support interlaced video output through vsp1
Use the newly exposed VSP1 interface to enable interlaced frame support through the VSP1 lif pipelines. Signed-off-by: Kieran Bingham --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 1 + drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5685d5af6998..9854d9deb944 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -248,6 +248,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) /* Signal polarities */ value = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0) | ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? DSMR_HSL : 0) + | ((mode->flags & DRM_MODE_FLAG_INTERLACE) ? DSMR_ODEV : 0) | DSMR_DIPM_DISP | DSMR_CSPM; rcar_du_crtc_write(rcrtc, DSMR, value); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c33840b..5e47daef8bd2 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -178,6 +178,9 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane) }; unsigned int i; + cfg.interlaced = !!(plane->plane.state->crtc->mode.flags + & DRM_MODE_FLAG_INTERLACE); + cfg.src.left = state->state.src.x1 >> 16; cfg.src.top = state->state.src.y1 >> 16; cfg.src.width = drm_rect_width(&state->state.src) >> 16; -- git-series 0.9.1
[PATCH v2 09/11] media: vsp1: Provide support for extended command pools
VSPD and VSP-DL devices can provide extended display lists supporting
extended command display list objects.
These extended commands require their own dma memory areas for a header
and body specific to the command type.
Implement a command pool to allocate all necessary memory in a single
DMA allocation to reduce pressure on the TLB, and provide convenient
re-usable command objects for the entities to utilise.
Signed-off-by: Kieran Bingham
---
v2:
- Fix spelling typo in commit message
- constify, and staticify the instantiation of vsp1_extended_commands
- s/autfld_cmds/autofld_cmds/
- staticify cmd pool functions (Thanks kbuild-bot)
drivers/media/platform/vsp1/vsp1_dl.c | 190 +++-
drivers/media/platform/vsp1/vsp1_dl.h | 3 +-
2 files changed, 193 insertions(+)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index cd91b50deed1..d8392bd866f1 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -121,6 +121,30 @@ struct vsp1_dl_body_pool {
};
/**
+ * struct vsp1_cmd_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_cmd_pool {
+ /* DMA allocation */
+ dma_addr_t dma;
+ size_t size;
+ void *mem;
+
+ struct vsp1_dl_ext_cmd *cmds;
+ 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
@@ -163,6 +187,7 @@ struct vsp1_dl_list {
* @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
+ * @autofld_cmds: command pool to support auto-fld interlaced mode
*/
struct vsp1_dl_manager {
unsigned int index;
@@ -176,6 +201,7 @@ struct vsp1_dl_manager {
struct vsp1_dl_list *pending;
struct vsp1_dl_body_pool *pool;
+ struct vsp1_dl_cmd_pool *autofld_cmds;
};
/*
-
@@ -339,6 +365,139 @@ void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32
reg, u32 data)
}
/*
-
+ * Display List Extended Command Management
+ */
+
+enum vsp1_extcmd_type {
+ VSP1_EXTCMD_AUTODISP,
+ VSP1_EXTCMD_AUTOFLD,
+};
+
+struct vsp1_extended_command_info {
+ u16 opcode;
+ size_t body_size;
+} static const vsp1_extended_commands[] = {
+ [VSP1_EXTCMD_AUTODISP] = { 0x02, 96 },
+ [VSP1_EXTCMD_AUTOFLD] = { 0x03, 160 },
+};
+
+/**
+ * vsp1_dl_cmd_pool_create - Create a pool of commands from a single allocation
+ * @vsp1: The VSP1 device
+ * @type: The command pool type
+ * @num_commands: The quantity of commands to allocate
+ *
+ * Allocate a pool of commands each with enough memory to contain the private
+ * data of each command. The allocation sizes are dependent upon the command
+ * type.
+ *
+ * Return a pointer to a pool on success or NULL if memory can't be allocated.
+ */
+static struct vsp1_dl_cmd_pool *
+vsp1_dl_cmd_pool_create(struct vsp1_device *vsp1, enum vsp1_extcmd_type type,
+ unsigned int num_cmds)
+{
+ struct vsp1_dl_cmd_pool *pool;
+ unsigned int i;
+ size_t cmd_size;
+
+ pool = kzalloc(sizeof(*pool), GFP_KERNEL);
+ if (!pool)
+ return NULL;
+
+ pool->cmds = kcalloc(num_cmds, sizeof(*pool->cmds), GFP_KERNEL);
+ if (!pool->cmds) {
+ kfree(pool);
+ return NULL;
+ }
+
+ cmd_size = sizeof(struct vsp1_dl_ext_cmd_header) +
+ vsp1_extended_commands[type].body_size;
+ cmd_size = ALIGN(cmd_size, 16);
+
+ pool->size = cmd_size * num_cmds;
+ pool->mem = dma_alloc_wc(vsp1->bus_master, pool->size, &pool->dma,
+GFP_KERNEL);
+ if (!pool->mem) {
+ kfree(pool->cmds);
+ kfree(pool);
+ return NULL;
+ }
+
+ spin_lock_init(&pool->lock);
+ INIT_LIST_HEAD(&pool->free);
+
+ for (i = 0; i < num_cmds; ++i) {
+ struct vsp1_dl_ext_cmd *cmd = &pool->cmds[i];
+ size_t cmd_offset = i * cmd_size;
+ size_t data_offset = sizeof(struct vsp1_dl_ext_cmd_header) +
+cmd_offset;
+
+ cmd->pool = pool;
+ cmd->cmd_opcode = vsp1_extended_commands[type].opcode;
+
+ /* TODO: Auto-disp can utili
[PATCH v2 02/11] media: vsp1: Remove packed attributes from aligned structures
The use of the packed attribute can cause a performance penalty for
all accesses to the struct members, as the compiler will assume that the
structure has the potential to have an unaligned base.
These structures are all correctly aligned and contain no holes, thus
the attribute is redundant and negatively impacts performance, so we
remove the attributes entirely.
Signed-off-by: Kieran Bingham
---
v2
- Remove attributes entirely
drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 37e2c984fbf3..85795b55a357 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -29,19 +29,19 @@
struct vsp1_dl_header_list {
u32 num_bytes;
u32 addr;
-} __attribute__((__packed__));
+};
struct vsp1_dl_header {
u32 num_lists;
struct vsp1_dl_header_list lists[8];
u32 next_header;
u32 flags;
-} __attribute__((__packed__));
+};
struct vsp1_dl_entry {
u32 addr;
u32 data;
-} __attribute__((__packed__));
+};
/**
* struct vsp1_dl_body - Display list body
--
git-series 0.9.1
[PATCH v2 06/11] media: vsp1: Provide VSP1 feature helper macro
The VSP1 devices define their specific capabilities through features
marked in their device info structure. Various parts of the code read
this info structure to infer if the features are available.
Wrap this into a more readable vsp1_feature(vsp1, f) macro to ensure
that usage is consistent throughout the driver.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1.h | 2 ++
drivers/media/platform/vsp1/vsp1_drv.c | 16
drivers/media/platform/vsp1/vsp1_wpf.c | 6 +++---
3 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1.h
b/drivers/media/platform/vsp1/vsp1.h
index 78ef838416b3..1c080538c993 100644
--- a/drivers/media/platform/vsp1/vsp1.h
+++ b/drivers/media/platform/vsp1/vsp1.h
@@ -69,6 +69,8 @@ struct vsp1_device_info {
bool uapi;
};
+#define vsp1_feature(vsp1, f) ((vsp1)->info->features & (f))
+
struct vsp1_device {
struct device *dev;
const struct vsp1_device_info *info;
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
b/drivers/media/platform/vsp1/vsp1_drv.c
index eed9516e25e1..6fa0019ffc6e 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -268,7 +268,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
}
/* Instantiate all the entities. */
- if (vsp1->info->features & VSP1_HAS_BRS) {
+ if (vsp1_feature(vsp1, VSP1_HAS_BRS)) {
vsp1->brs = vsp1_bru_create(vsp1, VSP1_ENTITY_BRS);
if (IS_ERR(vsp1->brs)) {
ret = PTR_ERR(vsp1->brs);
@@ -278,7 +278,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
list_add_tail(&vsp1->brs->entity.list_dev, &vsp1->entities);
}
- if (vsp1->info->features & VSP1_HAS_BRU) {
+ if (vsp1_feature(vsp1, VSP1_HAS_BRU)) {
vsp1->bru = vsp1_bru_create(vsp1, VSP1_ENTITY_BRU);
if (IS_ERR(vsp1->bru)) {
ret = PTR_ERR(vsp1->bru);
@@ -288,7 +288,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
list_add_tail(&vsp1->bru->entity.list_dev, &vsp1->entities);
}
- if (vsp1->info->features & VSP1_HAS_CLU) {
+ if (vsp1_feature(vsp1, VSP1_HAS_CLU)) {
vsp1->clu = vsp1_clu_create(vsp1);
if (IS_ERR(vsp1->clu)) {
ret = PTR_ERR(vsp1->clu);
@@ -314,7 +314,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
list_add_tail(&vsp1->hst->entity.list_dev, &vsp1->entities);
- if (vsp1->info->features & VSP1_HAS_HGO && vsp1->info->uapi) {
+ if (vsp1_feature(vsp1, VSP1_HAS_HGO) && vsp1->info->uapi) {
vsp1->hgo = vsp1_hgo_create(vsp1);
if (IS_ERR(vsp1->hgo)) {
ret = PTR_ERR(vsp1->hgo);
@@ -325,7 +325,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
&vsp1->entities);
}
- if (vsp1->info->features & VSP1_HAS_HGT && vsp1->info->uapi) {
+ if (vsp1_feature(vsp1, VSP1_HAS_HGT) && vsp1->info->uapi) {
vsp1->hgt = vsp1_hgt_create(vsp1);
if (IS_ERR(vsp1->hgt)) {
ret = PTR_ERR(vsp1->hgt);
@@ -356,7 +356,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
}
}
- if (vsp1->info->features & VSP1_HAS_LUT) {
+ if (vsp1_feature(vsp1, VSP1_HAS_LUT)) {
vsp1->lut = vsp1_lut_create(vsp1);
if (IS_ERR(vsp1->lut)) {
ret = PTR_ERR(vsp1->lut);
@@ -390,7 +390,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
}
}
- if (vsp1->info->features & VSP1_HAS_SRU) {
+ if (vsp1_feature(vsp1, VSP1_HAS_SRU)) {
vsp1->sru = vsp1_sru_create(vsp1);
if (IS_ERR(vsp1->sru)) {
ret = PTR_ERR(vsp1->sru);
@@ -524,7 +524,7 @@ static int vsp1_device_init(struct vsp1_device *vsp1)
vsp1_write(vsp1, VI6_DPR_HSI_ROUTE, VI6_DPR_NODE_UNUSED);
vsp1_write(vsp1, VI6_DPR_BRU_ROUTE, VI6_DPR_NODE_UNUSED);
- if (vsp1->info->features & VSP1_HAS_BRS)
+ if (vsp1_feature(vsp1, VSP1_HAS_BRS))
vsp1_write(vsp1, VI6_DPR_ILV_BRS_ROUTE, VI6_DPR_NODE_UNUSED);
vsp1_write(vsp1, VI6_DPR_HGO_SMPPT, (7 << VI6_DPR_SMPPT_TGW_SHIFT) |
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c
b/drivers/media/platform/vsp1/vsp1_wpf.c
index 68218625549e..f90e474cf2cc 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -146,13 +146,13 @@ stat
[PATCH v2 04/11] media: vsp1: Remove unused display list structure field
The vsp1 reference in the vsp1_dl_body structure is not used.
Remove it.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 8162f4ce66eb..310ce81cd724 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -48,7 +48,6 @@ struct vsp1_dl_entry {
* @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
* @size: size of the DMA memory in bytes
@@ -62,7 +61,6 @@ struct vsp1_dl_body {
refcount_t refcnt;
struct vsp1_dl_body_pool *pool;
- struct vsp1_device *vsp1;
struct vsp1_dl_entry *entries;
dma_addr_t dma;
--
git-series 0.9.1
[PATCH v2 05/11] media: vsp1: Clean up DLM objects on error
If there is an error allocating a display list within a DLM object
the existing display lists are not free'd, and neither is the DL body
pool.
Use the existing vsp1_dlm_destroy() function to clean up on error.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 310ce81cd724..680eedb6fc9f 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -831,8 +831,10 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device
*vsp1,
struct vsp1_dl_list *dl;
dl = vsp1_dl_list_alloc(dlm, dlm->pool);
- if (!dl)
+ if (!dl) {
+ vsp1_dlm_destroy(dlm);
return NULL;
+ }
list_add_tail(&dl->list, &dlm->free);
}
--
git-series 0.9.1
[PATCH v2 03/11] media: vsp1: Rename dl_child to dl_next
Both vsp1_dl_list_commit() and __vsp1_dl_list_put() walk the display
list chain referencing the nodes as children, when in reality they are
siblings.
Update the terminology to 'dl_next' to be consistent with the
vsp1_video_pipeline_run() usage.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_dl.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
b/drivers/media/platform/vsp1/vsp1_dl.c
index 85795b55a357..8162f4ce66eb 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -400,7 +400,7 @@ struct vsp1_dl_list *vsp1_dl_list_get(struct
vsp1_dl_manager *dlm)
/* This function must be called with the display list manager lock held.*/
static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
{
- struct vsp1_dl_list *dl_child;
+ struct vsp1_dl_list *dl_next;
if (!dl)
return;
@@ -410,8 +410,8 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
* hardware operation.
*/
if (dl->has_chain) {
- list_for_each_entry(dl_child, &dl->chain, chain)
- __vsp1_dl_list_put(dl_child);
+ list_for_each_entry(dl_next, &dl->chain, chain)
+ __vsp1_dl_list_put(dl_next);
}
dl->has_chain = false;
@@ -667,17 +667,17 @@ static void vsp1_dl_list_commit_singleshot(struct
vsp1_dl_list *dl)
void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
{
struct vsp1_dl_manager *dlm = dl->dlm;
- struct vsp1_dl_list *dl_child;
+ struct vsp1_dl_list *dl_next;
unsigned long flags;
if (dlm->mode == VSP1_DL_MODE_HEADER) {
/* Fill the header for the head and chained display lists. */
vsp1_dl_list_fill_header(dl, list_empty(&dl->chain));
- list_for_each_entry(dl_child, &dl->chain, chain) {
- bool last = list_is_last(&dl_child->chain, &dl->chain);
+ list_for_each_entry(dl_next, &dl->chain, chain) {
+ bool last = list_is_last(&dl_next->chain, &dl->chain);
- vsp1_dl_list_fill_header(dl_child, last);
+ vsp1_dl_list_fill_header(dl_next, last);
}
}
--
git-series 0.9.1
[PATCH v2 01/11] media: vsp1: drm: Fix minor grammar error
The pixel format is 'unsupported'. Fix the small debug message which
incorrectly declares this.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
b/drivers/media/platform/vsp1/vsp1_drm.c
index 3c8b1952799d..0459b970e9da 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -363,7 +363,7 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int
pipe_index,
*/
fmtinfo = vsp1_get_format_info(vsp1, cfg->pixelformat);
if (!fmtinfo) {
- dev_dbg(vsp1->dev, "Unsupport pixel format %08x for RPF\n",
+ dev_dbg(vsp1->dev, "Unsupported pixel format %08x for RPF\n",
cfg->pixelformat);
return -EINVAL;
}
--
git-series 0.9.1
Re: [PATCH 02/11] media: vsp1: use kernel __packed for structures
Hi David,
Just for reference here, I've posted a v2 of this patch now titled:
[PATCH v2 02/11] media: vsp1: Remove packed attributes from aligned structures
which removes the attributes instead of modifying them.
Thanks for the pointers!
Regards
Kieran
On 13/03/18 15:03, Kieran Bingham wrote:
> Hi David,
>
> On 13/03/18 13:38, David Laight wrote:
>> From: Kieran Bingham [mailto:kieran.bingham+rene...@ideasonboard.com]
>>> On 13/03/18 11:20, David Laight wrote:
>>>> From: Kieran Bingham
>>>>> Sent: 09 March 2018 22:04
>>>>> The kernel provides a __packed definition to abstract away from the
>>>>> compiler specific attributes tag.
>>>>>
>>>>> Convert all packed structures in VSP1 to use it.
>>>>>
>>>>> Signed-off-by: Kieran Bingham
>>>>> ---
>>>>> drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
>>>>> b/drivers/media/platform/vsp1/vsp1_dl.c
>>>>> index 37e2c984fbf3..382e45c2054e 100644
>>>>> --- a/drivers/media/platform/vsp1/vsp1_dl.c
>>>>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
>>>>> @@ -29,19 +29,19 @@
>>>>> struct vsp1_dl_header_list {
>>>>> u32 num_bytes;
>>>>> u32 addr;
>>>>> -} __attribute__((__packed__));
>>>>> +} __packed;
>>>>>
>>>>> struct vsp1_dl_header {
>>>>> u32 num_lists;
>>>>> struct vsp1_dl_header_list lists[8];
>>>>> u32 next_header;
>>>>> u32 flags;
>>>>> -} __attribute__((__packed__));
>>>>> +} __packed;
>>>>>
>>>>> struct vsp1_dl_entry {
>>>>> u32 addr;
>>>>> u32 data;
>>>>> -} __attribute__((__packed__));
>>>>> +} __packed;
>>>>
>>>> Do these structures ever actually appear in misaligned memory?
>>>> If they don't then they shouldn't be marked 'packed'.
>>>
>>> I believe the declaration is to ensure that the struct definition is not
>>> altered
>>> by the compiler as these structures specifically define the layout of how
>>> the
>>> memory is read by the VSP1 hardware.
>>
>> The C language and ABI define structure layouts.
>>
>>> Certainly 2 u32's sequentially stored in a struct are unlikely to be moved
>>> or
>>> rearranged by the compiler (though I believe it would be free to do so if it
>>> chose without this attribute), but I think the declaration shows the intent
>>> of
>>> the memory structure.
>>
>> The language requires the fields be in order and the ABI stops the compiler
>> adding 'random' padding.
>>
>>> Isn't this a common approach throughout the kernel when dealing with
>>> hardware
>>> defined memory structures ?
>>
>> Absolutely not.
>> __packed tells the compiler that the structure might be on any address
>> boundary.
>> On many architectures this means the compiler must use byte accesses with
>> shifts
>> and ors for every access.
>> The most a hardware defined structure will have is a compile-time assert
>> that it is the correct size (to avoid silly errors from changes).
>
>
>
> Ok - interesting, I see what you're saying - and certainly don't want the
> compiler to be performing byte accesses on the structures unnecessarily.
>
> I'm trying to distinguish the difference here. Is the single point that
> __packed
>
> causes byte-access, where as
>
> __attribute__((__packed__));
>
> does not?
>
> Looking at the GCC docs [0]: I see that __attribute__((__packed__)) tells the
> compiler that the "structure or union is placed to minimize the memory
> required".
>
> However, the keil compiler docs[1] do certainly declare that __packed causes
> byte alignment.
>
> I was confused/thrown off here by the Kernel defining __packed as
> __attribute__((packed)) at [2].
>
> Do __attribute__((packed)) and __attribute__((__packed__)) differ ?
>
> In which case, from what I've read so far I wish "__packed" was
> "__unaligned"...
>
>
> [0]
> https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-packed-type-attribute
>
> [1] http://www.keil.com/support/man/docs/armcc/armcc_chr1359124230195.htm
>
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler-gcc.h?h=v4.16-rc5#n92
>
>
> Regards
>
> Kieran
>
>
>> David
>>
Re: [RFC v3 11/19] kunit: add Python libraries for handing KUnit config and kernel
Hi Luis, On 04/12/2018 20:47, Luis Chamberlain wrote: > On Mon, Dec 03, 2018 at 03:48:15PM -0800, Brendan Higgins wrote: >> On Thu, Nov 29, 2018 at 5:54 AM Kieran Bingham >> wrote: >>> >>> Hi Brendan, >>> >>> Thanks again for this series! >>> >>> On 28/11/2018 19:36, Brendan Higgins wrote: >>>> The ultimate goal is to create minimal isolated test binaries; in the >>>> meantime we are using UML to provide the infrastructure to run tests, so >>>> define an abstract way to configure and run tests that allow us to >>>> change the context in which tests are built without affecting the user. >>>> This also makes pretty and dynamic error reporting, and a lot of other >>>> nice features easier. >>> >>> >>> I wonder if we could somehow generate a shared library object >>> 'libkernel' or 'libumlinux' from a UM configured set of headers and >>> objects so that we could create binary targets directly ? >> >> That's an interesting idea. I think it would be difficult to figure >> out exactly where to draw the line of what goes in there and what >> needs to be built specific to a test a priori. Of course, that leads >> into the biggest problem in general, needed to know what I need to >> build to test the thing that I want to test. >> >> Nevertheless, I could definitely imagine that being useful in a lot of cases. > > Whether or not we can abstract away the kernel into such a mechanism > with uml libraries is a good question worth exploring. > > Developers working upstream do modify their kernels a lot, so we'd have > to update such libraries quite a bit, but I think that's fine too. The > *real* value I think from the above suggestion would be enterprise / > mobile distros or stable kernel maintainers which have a static kernel > they need to support for a relatively *long time*, consider a 10 year > time frame. Running unit tests without qemu with uml and libraries for > respective kernels seems real worthy. I think any such library might be something generated by the kernel build system, so if someone makes substantial changes to a core component provided by the library - it can be up to them to build a corresponding userspace library as well. We could also consider to only provide *static* libraries rather than dynamic. So any one building some userspace tool / test with this would be required to compile against (the version of) the kernel they expect perhaps... - much like we expect modules to be compiled currently. And then the userspace binary would be sufficiently able to live it's life on it's own :) > The overhead for testing a unit test for said targets, *ideally*, would > just be to to reboot into the system with such libraries available, a > unit test would just look for the respective uname -r library and mimic > that kernel, much the same way enterprise distributions today rely on > having debugging symbols available to run against crash / gdb. Having > debug modules / kernel for crash requires such effort already, so this > would just be an extra layer of other prospect tests. Oh - although, yes - there are some good concepts there - but I'm a bit weary of how easy it would be to 'run' the said test against multiple kernel version libraries... there would be a lot of possible ABI conflicts perhaps. My main initial idea for a libumlinux is to provide infrastructure such as our linked-lists and other kernel formatting so that we can take kernel code directly to userspace for test and debug (assuming that there are no hardware dependencies or things that we can't mock out) I think all of this could complement kunit of course - this isn't suggesting an alternative implementation :-) > All ideaware for now, but the roadmap seems to be paving itself. I guess all great ideas start as ideaware somehow ... Now we just have to start the race to see who can tweak the kernel build system to produce an output library first :) (I won't be upset if I don't win the race) -- Regards -- Kieran
Re: [RFC v3 11/19] kunit: add Python libraries for handing KUnit config and kernel
Hi Matthew, On 06/12/2018 15:37, Matthew Wilcox wrote: > On Thu, Dec 06, 2018 at 12:32:47PM +0000, Kieran Bingham wrote: >> On 04/12/2018 20:47, Luis Chamberlain wrote: >>> On Mon, Dec 03, 2018 at 03:48:15PM -0800, Brendan Higgins wrote: >>>> On Thu, Nov 29, 2018 at 5:54 AM Kieran Bingham >>>> wrote: >>>>> >>>>> Hi Brendan, >>>>> >>>>> Thanks again for this series! >>>>> >>>>> On 28/11/2018 19:36, Brendan Higgins wrote: >>>>>> The ultimate goal is to create minimal isolated test binaries; in the >>>>>> meantime we are using UML to provide the infrastructure to run tests, so >>>>>> define an abstract way to configure and run tests that allow us to >>>>>> change the context in which tests are built without affecting the user. >>>>>> This also makes pretty and dynamic error reporting, and a lot of other >>>>>> nice features easier. >>>>> >>>>> >>>>> I wonder if we could somehow generate a shared library object >>>>> 'libkernel' or 'libumlinux' from a UM configured set of headers and >>>>> objects so that we could create binary targets directly ? >>>> >>>> That's an interesting idea. I think it would be difficult to figure >>>> out exactly where to draw the line of what goes in there and what >>>> needs to be built specific to a test a priori. Of course, that leads >>>> into the biggest problem in general, needed to know what I need to >>>> build to test the thing that I want to test. >>>> >>>> Nevertheless, I could definitely imagine that being useful in a lot of >>>> cases. >>> >>> Whether or not we can abstract away the kernel into such a mechanism >>> with uml libraries is a good question worth exploring. >>> >>> Developers working upstream do modify their kernels a lot, so we'd have >>> to update such libraries quite a bit, but I think that's fine too. The >>> *real* value I think from the above suggestion would be enterprise / >>> mobile distros or stable kernel maintainers which have a static kernel >>> they need to support for a relatively *long time*, consider a 10 year >>> time frame. Running unit tests without qemu with uml and libraries for >>> respective kernels seems real worthy. >> >> I think any such library might be something generated by the kernel >> build system, so if someone makes substantial changes to a core >> component provided by the library - it can be up to them to build a >> corresponding userspace library as well. >> >> We could also consider to only provide *static* libraries rather than >> dynamic. So any one building some userspace tool / test with this would >> be required to compile against (the version of) the kernel they expect >> perhaps... - much like we expect modules to be compiled currently. >> >> And then the userspace binary would be sufficiently able to live it's >> life on it's own :) >> >>> The overhead for testing a unit test for said targets, *ideally*, would >>> just be to to reboot into the system with such libraries available, a >>> unit test would just look for the respective uname -r library and mimic >>> that kernel, much the same way enterprise distributions today rely on >>> having debugging symbols available to run against crash / gdb. Having >>> debug modules / kernel for crash requires such effort already, so this >>> would just be an extra layer of other prospect tests. >> >> Oh - although, yes - there are some good concepts there - but I'm a bit >> weary of how easy it would be to 'run' the said test against multiple >> kernel version libraries... there would be a lot of possible ABI >> conflicts perhaps. >> >> My main initial idea for a libumlinux is to provide infrastructure such >> as our linked-lists and other kernel formatting so that we can take >> kernel code directly to userspace for test and debug (assuming that >> there are no hardware dependencies or things that we can't mock out) >> >> I think all of this could complement kunit of course - this isn't >> suggesting an alternative implementation :-) > > I suspect the reason Luis cc'd me on this is that we already have some > artisinally-crafted userspace kernel-mocking interfaces under tools/. Aha - excellent - I had hoped to grab you at Plumbers to talk about t
[PATCH v4] scripts/gdb: fix lx-version string output
From: Du Changbin
A bug is present in GDB which causes early string termination when
parsing variables. This has been reported [0], but we should ensure that
we can support at least basic printing of the core kernel strings.
For current gdb version (has been tested with 7.3 and 8.1), 'lx-version'
only prints one character.
(gdb) lx-version
L(gdb)
This can be fixed by casting 'linux_banner' as (char *).
(gdb) lx-version
Linux version 4.19.0-rc1+ (changbin@acer) (gcc version 7.3.0 (Ubuntu
7.3.0-16ubuntu3)) #21 SMP Sat Sep 1 21:43:30 CST 2018
[0] https://sourceware.org/bugzilla/show_bug.cgi?id=20077
Fixes: 2d061d999424 ("scripts/gdb: add version command")
Signed-off-by: Du Changbin
Acked-by: Jan Kiszka
[Kieran: Added detail to commit message]
Signed-off-by: Kieran Bingham
---
Andrew - could you consider this patch for inclusion in your tree
please?
Regards
Kieran
scripts/gdb/linux/proc.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/gdb/linux/proc.py b/scripts/gdb/linux/proc.py
index 086d27223c0c..0aebd7565b03 100644
--- a/scripts/gdb/linux/proc.py
+++ b/scripts/gdb/linux/proc.py
@@ -41,7 +41,7 @@ class LxVersion(gdb.Command):
def invoke(self, arg, from_tty):
# linux_banner should contain a newline
-gdb.write(gdb.parse_and_eval("linux_banner").string())
+gdb.write(gdb.parse_and_eval("(char *)linux_banner").string())
LxVersion()
--
2.17.1
[PATCH 0/2] drm: rcar-du: Add interlaced feature support flag
The R-Car DU on the D3 and E3 does not support interlaced pipelines, thus we need to have a means to reject interlaced configurations on those platoforms. Provide a new feature flag, and add that flag to all existing devices which currently support interlaced pipelines. When D3 and E3 support is added, this feature flag shall be ommited. Kieran Bingham (2): drm: rcar-du: Refactor Feature and Quirk definitions drm: rcar-du: Add interlaced feature flag drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 14 +++ drivers/gpu/drm/rcar-du/rcar_du_drv.c | 32 +- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 9 3 files changed, 40 insertions(+), 15 deletions(-) -- 2.17.1
Re: [PATCH -next] drm/shmob: Fix return value check in shmob_drm_probe
Hi YeuHaibing,
On 17/12/2018 09:18, YueHaibing wrote:
> In case of error, the function devm_ioremap_resource() returns ERR_PTR()
> and never returns NULL. The NULL test in the return value check should
> be replaced with IS_ERR().
>
This looks correct to me.
I wonder if this failure pattern occurs a lot in the kernel.
I wonder if we have a cocci-check for this ?
It seems the closest we have is scripts/coccinelle/null/eno.cocci.
Perhaps that could be extended sometime to check these globally.
> Fixes: 8f1597c8f1a5 ("drm: shmobile: Perform initialization/cleanup at
> probe/remove time")
> Signed-off-by: YueHaibing
Reviewed-by: Kieran Bingham
> ---
> drivers/gpu/drm/shmobile/shmob_drm_drv.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> index 8554102..f2cfd16 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> @@ -229,8 +229,8 @@ static int shmob_drm_probe(struct platform_device *pdev)
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> sdev->mmio = devm_ioremap_resource(&pdev->dev, res);
> - if (sdev->mmio == NULL)
> - return -ENOMEM;
> + if (IS_ERR(sdev->mmio))
> + return PTR_ERR(sdev->mmio);
>
> ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
> if (ret < 0)
>
Re: [PATCH] scripts/gdb: fix lx-version for gdb 7.3-
Hi Du,
On 17/10/18 03:36, Du Changbin wrote:
> For gdb version less than 7.3, lx-version only one character.
> (gdb) lx-version
> L(gdb)
>
> This can be fixed by casting 'linux_banner' as (char *).
> (gdb) lx-version
> Linux version 4.19.0-rc1+ (changbin@acer) (gcc version 7.3.0 (Ubuntu
> 7.3.0-16ubuntu3)) #21 SMP Sat Sep 1 21:43:30 CST 2018
>
> gdb 7.4 seems to be no such issue.
>
> Signed-off-by: Du Changbin
> ---
> scripts/gdb/linux/proc.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/gdb/linux/proc.py b/scripts/gdb/linux/proc.py
> index 086d27223c0c..0aebd7565b03 100644
> --- a/scripts/gdb/linux/proc.py
> +++ b/scripts/gdb/linux/proc.py
> @@ -41,7 +41,7 @@ class LxVersion(gdb.Command):
>
> def invoke(self, arg, from_tty):
> # linux_banner should contain a newline
> -gdb.write(gdb.parse_and_eval("linux_banner").string())
> +gdb.write(gdb.parse_and_eval("(char *)linux_banner").string())
Interesting - this is an issue I reported to the GDB team after I'd
tried to dig into the fault.
https://sourceware.org/bugzilla/show_bug.cgi?id=20077
the parse_and_eval finds a string length of 0, but then later in the GBD
parsing it gets incremented to 1, hence the single char. I guess it's
some failure in GDB determining the type of the pointer.
However, from reading the bugzilla entry and from what I recall - it
only occurred if you had done a backtrace before calling lx-version.
Did you discover that it would always return only the single character
in your testing ? Even if it was the first command executed?
My worry is - that if this fixes lx-version - do we also need to
manually cast lx-cmdline as well ? (I never saw the issue occur on that
cmd).
I think considering the age of the bugzilla at GDB, and the lack of any
GDB level fix or understanding - we should certainly apply this fix here
- and probably include it in the stable trees too.
With the title fix as highlighted by Jan,
Reviewed-by: Kieran Bingham
Would you be able to test the issue with the lx-cmdline command as well
please? And if it is suffering the same issue - perhaps you could either
submit a second patch to update that entry - or respin this one to
include both?
>
> LxVersion()
>
>
--
Regards
--
Kieran
Re: [RFC v1 06/31] arch: um: enabled running kunit from User Mode Linux
Hi Brendan,
I very excitedly jumped on these patches to try them out, as this is
essentially something I was trying to do a few weeks back.
On 17/10/18 00:50, Brendan Higgins wrote:
> Makes minimum number of changes outside of the KUnit directories for
> KUnit to build and run using UML.
>
> Signed-off-by: Brendan Higgins
> ---
> Makefile | 2 +-
> arch/um/Kconfig.rest | 3 +++
But this file isn't present on v4.19-rc8
It looks like the file is removed at f163977d21a2 ("um: cleanup Kconfig
files")
What version have you currently based these patches on?
Do you expect to keep a branch somewhere that's easy to pull in?
Please add me to the CC list as an interested party on later versions :-)
It might be of interest to the automated testing mailing list too ? (Tim?)
--
regards
Kieran
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 863f58503beed..c8e659468ed49 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -944,7 +944,7 @@ endif
>
>
> ifeq ($(KBUILD_EXTMOD),)
> -core-y += kernel/ certs/ mm/ fs/ ipc/ security/ crypto/ block/
> +core-y += kernel/ certs/ mm/ fs/ ipc/ security/ crypto/ block/
> kunit/
>
> vmlinux-dirs := $(patsubst %/,%,$(filter %/, $(init-y) $(init-m) \
>$(core-y) $(core-m) $(drivers-y) $(drivers-m) \
> diff --git a/arch/um/Kconfig.rest b/arch/um/Kconfig.rest
> index 08327b9c0cbea..484c4cfcad1ef 100644
> --- a/arch/um/Kconfig.rest
> +++ b/arch/um/Kconfig.rest
> @@ -1,4 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> +
> +source "kunit/Kconfig"
> +
> source "init/Kconfig"
>
> source "kernel/Kconfig.freezer"
>
Re: [PATCH] media: intel-ipu3: cio2: Remove redundant definitions
Hi Rajmohan
Thank you for the patch,
On 10/10/18 00:42, Rajmohan Mani wrote:
> Removed redundant CIO2_IMAGE_MAX_* definitions
>
> Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
>
> Signed-off-by: Rajmohan Mani
Reviewed-by: Kieran Bingham
Looks like this {sh,c}ould be bundled in with
"[PATCH 0/2] Trivial CIO2 patches" from Sakari at integration.
--
Regards
Kieran Bingham
> ---
> drivers/media/pci/intel/ipu3/ipu3-cio2.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index 240635be7a31..7caab9b8c2b9 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -10,8 +10,6 @@
> #define CIO2_PCI_ID 0x9d32
> #define CIO2_PCI_BAR 0
> #define CIO2_DMA_MASKDMA_BIT_MASK(39)
> -#define CIO2_IMAGE_MAX_WIDTH 4224
> -#define CIO2_IMAGE_MAX_LENGTH3136
>
> #define CIO2_IMAGE_MAX_WIDTH 4224
> #define CIO2_IMAGE_MAX_LENGTH3136
>
Re: [PATCH] media: vivid: Support 480p for webcam capture
On 06/10/18 11:29, Keiichi Watanabe wrote:
> Hi all,
>
> On Fri, Oct 5, 2018 at 6:18 PM, Hans Verkuil wrote:
>> On 10/03/18 09:08, Keiichi Watanabe wrote:
>>> I think 480p is a common frame size and it's worth supporting in vivid.
>>> But, my patch might be ad-hoc. Actually, I'm not sure which values are
>>> suitable for the intervals.
>>
>> I can apply this ad-hoc patch as-is.
>>
>> Or do you want to postpone this and work on a more generic solution?
>> Although I am not sure what that would look like.
>
> I prefer providing a more flexible way rather than this ad-hoc patch.
> It would be helpful if there is a way of changing supported frame sizes
> easily.
> Perhaps, Kieran and me would use it, at least:)
>
o/
>>
>> Proposals are welcome!
>>
>> The main purpose of this code is to have something that kind of acts like
>> a real webcam that has various resolutions, and a slower framerate for
>> higher resolutions (as you would expect).
>
> This sounds reasonable, so I guess we can keep this way as default and
> provide a way for specifying extra frame sizes as an option.
>
> For example, how about a module option like this?
> "webcam_sizes=640x480@15,320x240@30"
>
> If this parameter is passed to vivid, vivid works like a webcam that
> supports the following 7 pairs of frame size and fps:
> - 5 pairs of frame sizes and fps, as with the current implementation
> - 640x480 (15fps)
> - 320x240 (30fps)
I like the concept of being able to specify as a module parameter.
Perhaps we could have a magic marker on the string to define if the
existing frame sizes should be kept - or if this is just a complete
override ?
vivid.webcam=640x480@15,320x240@30 # Specify sizes explicitly
vivid.webcam=+640x480@15,320x240@30 # Append to existing frames
^ Magic Marker
We might of course want multiple rates per frame,
vivid.webcam=640x480@15-25-30-90-120, # Separator to be defined...
>
> If this parameter is not passed, vivid's behavior won't change from
> the current one.
>
> How do you think?
>
> We might want to stop using the default framesizes, i.e. vivid only
> supports framesize/fps that passed as the option.
> But, if we do so, the parameter will become mandatory and it would be
> annoying.
I think mandatory would be annoying yes.
Thus my suggestion for a magic marker above :)
--
Kieran
> So, I personally like to keep the default framesizes.
>
> Best regards,
> Kei
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>> We might want to add a more flexible/extensible way to specify frame sizes.
>>> e.g. passing frame sizes and intervals as module parameters
>>>
>>> Kei
>>>
>>> On Wed, Oct 3, 2018 at 4:06 PM, Keiichi Watanabe
>>> wrote:
Support 640x480 as a frame size for video input devices of vivid.
Signed-off-by: Keiichi Watanabe
---
drivers/media/platform/vivid/vivid-vid-cap.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c
b/drivers/media/platform/vivid/vivid-vid-cap.c
index 58e14dd1dcd3..da80bf4bc365 100644
--- a/drivers/media/platform/vivid/vivid-vid-cap.c
+++ b/drivers/media/platform/vivid/vivid-vid-cap.c
@@ -51,7 +51,7 @@ static const struct vivid_fmt formats_ovl[] = {
};
/* The number of discrete webcam framesizes */
-#define VIVID_WEBCAM_SIZES 5
+#define VIVID_WEBCAM_SIZES 6
/* The number of discrete webcam frameintervals */
#define VIVID_WEBCAM_IVALS (VIVID_WEBCAM_SIZES * 2)
@@ -59,6 +59,7 @@ static const struct vivid_fmt formats_ovl[] = {
static const struct v4l2_frmsize_discrete
webcam_sizes[VIVID_WEBCAM_SIZES] = {
{ 320, 180 },
{ 640, 360 },
+ { 640, 480 },
{ 1280, 720 },
{ 1920, 1080 },
{ 3840, 2160 },
@@ -75,6 +76,8 @@ static const struct v4l2_fract
webcam_intervals[VIVID_WEBCAM_IVALS] = {
{ 1, 5 },
{ 1, 10 },
{ 1, 15 },
+ { 1, 15 },
+ { 1, 25 },
{ 1, 25 },
{ 1, 30 },
{ 1, 50 },
--
2.19.0.605.g01d371f741-goog
>>
--
Regards
--
Kieran
Re: [PATCH] media: vivid: Support 480p for webcam capture
On 08/10/18 10:00, Hans Verkuil wrote: > On 10/08/2018 10:35 AM, Kieran Bingham wrote: >> On 06/10/18 11:29, Keiichi Watanabe wrote: >>> Hi all, >>> >>> On Fri, Oct 5, 2018 at 6:18 PM, Hans Verkuil wrote: >>>> On 10/03/18 09:08, Keiichi Watanabe wrote: >>>>> I think 480p is a common frame size and it's worth supporting in vivid. >>>>> But, my patch might be ad-hoc. Actually, I'm not sure which values are >>>>> suitable for the intervals. >>>> >>>> I can apply this ad-hoc patch as-is. >>>> >>>> Or do you want to postpone this and work on a more generic solution? >>>> Although I am not sure what that would look like. >>> >>> I prefer providing a more flexible way rather than this ad-hoc patch. >>> It would be helpful if there is a way of changing supported frame sizes >>> easily. >>> Perhaps, Kieran and me would use it, at least:) >>> >> >> o/ >> >>>> >>>> Proposals are welcome! >>>> >>>> The main purpose of this code is to have something that kind of acts like >>>> a real webcam that has various resolutions, and a slower framerate for >>>> higher resolutions (as you would expect). >>> >>> This sounds reasonable, so I guess we can keep this way as default and >>> provide a way for specifying extra frame sizes as an option. >>> >>> For example, how about a module option like this? >>> "webcam_sizes=640x480@15,320x240@30" >>> >>> If this parameter is passed to vivid, vivid works like a webcam that >>> supports the following 7 pairs of frame size and fps: >>> - 5 pairs of frame sizes and fps, as with the current implementation >>> - 640x480 (15fps) >>> - 320x240 (30fps) >> >> I like the concept of being able to specify as a module parameter. >> >> Perhaps we could have a magic marker on the string to define if the >> existing frame sizes should be kept - or if this is just a complete >> override ? > > No, just override. It's hard otherwise since you would have to keep the > lists sorted by resolution/fps. > > If you want to set it yourself, then just do a complete override. OK. That's reasonable for me. @@ -9,3 +9,0 @@ suggestion - magic-marker >> vivid.webcam=640x480@15,320x240@30 # Specify sizes explicitly >> vivid.webcam=+640x480@15,320x240@30 # Append to existing frames >> ^ Magic Marker >> >> We might of course want multiple rates per frame, >> >> vivid.webcam=640x480@15-25-30-90-120, # Separator to be defined... > > Stick to @, so: vivid.webcam=640x480@15@25@30@90@120 Yes, of course that makes a lot more sense now I see it :-) -- Kieran > This looks like a good proposal to me. > > BTW, I still would like to add 640x480 added to the default list: > it's a common resolution and it makes sense to add it. > > So I plan to accept the patch regardless. > > Regards, > > Hans > >> >> >>> >>> If this parameter is not passed, vivid's behavior won't change from >>> the current one. >>> >>> How do you think? >>> >>> We might want to stop using the default framesizes, i.e. vivid only >>> supports framesize/fps that passed as the option. >>> But, if we do so, the parameter will become mandatory and it would be >>> annoying. >> >> I think mandatory would be annoying yes. >> >> Thus my suggestion for a magic marker above :) >> >> -- >> Kieran >> >> >>> So, I personally like to keep the default framesizes. >>> >>> Best regards, >>> Kei >>> >>>> >>>> Regards, >>>> >>>> Hans >>>> >>>>> >>>>> We might want to add a more flexible/extensible way to specify frame >>>>> sizes. >>>>> e.g. passing frame sizes and intervals as module parameters >>>>> >>>>> Kei >>>>> >>>>> On Wed, Oct 3, 2018 at 4:06 PM, Keiichi Watanabe >>>>> wrote: >>>>>> Support 640x480 as a frame size for video input devices of vivid. >>>>>> >>>>>> Signed-off-by: Keiichi Watanabe >>>>>> --- >>>>>> drivers/media/platform/vivid/vivid-vid-cap.c | 5 - >>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>> &g
Re: [PATCH] kbuild: fix kernel/bounds.c 'W=1' warning
On 06/10/18 23:06, Masahiro Yamada wrote:
> On Sun, Oct 7, 2018 at 6:58 AM Masahiro Yamada
> wrote:
>>
>> Hi Miguel,
>>
>>
>> On Sun, Oct 7, 2018 at 6:18 AM Miguel Ojeda
>> wrote:
>>>
>>> On Fri, Oct 5, 2018 at 10:35 AM Arnd Bergmann wrote:
Building any configuration with 'make W=1' produces a warning:
kernel/bounds.c:16:6: warnign: no previous prototype for 'foo'
[-Wmissing-prototypes]
When also passing -Werror, this prevents us from building any
other files. Nobody ever calls the function, but we can't make
it 'static' either since we want the compiler output.
Calling it 'main' instead however avoids the warning, because gcc
does not insist on having a declaration for main.
>>>
>>> I think marking the function as static __used should do the trick and
>>> would be less confusing.
>>
>>
>>
>>
>> I tried __used, but I still see the warning.
>>
>>
>> masahiro@grover:~/ref/linux$ git diff
>> diff --git a/kernel/bounds.c b/kernel/bounds.c
>> index c373e88..aee0101 100644
>> --- a/kernel/bounds.c
>> +++ b/kernel/bounds.c
>> @@ -13,7 +13,7 @@
>> #include
>> #include
>>
>> -void foo(void)
>> +void __used foo(void)
>> {
>> /* The enum constants to put into include/generated/bounds.h */
>> DEFINE(NR_PAGEFLAGS, __NR_PAGEFLAGS);
>> masahiro@grover:~/ref/linux$ make W=1 prepare
>> CC kernel/bounds.s
>> kernel/bounds.c:16:13: warning: no previous prototype for ‘foo’
>> [-Wmissing-prototypes]
>> void __used foo(void)
>> ^
>> CC arch/x86/kernel/asm-offsets.s
>
>
>
> Sorry, I forgot to add 'static'.
>
> Adding both static and __used worked for me,
> and I like the idea.
>
Aha - I'd also tried converting to static in my earlier attempts, but
didn't realise we had __used!
updating as "static __used" causes the following diff:
diff -Nurp bounds.s.foo bounds.s.static-used
--- bounds.s.foo2018-10-05 10:20:53.269941404 +0100
+++ bounds.s.static-used2018-10-08 10:51:18.079309049 +0100
@@ -110,7 +110,6 @@
#NO_APP
.align 2
.p2align 3,,7
- .global foo
.type foo, %function
foo:
stp x29, x30, [sp, -16]!//,,,
I'd say this is a pretty good alternative fix - however I see Arnd's
version is already on it's way though akpm's tree...
https://ozlabs.org/~akpm/mmots/broken-out/kbuild-fix-kernel-boundsc-w%3D1-warning.patch
Anyway, as long as one of the variants gets there I'll be happy :)
--
Regards
Kieran
[PATCH v4 4/4] media: i2c: Add RDACM20 driver
From: Kieran Bingham
The RDACM20 is a GMSL camera supporting 1280x800 resolution images
developed by IMI based on an Omnivision 10635 sensor and a Maxim MAX9271
GMSL serializer.
The GMSL link carries power, control (I2C) and video data over a
single coax cable.
Signed-off-by: Jacopo Mondi
Signed-off-by: Laurent Pinchart
Signed-off-by: Niklas Söderlund
Signed-off-by: Kieran Bingham
---
v2:
- Fix MAINTAINERS entry
v3:
- Use new V4L2_MBUS_CSI2_DPHY bus type
- Remove 'always zero' error print
- Fix module description
---
MAINTAINERS | 10 +
drivers/media/i2c/Kconfig | 11 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/rdacm20-ov10635.h | 953
drivers/media/i2c/rdacm20.c | 635 ++
5 files changed, 1610 insertions(+)
create mode 100644 drivers/media/i2c/rdacm20-ov10635.h
create mode 100644 drivers/media/i2c/rdacm20.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 745f0fd1fff1..26ef20087a43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12230,6 +12230,16 @@ S: Supported
T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
F: tools/testing/selftests/rcutorture
+RDACM20 Camera Sensor
+M: Jacopo Mondi
+M: Kieran Bingham
+M: Laurent Pinchart
+M: Niklas Söderlund
+L: linux-me...@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/media/i2c/rdacm20.txt
+F: drivers/media/i2c/rdacm20*
+
RDC R-321X SoC
M: Florian Fainelli
S: Maintained
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index eadc00bdd3bf..5eded5e337ec 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -989,6 +989,17 @@ config VIDEO_S5C73M3
This is a V4L2 sensor driver for Samsung S5C73M3
8 Mpixel camera.
+config VIDEO_RDACM20
+ tristate "IMI RDACM20 camera support"
+ depends on I2C && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
+ select V4L2_FWNODE
+ help
+ This driver supports the IMI RDACM20 GMSL camera, used in
+ ADAS systems.
+
+ This camera should be used in conjunction with a GMSL
+ deserialiser such as the MAX9286.
+
comment "Flash devices"
config VIDEO_ADP1653
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 4de7fe62b179..121d28283d45 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -111,5 +111,6 @@ obj-$(CONFIG_VIDEO_IMX274) += imx274.o
obj-$(CONFIG_VIDEO_IMX319) += imx319.o
obj-$(CONFIG_VIDEO_IMX355) += imx355.o
obj-$(CONFIG_VIDEO_MAX9286)+= max9286.o
+obj-$(CONFIG_VIDEO_RDACM20)+= rdacm20.o
obj-$(CONFIG_SDR_MAX2175) += max2175.o
diff --git a/drivers/media/i2c/rdacm20-ov10635.h
b/drivers/media/i2c/rdacm20-ov10635.h
new file mode 100644
index ..3c53a3262ee2
--- /dev/null
+++ b/drivers/media/i2c/rdacm20-ov10635.h
@@ -0,0 +1,953 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * IMI RDACM20 camera OV10635 sensor register initialization values
+ *
+ * Copyright (C) 2017-2018 Jacopo Mondi
+ * Copyright (C) 2017-2018 Kieran Bingham
+ * Copyright (C) 2017-2018 Laurent Pinchart
+ * Copyright (C) 2017-2018 Niklas Söderlund
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ * Copyright (C) 2015 Cogent Embedded, Inc.
+ *
+ */
+
+/*
+ * Generated by the OmniVision ov10635 sensor camera wizard for
+ * 1280x800@30/UYVY/BT601/8bit.
+ */
+
+#ifndef __RDACM20_OV10635_H__
+#define __RDACM20_OV10635_H__
+
+#define OV10635_SENSOR_WIDTH 1312
+#define OV10635_SENSOR_HEIGHT 814
+
+#define OV10635_MAX_WIDTH 1280
+#define OV10635_MAX_HEIGHT 800
+
+/* VTS = PCLK / FPS / HTS / 2 (= 88MHz / 1572 / 30 / 2) */
+#define OV10635_HTS1572
+/* FPS = 29,9998 */
+#define OV10635_VTS933
+
+struct ov10635_reg {
+ u16 reg;
+ u8 val;
+};
+
+static const struct ov10635_reg ov10635_regs_wizard[] = {
+ { 0x301b, 0xff },
+ { 0x301c, 0xff },
+ { 0x301a, 0xff },
+ { 0x3011, 0x42 },
+ { 0x6900, 0x0c },
+ { 0x6901, 0x19 },
+ { 0x3503, 0x10 },
+ { 0x3025, 0x03 },
+ { 0x3003, 0x16 },
+ { 0x3004, 0x30 },
+ { 0x3005, 0x40 },
+ { 0x3006, 0x91 },
+ { 0x3600, 0x74 },
+ { 0x3601, 0x2b },
+ { 0x3612, 0x00 },
+ { 0x3611, 0x67 },
+ { 0x3633, 0xca },
+ { 0x3602, 0xaf },
+ { 0x3603, 0x04 },
+ { 0x3630, 0x28 },
+ { 0x3631, 0x16 },
+ { 0x3714, 0x10 },
+ { 0x371d, 0x01 },
+ { 0x4300, 0x3a },
+ { 0x3007, 0x01 },
+ { 0x3024, 0x03 },
+ { 0x3020, 0x0a },
+ { 0x3702, 0x0d },
+ { 0x3703, 0x20 },
+ { 0x3704, 0x15 },
+ { 0x3709, 0xa8 },
+ { 0x370c, 0xc7 },
+ { 0x370d, 0x80 },
+ { 0x3712, 0x00 },
+ { 0x3713, 0x20 },
+
