Re: [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 28.04.22 um 09:32 schrieb Christian König:
Am 28.04.22 um 09:23 schrieb Thomas Zimmermann:
[SNIP]
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index a6d89aed0bda..8fc0b42acdff 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -1,6 +1,7 @@
  // SPDX-License-Identifier: GPL-2.0-or-later
    #include <linux/dma-resv.h>
+#include <linux/dma-fence-chain.h>
    #include <drm/drm_atomic_state_helper.h>
  #include <drm/drm_atomic_uapi.h>
@@ -141,25 +142,67 @@
   * See drm_atomic_set_fence_for_plane() for a discussion of implicit and

This comment still refers to the function you just deleted. Maybe the deleted docs could be integrated here somehow, if still relevant?

Yeah, Daniel point that out as well.


   * explicit fencing in atomic modeset updates.
   */
-int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state)
+int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
+                    struct drm_plane_state *state)

We have a 100-character limit. Please leave this on the same line.

Despite some efforts to change this that is still documented as 80-character limit: https://www.kernel.org/doc/html/v5.18-rc4/process/coding-style.html#breaking-long-lines-and-strings

But didn't checkpatch stop warning about the 80-char limit?




  {
+    struct dma_fence *fence = dma_fence_get(state->fence);
      struct drm_gem_object *obj;

I'd declare this variable within the for loop.

-    struct dma_fence *fence;
+    enum dma_resv_usage usage;
+    size_t i;
      int ret;
        if (!state->fb)
          return 0;
  -    obj = drm_gem_fb_get_obj(state->fb, 0);
-    ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, &fence);
-    if (ret)
-        return ret;
-
-    /* TODO: drm_atomic_set_fence_for_plane() should be changed to be able
-     * to handle more fences in general for multiple BOs per fb.
+    /*
+     * Only add the kernel fences here if there is already a fence set via
+     * explicit fencing interfaces on the atomic ioctl.
+     *
+     * This way explicit fencing can be used to overrule implicit fencing,
+     * which is important to make explicit fencing use-cases work: One
+     * example is using one buffer for 2 screens with different refresh
+     * rates. Implicit fencing will clamp rendering to the refresh rate of
+     * the slower screen, whereas explicit fence allows 2 independent
+     * render and display loops on a single buffer. If a driver allows
+     * obeys both implicit and explicit fences for plane updates, then it
+     * will break all the benefits of explicit fencing.
       */
-    drm_atomic_set_fence_for_plane(state, fence);
+    usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE;
+
+    for (i = 0; i < ARRAY_SIZE(state->fb->obj); ++i) {

Instead of ARRAY_SIZE, rather use state->fb->format->num_planes. It's the number of planes (i.e., GEM objects) in the framebuffer.

+        struct dma_fence *new;
+
+        obj = drm_gem_fb_get_obj(state->fb, i);
+        if (!obj)

With the use of num_planes in the for loop, there should probably be a drm_WARN_ON_ONCE() around this test.

+            continue;

goto error handling.

Or is there a use case for framebuffers with empty planes? At least it's not possible to instantiate one via drm_gem_fb_init_with_funcs().


I was asking myself the same thing, but found this handling be used at other places as well.

I've gradually changed the code towards the use of num_planes and stricter error reporting. IMHO at some point, we should warn about empty plane BOs directly within drm_gem_fb_get_obj().

Best regards
Thomas


Thanks for taking a look,
Christian.

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux