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

[Brian Paul]

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.

FWIW, the new code looks good to me.  I only spotted one comment typo: 
s/prever/prefer/.

For your new code:
Reviewed-by: Brian Paul <brianp at vmware.com>

-Brian