<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Sat, Apr 23, 2016 at 8:35 AM, Pohjolainen, Topi <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>> wrote: <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 Fri, Apr 22, 2016 at 04:19:10PM -0700, Jason Ekstrand wrote: > They didn't really add anything other than a key and extra layers of > function calls. This commit just inlines the extra functions and gets rid > of the extra classes. > --- > src/mesa/drivers/dri/i965/brw_blorp_clear.cpp | 224 ++++++++++---------------- > 1 file changed, 87 insertions(+), 137 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp > index c7e57bc..b16102c 100644 > --- a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp > +++ b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp > @@ -44,43 +44,6 @@ struct brw_blorp_const_color_prog_key > bool pad[3]; > }; > > -/** > - * Parameters for a blorp operation where the fragment shader outputs a > - * constant color. This is used for both fast color clears and color > - * resolves. > - */ > -class brw_blorp_const_color_params : public brw_blorp_params > -{ > -public: > - brw_blorp_const_color_prog_key wm_prog_key; > -}; > - > -class brw_blorp_clear_params : public brw_blorp_const_color_params > -{ > -public: > - brw_blorp_clear_params(struct brw_context *brw, > - struct gl_framebuffer *fb, > - struct gl_renderbuffer *rb, > - GLubyte *color_mask, > - bool partial_clear, > - bool encode_srgb, > - unsigned layer); > -}; > - > - > -/** > - * Parameters for a blorp operation that performs a "render target resolve". > - * This is used to resolve pending fast clear pixels before a color buffer is > - * used for texturing, ReadPixels, or scanout. > - */ > -class brw_blorp_rt_resolve_params : public brw_blorp_const_color_params > -{ > -public: > - brw_blorp_rt_resolve_params(struct brw_context *brw, > - struct intel_mipmap_tree *mt); > -}; > - > - > class brw_blorp_const_color_program > { > public: > @@ -151,102 +114,6 @@ brw_blorp_params_get_clear_kernel(struct brw_context *brw, > } > } > > -brw_blorp_clear_params::brw_blorp_clear_params(struct brw_context *brw, > - struct gl_framebuffer *fb, > - struct gl_renderbuffer *rb, > - GLubyte *color_mask, > - bool partial_clear, > - bool encode_srgb, > - unsigned layer) </div></div>I would have kept this similarly you changed the default constructors to init() functions (do_single_blorp_clear() becomes way over 100 lines long). Maybe static void set_clear_params(struct brw_context *brw, struct gl_framebuffer *fb, struct gl_renderbuffer *rb, bool partial_clear, bool encode_srgb, unsigned layer) { const GLubyte *color_mask = brw->ctx.Color.ColorMask[buf]; But I don't care too much. I suppose you have further plans for this anyway. </blockquote><div> </div><div>Right. My thinking was mostly that we had a bunch of logic for handling different cases and half of it was in the constructor and half was in do_single_blorp_clear and there didn't seem to be a particularly good reason why any given piece was in one or the other. So I figured it't be easier to see what was going on if they were all together. It may be worth splitting it back out again in the future. </div><div>Also, one of the things I hope to do eventually is to start using ISL internally in blorp. If we do this, we'll have to divide the logic back up but the lines will be drawn differently. </div><div>If you want it kept split up, I can do that but that's my reasoning for just cramming it together. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > -{ > - struct gl_context *ctx = &brw->ctx; > - struct intel_renderbuffer *irb = intel_renderbuffer(rb); > - mesa_format format = irb->mt->format; > - > - if (!encode_srgb && _mesa_get_format_color_encoding(format) == GL_SRGB) > - format = _mesa_get_srgb_format_linear(format); > - > - dst.set(brw, irb->mt, irb->mt_level, layer, format, true); > - > - /* Override the surface format according to the context's sRGB rules. */ > - dst.brw_surfaceformat = brw->render_target_format[format]; > - > - x0 = fb->_Xmin; > - x1 = fb->_Xmax; > - if (rb->Name != 0) { > - y0 = fb->_Ymin; > - y1 = fb->_Ymax; > - } else { > - y0 = rb->Height - fb->_Ymax; > - y1 = rb->Height - fb->_Ymin; > - } > - > - memcpy(&wm_push_consts.dst_x0, ctx->Color.ClearColor.f, sizeof(float) * 4); > - > - memset(&wm_prog_key, 0, sizeof(wm_prog_key)); > - > - wm_prog_key.use_simd16_replicated_data = true; > - > - /* From the SNB PRM (Vol4_Part1): > - * > - * "Replicated data (Message Type = 111) is only supported when > - * accessing tiled memory. Using this Message Type to access linear > - * (untiled) memory is UNDEFINED." > - */ > - if (irb->mt->tiling == I915_TILING_NONE) > - wm_prog_key.use_simd16_replicated_data = false; > - > - /* Constant color writes ignore everyting in blend and color calculator > - * state. This is not documented. > - */ > - for (int i = 0; i < 4; i++) { > - if (_mesa_format_has_color_component(irb->mt->format, i) && > - !color_mask[i]) { > - color_write_disable[i] = true; > - wm_prog_key.use_simd16_replicated_data = false; > - } > - } > - > - if (irb->mt->fast_clear_state != INTEL_FAST_CLEAR_STATE_NO_MCS && > - !partial_clear && wm_prog_key.use_simd16_replicated_data && > - brw_is_color_fast_clear_compatible(brw, irb->mt, > - &ctx->Color.ClearColor)) { > - memset(&wm_push_consts, 0xff, 4*sizeof(float)); > - fast_clear_op = GEN7_PS_RENDER_TARGET_FAST_CLEAR_ENABLE; > - > - brw_get_fast_clear_rect(brw, fb, irb->mt, &x0, &y0, &x1, &y1); > - } else { > - brw_meta_get_buffer_rect(fb, &x0, &y0, &x1, &y1); > - } > - > - brw_blorp_params_get_clear_kernel(brw, this, &wm_prog_key); > -} > - > - > -brw_blorp_rt_resolve_params::brw_blorp_rt_resolve_params( > - struct brw_context *brw, > - struct intel_mipmap_tree *mt) > -{ > - const mesa_format format = _mesa_get_srgb_format_linear(mt->format); > - > - dst.set(brw, mt, 0 /* level */, 0 /* layer */, format, true); > - > - brw_get_resolve_rect(brw, mt, &x0, &y0, &x1, &y1); > - > - fast_clear_op = GEN7_PS_RENDER_TARGET_RESOLVE_ENABLE; > - > - /* Note: there is no need to initialize push constants because it doesn't > - * matter what data gets dispatched to the render target. However, we must > - * ensure that the fragment shader delivers the data using the "replicated > - * color" message. > - */ > - memset(&wm_prog_key, 0, sizeof(wm_prog_key)); > - wm_prog_key.use_simd16_replicated_data = true; > - > - brw_blorp_params_get_clear_kernel(brw, this, &wm_prog_key); > -} > - > - > void > brw_blorp_const_color_program::alloc_regs() > { > @@ -339,9 +206,71 @@ do_single_blorp_clear(struct brw_context *brw, struct gl_framebuffer *fb, > { > struct gl_context *ctx = &brw->ctx; > struct intel_renderbuffer *irb = intel_renderbuffer(rb); > + mesa_format format = irb->mt->format; > + > + brw_blorp_params params; > + > + if (!encode_srgb && _mesa_get_format_color_encoding(format) == GL_SRGB) > + format = _mesa_get_srgb_format_linear(format); > + > + params.dst.set(brw, irb->mt, irb->mt_level, layer, format, true); > + > + /* Override the surface format according to the context's sRGB rules. */ > + params.dst.brw_surfaceformat = brw->render_target_format[format]; > + > + params.x0 = fb->_Xmin; > + params.x1 = fb->_Xmax; > + if (rb->Name != 0) { > + params.y0 = fb->_Ymin; > + params.y1 = fb->_Ymax; > + } else { > + params.y0 = rb->Height - fb->_Ymax; > + params.y1 = rb->Height - fb->_Ymin; > + } > + > + memcpy(&params.wm_push_consts.dst_x0, > + ctx->Color.ClearColor.f, sizeof(float) * 4); > + > + brw_blorp_const_color_prog_key wm_prog_key; > + memset(&wm_prog_key, 0, sizeof(wm_prog_key)); > + > + wm_prog_key.use_simd16_replicated_data = true; > + > + /* From the SNB PRM (Vol4_Part1): > + * > + * "Replicated data (Message Type = 111) is only supported when > + * accessing tiled memory. Using this Message Type to access linear > + * (untiled) memory is UNDEFINED." > + */ > + if (irb->mt->tiling == I915_TILING_NONE) > + wm_prog_key.use_simd16_replicated_data = false; > + > + /* Constant color writes ignore everyting in blend and color calculator > + * state. This is not documented. > + */ > + for (int i = 0; i < 4; i++) { > + if (_mesa_format_has_color_component(irb->mt->format, i) && > + !ctx->Color.ColorMask[buf][i]) { > + params.color_write_disable[i] = true; > + wm_prog_key.use_simd16_replicated_data = false; > + } > + } </div></div>Just a heads up, you need a little rebase here. I pushed the RGBX fix, and this now rests in its own function. <div><div class="h5"> > + > + if (irb->mt->fast_clear_state != INTEL_FAST_CLEAR_STATE_NO_MCS && > + !partial_clear && wm_prog_key.use_simd16_replicated_data && > + brw_is_color_fast_clear_compatible(brw, irb->mt, > + &ctx->Color.ClearColor)) { > + memset(&params.wm_push_consts, 0xff, 4*sizeof(float)); > + params.fast_clear_op = GEN7_PS_RENDER_TARGET_FAST_CLEAR_ENABLE; > + > + brw_get_fast_clear_rect(brw, fb, irb->mt, &params.x0, &params.y0, > + &params.x1, &params.y1); > + } else { > + brw_meta_get_buffer_rect(fb, &params.x0, &params.y0, > + &params.x1, &params.y1); > + } > > - brw_blorp_clear_params params(brw, fb, rb, ctx->Color.ColorMask[buf], > - partial_clear, encode_srgb, layer); > + brw_blorp_params_get_clear_kernel(brw, &params, &wm_prog_key); > > const bool is_fast_clear = > params.fast_clear_op == GEN7_PS_RENDER_TARGET_FAST_CLEAR_ENABLE; > @@ -375,7 +304,7 @@ do_single_blorp_clear(struct brw_context *brw, struct gl_framebuffer *fb, > const char *clear_type; > if (is_fast_clear) > clear_type = "fast"; > - else if (params.wm_prog_key.use_simd16_replicated_data) > + else if (wm_prog_key.use_simd16_replicated_data) > clear_type = "replicated"; > else > clear_type = "slow"; > @@ -448,7 +377,28 @@ brw_blorp_resolve_color(struct brw_context *brw, struct intel_mipmap_tree *mt) > { > DBG("%s to mt %p\n", __FUNCTION__, mt); > > - brw_blorp_rt_resolve_params params(brw, mt); > + const mesa_format format = _mesa_get_srgb_format_linear(mt->format); > + > + brw_blorp_params params; > + > + params.dst.set(brw, mt, 0 /* level */, 0 /* layer */, format, true); > + > + brw_get_resolve_rect(brw, mt, &params.x0, &params.y0, > + &params.x1, &params.y1); > + > + params.fast_clear_op = GEN7_PS_RENDER_TARGET_RESOLVE_ENABLE; > + > + /* Note: there is no need to initialize push constants because it doesn't > + * matter what data gets dispatched to the render target. However, we must > + * ensure that the fragment shader delivers the data using the "replicated > + * color" message. > + */ > + brw_blorp_const_color_prog_key wm_prog_key; > + memset(&wm_prog_key, 0, sizeof(wm_prog_key)); > + wm_prog_key.use_simd16_replicated_data = true; > + > + brw_blorp_params_get_clear_kernel(brw, &params, &wm_prog_key); > + > brw_blorp_exec(brw, &params); > mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_RESOLVED; > } > -- > 2.5.0.400.gff86faf > </div></div>> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>