drm/vmwgfx: Fix compat shader namespace
On Fri, Jul 11, 2014 at 12:14:25AM +0200, Thomas Hellstr?m wrote: > > I agree, however that readability may be more important than the > fastpath execution speed gained from this. But in my case not so > much that I spontaneously feel like removing all branch prediction > hints from the vmwgfx driver. I'm just saying, let's not add new ones. > > > > >There are two rules of thumb for likely/unlikely: > > > > 1) Don't use it in the drivers/ directory. > > 2) Or don't use it without benchmarking it. > > Could you point me to a document or some sort of reference? http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2012-March/025252.html regards, dan carpenter
drm/vmwgfx: Fix compat shader namespace
On 2014-07-10 11:33, Dan Carpenter wrote: > On Wed, Jul 09, 2014 at 11:31:45PM +0200, Thomas Hellstr?m wrote: >>> Speaking of verbose, all the likely/unlikely annotations should be >>> removed. >> Is this your personal opinion or has there been some kind of kernel >> developer agreement not to add this annotation and remove it from >> the kernel tree? If not, I prefer to keep it. > It obviously makes the code less readable. It makes a small speedup if > the code is called 1 times with the and the expected value is true > every time. If more than 1 out of 1 values is unexpected then it is > a slow down. You may be right, but most people citicising branch prediction hints tend to stare blindly at probabilities and forget about fastpath optimization where you don't care at all about the execution speed of the slowpath and therefore hint the compiler to take the fastpath branch regardless of probabilities. My guess is it would be pretty hard to do that incorrectly. I agree, however that readability may be more important than the fastpath execution speed gained from this. But in my case not so much that I spontaneously feel like removing all branch prediction hints from the vmwgfx driver. > > There are two rules of thumb for likely/unlikely: > > 1) Don't use it in the drivers/ directory. > 2) Or don't use it without benchmarking it. Could you point me to a document or some sort of reference? Thanks, Thomas
drm/vmwgfx: Fix compat shader namespace
On Wed, Jul 09, 2014 at 11:31:45PM +0200, Thomas Hellstr?m wrote: > >Speaking of verbose, all the likely/unlikely annotations should be > >removed. > > Is this your personal opinion or has there been some kind of kernel > developer agreement not to add this annotation and remove it from > the kernel tree? If not, I prefer to keep it. It obviously makes the code less readable. It makes a small speedup if the code is called 1 times with the and the expected value is true every time. If more than 1 out of 1 values is unexpected then it is a slow down. There are two rules of thumb for likely/unlikely: 1) Don't use it in the drivers/ directory. 2) Or don't use it without benchmarking it. These are general rules, not mine. In the olden days we used to use it more often but then people did benchmarking and likely/unlikely annotations didn't make a single measurable difference on normal benchmarks at all. Maybe on a micro benchmark. Also perhaps in those days people hadn't done branch profiling so we were getting a lot of unexpected conditions and the slow downs were canceling the speed ups. regards, dan carpenter
drm/vmwgfx: Fix compat shader namespace
On 2014-07-09 14:48, Dan Carpenter wrote:
> Hello Thomas Hellstrom,
>
> The patch 18e4a4669c50: "drm/vmwgfx: Fix compat shader namespace"
> from Jun 9, 2014, leads to the following static checker warning:
>
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:477 vmw_cmd_res_reloc_add()
> warn: missing error code here? 'kzalloc()' failed.
>
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> 468
> 469 ret = vmw_resource_context_res_add(dev_priv,
> sw_context, res);
> 470 if (unlikely(ret != 0))
> 471 goto out_err;
> 472 node->staged_bindings =
> 473 kzalloc(sizeof(*node->staged_bindings),
> GFP_KERNEL);
> 474 if (node->staged_bindings == NULL) {
> 475 DRM_ERROR("Failed to allocate context
> binding "
> 476"information.\n");
> 477 goto out_err;
>
> This should just be "return -ENOMEM;". The goto is misleading because
> you expect it to do something useful.
Indeed. Thanks for pointing that out. Since this is old code being
reorganized, the goto slipped through. The missing error code has been
around for a while, though. I'll put together a patch for that.
>
> Soon checkpatch.pl will start complaining about the extra DRM_ERROR()
> because kzalloc() has a more useful printk builtin and this just wastes
> memory and makes the code more verbose.
Noted.
>
> Speaking of verbose, all the likely/unlikely annotations should be
> removed.
Is this your personal opinion or has there been some kind of kernel
developer agreement not to add this annotation and remove it from the
kernel tree? If not, I prefer to keep it.
>If the code were more readable then the missing error code
> would have been more noticeable. This code is buggy because it is ugly;
> there is a direct cause effect relationship.
I think ugliness in this case is in the eye of the beholder. The bug
likely entered long ago like these bugs tend to do because you're not
100% focused when the code is written. I find this statement a bit
incoherent because there's no branch prediction hint in the if statement
preceding the bug and although the error message may be redundant in
this case, I can't see why an error message would make the code ugly or
be the cause of a bug.
>
> 478 }
> 479 INIT_LIST_HEAD(>staged_bindings->list);
> 480 }
> 481
> 482 if (p_val)
> 483 *p_val = node;
> 484
> 485 out_err:
> 486 return ret;
> 487 }
>
> regards,
> dan carpenter
Thanks,
Thomas
drm/vmwgfx: Fix compat shader namespace
Hello Thomas Hellstrom,
The patch 18e4a4669c50: "drm/vmwgfx: Fix compat shader namespace"
from Jun 9, 2014, leads to the following static checker warning:
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:477 vmw_cmd_res_reloc_add()
warn: missing error code here? 'kzalloc()' failed.
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
468
469 ret = vmw_resource_context_res_add(dev_priv,
sw_context, res);
470 if (unlikely(ret != 0))
471 goto out_err;
472 node->staged_bindings =
473 kzalloc(sizeof(*node->staged_bindings),
GFP_KERNEL);
474 if (node->staged_bindings == NULL) {
475 DRM_ERROR("Failed to allocate context binding "
476"information.\n");
477 goto out_err;
This should just be "return -ENOMEM;". The goto is misleading because
you expect it to do something useful.
Soon checkpatch.pl will start complaining about the extra DRM_ERROR()
because kzalloc() has a more useful printk builtin and this just wastes
memory and makes the code more verbose.
Speaking of verbose, all the likely/unlikely annotations should be
removed. If the code were more readable then the missing error code
would have been more noticeable. This code is buggy because it is ugly;
there is a direct cause effect relationship.
478 }
479 INIT_LIST_HEAD(>staged_bindings->list);
480 }
481
482 if (p_val)
483 *p_val = node;
484
485 out_err:
486 return ret;
487 }
regards,
dan carpenter
[PATCH] drm/vmwgfx: Fix compat shader namespace
Contrary to the host-backed shader interface that has a per-context
name-space for shaders, the compat shader namespace was per client
(or rather, per file). Fix this so that the compat shader namespace is per
context, and at the same time, make command buffer managed context resource
management generic.
Signed-off-by: Thomas Hellstrom
Reviewed-by: Jakob Bornecrantz
---
drivers/gpu/drm/vmwgfx/Makefile| 3 +-
drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf_res.c | 341 +
drivers/gpu/drm/vmwgfx/vmwgfx_context.c| 38 ++-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c| 7 -
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h| 74 --
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c| 227 +
drivers/gpu/drm/vmwgfx/vmwgfx_shader.c | 396 +
7 files changed, 673 insertions(+), 413 deletions(-)
create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf_res.c
diff --git a/drivers/gpu/drm/vmwgfx/Makefile b/drivers/gpu/drm/vmwgfx/Makefile
index 458cdf6..ce0ab95 100644
--- a/drivers/gpu/drm/vmwgfx/Makefile
+++ b/drivers/gpu/drm/vmwgfx/Makefile
@@ -6,6 +6,7 @@ vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o
vmwgfx_drv.o \
vmwgfx_fifo.o vmwgfx_irq.o vmwgfx_ldu.o vmwgfx_ttm_glue.o \
vmwgfx_overlay.o vmwgfx_marker.o vmwgfx_gmrid_manager.o \
vmwgfx_fence.o vmwgfx_dmabuf.o vmwgfx_scrn.o vmwgfx_context.o \
- vmwgfx_surface.o vmwgfx_prime.o vmwgfx_mob.o vmwgfx_shader.o
+ vmwgfx_surface.o vmwgfx_prime.o vmwgfx_mob.o vmwgfx_shader.o \
+ vmwgfx_cmdbuf_res.o \
obj-$(CONFIG_DRM_VMWGFX) := vmwgfx.o
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf_res.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf_res.c
new file mode 100644
index 000..bfeb4b1
--- /dev/null
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf_res.c
@@ -0,0 +1,341 @@
+/**
+ *
+ * Copyright ? 2014 VMware, Inc., Palo Alto, CA., USA
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ **/
+
+#include "vmwgfx_drv.h"
+
+#define VMW_CMDBUF_RES_MAN_HT_ORDER 12
+
+enum vmw_cmdbuf_res_state {
+ VMW_CMDBUF_RES_COMMITED,
+ VMW_CMDBUF_RES_ADD,
+ VMW_CMDBUF_RES_DEL
+};
+
+/**
+ * struct vmw_cmdbuf_res - Command buffer managed resource entry.
+ *
+ * @res: Refcounted pointer to a struct vmw_resource.
+ * @hash: Hash entry for the manager hash table.
+ * @head: List head used either by the staging list or the manager list
+ * of commited resources.
+ * @state: Staging state of this resource entry.
+ * @man: Pointer to a resource manager for this entry.
+ */
+struct vmw_cmdbuf_res {
+ struct vmw_resource *res;
+ struct drm_hash_item hash;
+ struct list_head head;
+ enum vmw_cmdbuf_res_state state;
+ struct vmw_cmdbuf_res_manager *man;
+};
+
+/**
+ * struct vmw_cmdbuf_res_manager - Command buffer resource manager.
+ *
+ * @resources: Hash table containing staged and commited command buffer
+ * resources
+ * @list: List of commited command buffer resources.
+ * @dev_priv: Pointer to a device private structure.
+ *
+ * @resources and @list are protected by the cmdbuf mutex for now.
+ */
+struct vmw_cmdbuf_res_manager {
+ struct drm_open_hash resources;
+ struct list_head list;
+ struct vmw_private *dev_priv;
+};
+
+
+/**
+ * vmw_cmdbuf_res_lookup - Look up a command buffer resource
+ *
+ * @man: Pointer to the command buffer resource manager
+ * @resource_type: The resource type, that combined with the user key
+ * identifies the resource.
+ * @user_key: The user key.
+ *
+ * Returns a valid refcounted struct vmw_resource pointer on success,
+ * an error pointer on failure.
+ */
+struct vmw_resource *
+vmw_cmdbuf_res_lookup(struct
