* Re: [PATCH v2] dma-buf: Move sysfs work out of DMA-BUF export path
From: T.J. Mercier @ 2022-05-31 23:31 UTC (permalink / raw)
To: Christian König
Cc: Greg Kroah-Hartman, Suren Baghdasaryan, Kalesh Singh, Minchan Kim,
Greg Kroah-Hartman, John Stultz, Sumit Semwal, Hridya Valsaraju,
kernel-team, linux-media, dri-devel, linaro-mm-sig, linux-kernel,
Daniel Vetter
In-Reply-To: <38da6dcd-b395-f32f-5a47-6a8f2c6a4331@amd.com>
On Sun, May 29, 2022 at 11:12 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 25.05.22 um 23:05 schrieb T.J. Mercier:
> > On Wed, May 25, 2022 at 7:38 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >> On Tue, May 17, 2022 at 08:13:24AM +0200, Greg Kroah-Hartman wrote:
> >>> On Mon, May 16, 2022 at 05:08:05PM -0700, T.J. Mercier wrote:
> >>>> On Mon, May 16, 2022 at 12:21 PM Christian König
> >>>> <christian.koenig@amd.com> wrote:
> >>>>> Am 16.05.22 um 20:08 schrieb T.J. Mercier:
> >>>>>> On Mon, May 16, 2022 at 10:20 AM Christian König
> >>>>>> <christian.koenig@amd.com> wrote:
> >>>>>>> Am 16.05.22 um 19:13 schrieb T.J. Mercier:
> >>>>>>>> Recently, we noticed an issue where a process went into direct reclaim
> >>>>>>>> while holding the kernfs rw semaphore for sysfs in write (exclusive)
> >>>>>>>> mode. This caused processes who were doing DMA-BUF exports and releases
> >>>>>>>> to go into uninterruptible sleep since they needed to acquire the same
> >>>>>>>> semaphore for the DMA-BUF sysfs entry creation/deletion. In order to avoid
> >>>>>>>> blocking DMA-BUF export for an indeterminate amount of time while
> >>>>>>>> another process is holding the sysfs rw semaphore in exclusive mode,
> >>>>>>>> this patch moves the per-buffer sysfs file creation to the default work
> >>>>>>>> queue. Note that this can lead to a short-term inaccuracy in the dmabuf
> >>>>>>>> sysfs statistics, but this is a tradeoff to prevent the hot path from
> >>>>>>>> being blocked. A work_struct is added to dma_buf to achieve this, but as
> >>>>>>>> it is unioned with the kobject in the sysfs_entry, dma_buf does not
> >>>>>>>> increase in size.
> >>>>>>> I'm still not very keen of this approach as it strongly feels like we
> >>>>>>> are working around shortcoming somewhere else.
> >>>>>>>
> >>>>>> My read of the thread for the last version is that we're running into
> >>>>>> a situation where sysfs is getting used for something it wasn't
> >>>>>> originally intended for, but we're also stuck with this sysfs
> >>>>>> functionality for dmabufs.
> >>>>>>
> >>>>>>>> Fixes: bdb8d06dfefd ("dmabuf: Add the capability to expose DMA-BUF stats in sysfs")
> >>>>>>>> Originally-by: Hridya Valsaraju <hridya@google.com>
> >>>>>>>> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> >>>>>>>>
> >>>>>>>> ---
> >>>>>>>> See the originally submitted patch by Hridya Valsaraju here:
> >>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2022%2F1%2F4%2F1066&data=05%7C01%7Cchristian.koenig%40amd.com%7C8f00afd44b9744c45f5708da3e926503%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637891095771223650%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pubWqUyqhCWpXHhJHsoqarc3GLtB6IFB1rhgfsL4a1M%3D&reserved=0
> >>>>>>>>
> >>>>>>>> v2 changes:
> >>>>>>>> - Defer only sysfs creation instead of creation and teardown per
> >>>>>>>> Christian König
> >>>>>>>>
> >>>>>>>> - Use a work queue instead of a kthread for deferred work per
> >>>>>>>> Christian König
> >>>>>>>> ---
> >>>>>>>> drivers/dma-buf/dma-buf-sysfs-stats.c | 56 ++++++++++++++++++++-------
> >>>>>>>> include/linux/dma-buf.h | 14 ++++++-
> >>>>>>>> 2 files changed, 54 insertions(+), 16 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c
> >>>>>>>> index 2bba0babcb62..67b0a298291c 100644
> >>>>>>>> --- a/drivers/dma-buf/dma-buf-sysfs-stats.c
> >>>>>>>> +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
> >>>>>>>> @@ -11,6 +11,7 @@
> >>>>>>>> #include <linux/printk.h>
> >>>>>>>> #include <linux/slab.h>
> >>>>>>>> #include <linux/sysfs.h>
> >>>>>>>> +#include <linux/workqueue.h>
> >>>>>>>>
> >>>>>>>> #include "dma-buf-sysfs-stats.h"
> >>>>>>>>
> >>>>>>>> @@ -168,10 +169,46 @@ void dma_buf_uninit_sysfs_statistics(void)
> >>>>>>>> kset_unregister(dma_buf_stats_kset);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> +static void sysfs_add_workfn(struct work_struct *work)
> >>>>>>>> +{
> >>>>>>>> + struct dma_buf_sysfs_entry *sysfs_entry =
> >>>>>>>> + container_of(work, struct dma_buf_sysfs_entry, sysfs_add_work);
> >>>>>>>> + struct dma_buf *dmabuf = sysfs_entry->dmabuf;
> >>>>>>>> +
> >>>>>>>> + /*
> >>>>>>>> + * A dmabuf is ref-counted via its file member. If this handler holds the only
> >>>>>>>> + * reference to the dmabuf, there is no need for sysfs kobject creation. This is an
> >>>>>>>> + * optimization and a race; when the reference count drops to 1 immediately after
> >>>>>>>> + * this check it is not harmful as the sysfs entry will still get cleaned up in
> >>>>>>>> + * dma_buf_stats_teardown, which won't get called until the final dmabuf reference
> >>>>>>>> + * is released, and that can't happen until the end of this function.
> >>>>>>>> + */
> >>>>>>>> + if (file_count(dmabuf->file) > 1) {
> >>>>>>> Please completely drop that. I see absolutely no justification for this
> >>>>>>> additional complexity.
> >>>>>>>
> >>>>>> This case gets hit around 5% of the time in my testing so the else is
> >>>>>> not a completely unused branch.
> >>>>> Well I can only repeat myself: This means that your userspace is
> >>>>> severely broken!
> >>>>>
> >>>>> DMA-buf are meant to be long living objects
> >>>> This patch addresses export *latency* regardless of how long-lived the
> >>>> object is. Even a single, long-lived export will benefit from this
> >>>> change if it would otherwise be blocked on adding an object to sysfs.
> >>>> I think attempting to improve this latency still has merit.
> >>> Fixing the latency is nice, but as it's just pushing the needed work off
> >>> to another code path, it will take longer overall for the sysfs stuff to
> >>> be ready for userspace to see.
> >>>
> >>> Perhaps we need to step back and understand what this code is supposed
> >>> to be doing. As I recall, it was created because some systems do not
> >>> allow debugfs anymore, and they wanted the debugging information that
> >>> the dmabuf code was exposing to debugfs on a "normal" system. Moving
> >>> that logic to sysfs made sense, but now I am wondering why we didn't see
> >>> these issues in the debugfs code previously?
> >>>
> >>> Perhaps we should go just one step further and make a misc device node
> >>> for dmabug debugging information to be in and just have userspace
> >>> poll/read on the device node and we spit the info that used to be in
> >>> debugfs out through that? That way this only affects systems when they
> >>> want to read the information and not normal code paths? Yeah that's a
> >>> hack, but this whole thing feels overly complex now.
> >> A bit late on this discussion, but just wanted to add my +1 that we should
> >> either redesign the uapi, or fix the underlying latency issue in sysfs, or
> >> whatever else is deemed the proper fix.
> >>
> >> Making uapi interfaces async in ways that userspace can't discover is a
> >> hack that we really shouldn't consider, at least for upstream. All kinds
> >> of hilarious things might start to happen when an object exists, but not
> >> consistently in all the places where it should be visible. There's a
> >> reason sysfs has all these neat property groups so that absolutely
> >> everything is added atomically. Doing stuff later on just because usually
> >> no one notices that the illusion falls apart isn't great.
> >>
> >> Unfortunately I don't have a clear idea here what would be the right
> >> solution :-/ One idea perhaps: Should we dynamically enumerate the objects
> >> when userspace does a readdir()? That's absolutely not how sysfs works,
> >> but procfs works like that and there's discussions going around about
> >> moving these optimizations to other kernfs implementations. At least there
> >> was a recent lwn article on this:
> >>
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.net%2FArticles%2F895111%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C8f00afd44b9744c45f5708da3e926503%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637891095771223650%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Q58OZi79vmKMCZLL0pY7NniIW6hmSqyWjlEaZgqzYtM%3D&reserved=0
> >>
> >> But that would be serious amounts of work I guess.
> >> -Daniel
> >> --
> >> Daniel Vetter"
> >> Software Engineer, Intel Corporation
> >> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C8f00afd44b9744c45f5708da3e926503%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637891095771223650%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pOIl5yszzak4TPqjBYyL0mHjj%2F1nYRfNJbNPQTXBhbA%3D&reserved=0
> > Hi Daniel,
> >
> > My team has been discussing this, and I think we're approaching a
> > consensus on a way forward that involves deprecating the existing
> > uapi.
> >
> > I actually proposed a similar (but less elegant) idea to the readdir()
> > one. A new "dump_dmabuf_data" sysfs file that a user would write to,
> > which would cause a one-time creation of the per-buffer files. These
> > could be left around to become stale, or get cleaned up after first
> > read. However to me it seems impossible to correctly deal with
> > multiple simultaneous users with this technique. We're not currently
> > planning to pursue this.
> >
> > Thanks for the link to the article. That on-demand creation sounds
> > like it would allow us to keep the existing structure and files for
> > DMA-buf, assuming there is not a similar lock contention issue when
> > adding a new node to the virtual tree. :)
>
> I think that this on demand creation is even worse than the existing
> ideas, but if you can get Greg to accept the required sysfs changes than
> that's at least outside of my maintenance domain any more :)
Hah, ok. After chatting with Steven it sounds like an attempt at on
demand creation for sysfs is not likely to happen soon, as the focus
is on getting it working for tracefs first and letting it stew there
for a while to polish it. I'll check with Greg when he's available
again next week about whether that is a direction we should even
consider before moving forward from here.
>
>
> Regards,
> Christian.
^ permalink raw reply
* Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/
From: Stephen Brennan @ 2022-05-31 22:48 UTC (permalink / raw)
To: Kalesh Singh
Cc: Ioannis Ilkos, T.J. Mercier, Suren Baghdasaryan,
Cc: Android Kernel, Jonathan Corbet, Sumit Semwal,
Christian König, Andrew Morton, David Hildenbrand,
Christoph Anton Mitterer, Johannes Weiner, Colin Cross,
Mike Rapoport, Paul Gortmaker, Randy Dunlap, LKML, linux-fsdevel,
open list:DOCUMENTATION, Linux Media Mailing List,
DRI mailing list, moderated list:DMA BUFFER SHARING FRAMEWORK
In-Reply-To: <CAC_TJveDzDaYQKmuLSkGWpnuCW+gvrqdVJqq=wbzoTRjw4OoFw@mail.gmail.com>
Kalesh Singh <kaleshsingh@google.com> writes:
> On Tue, May 31, 2022 at 3:07 PM Stephen Brennan
> <stephen.s.brennan@oracle.com> wrote:
>>
>> On 5/31/22 14:25, Kalesh Singh wrote:
>> > In order to identify the type of memory a process has pinned through
>> > its open fds, add the file path to fdinfo output. This allows
>> > identifying memory types based on common prefixes. e.g. "/memfd...",
>> > "/dmabuf...", "/dev/ashmem...".
>> >
>> > Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
>> > the same as /proc/<pid>/maps which also exposes the file path of
>> > mappings; so the security permissions for accessing path is consistent
>> > with that of /proc/<pid>/maps.
>>
>> Hi Kalesh,
>
> Hi Stephen,
>
> Thanks for taking a look.
>
>>
>> I think I see the value in the size field, but I'm curious about path,
>> which is available via readlink /proc/<pid>/fd/<n>, since those are
>> symlinks to the file themselves.
>
> This could work if we are root, but the file permissions wouldn't
> allow us to do the readlink on other processes otherwise. We want to
> be able to capture the system state in production environments from
> some trusted process with ptrace read capability.
Interesting, thanks for explaining. It seems weird to have a duplicate
interface for the same information but such is life.
>>
>> File paths can contain fun characters like newlines or colons, which
>> could make parsing out filenames in this text file... fun. How would your
>> userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
>> readlink(2) API makes that easy already.
>
> I think since we have escaped the "\n" (seq_file_path(m, file, "\n")),
I really should have read through that function before commenting,
thanks for teaching me something new :)
Stephen
> then user space might parse this line like:
>
> if (strncmp(line, "path:\t", 6) == 0)
> char* path = line + 6;
>
>
> Thanks,
> Kalesh
>
>>
>> Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
>> to a different path between reading fdinfo and stating the fd)?
>>
>> Stephen
>>
>> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
>> > ---
>> >
>> > Changes from rfc:
>> > - Split adding 'size' and 'path' into a separate patches, per Christian
>> > - Fix indentation (use tabs) in documentaion, per Randy
>> >
>> > Documentation/filesystems/proc.rst | 14 ++++++++++++--
>> > fs/proc/fd.c | 4 ++++
>> > 2 files changed, 16 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
>> > index 779c05528e87..591f12d30d97 100644
>> > --- a/Documentation/filesystems/proc.rst
>> > +++ b/Documentation/filesystems/proc.rst
>> > @@ -1886,14 +1886,16 @@ if precise results are needed.
>> > 3.8 /proc/<pid>/fdinfo/<fd> - Information about opened file
>> > ---------------------------------------------------------------
>> > This file provides information associated with an opened file. The regular
>> > -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
>> > +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
>> > +and 'path'.
>> >
>> > The 'pos' represents the current offset of the opened file in decimal
>> > form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
>> > file has been created with [see open(2) for details] and 'mnt_id' represents
>> > mount ID of the file system containing the opened file [see 3.5
>> > /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
>> > -the file, and 'size' represents the size of the file in bytes.
>> > +the file, 'size' represents the size of the file in bytes, and 'path'
>> > +represents the file path.
>> >
>> > A typical output is::
>> >
>> > @@ -1902,6 +1904,7 @@ A typical output is::
>> > mnt_id: 19
>> > ino: 63107
>> > size: 0
>> > + path: /dev/null
>> >
>> > All locks associated with a file descriptor are shown in its fdinfo too::
>> >
>> > @@ -1920,6 +1923,7 @@ Eventfd files
>> > mnt_id: 9
>> > ino: 63107
>> > size: 0
>> > + path: anon_inode:[eventfd]
>> > eventfd-count: 5a
>> >
>> > where 'eventfd-count' is hex value of a counter.
>> > @@ -1934,6 +1938,7 @@ Signalfd files
>> > mnt_id: 9
>> > ino: 63107
>> > size: 0
>> > + path: anon_inode:[signalfd]
>> > sigmask: 0000000000000200
>> >
>> > where 'sigmask' is hex value of the signal mask associated
>> > @@ -1949,6 +1954,7 @@ Epoll files
>> > mnt_id: 9
>> > ino: 63107
>> > size: 0
>> > + path: anon_inode:[eventpoll]
>> > tfd: 5 events: 1d data: ffffffffffffffff pos:0 ino:61af sdev:7
>> >
>> > where 'tfd' is a target file descriptor number in decimal form,
>> > @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
>> > mnt_id: 9
>> > ino: 63107
>> > size: 0
>> > + path: anon_inode:inotify
>> > inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
>> >
>> > where 'wd' is a watch descriptor in decimal form, i.e. a target file
>> > @@ -1992,6 +1999,7 @@ For fanotify files the format is::
>> > mnt_id: 9
>> > ino: 63107
>> > size: 0
>> > + path: anon_inode:[fanotify]
>> > fanotify flags:10 event-flags:0
>> > fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
>> > fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
>> > @@ -2018,6 +2026,7 @@ Timerfd files
>> > mnt_id: 9
>> > ino: 63107
>> > size: 0
>> > + path: anon_inode:[timerfd]
>> > clockid: 0
>> > ticks: 0
>> > settime flags: 01
>> > @@ -2042,6 +2051,7 @@ DMA Buffer files
>> > mnt_id: 9
>> > ino: 63107
>> > size: 32768
>> > + path: /dmabuf:
>> > count: 2
>> > exp_name: system-heap
>> >
>> > diff --git a/fs/proc/fd.c b/fs/proc/fd.c
>> > index 464bc3f55759..8889a8ba09d4 100644
>> > --- a/fs/proc/fd.c
>> > +++ b/fs/proc/fd.c
>> > @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
>> > seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
>> > seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
>> >
>> > + seq_puts(m, "path:\t");
>> > + seq_file_path(m, file, "\n");
>> > + seq_putc(m, '\n');
>> > +
>> > /* show_fd_locks() never deferences files so a stale value is safe */
>> > show_fd_locks(m, file, files);
>> > if (seq_has_overflowed(m))
>>
>> --
>> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>>
^ permalink raw reply
* Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/
From: Kalesh Singh @ 2022-05-31 22:30 UTC (permalink / raw)
To: Stephen Brennan
Cc: Ioannis Ilkos, T.J. Mercier, Suren Baghdasaryan,
Cc: Android Kernel, Jonathan Corbet, Sumit Semwal,
Christian König, Andrew Morton, David Hildenbrand,
Christoph Anton Mitterer, Johannes Weiner, Colin Cross,
Mike Rapoport, Paul Gortmaker, Randy Dunlap, LKML, linux-fsdevel,
open list:DOCUMENTATION, Linux Media Mailing List,
DRI mailing list, moderated list:DMA BUFFER SHARING FRAMEWORK
In-Reply-To: <14f85d24-a9de-9706-32f0-30be4999c71c@oracle.com>
On Tue, May 31, 2022 at 3:07 PM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
>
> On 5/31/22 14:25, Kalesh Singh wrote:
> > In order to identify the type of memory a process has pinned through
> > its open fds, add the file path to fdinfo output. This allows
> > identifying memory types based on common prefixes. e.g. "/memfd...",
> > "/dmabuf...", "/dev/ashmem...".
> >
> > Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
> > the same as /proc/<pid>/maps which also exposes the file path of
> > mappings; so the security permissions for accessing path is consistent
> > with that of /proc/<pid>/maps.
>
> Hi Kalesh,
Hi Stephen,
Thanks for taking a look.
>
> I think I see the value in the size field, but I'm curious about path,
> which is available via readlink /proc/<pid>/fd/<n>, since those are
> symlinks to the file themselves.
This could work if we are root, but the file permissions wouldn't
allow us to do the readlink on other processes otherwise. We want to
be able to capture the system state in production environments from
some trusted process with ptrace read capability.
>
> File paths can contain fun characters like newlines or colons, which
> could make parsing out filenames in this text file... fun. How would your
> userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
> readlink(2) API makes that easy already.
I think since we have escaped the "\n" (seq_file_path(m, file, "\n")),
then user space might parse this line like:
if (strncmp(line, "path:\t", 6) == 0)
char* path = line + 6;
Thanks,
Kalesh
>
> Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
> to a different path between reading fdinfo and stating the fd)?
>
> Stephen
>
> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> > ---
> >
> > Changes from rfc:
> > - Split adding 'size' and 'path' into a separate patches, per Christian
> > - Fix indentation (use tabs) in documentaion, per Randy
> >
> > Documentation/filesystems/proc.rst | 14 ++++++++++++--
> > fs/proc/fd.c | 4 ++++
> > 2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> > index 779c05528e87..591f12d30d97 100644
> > --- a/Documentation/filesystems/proc.rst
> > +++ b/Documentation/filesystems/proc.rst
> > @@ -1886,14 +1886,16 @@ if precise results are needed.
> > 3.8 /proc/<pid>/fdinfo/<fd> - Information about opened file
> > ---------------------------------------------------------------
> > This file provides information associated with an opened file. The regular
> > -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
> > +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
> > +and 'path'.
> >
> > The 'pos' represents the current offset of the opened file in decimal
> > form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
> > file has been created with [see open(2) for details] and 'mnt_id' represents
> > mount ID of the file system containing the opened file [see 3.5
> > /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
> > -the file, and 'size' represents the size of the file in bytes.
> > +the file, 'size' represents the size of the file in bytes, and 'path'
> > +represents the file path.
> >
> > A typical output is::
> >
> > @@ -1902,6 +1904,7 @@ A typical output is::
> > mnt_id: 19
> > ino: 63107
> > size: 0
> > + path: /dev/null
> >
> > All locks associated with a file descriptor are shown in its fdinfo too::
> >
> > @@ -1920,6 +1923,7 @@ Eventfd files
> > mnt_id: 9
> > ino: 63107
> > size: 0
> > + path: anon_inode:[eventfd]
> > eventfd-count: 5a
> >
> > where 'eventfd-count' is hex value of a counter.
> > @@ -1934,6 +1938,7 @@ Signalfd files
> > mnt_id: 9
> > ino: 63107
> > size: 0
> > + path: anon_inode:[signalfd]
> > sigmask: 0000000000000200
> >
> > where 'sigmask' is hex value of the signal mask associated
> > @@ -1949,6 +1954,7 @@ Epoll files
> > mnt_id: 9
> > ino: 63107
> > size: 0
> > + path: anon_inode:[eventpoll]
> > tfd: 5 events: 1d data: ffffffffffffffff pos:0 ino:61af sdev:7
> >
> > where 'tfd' is a target file descriptor number in decimal form,
> > @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
> > mnt_id: 9
> > ino: 63107
> > size: 0
> > + path: anon_inode:inotify
> > inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
> >
> > where 'wd' is a watch descriptor in decimal form, i.e. a target file
> > @@ -1992,6 +1999,7 @@ For fanotify files the format is::
> > mnt_id: 9
> > ino: 63107
> > size: 0
> > + path: anon_inode:[fanotify]
> > fanotify flags:10 event-flags:0
> > fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
> > fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
> > @@ -2018,6 +2026,7 @@ Timerfd files
> > mnt_id: 9
> > ino: 63107
> > size: 0
> > + path: anon_inode:[timerfd]
> > clockid: 0
> > ticks: 0
> > settime flags: 01
> > @@ -2042,6 +2051,7 @@ DMA Buffer files
> > mnt_id: 9
> > ino: 63107
> > size: 32768
> > + path: /dmabuf:
> > count: 2
> > exp_name: system-heap
> >
> > diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> > index 464bc3f55759..8889a8ba09d4 100644
> > --- a/fs/proc/fd.c
> > +++ b/fs/proc/fd.c
> > @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
> > seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
> > seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
> >
> > + seq_puts(m, "path:\t");
> > + seq_file_path(m, file, "\n");
> > + seq_putc(m, '\n');
> > +
> > /* show_fd_locks() never deferences files so a stale value is safe */
> > show_fd_locks(m, file, files);
> > if (seq_has_overflowed(m))
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
^ permalink raw reply
* Re: [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/
From: Stephen Brennan @ 2022-05-31 22:07 UTC (permalink / raw)
To: Kalesh Singh
Cc: ilkos, tjmercier, surenb, kernel-team, Jonathan Corbet,
Sumit Semwal, Christian König, Andrew Morton,
David Hildenbrand, Christoph Anton Mitterer, Johannes Weiner,
Colin Cross, Mike Rapoport, Paul Gortmaker, Randy Dunlap,
linux-kernel, linux-fsdevel, linux-doc, linux-media, dri-devel,
linaro-mm-sig
In-Reply-To: <20220531212521.1231133-3-kaleshsingh@google.com>
On 5/31/22 14:25, Kalesh Singh wrote:
> In order to identify the type of memory a process has pinned through
> its open fds, add the file path to fdinfo output. This allows
> identifying memory types based on common prefixes. e.g. "/memfd...",
> "/dmabuf...", "/dev/ashmem...".
>
> Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
> the same as /proc/<pid>/maps which also exposes the file path of
> mappings; so the security permissions for accessing path is consistent
> with that of /proc/<pid>/maps.
Hi Kalesh,
I think I see the value in the size field, but I'm curious about path,
which is available via readlink /proc/<pid>/fd/<n>, since those are
symlinks to the file themselves.
File paths can contain fun characters like newlines or colons, which
could make parsing out filenames in this text file... fun. How would your
userspace parsing logic handle "/home/stephen/filename\nsize:\t4096"? The
readlink(2) API makes that easy already.
Is the goal avoiding races (e.g. file descriptor 3 is closed and reopened
to a different path between reading fdinfo and stating the fd)?
Stephen
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> ---
>
> Changes from rfc:
> - Split adding 'size' and 'path' into a separate patches, per Christian
> - Fix indentation (use tabs) in documentaion, per Randy
>
> Documentation/filesystems/proc.rst | 14 ++++++++++++--
> fs/proc/fd.c | 4 ++++
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> index 779c05528e87..591f12d30d97 100644
> --- a/Documentation/filesystems/proc.rst
> +++ b/Documentation/filesystems/proc.rst
> @@ -1886,14 +1886,16 @@ if precise results are needed.
> 3.8 /proc/<pid>/fdinfo/<fd> - Information about opened file
> ---------------------------------------------------------------
> This file provides information associated with an opened file. The regular
> -files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
> +files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
> +and 'path'.
>
> The 'pos' represents the current offset of the opened file in decimal
> form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
> file has been created with [see open(2) for details] and 'mnt_id' represents
> mount ID of the file system containing the opened file [see 3.5
> /proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
> -the file, and 'size' represents the size of the file in bytes.
> +the file, 'size' represents the size of the file in bytes, and 'path'
> +represents the file path.
>
> A typical output is::
>
> @@ -1902,6 +1904,7 @@ A typical output is::
> mnt_id: 19
> ino: 63107
> size: 0
> + path: /dev/null
>
> All locks associated with a file descriptor are shown in its fdinfo too::
>
> @@ -1920,6 +1923,7 @@ Eventfd files
> mnt_id: 9
> ino: 63107
> size: 0
> + path: anon_inode:[eventfd]
> eventfd-count: 5a
>
> where 'eventfd-count' is hex value of a counter.
> @@ -1934,6 +1938,7 @@ Signalfd files
> mnt_id: 9
> ino: 63107
> size: 0
> + path: anon_inode:[signalfd]
> sigmask: 0000000000000200
>
> where 'sigmask' is hex value of the signal mask associated
> @@ -1949,6 +1954,7 @@ Epoll files
> mnt_id: 9
> ino: 63107
> size: 0
> + path: anon_inode:[eventpoll]
> tfd: 5 events: 1d data: ffffffffffffffff pos:0 ino:61af sdev:7
>
> where 'tfd' is a target file descriptor number in decimal form,
> @@ -1968,6 +1974,7 @@ For inotify files the format is the following::
> mnt_id: 9
> ino: 63107
> size: 0
> + path: anon_inode:inotify
> inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
>
> where 'wd' is a watch descriptor in decimal form, i.e. a target file
> @@ -1992,6 +1999,7 @@ For fanotify files the format is::
> mnt_id: 9
> ino: 63107
> size: 0
> + path: anon_inode:[fanotify]
> fanotify flags:10 event-flags:0
> fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
> fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
> @@ -2018,6 +2026,7 @@ Timerfd files
> mnt_id: 9
> ino: 63107
> size: 0
> + path: anon_inode:[timerfd]
> clockid: 0
> ticks: 0
> settime flags: 01
> @@ -2042,6 +2051,7 @@ DMA Buffer files
> mnt_id: 9
> ino: 63107
> size: 32768
> + path: /dmabuf:
> count: 2
> exp_name: system-heap
>
> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> index 464bc3f55759..8889a8ba09d4 100644
> --- a/fs/proc/fd.c
> +++ b/fs/proc/fd.c
> @@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
> seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
> seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
>
> + seq_puts(m, "path:\t");
> + seq_file_path(m, file, "\n");
> + seq_putc(m, '\n');
> +
> /* show_fd_locks() never deferences files so a stale value is safe */
> show_fd_locks(m, file, files);
> if (seq_has_overflowed(m))
^ permalink raw reply
* Re: Per file OOM badness
From: Alex Deucher @ 2022-05-31 22:00 UTC (permalink / raw)
To: Christian König, Maling list - DRI developers
Cc: linux-media, LKML, Intel Graphics Development, amd-gfx list,
nouveau, linux-tegra, Linux-Fsdevel, linux-mm, Andrey Grodzovsky,
Hugh Dickens, Alexander Viro, Daniel Vetter, Deucher, Alexander,
Andrew Morton, Christian Koenig
In-Reply-To: <20220531100007.174649-1-christian.koenig@amd.com>
+ dri-devel
On Tue, May 31, 2022 at 6:00 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Hello everyone,
>
> To summarize the issue I'm trying to address here: Processes can allocate
> resources through a file descriptor without being held responsible for it.
>
> Especially for the DRM graphics driver subsystem this is rather
> problematic. Modern games tend to allocate huge amounts of system memory
> through the DRM drivers to make it accessible to GPU rendering.
>
> But even outside of the DRM subsystem this problem exists and it is
> trivial to exploit. See the following simple example of
> using memfd_create():
>
> fd = memfd_create("test", 0);
> while (1)
> write(fd, page, 4096);
>
> Compile this and you can bring down any standard desktop system within
> seconds.
>
> The background is that the OOM killer will kill every processes in the
> system, but just not the one which holds the only reference to the memory
> allocated by the memfd.
>
> Those problems where brought up on the mailing list multiple times now
> [1][2][3], but without any final conclusion how to address them. Since
> file descriptors are considered shared the process can not directly held
> accountable for allocations made through them. Additional to that file
> descriptors can also easily move between processes as well.
>
> So what this patch set does is to instead of trying to account the
> allocated memory to a specific process it adds a callback to struct
> file_operations which the OOM killer can use to query the specific OOM
> badness of this file reference. This badness is then divided by the
> file_count, so that every process using a shmem file, DMA-buf or DRM
> driver will get it's equal amount of OOM badness.
>
> Callbacks are then implemented for the two core users (memfd and DMA-buf)
> as well as 72 DRM based graphics drivers.
>
> The result is that the OOM killer can now much better judge if a process
> is worth killing to free up memory. Resulting a quite a bit better system
> stability in OOM situations, especially while running games.
>
> The only other possibility I can see would be to change the accounting of
> resources whenever references to the file structure change, but this would
> mean quite some additional overhead for a rather common operation.
>
> Additionally I think trying to limit device driver allocations using
> cgroups is orthogonal to this effort. While cgroups is very useful, it
> works on per process limits and tries to enforce a collaborative model on
> memory management while the OOM killer enforces a competitive model.
>
> Please comment and/or review, we have that problem flying around for years
> now and are not at a point where we finally need to find a solution for
> this.
>
> Regards,
> Christian.
>
> [1] https://lists.freedesktop.org/archives/dri-devel/2015-September/089778.html
> [2] https://lkml.org/lkml/2018/1/18/543
> [3] https://lkml.org/lkml/2021/2/4/799
>
>
^ permalink raw reply
* [PATCH 2/2] procfs: Add 'path' to /proc/<pid>/fdinfo/
From: Kalesh Singh @ 2022-05-31 21:25 UTC (permalink / raw)
Cc: ilkos, tjmercier, surenb, kernel-team, Kalesh Singh,
Jonathan Corbet, Sumit Semwal, Christian König,
Andrew Morton, David Hildenbrand, Christoph Anton Mitterer,
Johannes Weiner, Colin Cross, Mike Rapoport, Paul Gortmaker,
Randy Dunlap, linux-kernel, linux-fsdevel, linux-doc, linux-media,
dri-devel, linaro-mm-sig
In-Reply-To: <20220531212521.1231133-1-kaleshsingh@google.com>
In order to identify the type of memory a process has pinned through
its open fds, add the file path to fdinfo output. This allows
identifying memory types based on common prefixes. e.g. "/memfd...",
"/dmabuf...", "/dev/ashmem...".
Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
the same as /proc/<pid>/maps which also exposes the file path of
mappings; so the security permissions for accessing path is consistent
with that of /proc/<pid>/maps.
Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
---
Changes from rfc:
- Split adding 'size' and 'path' into a separate patches, per Christian
- Fix indentation (use tabs) in documentaion, per Randy
Documentation/filesystems/proc.rst | 14 ++++++++++++--
fs/proc/fd.c | 4 ++++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 779c05528e87..591f12d30d97 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -1886,14 +1886,16 @@ if precise results are needed.
3.8 /proc/<pid>/fdinfo/<fd> - Information about opened file
---------------------------------------------------------------
This file provides information associated with an opened file. The regular
-files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
+files have at least six fields -- 'pos', 'flags', 'mnt_id', 'ino', 'size',
+and 'path'.
The 'pos' represents the current offset of the opened file in decimal
form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
file has been created with [see open(2) for details] and 'mnt_id' represents
mount ID of the file system containing the opened file [see 3.5
/proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
-the file, and 'size' represents the size of the file in bytes.
+the file, 'size' represents the size of the file in bytes, and 'path'
+represents the file path.
A typical output is::
@@ -1902,6 +1904,7 @@ A typical output is::
mnt_id: 19
ino: 63107
size: 0
+ path: /dev/null
All locks associated with a file descriptor are shown in its fdinfo too::
@@ -1920,6 +1923,7 @@ Eventfd files
mnt_id: 9
ino: 63107
size: 0
+ path: anon_inode:[eventfd]
eventfd-count: 5a
where 'eventfd-count' is hex value of a counter.
@@ -1934,6 +1938,7 @@ Signalfd files
mnt_id: 9
ino: 63107
size: 0
+ path: anon_inode:[signalfd]
sigmask: 0000000000000200
where 'sigmask' is hex value of the signal mask associated
@@ -1949,6 +1954,7 @@ Epoll files
mnt_id: 9
ino: 63107
size: 0
+ path: anon_inode:[eventpoll]
tfd: 5 events: 1d data: ffffffffffffffff pos:0 ino:61af sdev:7
where 'tfd' is a target file descriptor number in decimal form,
@@ -1968,6 +1974,7 @@ For inotify files the format is the following::
mnt_id: 9
ino: 63107
size: 0
+ path: anon_inode:inotify
inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
where 'wd' is a watch descriptor in decimal form, i.e. a target file
@@ -1992,6 +1999,7 @@ For fanotify files the format is::
mnt_id: 9
ino: 63107
size: 0
+ path: anon_inode:[fanotify]
fanotify flags:10 event-flags:0
fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
@@ -2018,6 +2026,7 @@ Timerfd files
mnt_id: 9
ino: 63107
size: 0
+ path: anon_inode:[timerfd]
clockid: 0
ticks: 0
settime flags: 01
@@ -2042,6 +2051,7 @@ DMA Buffer files
mnt_id: 9
ino: 63107
size: 32768
+ path: /dmabuf:
count: 2
exp_name: system-heap
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 464bc3f55759..8889a8ba09d4 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -60,6 +60,10 @@ static int seq_show(struct seq_file *m, void *v)
seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
+ seq_puts(m, "path:\t");
+ seq_file_path(m, file, "\n");
+ seq_putc(m, '\n');
+
/* show_fd_locks() never deferences files so a stale value is safe */
show_fd_locks(m, file, files);
if (seq_has_overflowed(m))
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related
* [PATCH 1/2] procfs: Add 'size' to /proc/<pid>/fdinfo/
From: Kalesh Singh @ 2022-05-31 21:25 UTC (permalink / raw)
Cc: ilkos, tjmercier, surenb, kernel-team, Kalesh Singh,
Jonathan Corbet, Sumit Semwal, Christian König,
Andrew Morton, David Hildenbrand, Johannes Weiner,
Christoph Anton Mitterer, Paul Gortmaker, Mike Rapoport,
Randy Dunlap, linux-kernel, linux-fsdevel, linux-doc, linux-media,
dri-devel, linaro-mm-sig
In-Reply-To: <20220531212521.1231133-1-kaleshsingh@google.com>
To be able to account the amount of memory a process is keeping pinned
by open file descriptors add a 'size' field to fdinfo output.
dmabufs fds already expose a 'size' field for this reason, remove this
and make it a common field for all fds. This allows tracking of
other types of memory (e.g. memfd and ashmem in Android).
Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
---
Changes from rfc:
- Split adding 'size' and 'path' into a separate patches, per Christian
- Split fdinfo seq_printf into separate lines, per Christian
- Fix indentation (use tabs) in documentaion, per Randy
Documentation/filesystems/proc.rst | 12 ++++++++++--
drivers/dma-buf/dma-buf.c | 1 -
fs/proc/fd.c | 9 +++++----
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 1bc91fb8c321..779c05528e87 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -1886,13 +1886,14 @@ if precise results are needed.
3.8 /proc/<pid>/fdinfo/<fd> - Information about opened file
---------------------------------------------------------------
This file provides information associated with an opened file. The regular
-files have at least four fields -- 'pos', 'flags', 'mnt_id' and 'ino'.
+files have at least five fields -- 'pos', 'flags', 'mnt_id', 'ino', and 'size'.
+
The 'pos' represents the current offset of the opened file in decimal
form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
file has been created with [see open(2) for details] and 'mnt_id' represents
mount ID of the file system containing the opened file [see 3.5
/proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
-the file.
+the file, and 'size' represents the size of the file in bytes.
A typical output is::
@@ -1900,6 +1901,7 @@ A typical output is::
flags: 0100002
mnt_id: 19
ino: 63107
+ size: 0
All locks associated with a file descriptor are shown in its fdinfo too::
@@ -1917,6 +1919,7 @@ Eventfd files
flags: 04002
mnt_id: 9
ino: 63107
+ size: 0
eventfd-count: 5a
where 'eventfd-count' is hex value of a counter.
@@ -1930,6 +1933,7 @@ Signalfd files
flags: 04002
mnt_id: 9
ino: 63107
+ size: 0
sigmask: 0000000000000200
where 'sigmask' is hex value of the signal mask associated
@@ -1944,6 +1948,7 @@ Epoll files
flags: 02
mnt_id: 9
ino: 63107
+ size: 0
tfd: 5 events: 1d data: ffffffffffffffff pos:0 ino:61af sdev:7
where 'tfd' is a target file descriptor number in decimal form,
@@ -1962,6 +1967,7 @@ For inotify files the format is the following::
flags: 02000000
mnt_id: 9
ino: 63107
+ size: 0
inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
where 'wd' is a watch descriptor in decimal form, i.e. a target file
@@ -1985,6 +1991,7 @@ For fanotify files the format is::
flags: 02
mnt_id: 9
ino: 63107
+ size: 0
fanotify flags:10 event-flags:0
fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
@@ -2010,6 +2017,7 @@ Timerfd files
flags: 02
mnt_id: 9
ino: 63107
+ size: 0
clockid: 0
ticks: 0
settime flags: 01
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 32f55640890c..5f2ae38c960f 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -378,7 +378,6 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file)
{
struct dma_buf *dmabuf = file->private_data;
- seq_printf(m, "size:\t%zu\n", dmabuf->size);
/* Don't count the temporary reference taken inside procfs seq_show */
seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1);
seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name);
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 913bef0d2a36..464bc3f55759 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -54,10 +54,11 @@ static int seq_show(struct seq_file *m, void *v)
if (ret)
return ret;
- seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\nino:\t%lu\n",
- (long long)file->f_pos, f_flags,
- real_mount(file->f_path.mnt)->mnt_id,
- file_inode(file)->i_ino);
+ seq_printf(m, "pos:\t%lli\n", (long long)file->f_pos);
+ seq_printf(m, "flags:\t0%o\n", f_flags);
+ seq_printf(m, "mnt_id:\t%i\n", real_mount(file->f_path.mnt)->mnt_id);
+ seq_printf(m, "ino:\t%lu\n", file_inode(file)->i_ino);
+ seq_printf(m, "size:\t%lli\n", (long long)file_inode(file)->i_size);
/* show_fd_locks() never deferences files so a stale value is safe */
show_fd_locks(m, file, files);
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related
* [PATCH 0/2] procfs: Add file path and size to /proc/<pid>/fdinfo
From: Kalesh Singh @ 2022-05-31 21:25 UTC (permalink / raw)
Cc: ilkos, tjmercier, surenb, kernel-team, Kalesh Singh,
Jonathan Corbet, Sumit Semwal, Christian König,
Andrew Morton, Johannes Weiner, David Hildenbrand,
Christoph Anton Mitterer, Mike Rapoport, Paul Gortmaker,
Randy Dunlap, linux-kernel, linux-fsdevel, linux-doc, linux-media,
dri-devel, linaro-mm-sig
Processes can pin shared memory by keeping a handle to it through a
file descriptor; for instance dmabufs, memfd, and ashmem (in Android).
In the case of a memory leak, to identify the process pinning the
memory, userspace needs to:
- Iterate the /proc/<pid>/fd/* for each process
- Do a readlink on each entry to identify the type of memory from
the file path.
- stat() each entry to get the size of the memory.
The file permissions on /proc/<pid>/fd/* only allows for the owner
or root to perform the operations above; and so is not suitable for
capturing the system-wide state in a production environment.
This issue was addressed for dmabufs by making /proc/*/fdinfo/*
accessible to a process with PTRACE_MODE_READ_FSCREDS credentials[1]
To allow the same kind of tracking for other types of shared memory,
add the following fields to /proc/<pid>/fdinfo/<fd>:
path - This allows identifying the type of memory based on common
prefixes: e.g. "/memfd...", "/dmabuf...", "/dev/ashmem..."
This was not an issued when dmabuf tracking was introduced
because the exp_name field of dmabuf fdinfo could be used
to distinguish dmabuf fds from other types.
size - To track the amount of memory that is being pinned.
dmabufs expose size as an additional field in fdinfo. Remove
this and make it a common field for all fds.
Access to /proc/<pid>/fdinfo is governed by PTRACE_MODE_READ_FSCREDS
-- the same as for /proc/<pid>/maps which also exposes the path and
size for mapped memory regions.
This allows for a system process with PTRACE_MODE_READ_FSCREDS to
account the pinned per-process memory via fdinfo.
-----
There was some concern about exposing the file path in the RFC[2], to that
effect the change was split into separte patches. Also retrieving the file
path from fdinfo is guarded by the same capability (PTRACE_MODE_READ) as
/proc/<pid>/maps which also exposes file path, so this may not be an issue.
[1] https://lore.kernel.org/r/20210308170651.919148-1-kaleshsingh@google.com/
[2] https://lore.kernel.org/r/20220519214021.3572840-1-kaleshsingh@google.com/
Kalesh Singh (2):
procfs: Add 'size' to /proc/<pid>/fdinfo/
procfs: Add 'path' to /proc/<pid>/fdinfo/
Documentation/filesystems/proc.rst | 22 ++++++++++++++++++++--
drivers/dma-buf/dma-buf.c | 1 -
fs/proc/fd.c | 13 +++++++++----
3 files changed, 29 insertions(+), 7 deletions(-)
base-commit: 8ab2afa23bd197df47819a87f0265c0ac95c5b6a
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply
* [PATCH] mediatek/vcodec: Enable incoherent buffer allocation
From: Justin Green @ 2022-05-31 21:10 UTC (permalink / raw)
To: linux-media
Cc: tiffany.lin@mediatek.com, andrew-ct.chen@mediatek.com, mchehab,
matthias.bgg@gmail.com, nicolas.dufresne@collabora.com,
Sergey Senozhatsky
Set allow_cache_hints to 1 for the vb2_queue source and destination queues
in the mediatek vcodec V4L2 driver. This allows us to allocate buffers
with the V4L2_MEMORY_FLAG_NON_COHERENT set. On Mediatek SoCs, this enables
caching for this memory, which vastly improves performance when being read
from CPU. Read performance for these buffers is in turn important for
detiling MM21 video frames in software.
This change should be safe from race conditions since videobuf2 already
invalidates or flushes the appropriate cache lines in its prepare() and
finish() methods.
Tested on a MT8183 SoC. Resulted in both correct detiling and a 10X
speedup.
Signed-off-by: Justin Green <greenjustin@chromium.org>
---
.../platform/mediatek/vcodec/mtk_vcodec_dec.c | 38 ++++++++++---------
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
index 52e5d36aa912..f093aa715e1f 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
@@ -929,30 +929,32 @@ int mtk_vcodec_dec_queue_init(void *priv, struct
vb2_queue *src_vq,
mtk_v4l2_debug(3, "[%d]", ctx->id);
- src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
- src_vq->io_modes = VB2_DMABUF | VB2_MMAP;
- src_vq->drv_priv = ctx;
- src_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
- src_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
- src_vq->mem_ops = &vb2_dma_contig_memops;
- src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
- src_vq->lock = &ctx->dev->dev_mutex;
- src_vq->dev = &ctx->dev->plat_dev->dev;
+ src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+ src_vq->io_modes = VB2_DMABUF | VB2_MMAP;
+ src_vq->drv_priv = ctx;
+ src_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
+ src_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
+ src_vq->mem_ops = &vb2_dma_contig_memops;
+ src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
+ src_vq->lock = &ctx->dev->dev_mutex;
+ src_vq->dev = &ctx->dev->plat_dev->dev;
+ src_vq->allow_cache_hints = 1;
ret = vb2_queue_init(src_vq);
if (ret) {
mtk_v4l2_err("Failed to initialize videobuf2 queue(output)");
return ret;
}
- dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
- dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
- dst_vq->drv_priv = ctx;
- dst_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
- dst_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
- dst_vq->mem_ops = &vb2_dma_contig_memops;
- dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
- dst_vq->lock = &ctx->dev->dev_mutex;
- dst_vq->dev = &ctx->dev->plat_dev->dev;
+ dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+ dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
+ dst_vq->drv_priv = ctx;
+ dst_vq->buf_struct_size = sizeof(struct mtk_video_dec_buf);
+ dst_vq->ops = ctx->dev->vdec_pdata->vdec_vb2_ops;
+ dst_vq->mem_ops = &vb2_dma_contig_memops;
+ dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
+ dst_vq->lock = &ctx->dev->dev_mutex;
+ dst_vq->dev = &ctx->dev->plat_dev->dev;
+ dst_vq->allow_cache_hints = 1;
ret = vb2_queue_init(dst_vq);
if (ret)
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related
* Re: Re: [PATCH v6 11/17] media: uapi: Add V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS control
From: Jernej Škrabec @ 2022-05-31 18:20 UTC (permalink / raw)
To: mchehab, ezequiel, p.zabel, gregkh, mripard, paul.kocialkowski,
wens, samuel, nicolas.dufresne, andrzej.p, Hans Verkuil,
Benjamin Gaignard
Cc: linux-media, linux-kernel, linux-rockchip, linux-staging,
linux-arm-kernel, linux-sunxi, kernel
In-Reply-To: <b398272b-daf8-7499-b4fd-8a6f2af30053@collabora.com>
Dne torek, 31. maj 2022 ob 08:58:46 CEST je Benjamin Gaignard napisal(a):
>
> Le 30/05/2022 à 23:24, Jernej Škrabec a écrit :
> > Dne ponedeljek, 30. maj 2022 ob 15:49:57 CEST je Hans Verkuil napisal(a):
> >> On 30/05/2022 11:18, Hans Verkuil wrote:
> >>> On 29/05/2022 08:40, Jernej Škrabec wrote:
> >>>> Hi!
> >>>>
> >>>> This series looks very good and I plan to test it shortly on Cedrus,
but
> > I
> >>>> have one major concern below.
> >>>>
> >>>> Dne petek, 27. maj 2022 ob 16:31:28 CEST je Benjamin Gaignard
napisal(a):
> >>>>> The number of 'entry point offset' can be very variable.
> >>>>> Instead of using a large static array define a v4l2 dynamic array
> >>>>> of U32 (V4L2_CTRL_TYPE_U32).
> >>>>> The number of entry point offsets is reported by the elems field
> >>>>> and in struct v4l2_ctrl_hevc_slice_params.num_entry_point_offsets
> >>>>> field.
> >>>> Slice control by itself is variable length array, so you would actually
> > need
> >>>> 2D variable array for entry points which is not supported. However,
easy
> >>>> workaround for that is to flatten 2D array to 1D and either have another
> > slice
> >>>> control field which would tell first entry point index for convenience or
> > let
> >>>> driver calculate it by adding up all num_entry_point_offsets of previous
> >>>> slices.
> >>>>
> >>>> Hans, what do you think?
> >>> If I would support 2D variable array sizes, wouldn't that be more
elegant?
> >>>
> >>> The current implementation doesn't support that, but as the commit log
for
> >>> patch 1/17 says:
> >>>
> >>> "Currently dynamically sized arrays are limited to one dimensional
arrays,
> >>> but that might change in the future if there is a need for it."
> >>>
> >>> Let me know if you agree, and I'll try to implement this. It's been a
> > while
> >>> since I last looked at this, so I'm not sure how much work it is, but it
> > is
> >>> probably worth a shot.
> >> Digging more into this made me realize that this doesn't actually help
for
> > this
> >> particular case.
> >>
> >> I would lean towards your second suggestion of adding up all
> > num_entry_point_offsets
> >> of previous slices.
> > Just one question/clarification about dynamic arrays - does driver need to
> > reserve maximum amount of memory for dynamic array control at
initialization
> > time? If so, this would still be problematic, since there cound be a huge
> > amount of entry points in theory.
>
> When adding the control the driver could set .dims field to specify
> the max number of accepted slices.
> I have added '#define V4L2_HEVC_SLICE_MAX_COUNT 600' that you could use
> for this field. It is the value we have found when using slices with RKVDEC
> driver.
Is this maximum value applicable only to RKVDEC or is universal? Anyway, this
means maximum offset points control for Cedrus would be 600 * 1024 (max. offset
points supported per slice) * 4 ~= 2.4 MB, which is a lot for one control, but
I can live with that...
Best regards,
Jernej
>
> Regards,
> Benjamin
>
> >
> > Best regards,
> > Jernej
> >
> >> Regards,
> >>
> >> Hans
> >>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> >>>> Note, it seems that H265 decoding on Cedrus still works without entry
> > points,
> >>>> so this problem can be solved later. I'm not sure what we lose with
that
> > but
> >>>> it was suggested that this could influence speed or error resilience or
> > both.
> >>>> However, I think we're close to solve it, so I'd like to do that now.
> >>>>
> >>>> Best regards,
> >>>> Jernej
> >>>>
> >>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> >>>>> ---
> >>>>> .../userspace-api/media/v4l/ext-ctrls-codec.rst | 11 +++++++++
++
> >>>>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 5 +++++
> >>>>> include/media/hevc-ctrls.h | 5 ++++-
> >>>>> 3 files changed, 20 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
b/
> >>>> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> >>>>> index 0796b1563daa..05228e280f66 100644
> >>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> >>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> >>>>> @@ -3010,6 +3010,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> >>>>> * - __u32
> >>>>> - ``data_bit_offset``
> >>>>> - Offset (in bits) to the video data in the current slice data.
> >>>>> + * - __u32
> >>>>> + - ``num_entry_point_offsets``
> >>>>> + - Specifies the number of entry point offset syntax elements in
the
> >>>> slice header.
> >>>>> * - __u8
> >>>>> - ``nal_unit_type``
> >>>>> - Specifies the coding type of the slice (B, P or I).
> >>>>> @@ -3150,6 +3153,14 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> >>>>>
> >>>>> \normalsize
> >>>>>
> >>>>> +``V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS (integer)``
> >>>>> + Specifies entry point offsets in bytes.
> >>>>> + This control is a dynamically sized array. The number of entry
> > point
> >>>>> + offsets is reported by the ``elems`` field.
> >>>>> + This bitstream parameter is defined according to :ref:`hevc`.
> >>>>> + They are described in section 7.4.7.1 "General slice segment
header
> >>>>> + semantics" of the specification.
> >>>>> +
> >>>>> ``V4L2_CID_STATELESS_HEVC_SCALING_MATRIX (struct)``
> >>>>> Specifies the HEVC scaling matrix parameters used for the scaling
> >>>> process
> >>>>> for transform coefficients.
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/
> > v4l2-
> >>>> core/v4l2-ctrls-defs.c
> >>>>> index d594efbcbb93..e22921e7ea61 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>> @@ -1188,6 +1188,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>>>> case V4L2_CID_STATELESS_HEVC_DECODE_PARAMS: return
> >>>> "HEVC Decode Parameters";
> >>>>> case V4L2_CID_STATELESS_HEVC_DECODE_MODE: return
> >>>> "HEVC Decode Mode";
> >>>>> case V4L2_CID_STATELESS_HEVC_START_CODE: return
> >>>> "HEVC Start Code";
> >>>>> + case V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS: return
> >>>> "HEVC Entry Point Offsets";
> >>>>>
> >>>>> /* Colorimetry controls */
> >>>>> /* Keep the order of the 'case's the same as in v4l2-controls.h!
> >>>> */
> >>>>> @@ -1518,6 +1519,10 @@ void v4l2_ctrl_fill(u32 id, const char **name,
> > enum
> >>>> v4l2_ctrl_type *type,
> >>>>> case V4L2_CID_STATELESS_HEVC_DECODE_PARAMS:
> >>>>> *type = V4L2_CTRL_TYPE_HEVC_DECODE_PARAMS;
> >>>>> break;
> >>>>> + case V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS:
> >>>>> + *type = V4L2_CTRL_TYPE_U32;
> >>>>> + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY;
> >>>>> + break;
> >>>>> case V4L2_CID_STATELESS_VP9_COMPRESSED_HDR:
> >>>>> *type = V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR;
> >>>>> break;
> >>>>> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> >>>>> index a3c829ef531a..1319cb99ae3f 100644
> >>>>> --- a/include/media/hevc-ctrls.h
> >>>>> +++ b/include/media/hevc-ctrls.h
> >>>>> @@ -20,6 +20,7 @@
> >>>>> #define V4L2_CID_STATELESS_HEVC_DECODE_PARAMS
(V4L2_CID_CODEC_BASE
> >>>> + 1012)
> >>>>> #define V4L2_CID_STATELESS_HEVC_DECODE_MODE
(V4L2_CID_CODEC_BASE
> > + 1015)
> >>>>> #define V4L2_CID_STATELESS_HEVC_START_CODE
(V4L2_CID_CODEC_BASE + 1016)
> >>>>> +#define V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS
(V4L2_CID_CODEC_BASE
> > +
> >>>> 1017)
> >>>>>
> >>>>> /* enum v4l2_ctrl_type type values */
> >>>>> #define V4L2_CTRL_TYPE_HEVC_SPS 0x0120
> >>>>> @@ -318,6 +319,8 @@ struct v4l2_hevc_pred_weight_table {
> >>>>> *
> >>>>> * @bit_size: size (in bits) of the current slice data
> >>>>> * @data_bit_offset: offset (in bits) to the video data in the current
> > slice
> >>>> data
> >>>>> + * @num_entry_point_offsets: specifies the number of entry point offset
> > syntax
> >>>>> + * elements in the slice header.
> >>>>> * @nal_unit_type: specifies the coding type of the slice (B, P or I)
> >>>>> * @nuh_temporal_id_plus1: minus 1 specifies a temporal identifier for
> > the
> >>>> NAL unit
> >>>>> * @slice_type: see V4L2_HEVC_SLICE_TYPE_{}
> >>>>> @@ -360,7 +363,7 @@ struct v4l2_hevc_pred_weight_table {
> >>>>> struct v4l2_ctrl_hevc_slice_params {
> >>>>> __u32 bit_size;
> >>>>> __u32 data_bit_offset;
> >>>>> -
> >>>>> + __u32 num_entry_point_offsets;
> >>>>> /* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header */
> >>>>> __u8 nal_unit_type;
> >>>>> __u8 nuh_temporal_id_plus1;
> >>>>> --
> >>>>> 2.32.0
> >>>>>
> >>>>>
> >>>>
> >>
> >
>
^ permalink raw reply
* Re: [RFC PATCH v2] media: Add AV1 uAPI
From: Daniel Almeida @ 2022-05-31 16:42 UTC (permalink / raw)
To: Steve Cho; +Cc: hverkuil, linux-media, Nicolas Dufresne
In-Reply-To: <CAC-pXoMVKmAZ=9iM7ivuo8rvnL=KQWHnopWharDz-eDky0QS-g@mail.gmail.com>
Sending this again, as apparently my last submission contained some HTML
that prevented it from being sent on the media ML
---
Hi Steve,
> I think the below definition is expected to cause a build error.
>
> +struct v4l2_av1_loop_restoration {
> + u8 flags;
>
> s/u8/__u8/ is needed.
>
> At least, this change was needed to fix this build error on Chromium
> build environment.
This will be fixed in RFC v3
> Question about tile info structure.
>
> struct v4l2_av1_tile_info {
> __u8 flags;
> __u32 mi_col_starts[V4L2_AV1_MAX_TILE_COLS + 1];
> __u32 mi_row_starts[V4L2_AV1_MAX_TILE_ROWS + 1];
> __u32 width_in_sbs_minus_1[V4L2_AV1_MAX_TILE_COLS];
> __u32 height_in_sbs_minus_1[V4L2_AV1_MAX_TILE_ROWS];
>
> I see below from the spec and gstreamer implementation
> for width_in_sbs_minus_1 and height_in_sbs_minus_1 computation.
>
> sb_cols = seq_header->use_128x128_superblock ?
> ((parser->state.mi_cols + 31) >> 5) : ((parser->state.mi_cols +
15) >> 4);
> sb_rows = seq_header->use_128x128_superblock ?
((parser->state.mi_rows +
> 31) >> 5) : ((parser->state.mi_rows + 15) >> 4);
>
> Are we confident that V4L2_AV1_MAX_TILE_COLS is good enough size for
> width_in_sbs_minus_1?
> Or does it potentially need to be V4L2_AV1_MAX_TILE_COLS+1?
>
> I am asking to double check because I see V4L2_AV1_MAX_TILE_COLS+1
> used for corresponding field in libgav1.
> int tile_column_width_in_superblocks[kMaxTileColumns + 1];
I have checked with a few other APIs to be on the safe side.
In VA-API they use a trick to save space on the last element, therefore
these two arrays will only be 63 members wide.
In NVDEC, these two arrays are 64 members wide, which is the same as our
V4L2 stateless implementation.
In DXVA, these two arrays are also 64 members wide
While going through the spec alongside with the libgav1 source code, I
notice that the index used to index into the two arrays eventually gets
assigned to tile_info->tile_rows and tile_info->tile_cols, i.e.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/libgav1/src/src/obu_parser.cc;l=1704;drc=242da5037807dde3daf097ba74f875db83b8b613
https://source.chromium.org/chromium/chromium/src/+/main:third_party/libgav1/src/src/obu_parser.cc;drc=242da5037807dde3daf097ba74f875db83b8b613;l=1729
But the spec says that these variables must be less than or equal to
MAX_TILE_ROWS (i.e. 64) and MAX_TILE_COLS (i.e. 64), respectively, i.e.:
> TileCols specifies the number of tiles across the frame. It is a
requirement of bitstream conformance that TileCols is less
> than or equal to MAX_TILE_COLS.
> TileRows specifies the number of tiles down the frame. It is a
requirement of bitstream conformance that TileRows is less
> than or equal to MAX_TILE_ROWS.
In which case only the first 64 members would be filled when actually
submitting to the accelerator, i.e.:
https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/windows/d3d11_av1_accelerator.cc;drc=242da5037807dde3daf097ba74f875db83b8b613;l=316
Given what I said above, I feel confident with the current implementation.
> struct v4l2_ctrl_av1_frame_header {
> struct v4l2_av1_tile_info tile_info;
> struct v4l2_av1_quantization quantization;
> struct v4l2_av1_segmentation segmentation;
> struct v4l2_av1_loop_filter loop_filter;
> struct v4l2_av1_cdef cdef;
> struct v4l2_av1_loop_restoration loop_restoration;
> struct v4l2_av1_global_motion global_motion;
>
> We used "v4l2_ctrl_vp9_frame" for the similar purpose.
>
> I thought "_header" can be confusing in a sense that these are
> parameters setup from parsing av1 frame header,
> not necessarily "header" itself.
>
> How about making it "v4l2_ctrl_av1_frame" similar to vp9,
> or "v4l2_ctrl_av1_frame_params"?
Ok, expect this change on RFC v3.
> Don't we also need V4L2_PIX_FMT_AV1 in addition to V4L2_PIX_FMT_AV1_FRAME
> as we do with both VP8 and VP9? I see V4L2_PIX_FMT_AV1 is missing.
No, as the non "_FRAME" pixformats are used for the stateful interface.
> 1. "tab" seems to be used before "descr = ". […] for other cases.
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1441,6 +1441,7 @@ static void v4l_fill_fmtdesc(struct
v4l2_fmtdesc *fmt)
> case V4L2_PIX_FMT_MT21C: descr = "Mediatek
> Compressed Format"; break;
> + case V4L2_PIX_FMT_AV1_FRAME: descr = "AV1 Frame"; break;
>
> 2. nit: s/the AOMedia/the AOMedia/
>
> This patch adds the AOMedia Video 1 (AV1) kernel uAPI.
>
> 3. nit: s/AV1 film_gram/AV1 film grain/ ?
> +++ b/include/media/v4l2-ctrls.h
>
> + * @p_av1_film_grain: Pointer to an AV1 film_grain.
Will be fixed in v3.
>
> #define V4L2_AV1_LOOP_FILTER_FLAG_DELTA_LF_PRESENT BIT(2)
>
> /**
> * struct v4l2_av1_loop_filter - AV1 Loop filter params as defined in
section
> * 6.8.10. "Loop filter semantics" of the AV1 specification.
> ......
>
> struct v4l2_av1_loop_filter {
> ......
> __u8 delta_lf_res;
> __u8 delta_lf_multi;
> };
>
> - I think we should mention "6.8.16. Loop filter delta parameters
semantics" in the comment too.
>
Ok
> - What was the reason "delta_lf_present" is defined with
V4L2_AV1_LOOP_FILTER_FLAG_DELTA_LF_PRESENT
> instead of being inside of "v4l2_av1_loop_filter"?
> In other words, why do we want to treat it differently from
delta_lf_res or delta_lf_multi?
> I am asking this question as this was confusing to me.
Usually we try to keep single-bit flags into a single "flags" field to
save space. It is not a rule, but tends to get applied most of the time
(by almost all codec APIs, not only V4L2 stateless)
I did fail to see that delta_lf_multi is only a single bit wide though,
so by RFC v3 I will possibly have a flag for it as well.
> AV1 uAPI is using BIT() macro, which is probably from a kernel
internal header <linux/bits.h>.
> Is this planned usage? We think we can't include it from userspace.
>
> Thank you Chen-Yu for sharing his thought on the issue.
Looking at the other codec APIs in V4L2 stateless, apparently the
default is to declare the flag using a literal. I will convert the flags
in AV1 to not use the BIT macro anymore.
> Question about update_ref_delta, update_mode_delta flags for loop
filter params in the spec.
>
> I don't see these flags in v4l2_av1_loop_filter struct.
>
> After looking at gstreamer implementation, I think arrays ref_deltas,
mode_deltas are only filled in when these flags are 1.
>
> Is this correct understanding?
> If not, can you explain the background why these flags are omitted?
Possibly forgotten. Will fix in RFC v3.
> I am not sure how to setup loop_restoration_size[0] in
v4l2_av1_loop_restoration struct,
> which seems to use RESTORATION_TILESIZE_MAX = 256 at least for
gstreamer implementation.
>
> Is this RESTORATION_TILESIZE_MAX something potentially needs to be
added in the API by any chance?
> I do see this from the AV1 spec.
I usually only #define constants if they're used in the actual uAPI code
somehow as opposed to in intermediary steps such as parsing. You can
still define the spec constants as needed in userspace code. In this
particular case, you can compute loop_restoration_size[0] by following
the spec implementation, i.e.:
LoopRestorationSize[ 0 ] = RESTORATION_TILESIZE_MAX >> (2 - lr_unit_shift);
Where you can #define RESTORATION_TILESIZE_MAX 256 in your own userspace
code without it having to be part of the uAPI. I don't believe that
drivers will ever use that constant, but those that do may #define it on
their own code.
> could you have the V4L2 CID stuff inserted consistently? In some
places they are inserted before stateless HEVC / after VP8_FRAME, while
in others they are after VP9_FRAME. I'd expect them all to be at the
very end of the stateless block, after VP9_FRAME,
>
I am adding this feedback from Chen-Yu ^ as a sign that the issue it
talks about will be fixed in RFC v3.
-- Daniel
^ permalink raw reply
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop
From: Michael Nazzareno Trimarchi @ 2022-05-31 15:50 UTC (permalink / raw)
To: Tommaso Merciai
Cc: Jacopo Mondi, linuxfancy, linux-amarula, Shunqian Zheng,
Mauro Carvalho Chehab, linux-media, linux-kernel
In-Reply-To: <20220531154040.GA1331064@tom-ThinkPad-T14s-Gen-2i>
Hi
On Tue, May 31, 2022 at 5:40 PM Tommaso Merciai
<tommaso.merciai@amarulasolutions.com> wrote:
>
> Hi Jacopo,
> On Tue, May 31, 2022 at 03:14:09PM +0200, Jacopo Mondi wrote:
> > Hi Tommaso,
> >
> > On Thu, May 19, 2022 at 09:51:14AM +0200, Tommaso Merciai wrote:
> > > Enable regulator using regulator_bulk_enable/regulatore_bulk_disable
> > > function in __ov5695_power_on/__ov5695_power_off function instead of for loop.
> > > This reduce code size and make things more clear
> > >
> > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > > Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > ---
> > > drivers/media/i2c/ov5695.c | 25 +++++++------------------
> > > 1 file changed, 7 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
> > > index 439385938a51..880b586e55fe 100644
> > > --- a/drivers/media/i2c/ov5695.c
> > > +++ b/drivers/media/i2c/ov5695.c
> > > @@ -972,7 +972,7 @@ static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
> > >
> > > static int __ov5695_power_on(struct ov5695 *ov5695)
> > > {
> > > - int i, ret;
> > > + int ret;
> > > struct device *dev = &ov5695->client->dev;
> > >
> > > ret = clk_prepare_enable(ov5695->xvclk);
> > > @@ -987,13 +987,10 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> > > * The hardware requires the regulators to be powered on in order,
> > > * so enable them one by one.
> > > */
> >
> > The comment says that the hardware requires regulators to be enabled
> > in precise order
> >
They are enabled on the array order.
> > > - for (i = 0; i < OV5695_NUM_SUPPLIES; i++) {
> > > - ret = regulator_enable(ov5695->supplies[i].consumer);
> > > - if (ret) {
> > > - dev_err(dev, "Failed to enable %s: %d\n",
> > > - ov5695->supplies[i].supply, ret);
> > > - goto disable_reg_clk;
> > > - }
> > > + ret = regulator_bulk_enable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
> >
> > bulk_enable() uses the async API (async_schedule_domain() in
> > particular) which by the name makes me think such ordering guarantee
> > cannot be respected.
I don't think so. Will make no sense because if it fails, revert them.
Even the bulk disable disable them
in reverse order
> >
> > However most sensors require some kind of ordering when enabling
> > regulators, and most of the use the bulk API anyhow. The fact this
> > driver uses the bulk API to get an release the regulators but not for
> > enabling them and the above comment, makes me think it has been done
> > on purpose ? Could you check with the driver author maybe ?
>
> Thanks for suggestion, good question.
> I see also ov5693 driver use bulk_enable/bulk_disable
> on ov5693_sensor_powerdown and ov5693_sensor_powerup functions, I take
> this as reference (and I'm wrong)
>
> In a functional test on PX30_Mini_evb_v11_20190507, after this series
> I'm able to see the correct chip id during probe and do some capture.
>
> I think you are right Jacopo, we can drop off this [PATCH 1/4]
> On the following link I found the issue that you describe: [1]
>
WHy drop?
Michael
> >
> > > + if (ret) {
> > > + dev_err(dev, "Failed to enable regulators %d\n", ret);
> > > + goto disable_reg_clk;
> > > }
> > >
> > > gpiod_set_value_cansleep(ov5695->reset_gpio, 0);
> > > @@ -1003,8 +1000,7 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> > > return 0;
> > >
> > > disable_reg_clk:
> > > - for (--i; i >= 0; i--)
> > > - regulator_disable(ov5695->supplies[i].consumer);
> > > + regulator_bulk_disable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
> >
> > FYI the bulk API does this for you if enabling any of the regulators fails.
> > Hence this should not be necessary.
>
> Thanks for sharing! This is new to me.
> I'll update the series on v2 removing this patch.
>
> Regards,
> Tommaso
>
> [1]: https://mailweb.openeuler.org/hyperkitty/list/kernel@openeuler.org/message/4X54QYJDRRE4K5BW4FTDZUGRAL4GRQWY/
>
> > Thanks
> > j
> >
> > > clk_disable_unprepare(ov5695->xvclk);
> > >
> > > return ret;
> > > @@ -1012,8 +1008,6 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> > >
> > > static void __ov5695_power_off(struct ov5695 *ov5695)
> > > {
> > > - struct device *dev = &ov5695->client->dev;
> > > - int i, ret;
> > >
> > > clk_disable_unprepare(ov5695->xvclk);
> > > gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
> > > @@ -1022,12 +1016,7 @@ static void __ov5695_power_off(struct ov5695 *ov5695)
> > > * The hardware requires the regulators to be powered off in order,
> > > * so disable them one by one.
> > > */
> > > - for (i = OV5695_NUM_SUPPLIES - 1; i >= 0; i--) {
> > > - ret = regulator_disable(ov5695->supplies[i].consumer);
> > > - if (ret)
> > > - dev_err(dev, "Failed to disable %s: %d\n",
> > > - ov5695->supplies[i].supply, ret);
> > > - }
> > > + regulator_bulk_disable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
> > > }
> > >
> > > static int __maybe_unused ov5695_runtime_resume(struct device *dev)
> > > --
> > > 2.25.1
> > >
>
> --
> Tommaso Merciai
> Embedded Linux Engineer
> tommaso.merciai@amarulasolutions.com
> __________________________________
>
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> T. +39 042 243 5310
> info@amarulasolutions.com
> www.amarulasolutions.com
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop
From: Tommaso Merciai @ 2022-05-31 15:40 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linuxfancy, linux-amarula, michael, Shunqian Zheng,
Mauro Carvalho Chehab, linux-media, linux-kernel
In-Reply-To: <20220531131409.f54znvogejkwqqkf@uno.localdomain>
Hi Jacopo,
On Tue, May 31, 2022 at 03:14:09PM +0200, Jacopo Mondi wrote:
> Hi Tommaso,
>
> On Thu, May 19, 2022 at 09:51:14AM +0200, Tommaso Merciai wrote:
> > Enable regulator using regulator_bulk_enable/regulatore_bulk_disable
> > function in __ov5695_power_on/__ov5695_power_off function instead of for loop.
> > This reduce code size and make things more clear
> >
> > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> > ---
> > drivers/media/i2c/ov5695.c | 25 +++++++------------------
> > 1 file changed, 7 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
> > index 439385938a51..880b586e55fe 100644
> > --- a/drivers/media/i2c/ov5695.c
> > +++ b/drivers/media/i2c/ov5695.c
> > @@ -972,7 +972,7 @@ static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
> >
> > static int __ov5695_power_on(struct ov5695 *ov5695)
> > {
> > - int i, ret;
> > + int ret;
> > struct device *dev = &ov5695->client->dev;
> >
> > ret = clk_prepare_enable(ov5695->xvclk);
> > @@ -987,13 +987,10 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> > * The hardware requires the regulators to be powered on in order,
> > * so enable them one by one.
> > */
>
> The comment says that the hardware requires regulators to be enabled
> in precise order
>
> > - for (i = 0; i < OV5695_NUM_SUPPLIES; i++) {
> > - ret = regulator_enable(ov5695->supplies[i].consumer);
> > - if (ret) {
> > - dev_err(dev, "Failed to enable %s: %d\n",
> > - ov5695->supplies[i].supply, ret);
> > - goto disable_reg_clk;
> > - }
> > + ret = regulator_bulk_enable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
>
> bulk_enable() uses the async API (async_schedule_domain() in
> particular) which by the name makes me think such ordering guarantee
> cannot be respected.
>
> However most sensors require some kind of ordering when enabling
> regulators, and most of the use the bulk API anyhow. The fact this
> driver uses the bulk API to get an release the regulators but not for
> enabling them and the above comment, makes me think it has been done
> on purpose ? Could you check with the driver author maybe ?
Thanks for suggestion, good question.
I see also ov5693 driver use bulk_enable/bulk_disable
on ov5693_sensor_powerdown and ov5693_sensor_powerup functions, I take
this as reference (and I'm wrong)
In a functional test on PX30_Mini_evb_v11_20190507, after this series
I'm able to see the correct chip id during probe and do some capture.
I think you are right Jacopo, we can drop off this [PATCH 1/4]
On the following link I found the issue that you describe: [1]
>
> > + if (ret) {
> > + dev_err(dev, "Failed to enable regulators %d\n", ret);
> > + goto disable_reg_clk;
> > }
> >
> > gpiod_set_value_cansleep(ov5695->reset_gpio, 0);
> > @@ -1003,8 +1000,7 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> > return 0;
> >
> > disable_reg_clk:
> > - for (--i; i >= 0; i--)
> > - regulator_disable(ov5695->supplies[i].consumer);
> > + regulator_bulk_disable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
>
> FYI the bulk API does this for you if enabling any of the regulators fails.
> Hence this should not be necessary.
Thanks for sharing! This is new to me.
I'll update the series on v2 removing this patch.
Regards,
Tommaso
[1]: https://mailweb.openeuler.org/hyperkitty/list/kernel@openeuler.org/message/4X54QYJDRRE4K5BW4FTDZUGRAL4GRQWY/
> Thanks
> j
>
> > clk_disable_unprepare(ov5695->xvclk);
> >
> > return ret;
> > @@ -1012,8 +1008,6 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> >
> > static void __ov5695_power_off(struct ov5695 *ov5695)
> > {
> > - struct device *dev = &ov5695->client->dev;
> > - int i, ret;
> >
> > clk_disable_unprepare(ov5695->xvclk);
> > gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
> > @@ -1022,12 +1016,7 @@ static void __ov5695_power_off(struct ov5695 *ov5695)
> > * The hardware requires the regulators to be powered off in order,
> > * so disable them one by one.
> > */
> > - for (i = OV5695_NUM_SUPPLIES - 1; i >= 0; i--) {
> > - ret = regulator_disable(ov5695->supplies[i].consumer);
> > - if (ret)
> > - dev_err(dev, "Failed to disable %s: %d\n",
> > - ov5695->supplies[i].supply, ret);
> > - }
> > + regulator_bulk_disable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
> > }
> >
> > static int __maybe_unused ov5695_runtime_resume(struct device *dev)
> > --
> > 2.25.1
> >
--
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply
* [PATCH v11 5/5] media: renesas: vsp1: Add support for RZ/G2L VSPD
From: Biju Das @ 2022-05-31 14:19 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media,
linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
In-Reply-To: <20220531141958.575616-1-biju.das.jz@bp.renesas.com>
The RZ/G2L VSPD provides a single VSPD instance. It has the following
sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.
The VSPD block on RZ/G2L SoCs does not have a version register, so
added a new compatible string "renesas,r9a07g044-vsp2" with a data
pointer containing the info structure. Also the reset line is shared
with the DU module.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
v10->v11:
* No change.
v9->v10:
* No change.
v8->v9:
* Replaced the code comments RZ/G2L {SoC's,SoC} with RZ/G2L SoCs.
v7->v8:
* Split the patch for adding s/w version, feature bit and RZ/G2L support
* Added feature bit VSP1_HAS_NON_ZERO_LBA to device_info
* Added .soc for RZ/G2L
* Replaced the compatible "renesas,rzg2l-vsp2" -> "renesas,r9a07g044-vsp2"
v6->v7:
* Added Rb tag from Kieran
* Added a quirk to handle LIF0 buffer attribute related
changes for V3M and G2L.
* Removed the macro for VSP HW version
v5->v6:
* Rebased to media_staging and updated commit header
* Removed the extra tab from rzg2l_vsp2_device_info
* Changed the function vsp1_lookup->vsp1_lookup_info and
all info match related code moved here.
* Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
distinguish HW & SW IP_VSP_Version.
* Used 0x80 for RZG2L VSPD model and SoC identification
* Updated Switch() for LIF0 buffer attribute handling.
v4->v5:
* Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
* To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
for RZ/G2L SoC's.
v3->v4:
* Added Rb tag from Geert
* Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
v2->v3:
* Fixed version comparison in vsp1_lookup()
v1->v2:
* Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
* Added standalone device info for rzg2l-vsp2.
* Added vsp1_lookup helper function.
* Updated comments for LIF0 buffer attribute register
* Used last ID for rzg2l-vsp2.
RFC->v1:
* Used data pointer containing info structure to retrieve version information
RFC:
* https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
---
drivers/media/platform/renesas/vsp1/vsp1_drv.c | 13 +++++++++++++
drivers/media/platform/renesas/vsp1/vsp1_lif.c | 9 +++++----
drivers/media/platform/renesas/vsp1/vsp1_regs.h | 4 ++++
3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 256794c67e63..dd37fe81c4c5 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -833,6 +833,18 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
},
};
+static const struct vsp1_device_info rzg2l_vsp2_device_info = {
+ .version = VI6_IP_VERSION_MODEL_VSPD_RZG2L,
+ .model = "VSP2-D",
+ .soc = VI6_IP_VERSION_SOC_RZG2L,
+ .gen = 3,
+ .features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP | VSP1_HAS_EXT_DL
+ | VSP1_HAS_NON_ZERO_LBA,
+ .lif_count = 1,
+ .rpf_count = 2,
+ .wpf_count = 1,
+};
+
static const struct vsp1_device_info *vsp1_lookup_info(struct vsp1_device *vsp1)
{
const struct vsp1_device_info *info;
@@ -983,6 +995,7 @@ static int vsp1_remove(struct platform_device *pdev)
static const struct of_device_id vsp1_of_match[] = {
{ .compatible = "renesas,vsp1" },
{ .compatible = "renesas,vsp2" },
+ { .compatible = "renesas,r9a07g044-vsp2", .data = &rzg2l_vsp2_device_info },
{ },
};
MODULE_DEVICE_TABLE(of, vsp1_of_match);
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lif.c b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
index 9adb892edcdc..186a5730e1e3 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
@@ -107,6 +107,7 @@ static void lif_configure_stream(struct vsp1_entity *entity,
case VI6_IP_VERSION_MODEL_VSPDL_GEN3:
case VI6_IP_VERSION_MODEL_VSPD_V3:
+ case VI6_IP_VERSION_MODEL_VSPD_RZG2L:
hbth = 0;
obth = 1500;
lbth = 0;
@@ -130,10 +131,10 @@ static void lif_configure_stream(struct vsp1_entity *entity,
VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
/*
- * On R-Car V3M the LIF0 buffer attribute register has to be set to a
- * non-default value to guarantee proper operation (otherwise artifacts
- * may appear on the output). The value required by the manual is not
- * explained but is likely a buffer size or threshold.
+ * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to be
+ * set to a non-default value to guarantee proper operation (otherwise
+ * artifacts may appear on the output). The value required by the
+ * manual is not explained but is likely a buffer size or threshold.
*/
if (vsp1_feature(entity->vsp1, VSP1_HAS_NON_ZERO_LBA))
vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
index 4286d13eca32..8928f4c6bb55 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
@@ -767,6 +767,8 @@
#define VI6_IP_VERSION_MODEL_VSPDL_GEN3 (0x19 << 8)
#define VI6_IP_VERSION_MODEL_VSPBS_GEN3 (0x1a << 8)
#define VI6_IP_VERSION_MODEL_VSPD_V3U (0x1c << 8)
+/* RZ/G2L SoCs have no version register, So use 0x80 as the model version */
+#define VI6_IP_VERSION_MODEL_VSPD_RZG2L (0x80 << 8)
#define VI6_IP_VERSION_SOC_MASK (0xff << 0)
#define VI6_IP_VERSION_SOC_H2 (0x01 << 0)
@@ -780,6 +782,8 @@
#define VI6_IP_VERSION_SOC_M3N (0x04 << 0)
#define VI6_IP_VERSION_SOC_E3 (0x04 << 0)
#define VI6_IP_VERSION_SOC_V3U (0x05 << 0)
+/* RZ/G2L SoCs have no version register, So use 0x80 for SoC Identification */
+#define VI6_IP_VERSION_SOC_RZG2L (0x80 << 0)
#define VI6_IP_VERSION_VSP_SW (0xfffe << 16) /* SW VSP version */
--
2.25.1
^ permalink raw reply related
* [PATCH v11 4/5] media: renesas: vsp1: Add VSP1_HAS_NON_ZERO_LBA feature bit
From: Biju Das @ 2022-05-31 14:19 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media,
linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
In-Reply-To: <20220531141958.575616-1-biju.das.jz@bp.renesas.com>
As per HW manual V3M and RZ/G2L SoCs has nonzero LIF buffer
attributes. So, introduce a feature bit for handling the same.
This patch also adds separate device info structure for V3M and V3H
SoCs, as both these SoCs share the same model ID, but V3H does not
have VSP1_HAS_NON_ZERO_LBA feature bit.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v10->v11:
* No change
v9->v10:
* No change
v8->v9:
* Used generic check for matching SoCs with LBA feature.
v8:
* New patch
---
drivers/media/platform/renesas/vsp1/vsp1.h | 1 +
drivers/media/platform/renesas/vsp1/vsp1_drv.c | 18 +++++++++++++++++-
drivers/media/platform/renesas/vsp1/vsp1_lif.c | 3 +--
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
index ff4435705abb..2f6f0c6ae555 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1.h
@@ -55,6 +55,7 @@ struct vsp1_uif;
#define VSP1_HAS_HGT BIT(8)
#define VSP1_HAS_BRS BIT(9)
#define VSP1_HAS_EXT_DL BIT(10)
+#define VSP1_HAS_NON_ZERO_LBA BIT(11)
struct vsp1_device_info {
u32 version;
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 43e3740bb041..256794c67e63 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -791,6 +791,7 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
}, {
.version = VI6_IP_VERSION_MODEL_VSPD_V3,
.model = "VSP2-D",
+ .soc = VI6_IP_VERSION_SOC_V3H,
.gen = 3,
.features = VSP1_HAS_BRS | VSP1_HAS_BRU,
.lif_count = 1,
@@ -798,6 +799,17 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
.uif_count = 1,
.wpf_count = 1,
.num_bru_inputs = 5,
+ }, {
+ .version = VI6_IP_VERSION_MODEL_VSPD_V3,
+ .model = "VSP2-D",
+ .soc = VI6_IP_VERSION_SOC_V3M,
+ .gen = 3,
+ .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_NON_ZERO_LBA,
+ .lif_count = 1,
+ .rpf_count = 5,
+ .uif_count = 1,
+ .wpf_count = 1,
+ .num_bru_inputs = 5,
}, {
.version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
.model = "VSP2-DL",
@@ -841,8 +853,12 @@ static const struct vsp1_device_info *vsp1_lookup_info(struct vsp1_device *vsp1)
for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
info = &vsp1_device_infos[i];
- if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version)
+ if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version) {
+ if (info->soc &&
+ ((vsp1->version & VI6_IP_VERSION_SOC_MASK) != info->soc))
+ continue;
return info;
+ }
}
dev_err(vsp1->dev, "unsupported IP version 0x%08x\n", vsp1->version);
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lif.c b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
index 6a6857ac9327..9adb892edcdc 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
@@ -135,8 +135,7 @@ static void lif_configure_stream(struct vsp1_entity *entity,
* may appear on the output). The value required by the manual is not
* explained but is likely a buffer size or threshold.
*/
- if ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
- (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
+ if (vsp1_feature(entity->vsp1, VSP1_HAS_NON_ZERO_LBA))
vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
VI6_LIF_LBA_LBA0 |
(1536 << VI6_LIF_LBA_LBA1_SHIFT));
--
2.25.1
^ permalink raw reply related
* [PATCH v11 3/5] media: renesas: vsp1: Add support for VSP software version
From: Biju Das @ 2022-05-31 14:19 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media,
linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
In-Reply-To: <20220531141958.575616-1-biju.das.jz@bp.renesas.com>
The VSPD block on RZ/G2L SoCs does not have a version register.
This patch adds support for adding VSP software version based on
device match.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v10->v11:
* No change
v9->v10:
* No change
v8->v9:
* Added Rb tag from Geert
* Updated commit description RZ/G2L -> RZ/G2L SoCs.
* Replaced break with return info in case a Model match is found and
removed additional check for non-match case.
v8:
* New patch
---
drivers/media/platform/renesas/vsp1/vsp1.h | 1 +
.../media/platform/renesas/vsp1/vsp1_drv.c | 43 +++++++++++++------
.../media/platform/renesas/vsp1/vsp1_regs.h | 2 +
3 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
index baf898d577ec..ff4435705abb 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1.h
@@ -67,6 +67,7 @@ struct vsp1_device_info {
unsigned int uif_count;
unsigned int wpf_count;
unsigned int num_bru_inputs;
+ u8 soc;
bool uapi;
};
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 466826db29f9..43e3740bb041 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -821,11 +821,39 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
},
};
+static const struct vsp1_device_info *vsp1_lookup_info(struct vsp1_device *vsp1)
+{
+ const struct vsp1_device_info *info;
+ unsigned int i;
+
+ /*
+ * Try the info stored in match data first for devices that don't have
+ * a version register.
+ */
+ info = of_device_get_match_data(vsp1->dev);
+ if (info) {
+ vsp1->version = VI6_IP_VERSION_VSP_SW | info->version | info->soc;
+ return info;
+ }
+
+ vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
+
+ for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
+ info = &vsp1_device_infos[i];
+
+ if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version)
+ return info;
+ }
+
+ dev_err(vsp1->dev, "unsupported IP version 0x%08x\n", vsp1->version);
+
+ return NULL;
+}
+
static int vsp1_probe(struct platform_device *pdev)
{
struct vsp1_device *vsp1;
struct device_node *fcp_node;
- unsigned int i;
int ret;
int irq;
@@ -881,19 +909,8 @@ static int vsp1_probe(struct platform_device *pdev)
if (ret < 0)
goto done;
- vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
-
- for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
- if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) ==
- vsp1_device_infos[i].version) {
- vsp1->info = &vsp1_device_infos[i];
- break;
- }
- }
-
+ vsp1->info = vsp1_lookup_info(vsp1);
if (!vsp1->info) {
- dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
- vsp1->version);
vsp1_device_put(vsp1);
ret = -ENXIO;
goto done;
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
index fae7286eb01e..4286d13eca32 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
@@ -781,6 +781,8 @@
#define VI6_IP_VERSION_SOC_E3 (0x04 << 0)
#define VI6_IP_VERSION_SOC_V3U (0x05 << 0)
+#define VI6_IP_VERSION_VSP_SW (0xfffe << 16) /* SW VSP version */
+
/* -----------------------------------------------------------------------------
* RPF CLUT Registers
*/
--
2.25.1
^ permalink raw reply related
* [PATCH v11 2/5] media: renesas: vsp1: Add support to deassert/assert reset line
From: Biju Das @ 2022-05-31 14:19 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Philipp Zabel
Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media,
linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
In-Reply-To: <20220531141958.575616-1-biju.das.jz@bp.renesas.com>
As the resets DT property is mandatory, and is present in all .dtsi
in mainline, add support to perform deassert/assert using reference
counted reset handle.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v10->v11:
* To avoid lock-up on R-Car Gen2, added poll for reset status after deassert.
v9->v10:
* Moved {deassert,assert} calls to vsp1_pm_runtime_{resume,suspend}
v8->v9:
* No change
v7->v8:
* No Change
v6->v7:
* No change
v5->v6:
* Rebased to media_staging and updated commit header
* Added Rb tag from Laurent
* Added forward declaration for struct reset_control
* Updated vsp1_device_get() with changes suggested by Laurent
* Updated error message for reset_control_get form ctrl->control.
v4->v5:
* Added Rb tag from Geert
v3->v4:
* Restored error check for pm_runtime_resume_and_get and calls
assert() in case of failure.
v2->v3:
* Added Rb tag from Philipp
* If reset_control_deassert() failed, return ret directly.
v1->v2:
* Used reference counted reset handle to perform deassert/assert
RFC->v1:
* Added reset support as separate patch
* Moved rstc just after the bus_master field in struct vsp1_device
RFC:
* https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
---
drivers/media/platform/renesas/vsp1/vsp1.h | 2 ++
.../media/platform/renesas/vsp1/vsp1_drv.c | 32 +++++++++++++++++--
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
index 37cf33c7e6ca..baf898d577ec 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1.h
@@ -22,6 +22,7 @@
struct clk;
struct device;
struct rcar_fcp_device;
+struct reset_control;
struct vsp1_drm;
struct vsp1_entity;
@@ -79,6 +80,7 @@ struct vsp1_device {
void __iomem *mmio;
struct rcar_fcp_device *fcp;
struct device *bus_master;
+ struct reset_control *rstc;
struct vsp1_brx *brs;
struct vsp1_brx *bru;
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 1f73c48eb738..466826db29f9 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -11,11 +11,13 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/interrupt.h>
+#include <linux/iopoll.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/reset.h>
#include <linux/videodev2.h>
#include <media/rcar-fcp.h>
@@ -622,6 +624,7 @@ static int __maybe_unused vsp1_pm_runtime_suspend(struct device *dev)
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
rcar_fcp_disable(vsp1->fcp);
+ reset_control_assert(vsp1->rstc);
return 0;
}
@@ -631,13 +634,33 @@ static int __maybe_unused vsp1_pm_runtime_resume(struct device *dev)
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
int ret;
+ ret = reset_control_deassert(vsp1->rstc);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * On R-Car Gen2, vsp1 register access after deassert can cause
+ * lock-up. Therefore, we need to poll the status of the reset to
+ * avoid lock-up.
+ */
+ ret = read_poll_timeout_atomic(reset_control_status, ret, ret == 0, 1,
+ 100, false, vsp1->rstc);
+ if (ret < 0)
+ goto done;
+
if (vsp1->info) {
ret = vsp1_device_init(vsp1);
if (ret < 0)
- return ret;
+ goto done;
}
- return rcar_fcp_enable(vsp1->fcp);
+ ret = rcar_fcp_enable(vsp1->fcp);
+
+done:
+ if (ret < 0)
+ reset_control_assert(vsp1->rstc);
+
+ return ret;
}
static const struct dev_pm_ops vsp1_pm_ops = {
@@ -825,6 +848,11 @@ static int vsp1_probe(struct platform_device *pdev)
if (irq < 0)
return irq;
+ vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
+ if (IS_ERR(vsp1->rstc))
+ return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
+ "failed to get reset control\n");
+
/* FCP (optional). */
fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
if (fcp_node) {
--
2.25.1
^ permalink raw reply related
* [PATCH v11 1/5] media: dt-bindings: media: renesas,vsp1: Document RZ/G2L VSPD bindings
From: Biju Das @ 2022-05-31 14:19 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski
Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media,
linux-renesas-soc, devicetree, Geert Uytterhoeven, Chris Paterson,
Biju Das, Prabhakar Mahadev Lad, Krzysztof Kozlowski
In-Reply-To: <20220531141958.575616-1-biju.das.jz@bp.renesas.com>
Document VSPD found in RZ/G2L SoC. VSPD block is similar to VSP2-D
found on R-Car SoC's, but it does not have a version register and
it has 3 clocks compared to 1 clock on vsp1 and vsp2.
This patch introduces a new compatible 'renesas,r9a07g044-vsp2' to
handle these differences.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v10->v11:
* No change
v9->v10:
* No change
v8->v9:
* No change
v7->v8:
* Added Clock-names to false for Non RZ/G2L SoC's
* Replaced compatble 'renesas,rzg2l-vsp2'->'renesas,r9a07g044-vsp2'
* Removed RZ/V2L SoC, will be added later after testing it.
* Added Rb tag from Laurent.
v6->v7:
* No change
v5->v6:
* Removed LCDC reference clock description
* Changed the clock name from du.0->aclk
v4->v5:
* No change
v3->v4:
* No change
v2->v3:
* Added Rb tag from Krzysztof.
v1->v2:
* Changed compatible from vsp2-rzg2l->rzg2l-vsp2
RFC->v1:
* Updated commit description
* Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
* Defined the clocks
* Clock max Items is based on SoC Compatible string
RFC:
* https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-20-biju.das.jz@bp.renesas.com/
---
.../bindings/media/renesas,vsp1.yaml | 53 ++++++++++++++-----
1 file changed, 40 insertions(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
index 990e9c1dbc43..7a8f32473852 100644
--- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
@@ -17,6 +17,7 @@ description:
properties:
compatible:
enum:
+ - renesas,r9a07g044-vsp2 # RZ/G2L
- renesas,vsp1 # R-Car Gen2 and RZ/G1
- renesas,vsp2 # R-Car Gen3 and RZ/G2
@@ -26,8 +27,8 @@ properties:
interrupts:
maxItems: 1
- clocks:
- maxItems: 1
+ clocks: true
+ clock-names: true
power-domains:
maxItems: 1
@@ -50,17 +51,43 @@ required:
additionalProperties: false
-if:
- properties:
- compatible:
- items:
- - const: renesas,vsp1
-then:
- properties:
- renesas,fcp: false
-else:
- required:
- - renesas,fcp
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: renesas,vsp1
+ then:
+ properties:
+ renesas,fcp: false
+ else:
+ required:
+ - renesas,fcp
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: renesas,r9a07g044-vsp2
+ then:
+ properties:
+ clocks:
+ items:
+ - description: Main clock
+ - description: Register access clock
+ - description: Video clock
+ clock-names:
+ items:
+ - const: aclk
+ - const: pclk
+ - const: vclk
+ required:
+ - clock-names
+ else:
+ properties:
+ clocks:
+ maxItems: 1
+ clock-names: false
examples:
# R8A7790 (R-Car H2) VSP1-S
--
2.25.1
^ permalink raw reply related
* [PATCH v11 0/5] Add support for RZ/G2L VSPD
From: Biju Das @ 2022-05-31 14:19 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Biju Das, Laurent Pinchart, Philipp Zabel, Kieran Bingham,
linux-media, linux-renesas-soc, Geert Uytterhoeven,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
The RZ/G2L VSPD provides a single VSPD instance. It has the following
sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.
The VSPD block on RZ/G2L does not have a version register, so added a
new compatible string "renesas,r9a07g044-vsp2" with a data pointer containing
the info structure. Also the reset line is shared with the DU module.
This patch series is tested on RZ/G1N, RZ/G2M and RZ/G2L boards.
v10->v11:
* Added poll for reset status in order to avoid lock-up on R-Car Gen2
* with vsp register access after deassert.
v9->v10
* Moved {deassert,assert} calls to vsp1_pm_runtime_{resume,suspend}
V8->v9
* Added Rb tag from Geert for patch#3
* Replaced break with return info in case a Model match is found and
removed additional check for non-match case.
* Used generic check for matching SoCs with LBA feature.
* Replaced the code comments RZ/G2L {SoC's,SoC} with RZ/G2L SoCs.
v7->v8:
* Split the patch for adding s/w version, feature bit and RZ/G2L support
* Added feature bit VSP1_HAS_NON_ZERO_LBA to device_info
* Added .soc for RZ/G2L
* Replaced the compatible "renesas,rzg2l-vsp2" -> "renesas,r9a07g044-vsp2"
* Updated Clock-names to false for non RZ/G2L SoC's on binding doc
* Added Rb tag from Laurent for bindings
v6->v7:
* Added Rb tag from Kieran for patch#3
* Added a quirk to handle LIF0 buffer attribute related
changes for V3M and G2L.
* Removed the macro for VSP HW version
v5->v6:
* Rebased to media_staging and updated commit header
* Removed LCDC reference clock description from bindings
* Changed the clock name from du.0->aclk from bindings
* Added Rb tag from Laurent for reset patch
* Added forward declaration for struct reset_control
* Updated vsp1_device_get() with changes suggested by Laurent
* Updated error message for reset_control_get form ctrl->control.
* Removed the extra tab from rzg2l_vsp2_device_info
* Changed the function vsp1_lookup->vsp1_lookup_info and
all info match related code moved here.
* Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
distinguish HW & SW IP_VSP_Version.
* Used 0x80 for RZG2L VSPD model and SoC identification
* Updated Switch() for LIF0 buffer attribute handling.
v4->v5:
* Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
* To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
for SoC identification for RZ/G2L SoC's.
v3->v4:
* Restored error check for pm_runtime_resume_and_get and calls
assert() in case of failure.
* Added Rb tag from Geert
* Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M SoC's
v2->v3:
* Added Rb tags from Krzysztof and Philipp
* If reset_control_deassert() failed, return ret directly.
* Fixed version comparison in vsp1_lookup()
v1->v2:
* Used reference counted reset handle to perform deassert/assert
* Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
* Added standalone device info for rzg2l-vsp2.
* Added vsp1_lookup helper function.
* Updated comments for LIF0 buffer attribute register
* Used last ID for rzg2l-vsp2.
RFC->v1:
* Added reset support as separate patch
* Moved rstc just after the bus_master field in struct vsp1_device
* Used data pointer containing info structure to retrieve version information
* Updated commit description
* Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
* Defined the clocks
* Clock max Items is based on SoC Compatible string
RFC:
* https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
* https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-20-biju.das.jz@bp.renesas.com/
Biju Das (5):
media: dt-bindings: media: renesas,vsp1: Document RZ/G2L VSPD bindings
media: renesas: vsp1: Add support to deassert/assert reset line
media: renesas: vsp1: Add support for VSP software version
media: renesas: vsp1: Add VSP1_HAS_NON_ZERO_LBA feature bit
media: renesas: vsp1: Add support for RZ/G2L VSPD
.../bindings/media/renesas,vsp1.yaml | 53 ++++++---
drivers/media/platform/renesas/vsp1/vsp1.h | 4 +
.../media/platform/renesas/vsp1/vsp1_drv.c | 104 +++++++++++++++---
.../media/platform/renesas/vsp1/vsp1_lif.c | 12 +-
.../media/platform/renesas/vsp1/vsp1_regs.h | 6 +
5 files changed, 145 insertions(+), 34 deletions(-)
--
2.25.1
^ permalink raw reply
* Re: [PATCH] media: s5k4ecgx: Switch to GPIO descriptors
From: kernel test robot @ 2022-05-31 13:56 UTC (permalink / raw)
To: Linus Walleij, Mauro Carvalho Chehab
Cc: llvm, kbuild-all, linux-media, Linus Walleij, Sylwester Nawrocki
In-Reply-To: <20220531084546.67737-1-linus.walleij@linaro.org>
Hi Linus,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on v5.18 next-20220531]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Linus-Walleij/media-s5k4ecgx-Switch-to-GPIO-descriptors/20220531-164948
base: git://linuxtv.org/media_tree.git master
config: hexagon-randconfig-r041-20220531 (https://download.01.org/0day-ci/archive/20220531/202205312118.7XAVBcnY-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c825abd6b0198fb088d9752f556a70705bc99dfd)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d6a5013d9dd546a9c9d7ed3714e861f7593b1635
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Linus-Walleij/media-s5k4ecgx-Switch-to-GPIO-descriptors/20220531-164948
git checkout d6a5013d9dd546a9c9d7ed3714e861f7593b1635
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/media/i2c/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
drivers/media/i2c/s5k4ecgx.c:897:38: warning: missing terminating '"' character [-Winvalid-pp-token]
gpiod_set_consumer_name(priv->stby, "S5K4ECGX_STBY);
^
>> drivers/media/i2c/s5k4ecgx.c:897:38: error: expected expression
>> drivers/media/i2c/s5k4ecgx.c:947:26: error: use of undeclared identifier 's5k4ecgx_id'
MODULE_DEVICE_TABLE(i2c, s5k4ecgx_id);
^
>> drivers/media/i2c/s5k4ecgx.c:954:12: error: use of undeclared identifier 's5k4ecgx_remove'
.remove = s5k4ecgx_remove,
^
drivers/media/i2c/s5k4ecgx.c:955:14: error: use of undeclared identifier 's5k4ecgx_id'
.id_table = s5k4ecgx_id,
^
>> drivers/media/i2c/s5k4ecgx.c:958:1: error: function definition is not allowed here
module_i2c_driver(v4l2_i2c_driver);
^
include/linux/i2c.h:950:2: note: expanded from macro 'module_i2c_driver'
module_driver(__i2c_driver, i2c_add_driver, \
^
include/linux/device/driver.h:260:41: note: expanded from macro 'module_driver'
static int __init __driver##_init(void) \
^
>> drivers/media/i2c/s5k4ecgx.c:958:1: error: function definition is not allowed here
include/linux/i2c.h:950:2: note: expanded from macro 'module_i2c_driver'
module_driver(__i2c_driver, i2c_add_driver, \
^
include/linux/device/driver.h:263:3: note: expanded from macro 'module_driver'
} \
^
include/linux/module.h:132:2: note: expanded from macro '\
module_init'
{ return initfn; } \
^
>> drivers/media/i2c/s5k4ecgx.c:958:1: error: function definition is not allowed here
include/linux/i2c.h:950:2: note: expanded from macro 'module_i2c_driver'
module_driver(__i2c_driver, i2c_add_driver, \
^
include/linux/device/driver.h:265:42: note: expanded from macro 'module_driver'
static void __exit __driver##_exit(void) \
^
>> drivers/media/i2c/s5k4ecgx.c:958:1: error: function definition is not allowed here
include/linux/i2c.h:950:2: note: expanded from macro 'module_i2c_driver'
module_driver(__i2c_driver, i2c_add_driver, \
^
include/linux/device/driver.h:268:3: note: expanded from macro 'module_driver'
} \
^
include/linux/module.h:140:2: note: expanded from macro '\
module_exit'
{ return exitfn; } \
^
>> drivers/media/i2c/s5k4ecgx.c:964:36: error: expected '}'
MODULE_FIRMWARE(S5K4ECGX_FIRMWARE);
^
drivers/media/i2c/s5k4ecgx.c:856:1: note: to match this '{'
{
^
>> drivers/media/i2c/s5k4ecgx.c:947:1: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
MODULE_DEVICE_TABLE(i2c, s5k4ecgx_id);
^
include/linux/module.h:244:21: note: expanded from macro 'MODULE_DEVICE_TABLE'
extern typeof(name) __mod_##type##__##name##_device_table \
^
<scratch space>:94:1: note: expanded from here
__mod_i2c__s5k4ecgx_id_device_table
^
>> drivers/media/i2c/s5k4ecgx.c:895:8: error: use of undeclared label 'out_err'
goto out_err;
^
2 warnings and 10 errors generated.
vim +897 drivers/media/i2c/s5k4ecgx.c
853
854 static int s5k4ecgx_probe(struct i2c_client *client,
855 const struct i2c_device_id *id)
856 {
857 struct s5k4ecgx_platform_data *pdata = client->dev.platform_data;
858 struct v4l2_subdev *sd;
859 struct s5k4ecgx *priv;
860 int ret, i;
861
862 if (pdata == NULL) {
863 dev_err(&client->dev, "platform data is missing!\n");
864 return -EINVAL;
865 }
866
867 priv = devm_kzalloc(&client->dev, sizeof(struct s5k4ecgx), GFP_KERNEL);
868 if (!priv)
869 return -ENOMEM;
870
871 mutex_init(&priv->lock);
872 priv->streaming = 0;
873
874 sd = &priv->sd;
875 /* Registering subdev */
876 v4l2_i2c_subdev_init(sd, client, &s5k4ecgx_ops);
877 /* Static name; NEVER use in new drivers! */
878 strscpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
879
880 sd->internal_ops = &s5k4ecgx_subdev_internal_ops;
881 /* Support v4l2 sub-device user space API */
882 sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
883
884 priv->pad.flags = MEDIA_PAD_FL_SOURCE;
885 sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
886 ret = media_entity_pads_init(&sd->entity, 1, &priv->pad);
887 if (ret)
888 return ret;
889
890 /* Request GPIO lines asserted */
891 priv->stby = devm_gpiod_get(&client->dev, "standby", GPIOD_OUT_HIGH);
892 if (IS_ERR(priv->stby)) {
893 v4l2_err(sd, "failed to request gpio S5K4ECGX_STBY");
894 ret = PTR_ERR(priv->stby);
> 895 goto out_err;
896 }
> 897 gpiod_set_consumer_name(priv->stby, "S5K4ECGX_STBY);
898 priv->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
899 if (IS_ERR(priv->reset)) {
900 v4l2_err(sd, "failed to request gpio S5K4ECGX_RST");
901 ret = PTR_ERR(priv->reset);
902 goto out_err;
903 }
904 gpiod_set_consumer_name(priv->reset, "S5K4ECGX_RST");
905
906 for (i = 0; i < S5K4ECGX_NUM_SUPPLIES; i++)
907 priv->supplies[i].supply = s5k4ecgx_supply_names[i];
908
909 ret = devm_regulator_bulk_get(&client->dev, S5K4ECGX_NUM_SUPPLIES,
910 priv->supplies);
911 if (ret) {
912 dev_err(&client->dev, "Failed to get regulators\n");
913 goto out_err;
914 }
915 ret = s5k4ecgx_init_v4l2_ctrls(priv);
916 if (ret)
917 goto out_err;
918
919 priv->curr_pixfmt = &s5k4ecgx_formats[0];
920 priv->curr_frmsize = &s5k4ecgx_prev_sizes[0];
921
922 return 0;
923
924 out_err:
925 media_entity_cleanup(&priv->sd.entity);
926
927 return ret;
928 }
929
930 static int s5k4ecgx_remove(struct i2c_client *client)
931 {
932 struct v4l2_subdev *sd = i2c_get_clientdata(client);
933 struct s5k4ecgx *priv = to_s5k4ecgx(sd);
934
935 mutex_destroy(&priv->lock);
936 v4l2_device_unregister_subdev(sd);
937 v4l2_ctrl_handler_free(&priv->handler);
938 media_entity_cleanup(&sd->entity);
939
940 return 0;
941 }
942
943 static const struct i2c_device_id s5k4ecgx_id[] = {
944 { S5K4ECGX_DRIVER_NAME, 0 },
945 {}
946 };
> 947 MODULE_DEVICE_TABLE(i2c, s5k4ecgx_id);
948
949 static struct i2c_driver v4l2_i2c_driver = {
950 .driver = {
951 .name = S5K4ECGX_DRIVER_NAME,
952 },
953 .probe = s5k4ecgx_probe,
> 954 .remove = s5k4ecgx_remove,
955 .id_table = s5k4ecgx_id,
956 };
957
> 958 module_i2c_driver(v4l2_i2c_driver);
959
960 MODULE_DESCRIPTION("Samsung S5K4ECGX 5MP SOC camera");
961 MODULE_AUTHOR("Sangwook Lee <sangwook.lee@linaro.org>");
962 MODULE_AUTHOR("Seok-Young Jang <quartz.jang@samsung.com>");
963 MODULE_LICENSE("GPL");
> 964 MODULE_FIRMWARE(S5K4ECGX_FIRMWARE);
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply
* Re: [PATCH] media: s5k4ecgx: Switch to GPIO descriptors
From: kernel test robot @ 2022-05-31 13:55 UTC (permalink / raw)
To: Linus Walleij, Mauro Carvalho Chehab
Cc: llvm, kbuild-all, linux-media, Linus Walleij, Sylwester Nawrocki
In-Reply-To: <20220531084546.67737-1-linus.walleij@linaro.org>
Hi Linus,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on v5.18 next-20220531]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Linus-Walleij/media-s5k4ecgx-Switch-to-GPIO-descriptors/20220531-164948
base: git://linuxtv.org/media_tree.git master
config: arm-randconfig-c002-20220531 (https://download.01.org/0day-ci/archive/20220531/202205312157.9EPLfsUw-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c825abd6b0198fb088d9752f556a70705bc99dfd)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/d6a5013d9dd546a9c9d7ed3714e861f7593b1635
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Linus-Walleij/media-s5k4ecgx-Switch-to-GPIO-descriptors/20220531-164948
git checkout d6a5013d9dd546a9c9d7ed3714e861f7593b1635
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/media/i2c/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
drivers/media/i2c/s5k4ecgx.c:897:38: warning: missing terminating '"' character [-Winvalid-pp-token]
gpiod_set_consumer_name(priv->stby, "S5K4ECGX_STBY);
^
>> drivers/media/i2c/s5k4ecgx.c:897:38: error: expected expression
>> drivers/media/i2c/s5k4ecgx.c:954:12: error: use of undeclared identifier 's5k4ecgx_remove'
.remove = s5k4ecgx_remove,
^
>> drivers/media/i2c/s5k4ecgx.c:955:14: error: use of undeclared identifier 's5k4ecgx_id'
.id_table = s5k4ecgx_id,
^
>> drivers/media/i2c/s5k4ecgx.c:958:1: error: function definition is not allowed here
module_i2c_driver(v4l2_i2c_driver);
^
include/linux/i2c.h:950:2: note: expanded from macro 'module_i2c_driver'
module_driver(__i2c_driver, i2c_add_driver, \
^
include/linux/device/driver.h:260:41: note: expanded from macro 'module_driver'
static int __init __driver##_init(void) \
^
>> drivers/media/i2c/s5k4ecgx.c:958:1: error: use of undeclared identifier 'v4l2_i2c_driver_init'
include/linux/i2c.h:950:2: note: expanded from macro 'module_i2c_driver'
module_driver(__i2c_driver, i2c_add_driver, \
^
include/linux/device/driver.h:264:13: note: expanded from macro 'module_driver'
module_init(__driver##_init); \
^
<scratch space>:73:1: note: expanded from here
v4l2_i2c_driver_init
^
>> drivers/media/i2c/s5k4ecgx.c:958:1: error: function definition is not allowed here
include/linux/i2c.h:950:2: note: expanded from macro 'module_i2c_driver'
module_driver(__i2c_driver, i2c_add_driver, \
^
include/linux/device/driver.h:265:42: note: expanded from macro 'module_driver'
static void __exit __driver##_exit(void) \
^
>> drivers/media/i2c/s5k4ecgx.c:958:1: error: use of undeclared identifier 'v4l2_i2c_driver_exit'; did you mean 'v4l2_i2c_driver'?
include/linux/i2c.h:950:2: note: expanded from macro 'module_i2c_driver'
module_driver(__i2c_driver, i2c_add_driver, \
^
include/linux/device/driver.h:269:13: note: expanded from macro 'module_driver'
module_exit(__driver##_exit);
^
<scratch space>:89:1: note: expanded from here
v4l2_i2c_driver_exit
^
drivers/media/i2c/s5k4ecgx.c:949:26: note: 'v4l2_i2c_driver' declared here
static struct i2c_driver v4l2_i2c_driver = {
^
>> drivers/media/i2c/s5k4ecgx.c:958:1: error: initializing 'exitcall_t' (aka 'void (*)(void)') with an expression of incompatible type 'struct i2c_driver'
module_i2c_driver(v4l2_i2c_driver);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/i2c.h:950:2: note: expanded from macro 'module_i2c_driver'
module_driver(__i2c_driver, i2c_add_driver, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/device/driver.h:268:3: note: expanded from macro 'module_driver'
} \
^
include/linux/module.h:100:24: note: expanded from macro '\
module_exit'
#define module_exit(x) __exitcall(x);
^ ~
include/linux/init.h:299:20: note: expanded from macro '__exitcall'
static exitcall_t __exitcall_##fn __exit_call = fn
^ ~~
<scratch space>:90:1: note: expanded from here
__exitcall_v4l2_i2c_driver_exit
^
>> drivers/media/i2c/s5k4ecgx.c:964:36: error: expected '}'
MODULE_FIRMWARE(S5K4ECGX_FIRMWARE);
^
drivers/media/i2c/s5k4ecgx.c:856:1: note: to match this '{'
{
^
>> drivers/media/i2c/s5k4ecgx.c:949:26: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
static struct i2c_driver v4l2_i2c_driver = {
^
>> drivers/media/i2c/s5k4ecgx.c:895:8: error: use of undeclared label 'out_err'
goto out_err;
^
2 warnings and 10 errors generated.
vim +897 drivers/media/i2c/s5k4ecgx.c
853
854 static int s5k4ecgx_probe(struct i2c_client *client,
855 const struct i2c_device_id *id)
856 {
857 struct s5k4ecgx_platform_data *pdata = client->dev.platform_data;
858 struct v4l2_subdev *sd;
859 struct s5k4ecgx *priv;
860 int ret, i;
861
862 if (pdata == NULL) {
863 dev_err(&client->dev, "platform data is missing!\n");
864 return -EINVAL;
865 }
866
867 priv = devm_kzalloc(&client->dev, sizeof(struct s5k4ecgx), GFP_KERNEL);
868 if (!priv)
869 return -ENOMEM;
870
871 mutex_init(&priv->lock);
872 priv->streaming = 0;
873
874 sd = &priv->sd;
875 /* Registering subdev */
876 v4l2_i2c_subdev_init(sd, client, &s5k4ecgx_ops);
877 /* Static name; NEVER use in new drivers! */
878 strscpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
879
880 sd->internal_ops = &s5k4ecgx_subdev_internal_ops;
881 /* Support v4l2 sub-device user space API */
882 sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
883
884 priv->pad.flags = MEDIA_PAD_FL_SOURCE;
885 sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
886 ret = media_entity_pads_init(&sd->entity, 1, &priv->pad);
887 if (ret)
888 return ret;
889
890 /* Request GPIO lines asserted */
891 priv->stby = devm_gpiod_get(&client->dev, "standby", GPIOD_OUT_HIGH);
892 if (IS_ERR(priv->stby)) {
893 v4l2_err(sd, "failed to request gpio S5K4ECGX_STBY");
894 ret = PTR_ERR(priv->stby);
> 895 goto out_err;
896 }
> 897 gpiod_set_consumer_name(priv->stby, "S5K4ECGX_STBY);
898 priv->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
899 if (IS_ERR(priv->reset)) {
900 v4l2_err(sd, "failed to request gpio S5K4ECGX_RST");
901 ret = PTR_ERR(priv->reset);
902 goto out_err;
903 }
904 gpiod_set_consumer_name(priv->reset, "S5K4ECGX_RST");
905
906 for (i = 0; i < S5K4ECGX_NUM_SUPPLIES; i++)
907 priv->supplies[i].supply = s5k4ecgx_supply_names[i];
908
909 ret = devm_regulator_bulk_get(&client->dev, S5K4ECGX_NUM_SUPPLIES,
910 priv->supplies);
911 if (ret) {
912 dev_err(&client->dev, "Failed to get regulators\n");
913 goto out_err;
914 }
915 ret = s5k4ecgx_init_v4l2_ctrls(priv);
916 if (ret)
917 goto out_err;
918
919 priv->curr_pixfmt = &s5k4ecgx_formats[0];
920 priv->curr_frmsize = &s5k4ecgx_prev_sizes[0];
921
922 return 0;
923
924 out_err:
925 media_entity_cleanup(&priv->sd.entity);
926
927 return ret;
928 }
929
930 static int s5k4ecgx_remove(struct i2c_client *client)
931 {
932 struct v4l2_subdev *sd = i2c_get_clientdata(client);
933 struct s5k4ecgx *priv = to_s5k4ecgx(sd);
934
935 mutex_destroy(&priv->lock);
936 v4l2_device_unregister_subdev(sd);
937 v4l2_ctrl_handler_free(&priv->handler);
938 media_entity_cleanup(&sd->entity);
939
940 return 0;
941 }
942
943 static const struct i2c_device_id s5k4ecgx_id[] = {
944 { S5K4ECGX_DRIVER_NAME, 0 },
945 {}
946 };
947 MODULE_DEVICE_TABLE(i2c, s5k4ecgx_id);
948
> 949 static struct i2c_driver v4l2_i2c_driver = {
950 .driver = {
951 .name = S5K4ECGX_DRIVER_NAME,
952 },
953 .probe = s5k4ecgx_probe,
> 954 .remove = s5k4ecgx_remove,
> 955 .id_table = s5k4ecgx_id,
956 };
957
> 958 module_i2c_driver(v4l2_i2c_driver);
959
960 MODULE_DESCRIPTION("Samsung S5K4ECGX 5MP SOC camera");
961 MODULE_AUTHOR("Sangwook Lee <sangwook.lee@linaro.org>");
962 MODULE_AUTHOR("Seok-Young Jang <quartz.jang@samsung.com>");
963 MODULE_LICENSE("GPL");
> 964 MODULE_FIRMWARE(S5K4ECGX_FIRMWARE);
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply
* Re: [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM
From: Jacopo Mondi @ 2022-05-31 13:16 UTC (permalink / raw)
To: Quentin Schulz
Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
devicetree, linux-kernel, Quentin Schulz
In-Reply-To: <20220525145833.1165437-2-foss+kernel@0leil.net>
Hi Quentin,
one more question
On Wed, May 25, 2022 at 04:58:31PM +0200, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> Until now, this driver only supported ACPI. This adds support for
> Device Tree too while enabling clock and regulators in runtime PM.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> v5:
> - fixed -Wdeclaration-after-statement for delay_us,
>
> v4:
> - added delays based on clock cycles as specified in datasheet for
> pre-power-off and post-power-on,
> - re-arranged clk handling, shutdown toggling and regulator handling to
> better match power up/down sequence defined in datasheet,
> - added comment on need for regulator being stable before releasing
> shutdown pin,
>
> v3:
> - added linux/mod_devicetable.h include,
> - moved delay for reset pulse right after the regulators are enabled,
> - removed check on is_acpi_node in favor of checks on presence of OF
> properties (e.g. devm_clk_get_optional returns NULL),
> - moved power management out of system suspend/resume into runtime PM
> callbacks,
> - removed ACPI specific comment since it's not specific to this driver,
> - changed devm_clk_get to devm_clk_get_optional,
> - remove OF use of clock-frequency (handled by devm_clk_get_optional
> directly),
> - removed name of clock (only one, so no need for anything explicit)
> when requesting a clock from OF,
> - wrapped lines to 80 chars,
>
> v2:
> - fixed unused-const-variable warning by removing of_match_ptr in
> of_match_table, reported by kernel test robot,
>
> drivers/media/i2c/ov5675.c | 149 +++++++++++++++++++++++++++++++------
> 1 file changed, 128 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 82ba9f56baec8..ea801edb8e408 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -3,10 +3,14 @@
>
> #include <asm/unaligned.h>
> #include <linux/acpi.h>
> +#include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/module.h>
> #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> @@ -17,7 +21,7 @@
>
> #define OV5675_LINK_FREQ_450MHZ 450000000ULL
> #define OV5675_SCLK 90000000LL
> -#define OV5675_MCLK 19200000
> +#define OV5675_XVCLK_19_2 19200000
> #define OV5675_DATA_LANES 2
> #define OV5675_RGB_DEPTH 10
>
> @@ -76,6 +80,14 @@
>
> #define to_ov5675(_sd) container_of(_sd, struct ov5675, sd)
>
> +static const char * const ov5675_supply_names[] = {
> + "avdd", /* Analog power */
> + "dovdd", /* Digital I/O power */
> + "dvdd", /* Digital core power */
> +};
> +
> +#define OV5675_NUM_SUPPLIES ARRAY_SIZE(ov5675_supply_names)
> +
> enum {
> OV5675_LINK_FREQ_900MBPS,
> };
> @@ -484,6 +496,9 @@ struct ov5675 {
> struct v4l2_subdev sd;
> struct media_pad pad;
> struct v4l2_ctrl_handler ctrl_handler;
> + struct clk *xvclk;
> + struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES];
>
> /* V4L2 Controls */
> struct v4l2_ctrl *link_freq;
> @@ -944,6 +959,56 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
> return ret;
> }
>
> +static int ov5675_power_off(struct device *dev)
Does this (and power_on) require __maybe_unused to avoid a warning
when compiling without CONFIG_PM support ? Have you tried that ?
Thanks
j
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ov5675 *ov5675 = to_ov5675(sd);
> + /* 512 xvclk cycles after the last SCCB transation or MIPI frame end */
> + u32 delay_us = DIV_ROUND_UP(512, OV5675_XVCLK_19_2 / 1000 / 1000);
> +
> + usleep_range(delay_us, delay_us * 2);
> +
> + clk_disable_unprepare(ov5675->xvclk);
> + gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> + regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> +
> + return 0;
> +}
> +
> +static int ov5675_power_on(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ov5675 *ov5675 = to_ov5675(sd);
> + u32 delay_us = DIV_ROUND_UP(8192, OV5675_XVCLK_19_2 / 1000 / 1000);
> + int ret;
> +
> + ret = clk_prepare_enable(ov5675->xvclk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable xvclk: %d\n", ret);
> + return ret;
> + }
> +
> + gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> +
> + ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> + if (ret) {
> + clk_disable_unprepare(ov5675->xvclk);
> + return ret;
> + }
> +
> + /* Reset pulse should be at least 2ms and reset gpio released only once
> + * regulators are stable.
> + */
> + usleep_range(2000, 2200);
> +
> + gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
> +
> + /* 8192 xvclk cycles prior to the first SCCB transation */
> + usleep_range(delay_us, delay_us * 2);
> +
> + return 0;
> +}
> +
> static int __maybe_unused ov5675_suspend(struct device *dev)
> {
> struct v4l2_subdev *sd = dev_get_drvdata(dev);
> @@ -1106,32 +1171,60 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = {
> .open = ov5675_open,
> };
>
> -static int ov5675_check_hwcfg(struct device *dev)
> +static int ov5675_get_hwcfg(struct ov5675 *ov5675, struct device *dev)
> {
> struct fwnode_handle *ep;
> struct fwnode_handle *fwnode = dev_fwnode(dev);
> struct v4l2_fwnode_endpoint bus_cfg = {
> .bus_type = V4L2_MBUS_CSI2_DPHY
> };
> - u32 mclk;
> + u32 xvclk_rate;
> int ret;
> unsigned int i, j;
>
> if (!fwnode)
> return -ENXIO;
>
> - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> + ov5675->xvclk = devm_clk_get_optional(dev, NULL);
> + if (IS_ERR(ov5675->xvclk))
> + return dev_err_probe(dev, PTR_ERR(ov5675->xvclk),
> + "failed to get xvclk: %ld\n",
> + PTR_ERR(ov5675->xvclk));
>
> - if (ret) {
> - dev_err(dev, "can't get clock frequency");
> - return ret;
> + if (ov5675->xvclk) {
> + xvclk_rate = clk_get_rate(ov5675->xvclk);
> + } else {
> + ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> + &xvclk_rate);
> +
> + if (ret) {
> + dev_err(dev, "can't get clock frequency");
> + return ret;
> + }
> }
>
> - if (mclk != OV5675_MCLK) {
> - dev_err(dev, "external clock %d is not supported", mclk);
> + if (xvclk_rate != OV5675_XVCLK_19_2) {
> + dev_err(dev, "external clock rate %u is unsupported",
> + xvclk_rate);
> return -EINVAL;
> }
>
> + ov5675->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(ov5675->reset_gpio)) {
> + ret = PTR_ERR(ov5675->reset_gpio);
> + dev_err(dev, "failed to get reset-gpios: %d\n", ret);
> + return ret;
> + }
> +
> + for (i = 0; i < OV5675_NUM_SUPPLIES; i++)
> + ov5675->supplies[i].supply = ov5675_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(dev, OV5675_NUM_SUPPLIES,
> + ov5675->supplies);
> + if (ret)
> + return ret;
> +
> ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> if (!ep)
> return -ENXIO;
> @@ -1186,6 +1279,9 @@ static int ov5675_remove(struct i2c_client *client)
> pm_runtime_disable(&client->dev);
> mutex_destroy(&ov5675->mutex);
>
> + if (!pm_runtime_status_suspended(&client->dev))
> + ov5675_power_off(&client->dev);
> +
> return 0;
> }
>
> @@ -1195,25 +1291,31 @@ static int ov5675_probe(struct i2c_client *client)
> bool full_power;
> int ret;
>
> - ret = ov5675_check_hwcfg(&client->dev);
> + ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> + if (!ov5675)
> + return -ENOMEM;
> +
> + ret = ov5675_get_hwcfg(ov5675, &client->dev);
> if (ret) {
> - dev_err(&client->dev, "failed to check HW configuration: %d",
> + dev_err(&client->dev, "failed to get HW configuration: %d",
> ret);
> return ret;
> }
>
> - ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> - if (!ov5675)
> - return -ENOMEM;
> -
> v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops);
>
> + ret = ov5675_power_on(&client->dev);
> + if (ret) {
> + dev_err(&client->dev, "failed to power on: %d\n", ret);
> + return ret;
> + }
> +
> full_power = acpi_dev_state_d0(&client->dev);
> if (full_power) {
> ret = ov5675_identify_module(ov5675);
> if (ret) {
> dev_err(&client->dev, "failed to find sensor: %d", ret);
> - return ret;
> + goto probe_power_off;
> }
> }
>
> @@ -1243,11 +1345,6 @@ static int ov5675_probe(struct i2c_client *client)
> goto probe_error_media_entity_cleanup;
> }
>
> - /*
> - * Device is already turned on by i2c-core with ACPI domain PM.
> - * Enable runtime PM and turn off the device.
> - */
> -
> /* Set the device's state to active if it's in D0 state. */
> if (full_power)
> pm_runtime_set_active(&client->dev);
> @@ -1262,12 +1359,15 @@ static int ov5675_probe(struct i2c_client *client)
> probe_error_v4l2_ctrl_handler_free:
> v4l2_ctrl_handler_free(ov5675->sd.ctrl_handler);
> mutex_destroy(&ov5675->mutex);
> +probe_power_off:
> + ov5675_power_off(&client->dev);
>
> return ret;
> }
>
> static const struct dev_pm_ops ov5675_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(ov5675_suspend, ov5675_resume)
> + SET_RUNTIME_PM_OPS(ov5675_power_off, ov5675_power_on, NULL)
> };
>
> #ifdef CONFIG_ACPI
> @@ -1279,11 +1379,18 @@ static const struct acpi_device_id ov5675_acpi_ids[] = {
> MODULE_DEVICE_TABLE(acpi, ov5675_acpi_ids);
> #endif
>
> +static const struct of_device_id ov5675_of_match[] = {
> + { .compatible = "ovti,ov5675", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ov5675_of_match);
> +
> static struct i2c_driver ov5675_i2c_driver = {
> .driver = {
> .name = "ov5675",
> .pm = &ov5675_pm_ops,
> .acpi_match_table = ACPI_PTR(ov5675_acpi_ids),
> + .of_match_table = ov5675_of_match,
> },
> .probe_new = ov5675_probe,
> .remove = ov5675_remove,
> --
> 2.36.1
>
^ permalink raw reply
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop
From: Jacopo Mondi @ 2022-05-31 13:14 UTC (permalink / raw)
To: Tommaso Merciai
Cc: linuxfancy, linux-amarula, michael, Shunqian Zheng,
Mauro Carvalho Chehab, linux-media, linux-kernel
In-Reply-To: <20220519075117.1003520-2-tommaso.merciai@amarulasolutions.com>
Hi Tommaso,
On Thu, May 19, 2022 at 09:51:14AM +0200, Tommaso Merciai wrote:
> Enable regulator using regulator_bulk_enable/regulatore_bulk_disable
> function in __ov5695_power_on/__ov5695_power_off function instead of for loop.
> This reduce code size and make things more clear
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
> drivers/media/i2c/ov5695.c | 25 +++++++------------------
> 1 file changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
> index 439385938a51..880b586e55fe 100644
> --- a/drivers/media/i2c/ov5695.c
> +++ b/drivers/media/i2c/ov5695.c
> @@ -972,7 +972,7 @@ static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
>
> static int __ov5695_power_on(struct ov5695 *ov5695)
> {
> - int i, ret;
> + int ret;
> struct device *dev = &ov5695->client->dev;
>
> ret = clk_prepare_enable(ov5695->xvclk);
> @@ -987,13 +987,10 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> * The hardware requires the regulators to be powered on in order,
> * so enable them one by one.
> */
The comment says that the hardware requires regulators to be enabled
in precise order
> - for (i = 0; i < OV5695_NUM_SUPPLIES; i++) {
> - ret = regulator_enable(ov5695->supplies[i].consumer);
> - if (ret) {
> - dev_err(dev, "Failed to enable %s: %d\n",
> - ov5695->supplies[i].supply, ret);
> - goto disable_reg_clk;
> - }
> + ret = regulator_bulk_enable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
bulk_enable() uses the async API (async_schedule_domain() in
particular) which by the name makes me think such ordering guarantee
cannot be respected.
However most sensors require some kind of ordering when enabling
regulators, and most of the use the bulk API anyhow. The fact this
driver uses the bulk API to get an release the regulators but not for
enabling them and the above comment, makes me think it has been done
on purpose ? Could you check with the driver author maybe ?
> + if (ret) {
> + dev_err(dev, "Failed to enable regulators %d\n", ret);
> + goto disable_reg_clk;
> }
>
> gpiod_set_value_cansleep(ov5695->reset_gpio, 0);
> @@ -1003,8 +1000,7 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> return 0;
>
> disable_reg_clk:
> - for (--i; i >= 0; i--)
> - regulator_disable(ov5695->supplies[i].consumer);
> + regulator_bulk_disable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
FYI the bulk API does this for you if enabling any of the regulators fails.
Hence this should not be necessary.
Thanks
j
> clk_disable_unprepare(ov5695->xvclk);
>
> return ret;
> @@ -1012,8 +1008,6 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
>
> static void __ov5695_power_off(struct ov5695 *ov5695)
> {
> - struct device *dev = &ov5695->client->dev;
> - int i, ret;
>
> clk_disable_unprepare(ov5695->xvclk);
> gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
> @@ -1022,12 +1016,7 @@ static void __ov5695_power_off(struct ov5695 *ov5695)
> * The hardware requires the regulators to be powered off in order,
> * so disable them one by one.
> */
> - for (i = OV5695_NUM_SUPPLIES - 1; i >= 0; i--) {
> - ret = regulator_disable(ov5695->supplies[i].consumer);
> - if (ret)
> - dev_err(dev, "Failed to disable %s: %d\n",
> - ov5695->supplies[i].supply, ret);
> - }
> + regulator_bulk_disable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
> }
>
> static int __maybe_unused ov5695_runtime_resume(struct device *dev)
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support
From: Jacopo Mondi @ 2022-05-31 12:45 UTC (permalink / raw)
To: Quentin Schulz
Cc: Quentin Schulz, shawnx.tu, mchehab, robh+dt,
krzysztof.kozlowski+dt, linux-media, devicetree, linux-kernel
In-Reply-To: <842dbd3c-856b-e5a8-e942-545ceb6741ca@theobroma-systems.com>
Hi Quentin
On Tue, May 31, 2022 at 02:19:21PM +0200, Quentin Schulz wrote:
> Hi Jacopo,
>
> On 5/31/22 12:50, Jacopo Mondi wrote:
> > Hi Quentin
> >
> > On Wed, May 25, 2022 at 04:58:33PM +0200, Quentin Schulz wrote:
> > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > >
> > > The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
> > > pixels and there are an additional 24 black rows "at the bottom".
> > >
> > > [2624]
> > > +-----+------------------+-----+
> > > | | 16 dummy | |
> > > +-----+------------------+-----+
> > > | | | |
> > > | | [2592] | |
> > > | | | |
> > > |16 | valid | 16 |[2000]
> > > |dummy| |dummy|
> > > | | [1944]| |
> > > | | | |
> > > +-----+------------------+-----+
> > > | | 16 dummy | |
> > > +-----+------------------+-----+
> > > | | 24 black lines | |
> > > +-----+------------------+-----+
> > >
> > > The top-left coordinate is gotten from the registers specified in the
> > > modes which are identical for both currently supported modes.
> > >
> > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > ---
> > >
> > > v4:
> > > - explicit a bit more the commit log,
> > > - added drawing in the commit log,
> > > - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> > >
> > > added in v3
> > >
> > > drivers/media/i2c/ov5675.c | 33 +++++++++++++++++++++++++++++++++
> > > 1 file changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > index c1f3c387afde0..384a9ea2372c3 100644
> > > --- a/drivers/media/i2c/ov5675.c
> > > +++ b/drivers/media/i2c/ov5675.c
> > > @@ -1121,6 +1121,38 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
> > > return 0;
> > > }
> > >
> > > +static int ov5675_get_selection(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_state *state,
> > > + struct v4l2_subdev_selection *sel)
> > > +{
> > > + struct ov5675 *ov5675 = to_ov5675(sd);
> > > +
> > > + if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > + return -EINVAL;
> > > +
> > > + switch (sel->target) {
> > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > + sel->r.top = 0;
> > > + sel->r.left = 0;
> > > + sel->r.width = 2624;
> > > + sel->r.height = 2000;
> > > + return 0;
> > > + case V4L2_SEL_TGT_CROP:
> > > + sel->r.top = 16;
> > > + sel->r.left = 16;
> > > + sel->r.width = ov5675->cur_mode->width;
> > > + sel->r.height = ov5675->cur_mode->height;
> > > + return 0;
> >
> > I'm afraid this doesn't match exactly my understanding of the
> > discussion we had.
> >
> > The driver defines the following modes
> >
> > /*
> > * OV5670 sensor supports following resolutions with full FOV:
> > * 4:3 ==> {2592x1944, 1296x972, 648x486}
> > * 16:9 ==> {2560x1440, 1280x720, 640x360}
> > */
> > static const struct ov5670_mode supported_modes[] = {
> > {
> > .width = 2592,
> > .height = 1944,
> > },
> > {
> > .width = 1296,
> > .height = 972,
> > },
> > {
> > .width = 648,
> > .height = 486,
> > },
> > {
> > .width = 2560,
> > .height = 1440,
> > },
> > {
> > .width = 1280,
> > .height = 720,
> > },
> > {
> > .width = 640,
> > .height = 360,
> > }
> > };
> >
> > The comment says all modes retain the "full FOV", which I assume it
> > implies they are obtained by sub-sampling and not cropping.
> >
> > The first three modes (4:3) are indeed obtained by subsampling the
> > full active pixel array:
> >
> > (2592,1944) / 2 = (1296,972) / 2 = (648,486)
> >
> > The last three are obtained by subsampling a slightly cropped portion
> > of the pixel array
> >
> > (2560,1440) / 2 = (1280,720) / 2 = (640,360)
> >
> > If you set CROP = cur_mode->[width/height] you will instead report the
> > visible width/height, which as said it's obtained by subsampling (of a
> > slightly cropped portion of the pixel array for the last three ones)
> >
> > The CROP rectangle is then (2592, 1944) for the first three and (2560,
> > 1440) for the last three.
> >
> > I would add a v4l2_rect to struct ov5670_mode where to record that and
> > report it here.
> >
>
> That makes a lot of sense to me, thanks for your patience and explanations.
>
> FYI, you're looking at the wrong driver (ov5670 vs ov5675; a mistake I make
You know what's depressing ? -I- have a series out for ov5670 :(
I'm so sorry, my brain got short-circuited by that probably...
> every now and then too :) ). However, the datasheet does say that "The
> OV5675 supports a binning mode to provide a lower resolution output while
> maintaining the field of view.[...] The OV5675 supports 2x2 binning." so I
> assume we're in the same scenario as you just explained.
>
> Since the OV5675 modes currently supported by the drivers are 4/3 only and
> the smaller size mode a result of subsampling, they both have the same CROP
> rectangle.
>
Thankfully the comment still applies to ov5675 then, and both modes
have the same (2592, 1944) crop rectangle :)
> > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > + sel->r.top = 16;
> > > + sel->r.left = 16;
> > > + sel->r.width = supported_modes[0].width;
> > > + sel->r.height = supported_modes[0].height;
> > > + return 0;
> >
> > You could also define these values instead of fishing in the
> > supported_modes array, to protect against future changes to the array
> > itself. Up to you.
> >
>
> Since there's no cropping involved in the current modes, I assume we could
> just hardcode the width and height and tackle this limitation later, once we
> add more modes or support for configuring cropping (this patch only adds the
> getter and not the setter).
Fine with me!
Sorry again for the slip!
>
> Cheers,
> Quentin
^ permalink raw reply
* Re: [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support
From: Quentin Schulz @ 2022-05-31 12:19 UTC (permalink / raw)
To: Jacopo Mondi, Quentin Schulz
Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
devicetree, linux-kernel
In-Reply-To: <20220531105011.yxrosmwtw3mpaomb@uno.localdomain>
Hi Jacopo,
On 5/31/22 12:50, Jacopo Mondi wrote:
> Hi Quentin
>
> On Wed, May 25, 2022 at 04:58:33PM +0200, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
>> pixels and there are an additional 24 black rows "at the bottom".
>>
>> [2624]
>> +-----+------------------+-----+
>> | | 16 dummy | |
>> +-----+------------------+-----+
>> | | | |
>> | | [2592] | |
>> | | | |
>> |16 | valid | 16 |[2000]
>> |dummy| |dummy|
>> | | [1944]| |
>> | | | |
>> +-----+------------------+-----+
>> | | 16 dummy | |
>> +-----+------------------+-----+
>> | | 24 black lines | |
>> +-----+------------------+-----+
>>
>> The top-left coordinate is gotten from the registers specified in the
>> modes which are identical for both currently supported modes.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>
>> v4:
>> - explicit a bit more the commit log,
>> - added drawing in the commit log,
>> - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
>>
>> added in v3
>>
>> drivers/media/i2c/ov5675.c | 33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>> index c1f3c387afde0..384a9ea2372c3 100644
>> --- a/drivers/media/i2c/ov5675.c
>> +++ b/drivers/media/i2c/ov5675.c
>> @@ -1121,6 +1121,38 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
>> return 0;
>> }
>>
>> +static int ov5675_get_selection(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_selection *sel)
>> +{
>> + struct ov5675 *ov5675 = to_ov5675(sd);
>> +
>> + if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
>> + return -EINVAL;
>> +
>> + switch (sel->target) {
>> + case V4L2_SEL_TGT_CROP_BOUNDS:
>> + sel->r.top = 0;
>> + sel->r.left = 0;
>> + sel->r.width = 2624;
>> + sel->r.height = 2000;
>> + return 0;
>> + case V4L2_SEL_TGT_CROP:
>> + sel->r.top = 16;
>> + sel->r.left = 16;
>> + sel->r.width = ov5675->cur_mode->width;
>> + sel->r.height = ov5675->cur_mode->height;
>> + return 0;
>
> I'm afraid this doesn't match exactly my understanding of the
> discussion we had.
>
> The driver defines the following modes
>
> /*
> * OV5670 sensor supports following resolutions with full FOV:
> * 4:3 ==> {2592x1944, 1296x972, 648x486}
> * 16:9 ==> {2560x1440, 1280x720, 640x360}
> */
> static const struct ov5670_mode supported_modes[] = {
> {
> .width = 2592,
> .height = 1944,
> },
> {
> .width = 1296,
> .height = 972,
> },
> {
> .width = 648,
> .height = 486,
> },
> {
> .width = 2560,
> .height = 1440,
> },
> {
> .width = 1280,
> .height = 720,
> },
> {
> .width = 640,
> .height = 360,
> }
> };
>
> The comment says all modes retain the "full FOV", which I assume it
> implies they are obtained by sub-sampling and not cropping.
>
> The first three modes (4:3) are indeed obtained by subsampling the
> full active pixel array:
>
> (2592,1944) / 2 = (1296,972) / 2 = (648,486)
>
> The last three are obtained by subsampling a slightly cropped portion
> of the pixel array
>
> (2560,1440) / 2 = (1280,720) / 2 = (640,360)
>
> If you set CROP = cur_mode->[width/height] you will instead report the
> visible width/height, which as said it's obtained by subsampling (of a
> slightly cropped portion of the pixel array for the last three ones)
>
> The CROP rectangle is then (2592, 1944) for the first three and (2560,
> 1440) for the last three.
>
> I would add a v4l2_rect to struct ov5670_mode where to record that and
> report it here.
>
That makes a lot of sense to me, thanks for your patience and explanations.
FYI, you're looking at the wrong driver (ov5670 vs ov5675; a mistake I
make every now and then too :) ). However, the datasheet does say that
"The OV5675 supports a binning mode to provide a lower resolution output
while maintaining the field of view.[...] The OV5675 supports 2x2
binning." so I assume we're in the same scenario as you just explained.
Since the OV5675 modes currently supported by the drivers are 4/3 only
and the smaller size mode a result of subsampling, they both have the
same CROP rectangle.
>> + case V4L2_SEL_TGT_CROP_DEFAULT:
>> + sel->r.top = 16;
>> + sel->r.left = 16;
>> + sel->r.width = supported_modes[0].width;
>> + sel->r.height = supported_modes[0].height;
>> + return 0;
>
> You could also define these values instead of fishing in the
> supported_modes array, to protect against future changes to the array
> itself. Up to you.
>
Since there's no cropping involved in the current modes, I assume we
could just hardcode the width and height and tackle this limitation
later, once we add more modes or support for configuring cropping (this
patch only adds the getter and not the setter).
Cheers,
Quentin
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox