Re: [igt-dev] [PATCH V6 i-g-t 1/6] lib/igt_kms: Add writeback support

Ser, Simon
2019-07-09T14:42:54Z

On Tue, 2019-07-09 at 11:32 -0300, Rodrigo Siqueira wrote:
> On Wed, Jul 3, 2019 at 9:15 AM Ser, Simon <simon.ser@intel.com> wrote:
> > On Tue, 2019-06-18 at 18:56 -0300, Rodrigo Siqueira wrote:
> > > On Thu, Jun 13, 2019 at 11:54 AM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > > On Wed, Jun 12, 2019 at 11:16:02PM -0300, Brian Starkey wrote:
> > > > > Add support in igt_kms for writeback connectors, with the ability
> > > > > to attach framebuffers.
> > > > >
> > > > > v5: Rebase and add DRM_CLIENT_CAP_WRITEBACK_CONNECTORS before
> > > > > drmModeGetResources()
> > > >
> > > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > > >
> > > > Thanks for updating this! Given that I have done the last changes and this is
> > > > mostly a refresh, not sure if I should add more Reviewed-by's to the other
> > > > patches.
> > >
> > > Thanks Liviu!
> > >
> > > I just forgot to add my SoB, and for some reason, gmail does not allow
> > > me to send an email on someone behalf.
> >
> > FWIW, that's a good thing, and it's required to pass DMARC checks.
> >
> > Instead, git-send-email should add a From line at the beginning of the
> > message when sending a patch on behalf of someone else. I wonder what
> > happened here.
>
> Thank you for your help.
>
> I’m using neomutt for sending patches, and I’ll take a look at
> git-send-email. Additionally, it looks like Gmail requires that I add
> a new account in order to allow me to send patches on someone behalf.
> I’ll take some time to read about this issue, and I will try to resend
> the patchset again.

When sending a patch on someone else's behalf, you shouldn't send the
e-mail with the sender (From header) set to someone else. You should
use your normal address. The From line in the body of the message will
make Git understand the author is someone else.

So no new account setup required.

I really recommend using git-send-email for patches. There are too many
ways to make a mistake if you try sending emails manually. Here is a
tutorial:
https://git-send-email.io/

> Btw, if you have time, could you take a look in this series?

Sure, I've already began looking at the series. I'll continue soon.

Thanks for your work!

> Best regards
>
> > > Btw, I can fix it after
> > > everybody agrees that the kms_writeback is ready for landing.
> > >
> > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > > > > [rebased and updated to the latest igt style]
> > > > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> > > > > ---
> > > > > lib/igt_kms.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > lib/igt_kms.h | 6 ++++++
> > > > > 2 files changed, 63 insertions(+)
> > > > >
> > > > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > > > index da188a39..140db346 100644
> > > > > --- a/lib/igt_kms.c
> > > > > +++ b/lib/igt_kms.c
> > > > > @@ -325,6 +325,9 @@ const char * const igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> > > > > [IGT_CONNECTOR_BROADCAST_RGB] = "Broadcast RGB",
> > > > > [IGT_CONNECTOR_CONTENT_PROTECTION] = "Content Protection",
> > > > > [IGT_CONNECTOR_VRR_CAPABLE] = "vrr_capable",
> > > > > + [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",
> > > > > };
> > > > >
> > > > > /*
> > > > > @@ -557,6 +560,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" },
> > > > > {}
> > > > > };
> > > > >
> > > > > @@ -1889,6 +1893,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;
> > > > > + }
> > > > > }
> > > > >
> > > > > /**
> > > > > @@ -1901,6 +1911,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:
> > > > > @@ -1970,6 +1982,8 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> > > > >
> > > > > display->drm_fd = drm_fd;
> > > > >
> > > > > + drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > > > > +
> > > > > resources = drmModeGetResources(display->drm_fd);
> > > > > if (!resources)
> > > > > goto out;
> > > > > @@ -2263,6 +2277,11 @@ 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);
> > > > > + output->writeback_out_fence_fd = -1;
> > > > > + }
> > > > > }
> > > > >
> > > > > /**
> > > > > @@ -3325,6 +3344,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;
> > > > > + }
> > > > > }
> > > > >
> > > > > /*
> > > > > @@ -3447,6 +3471,16 @@ 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;
> > > > > + output->values[IGT_CONNECTOR_WRITEBACK_FB_ID] = 0;
> > > > > + igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_FB_ID);
> > > > > + igt_output_clear_prop_changed(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR);
> > > > > + }
> > > > > }
> > > > >
> > > > > if (display->first_commit) {
> > > > > @@ -4119,6 +4153,29 @@ 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 if
> > > > > + * the given @fb is valid.
> > > > > + */
> > > > > +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);
> > > > > + /* only request a writeback out fence if the framebuffer is valid */
> > > > > + if (fb)
> > > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> > > > > + (ptrdiff_t)&output->writeback_out_fence_fd);
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * igt_wait_for_vblank_count:
> > > > > * @drm_fd: A drm file descriptor
> > > > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > > > > index a448a003..cacc6b90 100644
> > > > > --- a/lib/igt_kms.h
> > > > > +++ b/lib/igt_kms.h
> > > > > @@ -123,6 +123,9 @@ enum igt_atomic_connector_properties {
> > > > > IGT_CONNECTOR_BROADCAST_RGB,
> > > > > IGT_CONNECTOR_CONTENT_PROTECTION,
> > > > > IGT_CONNECTOR_VRR_CAPABLE,
> > > > > + IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS,
> > > > > + IGT_CONNECTOR_WRITEBACK_FB_ID,
> > > > > + IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR,
> > > > > IGT_NUM_CONNECTOR_PROPS
> > > > > };
> > > > >
> > > > > @@ -364,6 +367,8 @@ typedef struct {
> > > > > bool use_override_mode;
> > > > > drmModeModeInfo override_mode;
> > > > >
> > > > > + int32_t writeback_out_fence_fd;
> > > > > +
> > > > > /* bitmask of changed properties */
> > > > > uint64_t changed;
> > > > >
> > > > > @@ -414,6 +419,7 @@ igt_plane_t *igt_output_get_plane_type_index(igt_output_t *output,
> > > > > igt_output_t *igt_output_from_connector(igt_display_t *display,
> > > > > drmModeConnector *connector);
> > > > > const drmModeModeInfo *igt_std_1024_mode_get(void);
> > > > > +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);
> > > > > int igt_pipe_count_plane_type(igt_pipe_t *pipe, int plane_type);
> > > > > --
> > > > > 2.21.0
> > > > >
> > > > >
> > > > > --
> > > > > Rodrigo Siqueira
> > > > > https://siqueira.tech
> > > >
> > > > --
> > > > ====================
> > > > > I would like to |
> > > > > fix the world, |
> > > > > but they're not |
> > > > > giving me the |
> > > > \ source code! /
> > > > ---------------
> > > > ¯\_(ツ)_/¯