[FOR 2.6.18 FIX][PATCH] drm: radeon flush TCL VAP for vertex program enable/disable

[FOR 2.6.18 FIX][PATCH] drm: radeon flush TCL VAP for vertex program enable/disable

[Dave Airlie]

Can we get this into 2.6.18? it fixes a lockup condition in r200 vertex 
programs.

From: Roland Scheidegger <rscheidegger_lists@hispeed.ch>

The radeon requires a VAP state flush when enabling/disabling
vertex programs on the r200 cards.

Signed-off-by: Dave Airlie <airlied@linux.ie>
---
  drivers/char/drm/radeon_state.c |    9 ++++++++-
  1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/char/drm/radeon_state.c b/drivers/char/drm/radeon_state.c
index 5bb2234..5a08a23 100644
--- a/drivers/char/drm/radeon_state.c
+++ b/drivers/char/drm/radeon_state.c
@@ -175,6 +175,14 @@ static __inline__ int radeon_check_and_f
  		}
  		break;

+	case R200_EMIT_VAP_CTL:{
+			RING_LOCALS;
+			BEGIN_RING(2);
+			OUT_RING_REG(RADEON_SE_TCL_STATE_FLUSH, 0);
+			ADVANCE_RING();
+		}
+		break;
+
  	case RADEON_EMIT_RB3D_COLORPITCH:
  	case RADEON_EMIT_RE_LINE_PATTERN:
  	case RADEON_EMIT_SE_LINE_WIDTH:
@@ -202,7 +210,6 @@ static __inline__ int radeon_check_and_f
  	case R200_EMIT_TCL_LIGHT_MODEL_CTL_0:
  	case R200_EMIT_TFACTOR_0:
  	case R200_EMIT_VTX_FMT_0:
-	case R200_EMIT_VAP_CTL:
  	case R200_EMIT_MATRIX_SELECT_0:
  	case R200_EMIT_TEX_PROC_CTL_2:
  	case R200_EMIT_TCL_UCP_VERT_BLEND_CTL:
-- 
1.4.1.gd3ba6

Re: [FOR 2.6.18 FIX][PATCH] drm: radeon flush TCL VAP for vertex program enable/disable

[Andrew Morton]

On Wed, 30 Aug 2006 23:17:55 +0100 (IST)
Dave Airlie <airlied@linux.ie> wrote:

> 
> Can we get this into 2.6.18? it fixes a lockup condition in r200 vertex 
> programs.
> 
> From: Roland Scheidegger <rscheidegger_lists@hispeed.ch>
> 
> The radeon requires a VAP state flush when enabling/disabling
> vertex programs on the r200 cards.
> 
> Signed-off-by: Dave Airlie <airlied@linux.ie>
> ---
>   drivers/char/drm/radeon_state.c |    9 ++++++++-
>   1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/char/drm/radeon_state.c b/drivers/char/drm/radeon_state.c
> index 5bb2234..5a08a23 100644
> --- a/drivers/char/drm/radeon_state.c
> +++ b/drivers/char/drm/radeon_state.c
> @@ -175,6 +175,14 @@ static __inline__ int radeon_check_and_f
>   		}
>   		break;
> 
> +	case R200_EMIT_VAP_CTL:{
> +			RING_LOCALS;
> +			BEGIN_RING(2);
> +			OUT_RING_REG(RADEON_SE_TCL_STATE_FLUSH, 0);
> +			ADVANCE_RING();
> +		}
> +		break;
> +
>   	case RADEON_EMIT_RB3D_COLORPITCH:
>   	case RADEON_EMIT_RE_LINE_PATTERN:
>   	case RADEON_EMIT_SE_LINE_WIDTH:
> @@ -202,7 +210,6 @@ static __inline__ int radeon_check_and_f
>   	case R200_EMIT_TCL_LIGHT_MODEL_CTL_0:
>   	case R200_EMIT_TFACTOR_0:
>   	case R200_EMIT_VTX_FMT_0:
> -	case R200_EMIT_VAP_CTL:
>   	case R200_EMIT_MATRIX_SELECT_0:
>   	case R200_EMIT_TEX_PROC_CTL_2:
>   	case R200_EMIT_TCL_UCP_VERT_BLEND_CTL:

That's a somewhat weird-looking patch.  It adds code which is quite
dissimilar from all the other cases in that switch statement.

Are you sure??

Re: [FOR 2.6.18 FIX][PATCH] drm: radeon flush TCL VAP for vertex program enable/disable

[Dave Airlie]

> That's a somewhat weird-looking patch.  It adds code which is quite
> dissimilar from all the other cases in that switch statement.
>
> Are you sure??

Yes it is the least likely to break anything spot, it is a type of fixup 
required for that packet to avoid lockups, there was a really ugly 
workaround in userspace...

Dave.

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
Linux kernel - DRI, VAX / pam_smb / ILUG

Re: [FOR 2.6.18 FIX][PATCH] drm: radeon flush TCL VAP for vertex program enable/disable

[Linus Torvalds]

On Wed, 30 Aug 2006, Andrew Morton wrote:
> 
> That's a somewhat weird-looking patch.  It adds code which is quite
> dissimilar from all the other cases in that switch statement.

It looks ok to me, although you have to look into the caller to see why it 
does what it does.

It would be "prettier" if it changed the size and data of the incoming 
packet instead, but the code as is isn't actually set up to be able to do 
that (the size setup and verification stuff is done before the fixup).

That said, I'd have expected that the VAP state flush is really something 
that the _client_ should do when it generates the commands, not the kernel 
after the fact. Although maybe the kernel could keep track of whether the 
flush is needed at all, and since we apparently allow untrusted generation 
of packets, maybe this is the right approach..

Anyway, the patch doesn't look _wrong_ to me, although it does seem to 
break the abstraction rules a bit.

			Linus

Re: [FOR 2.6.18 FIX][PATCH] drm: radeon flush TCL VAP for vertex program enable/disable

[Dave Airlie]

>
> It looks ok to me, although you have to look into the caller to see why it
> does what it does.
>
> It would be "prettier" if it changed the size and data of the incoming
> packet instead, but the code as is isn't actually set up to be able to do
> that (the size setup and verification stuff is done before the fixup).
>
> That said, I'd have expected that the VAP state flush is really something
> that the _client_ should do when it generates the commands, not the kernel
> after the fact. Although maybe the kernel could keep track of whether the
> flush is needed at all, and since we apparently allow untrusted generation
> of packets, maybe this is the right approach..

The problem is of course if the userspace side does it, the lockup is 
simple to trigger, we'd like if we can to stop them triggering it, we 
don't stop every lockup, but it would be nice to stop the ones we can...

Dave.

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
Linux kernel - DRI, VAX / pam_smb / ILUG

Re: [FOR 2.6.18 FIX][PATCH] drm: radeon flush TCL VAP for vertex program enable/disable

[Arjan van de Ven]

On Wed, 2006-08-30 at 15:53 -0700, Linus Torvalds wrote:
> 
> On Wed, 30 Aug 2006, Andrew Morton wrote:
> > 
> > That's a somewhat weird-looking patch.  It adds code which is quite
> > dissimilar from all the other cases in that switch statement.
> 
> It looks ok to me, although you have to look into the caller to see why it 
> does what it does.
> 
> It would be "prettier" if it changed the size and data of the incoming 
> packet instead, but the code as is isn't actually set up to be able to do 
> that (the size setup and verification stuff is done before the fixup).
> 
> That said, I'd have expected that the VAP state flush is really something 
> that the _client_ should do when it generates the commands, not the kernel 
> after the fact. 

but the client is unprivileged userspace!
The kernel needs to ensure correctness and probably even enforce it.