[igt-dev] [PATCH i-g-t v4 1/6] lib/igt_kms: Add writeback support
Liviu Dudau
Liviu.Dudau at arm.com
Fri Dec 7 18:12:53 UTC 2018
On Fri, Nov 09, 2018 at 03:09:41PM +0000, Brian Starkey wrote:
> Hi Liviu,
>
> Thanks for resurrecting this.
>
> On Mon, Nov 05, 2018 at 10:16:11AM +0000, Liviu Dudau wrote:
> >From: Brian Starkey <brian.starkey at arm.com>
> >
> >Add support in igt_kms for writeback connectors, with the ability
> >to attach framebuffers.
> >
> >Signed-off-by: Brian Starkey <brian.starkey at arm.com>
> >[rebased and updated to the latest igt style]
> >Signed-off-by: Liviu Dudau <liviu.dudau at arm.com>
> >---
> > include/drm-uapi/drm.h | 16 ++++++++++++
> > include/drm-uapi/drm_mode.h | 1 +
> > lib/igt_kms.c | 50 +++++++++++++++++++++++++++++++++++++
> > lib/igt_kms.h | 6 +++++
> > 4 files changed, 73 insertions(+)
> >
> >diff --git a/include/drm-uapi/drm.h b/include/drm-uapi/drm.h
> >index f0bd91de0..85c685a20 100644
> >--- a/include/drm-uapi/drm.h
> >+++ b/include/drm-uapi/drm.h
> >@@ -674,6 +674,22 @@ struct drm_get_cap {
> > */
> > #define DRM_CLIENT_CAP_ATOMIC 3
> >
> >+/**
> >+ * DRM_CLIENT_CAP_ASPECT_RATIO
> >+ *
> >+ * If set to 1, the DRM core will provide aspect ratio information in modes.
> >+ */
> >+#define DRM_CLIENT_CAP_ASPECT_RATIO 4
> >+
> >+/**
> >+ * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> >+ *
> >+ * If set to 1, the DRM core will expose special connectors to be used for
> >+ * writing back to memory the scene setup in the commit. Depends on client
> >+ * also supporting DRM_CLIENT_CAP_ATOMIC
> >+ */
> >+#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS 5
> >+
> > /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
> > struct drm_set_client_cap {
> > __u64 capability;
> >diff --git a/include/drm-uapi/drm_mode.h b/include/drm-uapi/drm_mode.h
> >index 2c575794f..7b47e184e 100644
> >--- a/include/drm-uapi/drm_mode.h
> >+++ b/include/drm-uapi/drm_mode.h
> >@@ -338,6 +338,7 @@ enum drm_mode_subconnector {
> > #define DRM_MODE_CONNECTOR_VIRTUAL 15
> > #define DRM_MODE_CONNECTOR_DSI 16
> > #define DRM_MODE_CONNECTOR_DPI 17
> >+#define DRM_MODE_CONNECTOR_WRITEBACK 18
> >
> > struct drm_mode_get_connector {
> >
> >diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >index d806ccc1e..00d356fe0 100644
> >--- a/lib/igt_kms.c
> >+++ b/lib/igt_kms.c
> >@@ -197,6 +197,9 @@ const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> > [IGT_CONNECTOR_DPMS] = "DPMS",
> > [IGT_CONNECTOR_BROADCAST_RGB] = "Broadcast RGB",
> > [IGT_CONNECTOR_CONTENT_PROTECTION] = "Content Protection",
> >+ [IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS] = "WRITEBACK_PIXEL_FORMATS",
> >+ [IGT_CONNECTOR_WRITEBACK_FB_ID] = "WRITEBACK_FB_ID",
> >+ [IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR] = "WRITEBACK_OUT_FENCE_PTR",
> > };
> >
> > /*
> >@@ -453,6 +456,7 @@ static const struct type_name connector_type_names[] = {
> > { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
> > { DRM_MODE_CONNECTOR_DSI, "DSI" },
> > { DRM_MODE_CONNECTOR_DPI, "DPI" },
> >+ { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
> > {}
> > };
> >
> >@@ -1802,6 +1806,12 @@ static void igt_output_reset(igt_output_t *output)
> > if (igt_output_has_prop(output, IGT_CONNECTOR_BROADCAST_RGB))
> > igt_output_set_prop_value(output, IGT_CONNECTOR_BROADCAST_RGB,
> > BROADCAST_RGB_FULL);
> >+ if (igt_output_has_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID))
> >+ igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, 0);
> >+ if (igt_output_has_prop(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR)) {
> >+ igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
> >+ output->writeback_out_fence_fd = -1;
> >+ }
> > }
> >
> > /**
> >@@ -1814,6 +1824,8 @@ static void igt_output_reset(igt_output_t *output)
> > * For outputs:
> > * - %IGT_CONNECTOR_CRTC_ID
> > * - %IGT_CONNECTOR_BROADCAST_RGB (if applicable)
> >+ * - %IGT_CONNECTOR_WRITEBACK_FB_ID
> >+ * - %IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR
> > * - igt_output_override_mode() to default.
> > *
> > * For pipes:
> >@@ -1898,6 +1910,8 @@ bool igt_display_init(igt_display_t *display, int drm_fd)
> > if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
> > display->is_atomic = 1;
> >
> >+ drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> >+
> > plane_resources = drmModeGetPlaneResources(display->drm_fd);
> > igt_assert(plane_resources);
> >
> >@@ -2147,6 +2161,9 @@ static void igt_output_fini(igt_output_t *output)
> > kmstest_free_connector_config(&output->config);
> > free(output->name);
> > output->name = NULL;
> >+
> >+ if (output->writeback_out_fence_fd != -1)
> >+ close(output->writeback_out_fence_fd);
>
> Is it worth setting it to -1 here too? output->name looks like it's
> set to NULL defensively.
I was following the style of igt_pipe_fini() where the out_fence_fd was
not set to -1 after closing, but I agree, it is probably safer to set it so.
>
> > }
> >
> > /**
> >@@ -3144,6 +3161,11 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto
> > output->props[i],
> > output->values[i]));
> > }
> >+
> >+ if (output->writeback_out_fence_fd != -1) {
> >+ close(output->writeback_out_fence_fd);
> >+ output->writeback_out_fence_fd = -1;
> >+ }
> > }
> >
> > /*
> >@@ -3264,6 +3286,14 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
> > else
> > /* no modeset in universal commit, no change to crtc. */
> > output->changed &= 1 << IGT_CONNECTOR_CRTC_ID;
> >+
> >+ if (s == COMMIT_ATOMIC) {
> >+ if (igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR))
> >+ igt_assert(output->writeback_out_fence_fd >= 0);
> >+
> >+ output->values[IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR] = 0;
> >+ igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_FB_ID);
>
> The WRITEBACK_FB_ID property is auto-clearing, so does that need
> special handling here? Might need to set
> output->values[IGT_CONNECTOR_WRITEBACK_FB_ID] = 0 and mark it as
> changed.
Will do.
>
> >+ }
> > }
> >
> > if (display->first_commit) {
> >@@ -3884,6 +3914,26 @@ void igt_pipe_request_out_fence(igt_pipe_t *pipe)
> > igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_OUT_FENCE_PTR, (ptrdiff_t)&pipe->out_fence_fd);
> > }
> >
> >+/**
> >+ * igt_output_set_writeback_fb:
> >+ * @output: Target output
> >+ * @fb: Target framebuffer
> >+ *
> >+ * This function sets the given @fb to be used as the target framebuffer for the
> >+ * writeback engine at the next atomic commit. It will also request a writeback
> >+ * out fence that will contain the fd number of the out fence created by KMS.
> >+ */
> >+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb)
> >+{
> >+ igt_display_t *display = output->display;
> >+
> >+ LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name, fb ? fb->fb_id : 0);
> >+
> >+ igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb ? fb->fb_id : 0);
> >+ igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> >+ (ptrdiff_t)&output->writeback_out_fence_fd);
>
> Did you mean ptrdiff_t? Might have been a typo in the
> CRTC_OUT_FENCE_PTR
No, again I was following the style of out_fence_fd variable where it
does use ptrdiff_t. Maybe that one is wrong too?
>
> Also commented on a later patch - but requesting the out-fence could
> be dependent on fb_id != 0, then you wouldn't need to open-code that
> condition in the sequence test.
Where would the dependency be checked?
Best regards,
Liviu
>
> Cheers,
> -Brian
>
> >+}
> >+
> > /**
> > * igt_wait_for_vblank_count:
> > * @drm_fd: A drm file descriptor
> >diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> >index e6aff339a..a84f9ac42 100644
> >--- a/lib/igt_kms.h
> >+++ b/lib/igt_kms.h
> >@@ -121,6 +121,9 @@ enum igt_atomic_connector_properties {
> > IGT_CONNECTOR_DPMS,
> > IGT_CONNECTOR_BROADCAST_RGB,
> > IGT_CONNECTOR_CONTENT_PROTECTION,
> >+ IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
> >+ IGT_CONNECTOR_WRITEBACK_FB_ID,
> >+ IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> > IGT_NUM_CONNECTOR_PROPS
> > };
> >
> >@@ -359,6 +362,8 @@ typedef struct {
> > bool use_override_mode;
> > drmModeModeInfo override_mode;
> >
> >+ int32_t writeback_out_fence_fd;
> >+
> > /* bitmask of changed properties */
> > uint64_t changed;
> >
> >@@ -404,6 +409,7 @@ igt_plane_t *igt_output_get_plane(igt_output_t *output, int plane_idx);
> > igt_plane_t *igt_output_get_plane_type(igt_output_t *output, int plane_type);
> > igt_output_t *igt_output_from_connector(igt_display_t *display,
> > drmModeConnector *connector);
> >+void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb);
> >
> > igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
> > igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe pipe);
> >--
> >2.19.1
> >
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
More information about the igt-dev
mailing list