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

[Maxime Ripard]

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.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20180925/bf594f93/attachment-0001.sig>