Re: [Mesa-dev] [PATCH 3/5] i965: Check base format to determine whether to use tiled memcpy
Reviewed-by: Jason Ekstrand On Thu, Nov 19, 2015 at 7:25 AM, Neil Roberts wrote: > The tiled memcpy doesn't work for copying from RGBX to RGBA because it > doesn't override the alpha component to 1.0. Commit 2cebaac479d4 added > a check to disable it for RGBX formats by looking at the TexFormat. > However a lot of the rest of the code base is written with the > assumption that an RGBA texture can be used internally to implement a > GL_RGB texture. If that is done then this check breaks. This patch > makes it instead check the base format of the texture which I think > more directly matches the intention. > > Cc: Jason Ekstrand > --- > src/mesa/drivers/dri/i965/intel_pixel_read.c | 7 --- > src/mesa/drivers/dri/i965/intel_tex_image.c | 7 --- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/intel_pixel_read.c > b/src/mesa/drivers/dri/i965/intel_pixel_read.c > index 9bcbbd1..c8aef65 100644 > --- a/src/mesa/drivers/dri/i965/intel_pixel_read.c > +++ b/src/mesa/drivers/dri/i965/intel_pixel_read.c > @@ -135,10 +135,11 @@ intel_readpixels_tiled_memcpy(struct gl_context * ctx, >return false; > > /* We can't handle copying from RGBX or BGRX because the tiled_memcpy > -* function doesn't set the last channel to 1. > +* function doesn't set the last channel to 1. Note this checks BaseFormat > +* rather than TexFormat in case the RGBX format is being simulated with > an > +* RGBA format. > */ > - if (rb->Format == MESA_FORMAT_B8G8R8X8_UNORM || > - rb->Format == MESA_FORMAT_R8G8B8X8_UNORM) > + if (rb->_BaseFormat == GL_RGB) >return false; > > if (!intel_get_memcpy(rb->Format, format, type, &mem_copy, &cpp, > diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c > b/src/mesa/drivers/dri/i965/intel_tex_image.c > index 34b91e8..e3710da7 100644 > --- a/src/mesa/drivers/dri/i965/intel_tex_image.c > +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c > @@ -399,10 +399,11 @@ intel_gettexsubimage_tiled_memcpy(struct gl_context > *ctx, >return false; > > /* We can't handle copying from RGBX or BGRX because the tiled_memcpy > -* function doesn't set the last channel to 1. > +* function doesn't set the last channel to 1. Note this checks BaseFormat > +* rather than TexFormat in case the RGBX format is being simulated with > an > +* RGBA format. > */ > - if (texImage->TexFormat == MESA_FORMAT_B8G8R8X8_UNORM || > - texImage->TexFormat == MESA_FORMAT_R8G8B8X8_UNORM) > + if (texImage->_BaseFormat == GL_RGB) >return false; > > if (!intel_get_memcpy(texImage->TexFormat, format, type, &mem_copy, &cpp, > -- > 1.9.3 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/5] i965: Check base format to determine whether to use tiled memcpy
Ben Widawsky writes: > I really don't know the corner cases well enough, but concept seems > good to me. In particular, I assume this is now going to return false > for more cases than previously - and that's okay, I guess? In practice I don't think it will disable any extra formats because the intel_get_memcpy function which this calls is a lot more strict about which specific formats it takes anyway. It's a bit weird because intel_get_memcpy explicitly allows BGRX and RGBX so Jason's original patch could have been implemented by just removing that explicit support. However now that the condition is being changed to look at _BaseFormat instead I think it does make more sense to leave the check in intel_gettexsubimage_tiled_memcpy. Regards, - Neil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/5] i965: Check base format to determine whether to use tiled memcpy
On Thu, Nov 19, 2015 at 04:25:19PM +0100, Neil Roberts wrote: > The tiled memcpy doesn't work for copying from RGBX to RGBA because it > doesn't override the alpha component to 1.0. Commit 2cebaac479d4 added > a check to disable it for RGBX formats by looking at the TexFormat. > However a lot of the rest of the code base is written with the > assumption that an RGBA texture can be used internally to implement a > GL_RGB texture. If that is done then this check breaks. This patch > makes it instead check the base format of the texture which I think > more directly matches the intention. > > Cc: Jason Ekstrand I really don't know the corner cases well enough, but concept seems good to me. In particular, I assume this is now going to return false for more cases than previously - and that's okay, I guess? > --- > src/mesa/drivers/dri/i965/intel_pixel_read.c | 7 --- > src/mesa/drivers/dri/i965/intel_tex_image.c | 7 --- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/intel_pixel_read.c > b/src/mesa/drivers/dri/i965/intel_pixel_read.c > index 9bcbbd1..c8aef65 100644 > --- a/src/mesa/drivers/dri/i965/intel_pixel_read.c > +++ b/src/mesa/drivers/dri/i965/intel_pixel_read.c > @@ -135,10 +135,11 @@ intel_readpixels_tiled_memcpy(struct gl_context * ctx, >return false; > > /* We can't handle copying from RGBX or BGRX because the tiled_memcpy > -* function doesn't set the last channel to 1. > +* function doesn't set the last channel to 1. Note this checks BaseFormat > +* rather than TexFormat in case the RGBX format is being simulated with > an > +* RGBA format. > */ > - if (rb->Format == MESA_FORMAT_B8G8R8X8_UNORM || > - rb->Format == MESA_FORMAT_R8G8B8X8_UNORM) > + if (rb->_BaseFormat == GL_RGB) >return false; > > if (!intel_get_memcpy(rb->Format, format, type, &mem_copy, &cpp, > diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c > b/src/mesa/drivers/dri/i965/intel_tex_image.c > index 34b91e8..e3710da7 100644 > --- a/src/mesa/drivers/dri/i965/intel_tex_image.c > +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c > @@ -399,10 +399,11 @@ intel_gettexsubimage_tiled_memcpy(struct gl_context > *ctx, >return false; > > /* We can't handle copying from RGBX or BGRX because the tiled_memcpy > -* function doesn't set the last channel to 1. > +* function doesn't set the last channel to 1. Note this checks BaseFormat > +* rather than TexFormat in case the RGBX format is being simulated with > an > +* RGBA format. > */ > - if (texImage->TexFormat == MESA_FORMAT_B8G8R8X8_UNORM || > - texImage->TexFormat == MESA_FORMAT_R8G8B8X8_UNORM) > + if (texImage->_BaseFormat == GL_RGB) >return false; > > if (!intel_get_memcpy(texImage->TexFormat, format, type, &mem_copy, &cpp, > -- > 1.9.3 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/5] i965: Check base format to determine whether to use tiled memcpy
The tiled memcpy doesn't work for copying from RGBX to RGBA because it doesn't override the alpha component to 1.0. Commit 2cebaac479d4 added a check to disable it for RGBX formats by looking at the TexFormat. However a lot of the rest of the code base is written with the assumption that an RGBA texture can be used internally to implement a GL_RGB texture. If that is done then this check breaks. This patch makes it instead check the base format of the texture which I think more directly matches the intention. Cc: Jason Ekstrand --- src/mesa/drivers/dri/i965/intel_pixel_read.c | 7 --- src/mesa/drivers/dri/i965/intel_tex_image.c | 7 --- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/mesa/drivers/dri/i965/intel_pixel_read.c b/src/mesa/drivers/dri/i965/intel_pixel_read.c index 9bcbbd1..c8aef65 100644 --- a/src/mesa/drivers/dri/i965/intel_pixel_read.c +++ b/src/mesa/drivers/dri/i965/intel_pixel_read.c @@ -135,10 +135,11 @@ intel_readpixels_tiled_memcpy(struct gl_context * ctx, return false; /* We can't handle copying from RGBX or BGRX because the tiled_memcpy -* function doesn't set the last channel to 1. +* function doesn't set the last channel to 1. Note this checks BaseFormat +* rather than TexFormat in case the RGBX format is being simulated with an +* RGBA format. */ - if (rb->Format == MESA_FORMAT_B8G8R8X8_UNORM || - rb->Format == MESA_FORMAT_R8G8B8X8_UNORM) + if (rb->_BaseFormat == GL_RGB) return false; if (!intel_get_memcpy(rb->Format, format, type, &mem_copy, &cpp, diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c index 34b91e8..e3710da7 100644 --- a/src/mesa/drivers/dri/i965/intel_tex_image.c +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c @@ -399,10 +399,11 @@ intel_gettexsubimage_tiled_memcpy(struct gl_context *ctx, return false; /* We can't handle copying from RGBX or BGRX because the tiled_memcpy -* function doesn't set the last channel to 1. +* function doesn't set the last channel to 1. Note this checks BaseFormat +* rather than TexFormat in case the RGBX format is being simulated with an +* RGBA format. */ - if (texImage->TexFormat == MESA_FORMAT_B8G8R8X8_UNORM || - texImage->TexFormat == MESA_FORMAT_R8G8B8X8_UNORM) + if (texImage->_BaseFormat == GL_RGB) return false; if (!intel_get_memcpy(texImage->TexFormat, format, type, &mem_copy, &cpp, -- 1.9.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev