[Mesa-dev] [PATCH rework] mesa: rework Driver.CopyImageSubData() and related code

[Pohjolainen, Topi]

On Fri, Sep 04, 2015 at 12:39:52PM -0600, Brian Paul wrote:
> On 08/28/2015 11:03 AM, Jason Ekstrand wrote:
> >On Fri, Aug 28, 2015 at 7:43 AM, Brian Paul <brianp at vmware.com> wrote:
> >>On 08/28/2015 12:43 AM, Jason Ekstrand wrote:
> >>>
> >>>On Thu, Aug 27, 2015 at 11:42 PM, Jason Ekstrand <jason at jlekstrand.net>
> >>>wrote:
> >>>>
> >>>>From: Brian Paul <brianp at vmware.com>
> >>>>
> >>>>Previously, core Mesa's _mesa_CopyImageSubData() created temporary
> >>>>textures
> >>>>to wrap renderbuffer sources/destinations.  This caused a bit of a mess
> >>>>in
> >>>>the Mesa/gallium state tracker because we had to basically undo that
> >>>>wrapping.
> >>>>
> >>>>Instead, change ctx->Driver.CopyImageSubData() to take both
> >>>>gl_renderbuffer
> >>>>and gl_texture_image src/dst pointers (one being null, the other
> >>>>non-null)
> >>>>so the driver can handle renderbuffer vs. texture as needed.
> >>>>
> >>>>For the i965 driver, we basically moved the code that wrapped textures
> >>>>around renderbuffers from copyimage.c down into the driver.  So that
> >>>>approach is still used there as before.
> >>>>
> >>>>The old code in copyimage.c also made some questionable calls to
> >>>>_mesa_BindTexture(), etc. which weren't undone at the end.
> >>>>
> >>>>v2 (Jason Ekstrand): Rework the intel bits
> >>>>---
> >>>>
> >>>>TBH, I haven't actually reviewed the rest of the patch yet but I figured
> >>>>I'd save Brian the time of guess-and-check implementing it on Intel.  I
> >>>>completely reworked the intel bits so we now use the tex/rb directly and
> >>>>pun the renderbuffer wrapping all the way down to the meta layer where it
> >>>>belongs.
> >>>
> >>>
> >>>I ran a few quick piglit tests on my Broadwell and it seems to work.
> >>>I also sent it off to our CI system and I'll have those results in the
> >>>morning.
> >>
> >>
> >>Thanks, Jason!  I appreciate you taking the time to do this.
> >>
> >>-Brian
> >
> >I looked through the patch for real this time, and everything looks
> >pretty good.  thanks for cleaning things up and adding spec comments.
> >The non-Intel bits are
> >
> >Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>
> >
> >Someone else should review the intel bits since I wrote them.  Also,
> >FWIW, I ran this patch through our CI system and there are no piglit
> >regressions on any Intel platform.
> 
> The intel changes looked ok to me, FWIW.  Can we get this committed so I can
> commit the rest of my series?

Just in case Jason was waiting for another review:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

Just for the record though, I think there was too much happening in one
patch and I would have preferred it split to ease the review.