On Wed, May 27, 2015 at 10:42 AM, Ben Widawsky <b...@bwidawsk.net> wrote:
> On Thu, May 21, 2015 at 05:46:48PM -0700, Anuj Phogat wrote:
>> On Thu, May 21, 2015 at 12:26 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
>
> SNIP
>
>> > As I mentioned above wrt the CL alignment, I think it's safe to do for both
>> > modes. Something like:
>> >
>> > /* Check both are at least cacheline aligned */
>> > if (brw->gen >= 8 && (src_offset | dst_offset) & 63)
>> > return false;
>> >
>> We do this check for SRC_COPY_BLT in alignemnet_valid(). I added
>> the restrictions for FAST_COPY_BLT in fast_copy_blit_error_check().
>> Doing this check for both satisy all alignment restrictions of both modes
>> except one in fast copy blit: when src is untiled and dst is tiled, dst
>> should be 64 byte aligned and src should be 16 bytes aligned. See
>> fast_copy_blit_error_check().
>> But we're adding a restriction for it to be 64 byte by the suggested
>> change. Is there any advantage of the suggested check for both?
>>
>
> Oh hmm, I hadn't noticed a 16byte alignment requirement anywhere. I thought
> 64b
> was the minimum - if it's not ignore me.
>
I noticed 16 byte alignment requirement has been removed in an
update. I'll use your suggestion.
>> >> if ((dst_y_tiled || src_y_tiled) && brw->gen < 6)
>> >> return false;
>> >>
>> >> assert(!dst_y_tiled || (dst_pitch % 128) == 0);
>> >> assert(!src_y_tiled || (src_pitch % 128) == 0);
>> >>
>> >> + if (use_fast_copy_blit == false &&
>> >> + (src_tr_mode != INTEL_MIPTREE_TRMODE_NONE ||
>> >> + dst_tr_mode != INTEL_MIPTREE_TRMODE_NONE))
>> >> + return false;
>> >> +
>> >
>> > It seems like we should make it an error to use this improperly:
>> >
>> > /* It's an error for a caller to try to do a regular blit with a yf/ys
>> > region */
>> > assert(use_fast_copy_blit &&
>> > (src_tr_mode != INTEL_MIPTREE_TRMODE_NONE) &&
>> > (dst_tr_mode != INTEL_MIPTREE_TRMODE_NONE))
>> >
>> Agree.
>> >> /* do space check before going any further */
>> >> do {
>> >> aper_array[0] = brw->batch.bo;
>> >> @@ -334,13 +498,6 @@ intelEmitCopyBlit(struct brw_context *brw,
>> >> src_buffer, src_pitch, src_offset, src_x, src_y,
>> >> dst_buffer, dst_pitch, dst_offset, dst_x, dst_y, w, h);
>> >>
>> >> - /* Blit pitch must be dword-aligned. Otherwise, the hardware appears
>> >> to drop
>> >> - * the low bits. Offsets must be naturally aligned.
>> >> - */
>> >> - if (src_pitch % 4 != 0 || src_offset % cpp != 0 ||
>> >> - dst_pitch % 4 != 0 || dst_offset % cpp != 0)
>> >> - return false;
>> >> -
>> >> /* For big formats (such as floating point), do the copy using 16 or
>> >> 32bpp
>> >> * and multiply the coordinates.
>> >> */
>> >> @@ -359,27 +516,76 @@ intelEmitCopyBlit(struct brw_context *brw,
>> >> }
>> >> }
>> >>
>> >> - BR13 = br13_for_cpp(cpp) | translate_raster_op(logic_op) << 16;
>> >> + if (use_fast_copy_blit &&
>> >> + fast_copy_blit_error_check(src_offset, src_pitch, src_tiling,
>> >> + dst_offset, dst_pitch, dst_tiling,
>> >> cpp)) {
>> >> + BR13 = br13_for_cpp(cpp);
>> >>
>> >
>> > If you follow about you can remove the error check here as we know it's
>> > safe to
>> > proceed.
>> >
>> Right. As i understand you mean to move the fast_copy_blit_error_check()
>> inside
>> can_fast_copy_blit() helper ?
>>
>> >> - switch (cpp) {
>> >> - case 1:
>> >> - case 2:
>> >> - CMD = XY_SRC_COPY_BLT_CMD;
>> >> - break;
>> >> - case 4:
>> >> - CMD = XY_SRC_COPY_BLT_CMD | XY_BLT_WRITE_ALPHA | XY_BLT_WRITE_RGB;
>> >> - break;
>> >> - default:
>> >> - return false;
>> >> - }
>> >> + if (src_tr_mode == INTEL_MIPTREE_TRMODE_YF)
>> >> + BR13 |= XY_SRC_TRMODE_YF;
>> >>
>> >> - if (dst_tiling != I915_TILING_NONE) {
>> >> - CMD |= XY_DST_TILED;
>> >> - dst_pitch /= 4;
>> >> - }
>> >> - if (src_tiling != I915_TILING_NONE) {
>> >> - CMD |= XY_SRC_TILED;
>> >> - src_pitch /= 4;
>> >> + if (dst_tr_mode == INTEL_MIPTREE_TRMODE_YF)
>> >> + BR13 |= XY_DST_TRMODE_YF;
>> >> +
>> >> + CMD = XY_FAST_COPY_BLT_CMD;
>> >> +
>> >> + if (dst_tiling != I915_TILING_NONE) {
>> >> + SET_TILING_XY_FAST_COPY_BLT(dst_tiling, dst_tr_mode, XY_DST);
>> >> + /* Pitch value should be specified as a number of Dwords. */
>> >> + dst_pitch /= 4;
>> >> + }
>> >> + if (src_tiling != I915_TILING_NONE) {
>> >> + SET_TILING_XY_FAST_COPY_BLT(src_tiling, src_tr_mode, XY_SRC);
>> >> + /* Pitch value should be specified as a number of Dwords. */
>> >> + src_pitch /= 4;
>> >> + }
>> >> +
>> >> + CMD |= get_tr_horizontal_align(src_tr_mode, cpp, true /* is_src
>> >> */);
>> >> + CMD |= get_tr_vertical_align(src_tr_mode, cpp, true /* is_src */);
>> >> +
>> >> + CMD |= get_tr_horizontal_align(dst_tr_mode, cpp, false /* is_src
>> >> */);
>> >> + CMD |= get_tr_vertical_align(dst_tr_mode, cpp, false /* is_src */);
>> >> +
>> >> + } else {
>> >> + /* Source and destination base addresses should be 4 KB aligned. */
>> >> + if (dst_tiling != I915_TILING_NONE) {
>> >> + if (dst_offset & 4095)
>> >> + return false;
>> >> + }
>> >> + if (src_tiling != I915_TILING_NONE) {
>> >> + if (src_offset & 4095)
>> >> + return false;
>> >> + }
>> >> +
>> >> + /* Blit pitch must be dword-aligned. Otherwise, the hardware
>> >> appears to drop
>> >> + * the low bits. Offsets must be naturally aligned.
>> >> + */
>> >> + if (src_pitch % 4 != 0 || src_offset % cpp != 0 ||
>> >> + dst_pitch % 4 != 0 || dst_offset % cpp != 0)
>> >> + return false;
>> >> +
>> >> + assert(cpp <= 4);
>> >> + BR13 = br13_for_cpp(cpp) | translate_raster_op(logic_op) << 16;
>> >> + switch (cpp) {
>> >> + case 1:
>> >> + case 2:
>> >> + CMD = XY_SRC_COPY_BLT_CMD;
>> >> + break;
>> >> + case 4:
>> >> + CMD = XY_SRC_COPY_BLT_CMD | XY_BLT_WRITE_ALPHA |
>> >> XY_BLT_WRITE_RGB;
>> >> + break;
>> >> + default:
>> >> + return false;
>> >> + }
>> >> +
>> >> + if (dst_tiling != I915_TILING_NONE) {
>> >> + CMD |= XY_DST_TILED;
>> >> + dst_pitch /= 4;
>> >> + }
>> >> + if (src_tiling != I915_TILING_NONE) {
>> >> + CMD |= XY_SRC_TILED;
>> >> + src_pitch /= 4;
>> >> + }
>> >
>> > I'd be in favor of extracting the CMD out into a helper to cut down the
>> > size of
>> > this function. Up to you.
>> >
>> I'll try to make a helper for CMD.
>>
>> > CMD = get_copy_blit_cmd()
>> > BR_13 = br13_for_cpp(cpp)
>> > if (!fast_copy)
>> > BR_13 |= translate_raster_op(logic_op) << 16;
>> >
>> > I think also the pitch division is common to both.
>> >
>> >> }
>> >>
>> >> if (dst_y2 <= dst_y || dst_x2 <= dst_x) {
>> >> @@ -533,11 +739,14 @@ intel_emit_linear_blit(struct brw_context *brw,
>> >> pitch = ROUND_DOWN_TO(MIN2(size, (1 << 15) - 1), 4);
>> >> height = (pitch == 0) ? 1 : size / pitch;
>> >> ok = intelEmitCopyBlit(brw, 1,
>> >> - pitch, src_bo, src_offset, I915_TILING_NONE,
>> >> - pitch, dst_bo, dst_offset, I915_TILING_NONE,
>> >> + pitch, src_bo, src_offset,
>> >> + I915_TILING_NONE, INTEL_MIPTREE_TRMODE_NONE,
>> >> + pitch, dst_bo, dst_offset,
>> >> + I915_TILING_NONE, INTEL_MIPTREE_TRMODE_NONE,
>> >> 0, 0, /* src x/y */
>> >> 0, 0, /* dst x/y */
>> >> pitch, height, /* w, h */
>> >> + false, /* overlap */
>> >> GL_COPY);
>> >> if (!ok)
>> >> _mesa_problem(ctx, "Failed to linear blit %dx%d\n", pitch, height);
>> >> @@ -549,11 +758,14 @@ intel_emit_linear_blit(struct brw_context *brw,
>> >> pitch = ALIGN(size, 4);
>> >> if (size != 0) {
>> >> ok = intelEmitCopyBlit(brw, 1,
>> >> - pitch, src_bo, src_offset, I915_TILING_NONE,
>> >> - pitch, dst_bo, dst_offset, I915_TILING_NONE,
>> >> + pitch, src_bo, src_offset,
>> >> + I915_TILING_NONE, INTEL_MIPTREE_TRMODE_NONE,
>> >> + pitch, dst_bo, dst_offset,
>> >> + I915_TILING_NONE, INTEL_MIPTREE_TRMODE_NONE,
>> >> 0, 0, /* src x/y */
>> >> 0, 0, /* dst x/y */
>> >> size, 1, /* w, h */
>> >> + false, /* overlap */
>> >> GL_COPY);
>> >> if (!ok)
>> >> _mesa_problem(ctx, "Failed to linear blit %dx%d\n", size, 1);
>> >> diff --git a/src/mesa/drivers/dri/i965/intel_blit.h
>> >> b/src/mesa/drivers/dri/i965/intel_blit.h
>> >> index f563939..29c9ead 100644
>> >> --- a/src/mesa/drivers/dri/i965/intel_blit.h
>> >> +++ b/src/mesa/drivers/dri/i965/intel_blit.h
>> >> @@ -37,13 +37,16 @@ intelEmitCopyBlit(struct brw_context *brw,
>> >> drm_intel_bo *src_buffer,
>> >> GLuint src_offset,
>> >> uint32_t src_tiling,
>> >> + uint32_t src_tr_mode,
>> >> GLshort dst_pitch,
>> >> drm_intel_bo *dst_buffer,
>> >> GLuint dst_offset,
>> >> uint32_t dst_tiling,
>> >> + uint32_t dst_tr_mode,
>> >> GLshort srcx, GLshort srcy,
>> >> GLshort dstx, GLshort dsty,
>> >> GLshort w, GLshort h,
>> >> + bool overlap,
>> >> GLenum logicop );
>> >>
>> >> bool intel_miptree_blit(struct brw_context *brw,
>> >> diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
>> >> b/src/mesa/drivers/dri/i965/intel_copy_image.c
>> >> index f4c7eff..1e4a1e2 100644
>> >> --- a/src/mesa/drivers/dri/i965/intel_copy_image.c
>> >> +++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
>> >> @@ -126,12 +126,15 @@ copy_image_with_blitter(struct brw_context *brw,
>> >> src_mt->pitch,
>> >> src_mt->bo, src_mt->offset,
>> >> src_mt->tiling,
>> >> + src_mt->tr_mode,
>> >> dst_mt->pitch,
>> >> dst_mt->bo, dst_mt->offset,
>> >> dst_mt->tiling,
>> >> + dst_mt->tr_mode,
>> >> src_x, src_y,
>> >> dst_x, dst_y,
>> >> src_width, src_height,
>> >> + false /* overlap */,
>> >> GL_COPY);
>> >> }
>> >>
>> >> diff --git a/src/mesa/drivers/dri/i965/intel_reg.h
>> >> b/src/mesa/drivers/dri/i965/intel_reg.h
>> >> index 488fb5b..6830148 100644
>> >> --- a/src/mesa/drivers/dri/i965/intel_reg.h
>> >> +++ b/src/mesa/drivers/dri/i965/intel_reg.h
>> >> @@ -87,6 +87,8 @@
>> >>
>> >> #define XY_SRC_COPY_BLT_CMD (CMD_2D | (0x53 << 22))
>> >>
>> >> +#define XY_FAST_COPY_BLT_CMD (CMD_2D | (0x42 << 22))
>> >> +
>> >> #define XY_TEXT_IMMEDIATE_BLIT_CMD (CMD_2D | (0x31 << 22))
>> >> # define XY_TEXT_BYTE_PACKED (1 << 16)
>> >>
>> >> @@ -95,11 +97,42 @@
>> >> #define XY_BLT_WRITE_RGB (1 << 20)
>> >> #define XY_SRC_TILED (1 << 15)
>> >> #define XY_DST_TILED (1 << 11)
>> >> +#define XY_SRC_TILED_X (1 << 20)
>> >> +#define XY_SRC_TILED_Y (2 << 20)
>> >> +#define XY_SRC_TILED_64K (3 << 20)
>> >> +#define XY_DST_TILED_X (1 << 13)
>> >> +#define XY_DST_TILED_Y (2 << 13)
>> >> +#define XY_DST_TILED_64K (3 << 13)
>> >> +
>> >
>> > You've mixed bits of two commands. Name these something like XY_FAST_*.
>> > Put them
>> > in a separate chunk of code.
>> OK
>> > Also, I'm OCD, but I would have reversed the order here, highest value on
>> > top,
>> > lowest on the bottom
>> >
>> I'll follow that order.
>> >> +#define XY_SRC_H_ALIGN_32 (0 << 17)
>> >> +#define XY_SRC_H_ALIGN_64 (1 << 17)
>> >> +#define XY_SRC_H_ALIGN_128 (2 << 17)
>> >> +#define XY_SRC_H_ALIGN_256 (3 << 17)
>> >> +#define XY_SRC_H_ALIGN_512 (4 << 17)
>> >> +
>> >> +#define XY_SRC_V_ALIGN_64 (0 << 15)
>> >> +#define XY_SRC_V_ALIGN_128 (1 << 15)
>> >> +#define XY_SRC_V_ALIGN_256 (2 << 15)
>> >> +
>> >> +#define XY_DST_H_ALIGN_32 (0 << 10)
>> >> +#define XY_DST_H_ALIGN_64 (1 << 10)
>> >> +#define XY_DST_H_ALIGN_128 (2 << 10)
>> >> +#define XY_DST_H_ALIGN_256 (3 << 10)
>> >> +#define XY_DST_H_ALIGN_512 (4 << 10)
>> >> +
>> >> +#define XY_DST_V_ALIGN_64 (0 << 8)
>> >> +#define XY_DST_V_ALIGN_128 (1 << 8)
>> >> +#define XY_DST_V_ALIGN_256 (2 << 8)
>> >>
>> >> /* BR13 */
>> >> #define BR13_8 (0x0 << 24)
>> >> #define BR13_565 (0x1 << 24)
>> >> #define BR13_8888 (0x3 << 24)
>> >> +#define BR13_16161616 (0x4 << 24)
>> >> +#define BR13_32323232 (0x5 << 24)
>> >> +
>> >> +#define XY_SRC_TRMODE_YF (1 << 31)
>> >> +#define XY_DST_TRMODE_YF (1 << 30)
>> >>
>> >> /* Pipeline Statistics Counter Registers */
>> >> #define IA_VERTICES_COUNT 0x2310
>> >
>> > One thing I don't see addressed in this patch is
>> >
>> > "When two sequential fast copy blits have different source surfaces, but
>> > their
>> > destinations refer to the same destination surface and therefore
>> > destinations
>> > overlap, a Flush must be inserted between the two blits."
>> I didn't notice this. I'll take care of it in V3.
>
> Great.
>
>> >
>> >
>> > --
>> > Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
