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

[Arkadiusz Hiler]

On Mon, Oct 01, 2018 at 04:55:22PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 01, 2018 at 01:47:12PM +0300, Arkadiusz Hiler wrote:
> > 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.
> 
> Practically nothing supports 24bpp.

Misread that as XRGB8888, my bad. It would have worked with that.

@Maxime: I guess fbinit() + malloc for the dummy fb will do the trick
for you, as you seem use that just for the conversion purposes.

-- 
Cheers,
Arek