<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Nov 1, 2017 at 9:24 AM, Tapani Pälli <span dir="ltr"><<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
On 11/01/2017 04:55 PM, Jason Ekstrand wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
On Wed, Nov 1, 2017 at 7:53 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a> <mailto:<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>>> wrote:<br>
<br>
On Fri, Oct 27, 2017 at 5:55 AM, Tapani Pälli<br></span><span class="">
<<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a> <mailto:<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a><wbr>>> wrote:<br>
<br>
Patch uses mem_ctx for allocation to ensure param array gets freed<br>
later, in blorp clear case this happens in<br>
blorp_params_get_clear_kernel.<br>
<br>
==6164== 48 bytes in 1 blocks are definitely lost in loss record<br>
61 of 193<br>
==6164== at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)<br>
==6164== by 0x12E31C6C: ralloc_size (ralloc.c:121)<br>
==6164== by 0x130189F1:<br>
fs_visitor::assign_constant_lo<wbr>cations() (brw_fs.cpp:2095)<br>
==6164== by 0x13022D32: fs_visitor::optimize() (brw_fs.cpp:5715)<br>
==6164== by 0x13024D5A: fs_visitor::run_fs(bool, bool)<br>
(brw_fs.cpp:6229)<br>
==6164== by 0x1302549A: brw_compile_fs (brw_fs.cpp:6570)<br>
==6164== by 0x130C4B07: blorp_compile_fs (blorp.c:194)<br>
==6164== by 0x130D384B: blorp_params_get_clear_kernel<br>
(blorp_clear.c:79)<br>
==6164== by 0x130D3C56: blorp_fast_clear (blorp_clear.c:332)<br>
==6164== by 0x12EFA439: do_single_blorp_clear (brw_blorp.c:1261)<br>
==6164== by 0x12EFC4AF: brw_blorp_clear_color (brw_blorp.c:1326)<br>
==6164== by 0x12EFF72B: brw_clear (brw_clear.c:297)<br>
<br>
Signed-off-by: Tapani Pälli <<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a><br></span>
<mailto:<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a><wbr>>><span class=""><br>
---<br>
src/intel/compiler/brw_fs.<wbr>cpp | 2 +-<br>
1 file changed, 1 insertion(+), 1 deletion(-)<br>
<br>
diff --git a/src/intel/compiler/brw_fs.cp<wbr>p<br>
b/src/intel/compiler/brw_fs.cp<wbr>p<br>
index 4616529abc..6b27c38be7 100644<br>
--- a/src/intel/compiler/brw_fs.cp<wbr>p<br>
+++ b/src/intel/compiler/brw_fs.cp<wbr>p<br>
@@ -2092,7 +2092,7 @@ fs_visitor::assign_constant_lo<wbr>cations()<br>
*/<br>
uint32_t *param = stage_prog_data->param;<br>
stage_prog_data->nr_params = num_push_constants;<br>
- stage_prog_data->param = ralloc_array(NULL, uint32_t,<br>
num_push_constants);<br>
+ stage_prog_data->param = ralloc_array(mem_ctx, uint32_t,<br>
num_push_constants);<br>
<br>
<br>
Wow, I don't know how I didn't see this pass. The more correct<br>
answer is that blorp no longer uses push constants, so we can just<br>
delete the whole mess. I'll send a patch.<br>
<br>
<br>
Gah! Ignore me. This is, indeed, correct.<br>
<br>
if (num_pull_constants > 0) {<br>
stage_prog_data->nr_pull_para<wbr>ms = num_pull_constants;<br>
stage_prog_data->pull_param = ralloc_array(NULL, uint32_t,<br>
<br>
<br>
We should be doing it here as well.<br>
<br>
</span></blockquote>
<br>
ok, did not catch that as the use case I was running did not use pull constants, I can send a separate fix for that one<font color="#888888">.</font></blockquote><div><br></div><div>As I said in the commit message of the patch I sent, it *almost* doesn't matter in practice. It only matters if shader compilation somehow fails which is not a common case. <br></div></div></div></div>