New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

khronos: Use vchiq_get_client_id to determine global PID. #315

Merged
merged 1 commit into from Jun 27, 2016

Conversation

Projects
None yet
4 participants
@lorenzo-stoakes
Contributor

lorenzo-stoakes commented May 18, 2016

PID namespacing breaks VCHIQ as it uses PIDs as unique identifiers for VCHIQ services, meaning messages sent from userland will contain a different ID than the one expected by the GPU.

This patch addresses the problem uses the existing GET_CLIENT_ID ioctl (via vchiq_get_client_id()) to retrieve the global PID as seen by the kernel rather than using the process's reported PID.

The main situation that causes this problem to occur is when trying to run code that interfaces with the GPU inside of a container (e.g. via EGL.)

I'm not certain this is the best means of achieving this aim, however I definitely think the fix belongs in the userland tools, as the kernel alternative is to either use the namespaced PID, which opens the door to conflicts (see raspberrypi/linux#1279) or to actively intercept messages and manually determine which part of the raw data contains the ID and replace it, which is potentially unreliable and adds overhead (see raspberrypi/linux#1382 - yes we've looked at this multiple different ways :)

I've tested this change against EGL applications and it works correctly with no noticeable issues, though of course it's hard to test all cases :)

I'd really love to get some feedback on this in case there is something I've missed or got wrong here. My main concerns in the approach I've taken here are:

  1. Correctness - Am I missing something here that renders this an invalid approach?
  2. Completeness - There are other parts of the code that reference PID, but khronos_platform_get_process_id() seems to be the main source of the PID namespacing issue. Perhaps more code needs adjustment?
  3. Performance - this patch causes an ioctl to be issued to determine the client ID when previously a getpid() was used, potentially introducing performance degradation. However, it does seem to only be needed at points where a message would be sent anyway so perhaps relatively this isn't that much overhead. This can potentially be mitigated by vchiq_get_client_id() using a cached client ID value instead of issuing the ioctl itself.

Thanks in advance for taking a look at this!

khronos: Use vchiq_get_client_id to determine global PID.
PID namespacing breaks VCHIQ as it uses PIDs as unique identifiers for VCHIQ
services, meaning messages sent from userland will contain a different ID than
the one expected by the GPU.

This patch addresses the problem uses the existing GET_CLIENT_ID ioctl to
retrieve the global PID as seen by the kernel rather than using the process's
reported PID.
@Ruffio

This comment has been minimized.

Ruffio commented Jun 27, 2016

Can this PR be accepted?

@pelwell

This comment has been minimized.

Contributor

pelwell commented Jun 27, 2016

I'm happy with the motivation behind the patch, and I think the implementation is correct. My hesitation has been on the grounds of a potential performance hit, but that doesn't seem to be a problem; if anything it seems marginally quicker:

Without:
1260 frames 17.4 seconds 72.6 fps 5.0/13.8/131.0/5.7 ms
1260 frames 17.1 seconds 73.8 fps 5.0/13.6/36.0/4.5 ms
1260 frames 17.2 seconds 73.4 fps 6.0/13.6/37.0/4.5 ms
1260 frames 17.1 seconds 73.7 fps 6.0/13.6/35.0/4.6 ms


With:
1260 frames 17.2 seconds 73.2 fps 5.0/13.7/133.0/5.7 ms
1260 frames 17.1 seconds 73.9 fps 5.0/13.5/36.0/4.5 ms
1260 frames 17.1 seconds 73.8 fps 6.0/13.6/36.0/4.5 ms
1260 frames 17.1 seconds 73.8 fps 5.0/13.5/36.0/4.5 ms

I'm happy for this to be merged - @popcornmix ?

@popcornmix popcornmix merged commit 1cd5205 into raspberrypi:master Jun 27, 2016

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Jun 27, 2016

firmware: image_fx: Allow dimensions to be changed without triggering…
… a size error

firmware: image_fx: Avoid setting the vpitch for YUVUV buffers
firmware: image_fx: Add YUVUV as supported type
firmware: image_fx: relax check on inout/output formats to support standalone deinterlace

firmware: h264_parse: Silently discard macroblocks with unsupported profile
See: #549

userland: MMAL_VC_SHM: Explicitly lock buffer when requested
See: raspberrypi/userland@dd9636d

userland: khronos: Use vchiq_get_client_id to determine global PID
See: raspberrypi/userland#315

userland: various warning fixes

popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Jun 27, 2016

firmware: image_fx: Allow dimensions to be changed without triggering…
… a size error

firmware: image_fx: Avoid setting the vpitch for YUVUV buffers
firmware: image_fx: Add YUVUV as supported type
firmware: image_fx: relax check on inout/output formats to support standalone deinterlace

firmware: h264_parse: Silently discard macroblocks with unsupported profile
See: raspberrypi/firmware#549

userland: MMAL_VC_SHM: Explicitly lock buffer when requested
See: raspberrypi/userland@dd9636d

userland: khronos: Use vchiq_get_client_id to determine global PID
See: raspberrypi/userland#315

userland: various warning fixes

neuschaefer pushed a commit to neuschaefer/raspi-binary-firmware that referenced this pull request Feb 27, 2017

firmware: image_fx: Allow dimensions to be changed without triggering…
… a size error

firmware: image_fx: Avoid setting the vpitch for YUVUV buffers
firmware: image_fx: Add YUVUV as supported type
firmware: image_fx: relax check on inout/output formats to support standalone deinterlace

firmware: h264_parse: Silently discard macroblocks with unsupported profile
See: raspberrypi#549

userland: MMAL_VC_SHM: Explicitly lock buffer when requested
See: raspberrypi/userland@dd9636d

userland: khronos: Use vchiq_get_client_id to determine global PID
See: raspberrypi/userland#315

userland: various warning fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment