* [PATCH 1/4] drm/radeon: simplify register checker
2015-08-23 0:57 [PATCH 0/4] some optimization for evergreen cs Grazvydas Ignotas
@ 2015-08-23 0:57 ` Grazvydas Ignotas
2015-08-23 0:57 ` [PATCH 2/4] drm/radeon: split evergreen_cs_check_reg Grazvydas Ignotas
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Grazvydas Ignotas @ 2015-08-23 0:57 UTC (permalink / raw)
To: dri-devel
To avoid having to distinguish between CAYMAN or older on every register
check, place a pointer in evergreen_cs_track and use it unconditionally.
Also make use of the fact that both reg_safe_bm[] arrays are of the same
length to remove another CAYMAN check.
Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
drivers/gpu/drm/radeon/evergreen_cs.c | 49 +++++++++++++----------------------
1 file changed, 18 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
index c9e0fbb..5c840da 100644
--- a/drivers/gpu/drm/radeon/evergreen_cs.c
+++ b/drivers/gpu/drm/radeon/evergreen_cs.c
@@ -34,6 +34,8 @@
#define MAX(a,b) (((a)>(b))?(a):(b))
#define MIN(a,b) (((a)<(b))?(a):(b))
+#define REG_SAFE_BM_SIZE ARRAY_SIZE(evergreen_reg_safe_bm)
+
int r600_dma_cs_next_reloc(struct radeon_cs_parser *p,
struct radeon_bo_list **cs_reloc);
struct evergreen_cs_track {
@@ -84,6 +86,7 @@ struct evergreen_cs_track {
u32 htile_surface;
struct radeon_bo *htile_bo;
unsigned long indirect_draw_buffer_size;
+ const unsigned *reg_safe_bm;
};
static u32 evergreen_cs_get_aray_mode(u32 tiling_flags)
@@ -1096,28 +1099,17 @@ static int evergreen_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
{
struct evergreen_cs_track *track = (struct evergreen_cs_track *)p->track;
struct radeon_bo_list *reloc;
- u32 last_reg;
u32 m, i, tmp, *ib;
int r;
- if (p->rdev->family >= CHIP_CAYMAN)
- last_reg = ARRAY_SIZE(cayman_reg_safe_bm);
- else
- last_reg = ARRAY_SIZE(evergreen_reg_safe_bm);
-
i = (reg >> 7);
- if (i >= last_reg) {
+ if (unlikely(i >= REG_SAFE_BM_SIZE)) {
dev_warn(p->dev, "forbidden register 0x%08x at %d\n", reg, idx);
return -EINVAL;
}
m = 1 << ((reg >> 2) & 31);
- if (p->rdev->family >= CHIP_CAYMAN) {
- if (!(cayman_reg_safe_bm[i] & m))
- return 0;
- } else {
- if (!(evergreen_reg_safe_bm[i] & m))
- return 0;
- }
+ if (!(track->reg_safe_bm[i] & m))
+ return 0;
ib = p->ib.ptr;
switch (reg) {
/* force following reg to 0 in an attempt to disable out buffer
@@ -1766,26 +1758,17 @@ static int evergreen_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
static bool evergreen_is_safe_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
{
- u32 last_reg, m, i;
-
- if (p->rdev->family >= CHIP_CAYMAN)
- last_reg = ARRAY_SIZE(cayman_reg_safe_bm);
- else
- last_reg = ARRAY_SIZE(evergreen_reg_safe_bm);
+ struct evergreen_cs_track *track = p->track;
+ u32 m, i;
i = (reg >> 7);
- if (i >= last_reg) {
+ if (unlikely(i >= REG_SAFE_BM_SIZE)) {
dev_warn(p->dev, "forbidden register 0x%08x at %d\n", reg, idx);
return false;
}
m = 1 << ((reg >> 2) & 31);
- if (p->rdev->family >= CHIP_CAYMAN) {
- if (!(cayman_reg_safe_bm[i] & m))
- return true;
- } else {
- if (!(evergreen_reg_safe_bm[i] & m))
- return true;
- }
+ if (!(track->reg_safe_bm[i] & m))
+ return true;
dev_warn(p->dev, "forbidden register 0x%08x at %d\n", reg, idx);
return false;
}
@@ -2644,11 +2627,15 @@ int evergreen_cs_parse(struct radeon_cs_parser *p)
if (track == NULL)
return -ENOMEM;
evergreen_cs_track_init(track);
- if (p->rdev->family >= CHIP_CAYMAN)
+ if (p->rdev->family >= CHIP_CAYMAN) {
tmp = p->rdev->config.cayman.tile_config;
- else
+ track->reg_safe_bm = cayman_reg_safe_bm;
+ } else {
tmp = p->rdev->config.evergreen.tile_config;
-
+ track->reg_safe_bm = evergreen_reg_safe_bm;
+ }
+ BUILD_BUG_ON(ARRAY_SIZE(cayman_reg_safe_bm) != REG_SAFE_BM_SIZE);
+ BUILD_BUG_ON(ARRAY_SIZE(evergreen_reg_safe_bm) != REG_SAFE_BM_SIZE);
switch (tmp & 0xf) {
case 0:
track->npipes = 1;
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/4] drm/radeon: split evergreen_cs_check_reg
2015-08-23 0:57 [PATCH 0/4] some optimization for evergreen cs Grazvydas Ignotas
2015-08-23 0:57 ` [PATCH 1/4] drm/radeon: simplify register checker Grazvydas Ignotas
@ 2015-08-23 0:57 ` Grazvydas Ignotas
2015-08-23 0:57 ` [PATCH 3/4] drm/radeon: refactor register check loop Grazvydas Ignotas
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Grazvydas Ignotas @ 2015-08-23 0:57 UTC (permalink / raw)
To: dri-devel
evergreen_cs_check_reg() is a large function and gcc doesn't want to
inline it. It has a quick check for reg_safe_bm[] to see if register
needs special handling, which often results in early exit. However
because the function is large, it has a long prologue/epilogue to
save/restore all the callee-save registers which according to perf is
taking significant amount of time. To avoid this, we can reuse
evergreen_is_safe_reg() to do the early check directly in register loop.
Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
drivers/gpu/drm/radeon/evergreen_cs.c | 49 +++++++++++++++++++----------------
1 file changed, 27 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
index 5c840da..4453799 100644
--- a/drivers/gpu/drm/radeon/evergreen_cs.c
+++ b/drivers/gpu/drm/radeon/evergreen_cs.c
@@ -1086,30 +1086,18 @@ static int evergreen_cs_parse_packet0(struct radeon_cs_parser *p,
}
/**
- * evergreen_cs_check_reg() - check if register is authorized or not
+ * evergreen_cs_handle_reg() - process registers that need special handling.
* @parser: parser structure holding parsing context
* @reg: register we are testing
* @idx: index into the cs buffer
- *
- * This function will test against evergreen_reg_safe_bm and return 0
- * if register is safe. If register is not flag as safe this function
- * will test it against a list of register needind special handling.
*/
-static int evergreen_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
+static int evergreen_cs_handle_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
{
struct evergreen_cs_track *track = (struct evergreen_cs_track *)p->track;
struct radeon_bo_list *reloc;
- u32 m, i, tmp, *ib;
+ u32 tmp, *ib;
int r;
- i = (reg >> 7);
- if (unlikely(i >= REG_SAFE_BM_SIZE)) {
- dev_warn(p->dev, "forbidden register 0x%08x at %d\n", reg, idx);
- return -EINVAL;
- }
- m = 1 << ((reg >> 2) & 31);
- if (!(track->reg_safe_bm[i] & m))
- return 0;
ib = p->ib.ptr;
switch (reg) {
/* force following reg to 0 in an attempt to disable out buffer
@@ -1756,20 +1744,27 @@ static int evergreen_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
return 0;
}
-static bool evergreen_is_safe_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
+/**
+ * evergreen_is_safe_reg() - check if register is authorized or not
+ * @parser: parser structure holding parsing context
+ * @reg: register we are testing
+ *
+ * This function will test against reg_safe_bm and return true
+ * if register is safe or false otherwise.
+ */
+static inline bool evergreen_is_safe_reg(struct radeon_cs_parser *p, u32 reg)
{
struct evergreen_cs_track *track = p->track;
u32 m, i;
i = (reg >> 7);
if (unlikely(i >= REG_SAFE_BM_SIZE)) {
- dev_warn(p->dev, "forbidden register 0x%08x at %d\n", reg, idx);
return false;
}
m = 1 << ((reg >> 2) & 31);
if (!(track->reg_safe_bm[i] & m))
return true;
- dev_warn(p->dev, "forbidden register 0x%08x at %d\n", reg, idx);
+
return false;
}
@@ -2306,7 +2301,9 @@ static int evergreen_packet3_check(struct radeon_cs_parser *p,
}
for (i = 0; i < pkt->count; i++) {
reg = start_reg + (4 * i);
- r = evergreen_cs_check_reg(p, reg, idx+1+i);
+ if (evergreen_is_safe_reg(p, reg))
+ continue;
+ r = evergreen_cs_handle_reg(p, reg, idx + 1 + i);
if (r)
return r;
}
@@ -2322,7 +2319,9 @@ static int evergreen_packet3_check(struct radeon_cs_parser *p,
}
for (i = 0; i < pkt->count; i++) {
reg = start_reg + (4 * i);
- r = evergreen_cs_check_reg(p, reg, idx+1+i);
+ if (evergreen_is_safe_reg(p, reg))
+ continue;
+ r = evergreen_cs_handle_reg(p, reg, idx + 1 + i);
if (r)
return r;
}
@@ -2577,8 +2576,11 @@ static int evergreen_packet3_check(struct radeon_cs_parser *p,
} else {
/* SRC is a reg. */
reg = radeon_get_ib_value(p, idx+1) << 2;
- if (!evergreen_is_safe_reg(p, reg, idx+1))
+ if (!evergreen_is_safe_reg(p, reg)) {
+ dev_warn(p->dev, "forbidden register 0x%08x at %d\n",
+ reg, idx + 1);
return -EINVAL;
+ }
}
if (idx_value & 0x2) {
u64 offset;
@@ -2601,8 +2603,11 @@ static int evergreen_packet3_check(struct radeon_cs_parser *p,
} else {
/* DST is a reg. */
reg = radeon_get_ib_value(p, idx+3) << 2;
- if (!evergreen_is_safe_reg(p, reg, idx+3))
+ if (!evergreen_is_safe_reg(p, reg)) {
+ dev_warn(p->dev, "forbidden register 0x%08x at %d\n",
+ reg, idx + 3);
return -EINVAL;
+ }
}
break;
case PACKET3_NOP:
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 3/4] drm/radeon: refactor register check loop
2015-08-23 0:57 [PATCH 0/4] some optimization for evergreen cs Grazvydas Ignotas
2015-08-23 0:57 ` [PATCH 1/4] drm/radeon: simplify register checker Grazvydas Ignotas
2015-08-23 0:57 ` [PATCH 2/4] drm/radeon: split evergreen_cs_check_reg Grazvydas Ignotas
@ 2015-08-23 0:57 ` Grazvydas Ignotas
2015-08-23 0:57 ` [PATCH 4/4] drm/radeon: remove volatile qualifier Grazvydas Ignotas
2015-09-22 23:03 ` [PATCH 0/4] some optimization for evergreen cs Grazvydas Ignotas
4 siblings, 0 replies; 8+ messages in thread
From: Grazvydas Ignotas @ 2015-08-23 0:57 UTC (permalink / raw)
To: dri-devel
After this patch the register check loop does the same thing as before,
except that now gcc does better job optimizing it: it now sees that
end_reg was already checked against PACKET3_SET_CONTEXT_REG_END and can
optimize REG_SAFE_BM_SIZE comparison out of evergreen_is_safe_reg()
as (PACKET3_SET_CONTEXT_REG_END >> 7) < REG_SAFE_BM_SIZE.
Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
drivers/gpu/drm/radeon/evergreen_cs.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
index 4453799..e31076e 100644
--- a/drivers/gpu/drm/radeon/evergreen_cs.c
+++ b/drivers/gpu/drm/radeon/evergreen_cs.c
@@ -2299,11 +2299,10 @@ static int evergreen_packet3_check(struct radeon_cs_parser *p,
DRM_ERROR("bad PACKET3_SET_CONFIG_REG\n");
return -EINVAL;
}
- for (i = 0; i < pkt->count; i++) {
- reg = start_reg + (4 * i);
+ for (reg = start_reg, idx++; reg <= end_reg; reg += 4, idx++) {
if (evergreen_is_safe_reg(p, reg))
continue;
- r = evergreen_cs_handle_reg(p, reg, idx + 1 + i);
+ r = evergreen_cs_handle_reg(p, reg, idx);
if (r)
return r;
}
@@ -2317,11 +2316,10 @@ static int evergreen_packet3_check(struct radeon_cs_parser *p,
DRM_ERROR("bad PACKET3_SET_CONTEXT_REG\n");
return -EINVAL;
}
- for (i = 0; i < pkt->count; i++) {
- reg = start_reg + (4 * i);
+ for (reg = start_reg, idx++; reg <= end_reg; reg += 4, idx++) {
if (evergreen_is_safe_reg(p, reg))
continue;
- r = evergreen_cs_handle_reg(p, reg, idx + 1 + i);
+ r = evergreen_cs_handle_reg(p, reg, idx);
if (r)
return r;
}
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 4/4] drm/radeon: remove volatile qualifier
2015-08-23 0:57 [PATCH 0/4] some optimization for evergreen cs Grazvydas Ignotas
` (2 preceding siblings ...)
2015-08-23 0:57 ` [PATCH 3/4] drm/radeon: refactor register check loop Grazvydas Ignotas
@ 2015-08-23 0:57 ` Grazvydas Ignotas
2015-09-22 23:03 ` [PATCH 0/4] some optimization for evergreen cs Grazvydas Ignotas
4 siblings, 0 replies; 8+ messages in thread
From: Grazvydas Ignotas @ 2015-08-23 0:57 UTC (permalink / raw)
To: dri-devel
There doesn't seem to be any need to have 'ib' volatile, the code is
not even consistent with it and some places already miss it. As it is
now it's just making gcc produce worse code. If there are special
requirements for that memory, then proper primitives like memory
barriers or accessor functions should be used, but it doesn't look
like that is needed here.
While at it, change the type to match the one in radeon_ib structure.
Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
drivers/gpu/drm/radeon/evergreen_cs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c b/drivers/gpu/drm/radeon/evergreen_cs.c
index e31076e..46f87d4 100644
--- a/drivers/gpu/drm/radeon/evergreen_cs.c
+++ b/drivers/gpu/drm/radeon/evergreen_cs.c
@@ -447,7 +447,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser *p, unsigned i
* command stream.
*/
if (!surf.mode) {
- volatile u32 *ib = p->ib.ptr;
+ uint32_t *ib = p->ib.ptr;
unsigned long tmp, nby, bsize, size, min = 0;
/* find the height the ddx wants */
@@ -1773,7 +1773,7 @@ static int evergreen_packet3_check(struct radeon_cs_parser *p,
{
struct radeon_bo_list *reloc;
struct evergreen_cs_track *track;
- volatile u32 *ib;
+ uint32_t *ib;
unsigned idx;
unsigned i;
unsigned start_reg, end_reg, reg;
@@ -2747,7 +2747,7 @@ int evergreen_dma_cs_parse(struct radeon_cs_parser *p)
struct radeon_cs_chunk *ib_chunk = p->chunk_ib;
struct radeon_bo_list *src_reloc, *dst_reloc, *dst2_reloc;
u32 header, cmd, count, sub_cmd;
- volatile u32 *ib = p->ib.ptr;
+ uint32_t *ib = p->ib.ptr;
u32 idx;
u64 src_offset, dst_offset, dst2_offset;
int r;
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 0/4] some optimization for evergreen cs
2015-08-23 0:57 [PATCH 0/4] some optimization for evergreen cs Grazvydas Ignotas
` (3 preceding siblings ...)
2015-08-23 0:57 ` [PATCH 4/4] drm/radeon: remove volatile qualifier Grazvydas Ignotas
@ 2015-09-22 23:03 ` Grazvydas Ignotas
2015-09-23 6:59 ` Dave Airlie
4 siblings, 1 reply; 8+ messages in thread
From: Grazvydas Ignotas @ 2015-09-22 23:03 UTC (permalink / raw)
To: dri-devel, Alex Deucher, Christian König
On Sun, Aug 23, 2015 at 3:57 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
> These patches try to reduce CPU usage of register command checker
> without affecting functionality.
> For me this gives 3-4% perf improvement in glxgears and ~1% CPU usage reduction
> in "The Talos Principle" CS thread.
>
> Grazvydas Ignotas (4):
> drm/radeon: simplify register checker
> drm/radeon: split evergreen_cs_check_reg
> drm/radeon: refactor register check loop
> drm/radeon: remove use of volatile qualifier
>
> drivers/gpu/drm/radeon/evergreen_cs.c | 104 +++++++++++++++-------------------
> 1 file changed, 47 insertions(+), 57 deletions(-)
Can someone take a look at these? They still apply on current mainline
and I've been using them for a while without issues. I've also ran
piglit gpu tests and found no regressions.
Gražvydas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/4] some optimization for evergreen cs
2015-09-22 23:03 ` [PATCH 0/4] some optimization for evergreen cs Grazvydas Ignotas
@ 2015-09-23 6:59 ` Dave Airlie
2015-09-23 21:54 ` Alex Deucher
0 siblings, 1 reply; 8+ messages in thread
From: Dave Airlie @ 2015-09-23 6:59 UTC (permalink / raw)
To: Grazvydas Ignotas; +Cc: Alex Deucher, Christian König, dri-devel
On 23 September 2015 at 09:03, Grazvydas Ignotas <notasas@gmail.com> wrote:
> On Sun, Aug 23, 2015 at 3:57 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
>> These patches try to reduce CPU usage of register command checker
>> without affecting functionality.
>> For me this gives 3-4% perf improvement in glxgears and ~1% CPU usage reduction
>> in "The Talos Principle" CS thread.
>>
>> Grazvydas Ignotas (4):
>> drm/radeon: simplify register checker
>> drm/radeon: split evergreen_cs_check_reg
>> drm/radeon: refactor register check loop
>> drm/radeon: remove use of volatile qualifier
>>
>> drivers/gpu/drm/radeon/evergreen_cs.c | 104 +++++++++++++++-------------------
>> 1 file changed, 47 insertions(+), 57 deletions(-)
>
> Can someone take a look at these? They still apply on current mainline
> and I've been using them for a while without issues. I've also ran
> piglit gpu tests and found no regressions.
Reviewed-by: Dave Airlie <airlied@redhat.com>
Alex can you pick these up?
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/4] some optimization for evergreen cs
2015-09-23 6:59 ` Dave Airlie
@ 2015-09-23 21:54 ` Alex Deucher
0 siblings, 0 replies; 8+ messages in thread
From: Alex Deucher @ 2015-09-23 21:54 UTC (permalink / raw)
To: Dave Airlie; +Cc: Alex Deucher, dri-devel, Christian König
On Wed, Sep 23, 2015 at 2:59 AM, Dave Airlie <airlied@gmail.com> wrote:
> On 23 September 2015 at 09:03, Grazvydas Ignotas <notasas@gmail.com> wrote:
>> On Sun, Aug 23, 2015 at 3:57 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
>>> These patches try to reduce CPU usage of register command checker
>>> without affecting functionality.
>>> For me this gives 3-4% perf improvement in glxgears and ~1% CPU usage reduction
>>> in "The Talos Principle" CS thread.
>>>
>>> Grazvydas Ignotas (4):
>>> drm/radeon: simplify register checker
>>> drm/radeon: split evergreen_cs_check_reg
>>> drm/radeon: refactor register check loop
>>> drm/radeon: remove use of volatile qualifier
>>>
>>> drivers/gpu/drm/radeon/evergreen_cs.c | 104 +++++++++++++++-------------------
>>> 1 file changed, 47 insertions(+), 57 deletions(-)
>>
>> Can someone take a look at these? They still apply on current mainline
>> and I've been using them for a while without issues. I've also ran
>> piglit gpu tests and found no regressions.
>
> Reviewed-by: Dave Airlie <airlied@redhat.com>
>
> Alex can you pick these up?
Applied. thanks!
Alex
>
> Dave.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread