On 9 May 2012 20:07, Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div>On Mon, 7 May 2012 14:31:51 -0700, Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> wrote: > This patch adds a new function, > drm_intel_bufmgr_gem_set_aub_annotations(), which can be used to > annotate the type and subtype of data stored in various sections of > each buffer. This data is used to populate type and subtype fields > when generating the .aub file, which improves the ability of later > debugging tools to analyze the contents of the .aub file. > > If drm_intel_bufmgr_gem_set_aub_annotations() is not called, then we > fall back to the old set of annotations (annotate the portion of the > batchbuffer that is executed as AUB_TRACE_TYPE_BATCH, and everything > else as AUB_TRACE_TYPE_NOTYPE). </div>This looks better than the interface I was thinking of. Only real nitpick note is that the style in this file is tab indents, rather than 8 spaces, same as linux kernel and 2d driver style. (I have since been convinced that 8 spaces is superior, but a mix is worse). </blockquote><div> Ok. Are there any plans to switch drm over to spaces or are we stuck with tabs forever? </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div> > /* > @@ -1989,23 +2027,31 @@ aub_exec(drm_intel_bo *bo, int ring_flag, int used) > drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr; > drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo; > int i; > + bool batch_buffer_needs_annotations; > > if (!bufmgr_gem->aub_file) > return; > > - /* Write out all but the batchbuffer to AUB memory */ > - for (i = 0; i < bufmgr_gem->exec_count - 1; i++) { > - if (bufmgr_gem->exec_bos[i] != bo) > - aub_write_bo(bufmgr_gem->exec_bos[i]); > + /* If batch buffer is not annotated, annotate it the best we > + * can. > + */ > + batch_buffer_needs_annotations = bo_gem->aub_annotation_count == 0; > + if (batch_buffer_needs_annotations) { > + drm_intel_aub_annotation annotations[2] = { > + { AUB_TRACE_TYPE_BATCH, 0, used }, > + { AUB_TRACE_TYPE_NOTYPE, 0, bo->size } > + }; > + drm_intel_bufmgr_gem_set_aub_annotations(bo, annotations, 2); </div>Looks like you don't actually need to be explicit about the range from used to bo->size given the "Write out any remaining unannotated data" code above, but either way. </blockquote><div> Fair point. I think I will leave the code as is just to avoid having to do another round of testing :) </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Assuming tab replacement: Reviewed-by: Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>> </blockquote></div>