[PATCH v5 2/9] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info
Brian Starkey
brian.starkey at arm.com
Fri Oct 19 13:09:38 UTC 2018
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 at 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 at 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
>
More information about the dri-devel
mailing list