On 06/14/2018 01:19 AM, Boris Ostrovsky wrote:
On 06/13/2018 07:57 AM, Oleksandr Andrushchenko wrote:
On 06/13/2018 05:58 AM, Boris Ostrovsky wrote:
On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:
+
+static struct gntdev_dmabuf *
+dmabuf_exp_wait_obj_get_by_fd(struct gntdev_dmabuf_priv *priv, int fd)
The name of this routine implies (to me) that we are getting a wait
object but IIUIC we are getting a gntdev_dmabuf that we are going to
later associate with a wait object.
How about dmabuf_exp_wait_obj_get_dmabuf_by_fd?
I would like to keep function prefixes, e.g. dmabuf_exp_wait_obj_
just to show to which functionality a routine belongs.
That's too long IMO. If you really want to keep the prefix then let's
keep this the way it is. Maybe it's just me who read it that way.
I'll rename it to dmabuf_exp_wait_obj_get_dmabuf.
As it already wants fd it seems to be clear that
the lookup will be based on it.
+#ifdef CONFIG_XEN_GNTDEV_DMABUF
+void gntdev_remove_map(struct gntdev_priv *priv, struct
gntdev_grant_map *map)
+{
+ mutex_lock(&priv->lock);
+ list_del(&map->next);
+ gntdev_put_map(NULL /* already removed */, map);
Why not pass call gntdev_put_map(priv, map) and then not have this
routine at all?
Well, I wish I could, but the main difference when calling
gntdev_put_map(priv, map)
with priv != NULL and my code is that:
void gntdev_put_map(struct gntdev_priv *priv, struct gntdev_grant_map
*map)
{
[...]
if (populate_freeable_maps && priv) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
mutex_lock(&priv->lock);
list_del(&map->next);
mutex_unlock(&priv->lock);
}
[...]
}
and
#define populate_freeable_maps use_ptemod
^^^^^^^^^^
which means the map will never be removed from the list in my case
because use_ptemod == false for dma-buf.
This is why I do that by hand, e.g. remove the item from the list
and then pass NULL for priv.
Also, I will remove gntdev_remove_map as I can now access
priv->lock and gntdev_put_map directly form gntdev-dmabuf.c
Yes, that's a good idea.
I really dislike the fact that we are taking a lock here that
gntdev_put_map() takes as well, although not with NULL argument. (And
yes, I see that gntdev_release() does it too.)
This can be re-factored later I guess?
OK.
-boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
