Can we get this into 2.6.18? it fixes a lockup condition in r200 vertex
programs.
From: Roland Scheidegger <[email protected]>
The radeon requires a VAP state flush when enabling/disabling
vertex programs on the r200 cards.
Signed-off-by: Dave Airlie <[email protected]>
---
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
On Wed, 30 Aug 2006 23:17:55 +0100 (IST)
Dave Airlie <[email protected]> wrote:
>
> Can we get this into 2.6.18? it fixes a lockup condition in r200 vertex
> programs.
>
> From: Roland Scheidegger <[email protected]>
>
> The radeon requires a VAP state flush when enabling/disabling
> vertex programs on the r200 cards.
>
> Signed-off-by: Dave Airlie <[email protected]>
> ---
> 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??
> 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
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
>
> 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
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.