Re: [RFC v4 1/3] drm/fence: add in-fences support

Hi Maarten,

2016-09-01 Maarten Lankhorst maarten.lankhorst@linux.intel.com:

Op 31-08-16 om 21:07 schreef Gustavo Padovan:

...

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

There is now a new property called FENCE_FD attached to every plane state that receives the sync_file fd from userspace via the atomic commit IOCTL.

The fd is then translated to a fence (that may be a fence_collection subclass or just a normal fence) and then used by DRM to fence_wait() for all fences in the sync_file to signal. So it only commits when all framebuffers are ready to scanout.

v2: Comments by Daniel Vetter:

• remove set state->fence = NULL in destroy phase
• accept fence -1 as valid and just return 0
• do not call fence_get() - sync_file_fences_get() already calls it
• fence_put() if state->fence is already set, in case userspace

set the property more than once.

v3: WARN_ON if fence is set but state has no FB

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 3 +++ drivers/gpu/drm/drm_crtc.c | 7 +++++++ include/drm/drm_crtc.h | 4 ++++ 5 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index c02be6a..07f9c60 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -12,6 +12,7 @@ menuconfig DRM select I2C select I2C_ALGOBIT select DMA_SHARED_BUFFER

• select SYNC_FILE help Kernel-level support for the Direct Rendering Infrastructure (DRI) introduced in XFree86 4.0. If you say Y here, you need to select

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 5cb2e22..9e6c4e7 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -30,6 +30,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_mode.h> #include <drm/drm_plane_helper.h> +#include <linux/sync_file.h>

#include "drm_crtc_internal.h"

@@ -690,6 +691,17 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, drm_atomic_set_fb_for_plane(state, fb); if (fb) drm_framebuffer_unreference(fb);

• } else if (property == config->prop_fence_fd) {

• if (U642I64(val) == -1)
  

• 	return 0;
  

• if (state->fence)
  

• 	fence_put(state->fence);
  

• state->fence = sync_file_get_fence(val);
  

• if (!state->fence)
  

• 	return -EINVAL;
  

@@ -749,6 +761,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,

if (property == config->prop_fb_id) { *val = (state->fb) ? state->fb->base.id : 0;

• } else if (property == config->prop_fence_fd) {

• *val = -1;
  

  } else if (property == config->prop_crtc_id) { *val = (state->crtc) ? state->crtc->base.id : 0; } else if (property == config->prop_crtc_x) {

@@ -824,6 +838,11 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -EINVAL; }

• if (WARN_ON(state->fence && !state->fb)) {

• DRM_DEBUG_ATOMIC("in-fence set but no FB\n");
  

• return -EINVAL;
  

• }

Why is this a error? Could be useful to fence a modeset disable.

Yes. I didn't envision that use case. I'll change that for the next version.

It might make more sense to put the fence inside the crtc state, not the plane state. Updates are done per crtc and moving planes between multiple crtc's inside a single commit is not allowed. I'd like to know what others think of that.

I'm not sure this patch is tested, looks like plane_duplicate_state is not clearing ->fence, probably resulting in WARNs.

It is tested, but I'n not seeing any warning there. Which WARNs are you talking about? And why do we need to clear fence there? we don't clean anything else on duplicate.

Gustavo