| Message ID | 20181019105752.17741-3-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add method to describe tile/bit_level_packed formats | expand |
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 >
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.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. 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 > >
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; + 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; + + 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 + * not byte aligned. + * + * @cpp had been kept from 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__ */
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> --- 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(-)