Hi

Am 25.07.21 um 21:49 schrieb Sam Ravnborg:

Hi Thomas,

On Sun, Jul 25, 2021 at 07:44:34PM +0200, Thomas Zimmermann wrote:

...

DRM uses a magic number of 4 for the maximum number of planes per color format. Declare this constant via DRM_FORMAT_MAX_PLANES and update the related code. Some code depends on the length of arrays that are now declared with DRM_FORMAT_MAX_PLANES. Convert it from '4' to ARRAY_SIZE.

v2:

• mention usage of ARRAY_SIZE() in the commit message (Maxime)
• also fix error handling in drm_gem_fb_init_with_funcs() (kernel test robot)
• include <drm/drm_fourcc.h> for DRM_FORMAT_MAX_PLANES

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

One nit below. Reviewed-by: Sam Ravnborg sam@ravnborg.org

Thanks a lot for your reviews.

...

---

drivers/gpu/drm/drm_gem_framebuffer_helper.c | 19 +++++++++++-------- include/drm/drm_fourcc.h | 13 +++++++++---- include/drm/drm_framebuffer.h | 8 ++++---- include/drm/drm_gem_atomic_helper.h | 3 ++- 4 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 67bc9edc1d98..421e029a6b3e 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -48,7 +48,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, unsigned int plane) {

• if (plane >= 4)

• if (plane >= ARRAY_SIZE(fb->obj)) return NULL;

  return fb->obj[plane];

@@ -62,7 +62,8 @@ drm_gem_fb_init(struct drm_device *dev, struct drm_gem_object **obj, unsigned int num_planes, const struct drm_framebuffer_funcs *funcs) {

• int ret, i;

• unsigned int i;

• int ret;

  drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);

This change looks accidental/unrelated. But I guess it is to be consistent in use of unsigned int when iterating the array.

Indeed. The current code uses a mixture of signed and unsigned types in several places. DRM_FORMAT_MAX_PLANES is defined as unsigned, so I updated all the related code accordingly.

Best regards Thomas

...

@@ -86,9 +87,9 @@ drm_gem_fb_init(struct drm_device *dev, */ void drm_gem_fb_destroy(struct drm_framebuffer *fb) {

• int i;

• size_t i;

• for (i = 0; i < 4; i++)

• for (i = 0; i < ARRAY_SIZE(fb->obj); i++) drm_gem_object_put(fb->obj[i]);

  drm_framebuffer_cleanup(fb);

@@ -145,8 +146,9 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev, const struct drm_framebuffer_funcs *funcs) { const struct drm_format_info *info;

• struct drm_gem_object *objs[4];
• int ret, i;

• struct drm_gem_object *objs[DRM_FORMAT_MAX_PLANES];

• unsigned int i;

• int ret;

  info = drm_get_format_info(dev, mode_cmd); if (!info) {

@@ -187,9 +189,10 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev, return 0;

err_gem_object_put:

• for (i--; i >= 0; i--)

• while (i > 0) {

• --i;
  

  drm_gem_object_put(objs[i]);

• } return ret; } EXPORT_SYMBOL_GPL(drm_gem_fb_init_with_funcs);

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 3b138d4ae67e..22aa64d07c79 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -25,6 +25,11 @@ #include <linux/types.h> #include <uapi/drm/drm_fourcc.h>

+/**

• • DRM_FORMAT_MAX_PLANES - maximum number of planes a DRM format can have
• */

+#define DRM_FORMAT_MAX_PLANES 4u

• /*
  • DRM formats are little endian. Define host endian variants for the
  • most common formats here, to reduce the #ifdefs needed in drivers.

@@ -78,7 +83,7 @@ struct drm_format_info { * triplet @char_per_block, @block_w, @block_h for better * describing the pixel format. */

• u8 cpp[4];
  

• u8 cpp[DRM_FORMAT_MAX_PLANES];
  
  

  /**

  • @char_per_block:

@@ -104,7 +109,7 @@ struct drm_format_info { * information from their drm_mode_config.get_format_info hook * if they want the core to be validating the pitch. */

• u8 char_per_block[4];
  

• u8 char_per_block[DRM_FORMAT_MAX_PLANES];
  

  };

  /**

@@ -113,7 +118,7 @@ struct drm_format_info { * Block width in pixels, this is intended to be accessed through * drm_format_info_block_width() */

• u8 block_w[4];

• u8 block_w[DRM_FORMAT_MAX_PLANES];

  /**

  • @block_h:

@@ -121,7 +126,7 @@ struct drm_format_info { * Block height in pixels, this is intended to be accessed through * drm_format_info_block_height() */

• u8 block_h[4];

• u8 block_h[DRM_FORMAT_MAX_PLANES];

  /** @hsub: Horizontal chroma subsampling factor */ u8 hsub;

diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index be658ebbec72..f67c5b7bcb68 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -27,12 +27,12 @@ #include <linux/list.h> #include <linux/sched.h>

+#include <drm/drm_fourcc.h> #include <drm/drm_mode_object.h>

struct drm_clip_rect; struct drm_device; struct drm_file; -struct drm_format_info; struct drm_framebuffer; struct drm_gem_object;

@@ -147,7 +147,7 @@ struct drm_framebuffer { * @pitches: Line stride per buffer. For userspace created object this * is copied from drm_mode_fb_cmd2. */

• unsigned int pitches[4];

• unsigned int pitches[DRM_FORMAT_MAX_PLANES]; /**
  • @offsets: Offset from buffer start to the actual pixel data in bytes,
  • per buffer. For userspace created object this is copied from

@@ -165,7 +165,7 @@ struct drm_framebuffer { * data (even for linear buffers). Specifying an x/y pixel offset is * instead done through the source rectangle in &struct drm_plane_state. */

• unsigned int offsets[4];

• unsigned int offsets[DRM_FORMAT_MAX_PLANES]; /**
  • @modifier: Data layout modifier. This is used to describe
  • tiling, or also special layouts (like compression) of auxiliary

@@ -210,7 +210,7 @@ struct drm_framebuffer { * This is used by the GEM framebuffer helpers, see e.g. * drm_gem_fb_create(). */

• struct drm_gem_object *obj[4];

• struct drm_gem_object *obj[DRM_FORMAT_MAX_PLANES]; };

  #define obj_to_fb(x) container_of(x, struct drm_framebuffer, base)

diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h index d82c23622156..f9f8b6f0494a 100644 --- a/include/drm/drm_gem_atomic_helper.h +++ b/include/drm/drm_gem_atomic_helper.h @@ -5,6 +5,7 @@

#include <linux/dma-buf-map.h>

+#include <drm/drm_fourcc.h> #include <drm/drm_plane.h>

struct drm_simple_display_pipe; @@ -40,7 +41,7 @@ struct drm_shadow_plane_state { * The memory mappings stored in map should be established in the plane's * prepare_fb callback and removed in the cleanup_fb callback. */

• struct dma_buf_map map[4];

• struct dma_buf_map map[DRM_FORMAT_MAX_PLANES]; };

  /**

-- 2.32.0

On Sun, Jul 25, 2021 at 07:44:36PM +0200, Thomas Zimmermann wrote:

Set the returned mapping address to NULL if a framebuffer plane does not have a BO associated with it. Likewise, ignore mappings of NULL during framebuffer unmap operations. Allows users of the functions to perform unmap operations of certain BOs by themselfes.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Reviewed-by: Sam Ravnborg sam@ravnborg.org

---

drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 243affbad437..02928607a716 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -336,8 +336,10 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb,

for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) { obj = drm_gem_fb_get_obj(fb, i);

• if (!obj)
  

• if (!obj) {
  

• 	dma_buf_map_clear(&map[i]);
  continue;
  

• }
  

  ret = drm_gem_vmap(obj, &map[i]); if (ret) goto err_drm_gem_vunmap;

@@ -377,6 +379,8 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb, obj = drm_gem_fb_get_obj(fb, i); if (!obj) continue;

• if (dma_buf_map_is_null(&map[i]))
  

• 	continue;
  

  drm_gem_vunmap(obj, &map[i]); }

}

2.32.0

On Sun, Jul 25, 2021 at 07:44:37PM +0200, Thomas Zimmermann wrote:

Abstract the framebuffer details by mapping its BOs with a call to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunmap().

The call to drm_gem_fb_vmap() ensures that all BOs are mapped correctly. Gud still only supports single-plane formats.

No functional changes.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Noralf Trønnes noralf@tronnes.org

Acked-by: Sam Ravnborg sam@ravnborg.org

---

drivers/gpu/drm/gud/gud_pipe.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c index 4d7a26b68a2e..7e009f562b30 100644 --- a/drivers/gpu/drm/gud/gud_pipe.c +++ b/drivers/gpu/drm/gud/gud_pipe.c @@ -14,8 +14,8 @@ #include <drm/drm_format_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_framebuffer.h> +#include <drm/drm_gem.h> #include <drm/drm_gem_framebuffer_helper.h> -#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_print.h> #include <drm/drm_rect.h> #include <drm/drm_simple_kms_helper.h> @@ -152,7 +152,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, { struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach; u8 compression = gdrm->compression;

• struct dma_buf_map map;

• struct dma_buf_map map[DRM_FORMAT_MAX_PLANES]; void *vaddr, *buf; size_t pitch, len; int ret = 0;

@@ -162,11 +162,11 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, if (len > gdrm->bulk_len) return -E2BIG;

• ret = drm_gem_shmem_vmap(fb->obj[0], &map);

• ret = drm_gem_fb_vmap(fb, map); if (ret) return ret;

• vaddr = map.vaddr + fb->offsets[0];

• vaddr = map[0].vaddr + fb->offsets[0];

  ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); if (ret)

@@ -225,7 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, end_cpu_access: drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); vunmap:

• drm_gem_shmem_vunmap(fb->obj[0], &map);

• drm_gem_fb_vunmap(fb, map);

  return ret;

}

2.32.0

On Sun, Jul 25, 2021 at 07:44:38PM +0200, Thomas Zimmermann wrote:

Abstract the framebuffer details by mappings its BOs with a call to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunamp().

Before, the output address with stored as raw pointer in the priv field of struct drm_writeback_job. Introduce the new type struct vkms_writeback_job, which holds the output mappings addresses while the writeback job is active.

The patchset also cleans up some internal casting an setup of the output addresses. No functional changes.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Acked-by: Sam Ravnborg sam@ravnborg.org

---

drivers/gpu/drm/vkms/vkms_composer.c | 2 +- drivers/gpu/drm/vkms/vkms_drv.h | 6 +++++- drivers/gpu/drm/vkms/vkms_writeback.c | 21 ++++++++++----------- 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index ead8fff81f30..49f109c3a2b3 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -257,7 +257,7 @@ void vkms_composer_worker(struct work_struct *work) return;

if (wb_pending)

• vaddr_out = crtc_state->active_writeback;
  

• vaddr_out = crtc_state->active_writeback->map[0].vaddr;
  
  

  ret = compose_active_planes(&vaddr_out, primary_composer, crtc_state);

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 8c731b6dcba7..8bc9e3f52e1f 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -20,6 +20,10 @@ #define XRES_MAX 8192 #define YRES_MAX 8192

+struct vkms_writeback_job {

• struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];

+};

struct vkms_composer { struct drm_framebuffer fb; struct drm_rect src, dst; @@ -57,7 +61,7 @@ struct vkms_crtc_state { int num_active_planes; /* stack of active planes for crc computation, should be in z order */ struct vkms_plane_state **active_planes;

• void *active_writeback;

• struct vkms_writeback_job *active_writeback;

  /* below four are protected by vkms_output.composer_lock */ bool crc_pending;

diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 0935686475a0..765bb85ba76c 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -65,21 +65,23 @@ static int vkms_wb_connector_get_modes(struct drm_connector *connector) static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector, struct drm_writeback_job *job) {

• struct drm_gem_object *gem_obj;
• struct dma_buf_map map;

• struct vkms_writeback_job *vkmsjob; int ret;

  if (!job->fb) return 0;

• gem_obj = drm_gem_fb_get_obj(job->fb, 0);
• ret = drm_gem_shmem_vmap(gem_obj, &map);

• vkmsjob = kzalloc(sizeof(*vkmsjob), GFP_KERNEL);
• if (!vkmsjob)

• return -ENOMEM;
  

• ret = drm_gem_fb_vmap(job->fb, vkmsjob->map); if (ret) { DRM_ERROR("vmap failed: %d\n", ret); return ret; }

• job->priv = map.vaddr;

• job->priv = vkmsjob;

  return 0;

} @@ -87,18 +89,15 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector, static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector, struct drm_writeback_job *job) {

• struct drm_gem_object *gem_obj;

• struct vkms_writeback_job *vkmsjob = job->priv; struct vkms_device *vkmsdev;

• struct dma_buf_map map;

  if (!job->fb) return;

• gem_obj = drm_gem_fb_get_obj(job->fb, 0);

• dma_buf_map_set_vaddr(&map, job->priv);

• drm_gem_shmem_vunmap(gem_obj, &map);

• drm_gem_fb_vunmap(job->fb, vkmsjob->map);

• vkmsdev = drm_device_to_vkms_device(gem_obj->dev);

• vkmsdev = drm_device_to_vkms_device(job->fb->dev); vkms_set_composer(&vkmsdev->output, false);

}

-- 2.32.0