Re: [PATCH] drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Kenneth Graunke <kenneth@whitecape.org>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers.
Date: Thu, 5 May 2016 11:49:52 +0100	[thread overview]
Message-ID: <20160505104952.GM19266@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1462442809-1842-1-git-send-email-kenneth@whitecape.org>

On Thu, May 05, 2016 at 03:06:49AM -0700, Kenneth Graunke wrote:
> Allowing register copies where the source and destination are both
> whitelisted should be safe, and is useful.  For example, Mesa uses
> this to load the command streamer math registers with data from the
> pipeline statistics counters.
> 
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 69a1ba8..14f3b44 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -215,7 +215,8 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {

HWS RCS only. ✔

>  	CMD(  MI_RS_CONTEXT,                    SMI,    F,  1,      S  ),
>  	CMD(  MI_LOAD_SCAN_LINES_INCL,          SMI,   !F,  0x3F,   M  ),
>  	CMD(  MI_LOAD_SCAN_LINES_EXCL,          SMI,   !F,  0x3F,   R  ),
> -	CMD(  MI_LOAD_REGISTER_REG,             SMI,   !F,  0xFF,   R  ),
> +	CMD(  MI_LOAD_REGISTER_REG,             SMI,   !F,  0xFF,   W,
> +	      .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 1 }    ),

Bits 22:2 are the address, 0:1 mbz. ✔

offset=1, length=3, step=1 => check both src/dst against whitelist ✔

>  	CMD(  MI_RS_STORE_DATA_IMM,             SMI,   !F,  0xFF,   S  ),
>  	CMD(  MI_LOAD_URB_MEM,                  SMI,   !F,  0xFF,   S  ),
>  	CMD(  MI_STORE_URB_MEM,                 SMI,   !F,  0xFF,   S  ),
> @@ -1113,6 +1114,12 @@ static bool check_cmd(const struct intel_engine_cs *engine,
>  					return false;
>  				}
>  
> +				if (desc->cmd.value == MI_LOAD_REGISTER_REG) {
> +					DRM_DEBUG_DRIVER("CMD: Rejected LRR to masked register 0x%08X\n",
> +							 reg_addr);
> +					return false;
> +				}

Since we can't inspect the value to see if meets the permitted mask. ✔

The only thing missing on the checklist would be an igt to load, copy,
then store that register back to memory to check this works.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx