[Mesa-dev] [PATCH v2 3/5] i965/clear: Remove an early return in fast_clear_depth
Reduce complexity and allow the next patch to delete some code. With this change, clear operations will still be skipped and setting the aux_state will cause no side-effects. Remove the associated comment which implies an early return. --- src/mesa/drivers/dri/i965/brw_clear.c | 5 - 1 file changed, 5 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_clear.c b/src/mesa/drivers/dri/i965/brw_clear.c index fdc31cd9b68..6521141d7f6 100644 --- a/src/mesa/drivers/dri/i965/brw_clear.c +++ b/src/mesa/drivers/dri/i965/brw_clear.c @@ -230,10 +230,6 @@ brw_fast_clear_depth(struct gl_context *ctx) } if (!need_clear) { - /* If all of the layers we intend to clear are already in the clear - * state then simply updating the miptree fast clear value is sufficient - * to change their clear value. - */ if (!same_clear_value) { /* BLORP updates the indirect clear color buffer when performing a * fast clear. Since we are skipping the fast clear here, we need to @@ -241,7 +237,6 @@ brw_fast_clear_depth(struct gl_context *ctx) */ intel_miptree_update_indirect_color(brw, mt); } - return true; } for (unsigned a = 0; a < num_layers; a++) { -- 2.16.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/5] i965/clear: Remove an early return in fast_clear_depth
On Tue, Apr 24, 2018 at 05:48:44PM -0700, Nanley Chery wrote: > Reduce complexity and allow the next patch to delete some code. With > this change, clear operations will still be skipped and setting the > aux_state will cause no side-effects. It's going to skip the fast clear, but if I understood correctly it will call intel_miptree_set_aux_state(), which marks the BRW_NEW_AUX_STATE and will make all the surface state to be reemited. I'm not sure if there's something else that already triggers that, but if that wasn't the case already, maybe we are going to be emitting a lot more state now? > Remove the associated comment which implies an early return. > --- > src/mesa/drivers/dri/i965/brw_clear.c | 5 - > 1 file changed, 5 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c > b/src/mesa/drivers/dri/i965/brw_clear.c > index fdc31cd9b68..6521141d7f6 100644 > --- a/src/mesa/drivers/dri/i965/brw_clear.c > +++ b/src/mesa/drivers/dri/i965/brw_clear.c > @@ -230,10 +230,6 @@ brw_fast_clear_depth(struct gl_context *ctx) > } > > if (!need_clear) { > - /* If all of the layers we intend to clear are already in the clear > - * state then simply updating the miptree fast clear value is > sufficient > - * to change their clear value. > - */ >if (!same_clear_value) { > /* BLORP updates the indirect clear color buffer when performing a >* fast clear. Since we are skipping the fast clear here, we need to > @@ -241,7 +237,6 @@ brw_fast_clear_depth(struct gl_context *ctx) >*/ > intel_miptree_update_indirect_color(brw, mt); >} > - return true; > } > > for (unsigned a = 0; a < num_layers; a++) { > -- > 2.16.2 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/5] i965/clear: Remove an early return in fast_clear_depth
On Wed, Apr 25, 2018 at 11:30:14AM -0700, Rafael Antognolli wrote: > On Tue, Apr 24, 2018 at 05:48:44PM -0700, Nanley Chery wrote: > > Reduce complexity and allow the next patch to delete some code. With > > this change, clear operations will still be skipped and setting the > > aux_state will cause no side-effects. > > It's going to skip the fast clear, but if I understood correctly it will > call intel_miptree_set_aux_state(), which marks the BRW_NEW_AUX_STATE > and will make all the surface state to be reemited. > > I'm not sure if there's something else that already triggers that, but > if that wasn't the case already, maybe we are going to be emitting a lot > more state now? > The surface state won't be re-emitted. intel_miptree_set_aux_state() will only mark BRW_NEW_AUX_STATE if the new aux state differs from the current one. In the case where we can skip the fast clear, the current and the new states will both equal ISL_AUX_STATE_CLEAR. -Nanley > > Remove the associated comment which implies an early return. > > --- > > src/mesa/drivers/dri/i965/brw_clear.c | 5 - > > 1 file changed, 5 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c > > b/src/mesa/drivers/dri/i965/brw_clear.c > > index fdc31cd9b68..6521141d7f6 100644 > > --- a/src/mesa/drivers/dri/i965/brw_clear.c > > +++ b/src/mesa/drivers/dri/i965/brw_clear.c > > @@ -230,10 +230,6 @@ brw_fast_clear_depth(struct gl_context *ctx) > > } > > > > if (!need_clear) { > > - /* If all of the layers we intend to clear are already in the clear > > - * state then simply updating the miptree fast clear value is > > sufficient > > - * to change their clear value. > > - */ > >if (!same_clear_value) { > > /* BLORP updates the indirect clear color buffer when performing a > >* fast clear. Since we are skipping the fast clear here, we need > > to > > @@ -241,7 +237,6 @@ brw_fast_clear_depth(struct gl_context *ctx) > >*/ > > intel_miptree_update_indirect_color(brw, mt); > >} > > - return true; > > } > > > > for (unsigned a = 0; a < num_layers; a++) { > > -- > > 2.16.2 > > > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/5] i965/clear: Remove an early return in fast_clear_depth
On Wed, Apr 25, 2018 at 11:40:15AM -0700, Nanley Chery wrote: > On Wed, Apr 25, 2018 at 11:30:14AM -0700, Rafael Antognolli wrote: > > On Tue, Apr 24, 2018 at 05:48:44PM -0700, Nanley Chery wrote: > > > Reduce complexity and allow the next patch to delete some code. With > > > this change, clear operations will still be skipped and setting the > > > aux_state will cause no side-effects. > > > > It's going to skip the fast clear, but if I understood correctly it will > > call intel_miptree_set_aux_state(), which marks the BRW_NEW_AUX_STATE > > and will make all the surface state to be reemited. > > > > I'm not sure if there's something else that already triggers that, but > > if that wasn't the case already, maybe we are going to be emitting a lot > > more state now? > > > > The surface state won't be re-emitted. intel_miptree_set_aux_state() > will only mark BRW_NEW_AUX_STATE if the new aux state differs from the > current one. In the case where we can skip the fast clear, the current > and the new states will both equal ISL_AUX_STATE_CLEAR. Ouch, you are right, I missed the big "if" there. In this case, this patch is Reviewed-by: Rafael Antognolli > > > Remove the associated comment which implies an early return. > > > --- > > > src/mesa/drivers/dri/i965/brw_clear.c | 5 - > > > 1 file changed, 5 deletions(-) > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c > > > b/src/mesa/drivers/dri/i965/brw_clear.c > > > index fdc31cd9b68..6521141d7f6 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_clear.c > > > +++ b/src/mesa/drivers/dri/i965/brw_clear.c > > > @@ -230,10 +230,6 @@ brw_fast_clear_depth(struct gl_context *ctx) > > > } > > > > > > if (!need_clear) { > > > - /* If all of the layers we intend to clear are already in the clear > > > - * state then simply updating the miptree fast clear value is > > > sufficient > > > - * to change their clear value. > > > - */ > > >if (!same_clear_value) { > > > /* BLORP updates the indirect clear color buffer when > > > performing a > > >* fast clear. Since we are skipping the fast clear here, we > > > need to > > > @@ -241,7 +237,6 @@ brw_fast_clear_depth(struct gl_context *ctx) > > >*/ > > > intel_miptree_update_indirect_color(brw, mt); > > >} > > > - return true; > > > } > > > > > > for (unsigned a = 0; a < num_layers; a++) { > > > -- > > > 2.16.2 > > > > > > ___ > > > mesa-dev mailing list > > > mesa-dev@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/5] i965/clear: Remove an early return in fast_clear_depth
On Wed, Apr 25, 2018 at 11:43:23AM -0700, Rafael Antognolli wrote: > On Wed, Apr 25, 2018 at 11:40:15AM -0700, Nanley Chery wrote: > > On Wed, Apr 25, 2018 at 11:30:14AM -0700, Rafael Antognolli wrote: > > > On Tue, Apr 24, 2018 at 05:48:44PM -0700, Nanley Chery wrote: > > > > Reduce complexity and allow the next patch to delete some code. With > > > > this change, clear operations will still be skipped and setting the > > > > aux_state will cause no side-effects. > > > > > > It's going to skip the fast clear, but if I understood correctly it will > > > call intel_miptree_set_aux_state(), which marks the BRW_NEW_AUX_STATE > > > and will make all the surface state to be reemited. > > > > > > I'm not sure if there's something else that already triggers that, but > > > if that wasn't the case already, maybe we are going to be emitting a lot > > > more state now? > > > > > > > The surface state won't be re-emitted. intel_miptree_set_aux_state() > > will only mark BRW_NEW_AUX_STATE if the new aux state differs from the > > current one. In the case where we can skip the fast clear, the current > > and the new states will both equal ISL_AUX_STATE_CLEAR. > > Ouch, you are right, I missed the big "if" there. In this case, this > patch is > > Reviewed-by: Rafael Antognolli > Thanks! > > > > Remove the associated comment which implies an early return. > > > > --- > > > > src/mesa/drivers/dri/i965/brw_clear.c | 5 - > > > > 1 file changed, 5 deletions(-) > > > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c > > > > b/src/mesa/drivers/dri/i965/brw_clear.c > > > > index fdc31cd9b68..6521141d7f6 100644 > > > > --- a/src/mesa/drivers/dri/i965/brw_clear.c > > > > +++ b/src/mesa/drivers/dri/i965/brw_clear.c > > > > @@ -230,10 +230,6 @@ brw_fast_clear_depth(struct gl_context *ctx) > > > > } > > > > > > > > if (!need_clear) { > > > > - /* If all of the layers we intend to clear are already in the > > > > clear > > > > - * state then simply updating the miptree fast clear value is > > > > sufficient > > > > - * to change their clear value. > > > > - */ > > > >if (!same_clear_value) { > > > > /* BLORP updates the indirect clear color buffer when > > > > performing a > > > >* fast clear. Since we are skipping the fast clear here, we > > > > need to > > > > @@ -241,7 +237,6 @@ brw_fast_clear_depth(struct gl_context *ctx) > > > >*/ > > > > intel_miptree_update_indirect_color(brw, mt); > > > >} > > > > - return true; > > > > } > > > > > > > > for (unsigned a = 0; a < num_layers; a++) { > > > > -- > > > > 2.16.2 > > > > > > > > ___ > > > > mesa-dev mailing list > > > > mesa-dev@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev