From: Thomas Zimmermann <tzimmermann@suse.de>
To: Lucas De Marchi <lucas.demarchi@intel.com>,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Cc: airlied@linux.ie, daniel.vetter@ffwll.ch,
christian.koenig@amd.com, srinivas.kandagatla@linaro.org,
gregkh@linuxfoundation.org, nouveau@lists.freedesktop.org,
linux-media@vger.kernel.org
Subject: Re: [PATCH 01/14] iosys-map: Introduce renamed dma-buf-map
Date: Fri, 28 Jan 2022 09:53:59 +0100 [thread overview]
Message-ID: <37e3c34b-6fe3-5270-2128-40158b6e0c9e@suse.de> (raw)
In-Reply-To: <20220128083626.3012259-2-lucas.demarchi@intel.com>
[-- Attachment #1.1: Type: text/plain, Size: 12249 bytes --]
Hi
Am 28.01.22 um 09:36 schrieb Lucas De Marchi:
> Add a new type, struct iosys_map, to eventually replace
> struct dma_buf_map and its helpers defiend in
> include/linux/dma-buf-map.h.
>
> This is mostly a copy of dma-buf-map with the renames in place and
> slightly different wording to avoid tying iosys_map to dma-buf: in fact
> it's just a shim layer to abstract system memory, that can be accessed
> via regular load and store, from IO memory that needs to be acessed via
> arch helpers. Over time the dma-buf-map proved useful outside of buffer
> sharing among drivers and started to be used in helper functions for
> allocation and generic use. See e.g.
>
> drivers/gpu/drm/drm_gem_shmem_helper.c
> drivers/gpu/drm/drm_gem_framebuffer_helper.c
> drivers/gpu/drm/drm_fb_helper.c
Well, that was the original motivation: framebuffer memory can be
located in system or I/O memory, and even change their location. For
some architectures this makes difference. IIRC the framebuffer console
crashed on sparc64 because we didn't access I/O memory in the correct
way. Hence, we added dma_buf_map to return the type of memory from
dma_buf_vmap/dma_buf_vunmap. And because everything is tied together,
we had quite a bit of churn throughout the DRM/media code. There are
still places in DRM where we access the raw pointers within dma_buf_map.
We need to clean this up at some point.
>
> In the i915 driver it's also desired to share the implementation for
> integrated graphics, which uses mostly system memory, with discrete
> graphics, which may need to access IO memory.
>
> Once all the drivers using dma_buf_map are converted, the dma_buf_map
> can be retired and iosys_map extended to cover new use cases.
Wrt the renaming: the old name isn't good and the new one isn't good
either. But I don't have strong feelings for either of them.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> MAINTAINERS | 1 +
> include/linux/dma-buf-map.h | 3 +
> include/linux/iosys-map.h | 254 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 258 insertions(+)
> create mode 100644 include/linux/iosys-map.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ea3e6c914384..27ebaded85f8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5734,6 +5734,7 @@ F: Documentation/driver-api/dma-buf.rst
> F: drivers/dma-buf/
> F: include/linux/*fence.h
> F: include/linux/dma-buf*
> +F: include/linux/iosys-map.h
If anything, I'd complain tha twe now have something in dma-buf that
isn't obviously connected to dma-buf.
Best regards
Thomas
> F: include/linux/dma-resv.h
> K: \bdma_(?:buf|fence|resv)\b
>
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 19fa0b5ae5ec..4b4b2930660b 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -263,4 +263,7 @@ static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> map->vaddr += incr;
> }
>
> +/* Temporary include for API migration */
> +#include <linux/iosys-map.h>
> +
> #endif /* __DMA_BUF_MAP_H__ */
> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
> new file mode 100644
> index 000000000000..ad1f08f8f97f
> --- /dev/null
> +++ b/include/linux/iosys-map.h
> @@ -0,0 +1,254 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Pointer abstraction for IO/system memory
> + */
> +
> +#ifndef __IOSYS_MAP_H__
> +#define __IOSYS_MAP_H__
> +
> +#include <linux/io.h>
> +#include <linux/string.h>
> +
> +/* Temporary include while user of dma-buf-map are converted to iosys-map */
> +#include <linux/dma-buf-map.h>
> +
> +/**
> + * DOC: overview
> + *
> + * When accessing a memory region, depending on the its location, users may have
> + * to access it with I/O operations or memory load/store operations. For
> + * example, copying to system memory could be done with memcpy(), copying to I/O
> + * memory would be done with memcpy_toio().
> + *
> + * .. code-block:: c
> + *
> + * void *vaddr = ...; // pointer to system memory
> + * memcpy(vaddr, src, len);
> + *
> + * void *vaddr_iomem = ...; // pointer to I/O memory
> + * memcpy_toio(vaddr, _iomem, src, len);
> + *
> + * The user of such pointer may not have information about the mapping of that
> + * region or may want to have a single code path to handle operations on that
> + * buffer, regardless if it's located in system or IO memory. The type
> + * :c:type:`struct iosys_map <iosys_map>` and its helpers abstract that so the
> + * buffer can be passed around to other drivers or have separate duties inside
> + * the same driver for allocation, read and write operations.
> + *
> + * Open-coding access to :c:type:`struct iosys_map <iosys_map>` is considered
> + * bad style. Rather then accessing its fields directly, use one of the provided
> + * helper functions, or implement your own. For example, instances of
> + * :c:type:`struct iosys_map <iosys_map>` can be initialized statically with
> + * IOSYS_MAP_INIT_VADDR(), or at runtime with iosys_map_set_vaddr(). These
> + * helpers will set an address in system memory.
> + *
> + * .. code-block:: c
> + *
> + * struct iosys_map map = IOSYS_MAP_INIT_VADDR(0xdeadbeaf);
> + *
> + * iosys_map_set_vaddr(&map, 0xdeadbeaf);
> + *
> + * To set an address in I/O memory, use iosys_map_set_vaddr_iomem().
> + *
> + * .. code-block:: c
> + *
> + * iosys_map_set_vaddr_iomem(&map, 0xdeadbeaf);
> + *
> + * Instances of struct iosys_map do not have to be cleaned up, but
> + * can be cleared to NULL with iosys_map_clear(). Cleared mappings
> + * always refer to system memory.
> + *
> + * .. code-block:: c
> + *
> + * iosys_map_clear(&map);
> + *
> + * Test if a mapping is valid with either iosys_map_is_set() or
> + * iosys_map_is_null().
> + *
> + * .. code-block:: c
> + *
> + * if (iosys_map_is_set(&map) != iosys_map_is_null(&map))
> + * // always true
> + *
> + * Instances of :c:type:`struct iosys_map <iosys_map>` can be compared for
> + * equality with iosys_map_is_equal(). Mappings that point to different memory
> + * spaces, system or I/O, are never equal. That's even true if both spaces are
> + * located in the same address space, both mappings contain the same address
> + * value, or both mappings refer to NULL.
> + *
> + * .. code-block:: c
> + *
> + * struct iosys_map sys_map; // refers to system memory
> + * struct iosys_map io_map; // refers to I/O memory
> + *
> + * if (iosys_map_is_equal(&sys_map, &io_map))
> + * // always false
> + *
> + * A set up instance of struct iosys_map can be used to access or manipulate the
> + * buffer memory. Depending on the location of the memory, the provided helpers
> + * will pick the correct operations. Data can be copied into the memory with
> + * iosys_map_memcpy_to(). The address can be manipulated with iosys_map_incr().
> + *
> + * .. code-block:: c
> + *
> + * const void *src = ...; // source buffer
> + * size_t len = ...; // length of src
> + *
> + * iosys_map_memcpy_to(&map, src, len);
> + * iosys_map_incr(&map, len); // go to first byte after the memcpy
> + */
> +
> +/**
> + * struct iosys_map - Pointer to IO/system memory
> + * @vaddr_iomem: The buffer's address if in I/O memory
> + * @vaddr: The buffer's address if in system memory
> + * @is_iomem: True if the buffer is located in I/O memory, or false
> + * otherwise.
> + */
> +#define iosys_map dma_buf_map
> +
> +/**
> + * IOSYS_MAP_INIT_VADDR - Initializes struct iosys_map to an address in system memory
> + * @vaddr_: A system-memory address
> + */
> +#define IOSYS_MAP_INIT_VADDR(vaddr_) \
> + { \
> + .vaddr = (vaddr_), \
> + .is_iomem = false, \
> + }
> +
> +/**
> + * iosys_map_set_vaddr - Sets a iosys mapping structure to an address in system memory
> + * @map: The iosys_map structure
> + * @vaddr: A system-memory address
> + *
> + * Sets the address and clears the I/O-memory flag.
> + */
> +static inline void iosys_map_set_vaddr(struct iosys_map *map, void *vaddr)
> +{
> + map->vaddr = vaddr;
> + map->is_iomem = false;
> +}
> +
> +/**
> + * iosys_map_set_vaddr_iomem - Sets a iosys mapping structure to an address in I/O memory
> + * @map: The iosys_map structure
> + * @vaddr_iomem: An I/O-memory address
> + *
> + * Sets the address and the I/O-memory flag.
> + */
> +static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
> + void __iomem *vaddr_iomem)
> +{
> + map->vaddr_iomem = vaddr_iomem;
> + map->is_iomem = true;
> +}
> +
> +/**
> + * iosys_map_is_equal - Compares two iosys mapping structures for equality
> + * @lhs: The iosys_map structure
> + * @rhs: A iosys_map structure to compare with
> + *
> + * Two iosys mapping structures are equal if they both refer to the same type of memory
> + * and to the same address within that memory.
> + *
> + * Returns:
> + * True is both structures are equal, or false otherwise.
> + */
> +static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
> + const struct iosys_map *rhs)
> +{
> + if (lhs->is_iomem != rhs->is_iomem)
> + return false;
> + else if (lhs->is_iomem)
> + return lhs->vaddr_iomem == rhs->vaddr_iomem;
> + else
> + return lhs->vaddr == rhs->vaddr;
> +}
> +
> +/**
> + * iosys_map_is_null - Tests for a iosys mapping to be NULL
> + * @map: The iosys_map structure
> + *
> + * Depending on the state of struct iosys_map.is_iomem, tests if the
> + * mapping is NULL.
> + *
> + * Returns:
> + * True if the mapping is NULL, or false otherwise.
> + */
> +static inline bool iosys_map_is_null(const struct iosys_map *map)
> +{
> + if (map->is_iomem)
> + return !map->vaddr_iomem;
> + return !map->vaddr;
> +}
> +
> +/**
> + * iosys_map_is_set - Tests if the iosys mapping has been set
> + * @map: The iosys_map structure
> + *
> + * Depending on the state of struct iosys_map.is_iomem, tests if the
> + * mapping has been set.
> + *
> + * Returns:
> + * True if the mapping is been set, or false otherwise.
> + */
> +static inline bool iosys_map_is_set(const struct iosys_map *map)
> +{
> + return !iosys_map_is_null(map);
> +}
> +
> +/**
> + * iosys_map_clear - Clears a iosys mapping structure
> + * @map: The iosys_map structure
> + *
> + * Clears all fields to zero, including struct iosys_map.is_iomem, so
> + * mapping structures that were set to point to I/O memory are reset for
> + * system memory. Pointers are cleared to NULL. This is the default.
> + */
> +static inline void iosys_map_clear(struct iosys_map *map)
> +{
> + if (map->is_iomem) {
> + map->vaddr_iomem = NULL;
> + map->is_iomem = false;
> + } else {
> + map->vaddr = NULL;
> + }
> +}
> +
> +/**
> + * iosys_map_memcpy_to - Memcpy into iosys mapping
> + * @dst: The iosys_map structure
> + * @src: The source buffer
> + * @len: The number of byte in src
> + *
> + * Copies data into a iosys mapping. The source buffer is in system
> + * memory. Depending on the buffer's location, the helper picks the correct
> + * method of accessing the memory.
> + */
> +static inline void iosys_map_memcpy_to(struct iosys_map *dst, const void *src,
> + size_t len)
> +{
> + if (dst->is_iomem)
> + memcpy_toio(dst->vaddr_iomem, src, len);
> + else
> + memcpy(dst->vaddr, src, len);
> +}
> +
> +/**
> + * iosys_map_incr - Increments the address stored in a iosys mapping
> + * @map: The iosys_map structure
> + * @incr: The number of bytes to increment
> + *
> + * Increments the address stored in a iosys mapping. Depending on the
> + * buffer's location, the correct value will be updated.
> + */
> +static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
> +{
> + if (map->is_iomem)
> + map->vaddr_iomem += incr;
> + else
> + map->vaddr += incr;
> +}
> +
> +#endif /* __IOSYS_MAP_H__ */
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
next prev parent reply other threads:[~2022-01-28 8:54 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-28 8:36 [PATCH 00/14] Rename dma-buf-map Lucas De Marchi
2022-01-28 8:36 ` [PATCH 01/14] iosys-map: Introduce renamed dma-buf-map Lucas De Marchi
2022-01-28 8:53 ` Thomas Zimmermann [this message]
2022-01-28 9:24 ` Lucas De Marchi
2022-01-28 9:39 ` Thomas Zimmermann
2022-01-28 22:26 ` Daniel Vetter
2022-01-28 8:36 ` [PATCH 02/14] misc: fastrpc: Replace dma-buf-map with iosys-map Lucas De Marchi
2022-01-28 8:36 ` [PATCH 03/14] dma-buf: " Lucas De Marchi
2022-01-28 8:36 ` [PATCH 04/14] media: " Lucas De Marchi
2022-01-28 8:36 ` [PATCH 05/14] drm/ttm: " Lucas De Marchi
2022-01-28 8:36 ` [PATCH 06/14] drm: Replace dma-buf-map with iosys-map in drivers Lucas De Marchi
2022-01-28 8:36 ` [PATCH 07/14] drm/i915: Replace dma-buf-map with iosys-map Lucas De Marchi
2022-01-28 8:36 ` [PATCH 08/14] drm/msm: " Lucas De Marchi
2022-01-28 8:36 ` [PATCH 09/14] drm/nouveau: " Lucas De Marchi
2022-01-28 19:17 ` Lyude Paul
2022-01-28 8:36 ` [PATCH 10/14] drm/tegra: " Lucas De Marchi
2022-01-28 8:36 ` [PATCH 11/14] drm/radeon: " Lucas De Marchi
2022-01-28 8:36 ` [PATCH 12/14] drm: Replace dma-buf-map with iosys-map in common code Lucas De Marchi
2022-01-28 8:36 ` [PATCH 13/14] Documentation: Refer to iosys-map instead of dma-buf-map Lucas De Marchi
2022-01-28 8:36 ` [PATCH 14/14] dma-buf-map: Remove API in favor of iosys-map Lucas De Marchi
2022-01-28 8:41 ` [PATCH 00/14] Rename dma-buf-map Christian König
2022-01-28 9:12 ` Lucas De Marchi
2022-01-28 9:22 ` Christian König
2022-01-28 9:40 ` Lucas De Marchi
2022-01-28 9:48 ` Christian König
2022-02-01 0:36 ` Lucas De Marchi
2022-02-01 7:46 ` Christian König
2022-02-01 8:08 ` Thomas Zimmermann
2022-02-01 22:08 ` Lucas De Marchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=37e3c34b-6fe3-5270-2128-40158b6e0c9e@suse.de \
--to=tzimmermann@suse.de \
--cc=airlied@linux.ie \
--cc=christian.koenig@amd.com \
--cc=daniel.vetter@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=lucas.demarchi@intel.com \
--cc=nouveau@lists.freedesktop.org \
--cc=srinivas.kandagatla@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).