Re: [PATCH 2/2] drm/vc4: Add exec flags to allow forcing a specific X/Y tile walk order.

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1: Type: text/plain, Size: 3731 bytes --]

Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> On Tue, 25 Jul 2017 09:27:33 -0700
> Eric Anholt <eric@anholt.net> wrote:
>
>> This is useful to allow GL to provide defined results for overlapping
>> glBlitFramebuffer, which X11 in turn uses to accelerate uncomposited
>> window movement without first blitting to a temporary.  x11perf
>> -copywinwin100 goes from 1850/sec to 4850/sec.
>> 
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>> 
>> The work-in-progress userspace is at:
>> 
>> https://github.com/anholt/xserver/commits/glamor-draw-bounds-overlap
>> https://github.com/anholt/mesa/commits/vc4-overlapping-blit
>> 
>> and the next step is to build the GL extension spec and piglit tests
>> for it.
>> 
>>  drivers/gpu/drm/vc4/vc4_drv.c       |  1 +
>>  drivers/gpu/drm/vc4/vc4_gem.c       |  5 ++++-
>>  drivers/gpu/drm/vc4/vc4_render_cl.c | 21 ++++++++++++++++-----
>>  include/uapi/drm/vc4_drm.h          | 11 +++++++++++
>>  4 files changed, 32 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
>> index c6b487c3d2b7..b5c2c28289ed 100644
>> --- a/drivers/gpu/drm/vc4/vc4_drv.c
>> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
>> @@ -99,6 +99,7 @@ static int vc4_get_param_ioctl(struct drm_device *dev, void *data,
>>  	case DRM_VC4_PARAM_SUPPORTS_BRANCHES:
>>  	case DRM_VC4_PARAM_SUPPORTS_ETC1:
>>  	case DRM_VC4_PARAM_SUPPORTS_THREADED_FS:
>> +	case DRM_VC4_PARAM_SUPPORTS_FIXED_RCL_ORDER:
>>  		args->value = true;
>>  		break;
>>  	default:
>> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
>> index a3e45e67f417..ba0782ebda34 100644
>> --- a/drivers/gpu/drm/vc4/vc4_gem.c
>> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
>> @@ -1008,7 +1008,10 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
>>  	struct ww_acquire_ctx acquire_ctx;
>>  	int ret = 0;
>>  
>> -	if ((args->flags & ~VC4_SUBMIT_CL_USE_CLEAR_COLOR) != 0) {
>> +	if ((args->flags & ~(VC4_SUBMIT_CL_USE_CLEAR_COLOR |
>> +			     VC4_SUBMIT_CL_FIXED_RCL_ORDER |
>> +			     VC4_SUBMIT_CL_RCL_ORDER_INCREASING_X |
>> +			     VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y)) != 0) {
>>  		DRM_DEBUG("Unknown flags: 0x%02x\n", args->flags);
>>  		return -EINVAL;
>>  	}
>> diff --git a/drivers/gpu/drm/vc4/vc4_render_cl.c b/drivers/gpu/drm/vc4/vc4_render_cl.c
>> index da3bfd53f0bd..c3b064052147 100644
>> --- a/drivers/gpu/drm/vc4/vc4_render_cl.c
>> +++ b/drivers/gpu/drm/vc4/vc4_render_cl.c
>> @@ -261,8 +261,17 @@ static int vc4_create_rcl_bo(struct drm_device *dev, struct vc4_exec_info *exec,
>>  	uint8_t max_y_tile = args->max_y_tile;
>>  	uint8_t xtiles = max_x_tile - min_x_tile + 1;
>>  	uint8_t ytiles = max_y_tile - min_y_tile + 1;
>> -	uint8_t x, y;
>> +	uint8_t xi, yi;
>>  	uint32_t size, loop_body_size;
>> +	bool positive_x = false;
>> +	bool positive_y = false;
>> +
>> +	if (args->flags & VC4_SUBMIT_CL_FIXED_RCL_ORDER) {
>> +		if (args->flags & VC4_SUBMIT_CL_RCL_ORDER_INCREASING_X)
>> +			positive_x = true;
>> +		if (args->flags & VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y)
>> +			positive_y = true;
>> +	}
>
> Are you sure you want the default value of positive_x/y to be false? It
> seems to me that before this patch you were always iterating in
> ascending order, but now, when VC4_SUBMIT_CL_FIXED_RCL_ORDER is not
> set you do the opposite. Maybe you really want to change the default
> behavior, just wanted to point this out.
>
> Otherwise,
>
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

I was undecided as well, but if you also thought it was funny to change
the default, that's convinced me to keep it the same.

Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]