[Mesa-dev] [PATCH V5 7/7] intel: implement create image from texture

[Abdiel Janulgue]

On Tuesday, January 22, 2013 11:02:34 AM Eric Anholt wrote:

> 128 pixels of 32bpp is a tile width, and 128 pixels high of that is 16
> tiles.  The values I see this function having are:
> 
> mask_x = 127
> mask_y = 15
> draw_x = 0
> draw_y = 128
> image->offset = (16 * 4096)
> 
> So when we go to texture from it, we'll add 16 * 4096 to the offset, and
> then we'll... Aha!  We only make use of the intra-tile offsets (0, 0)
> there, and drop the inter-tile offset (0, 128).  So normal texturing
> won't double-count. 

> If you SubImage, the mt->offset is ignored in the
> mapping we do, which is broken for create_sub_image.
> 
> I think we should either
> 
> 1) Rename draw_x/draw_y in the image struct to tile_x/tile_y, set them
> to (draw_x & mask_x) and (draw_y & mask_y), 

Please correct me if I am wrong, but I was assuming the new helper function 
intel_miptree_get_tile_offsets() would have done the same result anyway when it 
calculates the tiled offsets in brw_update_texture_surface() ? 

Where  intel_miptree_get_tile_offsets() returns:

 tile_x = mt->level[level].slice[slice].x_offset & mask_x;
 tile_y = mt->level[level].slice[slice].y_offset & mask_y;

(the x_/y_offsets here comes from draw_x/y in the exported image struct)

I didn't notice the issue earlier because the code above resolved correctly 
back to the exported lower mip-levels in my tests. But I didn't consider the 
sub-image use case you mentioned. So of course there could still be a problem.

In any case, I think I've seen a similar solution in renderbuffer code where 
rendering to miplevels other than 0 just saves the pixel offsets in 
intel_renderbuffer_set_draw_offset()  and intel_renderbuffer_tile_offsets() 
returns the calculated tile offsets. But this still might have not been enough?

> and make
> intel_miptree_image_map() add the mt->offset to its returned pointers.

I'm not sure how to follow thru with this one. Do you mean inside 
intel_miptree_map_gtt() and append the mt->offset to the pointer there?

> 
> 2) make *_wm_surface_state also add the intel_region_get_aligned_offset
> to its offset and not set image->offset in this function.
> 
> I think I prefer 1).
> 
> > +/**
> > + * Sets up a DRIImage structure to point to our shared image in a region
> > + */
> > +static bool
> > +intel_setup_image_from_mipmap_tree(struct intel_context *intel,
> > __DRIimage *image, +                                   struct
> > intel_mipmap_tree *mt, GLuint level, +                                  
> > GLuint zoffset)
> > +{
> > +   bool has_surface_tile_offset = false;
> > +   uint32_t draw_x, draw_y;
> > +
> > +   intel_miptree_check_level_layer(mt, level, zoffset);
> > +   intel_miptree_get_tile_offsets(mt, level, zoffset, &draw_x, &draw_y);
> > +
> > +#ifndef I915
> > +   has_surface_tile_offset =
> > brw_context(&intel->ctx)->has_surface_tile_offset; +#endif
> > +   if ((!has_surface_tile_offset &&
> > +        (draw_x != 0 || draw_y != 0)) ||
> > +       mt->stencil_mt) {
> > +      /* Non-tile aligned sufaces in gen4 hw and earlier have problems
> > resolving +       * back to our destination due to alignment issues.
> > Bail-out and report error +       */
> > +      return false;
> 
> The mt->stencil_mt failure is not about tile alignment, it's about how
> we can't export that data through our current _DRIimage struct.

Ok. Maybe lumping it within the comment is wrong. I can move the mt-
>stencil_mt check outside before this setup function gets called.