[igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.

[Paul Kocialkowski]

Hi,

On Fri, 2019-05-10 at 16:19 +0200, Maarten Lankhorst wrote:
> Op 10-05-2019 om 15:43 schreef Paul Kocialkowski:
> > Hi,
> > 
> > It looks like I fell behind on reviewing earlier version here, sorry
> > about that.
> > 
> > On Fri, 2019-05-10 at 14:33 +0200, Maarten Lankhorst wrote:
> > > Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
> > > through pixman") we can generate a valid cairo surface for any plane,
> > > use this to avoid having to implement our own conversion routine.
> > > 
> > > Instead of duplicating this functionality in igt_fb_convert_with_stride,
> > > we can simply convert this to a few cairo calls, because we now support
> > > cairo calls to any of the supported framebuffer formats.
> > I don't see how that is the case. Cairo definitely does not support our
> > tiled formats, so we can't just pipe them into it.
> > 
> > And we already use pixman for converting through the fb_convert call to
> > convert_pixman when possible. Also, my understanding is that cairo is
> > very limited format-wise, so we're much better off using pixman
> > directly (which is is what a non-accelerated cairo surface will do
> > anyway).
> > 
> > > This is required to make this function more generic, and convert from any
> > > format/modifier to any other format/modifier.
> > We've already designed it to be generic, we just need conversion
> > helpers from/to (currently only to) hardware-specific tiling modifiers.
> > 
> > We can't expect cairo or pixman to do that job and this change pretty
> > much kills support for the vc4 tiling modes we've added.
> > 
> > Unless I'm missing something, that's a NACK on my side.
> 
> You have a function that can convert a untiled fb to tiled, but not
> the other way around in igt_vc4.c

We don't need it for anything at the moment but it wouldn't be a
problem to come up with one, sure.

> If you could provide that, we could convert from any fb/modifier
> format to any fb/modifier format, even with VC4 tiling.

I don't fully get the point of that if our tests are not going to use
it, but it doesn't hurt either :)

> Our internal cairo hooks already provide helpers for conversion, see
> igt_get_cairo_surface() which calls
> create_cairo_surface__convert()

My feeling is that we should kill use of cairo formats and go with
pixman directly.

> Some conversions can only be done through cairo, converting between
> P01x and XRGB8888 cannot be done directly,
> our pixman representation for that are done in the floating point
> formats.

I'm not sure I'm following this point, but if there's a conversion that
cairo can do and pixman can't, it feels like the right thing would be
to fix pixman to support it. Is there a reason in particular why this
wouldn't work?

> The only way to convert between some framebuffer formats and
> modifiers in i915 is by using those conversion functions,
> the fact that igt_fb_convert_with_stride doesn't do those, makes a
> truly random hdmi-cmp-planes-random useless.

So it boils down to a missing format support issue, not really to an
issue with the existing flow.

> I was getting CRC mismatches because the i915 modifiers weren't
> respected. When I made sure they were being respected
> I ended up with a duplicate of the cairo context stuff. So why not
> lose a little speed and use that?

My counter-suggestion would be to do the opposite and make sure pixman
can deal with these cases on its own.

> If you write and test a detiler function, I can hook it up for you.
> :)
> If necessary I can do it myself, to move this patch forward.
> 
> Cairo shouldn't be much slower than pixman, because internally it
> already uses pixman calls for the backend.

Sure, but that's not cairo's role: cairo is about drawing, while pixman
is about pixel conversion. I think it would be best to stick to that.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com