[Cogl] [PATCH 0/2] Add a workaround for slow read pixels on Mesa

[Neil Roberts]

Thanks for the feedback. I've reworked the last two patches based on
your suggestions. I agree we should leave the regular expression patch
for now.

Robert Bragg wrote:

> Could this be just CoglVersion perhaps?

Yeah, I agree CoglGpuVersion was a bit weird. I've reworked it so that
instead of having a struct to store the version number there are some
macros in cogl-util.h to pack the version number into an int. That way
we don't need the comparison function and we can just compare the
ints. I think this ends up more readable and probably more efficient
anyway.

> I'm wondering if this should be named CoglGpuInfo since I can imagine
> some of the corresponding function names seeming a bit confusing if
> they read as if you are querying the gpu itself. That would mean a
> more verbose cogl_gpu_info_ namespace though.

Ok, I've renamed it to the cogl_gpu_info namespace. It doesn't seem
overly long.

> I think going forward we will end up wanting to generalizing the ways
> in which we teach Cogl about new gpu vendors, drivers  and chipsets
> using some form of declarative style (perhaps using the X_MACRO style
> we use for gl-prototypes, or a statically initialized array or
> structs). I do wonder if we should look at starting something like
> that now along with this patch or maybe we should just add that on top
> later?

The new patches are more headed in this way so that there is an array
of structs for the vendors and driver packages. Each struct entry
contains the enum value, the name of the thing and a pointer to a
function to determine if this thing matches the GPU/driver. The
function gets passed a struct containing pointers to the pertinent
strings from glGetString.

> The checks for (width > 8 || height > 8) and (format &
> ~COGL_PREMULT_BIT) == COGL_PIXEL_FORMAT_BGRA_8888 look like they are
> checking for very specific constraints for using the PBO fast path in
> the intel mesa driver that you would only know from reading the mesa
> source code.
>
> It seems that we should have a big comment here explaining why these
> are things we need to check for.

Ok, I've added a comment with some bullet points. The size restriction
isn't actually necessary to hit the PBO path but I just added it
anyway because it seemed like it wouldn't be worth allocating a PBO
for trivially small reads. But I agree this is quite misleading
without the comment.

> I also wonder if "COGL_GPU_DRIVER_BUG_SLOW_READ_PIXELS" is too vague
> and if it should maybe be something more explicit like
> "COGL_GPU_DRIVER_BUG_MESA_LT_8_0_1_SLOW_READ_PIXELS". I can imagine
> that this won't be the last bug we see relating to slow read-pixels
> performance.

Ok, I've renamed the enum to include the bug number and added a
comment above it.

Regards,
- Neil