<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Feb 27, 2017 at 7:23 PM, Ben Widawsky <span dir="ltr"><<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 17-02-27 18:40:41, Jason Ekstrand wrote:<br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Mon, Feb 27, 2017 at 5:38 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>><br> wrote:<br> <br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Mon, Feb 27, 2017 at 4:56 PM, Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> wrote:<br> <br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On 17-01-31 13:24:55, Jason Ekstrand wrote:<br> <br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> wrote:<br> <br> v2: Put the commit message as a comment (Topi)<br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> Cc: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>><br> Cc: Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a><wbr>><br> Cc: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>><br> Signed-off-by: Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>><br> Acked-by: Daniel Stone <<a href="mailto:daniels@collabora.com" target="_blank">daniels@collabora.com</a>><br> ---<br> src/mesa/drivers/dri/i965/int<wbr>el_screen.c | 5 ++++-<br> 1 file changed, 4 insertions(+), 1 deletion(-)<br> <br> diff --git a/src/mesa/drivers/dri/i965/in<wbr>tel_screen.c<br> b/src/mesa/drivers/dri/i965/in<wbr>tel_screen.c<br> index 85070bb54d..12b3b071e4 100644<br> --- a/src/mesa/drivers/dri/i965/in<wbr>tel_screen.c<br> +++ b/src/mesa/drivers/dri/i965/in<wbr>tel_screen.c<br> @@ -1023,7 +1023,10 @@ intel_from_planar(__DRIimage *parent, int plane,<br> void *loaderPrivate)<br> if (parent == NULL || parent->planar_format == NULL) {<br> if (is_aux) {<br> offset = parent->aux_offset;<br> - stride = ALIGN(parent->pitch / 32, 128);<br> + /* Make CCS stride match kernel's expectations. Mesa's<br> internals<br> + * expect: stride = ALIGN(parent->pitch / 32, 128)<br> + */<br> + stride = ALIGN(parent->pitch / 64, 128);<br> <br> <br> </blockquote></blockquote></blockquote></blockquote> I just realized that this doesn't match the picture you drew in 22/32 where<br> the CCS and the main color surface have the same stride.<br> <br> </blockquote> <br></div></div> Here is the image, it seems correct to me. Is there some clarification you'd<br> like me to make?<br> <br> ┌─────────────────┐<br> │ │<br> │ │<br> │ │<br> │ Image │<br> │ │<br> │ │<br> │xxxxxxxxxxxxxxxxx│<br> ├─────┬───────────┘<br> │ │ |<br> │ccs │ unused |<br> └─────┘-----------┘<br> <------pitch------><span class=""><br></span></blockquote><div><br></div><div>This picture matches what you do for allocation. You divide the main surface height by 64 (rounded up) to get the CCS surface height in Y-tiled lines which is correct. Then you add this to the height of the primary surface and pass that to libdrm. The amount of data allocated for the ccs is then DIV_ROUND_UP(color.height, 64) * color.pitch. That is way more than enough data but the waste isn't too bad so whatever.<br><br></div><div>In patch 19, we use ISL to compute the CCS layout and it picks whatever stride it wants which, I believe, will be ALIGN(color.width, 1024) / 8. (Note this calculation is done based on the width of the color surface, not the stride)<br><br></div><div>Then, in this patch, we tell the client that the pitch is ALIGN(color.pitch / 64, 128). There is a factor of two in here thanks to the current kernel API, but it's also based on color.pitch not color.width so it's liable to mismatch with ISL.<br><br></div><div>We have three different calculations for the CCS pitch in three different places and no two of them match.<br><br></div><div>It's fine if the allocation doesn't match the others so long as it's it's large enough. The other two, however, need to match (with a possible factor of 2 difference).<br><br></div><div>Is that making sense?<br></div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Does the kernel expect the alignment to be 128 or 64? Given that ville<br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> likes 64-wide tiles, I think it should be 64. Really, I think the more<br> accurate calculation would be<br> <br> stride = ALIGN(parent->pitch, 4096) / 64;<br> <br> </blockquote> <br> </blockquote></blockquote> Isn't the actual CCS stride stored in parent->strides[0]? Why are we<br> re-computing it from the plane 0 pitch?<br> <br> </blockquote> <br></span> I believe the parent is just the parent image (ie. pixel data), in real planar<br> images, we have the planes for the formats, but it's somewhat of a hack job<br> here. If I have this wrong, or you have a better idea, please let me know.<div class="HOEnZb"><div class="h5"><br> <br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 4096 is the stride in bytes in the primary surface required to cross a<br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> single CCS tile. The calculation you have above will work in the sense<br> that the worst that happens is for it to align up a bit too far. In any<br> case, what matters is that we a) have enough space and b) exactly match<br> the<br> kernel's calculation.<br> <br> --Jason<br> <br> <br> <br> </blockquote> This formula doesn't match the kernel's expectations (unless Ville has<br> updated<br> patches somewhere).<br> <br> </blockquote> <br> Hrm... Ok. Having the alignment too big won't break anything, it's just a<br> bit odd.<br> <br> <br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> If you want I can do:<br> stride = ALIGN(parent->pitch, 4096) / 128;<br> </blockquote> <br> <br> That' wouldn't match either. Let's make sure the formulas match exactly.<br> <br> <br> </blockquote></blockquote> </div></div></blockquote></div><br></div></div>