[PATCH] drm/radeon/kms: Fix for CS track check to allow at least one enabled component

[PATCH] drm/radeon/kms: Fix for CS track check to allow at least one enabled component

[Oldřich Jedlička]

The current code causes failing of rendering (discovered on my KDE
4.4, mesa git master, xorg-video-ati git master) with message:

r600_cs_track_check:252 mask 0x0000000F | 0xFFFFFFFF no cb for 1

The problem is in the check of "at least one component is enabled" in
r600_cs_track_check() - it finds the first component and does not check
the rest. My patch fixes this by introducing is_valid local variable
and checking all enabled components. The check returns failure only
when there is really no component enabled.

Signed-off-by: Oldřich Jedlička <oldium.pro@seznam.cz>
---
 drivers/gpu/drm/radeon/r600_cs.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/radeon/r600_cs.c 
b/drivers/gpu/drm/radeon/r600_cs.c
index 7cabb62..f1dab96 100644
--- a/drivers/gpu/drm/radeon/r600_cs.c
+++ b/drivers/gpu/drm/radeon/r600_cs.c
@@ -233,6 +233,7 @@ static int r600_cs_track_check(struct radeon_cs_parser *p)
 	struct r600_cs_track *track = p->track;
 	u32 tmp;
 	int r, i;
+	bool is_valid = false;
 
 	/* on legacy kernel we don't perform advanced check */
 	if (p->rdev == NULL)
@@ -247,17 +248,20 @@ static int r600_cs_track_check(struct radeon_cs_parser 
*p)
 	for (i = 0; i < 8; i++) {
 		if ((tmp >> (i * 4)) & 0xF) {
 			/* at least one component is enabled */
-			if (track->cb_color_bo[i] == NULL) {
-				dev_warn(p->dev, "%s:%d mask 0x%08X | 0x%08X no cb for %d\n",
-					__func__, __LINE__, track->cb_target_mask, track-
>cb_shader_mask, i);
-				return -EINVAL;
+			if (track->cb_color_bo[i] != NULL) {
+				/* perform rewrite of CB_COLOR[0-7]_SIZE */
+				r = r600_cs_track_validate_cb(p, i);
+				if (r)
+					return r;
+				is_valid = true;
 			}
-			/* perform rewrite of CB_COLOR[0-7]_SIZE */
-			r = r600_cs_track_validate_cb(p, i);
-			if (r)
-				return r;
 		}
 	}
+	if (!is_valid) {
+		dev_warn(p->dev, "%s:%d mask 0x%08X | 0x%08X no cb found\n",
+			__func__, __LINE__, track->cb_target_mask, track->cb_shader_mask);
+		return -EINVAL;
+	}
 	return 0;
 }
 
-- 
1.6.6.1


------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--

Re: [PATCH] drm/radeon/kms: Fix for CS track check to allow at least one enabled component

[Oldřich Jedlička]

This patch (sent originally to xorg-driver-ati mailing list 4 days ago) fixes 
problem introduced by Jerome's patch for command stream checking for r600/r700 
(sent few minutes ago). My patch should be applied after his one.

Cheers,
Oldřich.

On Wednesday 10 of February 2010 at 23:36:12, Oldřich Jedlička wrote:
> The current code causes failing of rendering (discovered on my KDE
> 4.4, mesa git master, xorg-video-ati git master) with message:
> 
> r600_cs_track_check:252 mask 0x0000000F | 0xFFFFFFFF no cb for 1
> 
> The problem is in the check of "at least one component is enabled" in
> r600_cs_track_check() - it finds the first component and does not check
> the rest. My patch fixes this by introducing is_valid local variable
> and checking all enabled components. The check returns failure only
> when there is really no component enabled.
> 
> Signed-off-by: Oldřich Jedlička <oldium.pro@seznam.cz>
> ---
>  drivers/gpu/drm/radeon/r600_cs.c |   20 ++++++++++++--------
>  1 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/r600_cs.c
> b/drivers/gpu/drm/radeon/r600_cs.c
> index 7cabb62..f1dab96 100644
> --- a/drivers/gpu/drm/radeon/r600_cs.c
> +++ b/drivers/gpu/drm/radeon/r600_cs.c
> @@ -233,6 +233,7 @@ static int r600_cs_track_check(struct radeon_cs_parser
> *p) struct r600_cs_track *track = p->track;
>  	u32 tmp;
>  	int r, i;
> +	bool is_valid = false;
> 
>  	/* on legacy kernel we don't perform advanced check */
>  	if (p->rdev == NULL)
> @@ -247,17 +248,20 @@ static int r600_cs_track_check(struct
> radeon_cs_parser *p)
>  	for (i = 0; i < 8; i++) {
>  		if ((tmp >> (i * 4)) & 0xF) {
>  			/* at least one component is enabled */
> -			if (track->cb_color_bo[i] == NULL) {
> -				dev_warn(p->dev, "%s:%d mask 0x%08X | 0x%08X no cb for %d\n",
> -					__func__, __LINE__, track->cb_target_mask, track-
> 
> >cb_shader_mask, i);
> 
> -				return -EINVAL;
> +			if (track->cb_color_bo[i] != NULL) {
> +				/* perform rewrite of CB_COLOR[0-7]_SIZE */
> +				r = r600_cs_track_validate_cb(p, i);
> +				if (r)
> +					return r;
> +				is_valid = true;
>  			}
> -			/* perform rewrite of CB_COLOR[0-7]_SIZE */
> -			r = r600_cs_track_validate_cb(p, i);
> -			if (r)
> -				return r;
>  		}
>  	}
> +	if (!is_valid) {
> +		dev_warn(p->dev, "%s:%d mask 0x%08X | 0x%08X no cb found\n",
> +			__func__, __LINE__, track->cb_target_mask, track-
>cb_shader_mask);
> +		return -EINVAL;
> +	}
>  	return 0;
>  }

------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--

[PATCH v2] drm/radeon/kms: Fix for CS track check to allow at least one enabled component

[Oldřich Jedlička]

The current code causes failing of rendering (discovered on my KDE
4.4) with message:

r600_cs_track_check:252 mask 0x0000000F | 0xFFFFFFFF no cb for 1

The problem is in the check of "at least one component is enabled" in
r600_cs_track_check() - it finds the first component and does not check
the rest. My patch fixes this by introducing is_valid local variable
and checking all enabled components. The check returns failure only
when there is really no component enabled.

Signed-off-by: Oldřich Jedlička <oldium.pro@seznam.cz>
---
 drivers/gpu/drm/radeon/r600_cs.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c
index 7cabb62..f1dab96 100644
--- a/drivers/gpu/drm/radeon/r600_cs.c
+++ b/drivers/gpu/drm/radeon/r600_cs.c
@@ -233,6 +233,7 @@ static int r600_cs_track_check(struct radeon_cs_parser *p)
 	struct r600_cs_track *track = p->track;
 	u32 tmp;
 	int r, i;
+	bool is_valid = false;
 
 	/* on legacy kernel we don't perform advanced check */
 	if (p->rdev == NULL)
@@ -247,17 +248,20 @@ static int r600_cs_track_check(struct radeon_cs_parser *p)
 	for (i = 0; i < 8; i++) {
 		if ((tmp >> (i * 4)) & 0xF) {
 			/* at least one component is enabled */
-			if (track->cb_color_bo[i] == NULL) {
-				dev_warn(p->dev, "%s:%d mask 0x%08X | 0x%08X no cb for %d\n",
-					__func__, __LINE__, track->cb_target_mask, track->cb_shader_mask, i);
-				return -EINVAL;
+			if (track->cb_color_bo[i] != NULL) {
+				/* perform rewrite of CB_COLOR[0-7]_SIZE */
+				r = r600_cs_track_validate_cb(p, i);
+				if (r)
+					return r;
+				is_valid = true;
 			}
-			/* perform rewrite of CB_COLOR[0-7]_SIZE */
-			r = r600_cs_track_validate_cb(p, i);
-			if (r)
-				return r;
 		}
 	}
+	if (!is_valid) {
+		dev_warn(p->dev, "%s:%d mask 0x%08X | 0x%08X no cb found\n",
+			__func__, __LINE__, track->cb_target_mask, track->cb_shader_mask);
+		return -EINVAL;
+	}
 	return 0;
 }
 
-- 
1.6.6.1


------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--

Re: [PATCH v2] drm/radeon/kms: Fix for CS track check to allow at least one enabled component

[Oldřich Jedlička]

My patch isn't needed any more. The last Jerome's patch titled 

  [PATCH] drm/radeon/kms: r600/r700 command stream checker

works well.

Cheers,
Oldřich.

On Wednesday 10 of February 2010 at 23:54:13, Oldřich Jedlička wrote:
> The current code causes failing of rendering (discovered on my KDE
> 4.4) with message:
> 
> r600_cs_track_check:252 mask 0x0000000F | 0xFFFFFFFF no cb for 1
> 
> The problem is in the check of "at least one component is enabled" in
> r600_cs_track_check() - it finds the first component and does not check
> the rest. My patch fixes this by introducing is_valid local variable
> and checking all enabled components. The check returns failure only
> when there is really no component enabled.
> 
> Signed-off-by: Oldřich Jedlička <oldium.pro@seznam.cz>
> ---
>  drivers/gpu/drm/radeon/r600_cs.c |   20 ++++++++++++--------
>  1 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/r600_cs.c
> b/drivers/gpu/drm/radeon/r600_cs.c index 7cabb62..f1dab96 100644
> --- a/drivers/gpu/drm/radeon/r600_cs.c
> +++ b/drivers/gpu/drm/radeon/r600_cs.c
> @@ -233,6 +233,7 @@ static int r600_cs_track_check(struct radeon_cs_parser
> *p) struct r600_cs_track *track = p->track;
>  	u32 tmp;
>  	int r, i;
> +	bool is_valid = false;
> 
>  	/* on legacy kernel we don't perform advanced check */
>  	if (p->rdev == NULL)
> @@ -247,17 +248,20 @@ static int r600_cs_track_check(struct
> radeon_cs_parser *p) for (i = 0; i < 8; i++) {
>  		if ((tmp >> (i * 4)) & 0xF) {
>  			/* at least one component is enabled */
> -			if (track->cb_color_bo[i] == NULL) {
> -				dev_warn(p->dev, "%s:%d mask 0x%08X | 0x%08X no cb for %d\n",
> -					__func__, __LINE__, track->cb_target_mask, track-
>cb_shader_mask, i);
> -				return -EINVAL;
> +			if (track->cb_color_bo[i] != NULL) {
> +				/* perform rewrite of CB_COLOR[0-7]_SIZE */
> +				r = r600_cs_track_validate_cb(p, i);
> +				if (r)
> +					return r;
> +				is_valid = true;
>  			}
> -			/* perform rewrite of CB_COLOR[0-7]_SIZE */
> -			r = r600_cs_track_validate_cb(p, i);
> -			if (r)
> -				return r;
>  		}
>  	}
> +	if (!is_valid) {
> +		dev_warn(p->dev, "%s:%d mask 0x%08X | 0x%08X no cb found\n",
> +			__func__, __LINE__, track->cb_target_mask, track-
>cb_shader_mask);
> +		return -EINVAL;
> +	}
>  	return 0;
>  }

------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--

Re: [PATCH] drm/radeon/kms: Fix for CS track check to allow at least one enabled component

[Jerome Glisse]

On Wed, Feb 10, 2010 at 11:36:12PM +0100, Oldřich Jedlička wrote:
> The current code causes failing of rendering (discovered on my KDE
> 4.4, mesa git master, xorg-video-ati git master) with message:
> 
> r600_cs_track_check:252 mask 0x0000000F | 0xFFFFFFFF no cb for 1
> 
> The problem is in the check of "at least one component is enabled" in
> r600_cs_track_check() - it finds the first component and does not check
> the rest. My patch fixes this by introducing is_valid local variable
> and checking all enabled components. The check returns failure only
> when there is really no component enabled.
> 
> Signed-off-by: Oldřich Jedlička <oldium.pro@seznam.cz>

Already fixed in the newer version, also your patch defeated the
purpose of the checker. To work around this i only trust cb_target_mask.
Thanks for reporting this issue.

Cheers,
Jerome

------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel