Initialize nsamples to 1 in case userspace does not emit PA_SC_AA_CONFIG.
Signed-off-by: Alex Deucher alexdeucher@gmail.com Cc: Andre Maasikas amaasikas@gmail.com --- drivers/gpu/drm/radeon/r600_cs.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c index 8e165b2..b03f93b 100644 --- a/drivers/gpu/drm/radeon/r600_cs.c +++ b/drivers/gpu/drm/radeon/r600_cs.c @@ -163,6 +163,7 @@ static void r600_cs_track_init(struct r600_cs_track *track) track->db_depth_size = 0xFFFFFFFF; track->db_depth_size_idx = 0; track->db_depth_control = 0xFFFFFFFF; + track->nsamples = 1; }
static inline int r600_cs_track_validate_cb(struct radeon_cs_parser *p, int i)
On 08/13/2010 12:48 AM, Alex Deucher wrote:
Initialize nsamples to 1 in case userspace does not emit PA_SC_AA_CONFIG.
Signed-off-by: Alex Deucher alexdeucher@gmail.com Cc: Andre Maasikas amaasikas@gmail.com
drivers/gpu/drm/radeon/r600_cs.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c index 8e165b2..b03f93b 100644 --- a/drivers/gpu/drm/radeon/r600_cs.c +++ b/drivers/gpu/drm/radeon/r600_cs.c @@ -163,6 +163,7 @@ static void r600_cs_track_init(struct r600_cs_track *track) track->db_depth_size = 0xFFFFFFFF; track->db_depth_size_idx = 0; track->db_depth_control = 0xFFFFFFFF;
- track->nsamples = 1;
}
static inline int r600_cs_track_validate_cb(struct radeon_cs_parser *p, int i)
I would NAK this one, accepting such things pretty much defeat the idea of checker, i think userspace should always set nsamples as otherwise it might trick kernel and overwritte others memory. ie: cs1 set nsample 8 & all others properly so cs1 is accepted cs2 don't set nsample but bind a smaller bo and so can overwritte others buffer
Cheers, Jerome
On Mon, Aug 16, 2010 at 9:19 AM, Jerome Glisse glisse@freedesktop.org wrote:
On 08/13/2010 12:48 AM, Alex Deucher wrote:
Initialize nsamples to 1 in case userspace does not emit PA_SC_AA_CONFIG.
Signed-off-by: Alex Deucher alexdeucher@gmail.com Cc: Andre Maasikas amaasikas@gmail.com
drivers/gpu/drm/radeon/r600_cs.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c index 8e165b2..b03f93b 100644 --- a/drivers/gpu/drm/radeon/r600_cs.c +++ b/drivers/gpu/drm/radeon/r600_cs.c @@ -163,6 +163,7 @@ static void r600_cs_track_init(struct r600_cs_track *track) track->db_depth_size = 0xFFFFFFFF; track->db_depth_size_idx = 0; track->db_depth_control = 0xFFFFFFFF;
- track->nsamples = 1;
}
static inline int r600_cs_track_validate_cb(struct radeon_cs_parser *p, int i)
I would NAK this one, accepting such things pretty much defeat the idea of checker, i think userspace should always set nsamples as otherwise it might trick kernel and overwritte others memory. ie: cs1 set nsample 8 & all others properly so cs1 is accepted cs2 don't set nsample but bind a smaller bo and so can overwritte others buffer
Unfortunately, existing userspace already sends buffers without this reg set. It's just that prior to tiling we didn't care what it was set to.
Alex
On 08/16/2010 09:22 AM, Alex Deucher wrote:
On Mon, Aug 16, 2010 at 9:19 AM, Jerome Glisse glisse@freedesktop.org wrote:
On 08/13/2010 12:48 AM, Alex Deucher wrote:
Initialize nsamples to 1 in case userspace does not emit PA_SC_AA_CONFIG.
Signed-off-by: Alex Deucher alexdeucher@gmail.com Cc: Andre Maasikas amaasikas@gmail.com
drivers/gpu/drm/radeon/r600_cs.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c index 8e165b2..b03f93b 100644 --- a/drivers/gpu/drm/radeon/r600_cs.c +++ b/drivers/gpu/drm/radeon/r600_cs.c @@ -163,6 +163,7 @@ static void r600_cs_track_init(struct r600_cs_track *track) track->db_depth_size = 0xFFFFFFFF; track->db_depth_size_idx = 0; track->db_depth_control = 0xFFFFFFFF;
track->nsamples = 1;}
static inline int r600_cs_track_validate_cb(struct radeon_cs_parser *p, int i)
I would NAK this one, accepting such things pretty much defeat the idea of checker, i think userspace should always set nsamples as otherwise it might trick kernel and overwritte others memory. ie: cs1 set nsample 8 & all others properly so cs1 is accepted cs2 don't set nsample but bind a smaller bo and so can overwritte others buffer
Unfortunately, existing userspace already sends buffers without this reg set. It's just that prior to tiling we didn't care what it was set to.
Alex
I guess once again we hit by our own fault of prematurely going out of staging.
Jerome
On Mon, Aug 16, 2010 at 9:54 AM, Jerome Glisse glisse@freedesktop.org wrote:
On 08/16/2010 09:22 AM, Alex Deucher wrote:
On Mon, Aug 16, 2010 at 9:19 AM, Jerome Glisse glisse@freedesktop.org wrote:
On 08/13/2010 12:48 AM, Alex Deucher wrote:
Initialize nsamples to 1 in case userspace does not emit PA_SC_AA_CONFIG.
Signed-off-by: Alex Deucher alexdeucher@gmail.com Cc: Andre Maasikas amaasikas@gmail.com
drivers/gpu/drm/radeon/r600_cs.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c index 8e165b2..b03f93b 100644 --- a/drivers/gpu/drm/radeon/r600_cs.c +++ b/drivers/gpu/drm/radeon/r600_cs.c @@ -163,6 +163,7 @@ static void r600_cs_track_init(struct r600_cs_track *track) track->db_depth_size = 0xFFFFFFFF; track->db_depth_size_idx = 0; track->db_depth_control = 0xFFFFFFFF;
- track->nsamples = 1;
}
static inline int r600_cs_track_validate_cb(struct radeon_cs_parser *p, int i)
I would NAK this one, accepting such things pretty much defeat the idea of checker, i think userspace should always set nsamples as otherwise it might trick kernel and overwritte others memory. ie: cs1 set nsample 8 & all others properly so cs1 is accepted cs2 don't set nsample but bind a smaller bo and so can overwritte others buffer
Unfortunately, existing userspace already sends buffers without this reg set. It's just that prior to tiling we didn't care what it was set to.
Alex
I guess once again we hit by our own fault of prematurely going out of staging.
I *think* the only thing that doesn't emit this reg is the blit code in mesa git master, so we could just fix it there as it's not officially released, but there's quite a bit of mesa git master out in the wild.
Alex
On Mon, 2010-08-16 at 09:54 -0400, Jerome Glisse wrote:
On 08/16/2010 09:22 AM, Alex Deucher wrote:
On Mon, Aug 16, 2010 at 9:19 AM, Jerome Glisse glisse@freedesktop.org wrote:
On 08/13/2010 12:48 AM, Alex Deucher wrote:
Initialize nsamples to 1 in case userspace does not emit PA_SC_AA_CONFIG.
Signed-off-by: Alex Deucher alexdeucher@gmail.com Cc: Andre Maasikas amaasikas@gmail.com
drivers/gpu/drm/radeon/r600_cs.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c index 8e165b2..b03f93b 100644 --- a/drivers/gpu/drm/radeon/r600_cs.c +++ b/drivers/gpu/drm/radeon/r600_cs.c @@ -163,6 +163,7 @@ static void r600_cs_track_init(struct r600_cs_track *track) track->db_depth_size = 0xFFFFFFFF; track->db_depth_size_idx = 0; track->db_depth_control = 0xFFFFFFFF;
track->nsamples = 1;}
static inline int r600_cs_track_validate_cb(struct radeon_cs_parser *p, int i)
I would NAK this one, accepting such things pretty much defeat the idea of checker, i think userspace should always set nsamples as otherwise it might trick kernel and overwritte others memory. ie: cs1 set nsample 8 & all others properly so cs1 is accepted cs2 don't set nsample but bind a smaller bo and so can overwritte others buffer
Unfortunately, existing userspace already sends buffers without this reg set. It's just that prior to tiling we didn't care what it was set to.
Alex
I guess once again we hit by our own fault of prematurely going out of staging.
Jerome, staying in staging forever until every feature of the hw is implemented isn't possible, at some point you have to do real engineer work and this sounds like its workaroundable
How about if we get a tiling CB in the checker, we make sure nsamples is set? will that work?
Dave.
On Mon, Aug 16, 2010 at 4:39 PM, Dave Airlie airlied@redhat.com wrote:
On Mon, 2010-08-16 at 09:54 -0400, Jerome Glisse wrote:
On 08/16/2010 09:22 AM, Alex Deucher wrote:
On Mon, Aug 16, 2010 at 9:19 AM, Jerome Glisse glisse@freedesktop.org wrote:
On 08/13/2010 12:48 AM, Alex Deucher wrote:
Initialize nsamples to 1 in case userspace does not emit PA_SC_AA_CONFIG.
Signed-off-by: Alex Deucher alexdeucher@gmail.com Cc: Andre Maasikas amaasikas@gmail.com
drivers/gpu/drm/radeon/r600_cs.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c index 8e165b2..b03f93b 100644 --- a/drivers/gpu/drm/radeon/r600_cs.c +++ b/drivers/gpu/drm/radeon/r600_cs.c @@ -163,6 +163,7 @@ static void r600_cs_track_init(struct r600_cs_track *track) track->db_depth_size = 0xFFFFFFFF; track->db_depth_size_idx = 0; track->db_depth_control = 0xFFFFFFFF;
- track->nsamples = 1;
}
static inline int r600_cs_track_validate_cb(struct radeon_cs_parser *p, int i)
I would NAK this one, accepting such things pretty much defeat the idea of checker, i think userspace should always set nsamples as otherwise it might trick kernel and overwritte others memory. ie: cs1 set nsample 8 & all others properly so cs1 is accepted cs2 don't set nsample but bind a smaller bo and so can overwritte others buffer
Unfortunately, existing userspace already sends buffers without this reg set. It's just that prior to tiling we didn't care what it was set to.
Alex
I guess once again we hit by our own fault of prematurely going out of staging.
Jerome, staying in staging forever until every feature of the hw is implemented isn't possible, at some point you have to do real engineer work and this sounds like its workaroundable
How about if we get a tiling CB in the checker, we make sure nsamples is set? will that work?
I've fixed the issue in mesa. The problematic mesa code has not been officially part of a release yet. That said, I think 1 is a reasonable default for nsamples regardless as unless MSAA is enabled, you can pretty safely assume 1.
Alex
Dave.
On Mon, Aug 16, 2010 at 4:53 PM, Alex Deucher alexdeucher@gmail.com wrote:
On Mon, Aug 16, 2010 at 4:39 PM, Dave Airlie airlied@redhat.com wrote:
On Mon, 2010-08-16 at 09:54 -0400, Jerome Glisse wrote:
On 08/16/2010 09:22 AM, Alex Deucher wrote:
On Mon, Aug 16, 2010 at 9:19 AM, Jerome Glisse glisse@freedesktop.org wrote:
On 08/13/2010 12:48 AM, Alex Deucher wrote:
Initialize nsamples to 1 in case userspace does not emit PA_SC_AA_CONFIG.
Signed-off-by: Alex Deucher alexdeucher@gmail.com Cc: Andre Maasikas amaasikas@gmail.com
drivers/gpu/drm/radeon/r600_cs.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c index 8e165b2..b03f93b 100644 --- a/drivers/gpu/drm/radeon/r600_cs.c +++ b/drivers/gpu/drm/radeon/r600_cs.c @@ -163,6 +163,7 @@ static void r600_cs_track_init(struct r600_cs_track *track) track->db_depth_size = 0xFFFFFFFF; track->db_depth_size_idx = 0; track->db_depth_control = 0xFFFFFFFF;
- track->nsamples = 1;
}
static inline int r600_cs_track_validate_cb(struct radeon_cs_parser *p, int i)
I would NAK this one, accepting such things pretty much defeat the idea of checker, i think userspace should always set nsamples as otherwise it might trick kernel and overwritte others memory. ie: cs1 set nsample 8 & all others properly so cs1 is accepted cs2 don't set nsample but bind a smaller bo and so can overwritte others buffer
Unfortunately, existing userspace already sends buffers without this reg set. It's just that prior to tiling we didn't care what it was set to.
Alex
I guess once again we hit by our own fault of prematurely going out of staging.
Jerome, staying in staging forever until every feature of the hw is implemented isn't possible, at some point you have to do real engineer work and this sounds like its workaroundable
How about if we get a tiling CB in the checker, we make sure nsamples is set? will that work?
I've fixed the issue in mesa. The problematic mesa code has not been officially part of a release yet. That said, I think 1 is a reasonable default for nsamples regardless as unless MSAA is enabled, you can pretty safely assume 1.
If we don't set a default value, we would need to make sure IBs always contain PA_SC_AA_CONFIG which is nor currently enforced. Thoughts?
Alex
Alex
Dave.
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Dave Airlie -
Jerome Glisse