<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Nov 21, 2017 at 3:05 PM, Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed 08 Nov 2017, Jason Ekstrand wrote: > The old code made a new miptree that referenced the same BO as the > renderbuffer and just trusted in the memory aliasing to work. There are > only two ways in which the new miptree is liable to differ from the one > in the renderbuffer and neither of them matter: Ouch. I didn't realize intelSetTexBuffer2 created a new miptree, but I'm not surprised. <div><div class="h5"> > > 1) It may have a different target. The only targets that we can ever > see in intelSetTexBuffer2 are GL_TEXTURE_2D and GL_TEXTURE_RECTANGLE > and the difference between the two doesn't matter as far as the > miptree is concerned; genX(update_sampler_state) only looks at the > gl_texture_object and not the miptree when determining whether or > not to use normalized coordinates. > > 2) It may have a very slightly different format. Again, this doesn't > matter because we've supported texture views for quite some time so > we always look at the gl_texture_object format instead of the > miptree format for hardware setup anyway. > > On the other hand, because we were recreating the miptree, we were using > intel_miptree_create_for_bo which doesn't understand modifiers. We > really want this function to work without doing a resolve so long as you > have modifiers so we need to fix that. > > Cc: "17.3" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>> > --- > src/mesa/drivers/dri/i965/intel_tex_image.c | 21 +++++++-------------- > 1 file changed, 7 insertions(+), 14 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c > index 37c8e24..c52992a 100644 > --- a/src/mesa/drivers/dri/i965/intel_tex_image.c > +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c > @@ -405,6 +405,7 @@ static void > intel_set_texture_image_mt(struct brw_context *brw, > struct gl_texture_image *image, > GLenum internal_format, > + mesa_format format, > struct intel_mipmap_tree *mt) > > { > @@ -415,7 +416,7 @@ intel_set_texture_image_mt(struct brw_context *brw, > _mesa_init_teximage_fields(&brw->ctx, image, > mt->surf.logical_level0_px.width, > mt->surf.logical_level0_px.height, 1, > - 0, internal_format, mt->format); > + 0, internal_format, format); > > brw->ctx.Driver.FreeTextureImageBuffer(&brw->ctx, image); > > @@ -442,7 +443,6 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target, > struct gl_texture_object *texObj; > struct gl_texture_image *texImage; > mesa_format texFormat = MESA_FORMAT_NONE; > - struct intel_mipmap_tree *mt; > GLenum internal_format = 0; > > texObj = _mesa_get_current_tex_object(ctx, target); > @@ -464,31 +464,24 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target, > if (rb->mt->cpp == 4) { > if (texture_format == __DRI_TEXTURE_FORMAT_RGB) { > internal_format = GL_RGB; > - texFormat = MESA_FORMAT_B8G8R8X8_UNORM; > + texFormat = MESA_FORMAT_B8G8R8A8_UNORM; </div></div>Why replace rgbx with rgba? I suspect the replace is due to the same reasons explained in intel_miptree_create_for_dri_image(). Whatever the reasons are, they're subtle and deserve a comment. </blockquote><div> </div><div>I believe your fears go away if you re-order things and put 3 before 2. Why RGBA instead of RGBX? Because the underlying miptree of the renderbuffer is likely to have that format. That said, it's not actually guaranteed so making that change in this patch is a bit bogus. If we just make the change in 2 instead, I believe all bogosity is gone. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> For reference, here is the rgbx->rgba replacement in intel_miptree_create_for_dri_image: if (!brw->ctx.TextureFormatSupported[format]) { /* The texture storage paths in core Mesa detect if the driver does not * support the user-requested format, and then searches for a * fallback format. The DRIimage code bypasses core Mesa, though. So we * do the fallbacks here for important formats. * * We must support DRM_FOURCC_XBGR8888 textures because the Android * framework produces HAL_PIXEL_FORMAT_RGBX8888 winsys surfaces, which * the Chrome OS compositor consumes as dma_buf EGLImages. */ format = _mesa_format_fallback_rgbx_to_rgba(format); <div><div class="h5"> } > } > else { > internal_format = GL_RGBA; > texFormat = MESA_FORMAT_B8G8R8A8_UNORM; > } > } else if (rb->mt->cpp == 2) { > + /* This is 565 */ > internal_format = GL_RGB; > texFormat = MESA_FORMAT_B5G6R5_UNORM; > } > > intel_miptree_make_shareable(brw, rb->mt); > - mt = intel_miptree_create_for_bo(brw, rb->mt->bo, texFormat, 0, > - rb->Base.Base.Width, > - rb->Base.Base.Height, > - 1, rb->mt->surf.row_pitch, > - MIPTREE_CREATE_DEFAULT); > - if (mt == NULL) > - return; > - mt->target = target; > > _mesa_lock_texture(&brw->ctx, texObj); > texImage = _mesa_get_tex_image(ctx, texObj, target, 0); > - intel_set_texture_image_mt(brw, texImage, internal_format, mt); > - intel_miptree_release(&mt); > + intel_set_texture_image_mt(brw, texImage, internal_format, > + texFormat, rb->mt); > _mesa_unlock_texture(&brw->ctx, texObj); > } > > @@ -581,7 +574,7 @@ intel_image_target_texture_2d(struct gl_context *ctx, GLenum target, > const GLenum internal_format = > image->internal_format != 0 ? > image->internal_format : _mesa_get_format_base_format(mt->format); > - intel_set_texture_image_mt(brw, texImage, internal_format, mt); > + intel_set_texture_image_mt(brw, texImage, internal_format, mt->format, mt); </div></div>This hunk looks correct. mt->format is the only suitable format available in this function. > intel_miptree_release(&mt); > } > > -- > 2.5.0.400.gff86faf > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>