[Mesa-dev] [PATCH 2/7] intel: Move BCS_SWCTRL setup to a helper function.

[Kenneth Graunke]

On 06/06/2013 09:59 AM, Eric Anholt wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
>
>> On 06/05/2013 10:14 AM, Eric Anholt wrote:
>>> This will get reused shortly.
>>> ---
>>>    src/mesa/drivers/dri/intel/intel_blit.c | 70 +++++++++++++++++----------------
>>>    1 file changed, 36 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/intel/intel_blit.c b/src/mesa/drivers/dri/intel/intel_blit.c
>>> index 1f6ad09..70d6dcc 100644
>>> --- a/src/mesa/drivers/dri/intel/intel_blit.c
>>> +++ b/src/mesa/drivers/dri/intel/intel_blit.c
>>> @@ -86,6 +86,33 @@ br13_for_cpp(int cpp)
>>>    }
>>>
>>>    /**
>>> + * Emits the packet for switching the blitter from X to Y tiled or back.
>>> + *
>>> + * The BATCH_BEGIN_BLT must already be started, since the pairs of this
>>> + * command emitted need to be in the same batchbuffer as the blit they're
>>> + * wrapping (otherwise, our state might leak out into another context, since
>>> + * BCS_SWCTRL is saved as part of the power context, not a render context).
>>> + */
>>> +static void
>>> +set_blitter_tiling(struct intel_context *intel,
>>> +                   bool dst_y_tiled, bool src_y_tiled)
>>> +{
>>> +   assert(intel->gen >= 6);
>>> +
>>> +   /* Idle the blitter before we update how tiling is interpreted. */
>>> +   OUT_BATCH(MI_FLUSH_DW);
>>> +   OUT_BATCH(0);
>>> +   OUT_BATCH(0);
>>> +   OUT_BATCH(0);
>>> +
>>> +   OUT_BATCH(MI_LOAD_REGISTER_IMM | (3 - 2));
>>> +   OUT_BATCH(BCS_SWCTRL);
>>> +   OUT_BATCH((BCS_SWCTRL_DST_Y | BCS_SWCTRL_SRC_Y) << 16 |
>>> +             (dst_y_tiled ? BCS_SWCTRL_DST_Y : 0) |
>>> +             (src_y_tiled ? BCS_SWCTRL_SRC_Y : 0));
>>> +}
>>> +
>>> +/**
>>>     * Implements a rectangular block transfer (blit) of pixels between two
>>>     * miptrees.
>>>     *
>>> @@ -204,21 +231,20 @@ intelEmitCopyBlit(struct intel_context *intel,
>>>       int dst_y2 = dst_y + h;
>>>       int dst_x2 = dst_x + w;
>>>       drm_intel_bo *aper_array[3];
>>> -   uint32_t bcs_swctrl = 0;
>>> +   bool dst_y_tiled = dst_tiling == I915_TILING_Y;
>>> +   bool src_y_tiled = src_tiling == I915_TILING_Y;
>>>       BATCH_LOCALS;
>>>
>>>       if (dst_tiling != I915_TILING_NONE) {
>>>          if (dst_offset & 4095)
>>>    	 return false;
>>> -      if (dst_tiling == I915_TILING_Y && intel->gen < 6)
>>> -	 return false;
>>>       }
>>>       if (src_tiling != I915_TILING_NONE) {
>>>          if (src_offset & 4095)
>>>    	 return false;
>>> -      if (src_tiling == I915_TILING_Y && intel->gen < 6)
>>> -	 return false;
>>>       }
>>> +   if ((dst_y_tiled || src_y_tiled) && intel->gen < 6)
>>> +      return false;
>>>
>>>       /* do space check before going any further */
>>>       do {
>>> @@ -284,16 +310,10 @@ intelEmitCopyBlit(struct intel_context *intel,
>>>       if (dst_tiling != I915_TILING_NONE) {
>>>          CMD |= XY_DST_TILED;
>>>          dst_pitch /= 4;
>>> -
>>> -      if (dst_tiling == I915_TILING_Y)
>>> -         bcs_swctrl |= BCS_SWCTRL_DST_Y;
>>>       }
>>>       if (src_tiling != I915_TILING_NONE) {
>>>          CMD |= XY_SRC_TILED;
>>>          src_pitch /= 4;
>>> -
>>> -      if (src_tiling == I915_TILING_Y)
>>> -         bcs_swctrl |= BCS_SWCTRL_SRC_Y;
>>>       }
>>>    #endif
>>>
>>> @@ -304,20 +324,10 @@ intelEmitCopyBlit(struct intel_context *intel,
>>>       assert(dst_x < dst_x2);
>>>       assert(dst_y < dst_y2);
>>>
>>> -   BEGIN_BATCH_BLT(8 + ((bcs_swctrl != 0) ? 14 : 0));
>>> +   BEGIN_BATCH_BLT(8 + ((dst_y_tiled || src_y_tiled) ? 14 : 0));
>>
>> I would much prefer to see set_blitter_tiling() include
>> BEGIN_BATCH_BLT(7)/ADVANCE_BATCH().  Then this could just be
>> BEGIN_BATCH_BLT(8).  That's much clearer: you can statically see that
>> each BEGIN/ADVANCE block has the right number of OUT_BATCH calls.
>>
>> Everything will still be within a single batch since it doesn't get
>> flushed until you start emitting render ring packets.
>
> Unless you run out of batch space between the setup and rendering, or
> rendering and restore.  In that case, you or somebody else will end up
> with bad tiling state, which is why I did it this way and commented that
> it was intentional in set_blitter_tiling().

So you did.  That's very unlikely to occur, but guarding against it is 
probably wise nonetheless.  Objection withdrawn.

--Ken