Re: [PATCH v5 2/9] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Brian Starkey <brian.starkey@arm.com>]

Hi Alex,

On Fri, Oct 19, 2018 at 11:57:45AM +0100, Alexandru Gheorghe wrote:
>For some pixel formats .cpp structure in drm_format info it's not
>enough to describe the peculiarities of the pixel layout, for example
>tiled formats or packed formats at bit level.
>
>What's implemented here is to add three new members to drm_format_info
>that could describe such formats:
>
>- char_per_block[3]
>- block_w[3]
>- block_h[3]
>
>char_per_block will be put in a union alongside cpp, for transparent
>compatibility  with the existing format descriptions.
>
>Regarding, block_w and block_h they are intended to be used through
>their equivalent getters drm_format_info_block_width /
>drm_format_info_block_height, the reason of the getters is to abstract
>the fact that for normal formats block_w and block_h will be unset/0,
>but the methods will be returning 1.
>
>Additionally, convenience function drm_format_info_min_pitch had been
>added that computes the minimum required pitch for a given pixel
>format and buffer width.
>
>Using that the following drm core functions had been updated to
>generically handle both block and non-block formats:
>
>- drm_fb_cma_get_gem_addr: for block formats it will just return the
>  beginning of the block.
>- framebuffer_check: Use the newly added drm_format_info_min_pitch.
>- drm_gem_fb_create_with_funcs: Use the newly added
>  drm_format_info_min_pitch.
>- In places where is not expecting to handle block formats, like fbdev
>  helpers I just added some warnings in case the block width/height
>  are greater than 1.
>
>Changes since v3:
> - Add helper function for computing the minimum required pitch.
> - Improve/cleanup documentation
>
>Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

I commented on a couple of tiny typographical things below, but
otherwise this looks good to me. Thanks!

Reviewed-by: Brian Starkey <brian.starkey@arm.com>

>---
> drivers/gpu/drm/drm_fb_cma_helper.c          | 21 ++++++-
> drivers/gpu/drm/drm_fb_helper.c              |  6 ++
> drivers/gpu/drm/drm_fourcc.c                 | 62 ++++++++++++++++++++
> drivers/gpu/drm/drm_framebuffer.c            |  6 +-
> drivers/gpu/drm/drm_gem_framebuffer_helper.c |  2 +-
> include/drm/drm_fourcc.h                     | 61 ++++++++++++++++++-
> 6 files changed, 149 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
>index d52344a03aa8..83941a8ca0e0 100644
>--- a/drivers/gpu/drm/drm_fb_cma_helper.c
>+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>@@ -72,7 +72,9 @@ struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
> EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
>
> /**
>- * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer
>+ * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer, for pixel
>+ * formats where values are grouped in blocks this will get you the beginning of
>+ * the block
>  * @fb: The framebuffer
>  * @state: Which state of drm plane
>  * @plane: Which plane
>@@ -87,6 +89,14 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> 	struct drm_gem_cma_object *obj;
> 	dma_addr_t paddr;
> 	u8 h_div = 1, v_div = 1;
>+	u32 block_w = drm_format_info_block_width(fb->format, plane);
>+	u32 block_h = drm_format_info_block_height(fb->format, plane);
>+	u32 block_size = fb->format->char_per_block[plane];
>+	u32 sample_x;
>+	u32 sample_y;
>+	u32 block_start_y;
>+	u32 num_hblocks;
>+

nit: extra newline

>
> 	obj = drm_fb_cma_get_gem_obj(fb, plane);
> 	if (!obj)
>@@ -99,8 +109,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> 		v_div = fb->format->vsub;
> 	}
>
>-	paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
>-	paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
>+	sample_x = (state->src_x >> 16) / h_div;
>+	sample_y = (state->src_y >> 16) / v_div;
>+	block_start_y = (sample_y / block_h) * block_h;
>+	num_hblocks = sample_x / block_w;
>+
>+	paddr += fb->pitches[plane] * block_start_y;
>+	paddr += block_size * num_hblocks;
>
> 	return paddr;
> }
>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>index a504a5e05676..9add0d7da744 100644
>--- a/drivers/gpu/drm/drm_fb_helper.c
>+++ b/drivers/gpu/drm/drm_fb_helper.c
>@@ -1595,6 +1595,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> 	if (var->pixclock != 0 || in_dbg_master())
> 		return -EINVAL;
>
>+	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
>+	    (drm_format_info_block_height(fb->format, 0) > 1))
>+		return -EINVAL;
>+
> 	/*
> 	 * Changes struct fb_var_screeninfo are currently not pushed back
> 	 * to KMS, hence fail if different settings are requested.
>@@ -1969,6 +1973,8 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> {
> 	struct drm_framebuffer *fb = fb_helper->fb;
>
>+	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
>+		(drm_format_info_block_height(fb->format, 0) > 1));
> 	info->pseudo_palette = fb_helper->pseudo_palette;
> 	info->var.xres_virtual = fb->width;
> 	info->var.yres_virtual = fb->height;
>diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>index e177f6d0d1f4..a843a5fc8dbf 100644
>--- a/drivers/gpu/drm/drm_fourcc.c
>+++ b/drivers/gpu/drm/drm_fourcc.c
>@@ -403,3 +403,65 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
> 	return height / info->vsub;
> }
> EXPORT_SYMBOL(drm_format_plane_height);
>+
>+/**
>+ * drm_format_info_block_width - width in pixels of block.
>+ * @info: pixel format info
>+ * @plane: plane index
>+ *
>+ * Returns:
>+ * The width in pixels of a block, depending on the plane index.
>+ */
>+unsigned int drm_format_info_block_width(const struct drm_format_info *info,
>+					 int plane)
>+{
>+	if (!info || plane < 0 || plane >= info->num_planes)
>+		return 0;

Thinking aloud: The other helpers don't check < 0, but it looks to me
that it would make sense (someday...) to change all of the 'plane'
arguments to 'unsigned int' so that there's no possibility of < 0.

>+
>+	if (!info->block_w[plane])
>+		return 1;
>+	return info->block_w[plane];
>+}
>+EXPORT_SYMBOL(drm_format_info_block_width);
>+
>+/**
>+ * drm_format_info_block_height - height in pixels of a block
>+ * @info: pixel format info
>+ * @plane: plane index
>+ *
>+ * Returns:
>+ * The height in pixels of a block, depending on the plane index.
>+ */
>+unsigned int drm_format_info_block_height(const struct drm_format_info *info,
>+					  int plane)
>+{
>+	if (!info || plane < 0 || plane >= info->num_planes)
>+		return 0;
>+
>+	if (!info->block_h[plane])
>+		return 1;
>+	return info->block_h[plane];
>+}
>+EXPORT_SYMBOL(drm_format_info_block_height);
>+
>+/**
>+ * drm_format_info_min_pitch - computes the minimum required pitch in bytes
>+ * @info: pixel format info
>+ * @plane: plane index
>+ * @buffer_width: buffer width in pixels
>+ *
>+ * Returns:
>+ * The minimum required pitch in bytes for a buffer by taking into consideration
>+ * the pixel format information and the buffer width.
>+ */
>+uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
>+				   int plane, unsigned int buffer_width)
>+{
>+	if (!info || plane < 0 || plane >= info->num_planes)
>+		return 0;
>+
>+	return DIV_ROUND_UP((u64)buffer_width * info->char_per_block[plane],
>+			    drm_format_info_block_width(info, plane) *
>+			    drm_format_info_block_height(info, plane));
>+}
>+EXPORT_SYMBOL(drm_format_info_min_pitch);
>diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>index 3bf729d0aae5..6aca8a1ccdb6 100644
>--- a/drivers/gpu/drm/drm_framebuffer.c
>+++ b/drivers/gpu/drm/drm_framebuffer.c
>@@ -195,20 +195,20 @@ static int framebuffer_check(struct drm_device *dev,
> 	for (i = 0; i < info->num_planes; i++) {
> 		unsigned int width = fb_plane_width(r->width, info, i);
> 		unsigned int height = fb_plane_height(r->height, info, i);
>-		unsigned int cpp = info->cpp[i];
>+		u64 min_pitch = drm_format_info_min_pitch(info, i, width);
>
> 		if (!r->handles[i]) {
> 			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
> 			return -EINVAL;
> 		}
>
>-		if ((uint64_t) width * cpp > UINT_MAX)
>+		if (min_pitch > UINT_MAX)
> 			return -ERANGE;
>
> 		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
> 			return -ERANGE;
>
>-		if (r->pitches[i] < width * cpp) {
>+		if (r->pitches[i] < min_pitch) {
> 			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> 			return -EINVAL;
> 		}
>diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>index ded7a379ac35..acb466d25afc 100644
>--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>@@ -171,7 +171,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> 		}
>
> 		min_size = (height - 1) * mode_cmd->pitches[i]
>-			 + width * info->cpp[i]
>+			 + drm_format_info_min_pitch(info, i, width)
> 			 + mode_cmd->offsets[i];
>
> 		if (objs[i]->size < min_size) {
>diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
>index 345f11227e9e..253d4c07a10c 100644
>--- a/include/drm/drm_fourcc.h
>+++ b/include/drm/drm_fourcc.h
>@@ -69,8 +69,59 @@ struct drm_format_info {
> 	/** @num_planes: Number of color planes (1 to 3) */
> 	u8 num_planes;
>
>-	/** @cpp: Number of bytes per pixel (per plane) */
>-	u8 cpp[3];
>+	union {
>+		/**
>+		 * @cpp:
>+		 *
>+		 * Number of bytes per pixel (per plane), this is aliased with
>+		 * @char_per_block. It is deprecated in favour of using the
>+		 * triplet @char_per_block, @block_w, @block_h for better
>+		 * describing the pixel format.
>+		 */
>+		u8 cpp[3];
>+
>+		/**
>+		 * @char_per_block:
>+		 *
>+		 * Number of bytes per block (per plane), where blocks are
>+		 * defined as a rectangle of pixels which are stored next to
>+		 * each other in a byte aligned memory region. Together with
>+		 * @block_w and @block_h this is used to properly describe tiles
>+		 * in tiled formats or to describe groups of pixels in packed
>+		 * formats for which the memory needed for a single pixel it's

s/it's/is/

>+		 * not byte aligned.
>+		 *
>+		 * @cpp had been kept from historical reasons because there are

s/had/has/, s/from/for/

>+		 * a lot of places in drivers where it's used. In drm core for
>+		 * generic code paths the preferred way is to use
>+		 * @char_per_block, drm_format_info_block_width() and
>+		 * drm_format_info_block_height() which allows handling both
>+		 * block and non-block formats in the same way.
>+		 *
>+		 * For formats that are intended to be used only with non-linear
>+		 * modifiers both @cpp and @char_per_block must be 0 in the
>+		 * generic format table. Drivers could supply accurate
>+		 * information from their drm_mode_config.get_format_info hook
>+		 * if they want the core to be validating the pitch.
>+		 */
>+		u8 char_per_block[3];
>+	};
>+
>+	/**
>+	 * @block_w:
>+	 *
>+	 * Block width in pixels, this is intended to be accessed through
>+	 * drm_format_info_block_width()
>+	 */
>+	u8 block_w[3];
>+
>+	/**
>+	 * @block_h:
>+	 *
>+	 * Block height in pixels, this is intended to be accessed through
>+	 * drm_format_info_block_height()
>+	 */
>+	u8 block_h[3];
>
> 	/** @hsub: Horizontal chroma subsampling factor */
> 	u8 hsub;
>@@ -106,6 +157,12 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
> int drm_format_vert_chroma_subsampling(uint32_t format);
> int drm_format_plane_width(int width, uint32_t format, int plane);
> int drm_format_plane_height(int height, uint32_t format, int plane);
>+unsigned int drm_format_info_block_width(const struct drm_format_info *info,
>+					 int plane);
>+unsigned int drm_format_info_block_height(const struct drm_format_info *info,
>+					  int plane);
>+uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
>+				   int plane, unsigned int buffer_width);
> const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>
> #endif /* __DRM_FOURCC_H__ */
>-- 
>2.18.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel