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

[Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5204 bytes --]

On 03/18, Liviu Dudau wrote:
> On Wed, Mar 06, 2019 at 06:30:05PM -0300, Rodrigo Siqueira wrote:
> > Hi Liviu,
> > 
> > I’m using your patchset to guide my implementation of writeback in the
> > VKMS, so, first of all, thanks :)
> > 
> > During my work, I noticed that you’re setting the drmSetClientCap()
> > before drmModeGetResources() which make the writeback capability
> 
> before? I believe your patchset moves it to be before, which makes
> sense, as the writeback applies to connectors, not planes (sorry for the
> confusion, I think I wrote the code when we were toying with the idea
> that writeback was going to be a plane property).
> 
> > ‘invisible’ for drmModeGetResources(). I made the following change, and
> > I could pass the igt_display_require():
> > 
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index f4ff3a96..bb2d8e06 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -1912,6 +1912,12 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >  
> >         display->drm_fd = drm_fd;
> >  
> > +       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> > +       if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
> > +               display->is_atomic = 1;
> > +
> > +       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > +
> >         resources = drmModeGetResources(display->drm_fd);
> >         if (!resources)
> >                 goto out;
> > @@ -1924,12 +1930,6 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >         display->pipes = calloc(sizeof(igt_pipe_t), display->n_pipes);
> >         igt_assert_f(display->pipes, "Failed to allocate memory for %d pipes\n", display->n_pipes);
> >  
> > -       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> > -       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);
> > 
> > I'm not 100% confident about this issue, because of this I will send an
> > RFC and see if I can get more details about this issue.
> 
> It actually looks correct to me, so 
> 
> Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> > 
> > Additionally, if you need any help with this patchset I will be glad to
> > help since I’m using it \o/
> 
> Well, I need to send an updated version and if you could review that
> then I can merge it into igt.

Sure! I will be glad to take some time to review your code :)

I can also try to test it on my current implementation of VKMS.
One question, I have a NanoPc-T1 device [1] which has a Mali GPU, can I
test the writeback feature in this device? All kind of Mali GPU has
support for the writeback feature?

1. http://wiki.friendlyarm.com/wiki/index.php/NanoPC-T1

Thanks!
Best Regards
 
> Best regards,
> Liviu
> 
> > 
> > Best Regards
> > Rodrigo Siqueira
> > 
> > On 02/04, Brian Starkey wrote:
> > > Hi Liviu,
> > > 
> > > On Tue, Jan 15, 2019 at 05:47:42PM +0000, Liviu Dudau wrote:
> > > > From: Brian Starkey <brian.starkey@arm.com>
> > > 
> > > [snip]
> > > 
> > > >  
> > > > +/**
> > > > + * 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);
> > > 
> > > I'm still not sure (ptrdiff_t) is the right type here, (uintptr_t)
> > > seems better.
> > > 
> > > Thanks,
> > > -Brian
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > 
> > -- 
> > Rodrigo Siqueira
> > https://siqueira.tech
> > Graduate Student
> > Department of Computer Science
> > University of São Paulo
> 
> 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev