First batch of fixes for V4L2 camera driver #2609
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Sorry, hold fire for a bit. I'd missed an upstream patch that 9595ede significantly modifiies, so I'll backport and rebase the rest. |
commit dd9bb50 upstream. Interleaved RGB and single plane YUV formats can be delivered by the GPU without the secondary step of removing padding, as the bytesperline field can be set appropriately. Planar YUV needs the GPU to still remove padding, as there is no way to report that there is padding between the planes (ie on the height). The multi-planar formats are NOT applicable, as there is no easy way to make them contiguous in memory (ie one large allocation that gets broken up). The whole task is passed across to videobuf2 which has no notion of that requirement. v2: Changes by anholt from the downstream driver: Flag two more planar formats as needing padding removal, and remove broken userspace workaround. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 96b7e81 upstream. The struct mmal_msg_context was being allocated for every message being sent to the VPU, and freed when it came back. Whilst that is required behaviour for some messages (mainly the synchronous ones), it is wasteful for the video buffers that make up the majority of the traffic. Add to the buffer_init/cleanup hooks that it allocates/frees the msg_context required. v2: changes by anholt from the downstream tree: clean up indentation, pass an error value through, forward-declare the struct so we have less void * Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 71fcbc4 upstream. There is no requirement to serialise bulk transfers as that is all done in VCHI, and if a second MMAL_MSG_TYPE_BUFFER_TO_HOST happened before the VCHI_CALLBACK_BULK_RECEIVED, then the service_callback thread is deadlocked. Remove the bulk_mutex so that multiple receives can be scheduled at a time. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 7cc31d5 upstream. For historical reasons, the number of buffers passed to the VPU over MMAL did not match that passed from V4L2. That is a silly situation as the driver has to duplicate serialisation and other functions that have already been implemented in V4L2/videobuf2. As we had more V4L2 buffers than MMAL ones, the MMAL buffer headers were returned to the VPU immediately on being filled, which is now invalid. Match the number of buffers notified in queue_setup with that used in MMAL. Return buffers only when we get them from V4L2. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 9384167 upstream The MMAL and V4L2 buffers had been disassociated, and linked on demand. Seeing as both are finite and low in number, and we now have the same number of each, link them for the duration. This removes the complexity of maintaining lists as the struct mmal_buffer context comes back from the VPU, so we can directly link back to the relevant V4L2 buffer. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 514a6ab upstream. mmal-parameters.h didn't have the normal ... protection to stop it being included multiple times. Add it. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 84adcb1 upstream. struct vchiq_mmal_rect is only referenced from mmal-parameters.h, yet was defined in mmal-vchiq.h. Move it to avoid having to include multiple headers for no reason. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 84db34c upstream. The error conditions don't warrant taking the kernel down, so remove BUG_ON. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit a9e1481 upstream. Fix a typo flagged by checkpatch, and another in the same line. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 6166045 upstream As requested by Mauro Carvalho Chehab in review. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 40b73e1 upstream. The v4l2 uapi uses u8[] for strings, so cast those to char * to avoid compiler warnings about unsigned vs signed with sprintf() and friends. Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit aa4f227 upstream. Fixes a compiler warning about a set-but-not-used variable. I think this was just leftover dead code from before set_framerate_params(), since that also sets up some mmal_parameter_rational structs for fps. Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
…ngs. commit 9dabe66 upstream The arg is a u32 *, so switch over to that in our declarations. Signed-off-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 1e85394 upstream. This patch fixes the checkpatch.pl check hint: CHECK: Prefer using the BIT_ULL macro Signed-off-by: Kilian Köppchen <kiliankoeppchen@googlemail.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 0723103 upstream When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this. Clean up the vchiq_arm code by not caring about the value of debugfs calls. This ends up removing a number of lines of code that are not needed. Cc: Stefan Wahren <stefan.wahren@i2se.com> Cc: Kees Cook <keescook@chromium.org> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Keerthi Reddy <keerthigd4990@gmail.com> Cc: linux-rpi-kernel@lists.infradead.org Cc: linux-arm-kernel@lists.infradead.org Reviewed-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 2739dea upstream vchiq_debugfs_top() is only a wrapper around a pointer to a dentry, so just use the dentry directly instead, making it a static variable instead of part of a static structure. This also removes the pointless BUG_ON() when checking that dentry as no one should ever care if debugfs is working or not, and the kernel should really not panic over something as trivial as that. Suggested-by: Eric Anholt <eric@anholt.net> Cc: Stefan Wahren <stefan.wahren@i2se.com> Cc: Kees Cook <keescook@chromium.org> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Keerthi Reddy <keerthigd4990@gmail.com> Cc: linux-rpi-kernel@lists.infradead.org Cc: linux-arm-kernel@lists.infradead.org Reviewed-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 24e8d3f upstream This does not need to be part of a wrapper function, or in a structure, just properly reference it directly as a single variable. The whole variable will be going away soon anyway, this is just a step toward that direction. Suggested-by: Eric Anholt <eric@anholt.net> Cc: Stefan Wahren <stefan.wahren@i2se.com> Cc: Kees Cook <keescook@chromium.org> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Keerthi Reddy <keerthigd4990@gmail.com> Cc: linux-rpi-kernel@lists.infradead.org Cc: linux-arm-kernel@lists.infradead.org Reviewed-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
6by9
force-pushed
the
rpi-4.14.y-codecs-push-pt1
branch
from
efa123d
to
18a401d
Compare
|
All good now, and also rebased. |
|
Our other backports all use "commit upstream." as the first line in the commit message. I'll review the content separately. |
|
It looks like a mix of "commit hash upstream" and "Upstream commit hash". Ah, I have missed it on the Genki Sky's patches that I backported in the second pass. I'll amend. |
commit 127892f upstream This structure, and the one static variable that was declared with it, were not being used for anything. The log_categories field was being set, but never used again. So just remove it entirely as it is not needed at all. Suggested-by: Eric Anholt <eric@anholt.net> Cc: Stefan Wahren <stefan.wahren@i2se.com> Cc: Kees Cook <keescook@chromium.org> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Keerthi Reddy <keerthigd4990@gmail.com> Cc: linux-rpi-kernel@lists.infradead.org Cc: linux-arm-kernel@lists.infradead.org Reviewed-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
pelwell
reviewed
Jul 23, 2018
| struct work_struct work; | ||
| /* work struct for defered callback */ |
| @@ -1934,6 +1945,9 @@ int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance) | |||
|
|
|||
| params.callback_param = instance; | |||
|
|
|||
| instance->bulk_wq = alloc_ordered_workqueue("mmal-vchiq", | |||
| WQ_MEM_RECLAIM); | |||
There was a problem hiding this comment.
Should an allocation failure be acted upon?
| @@ -1949,6 +1963,8 @@ int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance) | |||
|
|
|||
| err_close_services: | |||
|
|
|||
| if (instance->bulk_wq) | |||
There was a problem hiding this comment.
Depending on the answer to an earlier question you may want to remove this condition.
| @@ -657,6 +652,21 @@ static void stop_streaming(struct vb2_queue *vq) | |||
| ret); | |||
| } | |||
|
|
|||
| /* wait for all buffers to be returned */ | |||
| while (atomic_read(&port->buffers_with_vpu)) { | |||
There was a problem hiding this comment.
Are atomic operations known to work on BCM283x?
There was a problem hiding this comment.
I believe they do, otherwise huge chunks of the kernel relying on it are going to fail.
commit 54f1569 upstream There's no need to set this to be int * when it is only used as a void *. This lets us remove the unneeded cast, and unneeded temporary variable the one place it is referenced in the code. Suggested-by: Eric Anholt <eric@anholt.net> Cc: Stefan Wahren <stefan.wahren@i2se.com> Cc: Kees Cook <keescook@chromium.org> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Keerthi Reddy <keerthigd4990@gmail.com> Cc: linux-rpi-kernel@lists.infradead.org Cc: linux-arm-kernel@lists.infradead.org Reviewed-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 3b93c0f upstream The log entry dentries are only set, never referenced, so no need to keep them around. Remove the pointer from struct vchiq_debugfs_log_entry as it is not needed anymore and get rid of the separate vchiq_debugfs_create_log_entries() function as it is only used in one place. Suggested-by: Eric Anholt <eric@anholt.net> Cc: Stefan Wahren <stefan.wahren@i2se.com> Cc: Kees Cook <keescook@chromium.org> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Keerthi Reddy <keerthigd4990@gmail.com> Cc: linux-rpi-kernel@lists.infradead.org Cc: linux-arm-kernel@lists.infradead.org Reviewed-by: Eric Anholt <eric@anholt.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 7260ea5 upstream This was found using checkpatch.pl's MULTILINE_DEREFERENCE warning. Putting the dereference onto one line makes them easier to read, especially when part of a larger expression (in this case, function arguments and ternary operator), and when the dereferences are short (as they are here). Signed-off-by: Genki Sky <sky@genki.is> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 6cf83f2 upstream with minor mods struct timeval is deprecated for in-kernel use, and converting this function to use ktime_t makes it simpler as well. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Commit 5ced5a899b405e09cc681b8a1a449815b66744c7 upstream. This was found using checkpatch.pl's SPLIT_STRING warning. While joining these strings makes for long lines, the kernel codebase consistently does it this way to make user-visible strings easier to grep for. Signed-off-by: Genki Sky <sky@genki.is> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Commit b891f25d80d16e314dc191f019c4e346e41bfb36 upstream. This was found using checkpatch.pl's EMBEDDED_FUNCTION_NAME warning. It is easier to be consistent and always use __func__ instead of having to remember to update any hardcoded references to the original name. Signed-off-by: Genki Sky <sky@genki.is> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
vchi_bulk_queue_receive will queue up to a default of 4 bulk receives on a connection before blocking. If called from the VCHI service_callback thread, then that thread is unable to service the VCHI_CALLBACK_BULK_RECEIVED events that would enable the queue call to succeed. Add a workqueue to schedule the call vchi_bulk_queue_receive in an alternate context to avoid the lock up. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Fix several instances where it is easier to return early on error conditions than handle it as an else clause. As requested by Mauro. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
None of the listed author email addresses were valid. Keep list of authors and the companies they represented. Update my email address. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Fix comment style violations in the header files. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Fix checkpatch warnings over spaces around operators. Many were around operations that can be replaced with the BIT(x) macro, so replace with that where appropriate. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
We have numerous lines over 80 chars, or oddly split. Many of these are due to using long enum names such as MMAL_COMPONENT_CAMERA. Reduce the length of these enum names. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Fix checkpatch errors "Avoid multiple line dereference" Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Fix mismatched or missing brace issues flagged by checkpatch. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Fix checkpatch errors for missing blank lines after variable or structure declarations. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Fix checkpatch errors for "Logical continuations should be on the previous line". Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Fix checkpatch "Alignment should match open parenthesis" errors. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Fix a typo flagged up by checkpatch. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
With the recent change to match MMAL and V4L2 buffers there is a need to wait for all MMAL buffers to be returned during stop_streaming. Fixes: 9384167 "staging: bcm2835-camera: Remove V4L2/MMAL buffer remapping" Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Before 9384167 there was a need to ensure that there were sufficient buffers supplied from the user to cover those being sent to the VPU (always 1). With 9384167 the buffers are linked 1:1 between MMAL and V4L2, therefore there is no need for that check, and indeed it is wrong as there is no need to submit all the buffers before starting streaming. Fixes: 9384167 "staging: bcm2835-camera: Remove V4L2/MMAL buffer remapping" Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
The change to mapping V4L2 to MMAL buffers 1:1 didn't handle the condition we get with raw pixel buffers (eg YUV and RGB) direct from the camera's stills port. That sends the pixel buffer and then an empty buffer with the EOS flag set. The EOS buffer wasn't handled and returned an error up the stack. Handle the condition correctly by returning it to the component if streaming, or returning with an error if stopping streaming. Fixes: 9384167 "staging: bcm2835-camera: Remove V4L2/MMAL buffer remapping" Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Set the sequence number in vb2_v4l2_buffer mainly so the latest v4l2-ctl reports the frame rate correctly. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
There is an awkward situation with H264 header bytes. Currently they are returned with a PTS of 0 because they aren't associated with a timestamped frame to encode. These are handled by either returning the timestamp of the last buffer to have been received, or in the case of the first buffer the timestamp taken at start_streaming. This results in a race where the current frame may have started before we take the start time, which results in the first encoded frame having an earlier timestamp than the header bytes. Ensure that we never return a negative delta to the user by checking against the previous timestamp. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
6by9
force-pushed
the
rpi-4.14.y-codecs-push-pt1
branch
from
18a401d
to
6f270eb
Compare
|
That's hopefully addressed the review comments. |
|
@pelwell Any further review comments, or can this be merged? I've got all the V4L2 codec changes waiting, but wanted to get this framework stuff sorted first. |
popcornmix
added a commit
to raspberrypi/firmware
that referenced
this pull request
Aug 24, 2018
kernel: bcm2835: interpolate audio delay See: #1026 kernel: dtoverlays: Add support for ADV7280-M and ADV7281-M chips See: raspberrypi/linux#2656 kernel: First batch of fixes for V4L2 camera driver See: raspberrypi/linux#2609 kernel: Revert mm: alloc_contig: re-allow CMA to compact FS pages See: raspberrypi/linux#2647 firmware: rawcam: Fix double buffer return issue firmware: rawcam: Code cleanup firmware: host_apps: Fixup partially merged commit from userland See: #1027 firmware: mmal: Add KEEP_PORT_FORMATS flag to mmal connection See: raspberrypi/userland#483 firmware: RaspiStill: Apply gpsd info as EXIF tags See: raspberrypi/userland#286
popcornmix
added a commit
to Hexxeh/rpi-firmware
that referenced
this pull request
Aug 24, 2018
kernel: bcm2835: interpolate audio delay See: raspberrypi/firmware#1026 kernel: dtoverlays: Add support for ADV7280-M and ADV7281-M chips See: raspberrypi/linux#2656 kernel: First batch of fixes for V4L2 camera driver See: raspberrypi/linux#2609 kernel: Revert mm: alloc_contig: re-allow CMA to compact FS pages See: raspberrypi/linux#2647 firmware: rawcam: Fix double buffer return issue firmware: rawcam: Code cleanup firmware: host_apps: Fixup partially merged commit from userland See: raspberrypi/firmware#1027 firmware: mmal: Add KEEP_PORT_FORMATS flag to mmal connection See: raspberrypi/userland#483 firmware: RaspiStill: Apply gpsd info as EXIF tags See: raspberrypi/userland#286
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first 20 patches are backports from upstream stuff merged in 4.18. The only bits I haven't merged from upstream are the bits related to switching to being a platform driver as I can't get those to work properly.
The rest are mainly checkpatch cleanups, and four fixes to the backports. I'm about to throw them all at staging-next.