[igt-dev] [PATCH v7 02/14] fb: Use an igt_fb for the cairo shadow buffer

[Arkadiusz Hiler]

On Tue, Sep 25, 2018 at 10:51:24AM +0200, Maxime Ripard wrote:
> On Wed, Sep 19, 2018 at 04:21:38PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 11, 2018 at 10:47:29AM +0200, Maxime Ripard wrote:
> > > In the case where an igt_fb user wants to get a cairo surface out of that
> > > framebuffer, if the format of that framebuffer cannot be imported as-is in
> > > Cairo, the current code will do an anonymous mapping to create a shadow
> > > buffer where an RGB24 copy of that buffer will be created.
> > > 
> > > However, making that shadow buffer into an igt_fb itself will help us do
> > > further improvements on the conversion code.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard at bootlin.com>
> > > ---
> > >  lib/igt_fb.c | 28 +++++++++++++++++-----------
> > >  1 file changed, 17 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > index 542c62610412..dd180c6c8016 100644
> > > --- a/lib/igt_fb.c
> > > +++ b/lib/igt_fb.c
> > > @@ -1381,12 +1381,12 @@ static void create_cairo_surface__gtt(int fd, struct igt_fb *fb)
> > >  
> > >  struct fb_convert_blit_upload {
> > >  	int fd;
> > > +
> > >  	struct igt_fb *fb;
> > > +	uint8_t *ptr;
> > >  
> > > -	struct {
> > > -		uint8_t *map;
> > > -		unsigned stride, size;
> > > -	} rgb24;
> > > +	struct igt_fb shadow_fb;
> > > +	uint8_t *shadow_ptr;
> > 
> > We seem to be pairing fb and ptr a lot. I wonder if we should suck the
> > the ptr into the fb struct. But maybe someone wants to map the same
> > thing multiple times? Not sure.
> 
> Is it something you want me to change then?
> 
> > >  	struct fb_blit_linear linear;
> > >  };
> > > @@ -1780,7 +1780,8 @@ static void destroy_cairo_surface__convert(void *arg)
> > >  			     fb->drm_format);
> > >  	}
> > >  
> > > -	munmap(blit->rgb24.map, blit->rgb24.size);
> > > +	igt_fb_unmap_buffer(&blit->shadow_fb, blit->shadow_ptr);
> > > +	igt_remove_fb(blit->fd, &blit->shadow_fb);
> > >  
> > >  	if (blit->linear.handle)
> > >  		free_linear_mapping(blit->fd, blit->fb, &blit->linear);
> > > @@ -1795,14 +1796,19 @@ static void destroy_cairo_surface__convert(void *arg)
> > >  static void create_cairo_surface__convert(int fd, struct igt_fb *fb)
> > >  {
> > >  	struct fb_convert_blit_upload *blit = malloc(sizeof(*blit));
> > > +	int shadow_id;
> > > +
> > >  	igt_assert(blit);
> > >  
> > >  	blit->fd = fd;
> > >  	blit->fb = fb;
> > > -	blit->rgb24.stride = ALIGN(fb->width * 4, 16);
> > > -	blit->rgb24.size = ALIGN(blit->rgb24.stride * fb->height, sysconf(_SC_PAGESIZE));
> > > -	blit->rgb24.map = mmap(NULL, blit->rgb24.size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > > -	igt_assert(blit->rgb24.map != MAP_FAILED);
> > > +
> > > +	shadow_id = igt_create_fb(fd, fb->width, fb->height, DRM_FORMAT_RGB888,
> > > +				  LOCAL_DRM_FORMAT_MOD_NONE, &blit->shadow_fb);
> > > +	igt_assert(shadow_id > 0);
> > 
> > We can't actually expect addfb to work for RGB888.
> 
> Because the driver might not support it?
> 
> > That thing is not supported by most hardware. Since my series adding
> > fb_init() etc. hasn't gotten reviewed I guess we'll just need to
> > populate the igt_fb by hand here, or something.
> 
> At this point, I have to ask: is there any actual will to have this
> patch merged at some point?
> 
> This series is just a long history of either being ignored, dealing
> with the i915-specific changes that got in and are breaking the other
> users, or huge reworks for i915-specific uses cases I have no way to
> test.
> 
> If the current idea is back to igt is for i915 solely, then there's no
> point in wasting time on this.

Sorry for wasting your time up to this point. If you will ever feel
blocked, please do contact me or Petri.

Seeing some comments here and there, as well as reviewed-bys I was
wrongly assuming that this is progressing. This is my failure here, and
getting to v7 should have alarmed me. I am truly sorry.

Back to the topic - yes your contributions are welcome.

I really like the idea, that instead of manually fiddling with rgb24 we
could deal with a proper igt_fb here, because it makes life easier later.

@Ville: We have your fb_init() merged now, so we can use it here, so it
should be hardware-that-does-not-support-rgb888-proof. And it's not like
we should have blocked here in the first place, as all the hardware we
run on seems to support it, and it could be reworked later on.

@Maxime: Let's merge this in chunks now. Let's start with the first two
patches. I'll rebase them, switch them to use fb_init(), do the changes
in convert_nv12_to_rgb24 and send them for CI approval.

-- 
Cheers,
Arek