[Intel-gfx] [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Chris Wilson]

On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote:
> On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote:
> >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote:
> >>Hi Gustavo,
> >>
> >>A little late to the party here, but I was blocked by our internal
> >>contributions process...
> >>
> >>I didn't see these end up in my checkout yet though, so I guess they
> >>aren't picked up yet.
> >>
> >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
> >>>From: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> >>>
> >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for
> >>>atomic commits.
> >>>
> >>>Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> >>>---
> >>>lib/igt_kms.c | 20 +++++++++++++++++++-
> >>>lib/igt_kms.h |  3 +++
> >>>2 files changed, 22 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >>>index 4748c0a..f25e1eb 100644
> >>>--- a/lib/igt_kms.c
> >>>+++ b/lib/igt_kms.c
> >>>@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> >>>	"DEGAMMA_LUT",
> >>>	"GAMMA_LUT",
> >>>	"MODE_ID",
> >>>-	"ACTIVE"
> >>>+	"ACTIVE",
> >>>+	"OUT_FENCE_PTR"
> >>>};
> >>>
> >>>const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> >>>@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
> >>>		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, !!output);
> >>>	}
> >>>
> >>>+	if (pipe_obj->out_fence_ptr)
> >>>+		igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
> >>>+
> >>>	/*
> >>>	 *	TODO: Add all crtc level properties here
> >>>	 */
> >>>@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
> >>>}
> >>>
> >>>/**
> >>>+ * igt_pipe_set_out_fence_ptr:
> >>>+ * @pipe: pipe pointer to which background color to be set
> >>>+ * @fence_ptr: out fence pointer
> >>
> >>I don't think fence_ptr can be int *. It needs to be a pointer to a
> >>64-bit type.
> >>
> >>>+ *
> >>>+ * Sets the out fence pointer that will be passed to the kernel in
> >>>+ * the atomic ioctl. When the kernel returns the out fence pointer
> >>>+ * will contain the fd number of the out fence created by KMS.
> >>>+ */
> >>>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
> >>>+{
> >>>+	pipe->out_fence_ptr = (uint64_t) fence_ptr;
> >>>+}
> >>>+
> >>>+/**
> >>> * igt_crtc_set_background:
> >>> * @pipe: pipe pointer to which background color to be set
> >>> * @background: background color value in BGR 16bpc
> >>>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> >>>index 344f931..02d7bd1 100644
> >>>--- a/lib/igt_kms.h
> >>>+++ b/lib/igt_kms.h
> >>>@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
> >>>       IGT_CRTC_GAMMA_LUT,
> >>>       IGT_CRTC_MODE_ID,
> >>>       IGT_CRTC_ACTIVE,
> >>>+       IGT_CRTC_OUT_FENCE_PTR,
> >>>       IGT_NUM_CRTC_PROPS
> >>>};
> >>>
> >>>@@ -298,6 +299,7 @@ struct igt_pipe {
> >>>
> >>>	uint64_t mode_blob;
> >>>	bool mode_changed;
> >>>+	uint64_t out_fence_ptr;
> >>
> >>IMO this should be:
> >>
> >>	int64_t *out_fence_ptr;
> >
> >In userspace, fences are *fd*, a plain int. It is only the uabi that we
> >pass pointers as u64 to the kernel, and indeed that should be limited to
> >the uabi wrapper.
> >-Chris
> 
> Where's the uabi wrapper in this case?
> 
> Wherever it is, afaik someone needs to have 64-bit type for the kernel
> to stash its fd in - on the kernel side out_fence_ptr is
> (s64 __user *), so if there's not a 64-bit variable on the other end
> of it then someone's going to have a bad day.

We do not have pointers in the uabi because they are different sizes on
different platforms. The uabi must be a u64 representation of a user
address to store the result - that is what we pass to the crtc set
property ioctl. That it has been futher managled not to pass around fd
is an interesting twist, but ideally that sillyness should not make
itself into our API.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre