| Message ID | 20180301173832.9654-2-liviu.dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 01-03-18 om 18:38 schreef Liviu Dudau: > From: Brian Starkey <brian.starkey@arm.com> > > Add support in igt_kms for Writeback connectors, with the ability to > attach framebuffers and retrieve fences. > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > --- > lib/igt_kms.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > lib/igt_kms.h | 14 ++++++++++++ > 2 files changed, 85 insertions(+), 1 deletion(-) > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index 022abfe7..00d9d2e2 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -190,7 +190,10 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { > "scaling mode", > "CRTC_ID", > "DPMS", > - "Broadcast RGB" > + "Broadcast RGB", > + "WRITEBACK_PIXEL_FORMATS", > + "WRITEBACK_FB_ID", > + "WRITEBACK_OUT_FENCE_PTR" > }; > > /* > @@ -452,6 +455,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" }, > {} > }; > > @@ -1947,6 +1951,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) > output->force_reprobe = true; > output->id = resources->connectors[i]; > output->display = display; > + output->writeback_out_fence_fd = -1; > > igt_output_refresh(output); > } > @@ -2040,6 +2045,43 @@ igt_output_t *igt_output_from_connector(igt_display_t *display, > return found; > } > > +void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb) > +{ > + igt_display_t *display = output->display; > + struct kmstest_connector_config *config = &output->config; > + > + if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > + return; igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, ...); As a bonus you don't need to check for WRITEBACK type, because this property will be undefined and commit will fail. > + LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name, > + fb ? fb->fb_id : 0); > + > + output->writeback_fb = fb; > +} Probably want to merge this with igt_output_request_writeback_out_fence(), else there's no point for the convenience function. :) > +static void igt_output_reset_writeback_out_fence(igt_output_t *output) > +{ > + if (output->writeback_out_fence_fd >= 0) { > + close(output->writeback_out_fence_fd); > + output->writeback_out_fence_fd = -1; > + } > +} > + > +void igt_output_request_writeback_out_fence(igt_output_t *output) > +{ > + igt_output_reset_writeback_out_fence(output); > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, > + (ptrdiff_t)&output->writeback_out_fence_fd); > +} > + > +int igt_output_get_last_writeback_out_fence(igt_output_t *output) > +{ > + int fd = output->writeback_out_fence_fd; > + output->writeback_out_fence_fd = -1; > + > + return fd; > +} Please handle this the same we do for pipe_obj->out_fence_fd.. No need to do it differently. > + > static void igt_pipe_fini(igt_pipe_t *pipe) > { > int i; > @@ -2063,6 +2105,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe) > static void igt_output_fini(igt_output_t *output) > { > kmstest_free_connector_config(&output->config); > + if (output->writeback_out_fence_fd >= 0) > + close(output->writeback_out_fence_fd); > free(output->name); > output->name = NULL; > } > @@ -2879,6 +2923,31 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto > output->props[i], > output->values[i])); > } > + > + if (output->writeback_fb) > + output->writeback_fb = NULL; > + > + igt_output_reset_writeback_out_fence(output); > +} > + > +static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret) > +{ > + int i; > + > + for (i = 0; i < display->n_outputs; i++) { > + igt_output_t *output = &display->outputs[i]; > + > + if (!output->config.connector) > + continue; > + > + if (!igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR)) > + continue; > + > + igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR); > + > + if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)) > + igt_assert(output->writeback_out_fence_fd == -1); > + } > } > > /* > @@ -2929,6 +2998,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_ > } > > ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); > + handle_writeback_out_fences(display, flags, ret); > > drmModeAtomicFree(req); > return ret; > diff --git a/lib/igt_kms.h b/lib/igt_kms.h > index 1c46186e..59ccc4c6 100644 > --- a/lib/igt_kms.h > +++ b/lib/igt_kms.h > @@ -38,6 +38,10 @@ > #include "igt_fb.h" > #include "ioctl_wrappers.h" > > +#ifndef DRM_MODE_CONNECTOR_WRITEBACK > +#define DRM_MODE_CONNECTOR_WRITEBACK 18 > +#endif > + > /* Low-level helpers with kmstest_ prefix */ > > /** > @@ -120,6 +124,9 @@ enum igt_atomic_connector_properties { > IGT_CONNECTOR_CRTC_ID, > IGT_CONNECTOR_DPMS, > IGT_CONNECTOR_BROADCAST_RGB, > + IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS, > + IGT_CONNECTOR_WRITEBACK_FB_ID, > + IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, > IGT_NUM_CONNECTOR_PROPS > }; > > @@ -345,6 +352,9 @@ typedef struct { > > uint32_t props[IGT_NUM_CONNECTOR_PROPS]; > uint64_t values[IGT_NUM_CONNECTOR_PROPS]; > + > + struct igt_fb *writeback_fb; I've spent a lot of effort into making sure we can set any property directly. Please don't add this struct member here, there's no need for it. > + int32_t writeback_out_fence_fd; > } igt_output_t; > > struct igt_display { > @@ -380,6 +390,10 @@ 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); > +void igt_output_request_writeback_out_fence(igt_output_t *output); > +int igt_output_get_last_writeback_out_fence(igt_output_t *output); > + > igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type); > > void igt_pipe_request_out_fence(igt_pipe_t *pipe);
Hi Maarten, Thanks for reviewing this! As this patch is quite old, I agree that it needs a refresh in order to catch up with the latest igt. I will take your feedback and come up with an update in the near future. Best regards, Liviu On Mon, Jun 25, 2018 at 02:19:46PM +0200, Maarten Lankhorst wrote: > Op 01-03-18 om 18:38 schreef Liviu Dudau: > > From: Brian Starkey <brian.starkey@arm.com> > > > > Add support in igt_kms for Writeback connectors, with the ability to > > attach framebuffers and retrieve fences. > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > --- > > lib/igt_kms.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > lib/igt_kms.h | 14 ++++++++++++ > > 2 files changed, 85 insertions(+), 1 deletion(-) > > > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > > index 022abfe7..00d9d2e2 100644 > > --- a/lib/igt_kms.c > > +++ b/lib/igt_kms.c > > @@ -190,7 +190,10 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { > > "scaling mode", > > "CRTC_ID", > > "DPMS", > > - "Broadcast RGB" > > + "Broadcast RGB", > > + "WRITEBACK_PIXEL_FORMATS", > > + "WRITEBACK_FB_ID", > > + "WRITEBACK_OUT_FENCE_PTR" > > }; > > > > /* > > @@ -452,6 +455,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" }, > > {} > > }; > > > > @@ -1947,6 +1951,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) > > output->force_reprobe = true; > > output->id = resources->connectors[i]; > > output->display = display; > > + output->writeback_out_fence_fd = -1; > > > > igt_output_refresh(output); > > } > > @@ -2040,6 +2045,43 @@ igt_output_t *igt_output_from_connector(igt_display_t *display, > > return found; > > } > > > > +void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb) > > +{ > > + igt_display_t *display = output->display; > > + struct kmstest_connector_config *config = &output->config; > > + > > + if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > > + return; > > igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, ...); > > As a bonus you don't need to check for WRITEBACK type, because this property will be undefined and commit will fail. > > > + LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name, > > + fb ? fb->fb_id : 0); > > + > > + output->writeback_fb = fb; > > +} > Probably want to merge this with igt_output_request_writeback_out_fence(), else there's no point for the convenience function. :) > > > +static void igt_output_reset_writeback_out_fence(igt_output_t *output) > > +{ > > + if (output->writeback_out_fence_fd >= 0) { > > + close(output->writeback_out_fence_fd); > > + output->writeback_out_fence_fd = -1; > > + } > > +} > > + > > +void igt_output_request_writeback_out_fence(igt_output_t *output) > > +{ > > + igt_output_reset_writeback_out_fence(output); > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, > > + (ptrdiff_t)&output->writeback_out_fence_fd); > > +} > > + > > +int igt_output_get_last_writeback_out_fence(igt_output_t *output) > > +{ > > + int fd = output->writeback_out_fence_fd; > > + output->writeback_out_fence_fd = -1; > > + > > + return fd; > > +} > Please handle this the same we do for pipe_obj->out_fence_fd.. No need to do it differently. > > + > > static void igt_pipe_fini(igt_pipe_t *pipe) > > { > > int i; > > @@ -2063,6 +2105,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe) > > static void igt_output_fini(igt_output_t *output) > > { > > kmstest_free_connector_config(&output->config); > > + if (output->writeback_out_fence_fd >= 0) > > + close(output->writeback_out_fence_fd); > > free(output->name); > > output->name = NULL; > > } > > @@ -2879,6 +2923,31 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto > > output->props[i], > > output->values[i])); > > } > > + > > + if (output->writeback_fb) > > + output->writeback_fb = NULL; > > + > > + igt_output_reset_writeback_out_fence(output); > > +} > > + > > +static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret) > > +{ > > + int i; > > + > > + for (i = 0; i < display->n_outputs; i++) { > > + igt_output_t *output = &display->outputs[i]; > > + > > + if (!output->config.connector) > > + continue; > > + > > + if (!igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR)) > > + continue; > > + > > + igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR); > > + > > + if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)) > > + igt_assert(output->writeback_out_fence_fd == -1); > > + } > > } > > > > /* > > @@ -2929,6 +2998,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_ > > } > > > > ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); > > + handle_writeback_out_fences(display, flags, ret); > > > > drmModeAtomicFree(req); > > return ret; > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h > > index 1c46186e..59ccc4c6 100644 > > --- a/lib/igt_kms.h > > +++ b/lib/igt_kms.h > > @@ -38,6 +38,10 @@ > > #include "igt_fb.h" > > #include "ioctl_wrappers.h" > > > > +#ifndef DRM_MODE_CONNECTOR_WRITEBACK > > +#define DRM_MODE_CONNECTOR_WRITEBACK 18 > > +#endif > > + > > /* Low-level helpers with kmstest_ prefix */ > > > > /** > > @@ -120,6 +124,9 @@ enum igt_atomic_connector_properties { > > IGT_CONNECTOR_CRTC_ID, > > IGT_CONNECTOR_DPMS, > > IGT_CONNECTOR_BROADCAST_RGB, > > + IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS, > > + IGT_CONNECTOR_WRITEBACK_FB_ID, > > + IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, > > IGT_NUM_CONNECTOR_PROPS > > }; > > > > @@ -345,6 +352,9 @@ typedef struct { > > > > uint32_t props[IGT_NUM_CONNECTOR_PROPS]; > > uint64_t values[IGT_NUM_CONNECTOR_PROPS]; > > + > > + struct igt_fb *writeback_fb; > I've spent a lot of effort into making sure we can set any property directly. Please don't add this struct member here, > there's no need for it. > > + int32_t writeback_out_fence_fd; > > } igt_output_t; > > > > struct igt_display { > > @@ -380,6 +390,10 @@ 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); > > +void igt_output_request_writeback_out_fence(igt_output_t *output); > > +int igt_output_get_last_writeback_out_fence(igt_output_t *output); > > + > > igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type); > > > > void igt_pipe_request_out_fence(igt_pipe_t *pipe); > >
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 022abfe7..00d9d2e2 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -190,7 +190,10 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { "scaling mode", "CRTC_ID", "DPMS", - "Broadcast RGB" + "Broadcast RGB", + "WRITEBACK_PIXEL_FORMATS", + "WRITEBACK_FB_ID", + "WRITEBACK_OUT_FENCE_PTR" }; /* @@ -452,6 +455,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" }, {} }; @@ -1947,6 +1951,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) output->force_reprobe = true; output->id = resources->connectors[i]; output->display = display; + output->writeback_out_fence_fd = -1; igt_output_refresh(output); } @@ -2040,6 +2045,43 @@ igt_output_t *igt_output_from_connector(igt_display_t *display, return found; } +void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb) +{ + igt_display_t *display = output->display; + struct kmstest_connector_config *config = &output->config; + + if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) + return; + + LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name, + fb ? fb->fb_id : 0); + + output->writeback_fb = fb; +} + +static void igt_output_reset_writeback_out_fence(igt_output_t *output) +{ + if (output->writeback_out_fence_fd >= 0) { + close(output->writeback_out_fence_fd); + output->writeback_out_fence_fd = -1; + } +} + +void igt_output_request_writeback_out_fence(igt_output_t *output) +{ + igt_output_reset_writeback_out_fence(output); + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, + (ptrdiff_t)&output->writeback_out_fence_fd); +} + +int igt_output_get_last_writeback_out_fence(igt_output_t *output) +{ + int fd = output->writeback_out_fence_fd; + output->writeback_out_fence_fd = -1; + + return fd; +} + static void igt_pipe_fini(igt_pipe_t *pipe) { int i; @@ -2063,6 +2105,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe) static void igt_output_fini(igt_output_t *output) { kmstest_free_connector_config(&output->config); + if (output->writeback_out_fence_fd >= 0) + close(output->writeback_out_fence_fd); free(output->name); output->name = NULL; } @@ -2879,6 +2923,31 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto output->props[i], output->values[i])); } + + if (output->writeback_fb) + output->writeback_fb = NULL; + + igt_output_reset_writeback_out_fence(output); +} + +static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret) +{ + int i; + + for (i = 0; i < display->n_outputs; i++) { + igt_output_t *output = &display->outputs[i]; + + if (!output->config.connector) + continue; + + if (!igt_output_is_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR)) + continue; + + igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR); + + if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)) + igt_assert(output->writeback_out_fence_fd == -1); + } } /* @@ -2929,6 +2998,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_ } ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); + handle_writeback_out_fences(display, flags, ret); drmModeAtomicFree(req); return ret; diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 1c46186e..59ccc4c6 100644 --- a/lib/igt_kms.h +++ b/lib/igt_kms.h @@ -38,6 +38,10 @@ #include "igt_fb.h" #include "ioctl_wrappers.h" +#ifndef DRM_MODE_CONNECTOR_WRITEBACK +#define DRM_MODE_CONNECTOR_WRITEBACK 18 +#endif + /* Low-level helpers with kmstest_ prefix */ /** @@ -120,6 +124,9 @@ enum igt_atomic_connector_properties { IGT_CONNECTOR_CRTC_ID, IGT_CONNECTOR_DPMS, IGT_CONNECTOR_BROADCAST_RGB, + IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS, + IGT_CONNECTOR_WRITEBACK_FB_ID, + IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, IGT_NUM_CONNECTOR_PROPS }; @@ -345,6 +352,9 @@ typedef struct { uint32_t props[IGT_NUM_CONNECTOR_PROPS]; uint64_t values[IGT_NUM_CONNECTOR_PROPS]; + + struct igt_fb *writeback_fb; + int32_t writeback_out_fence_fd; } igt_output_t; struct igt_display { @@ -380,6 +390,10 @@ 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); +void igt_output_request_writeback_out_fence(igt_output_t *output); +int igt_output_get_last_writeback_out_fence(igt_output_t *output); + igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type); void igt_pipe_request_out_fence(igt_pipe_t *pipe);