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@xxxxxxx>
> >
> > Add support in igt_kms for Writeback connectors, with the ability to
> > attach framebuffers and retrieve fences.
> >
> > Signed-off-by: Brian Starkey <brian.starkey@xxxxxxx>
> > ---
> > 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);
>
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
