Re: [PATCH 1/3] habanalabs: implement dma-fence mechanism

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Jul 13, 2020 at 06:54:22PM +0300, Oded Gabbay wrote:
> From: Ofir Bitton <obitton@habana.ai>
> 
> Instead of using standard dma-fence mechanism designed for GPU's, we
> introduce our own implementation based on the former one. This
> implementation is much more sparse than the original, contains only
> mandatory functionality required by the driver.

Sad you can't use the in-kernel code for this, I really don't understand
what's wrong with using it as-is.

Daniel, why do we need/want duplicate code floating around in the tree
like this?

Copying code leads to errors, here's some documentation ones:

> --- /dev/null
> +++ b/drivers/misc/habanalabs/hl_dma_fence.c
> @@ -0,0 +1,338 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Fence mechanism for dma-buf and to allow for asynchronous dma access

Is that what this still does?

> + *
> + * Copyright (C) 2012 Canonical Ltd
> + * Copyright (C) 2012 Texas Instruments
> + *
> + * Authors:
> + * Rob Clark <robdclark@gmail.com>
> + * Maarten Lankhorst <maarten.lankhorst@canonical.com>
> + *
> + * The dma_fence module is a copy of dma-fence at drivers/dma-buf.

"The hl_dma_fence" module...

And is it a stand-alone module?  Or just a single file?

> + * This was done due to an explicit request by GPU developers who asked not
> + * to use the dma-buf module because we aren't part of DRM subsystem.

Why is dma-buf only for use for DRM?

If it is, should the symbol namespace be set to that to catch users that
want to use it for their own code?

> + * This copy was stripped from all extra features that habanalabs driver
> + * doesn't use, including the uapi interface dma-buf exposes.
> + * In addition, we removed the callbacks because the only usage is from inside
> + * habanalabs driver
> + */
> +
> +#include "hl_dma_fence.h"
> +#include "habanalabs.h"
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/atomic.h>
> +#include <linux/sched/signal.h>
> +
> +/**
> + * DOC: DMA fences overview
> + *
> + * DMA fences, represented by &struct hl_dma_fence, are the kernel internal
> + * synchronization primitive for DMA operations like GPU rendering, video
> + * encoding/decoding, or displaying buffers on a screen.

I don't think this is correct anymore, right?  :(

> --- /dev/null
> +++ b/drivers/misc/habanalabs/hl_dma_fence.h
> @@ -0,0 +1,148 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Fence mechanism for dma-buf to allow for asynchronous dma access
> + *
> + * Copyright (C) 2012 Canonical Ltd
> + * Copyright (C) 2012 Texas Instruments
> + *
> + * Authors:
> + * Rob Clark <robdclark@gmail.com>
> + * Maarten Lankhorst <maarten.lankhorst@canonical.com>
> + *
> + * The dma_fence module is a copy of dma-fence at drivers/dma-buf.

Same comments here for the .h file.

thanks,

greg k-h