On Mon, Oct 29, 2018 at 05:14:37PM +0000, Alexandru-Cosmin 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
>
> Reviewed-by: Brian Starkey <brian.star...@arm.com>
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheor...@arm.com>
Really neat kerneldoc now.
Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> ---
> drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++-
> 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, 148 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> b/drivers/gpu/drm/drm_fb_cma_helper.c
> index fc2b42dd3dc6..b07ab3f613e0 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,13 @@ 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;
>
> obj = drm_fb_cma_get_gem_obj(fb, plane);
> if (!obj)
> @@ -99,8 +108,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 9b111e846847..8024524f0547 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1614,6 +1614,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.
> @@ -1988,6 +1992,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;
> +
> + 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..bcb389f04618 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 is not
> + * byte aligned.
> + *
> + * @cpp has been kept for historical reasons because there are
> + * 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.19.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
