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

[Liviu Dudau <Liviu.Dudau@arm.com>]

On Mon, Mar 18, 2019 at 07:05:29PM -0300, Rodrigo Siqueira wrote:
> 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

(sorry, email fell through the cracks)

The writeback is a Mali Display Processor feature, not GPU. As such you
need a device that has Mali DP in it. Commercially there will be a
platform later this year, but until it is announced I cannot say more
about it.

Best regards,
Liviu

> 
> 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



-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev