On Fri, Oct 19, 2018 at 02:09:38PM +0100, Brian Starkey wrote:
> 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.gheor...@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.star...@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.
The only reason is int it's for consistency with the others, and I
added <0, once I started unit testing it.
>
> >+
> >+ 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
> >
--
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
