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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
