[Mesa-dev] [PATCH 2/2] i965/hsw: Apply non-msrt fast color clear w/a to all HSW GTs

[Kenneth Graunke]

On 11/26/2013 05:34 PM, Chad Versace wrote:
> Pre-patch, the workaround was applied to only HSW GT3. However, the
> workaround also fixes render corruption on the HSW GT1 Chromebook,
> codenamed Falco.
> 
> CC: Anuj Phogat <anuj.phogat at gmail.com>
> CC: Paul Berry <stereotype441 at gmail.com>
> OTC-Tracker: CHRMOS-812
> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_blorp_clear.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
> index 63d83d7..2620ce6 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
> @@ -265,7 +265,7 @@ brw_blorp_clear_params::brw_blorp_clear_params(struct brw_context *brw,
>        x_align *= 16;
>        y_align *= 32;
>  
> -      if (brw->is_haswell && brw->gt == 3) {
> +      if (brw->is_haswell) {
>           /* From BSpec: 3D-Media-GPGPU Engine > 3D Pipeline > Pixel > Pixel
>            * Backend > MCS Buffer for Render Target(s) [DevIVB+] > Table "Color
>            * Clear of Non-MultiSampled Render Target Restrictions":
> @@ -275,8 +275,8 @@ brw_blorp_clear_params::brw_blorp_clear_params(struct brw_context *brw,
>            *   y_align values computed above are the relevant entries in the
>            *   referred table.
>            *
> -          * Note: An older BSpec documented the above restriction for only
> -          * HSW GT3.
> +          * Note: An older BSpec documented the above restriction for only HSW
> +          * GT3. However, the restriction also fixes corruption on HSW GT1.
>            */
>           x0 = ROUND_DOWN_TO(x0, 2 * x_align);
>           y0 = ROUND_DOWN_TO(y0, 2 * y_align);
> 

Personally I would drop the "Note: An older BSpec documented the above
restriction for only HSW GT3." comment.  It wasn't in the public docs,
and the older internal docs you're talking to have been revised.  So I
don't think it'll be terribly interesting in the future.

I would be fine with applying this to all Gen7 platforms, as Paul
mentioned...it's documented as necessary, a minimal cost, and could save
headaches in the future.  But it may also be superfluous, so I'm okay
with your patch as is.

I would squash patches 1 and 2.  It makes sense to update the comments
saying "Hey, this applies more broadly" at the same time you fix the
code to apply it more broadly. :)

With or without any changes, both patches are:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Also I agree with Paul:
Cc: mesa-stable at lists.freedesktop.org