[RFC PATCH v2 0/3] Groundwork for AFBC YUV formats

[RFC PATCH v2 0/3] Groundwork for AFBC YUV formats

[Brian Starkey]

Hi,

This is the second round of RFC for adding a bunch of new YUV formats
for Mali/AFBC. I've included a proper AFBC documentation file too, for
posterity.

Some of the new formats don't have an integer number of
bytes-per-pixel, so I've added a bpp field to drm_format_info
(patch 1), keen to hear thoughts on that.

These three patches don't make a huge amount of sense in isolation, so
we'll pull them into a more comprehensive series for AFBC support to
merge them later.

Thanks,
-Brian

Brian Starkey (3):
  drm/fourcc: Add 'bpp' field for formats with non-integer
    bytes-per-pixel
  drm/fourcc: Add remaining fourccs for Mali
  drm/afbc: Add AFBC modifier usage documentation

 Documentation/gpu/afbc.rst                   | 226 +++++++++++++++++++++++++++
 Documentation/gpu/drivers.rst                |   1 +
 MAINTAINERS                                  |   1 +
 drivers/gpu/drm/drm_fb_cma_helper.c          |   6 +-
 drivers/gpu/drm/drm_fb_helper.c              |   8 +-
 drivers/gpu/drm/drm_fourcc.c                 |  57 +++++++
 drivers/gpu/drm/drm_framebuffer.c            |  41 ++++-
 drivers/gpu/drm/drm_gem_framebuffer_helper.c |   3 +-
 include/drm/drm_fourcc.h                     |   4 +
 include/uapi/drm/drm_fourcc.h                |  17 ++
 10 files changed, 351 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/gpu/afbc.rst

-- 
2.16.1

[RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Brian Starkey]

Some formats have a non-integer number of bytes per pixel, which can't
be handled with the existing 'cpp' field in drm_format_info. To handle
these formats, add a 'bpp' field, which is only used if cpp[0] == 0.

This updates all the users of format->cpp in the core DRM code,
converting them to use a new function to get the bits-per-pixel for any
format.

It's assumed that drivers will use the 'bpp' field when they add support
for pixel formats with non-integer bytes-per-pixel.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
 drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
 drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
 include/drm/drm_fourcc.h                     |  4 +++
 6 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 186d00adfb5f..e279d70d3e60 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
 {
 	struct drm_gem_cma_object *obj;
 	dma_addr_t paddr;
+	u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
+
+	/* This can't work for non-integer bytes-per-pixel */
+	WARN_ON(bpp % 8);
 
 	obj = drm_fb_cma_get_gem_obj(fb, plane);
 	if (!obj)
 		return 0;
 
 	paddr = obj->paddr + fb->offsets[plane];
-	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
+	paddr += (bpp / 8) * (state->src_x >> 16);
 	paddr += fb->pitches[plane] * (state->src_y >> 16);
 
 	return paddr;
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0646b108030b..ab369f250af4 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_framebuffer *fb = fb_helper->fb;
 	int depth;
+	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
 
 	if (var->pixclock != 0 || in_dbg_master())
 		return -EINVAL;
@@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	 * Changes struct fb_var_screeninfo are currently not pushed back
 	 * to KMS, hence fail if different settings are requested.
 	 */
-	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
+	if (var->bits_per_pixel != bpp ||
 	    var->xres > fb->width || var->yres > fb->height ||
 	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
 		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
 			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
 			  var->xres, var->yres, var->bits_per_pixel,
 			  var->xres_virtual, var->yres_virtual,
-			  fb->width, fb->height, fb->format->cpp[0] * 8);
+			  fb->width, fb->height, bpp);
 		return -EINVAL;
 	}
 
@@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
 			    uint32_t fb_width, uint32_t fb_height)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
+	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
 
 	info->pseudo_palette = fb_helper->pseudo_palette;
 	info->var.xres_virtual = fb->width;
 	info->var.yres_virtual = fb->height;
-	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
+	info->var.bits_per_pixel = bpp;
 	info->var.accel_flags = FB_ACCELF_TEXT;
 	info->var.xoffset = 0;
 	info->var.yoffset = 0;
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 3b42c25bd58d..bb28919c32f3 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
 	if (!info || plane >= info->num_planes)
 		return 0;
 
+	/*
+	 * Not valid for formats with non-integer cpp,
+	 * use drm_format{_info}_plane_bpp instead
+	 */
+	WARN_ON(!info->cpp[0]);
+
 	return info->cpp[plane];
 }
 EXPORT_SYMBOL(drm_format_plane_cpp);
 
+/**
+ * drm_format_plane_bpp - determine the bits per pixel value
+ * @format: pixel format (DRM_FORMAT_*)
+ * @plane: plane index
+ *
+ * Returns:
+ * The bits per pixel value for the specified plane.
+ */
+int drm_format_plane_bpp(uint32_t format, int plane)
+{
+	const struct drm_format_info *info;
+
+	info = drm_format_info(format);
+	if (!info)
+		return 0;
+
+	return drm_format_info_plane_bpp(info, plane);
+}
+EXPORT_SYMBOL(drm_format_plane_bpp);
+
+/**
+ * drm_format_info_plane_bpp - determine the bits per pixel value
+ *
+ * Convenience function which handles formats with both integer
+ * and non-integer bytes-per-pixel.
+ *
+ * @format: pixel format info structure
+ * @plane: plane index
+ *
+ * Returns:
+ * The bits per pixel value for the specified plane.
+ */
+int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
+{
+	if (plane >= info->num_planes)
+		return 0;
+
+	if (info->cpp[0])
+		return info->cpp[plane] * 8;
+
+	return info->bpp[plane];
+}
+EXPORT_SYMBOL(drm_format_info_plane_bpp);
+
 /**
  * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
  * @format: pixel format (DRM_FORMAT_*)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 8c4d32adcc17..7e00360ff70d 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -185,20 +185,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];
+		unsigned int bpp = drm_format_info_plane_bpp(info, i);
 
 		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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
 			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
 			return -EINVAL;
 		}
@@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->height = fb->height;
 	r->width = fb->width;
 	r->depth = fb->format->depth;
-	r->bpp = fb->format->cpp[0] * 8;
+	r->bpp = drm_format_info_plane_bpp(fb->format, 0);
 	r->pitch = fb->pitches[0];
 
 	/* GET_FB() is an unprivileged ioctl so we must not return a
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index acfbc0641a06..dfe224ccaeba 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
 		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
 		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
 		unsigned int min_size;
+		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
 
 		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
 		if (!objs[i]) {
@@ -170,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]
+			 + DIV_ROUND_UP(width * bpp, 8)
 			 + 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 3e86408dac9f..d4af4dab1623 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
  *	use in new code and set to 0 for new formats.
  * @num_planes: Number of color planes (1 to 3)
  * @cpp: Number of bytes per pixel (per plane)
+ * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
  * @hsub: Horizontal chroma subsampling factor
  * @vsub: Vertical chroma subsampling factor
  * @has_alpha: Does the format embeds an alpha component?
@@ -45,6 +46,7 @@ struct drm_format_info {
 	u8 depth;
 	u8 num_planes;
 	u8 cpp[3];
+	u8 bpp[3];
 	u8 hsub;
 	u8 vsub;
 	bool has_alpha;
@@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
 uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
 int drm_format_num_planes(uint32_t format);
 int drm_format_plane_cpp(uint32_t format, int plane);
+int drm_format_plane_bpp(uint32_t format, int plane);
+int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
 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);
-- 
2.16.1

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Daniel Vetter]

On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
> Some formats have a non-integer number of bytes per pixel, which can't
> be handled with the existing 'cpp' field in drm_format_info. To handle
> these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
> 
> This updates all the users of format->cpp in the core DRM code,
> converting them to use a new function to get the bits-per-pixel for any
> format.
> 
> It's assumed that drivers will use the 'bpp' field when they add support
> for pixel formats with non-integer bytes-per-pixel.
> 
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>

I assume you still require that stuff is eventually aligned to bytes? In
that case, can we subsume this into the tile work Alex is doing? It's
essentially just another special case of having storage-size units
measured in bytes which span more than 1x1 pixel. And I kinda don't want a
metric pile of special cases here in the format code, because that just
means every driver handles a different subset, with different bugs.
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
>  drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
>  drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
>  include/drm/drm_fourcc.h                     |  4 +++
>  6 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 186d00adfb5f..e279d70d3e60 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
>  {
>  	struct drm_gem_cma_object *obj;
>  	dma_addr_t paddr;
> +	u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
> +
> +	/* This can't work for non-integer bytes-per-pixel */
> +	WARN_ON(bpp % 8);
>  
>  	obj = drm_fb_cma_get_gem_obj(fb, plane);
>  	if (!obj)
>  		return 0;
>  
>  	paddr = obj->paddr + fb->offsets[plane];
> -	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
> +	paddr += (bpp / 8) * (state->src_x >> 16);
>  	paddr += fb->pitches[plane] * (state->src_y >> 16);
>  
>  	return paddr;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0646b108030b..ab369f250af4 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  	struct drm_fb_helper *fb_helper = info->par;
>  	struct drm_framebuffer *fb = fb_helper->fb;
>  	int depth;
> +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>  
>  	if (var->pixclock != 0 || in_dbg_master())
>  		return -EINVAL;
> @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  	 * Changes struct fb_var_screeninfo are currently not pushed back
>  	 * to KMS, hence fail if different settings are requested.
>  	 */
> -	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
> +	if (var->bits_per_pixel != bpp ||
>  	    var->xres > fb->width || var->yres > fb->height ||
>  	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
>  		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
>  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
>  			  var->xres, var->yres, var->bits_per_pixel,
>  			  var->xres_virtual, var->yres_virtual,
> -			  fb->width, fb->height, fb->format->cpp[0] * 8);
> +			  fb->width, fb->height, bpp);
>  		return -EINVAL;
>  	}
>  
> @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
>  			    uint32_t fb_width, uint32_t fb_height)
>  {
>  	struct drm_framebuffer *fb = fb_helper->fb;
> +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>  
>  	info->pseudo_palette = fb_helper->pseudo_palette;
>  	info->var.xres_virtual = fb->width;
>  	info->var.yres_virtual = fb->height;
> -	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
> +	info->var.bits_per_pixel = bpp;
>  	info->var.accel_flags = FB_ACCELF_TEXT;
>  	info->var.xoffset = 0;
>  	info->var.yoffset = 0;
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 3b42c25bd58d..bb28919c32f3 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
>  	if (!info || plane >= info->num_planes)
>  		return 0;
>  
> +	/*
> +	 * Not valid for formats with non-integer cpp,
> +	 * use drm_format{_info}_plane_bpp instead
> +	 */
> +	WARN_ON(!info->cpp[0]);
> +
>  	return info->cpp[plane];
>  }
>  EXPORT_SYMBOL(drm_format_plane_cpp);
>  
> +/**
> + * drm_format_plane_bpp - determine the bits per pixel value
> + * @format: pixel format (DRM_FORMAT_*)
> + * @plane: plane index
> + *
> + * Returns:
> + * The bits per pixel value for the specified plane.
> + */
> +int drm_format_plane_bpp(uint32_t format, int plane)
> +{
> +	const struct drm_format_info *info;
> +
> +	info = drm_format_info(format);
> +	if (!info)
> +		return 0;
> +
> +	return drm_format_info_plane_bpp(info, plane);
> +}
> +EXPORT_SYMBOL(drm_format_plane_bpp);
> +
> +/**
> + * drm_format_info_plane_bpp - determine the bits per pixel value
> + *
> + * Convenience function which handles formats with both integer
> + * and non-integer bytes-per-pixel.
> + *
> + * @format: pixel format info structure
> + * @plane: plane index
> + *
> + * Returns:
> + * The bits per pixel value for the specified plane.
> + */
> +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
> +{
> +	if (plane >= info->num_planes)
> +		return 0;
> +
> +	if (info->cpp[0])
> +		return info->cpp[plane] * 8;
> +
> +	return info->bpp[plane];
> +}
> +EXPORT_SYMBOL(drm_format_info_plane_bpp);
> +
>  /**
>   * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
>   * @format: pixel format (DRM_FORMAT_*)
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 8c4d32adcc17..7e00360ff70d 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -185,20 +185,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];
> +		unsigned int bpp = drm_format_info_plane_bpp(info, i);
>  
>  		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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
>  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
>  			return -EINVAL;
>  		}
> @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
>  	r->height = fb->height;
>  	r->width = fb->width;
>  	r->depth = fb->format->depth;
> -	r->bpp = fb->format->cpp[0] * 8;
> +	r->bpp = drm_format_info_plane_bpp(fb->format, 0);
>  	r->pitch = fb->pitches[0];
>  
>  	/* GET_FB() is an unprivileged ioctl so we must not return a
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index acfbc0641a06..dfe224ccaeba 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>  		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
>  		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
>  		unsigned int min_size;
> +		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
>  
>  		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
>  		if (!objs[i]) {
> @@ -170,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]
> +			 + DIV_ROUND_UP(width * bpp, 8)
>  			 + 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 3e86408dac9f..d4af4dab1623 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
>   *	use in new code and set to 0 for new formats.
>   * @num_planes: Number of color planes (1 to 3)
>   * @cpp: Number of bytes per pixel (per plane)
> + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
>   * @hsub: Horizontal chroma subsampling factor
>   * @vsub: Vertical chroma subsampling factor
>   * @has_alpha: Does the format embeds an alpha component?
> @@ -45,6 +46,7 @@ struct drm_format_info {
>  	u8 depth;
>  	u8 num_planes;
>  	u8 cpp[3];
> +	u8 bpp[3];
>  	u8 hsub;
>  	u8 vsub;
>  	bool has_alpha;
> @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
>  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
>  int drm_format_num_planes(uint32_t format);
>  int drm_format_plane_cpp(uint32_t format, int plane);
> +int drm_format_plane_bpp(uint32_t format, int plane);
> +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
>  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);
> -- 
> 2.16.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Brian Starkey]

Hi Daniel,

On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
>On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
>> Some formats have a non-integer number of bytes per pixel, which can't
>> be handled with the existing 'cpp' field in drm_format_info. To handle
>> these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
>>
>> This updates all the users of format->cpp in the core DRM code,
>> converting them to use a new function to get the bits-per-pixel for any
>> format.
>>
>> It's assumed that drivers will use the 'bpp' field when they add support
>> for pixel formats with non-integer bytes-per-pixel.
>>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>
>I assume you still require that stuff is eventually aligned to bytes? In
>that case, can we subsume this into the tile work Alex is doing? It's
>essentially just another special case of having storage-size units
>measured in bytes which span more than 1x1 pixel. And I kinda don't want a
>metric pile of special cases here in the format code, because that just
>means every driver handles a different subset, with different bugs.
>-Daniel

Sorry for the delay, been struggling to free some cycles to think
about this.

I'm not sure how to pull this in with the tiling stuff. In the AFBC
case then our AFBC superblocks are always nice round numbers (256
pixels), and so it does end up being a multiple of bytes.

However, AFBC supports different superblock sizes, so picking just one
doesn't really work out, and putting AFBC in the core format table
which reflects AFBC doesn't seem good.

We could make something up (e.g. call these formats "tiled" with 2x4
tiles, which guarantees a multiple of 8), but it would be an
arbitrarily-selected lie, which often seems to spell trouble. If we
did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
we still need to add a new field anyway.

What's the pile of special cases you're worried about? The helper I've
added here means that drivers which need to care can use one API and
not implement their own bugs.

Cheers,
-Brian

>
>> ---
>>  drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
>>  drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
>>  drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
>>  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
>>  include/drm/drm_fourcc.h                     |  4 +++
>>  6 files changed, 70 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
>> index 186d00adfb5f..e279d70d3e60 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
>>  {
>>  	struct drm_gem_cma_object *obj;
>>  	dma_addr_t paddr;
>> +	u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
>> +
>> +	/* This can't work for non-integer bytes-per-pixel */
>> +	WARN_ON(bpp % 8);
>>
>>  	obj = drm_fb_cma_get_gem_obj(fb, plane);
>>  	if (!obj)
>>  		return 0;
>>
>>  	paddr = obj->paddr + fb->offsets[plane];
>> -	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
>> +	paddr += (bpp / 8) * (state->src_x >> 16);
>>  	paddr += fb->pitches[plane] * (state->src_y >> 16);
>>
>>  	return paddr;
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 0646b108030b..ab369f250af4 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>>  	struct drm_fb_helper *fb_helper = info->par;
>>  	struct drm_framebuffer *fb = fb_helper->fb;
>>  	int depth;
>> +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>>
>>  	if (var->pixclock != 0 || in_dbg_master())
>>  		return -EINVAL;
>> @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>>  	 * Changes struct fb_var_screeninfo are currently not pushed back
>>  	 * to KMS, hence fail if different settings are requested.
>>  	 */
>> -	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
>> +	if (var->bits_per_pixel != bpp ||
>>  	    var->xres > fb->width || var->yres > fb->height ||
>>  	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
>>  		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
>>  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
>>  			  var->xres, var->yres, var->bits_per_pixel,
>>  			  var->xres_virtual, var->yres_virtual,
>> -			  fb->width, fb->height, fb->format->cpp[0] * 8);
>> +			  fb->width, fb->height, bpp);
>>  		return -EINVAL;
>>  	}
>>
>> @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
>>  			    uint32_t fb_width, uint32_t fb_height)
>>  {
>>  	struct drm_framebuffer *fb = fb_helper->fb;
>> +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>>
>>  	info->pseudo_palette = fb_helper->pseudo_palette;
>>  	info->var.xres_virtual = fb->width;
>>  	info->var.yres_virtual = fb->height;
>> -	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
>> +	info->var.bits_per_pixel = bpp;
>>  	info->var.accel_flags = FB_ACCELF_TEXT;
>>  	info->var.xoffset = 0;
>>  	info->var.yoffset = 0;
>> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>> index 3b42c25bd58d..bb28919c32f3 100644
>> --- a/drivers/gpu/drm/drm_fourcc.c
>> +++ b/drivers/gpu/drm/drm_fourcc.c
>> @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
>>  	if (!info || plane >= info->num_planes)
>>  		return 0;
>>
>> +	/*
>> +	 * Not valid for formats with non-integer cpp,
>> +	 * use drm_format{_info}_plane_bpp instead
>> +	 */
>> +	WARN_ON(!info->cpp[0]);
>> +
>>  	return info->cpp[plane];
>>  }
>>  EXPORT_SYMBOL(drm_format_plane_cpp);
>>
>> +/**
>> + * drm_format_plane_bpp - determine the bits per pixel value
>> + * @format: pixel format (DRM_FORMAT_*)
>> + * @plane: plane index
>> + *
>> + * Returns:
>> + * The bits per pixel value for the specified plane.
>> + */
>> +int drm_format_plane_bpp(uint32_t format, int plane)
>> +{
>> +	const struct drm_format_info *info;
>> +
>> +	info = drm_format_info(format);
>> +	if (!info)
>> +		return 0;
>> +
>> +	return drm_format_info_plane_bpp(info, plane);
>> +}
>> +EXPORT_SYMBOL(drm_format_plane_bpp);
>> +
>> +/**
>> + * drm_format_info_plane_bpp - determine the bits per pixel value
>> + *
>> + * Convenience function which handles formats with both integer
>> + * and non-integer bytes-per-pixel.
>> + *
>> + * @format: pixel format info structure
>> + * @plane: plane index
>> + *
>> + * Returns:
>> + * The bits per pixel value for the specified plane.
>> + */
>> +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
>> +{
>> +	if (plane >= info->num_planes)
>> +		return 0;
>> +
>> +	if (info->cpp[0])
>> +		return info->cpp[plane] * 8;
>> +
>> +	return info->bpp[plane];
>> +}
>> +EXPORT_SYMBOL(drm_format_info_plane_bpp);
>> +
>>  /**
>>   * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
>>   * @format: pixel format (DRM_FORMAT_*)
>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> index 8c4d32adcc17..7e00360ff70d 100644
>> --- a/drivers/gpu/drm/drm_framebuffer.c
>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>> @@ -185,20 +185,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];
>> +		unsigned int bpp = drm_format_info_plane_bpp(info, i);
>>
>>  		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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
>>  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
>>  			return -EINVAL;
>>  		}
>> @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
>>  	r->height = fb->height;
>>  	r->width = fb->width;
>>  	r->depth = fb->format->depth;
>> -	r->bpp = fb->format->cpp[0] * 8;
>> +	r->bpp = drm_format_info_plane_bpp(fb->format, 0);
>>  	r->pitch = fb->pitches[0];
>>
>>  	/* GET_FB() is an unprivileged ioctl so we must not return a
>> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> index acfbc0641a06..dfe224ccaeba 100644
>> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>>  		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
>>  		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
>>  		unsigned int min_size;
>> +		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
>>
>>  		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
>>  		if (!objs[i]) {
>> @@ -170,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]
>> +			 + DIV_ROUND_UP(width * bpp, 8)
>>  			 + 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 3e86408dac9f..d4af4dab1623 100644
>> --- a/include/drm/drm_fourcc.h
>> +++ b/include/drm/drm_fourcc.h
>> @@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
>>   *	use in new code and set to 0 for new formats.
>>   * @num_planes: Number of color planes (1 to 3)
>>   * @cpp: Number of bytes per pixel (per plane)
>> + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
>>   * @hsub: Horizontal chroma subsampling factor
>>   * @vsub: Vertical chroma subsampling factor
>>   * @has_alpha: Does the format embeds an alpha component?
>> @@ -45,6 +46,7 @@ struct drm_format_info {
>>  	u8 depth;
>>  	u8 num_planes;
>>  	u8 cpp[3];
>> +	u8 bpp[3];
>>  	u8 hsub;
>>  	u8 vsub;
>>  	bool has_alpha;
>> @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
>>  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
>>  int drm_format_num_planes(uint32_t format);
>>  int drm_format_plane_cpp(uint32_t format, int plane);
>> +int drm_format_plane_bpp(uint32_t format, int plane);
>> +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
>>  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);
>> --
>> 2.16.1
>>
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Daniel Vetter]

On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote:
> Hi Daniel,
> 
> On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
> > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
> > > Some formats have a non-integer number of bytes per pixel, which can't
> > > be handled with the existing 'cpp' field in drm_format_info. To handle
> > > these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
> > > 
> > > This updates all the users of format->cpp in the core DRM code,
> > > converting them to use a new function to get the bits-per-pixel for any
> > > format.
> > > 
> > > It's assumed that drivers will use the 'bpp' field when they add support
> > > for pixel formats with non-integer bytes-per-pixel.
> > > 
> > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > 
> > I assume you still require that stuff is eventually aligned to bytes? In
> > that case, can we subsume this into the tile work Alex is doing? It's
> > essentially just another special case of having storage-size units
> > measured in bytes which span more than 1x1 pixel. And I kinda don't want a
> > metric pile of special cases here in the format code, because that just
> > means every driver handles a different subset, with different bugs.
> > -Daniel
> 
> Sorry for the delay, been struggling to free some cycles to think
> about this.
> 
> I'm not sure how to pull this in with the tiling stuff. In the AFBC
> case then our AFBC superblocks are always nice round numbers (256
> pixels), and so it does end up being a multiple of bytes.
> 
> However, AFBC supports different superblock sizes, so picking just one
> doesn't really work out, and putting AFBC in the core format table
> which reflects AFBC doesn't seem good.
> 
> We could make something up (e.g. call these formats "tiled" with 2x4
> tiles, which guarantees a multiple of 8), but it would be an
> arbitrarily-selected lie, which often seems to spell trouble. If we
> did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
> we still need to add a new field anyway.
> 
> What's the pile of special cases you're worried about? The helper I've
> added here means that drivers which need to care can use one API and
> not implement their own bugs.

I'm confused ... the new bits-per-pixel stuff you're adding here is for
yuv formats, not afbc. I'm just suggesting we have only 1 way of
describing such formats that need more descriptive power than cpp, whether
they have some kind of pixel-groups or small tiles.

For very special stuff like afbc you need to validate in the driver
anyway, too complicated. So I have no idea why you bring this up here?
-Daniel

> 
> Cheers,
> -Brian
> 
> > 
> > > ---
> > >  drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
> > >  drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
> > >  drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
> > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
> > >  include/drm/drm_fourcc.h                     |  4 +++
> > >  6 files changed, 70 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > index 186d00adfb5f..e279d70d3e60 100644
> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > >  {
> > >  	struct drm_gem_cma_object *obj;
> > >  	dma_addr_t paddr;
> > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
> > > +
> > > +	/* This can't work for non-integer bytes-per-pixel */
> > > +	WARN_ON(bpp % 8);
> > > 
> > >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
> > >  	if (!obj)
> > >  		return 0;
> > > 
> > >  	paddr = obj->paddr + fb->offsets[plane];
> > > -	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
> > > +	paddr += (bpp / 8) * (state->src_x >> 16);
> > >  	paddr += fb->pitches[plane] * (state->src_y >> 16);
> > > 
> > >  	return paddr;
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 0646b108030b..ab369f250af4 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > >  	struct drm_fb_helper *fb_helper = info->par;
> > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > >  	int depth;
> > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
> > > 
> > >  	if (var->pixclock != 0 || in_dbg_master())
> > >  		return -EINVAL;
> > > @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > >  	 * Changes struct fb_var_screeninfo are currently not pushed back
> > >  	 * to KMS, hence fail if different settings are requested.
> > >  	 */
> > > -	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
> > > +	if (var->bits_per_pixel != bpp ||
> > >  	    var->xres > fb->width || var->yres > fb->height ||
> > >  	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> > >  		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
> > >  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
> > >  			  var->xres, var->yres, var->bits_per_pixel,
> > >  			  var->xres_virtual, var->yres_virtual,
> > > -			  fb->width, fb->height, fb->format->cpp[0] * 8);
> > > +			  fb->width, fb->height, bpp);
> > >  		return -EINVAL;
> > >  	}
> > > 
> > > @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> > >  			    uint32_t fb_width, uint32_t fb_height)
> > >  {
> > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
> > > 
> > >  	info->pseudo_palette = fb_helper->pseudo_palette;
> > >  	info->var.xres_virtual = fb->width;
> > >  	info->var.yres_virtual = fb->height;
> > > -	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
> > > +	info->var.bits_per_pixel = bpp;
> > >  	info->var.accel_flags = FB_ACCELF_TEXT;
> > >  	info->var.xoffset = 0;
> > >  	info->var.yoffset = 0;
> > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > index 3b42c25bd58d..bb28919c32f3 100644
> > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
> > >  	if (!info || plane >= info->num_planes)
> > >  		return 0;
> > > 
> > > +	/*
> > > +	 * Not valid for formats with non-integer cpp,
> > > +	 * use drm_format{_info}_plane_bpp instead
> > > +	 */
> > > +	WARN_ON(!info->cpp[0]);
> > > +
> > >  	return info->cpp[plane];
> > >  }
> > >  EXPORT_SYMBOL(drm_format_plane_cpp);
> > > 
> > > +/**
> > > + * drm_format_plane_bpp - determine the bits per pixel value
> > > + * @format: pixel format (DRM_FORMAT_*)
> > > + * @plane: plane index
> > > + *
> > > + * Returns:
> > > + * The bits per pixel value for the specified plane.
> > > + */
> > > +int drm_format_plane_bpp(uint32_t format, int plane)
> > > +{
> > > +	const struct drm_format_info *info;
> > > +
> > > +	info = drm_format_info(format);
> > > +	if (!info)
> > > +		return 0;
> > > +
> > > +	return drm_format_info_plane_bpp(info, plane);
> > > +}
> > > +EXPORT_SYMBOL(drm_format_plane_bpp);
> > > +
> > > +/**
> > > + * drm_format_info_plane_bpp - determine the bits per pixel value
> > > + *
> > > + * Convenience function which handles formats with both integer
> > > + * and non-integer bytes-per-pixel.
> > > + *
> > > + * @format: pixel format info structure
> > > + * @plane: plane index
> > > + *
> > > + * Returns:
> > > + * The bits per pixel value for the specified plane.
> > > + */
> > > +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
> > > +{
> > > +	if (plane >= info->num_planes)
> > > +		return 0;
> > > +
> > > +	if (info->cpp[0])
> > > +		return info->cpp[plane] * 8;
> > > +
> > > +	return info->bpp[plane];
> > > +}
> > > +EXPORT_SYMBOL(drm_format_info_plane_bpp);
> > > +
> > >  /**
> > >   * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
> > >   * @format: pixel format (DRM_FORMAT_*)
> > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > > index 8c4d32adcc17..7e00360ff70d 100644
> > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > @@ -185,20 +185,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];
> > > +		unsigned int bpp = drm_format_info_plane_bpp(info, i);
> > > 
> > >  		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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
> > >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> > >  			return -EINVAL;
> > >  		}
> > > @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
> > >  	r->height = fb->height;
> > >  	r->width = fb->width;
> > >  	r->depth = fb->format->depth;
> > > -	r->bpp = fb->format->cpp[0] * 8;
> > > +	r->bpp = drm_format_info_plane_bpp(fb->format, 0);
> > >  	r->pitch = fb->pitches[0];
> > > 
> > >  	/* GET_FB() is an unprivileged ioctl so we must not return a
> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > index acfbc0641a06..dfe224ccaeba 100644
> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > >  		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> > >  		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> > >  		unsigned int min_size;
> > > +		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
> > > 
> > >  		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
> > >  		if (!objs[i]) {
> > > @@ -170,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]
> > > +			 + DIV_ROUND_UP(width * bpp, 8)
> > >  			 + 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 3e86408dac9f..d4af4dab1623 100644
> > > --- a/include/drm/drm_fourcc.h
> > > +++ b/include/drm/drm_fourcc.h
> > > @@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
> > >   *	use in new code and set to 0 for new formats.
> > >   * @num_planes: Number of color planes (1 to 3)
> > >   * @cpp: Number of bytes per pixel (per plane)
> > > + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
> > >   * @hsub: Horizontal chroma subsampling factor
> > >   * @vsub: Vertical chroma subsampling factor
> > >   * @has_alpha: Does the format embeds an alpha component?
> > > @@ -45,6 +46,7 @@ struct drm_format_info {
> > >  	u8 depth;
> > >  	u8 num_planes;
> > >  	u8 cpp[3];
> > > +	u8 bpp[3];
> > >  	u8 hsub;
> > >  	u8 vsub;
> > >  	bool has_alpha;
> > > @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
> > >  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
> > >  int drm_format_num_planes(uint32_t format);
> > >  int drm_format_plane_cpp(uint32_t format, int plane);
> > > +int drm_format_plane_bpp(uint32_t format, int plane);
> > > +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
> > >  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);
> > > --
> > > 2.16.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Brian Starkey]

Hi,

On Fri, Sep 07, 2018 at 09:28:44PM +0200, Daniel Vetter wrote:
>On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote:
>> Hi Daniel,
>>
>> On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
>> > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
>> > > Some formats have a non-integer number of bytes per pixel, which can't
>> > > be handled with the existing 'cpp' field in drm_format_info. To handle
>> > > these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
>> > >
>> > > This updates all the users of format->cpp in the core DRM code,
>> > > converting them to use a new function to get the bits-per-pixel for any
>> > > format.
>> > >
>> > > It's assumed that drivers will use the 'bpp' field when they add support
>> > > for pixel formats with non-integer bytes-per-pixel.
>> > >
>> > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> >
>> > I assume you still require that stuff is eventually aligned to bytes? In
>> > that case, can we subsume this into the tile work Alex is doing? It's
>> > essentially just another special case of having storage-size units
>> > measured in bytes which span more than 1x1 pixel. And I kinda don't want a
>> > metric pile of special cases here in the format code, because that just
>> > means every driver handles a different subset, with different bugs.
>> > -Daniel
>>
>> Sorry for the delay, been struggling to free some cycles to think
>> about this.
>>
>> I'm not sure how to pull this in with the tiling stuff. In the AFBC
>> case then our AFBC superblocks are always nice round numbers (256
>> pixels), and so it does end up being a multiple of bytes.
>>
>> However, AFBC supports different superblock sizes, so picking just one
>> doesn't really work out, and putting AFBC in the core format table
>> which reflects AFBC doesn't seem good.
>>
>> We could make something up (e.g. call these formats "tiled" with 2x4
>> tiles, which guarantees a multiple of 8), but it would be an
>> arbitrarily-selected lie, which often seems to spell trouble. If we
>> did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
>> we still need to add a new field anyway.
>>
>> What's the pile of special cases you're worried about? The helper I've
>> added here means that drivers which need to care can use one API and
>> not implement their own bugs.
>
>I'm confused ... the new bits-per-pixel stuff you're adding here is for
>yuv formats, not afbc. I'm just suggesting we have only 1 way of
>describing such formats that need more descriptive power than cpp, whether
>they have some kind of pixel-groups or small tiles.

Well, not really. The three formats which have non-integer cpp are:
DRM_FORMAT_VUY101010, DRM_FORMAT_YUV420_8BIT and
DRM_FORMAT_YUV420_10BIT. These formats are only valid with non-linear
modifiers (no linear encoding is defined). Mali only supports them
with AFBC.

The formats themselves have no notion of tiling or grouping - the
modifier adds that. I'm not aware of any non-AFBC uses of these
formats, so I don't want to "make up" a small-tile layout restriction
for them.

>
>For very special stuff like afbc you need to validate in the driver
>anyway, too complicated. So I have no idea why you bring this up here?

Sure, we can just let drivers provide their own format_info's for
these, if that's what you prefer. The core format checking code can
error out if it ever encounters them.

Cheers,
-Brian

>-Daniel
>
>>
>> Cheers,
>> -Brian
>>
>> >
>> > > ---
>> > >  drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
>> > >  drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
>> > >  drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
>> > >  drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
>> > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
>> > >  include/drm/drm_fourcc.h                     |  4 +++
>> > >  6 files changed, 70 insertions(+), 9 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
>> > > index 186d00adfb5f..e279d70d3e60 100644
>> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> > > @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
>> > >  {
>> > >  	struct drm_gem_cma_object *obj;
>> > >  	dma_addr_t paddr;
>> > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
>> > > +
>> > > +	/* This can't work for non-integer bytes-per-pixel */
>> > > +	WARN_ON(bpp % 8);
>> > >
>> > >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
>> > >  	if (!obj)
>> > >  		return 0;
>> > >
>> > >  	paddr = obj->paddr + fb->offsets[plane];
>> > > -	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
>> > > +	paddr += (bpp / 8) * (state->src_x >> 16);
>> > >  	paddr += fb->pitches[plane] * (state->src_y >> 16);
>> > >
>> > >  	return paddr;
>> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> > > index 0646b108030b..ab369f250af4 100644
>> > > --- a/drivers/gpu/drm/drm_fb_helper.c
>> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
>> > > @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>> > >  	struct drm_fb_helper *fb_helper = info->par;
>> > >  	struct drm_framebuffer *fb = fb_helper->fb;
>> > >  	int depth;
>> > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>> > >
>> > >  	if (var->pixclock != 0 || in_dbg_master())
>> > >  		return -EINVAL;
>> > > @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>> > >  	 * Changes struct fb_var_screeninfo are currently not pushed back
>> > >  	 * to KMS, hence fail if different settings are requested.
>> > >  	 */
>> > > -	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
>> > > +	if (var->bits_per_pixel != bpp ||
>> > >  	    var->xres > fb->width || var->yres > fb->height ||
>> > >  	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
>> > >  		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
>> > >  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
>> > >  			  var->xres, var->yres, var->bits_per_pixel,
>> > >  			  var->xres_virtual, var->yres_virtual,
>> > > -			  fb->width, fb->height, fb->format->cpp[0] * 8);
>> > > +			  fb->width, fb->height, bpp);
>> > >  		return -EINVAL;
>> > >  	}
>> > >
>> > > @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
>> > >  			    uint32_t fb_width, uint32_t fb_height)
>> > >  {
>> > >  	struct drm_framebuffer *fb = fb_helper->fb;
>> > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>> > >
>> > >  	info->pseudo_palette = fb_helper->pseudo_palette;
>> > >  	info->var.xres_virtual = fb->width;
>> > >  	info->var.yres_virtual = fb->height;
>> > > -	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
>> > > +	info->var.bits_per_pixel = bpp;
>> > >  	info->var.accel_flags = FB_ACCELF_TEXT;
>> > >  	info->var.xoffset = 0;
>> > >  	info->var.yoffset = 0;
>> > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>> > > index 3b42c25bd58d..bb28919c32f3 100644
>> > > --- a/drivers/gpu/drm/drm_fourcc.c
>> > > +++ b/drivers/gpu/drm/drm_fourcc.c
>> > > @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
>> > >  	if (!info || plane >= info->num_planes)
>> > >  		return 0;
>> > >
>> > > +	/*
>> > > +	 * Not valid for formats with non-integer cpp,
>> > > +	 * use drm_format{_info}_plane_bpp instead
>> > > +	 */
>> > > +	WARN_ON(!info->cpp[0]);
>> > > +
>> > >  	return info->cpp[plane];
>> > >  }
>> > >  EXPORT_SYMBOL(drm_format_plane_cpp);
>> > >
>> > > +/**
>> > > + * drm_format_plane_bpp - determine the bits per pixel value
>> > > + * @format: pixel format (DRM_FORMAT_*)
>> > > + * @plane: plane index
>> > > + *
>> > > + * Returns:
>> > > + * The bits per pixel value for the specified plane.
>> > > + */
>> > > +int drm_format_plane_bpp(uint32_t format, int plane)
>> > > +{
>> > > +	const struct drm_format_info *info;
>> > > +
>> > > +	info = drm_format_info(format);
>> > > +	if (!info)
>> > > +		return 0;
>> > > +
>> > > +	return drm_format_info_plane_bpp(info, plane);
>> > > +}
>> > > +EXPORT_SYMBOL(drm_format_plane_bpp);
>> > > +
>> > > +/**
>> > > + * drm_format_info_plane_bpp - determine the bits per pixel value
>> > > + *
>> > > + * Convenience function which handles formats with both integer
>> > > + * and non-integer bytes-per-pixel.
>> > > + *
>> > > + * @format: pixel format info structure
>> > > + * @plane: plane index
>> > > + *
>> > > + * Returns:
>> > > + * The bits per pixel value for the specified plane.
>> > > + */
>> > > +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
>> > > +{
>> > > +	if (plane >= info->num_planes)
>> > > +		return 0;
>> > > +
>> > > +	if (info->cpp[0])
>> > > +		return info->cpp[plane] * 8;
>> > > +
>> > > +	return info->bpp[plane];
>> > > +}
>> > > +EXPORT_SYMBOL(drm_format_info_plane_bpp);
>> > > +
>> > >  /**
>> > >   * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
>> > >   * @format: pixel format (DRM_FORMAT_*)
>> > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> > > index 8c4d32adcc17..7e00360ff70d 100644
>> > > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > > @@ -185,20 +185,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];
>> > > +		unsigned int bpp = drm_format_info_plane_bpp(info, i);
>> > >
>> > >  		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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
>> > >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
>> > >  			return -EINVAL;
>> > >  		}
>> > > @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
>> > >  	r->height = fb->height;
>> > >  	r->width = fb->width;
>> > >  	r->depth = fb->format->depth;
>> > > -	r->bpp = fb->format->cpp[0] * 8;
>> > > +	r->bpp = drm_format_info_plane_bpp(fb->format, 0);
>> > >  	r->pitch = fb->pitches[0];
>> > >
>> > >  	/* GET_FB() is an unprivileged ioctl so we must not return a
>> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > index acfbc0641a06..dfe224ccaeba 100644
>> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>> > >  		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
>> > >  		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
>> > >  		unsigned int min_size;
>> > > +		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
>> > >
>> > >  		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
>> > >  		if (!objs[i]) {
>> > > @@ -170,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]
>> > > +			 + DIV_ROUND_UP(width * bpp, 8)
>> > >  			 + 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 3e86408dac9f..d4af4dab1623 100644
>> > > --- a/include/drm/drm_fourcc.h
>> > > +++ b/include/drm/drm_fourcc.h
>> > > @@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
>> > >   *	use in new code and set to 0 for new formats.
>> > >   * @num_planes: Number of color planes (1 to 3)
>> > >   * @cpp: Number of bytes per pixel (per plane)
>> > > + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
>> > >   * @hsub: Horizontal chroma subsampling factor
>> > >   * @vsub: Vertical chroma subsampling factor
>> > >   * @has_alpha: Does the format embeds an alpha component?
>> > > @@ -45,6 +46,7 @@ struct drm_format_info {
>> > >  	u8 depth;
>> > >  	u8 num_planes;
>> > >  	u8 cpp[3];
>> > > +	u8 bpp[3];
>> > >  	u8 hsub;
>> > >  	u8 vsub;
>> > >  	bool has_alpha;
>> > > @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
>> > >  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
>> > >  int drm_format_num_planes(uint32_t format);
>> > >  int drm_format_plane_cpp(uint32_t format, int plane);
>> > > +int drm_format_plane_bpp(uint32_t format, int plane);
>> > > +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
>> > >  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);
>> > > --
>> > > 2.16.1
>> > >
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > http://blog.ffwll.ch
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Daniel Vetter]

On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote:
> Hi,
> 
> On Fri, Sep 07, 2018 at 09:28:44PM +0200, Daniel Vetter wrote:
> > On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote:
> > > Hi Daniel,
> > > 
> > > On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
> > > > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
> > > > > Some formats have a non-integer number of bytes per pixel, which can't
> > > > > be handled with the existing 'cpp' field in drm_format_info. To handle
> > > > > these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
> > > > >
> > > > > This updates all the users of format->cpp in the core DRM code,
> > > > > converting them to use a new function to get the bits-per-pixel for any
> > > > > format.
> > > > >
> > > > > It's assumed that drivers will use the 'bpp' field when they add support
> > > > > for pixel formats with non-integer bytes-per-pixel.
> > > > >
> > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > > >
> > > > I assume you still require that stuff is eventually aligned to bytes? In
> > > > that case, can we subsume this into the tile work Alex is doing? It's
> > > > essentially just another special case of having storage-size units
> > > > measured in bytes which span more than 1x1 pixel. And I kinda don't want a
> > > > metric pile of special cases here in the format code, because that just
> > > > means every driver handles a different subset, with different bugs.
> > > > -Daniel
> > > 
> > > Sorry for the delay, been struggling to free some cycles to think
> > > about this.
> > > 
> > > I'm not sure how to pull this in with the tiling stuff. In the AFBC
> > > case then our AFBC superblocks are always nice round numbers (256
> > > pixels), and so it does end up being a multiple of bytes.
> > > 
> > > However, AFBC supports different superblock sizes, so picking just one
> > > doesn't really work out, and putting AFBC in the core format table
> > > which reflects AFBC doesn't seem good.
> > > 
> > > We could make something up (e.g. call these formats "tiled" with 2x4
> > > tiles, which guarantees a multiple of 8), but it would be an
> > > arbitrarily-selected lie, which often seems to spell trouble. If we
> > > did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
> > > we still need to add a new field anyway.
> > > 
> > > What's the pile of special cases you're worried about? The helper I've
> > > added here means that drivers which need to care can use one API and
> > > not implement their own bugs.
> > 
> > I'm confused ... the new bits-per-pixel stuff you're adding here is for
> > yuv formats, not afbc. I'm just suggesting we have only 1 way of
> > describing such formats that need more descriptive power than cpp, whether
> > they have some kind of pixel-groups or small tiles.
> 
> Well, not really. The three formats which have non-integer cpp are:
> DRM_FORMAT_VUY101010, DRM_FORMAT_YUV420_8BIT and
> DRM_FORMAT_YUV420_10BIT. These formats are only valid with non-linear
> modifiers (no linear encoding is defined). Mali only supports them
> with AFBC.
> 
> The formats themselves have no notion of tiling or grouping - the
> modifier adds that. I'm not aware of any non-AFBC uses of these
> formats, so I don't want to "make up" a small-tile layout restriction
> for them.

Ah, I missed that.

> > For very special stuff like afbc you need to validate in the driver
> > anyway, too complicated. So I have no idea why you bring this up here?
> 
> Sure, we can just let drivers provide their own format_info's for
> these, if that's what you prefer. The core format checking code can
> error out if it ever encounters them.

It's format_info we're talking about. What I mean is that you just set all
these to 0 and let the format_info code ignore it. And then having a
bespoke drm_format_check_afbc helper function or similar, which checks all
the layout restrictions of afbc.

I still maintain that bpp and tile_size are equavalent, and we really
don't need both. Both are defacto a form of numerator/denumerator. If you
don't like that you have to introduce "fake" tiles for afbc, then we can
rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't
change one bit of the math. bpp simply hardcodes a denumerator of 8, and I
don't see why we need that special case. Except if you love to write
redundant self tests for all the math :-)

So two options that I think are reasonable:
- one common numerator/denumerator. I don't care how you call that
  bikeshed.
- don't check afbc using format_info, have your own helper that does that
  using custom code.

Cheers, Daniel

> Cheers,
> -Brian
> 
> > -Daniel
> > 
> > > 
> > > Cheers,
> > > -Brian
> > > 
> > > >
> > > > > ---
> > > > >  drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
> > > > >  drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
> > > > >  drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
> > > > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
> > > > >  include/drm/drm_fourcc.h                     |  4 +++
> > > > >  6 files changed, 70 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > > index 186d00adfb5f..e279d70d3e60 100644
> > > > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > > @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > > > >  {
> > > > >  	struct drm_gem_cma_object *obj;
> > > > >  	dma_addr_t paddr;
> > > > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
> > > > > +
> > > > > +	/* This can't work for non-integer bytes-per-pixel */
> > > > > +	WARN_ON(bpp % 8);
> > > > >
> > > > >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
> > > > >  	if (!obj)
> > > > >  		return 0;
> > > > >
> > > > >  	paddr = obj->paddr + fb->offsets[plane];
> > > > > -	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
> > > > > +	paddr += (bpp / 8) * (state->src_x >> 16);
> > > > >  	paddr += fb->pitches[plane] * (state->src_y >> 16);
> > > > >
> > > > >  	return paddr;
> > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > > index 0646b108030b..ab369f250af4 100644
> > > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > > @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > > > >  	struct drm_fb_helper *fb_helper = info->par;
> > > > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > > > >  	int depth;
> > > > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
> > > > >
> > > > >  	if (var->pixclock != 0 || in_dbg_master())
> > > > >  		return -EINVAL;
> > > > > @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > > > >  	 * Changes struct fb_var_screeninfo are currently not pushed back
> > > > >  	 * to KMS, hence fail if different settings are requested.
> > > > >  	 */
> > > > > -	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
> > > > > +	if (var->bits_per_pixel != bpp ||
> > > > >  	    var->xres > fb->width || var->yres > fb->height ||
> > > > >  	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> > > > >  		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
> > > > >  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
> > > > >  			  var->xres, var->yres, var->bits_per_pixel,
> > > > >  			  var->xres_virtual, var->yres_virtual,
> > > > > -			  fb->width, fb->height, fb->format->cpp[0] * 8);
> > > > > +			  fb->width, fb->height, bpp);
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >
> > > > > @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> > > > >  			    uint32_t fb_width, uint32_t fb_height)
> > > > >  {
> > > > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > > > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
> > > > >
> > > > >  	info->pseudo_palette = fb_helper->pseudo_palette;
> > > > >  	info->var.xres_virtual = fb->width;
> > > > >  	info->var.yres_virtual = fb->height;
> > > > > -	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
> > > > > +	info->var.bits_per_pixel = bpp;
> > > > >  	info->var.accel_flags = FB_ACCELF_TEXT;
> > > > >  	info->var.xoffset = 0;
> > > > >  	info->var.yoffset = 0;
> > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > > > index 3b42c25bd58d..bb28919c32f3 100644
> > > > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > > > @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
> > > > >  	if (!info || plane >= info->num_planes)
> > > > >  		return 0;
> > > > >
> > > > > +	/*
> > > > > +	 * Not valid for formats with non-integer cpp,
> > > > > +	 * use drm_format{_info}_plane_bpp instead
> > > > > +	 */
> > > > > +	WARN_ON(!info->cpp[0]);
> > > > > +
> > > > >  	return info->cpp[plane];
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_format_plane_cpp);
> > > > >
> > > > > +/**
> > > > > + * drm_format_plane_bpp - determine the bits per pixel value
> > > > > + * @format: pixel format (DRM_FORMAT_*)
> > > > > + * @plane: plane index
> > > > > + *
> > > > > + * Returns:
> > > > > + * The bits per pixel value for the specified plane.
> > > > > + */
> > > > > +int drm_format_plane_bpp(uint32_t format, int plane)
> > > > > +{
> > > > > +	const struct drm_format_info *info;
> > > > > +
> > > > > +	info = drm_format_info(format);
> > > > > +	if (!info)
> > > > > +		return 0;
> > > > > +
> > > > > +	return drm_format_info_plane_bpp(info, plane);
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_format_plane_bpp);
> > > > > +
> > > > > +/**
> > > > > + * drm_format_info_plane_bpp - determine the bits per pixel value
> > > > > + *
> > > > > + * Convenience function which handles formats with both integer
> > > > > + * and non-integer bytes-per-pixel.
> > > > > + *
> > > > > + * @format: pixel format info structure
> > > > > + * @plane: plane index
> > > > > + *
> > > > > + * Returns:
> > > > > + * The bits per pixel value for the specified plane.
> > > > > + */
> > > > > +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
> > > > > +{
> > > > > +	if (plane >= info->num_planes)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (info->cpp[0])
> > > > > +		return info->cpp[plane] * 8;
> > > > > +
> > > > > +	return info->bpp[plane];
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_format_info_plane_bpp);
> > > > > +
> > > > >  /**
> > > > >   * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
> > > > >   * @format: pixel format (DRM_FORMAT_*)
> > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > > > > index 8c4d32adcc17..7e00360ff70d 100644
> > > > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > > > @@ -185,20 +185,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];
> > > > > +		unsigned int bpp = drm_format_info_plane_bpp(info, i);
> > > > >
> > > > >  		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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
> > > > >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> > > > >  			return -EINVAL;
> > > > >  		}
> > > > > @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
> > > > >  	r->height = fb->height;
> > > > >  	r->width = fb->width;
> > > > >  	r->depth = fb->format->depth;
> > > > > -	r->bpp = fb->format->cpp[0] * 8;
> > > > > +	r->bpp = drm_format_info_plane_bpp(fb->format, 0);
> > > > >  	r->pitch = fb->pitches[0];
> > > > >
> > > > >  	/* GET_FB() is an unprivileged ioctl so we must not return a
> > > > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > > index acfbc0641a06..dfe224ccaeba 100644
> > > > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > > @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > > > >  		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> > > > >  		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> > > > >  		unsigned int min_size;
> > > > > +		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
> > > > >
> > > > >  		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
> > > > >  		if (!objs[i]) {
> > > > > @@ -170,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]
> > > > > +			 + DIV_ROUND_UP(width * bpp, 8)
> > > > >  			 + 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 3e86408dac9f..d4af4dab1623 100644
> > > > > --- a/include/drm/drm_fourcc.h
> > > > > +++ b/include/drm/drm_fourcc.h
> > > > > @@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
> > > > >   *	use in new code and set to 0 for new formats.
> > > > >   * @num_planes: Number of color planes (1 to 3)
> > > > >   * @cpp: Number of bytes per pixel (per plane)
> > > > > + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
> > > > >   * @hsub: Horizontal chroma subsampling factor
> > > > >   * @vsub: Vertical chroma subsampling factor
> > > > >   * @has_alpha: Does the format embeds an alpha component?
> > > > > @@ -45,6 +46,7 @@ struct drm_format_info {
> > > > >  	u8 depth;
> > > > >  	u8 num_planes;
> > > > >  	u8 cpp[3];
> > > > > +	u8 bpp[3];
> > > > >  	u8 hsub;
> > > > >  	u8 vsub;
> > > > >  	bool has_alpha;
> > > > > @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
> > > > >  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
> > > > >  int drm_format_num_planes(uint32_t format);
> > > > >  int drm_format_plane_cpp(uint32_t format, int plane);
> > > > > +int drm_format_plane_bpp(uint32_t format, int plane);
> > > > > +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
> > > > >  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);
> > > > > --
> > > > > 2.16.1
> > > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > 
> > -- 
> > 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Brian Starkey]

On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote:
>On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote:
>> Hi,
>>
>> On Fri, Sep 07, 2018 at 09:28:44PM +0200, Daniel Vetter wrote:
>> > On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote:
>> > > Hi Daniel,
>> > >
>> > > On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
>> > > > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
>> > > > > Some formats have a non-integer number of bytes per pixel, which can't
>> > > > > be handled with the existing 'cpp' field in drm_format_info. To handle
>> > > > > these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
>> > > > >
>> > > > > This updates all the users of format->cpp in the core DRM code,
>> > > > > converting them to use a new function to get the bits-per-pixel for any
>> > > > > format.
>> > > > >
>> > > > > It's assumed that drivers will use the 'bpp' field when they add support
>> > > > > for pixel formats with non-integer bytes-per-pixel.
>> > > > >
>> > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> > > >
>> > > > I assume you still require that stuff is eventually aligned to bytes? In
>> > > > that case, can we subsume this into the tile work Alex is doing? It's
>> > > > essentially just another special case of having storage-size units
>> > > > measured in bytes which span more than 1x1 pixel. And I kinda don't want a
>> > > > metric pile of special cases here in the format code, because that just
>> > > > means every driver handles a different subset, with different bugs.
>> > > > -Daniel
>> > >
>> > > Sorry for the delay, been struggling to free some cycles to think
>> > > about this.
>> > >
>> > > I'm not sure how to pull this in with the tiling stuff. In the AFBC
>> > > case then our AFBC superblocks are always nice round numbers (256
>> > > pixels), and so it does end up being a multiple of bytes.
>> > >
>> > > However, AFBC supports different superblock sizes, so picking just one
>> > > doesn't really work out, and putting AFBC in the core format table
>> > > which reflects AFBC doesn't seem good.
>> > >
>> > > We could make something up (e.g. call these formats "tiled" with 2x4
>> > > tiles, which guarantees a multiple of 8), but it would be an
>> > > arbitrarily-selected lie, which often seems to spell trouble. If we
>> > > did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
>> > > we still need to add a new field anyway.
>> > >
>> > > What's the pile of special cases you're worried about? The helper I've
>> > > added here means that drivers which need to care can use one API and
>> > > not implement their own bugs.
>> >
>> > I'm confused ... the new bits-per-pixel stuff you're adding here is for
>> > yuv formats, not afbc. I'm just suggesting we have only 1 way of
>> > describing such formats that need more descriptive power than cpp, whether
>> > they have some kind of pixel-groups or small tiles.
>>
>> Well, not really. The three formats which have non-integer cpp are:
>> DRM_FORMAT_VUY101010, DRM_FORMAT_YUV420_8BIT and
>> DRM_FORMAT_YUV420_10BIT. These formats are only valid with non-linear
>> modifiers (no linear encoding is defined). Mali only supports them
>> with AFBC.
>>
>> The formats themselves have no notion of tiling or grouping - the
>> modifier adds that. I'm not aware of any non-AFBC uses of these
>> formats, so I don't want to "make up" a small-tile layout restriction
>> for them.
>
>Ah, I missed that.
>
>> > For very special stuff like afbc you need to validate in the driver
>> > anyway, too complicated. So I have no idea why you bring this up here?
>>
>> Sure, we can just let drivers provide their own format_info's for
>> these, if that's what you prefer. The core format checking code can
>> error out if it ever encounters them.
>
>It's format_info we're talking about. What I mean is that you just set all
>these to 0 and let the format_info code ignore it. And then having a
>bespoke drm_format_check_afbc helper function or similar, which checks all
>the layout restrictions of afbc.
>
>I still maintain that bpp and tile_size are equavalent, and we really
>don't need both. Both are defacto a form of numerator/denumerator. If you
>don't like that you have to introduce "fake" tiles for afbc, then we can
>rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't
>change one bit of the math. bpp simply hardcodes a denumerator of 8, and I
>don't see why we need that special case. Except if you love to write
>redundant self tests for all the math :-)
>
>So two options that I think are reasonable:
>- one common numerator/denumerator. I don't care how you call that
>  bikeshed.

Sorry for being dense, but I'm still struggling to get my head around
what you're suggesting. In particular "bpp simply hardcodes a
denumerator of 8" didn't make any sense to me. Could you give concrete
examples for how you think this would look for e.g.

 - DRM_FORMAT_VUY101010. 30-bits per pixel, no tiling.
 - DRM_FORMAT_Y0L2. 16-bits per pixel, 2x2 pixel tiles

I think we need two things:
 - The size, in bits, of a tile
 - The width and height, in pixels, of a tile (currently implicitly
   1x1)

Do you disagree? Are you just saying that instead of adding .bpp I
should be replacing .cpp with .bpp wholesale?

>- don't check afbc using format_info, have your own helper that does that
>  using custom code.

We can do this, no problem.

Thanks,
-Brian

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Daniel Vetter]

On Wed, Sep 12, 2018 at 3:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote:
>>
>> On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote:
>>>
>>> Hi,
>>>
>>> On Fri, Sep 07, 2018 at 09:28:44PM +0200, Daniel Vetter wrote:
>>> > On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote:
>>> > > Hi Daniel,
>>> > >
>>> > > On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
>>> > > > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
>>> > > > > Some formats have a non-integer number of bytes per pixel, which
>>> > > > > can't
>>> > > > > be handled with the existing 'cpp' field in drm_format_info. To
>>> > > > > handle
>>> > > > > these formats, add a 'bpp' field, which is only used if cpp[0] ==
>>> > > > > 0.
>>> > > > >
>>> > > > > This updates all the users of format->cpp in the core DRM code,
>>> > > > > converting them to use a new function to get the bits-per-pixel
>>> > > > > for any
>>> > > > > format.
>>> > > > >
>>> > > > > It's assumed that drivers will use the 'bpp' field when they add
>>> > > > > support
>>> > > > > for pixel formats with non-integer bytes-per-pixel.
>>> > > > >
>>> > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>>> > > >
>>> > > > I assume you still require that stuff is eventually aligned to
>>> > > > bytes? In
>>> > > > that case, can we subsume this into the tile work Alex is doing?
>>> > > > It's
>>> > > > essentially just another special case of having storage-size units
>>> > > > measured in bytes which span more than 1x1 pixel. And I kinda don't
>>> > > > want a
>>> > > > metric pile of special cases here in the format code, because that
>>> > > > just
>>> > > > means every driver handles a different subset, with different bugs.
>>> > > > -Daniel
>>> > >
>>> > > Sorry for the delay, been struggling to free some cycles to think
>>> > > about this.
>>> > >
>>> > > I'm not sure how to pull this in with the tiling stuff. In the AFBC
>>> > > case then our AFBC superblocks are always nice round numbers (256
>>> > > pixels), and so it does end up being a multiple of bytes.
>>> > >
>>> > > However, AFBC supports different superblock sizes, so picking just
>>> > > one
>>> > > doesn't really work out, and putting AFBC in the core format table
>>> > > which reflects AFBC doesn't seem good.
>>> > >
>>> > > We could make something up (e.g. call these formats "tiled" with 2x4
>>> > > tiles, which guarantees a multiple of 8), but it would be an
>>> > > arbitrarily-selected lie, which often seems to spell trouble. If we
>>> > > did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
>>> > > we still need to add a new field anyway.
>>> > >
>>> > > What's the pile of special cases you're worried about? The helper
>>> > > I've
>>> > > added here means that drivers which need to care can use one API and
>>> > > not implement their own bugs.
>>> >
>>> > I'm confused ... the new bits-per-pixel stuff you're adding here is for
>>> > yuv formats, not afbc. I'm just suggesting we have only 1 way of
>>> > describing such formats that need more descriptive power than cpp,
>>> > whether
>>> > they have some kind of pixel-groups or small tiles.
>>>
>>> Well, not really. The three formats which have non-integer cpp are:
>>> DRM_FORMAT_VUY101010, DRM_FORMAT_YUV420_8BIT and
>>> DRM_FORMAT_YUV420_10BIT. These formats are only valid with non-linear
>>> modifiers (no linear encoding is defined). Mali only supports them
>>> with AFBC.
>>>
>>> The formats themselves have no notion of tiling or grouping - the
>>> modifier adds that. I'm not aware of any non-AFBC uses of these
>>> formats, so I don't want to "make up" a small-tile layout restriction
>>> for them.
>>
>>
>> Ah, I missed that.
>>
>>> > For very special stuff like afbc you need to validate in the driver
>>> > anyway, too complicated. So I have no idea why you bring this up here?
>>>
>>> Sure, we can just let drivers provide their own format_info's for
>>> these, if that's what you prefer. The core format checking code can
>>> error out if it ever encounters them.
>>
>>
>> It's format_info we're talking about. What I mean is that you just set all
>> these to 0 and let the format_info code ignore it. And then having a
>> bespoke drm_format_check_afbc helper function or similar, which checks all
>> the layout restrictions of afbc.
>>
>> I still maintain that bpp and tile_size are equavalent, and we really
>> don't need both. Both are defacto a form of numerator/denumerator. If you
>> don't like that you have to introduce "fake" tiles for afbc, then we can
>> rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't
>> change one bit of the math. bpp simply hardcodes a denumerator of 8, and I
>> don't see why we need that special case. Except if you love to write
>> redundant self tests for all the math :-)
>>
>> So two options that I think are reasonable:
>> - one common numerator/denumerator. I don't care how you call that
>>  bikeshed.
>
>
> Sorry for being dense, but I'm still struggling to get my head around
> what you're suggesting. In particular "bpp simply hardcodes a
> denumerator of 8" didn't make any sense to me. Could you give concrete
> examples for how you think this would look for e.g.
>
> - DRM_FORMAT_VUY101010. 30-bits per pixel, no tiling.
> - DRM_FORMAT_Y0L2. 16-bits per pixel, 2x2 pixel tiles

Ok, a few examples:

4 cpp: tile_size = 4, tile_h/w = 1

30 bpp: In cpp that's 30 / 8 = 15 / 4. So would be a tile_size = 15,
tile_w = 4, tile_h = 1. If you check the math, this matches exactly
all the same addfb values as what you have in your bpp computation
(but only if you simplify the quotient).

16 bpp, 2x2 tiles: No real math needed here, tile_size = 2 * 2 * 2 =
8, tile_h/w = 2.


> I think we need two things:
> - The size, in bits, of a tile

Nope, you only need it in bytes, because in the end all buffers must
align to bytes. The generic formula is:

X bpp: tile_size = X / gcd(X, 8), tile_w = 8 / gcd(X, 8), tile_h = 1.

No bits needed anywhere.

> - The width and height, in pixels, of a tile (currently implicitly
>   1x1)

Yeah, but only if you use the cpp thing. We could even throw that out,
and entirely replace it with tile_size, with the rule that if tile_h/w
= 0, then you assume they're both 1.

> Do you disagree? Are you just saying that instead of adding .bpp I
> should be replacing .cpp with .bpp wholesale?

Hopefully the above explains what I mean, and demonstrates that you
can express any bpp in terms of tile_size/tile_w. Because bpp is just
a special quotient, with a fixed 8 divisor.
-Daniel

>> - don't check afbc using format_info, have your own helper that does that
>>  using custom code.
>
>
> We can do this, no problem.
>
> Thanks,
> -Brian
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Liviu Dudau]

On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote:
> On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote:
> > Hi,
> > 
> > On Fri, Sep 07, 2018 at 09:28:44PM +0200, Daniel Vetter wrote:
> > > On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote:
> > > > Hi Daniel,
> > > > 
> > > > On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
> > > > > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
> > > > > > Some formats have a non-integer number of bytes per pixel, which can't
> > > > > > be handled with the existing 'cpp' field in drm_format_info. To handle
> > > > > > these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
> > > > > >
> > > > > > This updates all the users of format->cpp in the core DRM code,
> > > > > > converting them to use a new function to get the bits-per-pixel for any
> > > > > > format.
> > > > > >
> > > > > > It's assumed that drivers will use the 'bpp' field when they add support
> > > > > > for pixel formats with non-integer bytes-per-pixel.
> > > > > >
> > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > > > >
> > > > > I assume you still require that stuff is eventually aligned to bytes? In
> > > > > that case, can we subsume this into the tile work Alex is doing? It's
> > > > > essentially just another special case of having storage-size units
> > > > > measured in bytes which span more than 1x1 pixel. And I kinda don't want a
> > > > > metric pile of special cases here in the format code, because that just
> > > > > means every driver handles a different subset, with different bugs.
> > > > > -Daniel
> > > > 
> > > > Sorry for the delay, been struggling to free some cycles to think
> > > > about this.
> > > > 
> > > > I'm not sure how to pull this in with the tiling stuff. In the AFBC
> > > > case then our AFBC superblocks are always nice round numbers (256
> > > > pixels), and so it does end up being a multiple of bytes.
> > > > 
> > > > However, AFBC supports different superblock sizes, so picking just one
> > > > doesn't really work out, and putting AFBC in the core format table
> > > > which reflects AFBC doesn't seem good.
> > > > 
> > > > We could make something up (e.g. call these formats "tiled" with 2x4
> > > > tiles, which guarantees a multiple of 8), but it would be an
> > > > arbitrarily-selected lie, which often seems to spell trouble. If we
> > > > did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
> > > > we still need to add a new field anyway.
> > > > 
> > > > What's the pile of special cases you're worried about? The helper I've
> > > > added here means that drivers which need to care can use one API and
> > > > not implement their own bugs.
> > > 
> > > I'm confused ... the new bits-per-pixel stuff you're adding here is for
> > > yuv formats, not afbc. I'm just suggesting we have only 1 way of
> > > describing such formats that need more descriptive power than cpp, whether
> > > they have some kind of pixel-groups or small tiles.
> > 
> > Well, not really. The three formats which have non-integer cpp are:
> > DRM_FORMAT_VUY101010, DRM_FORMAT_YUV420_8BIT and
> > DRM_FORMAT_YUV420_10BIT. These formats are only valid with non-linear
> > modifiers (no linear encoding is defined). Mali only supports them
> > with AFBC.
> > 
> > The formats themselves have no notion of tiling or grouping - the
> > modifier adds that. I'm not aware of any non-AFBC uses of these
> > formats, so I don't want to "make up" a small-tile layout restriction
> > for them.
> 
> Ah, I missed that.
> 
> > > For very special stuff like afbc you need to validate in the driver
> > > anyway, too complicated. So I have no idea why you bring this up here?
> > 
> > Sure, we can just let drivers provide their own format_info's for
> > these, if that's what you prefer. The core format checking code can
> > error out if it ever encounters them.
> 
> It's format_info we're talking about. What I mean is that you just set all
> these to 0 and let the format_info code ignore it. And then having a
> bespoke drm_format_check_afbc helper function or similar, which checks all
> the layout restrictions of afbc.
> 
> I still maintain that bpp and tile_size are equavalent, and we really
> don't need both. Both are defacto a form of numerator/denumerator. If you
> don't like that you have to introduce "fake" tiles for afbc, then we can
> rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't
> change one bit of the math. bpp simply hardcodes a denumerator of 8, and I
> don't see why we need that special case. Except if you love to write
> redundant self tests for all the math :-)

My $.02 worth of thoughts:

I get the fact that Daniel doesn't like us to add 3 new variables into
format_info (bpp, tile_w, tile_h) and that adding a "bits_per_unit"
variable should be able to take care of linear (where unit = 1 pixel)
and tiled (where unit = tile_w * tile_h pixels) formats. And I also see
Daniel's option 2 below, where he says it is reasonable to check AFBC
without using format_info. 

However, the problem we are trying to solve is 2 fold: we are trying to
calculate the size of the framebuffer (and the "bits_per_unit" or
Brian's bpp is useful for that), but we also try to validate the sizes
passed by userspace based on the drm_fourcc.h+modifier info. In that
case, the driver still needs to store somewhere the tile_w/tile_h for
that given format in order to check that the stride is a whole multiple
of tile sizes, so we thought that putting it in format_info is not
entirely pointless, because others might use those variables in order to
do their driver specific validation, without creating new structures.

Did I capture the discussion correctly? If so, can we agree that it is
not just the framebuffer size calculation that matters and that tiled
formats validation requires a tile_w/tile_h info, therefore Alex's
patches and Brian's need to be discussed separately (so that we can
bikeshed on whether format_info is the right place or not)?

Best regards,
Liviu

> 
> So two options that I think are reasonable:
> - one common numerator/denumerator. I don't care how you call that
>   bikeshed.
> - don't check afbc using format_info, have your own helper that does that
>   using custom code.
> 
> Cheers, Daniel
> 
> > Cheers,
> > -Brian
> > 
> > > -Daniel
> > > 
> > > > 
> > > > Cheers,
> > > > -Brian
> > > > 
> > > > >
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
> > > > > >  drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
> > > > > >  drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
> > > > > >  drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
> > > > > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
> > > > > >  include/drm/drm_fourcc.h                     |  4 +++
> > > > > >  6 files changed, 70 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > > > index 186d00adfb5f..e279d70d3e60 100644
> > > > > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > > > @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > > > > >  {
> > > > > >  	struct drm_gem_cma_object *obj;
> > > > > >  	dma_addr_t paddr;
> > > > > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
> > > > > > +
> > > > > > +	/* This can't work for non-integer bytes-per-pixel */
> > > > > > +	WARN_ON(bpp % 8);
> > > > > >
> > > > > >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
> > > > > >  	if (!obj)
> > > > > >  		return 0;
> > > > > >
> > > > > >  	paddr = obj->paddr + fb->offsets[plane];
> > > > > > -	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
> > > > > > +	paddr += (bpp / 8) * (state->src_x >> 16);
> > > > > >  	paddr += fb->pitches[plane] * (state->src_y >> 16);
> > > > > >
> > > > > >  	return paddr;
> > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > > > index 0646b108030b..ab369f250af4 100644
> > > > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > > > @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > > > > >  	struct drm_fb_helper *fb_helper = info->par;
> > > > > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > > > > >  	int depth;
> > > > > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
> > > > > >
> > > > > >  	if (var->pixclock != 0 || in_dbg_master())
> > > > > >  		return -EINVAL;
> > > > > > @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > > > > >  	 * Changes struct fb_var_screeninfo are currently not pushed back
> > > > > >  	 * to KMS, hence fail if different settings are requested.
> > > > > >  	 */
> > > > > > -	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
> > > > > > +	if (var->bits_per_pixel != bpp ||
> > > > > >  	    var->xres > fb->width || var->yres > fb->height ||
> > > > > >  	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> > > > > >  		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
> > > > > >  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
> > > > > >  			  var->xres, var->yres, var->bits_per_pixel,
> > > > > >  			  var->xres_virtual, var->yres_virtual,
> > > > > > -			  fb->width, fb->height, fb->format->cpp[0] * 8);
> > > > > > +			  fb->width, fb->height, bpp);
> > > > > >  		return -EINVAL;
> > > > > >  	}
> > > > > >
> > > > > > @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> > > > > >  			    uint32_t fb_width, uint32_t fb_height)
> > > > > >  {
> > > > > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > > > > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
> > > > > >
> > > > > >  	info->pseudo_palette = fb_helper->pseudo_palette;
> > > > > >  	info->var.xres_virtual = fb->width;
> > > > > >  	info->var.yres_virtual = fb->height;
> > > > > > -	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
> > > > > > +	info->var.bits_per_pixel = bpp;
> > > > > >  	info->var.accel_flags = FB_ACCELF_TEXT;
> > > > > >  	info->var.xoffset = 0;
> > > > > >  	info->var.yoffset = 0;
> > > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > > > > index 3b42c25bd58d..bb28919c32f3 100644
> > > > > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > > > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > > > > @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
> > > > > >  	if (!info || plane >= info->num_planes)
> > > > > >  		return 0;
> > > > > >
> > > > > > +	/*
> > > > > > +	 * Not valid for formats with non-integer cpp,
> > > > > > +	 * use drm_format{_info}_plane_bpp instead
> > > > > > +	 */
> > > > > > +	WARN_ON(!info->cpp[0]);
> > > > > > +
> > > > > >  	return info->cpp[plane];
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(drm_format_plane_cpp);
> > > > > >
> > > > > > +/**
> > > > > > + * drm_format_plane_bpp - determine the bits per pixel value
> > > > > > + * @format: pixel format (DRM_FORMAT_*)
> > > > > > + * @plane: plane index
> > > > > > + *
> > > > > > + * Returns:
> > > > > > + * The bits per pixel value for the specified plane.
> > > > > > + */
> > > > > > +int drm_format_plane_bpp(uint32_t format, int plane)
> > > > > > +{
> > > > > > +	const struct drm_format_info *info;
> > > > > > +
> > > > > > +	info = drm_format_info(format);
> > > > > > +	if (!info)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	return drm_format_info_plane_bpp(info, plane);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(drm_format_plane_bpp);
> > > > > > +
> > > > > > +/**
> > > > > > + * drm_format_info_plane_bpp - determine the bits per pixel value
> > > > > > + *
> > > > > > + * Convenience function which handles formats with both integer
> > > > > > + * and non-integer bytes-per-pixel.
> > > > > > + *
> > > > > > + * @format: pixel format info structure
> > > > > > + * @plane: plane index
> > > > > > + *
> > > > > > + * Returns:
> > > > > > + * The bits per pixel value for the specified plane.
> > > > > > + */
> > > > > > +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
> > > > > > +{
> > > > > > +	if (plane >= info->num_planes)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	if (info->cpp[0])
> > > > > > +		return info->cpp[plane] * 8;
> > > > > > +
> > > > > > +	return info->bpp[plane];
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(drm_format_info_plane_bpp);
> > > > > > +
> > > > > >  /**
> > > > > >   * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
> > > > > >   * @format: pixel format (DRM_FORMAT_*)
> > > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > > > > > index 8c4d32adcc17..7e00360ff70d 100644
> > > > > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > > > > @@ -185,20 +185,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];
> > > > > > +		unsigned int bpp = drm_format_info_plane_bpp(info, i);
> > > > > >
> > > > > >  		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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
> > > > > >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> > > > > >  			return -EINVAL;
> > > > > >  		}
> > > > > > @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
> > > > > >  	r->height = fb->height;
> > > > > >  	r->width = fb->width;
> > > > > >  	r->depth = fb->format->depth;
> > > > > > -	r->bpp = fb->format->cpp[0] * 8;
> > > > > > +	r->bpp = drm_format_info_plane_bpp(fb->format, 0);
> > > > > >  	r->pitch = fb->pitches[0];
> > > > > >
> > > > > >  	/* GET_FB() is an unprivileged ioctl so we must not return a
> > > > > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > > > index acfbc0641a06..dfe224ccaeba 100644
> > > > > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > > > @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > > > > >  		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> > > > > >  		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> > > > > >  		unsigned int min_size;
> > > > > > +		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
> > > > > >
> > > > > >  		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
> > > > > >  		if (!objs[i]) {
> > > > > > @@ -170,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]
> > > > > > +			 + DIV_ROUND_UP(width * bpp, 8)
> > > > > >  			 + 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 3e86408dac9f..d4af4dab1623 100644
> > > > > > --- a/include/drm/drm_fourcc.h
> > > > > > +++ b/include/drm/drm_fourcc.h
> > > > > > @@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
> > > > > >   *	use in new code and set to 0 for new formats.
> > > > > >   * @num_planes: Number of color planes (1 to 3)
> > > > > >   * @cpp: Number of bytes per pixel (per plane)
> > > > > > + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
> > > > > >   * @hsub: Horizontal chroma subsampling factor
> > > > > >   * @vsub: Vertical chroma subsampling factor
> > > > > >   * @has_alpha: Does the format embeds an alpha component?
> > > > > > @@ -45,6 +46,7 @@ struct drm_format_info {
> > > > > >  	u8 depth;
> > > > > >  	u8 num_planes;
> > > > > >  	u8 cpp[3];
> > > > > > +	u8 bpp[3];
> > > > > >  	u8 hsub;
> > > > > >  	u8 vsub;
> > > > > >  	bool has_alpha;
> > > > > > @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
> > > > > >  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
> > > > > >  int drm_format_num_planes(uint32_t format);
> > > > > >  int drm_format_plane_cpp(uint32_t format, int plane);
> > > > > > +int drm_format_plane_bpp(uint32_t format, int plane);
> > > > > > +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
> > > > > >  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);
> > > > > > --
> > > > > > 2.16.1
> > > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > 
> > > -- 
> > > 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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Brian Starkey]

On Wed, Sep 12, 2018 at 04:27:04PM +0100, Liviu Dudau wrote:
>On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote:
>> On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote:
>> > Hi,
>> >
>> > On Fri, Sep 07, 2018 at 09:28:44PM +0200, Daniel Vetter wrote:
>> > > On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote:
>> > > > Hi Daniel,
>> > > >
>> > > > On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
>> > > > > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
>> > > > > > Some formats have a non-integer number of bytes per pixel, which can't
>> > > > > > be handled with the existing 'cpp' field in drm_format_info. To handle
>> > > > > > these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
>> > > > > >
>> > > > > > This updates all the users of format->cpp in the core DRM code,
>> > > > > > converting them to use a new function to get the bits-per-pixel for any
>> > > > > > format.
>> > > > > >
>> > > > > > It's assumed that drivers will use the 'bpp' field when they add support
>> > > > > > for pixel formats with non-integer bytes-per-pixel.
>> > > > > >
>> > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> > > > >
>> > > > > I assume you still require that stuff is eventually aligned to bytes? In
>> > > > > that case, can we subsume this into the tile work Alex is doing? It's
>> > > > > essentially just another special case of having storage-size units
>> > > > > measured in bytes which span more than 1x1 pixel. And I kinda don't want a
>> > > > > metric pile of special cases here in the format code, because that just
>> > > > > means every driver handles a different subset, with different bugs.
>> > > > > -Daniel
>> > > >
>> > > > Sorry for the delay, been struggling to free some cycles to think
>> > > > about this.
>> > > >
>> > > > I'm not sure how to pull this in with the tiling stuff. In the AFBC
>> > > > case then our AFBC superblocks are always nice round numbers (256
>> > > > pixels), and so it does end up being a multiple of bytes.
>> > > >
>> > > > However, AFBC supports different superblock sizes, so picking just one
>> > > > doesn't really work out, and putting AFBC in the core format table
>> > > > which reflects AFBC doesn't seem good.
>> > > >
>> > > > We could make something up (e.g. call these formats "tiled" with 2x4
>> > > > tiles, which guarantees a multiple of 8), but it would be an
>> > > > arbitrarily-selected lie, which often seems to spell trouble. If we
>> > > > did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
>> > > > we still need to add a new field anyway.
>> > > >
>> > > > What's the pile of special cases you're worried about? The helper I've
>> > > > added here means that drivers which need to care can use one API and
>> > > > not implement their own bugs.
>> > >
>> > > I'm confused ... the new bits-per-pixel stuff you're adding here is for
>> > > yuv formats, not afbc. I'm just suggesting we have only 1 way of
>> > > describing such formats that need more descriptive power than cpp, whether
>> > > they have some kind of pixel-groups or small tiles.
>> >
>> > Well, not really. The three formats which have non-integer cpp are:
>> > DRM_FORMAT_VUY101010, DRM_FORMAT_YUV420_8BIT and
>> > DRM_FORMAT_YUV420_10BIT. These formats are only valid with non-linear
>> > modifiers (no linear encoding is defined). Mali only supports them
>> > with AFBC.
>> >
>> > The formats themselves have no notion of tiling or grouping - the
>> > modifier adds that. I'm not aware of any non-AFBC uses of these
>> > formats, so I don't want to "make up" a small-tile layout restriction
>> > for them.
>>
>> Ah, I missed that.
>>
>> > > For very special stuff like afbc you need to validate in the driver
>> > > anyway, too complicated. So I have no idea why you bring this up here?
>> >
>> > Sure, we can just let drivers provide their own format_info's for
>> > these, if that's what you prefer. The core format checking code can
>> > error out if it ever encounters them.
>>
>> It's format_info we're talking about. What I mean is that you just set all
>> these to 0 and let the format_info code ignore it. And then having a
>> bespoke drm_format_check_afbc helper function or similar, which checks all
>> the layout restrictions of afbc.
>>
>> I still maintain that bpp and tile_size are equavalent, and we really
>> don't need both. Both are defacto a form of numerator/denumerator. If you
>> don't like that you have to introduce "fake" tiles for afbc, then we can
>> rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't
>> change one bit of the math. bpp simply hardcodes a denumerator of 8, and I
>> don't see why we need that special case. Except if you love to write
>> redundant self tests for all the math :-)
>
>My $.02 worth of thoughts:
>
>I get the fact that Daniel doesn't like us to add 3 new variables into
>format_info (bpp, tile_w, tile_h) and that adding a "bits_per_unit"
>variable should be able to take care of linear (where unit = 1 pixel)
>and tiled (where unit = tile_w * tile_h pixels) formats. And I also see
>Daniel's option 2 below, where he says it is reasonable to check AFBC
>without using format_info.
>
>However, the problem we are trying to solve is 2 fold: we are trying to
>calculate the size of the framebuffer (and the "bits_per_unit" or
>Brian's bpp is useful for that), but we also try to validate the sizes
>passed by userspace based on the drm_fourcc.h+modifier info. In that
>case, the driver still needs to store somewhere the tile_w/tile_h for
>that given format in order to check that the stride is a whole multiple
>of tile sizes, so we thought that putting it in format_info is not
>entirely pointless, because others might use those variables in order to
>do their driver specific validation, without creating new structures.

Even if we only wanted to do one of those things (calculate the size
in bytes of the framebuffer), we need to know the bits_per_unit, as
well as the number of units. Without the second (+third) variables, we
can't figure out number of units and so we can't figure out the size.

Separate point: modifiers certainly shouldn't come in to this
discussion - modifier-related validation is always going to need to be
in the driver, and is a separate thing entirely (not contained in
drm_format_info).

>
>Did I capture the discussion correctly? If so, can we agree that it is
>not just the framebuffer size calculation that matters and that tiled
>formats validation requires a tile_w/tile_h info, therefore Alex's
>patches and Brian's need to be discussed separately (so that we can
>bikeshed on whether format_info is the right place or not)?

It looks to me that the right thing to do is set cpp to zero for the
"non integer" formats I want to add, and entirely defer to drivers to
figure those out (after all, any size calculation the core does for
them will be moot, because they are only valid with modifiers, and
modifiers can change the size).

How the tiled stuff (Y0L0, Y0L2) gets described can be a separate
topic. If it happens to support non-integer cpp formats as a side
effect, then more the better - when someone comes along with a
non-integer-cpp format which actually _is_ supported in linear layout
they can use it. I hope Daniel can describe in more detail exactly how
he sees that bit working though, because I don't think that either
Alex or me understand that yet.

Cheers,
-Brian

>
>Best regards,
>Liviu
>
>>
>> So two options that I think are reasonable:
>> - one common numerator/denumerator. I don't care how you call that
>>   bikeshed.
>> - don't check afbc using format_info, have your own helper that does that
>>   using custom code.
>>
>> Cheers, Daniel
>>
>> > Cheers,
>> > -Brian
>> >
>> > > -Daniel
>> > >
>> > > >
>> > > > Cheers,
>> > > > -Brian
>> > > >
>> > > > >
>> > > > > > ---
>> > > > > >  drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
>> > > > > >  drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
>> > > > > >  drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
>> > > > > >  drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
>> > > > > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
>> > > > > >  include/drm/drm_fourcc.h                     |  4 +++
>> > > > > >  6 files changed, 70 insertions(+), 9 deletions(-)
>> > > > > >
>> > > > > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
>> > > > > > index 186d00adfb5f..e279d70d3e60 100644
>> > > > > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> > > > > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> > > > > > @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
>> > > > > >  {
>> > > > > >  	struct drm_gem_cma_object *obj;
>> > > > > >  	dma_addr_t paddr;
>> > > > > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
>> > > > > > +
>> > > > > > +	/* This can't work for non-integer bytes-per-pixel */
>> > > > > > +	WARN_ON(bpp % 8);
>> > > > > >
>> > > > > >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
>> > > > > >  	if (!obj)
>> > > > > >  		return 0;
>> > > > > >
>> > > > > >  	paddr = obj->paddr + fb->offsets[plane];
>> > > > > > -	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
>> > > > > > +	paddr += (bpp / 8) * (state->src_x >> 16);
>> > > > > >  	paddr += fb->pitches[plane] * (state->src_y >> 16);
>> > > > > >
>> > > > > >  	return paddr;
>> > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> > > > > > index 0646b108030b..ab369f250af4 100644
>> > > > > > --- a/drivers/gpu/drm/drm_fb_helper.c
>> > > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
>> > > > > > @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>> > > > > >  	struct drm_fb_helper *fb_helper = info->par;
>> > > > > >  	struct drm_framebuffer *fb = fb_helper->fb;
>> > > > > >  	int depth;
>> > > > > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>> > > > > >
>> > > > > >  	if (var->pixclock != 0 || in_dbg_master())
>> > > > > >  		return -EINVAL;
>> > > > > > @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>> > > > > >  	 * Changes struct fb_var_screeninfo are currently not pushed back
>> > > > > >  	 * to KMS, hence fail if different settings are requested.
>> > > > > >  	 */
>> > > > > > -	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
>> > > > > > +	if (var->bits_per_pixel != bpp ||
>> > > > > >  	    var->xres > fb->width || var->yres > fb->height ||
>> > > > > >  	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
>> > > > > >  		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
>> > > > > >  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
>> > > > > >  			  var->xres, var->yres, var->bits_per_pixel,
>> > > > > >  			  var->xres_virtual, var->yres_virtual,
>> > > > > > -			  fb->width, fb->height, fb->format->cpp[0] * 8);
>> > > > > > +			  fb->width, fb->height, bpp);
>> > > > > >  		return -EINVAL;
>> > > > > >  	}
>> > > > > >
>> > > > > > @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
>> > > > > >  			    uint32_t fb_width, uint32_t fb_height)
>> > > > > >  {
>> > > > > >  	struct drm_framebuffer *fb = fb_helper->fb;
>> > > > > > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>> > > > > >
>> > > > > >  	info->pseudo_palette = fb_helper->pseudo_palette;
>> > > > > >  	info->var.xres_virtual = fb->width;
>> > > > > >  	info->var.yres_virtual = fb->height;
>> > > > > > -	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
>> > > > > > +	info->var.bits_per_pixel = bpp;
>> > > > > >  	info->var.accel_flags = FB_ACCELF_TEXT;
>> > > > > >  	info->var.xoffset = 0;
>> > > > > >  	info->var.yoffset = 0;
>> > > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>> > > > > > index 3b42c25bd58d..bb28919c32f3 100644
>> > > > > > --- a/drivers/gpu/drm/drm_fourcc.c
>> > > > > > +++ b/drivers/gpu/drm/drm_fourcc.c
>> > > > > > @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
>> > > > > >  	if (!info || plane >= info->num_planes)
>> > > > > >  		return 0;
>> > > > > >
>> > > > > > +	/*
>> > > > > > +	 * Not valid for formats with non-integer cpp,
>> > > > > > +	 * use drm_format{_info}_plane_bpp instead
>> > > > > > +	 */
>> > > > > > +	WARN_ON(!info->cpp[0]);
>> > > > > > +
>> > > > > >  	return info->cpp[plane];
>> > > > > >  }
>> > > > > >  EXPORT_SYMBOL(drm_format_plane_cpp);
>> > > > > >
>> > > > > > +/**
>> > > > > > + * drm_format_plane_bpp - determine the bits per pixel value
>> > > > > > + * @format: pixel format (DRM_FORMAT_*)
>> > > > > > + * @plane: plane index
>> > > > > > + *
>> > > > > > + * Returns:
>> > > > > > + * The bits per pixel value for the specified plane.
>> > > > > > + */
>> > > > > > +int drm_format_plane_bpp(uint32_t format, int plane)
>> > > > > > +{
>> > > > > > +	const struct drm_format_info *info;
>> > > > > > +
>> > > > > > +	info = drm_format_info(format);
>> > > > > > +	if (!info)
>> > > > > > +		return 0;
>> > > > > > +
>> > > > > > +	return drm_format_info_plane_bpp(info, plane);
>> > > > > > +}
>> > > > > > +EXPORT_SYMBOL(drm_format_plane_bpp);
>> > > > > > +
>> > > > > > +/**
>> > > > > > + * drm_format_info_plane_bpp - determine the bits per pixel value
>> > > > > > + *
>> > > > > > + * Convenience function which handles formats with both integer
>> > > > > > + * and non-integer bytes-per-pixel.
>> > > > > > + *
>> > > > > > + * @format: pixel format info structure
>> > > > > > + * @plane: plane index
>> > > > > > + *
>> > > > > > + * Returns:
>> > > > > > + * The bits per pixel value for the specified plane.
>> > > > > > + */
>> > > > > > +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
>> > > > > > +{
>> > > > > > +	if (plane >= info->num_planes)
>> > > > > > +		return 0;
>> > > > > > +
>> > > > > > +	if (info->cpp[0])
>> > > > > > +		return info->cpp[plane] * 8;
>> > > > > > +
>> > > > > > +	return info->bpp[plane];
>> > > > > > +}
>> > > > > > +EXPORT_SYMBOL(drm_format_info_plane_bpp);
>> > > > > > +
>> > > > > >  /**
>> > > > > >   * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
>> > > > > >   * @format: pixel format (DRM_FORMAT_*)
>> > > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> > > > > > index 8c4d32adcc17..7e00360ff70d 100644
>> > > > > > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > > > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > > > > > @@ -185,20 +185,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];
>> > > > > > +		unsigned int bpp = drm_format_info_plane_bpp(info, i);
>> > > > > >
>> > > > > >  		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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
>> > > > > >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
>> > > > > >  			return -EINVAL;
>> > > > > >  		}
>> > > > > > @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
>> > > > > >  	r->height = fb->height;
>> > > > > >  	r->width = fb->width;
>> > > > > >  	r->depth = fb->format->depth;
>> > > > > > -	r->bpp = fb->format->cpp[0] * 8;
>> > > > > > +	r->bpp = drm_format_info_plane_bpp(fb->format, 0);
>> > > > > >  	r->pitch = fb->pitches[0];
>> > > > > >
>> > > > > >  	/* GET_FB() is an unprivileged ioctl so we must not return a
>> > > > > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > > > > index acfbc0641a06..dfe224ccaeba 100644
>> > > > > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > > > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > > > > @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>> > > > > >  		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
>> > > > > >  		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
>> > > > > >  		unsigned int min_size;
>> > > > > > +		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
>> > > > > >
>> > > > > >  		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
>> > > > > >  		if (!objs[i]) {
>> > > > > > @@ -170,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]
>> > > > > > +			 + DIV_ROUND_UP(width * bpp, 8)
>> > > > > >  			 + 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 3e86408dac9f..d4af4dab1623 100644
>> > > > > > --- a/include/drm/drm_fourcc.h
>> > > > > > +++ b/include/drm/drm_fourcc.h
>> > > > > > @@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
>> > > > > >   *	use in new code and set to 0 for new formats.
>> > > > > >   * @num_planes: Number of color planes (1 to 3)
>> > > > > >   * @cpp: Number of bytes per pixel (per plane)
>> > > > > > + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
>> > > > > >   * @hsub: Horizontal chroma subsampling factor
>> > > > > >   * @vsub: Vertical chroma subsampling factor
>> > > > > >   * @has_alpha: Does the format embeds an alpha component?
>> > > > > > @@ -45,6 +46,7 @@ struct drm_format_info {
>> > > > > >  	u8 depth;
>> > > > > >  	u8 num_planes;
>> > > > > >  	u8 cpp[3];
>> > > > > > +	u8 bpp[3];
>> > > > > >  	u8 hsub;
>> > > > > >  	u8 vsub;
>> > > > > >  	bool has_alpha;
>> > > > > > @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
>> > > > > >  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
>> > > > > >  int drm_format_num_planes(uint32_t format);
>> > > > > >  int drm_format_plane_cpp(uint32_t format, int plane);
>> > > > > > +int drm_format_plane_bpp(uint32_t format, int plane);
>> > > > > > +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
>> > > > > >  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);
>> > > > > > --
>> > > > > > 2.16.1
>> > > > > >
>> > > > >
>> > > > > --
>> > > > > Daniel Vetter
>> > > > > Software Engineer, Intel Corporation
>> > > > > http://blog.ffwll.ch
>> > >
>> > > --
>> > > 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
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
>-- 
>====================
>| I would like to |
>| fix the world,  |
>| but they're not |
>| giving me the   |
> \ source code!  /
>  ---------------
>    ¯\_(ツ)_/¯

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Daniel Vetter]

On Wed, Sep 12, 2018 at 5:27 PM, Liviu Dudau <liviu.dudau@arm.com> wrote:
> On Mon, Sep 10, 2018 at 09:53:25PM +0200, Daniel Vetter wrote:
>> On Mon, Sep 10, 2018 at 09:50:03AM +0100, Brian Starkey wrote:
>> > Hi,
>> >
>> > On Fri, Sep 07, 2018 at 09:28:44PM +0200, Daniel Vetter wrote:
>> > > On Fri, Sep 07, 2018 at 01:45:36PM +0100, Brian Starkey wrote:
>> > > > Hi Daniel,
>> > > >
>> > > > On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
>> > > > > On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
>> > > > > > Some formats have a non-integer number of bytes per pixel, which can't
>> > > > > > be handled with the existing 'cpp' field in drm_format_info. To handle
>> > > > > > these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
>> > > > > >
>> > > > > > This updates all the users of format->cpp in the core DRM code,
>> > > > > > converting them to use a new function to get the bits-per-pixel for any
>> > > > > > format.
>> > > > > >
>> > > > > > It's assumed that drivers will use the 'bpp' field when they add support
>> > > > > > for pixel formats with non-integer bytes-per-pixel.
>> > > > > >
>> > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> > > > >
>> > > > > I assume you still require that stuff is eventually aligned to bytes? In
>> > > > > that case, can we subsume this into the tile work Alex is doing? It's
>> > > > > essentially just another special case of having storage-size units
>> > > > > measured in bytes which span more than 1x1 pixel. And I kinda don't want a
>> > > > > metric pile of special cases here in the format code, because that just
>> > > > > means every driver handles a different subset, with different bugs.
>> > > > > -Daniel
>> > > >
>> > > > Sorry for the delay, been struggling to free some cycles to think
>> > > > about this.
>> > > >
>> > > > I'm not sure how to pull this in with the tiling stuff. In the AFBC
>> > > > case then our AFBC superblocks are always nice round numbers (256
>> > > > pixels), and so it does end up being a multiple of bytes.
>> > > >
>> > > > However, AFBC supports different superblock sizes, so picking just one
>> > > > doesn't really work out, and putting AFBC in the core format table
>> > > > which reflects AFBC doesn't seem good.
>> > > >
>> > > > We could make something up (e.g. call these formats "tiled" with 2x4
>> > > > tiles, which guarantees a multiple of 8), but it would be an
>> > > > arbitrarily-selected lie, which often seems to spell trouble. If we
>> > > > did do that, would you re-define cpp as "bytes-per-tile"? Otherwise
>> > > > we still need to add a new field anyway.
>> > > >
>> > > > What's the pile of special cases you're worried about? The helper I've
>> > > > added here means that drivers which need to care can use one API and
>> > > > not implement their own bugs.
>> > >
>> > > I'm confused ... the new bits-per-pixel stuff you're adding here is for
>> > > yuv formats, not afbc. I'm just suggesting we have only 1 way of
>> > > describing such formats that need more descriptive power than cpp, whether
>> > > they have some kind of pixel-groups or small tiles.
>> >
>> > Well, not really. The three formats which have non-integer cpp are:
>> > DRM_FORMAT_VUY101010, DRM_FORMAT_YUV420_8BIT and
>> > DRM_FORMAT_YUV420_10BIT. These formats are only valid with non-linear
>> > modifiers (no linear encoding is defined). Mali only supports them
>> > with AFBC.
>> >
>> > The formats themselves have no notion of tiling or grouping - the
>> > modifier adds that. I'm not aware of any non-AFBC uses of these
>> > formats, so I don't want to "make up" a small-tile layout restriction
>> > for them.
>>
>> Ah, I missed that.
>>
>> > > For very special stuff like afbc you need to validate in the driver
>> > > anyway, too complicated. So I have no idea why you bring this up here?
>> >
>> > Sure, we can just let drivers provide their own format_info's for
>> > these, if that's what you prefer. The core format checking code can
>> > error out if it ever encounters them.
>>
>> It's format_info we're talking about. What I mean is that you just set all
>> these to 0 and let the format_info code ignore it. And then having a
>> bespoke drm_format_check_afbc helper function or similar, which checks all
>> the layout restrictions of afbc.
>>
>> I still maintain that bpp and tile_size are equavalent, and we really
>> don't need both. Both are defacto a form of numerator/denumerator. If you
>> don't like that you have to introduce "fake" tiles for afbc, then we can
>> rename tile_size to numerator and tile_h/w to denumerator_h/w. Doesn't
>> change one bit of the math. bpp simply hardcodes a denumerator of 8, and I
>> don't see why we need that special case. Except if you love to write
>> redundant self tests for all the math :-)
>
> My $.02 worth of thoughts:
>
> I get the fact that Daniel doesn't like us to add 3 new variables into
> format_info (bpp, tile_w, tile_h) and that adding a "bits_per_unit"
> variable should be able to take care of linear (where unit = 1 pixel)
> and tiled (where unit = tile_w * tile_h pixels) formats. And I also see
> Daniel's option 2 below, where he says it is reasonable to check AFBC
> without using format_info.
>
> However, the problem we are trying to solve is 2 fold: we are trying to
> calculate the size of the framebuffer (and the "bits_per_unit" or
> Brian's bpp is useful for that), but we also try to validate the sizes
> passed by userspace based on the drm_fourcc.h+modifier info. In that
> case, the driver still needs to store somewhere the tile_w/tile_h for
> that given format in order to check that the stride is a whole multiple
> of tile sizes, so we thought that putting it in format_info is not
> entirely pointless, because others might use those variables in order to
> do their driver specific validation, without creating new structures.
>
> Did I capture the discussion correctly? If so, can we agree that it is
> not just the framebuffer size calculation that matters and that tiled
> formats validation requires a tile_w/tile_h info, therefore Alex's
> patches and Brian's need to be discussed separately (so that we can
> bikeshed on whether format_info is the right place or not)?

I dont think this captures the gist of what I have in mind. I've laid
out the math in another reply. What I don't want is to add bpp and
tile_size/h/w, because that's redundant, and you can express any bpp
value in terms of tile_size/w. We should probably also throw out cpp,
but that's a bit more involved (well, just need a wrapper to compute
cpp and then a cocci script and done).
-Daniel

> Best regards,
> Liviu
>
>>
>> So two options that I think are reasonable:
>> - one common numerator/denumerator. I don't care how you call that
>>   bikeshed.
>> - don't check afbc using format_info, have your own helper that does that
>>   using custom code.
>>
>> Cheers, Daniel
>>
>> > Cheers,
>> > -Brian
>> >
>> > > -Daniel
>> > >
>> > > >
>> > > > Cheers,
>> > > > -Brian
>> > > >
>> > > > >
>> > > > > > ---
>> > > > > >  drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
>> > > > > >  drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
>> > > > > >  drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
>> > > > > >  drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
>> > > > > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
>> > > > > >  include/drm/drm_fourcc.h                     |  4 +++
>> > > > > >  6 files changed, 70 insertions(+), 9 deletions(-)
>> > > > > >
>> > > > > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
>> > > > > > index 186d00adfb5f..e279d70d3e60 100644
>> > > > > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> > > > > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> > > > > > @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
>> > > > > >  {
>> > > > > >     struct drm_gem_cma_object *obj;
>> > > > > >     dma_addr_t paddr;
>> > > > > > +   u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
>> > > > > > +
>> > > > > > +   /* This can't work for non-integer bytes-per-pixel */
>> > > > > > +   WARN_ON(bpp % 8);
>> > > > > >
>> > > > > >     obj = drm_fb_cma_get_gem_obj(fb, plane);
>> > > > > >     if (!obj)
>> > > > > >             return 0;
>> > > > > >
>> > > > > >     paddr = obj->paddr + fb->offsets[plane];
>> > > > > > -   paddr += fb->format->cpp[plane] * (state->src_x >> 16);
>> > > > > > +   paddr += (bpp / 8) * (state->src_x >> 16);
>> > > > > >     paddr += fb->pitches[plane] * (state->src_y >> 16);
>> > > > > >
>> > > > > >     return paddr;
>> > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> > > > > > index 0646b108030b..ab369f250af4 100644
>> > > > > > --- a/drivers/gpu/drm/drm_fb_helper.c
>> > > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
>> > > > > > @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>> > > > > >     struct drm_fb_helper *fb_helper = info->par;
>> > > > > >     struct drm_framebuffer *fb = fb_helper->fb;
>> > > > > >     int depth;
>> > > > > > +   u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>> > > > > >
>> > > > > >     if (var->pixclock != 0 || in_dbg_master())
>> > > > > >             return -EINVAL;
>> > > > > > @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>> > > > > >      * Changes struct fb_var_screeninfo are currently not pushed back
>> > > > > >      * to KMS, hence fail if different settings are requested.
>> > > > > >      */
>> > > > > > -   if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
>> > > > > > +   if (var->bits_per_pixel != bpp ||
>> > > > > >         var->xres > fb->width || var->yres > fb->height ||
>> > > > > >         var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
>> > > > > >             DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
>> > > > > >                       "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
>> > > > > >                       var->xres, var->yres, var->bits_per_pixel,
>> > > > > >                       var->xres_virtual, var->yres_virtual,
>> > > > > > -                     fb->width, fb->height, fb->format->cpp[0] * 8);
>> > > > > > +                     fb->width, fb->height, bpp);
>> > > > > >             return -EINVAL;
>> > > > > >     }
>> > > > > >
>> > > > > > @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
>> > > > > >                         uint32_t fb_width, uint32_t fb_height)
>> > > > > >  {
>> > > > > >     struct drm_framebuffer *fb = fb_helper->fb;
>> > > > > > +   u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
>> > > > > >
>> > > > > >     info->pseudo_palette = fb_helper->pseudo_palette;
>> > > > > >     info->var.xres_virtual = fb->width;
>> > > > > >     info->var.yres_virtual = fb->height;
>> > > > > > -   info->var.bits_per_pixel = fb->format->cpp[0] * 8;
>> > > > > > +   info->var.bits_per_pixel = bpp;
>> > > > > >     info->var.accel_flags = FB_ACCELF_TEXT;
>> > > > > >     info->var.xoffset = 0;
>> > > > > >     info->var.yoffset = 0;
>> > > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>> > > > > > index 3b42c25bd58d..bb28919c32f3 100644
>> > > > > > --- a/drivers/gpu/drm/drm_fourcc.c
>> > > > > > +++ b/drivers/gpu/drm/drm_fourcc.c
>> > > > > > @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
>> > > > > >     if (!info || plane >= info->num_planes)
>> > > > > >             return 0;
>> > > > > >
>> > > > > > +   /*
>> > > > > > +    * Not valid for formats with non-integer cpp,
>> > > > > > +    * use drm_format{_info}_plane_bpp instead
>> > > > > > +    */
>> > > > > > +   WARN_ON(!info->cpp[0]);
>> > > > > > +
>> > > > > >     return info->cpp[plane];
>> > > > > >  }
>> > > > > >  EXPORT_SYMBOL(drm_format_plane_cpp);
>> > > > > >
>> > > > > > +/**
>> > > > > > + * drm_format_plane_bpp - determine the bits per pixel value
>> > > > > > + * @format: pixel format (DRM_FORMAT_*)
>> > > > > > + * @plane: plane index
>> > > > > > + *
>> > > > > > + * Returns:
>> > > > > > + * The bits per pixel value for the specified plane.
>> > > > > > + */
>> > > > > > +int drm_format_plane_bpp(uint32_t format, int plane)
>> > > > > > +{
>> > > > > > +   const struct drm_format_info *info;
>> > > > > > +
>> > > > > > +   info = drm_format_info(format);
>> > > > > > +   if (!info)
>> > > > > > +           return 0;
>> > > > > > +
>> > > > > > +   return drm_format_info_plane_bpp(info, plane);
>> > > > > > +}
>> > > > > > +EXPORT_SYMBOL(drm_format_plane_bpp);
>> > > > > > +
>> > > > > > +/**
>> > > > > > + * drm_format_info_plane_bpp - determine the bits per pixel value
>> > > > > > + *
>> > > > > > + * Convenience function which handles formats with both integer
>> > > > > > + * and non-integer bytes-per-pixel.
>> > > > > > + *
>> > > > > > + * @format: pixel format info structure
>> > > > > > + * @plane: plane index
>> > > > > > + *
>> > > > > > + * Returns:
>> > > > > > + * The bits per pixel value for the specified plane.
>> > > > > > + */
>> > > > > > +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
>> > > > > > +{
>> > > > > > +   if (plane >= info->num_planes)
>> > > > > > +           return 0;
>> > > > > > +
>> > > > > > +   if (info->cpp[0])
>> > > > > > +           return info->cpp[plane] * 8;
>> > > > > > +
>> > > > > > +   return info->bpp[plane];
>> > > > > > +}
>> > > > > > +EXPORT_SYMBOL(drm_format_info_plane_bpp);
>> > > > > > +
>> > > > > >  /**
>> > > > > >   * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
>> > > > > >   * @format: pixel format (DRM_FORMAT_*)
>> > > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> > > > > > index 8c4d32adcc17..7e00360ff70d 100644
>> > > > > > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > > > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > > > > > @@ -185,20 +185,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];
>> > > > > > +           unsigned int bpp = drm_format_info_plane_bpp(info, i);
>> > > > > >
>> > > > > >             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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
>> > > > > >                     DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
>> > > > > >                     return -EINVAL;
>> > > > > >             }
>> > > > > > @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
>> > > > > >     r->height = fb->height;
>> > > > > >     r->width = fb->width;
>> > > > > >     r->depth = fb->format->depth;
>> > > > > > -   r->bpp = fb->format->cpp[0] * 8;
>> > > > > > +   r->bpp = drm_format_info_plane_bpp(fb->format, 0);
>> > > > > >     r->pitch = fb->pitches[0];
>> > > > > >
>> > > > > >     /* GET_FB() is an unprivileged ioctl so we must not return a
>> > > > > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > > > > index acfbc0641a06..dfe224ccaeba 100644
>> > > > > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > > > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > > > > @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>> > > > > >             unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
>> > > > > >             unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
>> > > > > >             unsigned int min_size;
>> > > > > > +           u8 bpp = drm_format_info_plane_bpp(fb->format, i);
>> > > > > >
>> > > > > >             objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
>> > > > > >             if (!objs[i]) {
>> > > > > > @@ -170,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]
>> > > > > > +                    + DIV_ROUND_UP(width * bpp, 8)
>> > > > > >                      + 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 3e86408dac9f..d4af4dab1623 100644
>> > > > > > --- a/include/drm/drm_fourcc.h
>> > > > > > +++ b/include/drm/drm_fourcc.h
>> > > > > > @@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
>> > > > > >   * use in new code and set to 0 for new formats.
>> > > > > >   * @num_planes: Number of color planes (1 to 3)
>> > > > > >   * @cpp: Number of bytes per pixel (per plane)
>> > > > > > + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
>> > > > > >   * @hsub: Horizontal chroma subsampling factor
>> > > > > >   * @vsub: Vertical chroma subsampling factor
>> > > > > >   * @has_alpha: Does the format embeds an alpha component?
>> > > > > > @@ -45,6 +46,7 @@ struct drm_format_info {
>> > > > > >     u8 depth;
>> > > > > >     u8 num_planes;
>> > > > > >     u8 cpp[3];
>> > > > > > +   u8 bpp[3];
>> > > > > >     u8 hsub;
>> > > > > >     u8 vsub;
>> > > > > >     bool has_alpha;
>> > > > > > @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
>> > > > > >  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
>> > > > > >  int drm_format_num_planes(uint32_t format);
>> > > > > >  int drm_format_plane_cpp(uint32_t format, int plane);
>> > > > > > +int drm_format_plane_bpp(uint32_t format, int plane);
>> > > > > > +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
>> > > > > >  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);
>> > > > > > --
>> > > > > > 2.16.1
>> > > > > >
>> > > > >
>> > > > > --
>> > > > > Daniel Vetter
>> > > > > Software Engineer, Intel Corporation
>> > > > > http://blog.ffwll.ch
>> > >
>> > > --
>> > > 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
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Ayan Halder]

On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
> On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:
> > Some formats have a non-integer number of bytes per pixel, which can't
> > be handled with the existing 'cpp' field in drm_format_info. To handle
> > these formats, add a 'bpp' field, which is only used if cpp[0] == 0.
> > 
> > This updates all the users of format->cpp in the core DRM code,
> > converting them to use a new function to get the bits-per-pixel for any
> > format.
> > 
> > It's assumed that drivers will use the 'bpp' field when they add support
> > for pixel formats with non-integer bytes-per-pixel.
> > 
> > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> 
> I assume you still require that stuff is eventually aligned to bytes? In
> that case, can we subsume this into the tile work Alex is doing? It's
> essentially just another special case of having storage-size units
> measured in bytes which span more than 1x1 pixel. And I kinda don't want a
> metric pile of special cases here in the format code, because that just
> means every driver handles a different subset, with different bugs.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_fb_cma_helper.c          |  6 +++-
> >  drivers/gpu/drm/drm_fb_helper.c              |  8 +++--
> >  drivers/gpu/drm/drm_fourcc.c                 | 50 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_framebuffer.c            |  8 ++---
> >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
> >  include/drm/drm_fourcc.h                     |  4 +++
> >  6 files changed, 70 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > index 186d00adfb5f..e279d70d3e60 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -118,13 +118,17 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> >  {
> >  	struct drm_gem_cma_object *obj;
> >  	dma_addr_t paddr;
> > +	u8 bpp = drm_format_info_plane_bpp(fb->format, plane);
> > +
> > +	/* This can't work for non-integer bytes-per-pixel */
> > +	WARN_ON(bpp % 8);
> >  
> >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
> >  	if (!obj)
> >  		return 0;
> >  
> >  	paddr = obj->paddr + fb->offsets[plane];
> > -	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
> > +	paddr += (bpp / 8) * (state->src_x >> 16);
> >  	paddr += fb->pitches[plane] * (state->src_y >> 16);
> >  
> >  	return paddr;
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 0646b108030b..ab369f250af4 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1572,6 +1572,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >  	struct drm_fb_helper *fb_helper = info->par;
> >  	struct drm_framebuffer *fb = fb_helper->fb;
> >  	int depth;
> > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
> >  
> >  	if (var->pixclock != 0 || in_dbg_master())
> >  		return -EINVAL;
> > @@ -1580,14 +1581,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >  	 * Changes struct fb_var_screeninfo are currently not pushed back
> >  	 * to KMS, hence fail if different settings are requested.
> >  	 */
> > -	if (var->bits_per_pixel != fb->format->cpp[0] * 8 ||
> > +	if (var->bits_per_pixel != bpp ||
> >  	    var->xres > fb->width || var->yres > fb->height ||
> >  	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> >  		DRM_DEBUG("fb requested width/height/bpp can't fit in current fb "
> >  			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
> >  			  var->xres, var->yres, var->bits_per_pixel,
> >  			  var->xres_virtual, var->yres_virtual,
> > -			  fb->width, fb->height, fb->format->cpp[0] * 8);
> > +			  fb->width, fb->height, bpp);
> >  		return -EINVAL;
> >  	}
> >  
> > @@ -1949,11 +1950,12 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> >  			    uint32_t fb_width, uint32_t fb_height)
> >  {
> >  	struct drm_framebuffer *fb = fb_helper->fb;
> > +	u8 bpp = drm_format_info_plane_bpp(fb->format, 0);
> >  
> >  	info->pseudo_palette = fb_helper->pseudo_palette;
> >  	info->var.xres_virtual = fb->width;
> >  	info->var.yres_virtual = fb->height;
> > -	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
> > +	info->var.bits_per_pixel = bpp;
> >  	info->var.accel_flags = FB_ACCELF_TEXT;
> >  	info->var.xoffset = 0;
> >  	info->var.yoffset = 0;
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 3b42c25bd58d..bb28919c32f3 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -272,10 +272,60 @@ int drm_format_plane_cpp(uint32_t format, int plane)
> >  	if (!info || plane >= info->num_planes)
> >  		return 0;
> >  
> > +	/*
> > +	 * Not valid for formats with non-integer cpp,
> > +	 * use drm_format{_info}_plane_bpp instead
> > +	 */
> > +	WARN_ON(!info->cpp[0]);
> > +
> >  	return info->cpp[plane];
> >  }
> >  EXPORT_SYMBOL(drm_format_plane_cpp);
> >  
> > +/**
> > + * drm_format_plane_bpp - determine the bits per pixel value
> > + * @format: pixel format (DRM_FORMAT_*)
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The bits per pixel value for the specified plane.
> > + */
> > +int drm_format_plane_bpp(uint32_t format, int plane)
> > +{
> > +	const struct drm_format_info *info;
> > +
> > +	info = drm_format_info(format);
> > +	if (!info)
> > +		return 0;
> > +
> > +	return drm_format_info_plane_bpp(info, plane);
> > +}
> > +EXPORT_SYMBOL(drm_format_plane_bpp);
> > +
> > +/**
> > + * drm_format_info_plane_bpp - determine the bits per pixel value
> > + *
> > + * Convenience function which handles formats with both integer
> > + * and non-integer bytes-per-pixel.
> > + *
> > + * @format: pixel format info structure
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The bits per pixel value for the specified plane.
> > + */
> > +int drm_format_info_plane_bpp(const struct drm_format_info *info, int plane)
> > +{
> > +	if (plane >= info->num_planes)
> > +		return 0;
> > +
> > +	if (info->cpp[0])
> > +		return info->cpp[plane] * 8;
> > +
> > +	return info->bpp[plane];
> > +}
> > +EXPORT_SYMBOL(drm_format_info_plane_bpp);
> > +
> >  /**
> >   * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
> >   * @format: pixel format (DRM_FORMAT_*)
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 8c4d32adcc17..7e00360ff70d 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -185,20 +185,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];
> > +		unsigned int bpp = drm_format_info_plane_bpp(info, i);
> >  
> >  		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 ((uint64_t) DIV_ROUND_UP(width * bpp, 8) > 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 ((uint64_t) r->pitches[i] * 8 < (uint64_t) width * bpp) {
> >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> >  			return -EINVAL;
> >  		}
> > @@ -476,7 +476,7 @@ int drm_mode_getfb(struct drm_device *dev,
> >  	r->height = fb->height;
> >  	r->width = fb->width;
> >  	r->depth = fb->format->depth;
> > -	r->bpp = fb->format->cpp[0] * 8;
> > +	r->bpp = drm_format_info_plane_bpp(fb->format, 0);
> >  	r->pitch = fb->pitches[0];
> >  
> >  	/* GET_FB() is an unprivileged ioctl so we must not return a
> > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > index acfbc0641a06..dfe224ccaeba 100644
> > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > @@ -161,6 +161,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> >  		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> >  		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> >  		unsigned int min_size;
> > +		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
You might want to pass info here instead of fb->format because fb is
NULL. ie drm_format_info_plane_bpp(info, i);

> >  
> >  		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
> >  		if (!objs[i]) {
> > @@ -170,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]
> > +			 + DIV_ROUND_UP(width * bpp, 8)
> >  			 + 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 3e86408dac9f..d4af4dab1623 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -36,6 +36,7 @@ struct drm_mode_fb_cmd2;
> >   *	use in new code and set to 0 for new formats.
> >   * @num_planes: Number of color planes (1 to 3)
> >   * @cpp: Number of bytes per pixel (per plane)
> > + * @bpp: Number of bits per pixel (per plane), only valid if cpp[0] == 0.
> >   * @hsub: Horizontal chroma subsampling factor
> >   * @vsub: Vertical chroma subsampling factor
> >   * @has_alpha: Does the format embeds an alpha component?
> > @@ -45,6 +46,7 @@ struct drm_format_info {
> >  	u8 depth;
> >  	u8 num_planes;
> >  	u8 cpp[3];
> > +	u8 bpp[3];
> >  	u8 hsub;
> >  	u8 vsub;
> >  	bool has_alpha;
> > @@ -66,6 +68,8 @@ drm_get_format_info(struct drm_device *dev,
> >  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
> >  int drm_format_num_planes(uint32_t format);
> >  int drm_format_plane_cpp(uint32_t format, int plane);
> > +int drm_format_plane_bpp(uint32_t format, int plane);
> > +int drm_format_info_plane_bpp(const struct drm_format_info *format, int plane);
> >  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);
> > -- 
> > 2.16.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

Re: [RFC PATCH v2 1/3] drm/fourcc: Add 'bpp' field for formats with non-integer bytes-per-pixel

[Brian Starkey]

Hi Ayan,

On Mon, Sep 10, 2018 at 03:11:34PM +0100, Ayan Halder wrote:
>On Fri, Aug 31, 2018 at 10:17:30AM +0200, Daniel Vetter wrote:
>> On Thu, Aug 23, 2018 at 04:23:41PM +0100, Brian Starkey wrote:

[snip]

>> >  		unsigned int min_size;
>> > +		u8 bpp = drm_format_info_plane_bpp(fb->format, i);
>You might want to pass info here instead of fb->format because fb is
>NULL. ie drm_format_info_plane_bpp(info, i);
>

Thanks, good catch. I have to admit I only compile-tested these.

-Brian

[RFC PATCH v2 2/3] drm/fourcc: Add remaining fourccs for Mali

[Brian Starkey]

As we look to enable AFBC using DRM format modifiers, we run into
problems which we've historically handled via vendor-private details
(i.e. gralloc, on Android).

The AFBC modifiers can be found in [3].

AFBC (as an encoding) is fully flexible, and for example YUV data can
be encoded into 1, 2 or 3 encoded "planes", much like the linear
equivalents. Component order is also meaningful, as AFBC doesn't
necessarily care about what each "channel" of the data it encodes
contains. Therefore ABGR8888 and RGBA8888 can be encoded in AFBC with
different representations. Similarly, 'X' components may be encoded
into AFBC streams in cases where a decoder expects to decode a 4th
component.

In addition, AFBC is a licensable IP, meaning that to support the
ecosystem we need to ensure that _all_ AFBC users are able to describe
the encodings that they need. This is much better achieved by
preserving meaning in the fourcc codes when they are combined with an
AFBC modifier.

In essence, we want to use the modifier to describe the parameters of
the AFBC encode/decode, and use the fourcc code to describe the data
being encoded/decoded.

To do anything different would be to introduce redundancy - we would
need to duplicate in the modifier information which is _already_
conveyed clearly and non-ambigiously by a fourcc code.

I hope that for RGB this is non-controversial.
(BGRA8888 + MODIFIER_AFBC) is a different format from
(RGBA8888 + MODIFIER_AFBC).

Possibly more controversial is that (XBGR8888 + MODIFIER_AFBC)
is different from (BGR888 + MODIFIER_AFBC). I understand that in some
schemes it is not the case - but in AFBC it is so.

Where we run into problems is where there are not already fourcc codes
which represent the data which the AFBC encoder/decoder is processing.
To that end, we want to introduce new fourcc codes to describe the
data being encoded/decoded, in the places where none of the existing
fourcc codes are applicable.

Where we don't support an equivalent non-compressed layout, or where
no "obvious" linear layout exists, we are proposing adding fourcc
codes which have no associated linear layout - because any layout we
proposed would be completely arbitrary.

Some formats are following the naming conventions from [2].

The summary of the new formats is:
 DRM_FORMAT_VUY888 - Packed 8-bit YUV 444. Y followed by U then V.
 DRM_FORMAT_VUY101010 - Packed 10-bit YUV 444. Y followed by U then
                        V. No defined linear encoding.
 DRM_FORMAT_Y210 - Packed 10-bit YUV 422. Y followed by U (then Y)
                   then V. 10-bit samples in 16-bit words.
 DRM_FORMAT_Y410 - Packed 10-bit YUV 444, with 2-bit alpha.
 DRM_FORMAT_P210 - Semi-planar 10-bit YUV 422. Y plane, followed by
                   interleaved U-then-V plane. 10-bit samples in
                   16-bit words.
 DRM_FORMAT_YUV420_8BIT - Packed 8-bit YUV 420. Y followed by U then
                          V. No defined linear encoding
 DRM_FORMAT_YUV420_10BIT - Packed 10-bit YUV 420. Y followed by U
                           then V. No defined linear encoding

Please also note that in the absence of AFBC, we would still need to
add Y410, Y210 and P210.

Full rationale follows:

YUV 444 8-bit, 1-plane
----------------------
 The currently defined AYUV format encodes a 4th alpha component,
 which makes it unsuitable for representing a 3-component YUV 444
 AFBC stream.

 The proposed[1] XYUV format which is supported by Mali-DP in linear
 layout is also unsuitable, because the component order is the
 opposite of the AFBC version, and it encodes a 4th 'X' component.

 DRM_FORMAT_VUY888 is the "obvious" format for a 3-component, packed,
 YUV 444 8-bit format, with the component order which our HW expects to
 encode/decode. It conforms to the same naming convention as the
 existing packed YUV 444 format.
 The naming here is meant to be consistent with DRM_FORMAT_AYUV and
 DRM_FORMAT_XYUV[1]

YUV 444 10-bit, 1-plane
-----------------------
 There is no currently-defined YUV 444 10-bit format in
 drm_fourcc.h, irrespective of number of planes.

 The proposed[1] XVYU2101010 format which is supported by Mali-DP in
 linear layout uses the wrong component order, and also encodes a 4th
 'X' component, which doesn't match the AFBC version of YUV 444
 10-bit which we support.

 DRM_FORMAT_Y410 is the same layout as XVYU2101010, but with 2 bits of
 alpha.  This format is supported with linear layout by Mali GPUs. The
 naming follows[2].

 There is no "obvious" linear encoding for a 3-component 10:10:10
 packed format, and so DRM_FORMAT_VUY101010 defines a component
 order, but not a bit encoding. Again, the naming is meant to be
 consistent with DRM_FORMAT_AYUV.

YUV 422 8-bit, 1-plane
----------------------
 The existing DRM_FORMAT_YUYV (and the other component orders) are
 single-planar YUV 422 8-bit formats. Following the convention of
 the component orders of the RGB formats, YUYV has the correct
 component order for our AFBC encoding (Y followed by U followed by
 V). We can use YUYV for AFBC YUV 422 8-bit.

YUV 422 10-bit, 1-plane
-----------------------
 There is no currently-defined YUV 422 10-bit format in drm_fourcc.h

 DRM_FORMAT_Y210 is analogous to YUYV, but with 10-bits per sample
 packed into the upper 10-bits of 16-bit samples. This format is
 supported in both linear and AFBC by Mali GPUs.

YUV 422 10-bit, 2-plane
-----------------------
 The recently defined DRM_FORMAT_P010 format is a 10-bit semi-planar
 YUV 420 format, which has the correct component ordering for an AFBC
 2-plane YUV 420 buffer. The linear layout contains meaningless padding
 bits, which will not be encoded in an AFBC stream.

YUV 420 8-bit, 1-plane
----------------------
 There is no currently defined single-planar YUV 420, 8-bit format
 in drm_fourcc.h. There's differing opinions on whether using the
 existing fourcc-implied n_planes where possible is a good idea or
 not when using modifiers.

 For me, it's much more "obvious" to use NV12 for 2-plane AFBC and
 YUV420 for 3-plane AFBC. This keeps the aforementioned separation
 between the AFBC codec settings (in the modifier) and the pixel data
 format (in the fourcc). With different vendors using AFBC, this helps
 to ensure that there is no confusion in interoperation. It also
 ensures that the AFBC modifiers describe AFBC itself (which is a
 licensable component), and not implementation details which are not
 defined by AFBC.

 The proposed[1] X0L0 format which Mali-DP supports with Linear layout
 is unsuitable, as it contains a 4th 'X' component, and our AFBC
 decoder expects only 3 components.

 To that end, we propose a new YUV 420 8-bit format. There is no
 "obvious" linear encoding for a 3-component 8:8:8, 420, packed format,
 and so DRM_FORMAT_YUV420_8BIT defines a component order, but not a
 bit encoding. I'm happy to hear different naming suggestions.

YUV 420 8-bit, 2-, 3-plane
--------------------------
 These already exist, we can use NV12 and YUV420.

YUV 420 10-bit, 1-plane
-----------------------
 As above, no current definition exists, and X0L2 encodes a 4th 'X'
 channel.

 Analogous to DRM_FORMAT_YUV420_8BIT, we define DRM_FORMAT_YUV420_10BIT.

[1] https://lists.freedesktop.org/archives/dri-devel/2018-July/184598.html
[2] https://docs.microsoft.com/en-us/windows/desktop/medfound/10-bit-and-16-bit-yuv-video-formats
[3] https://patchwork.kernel.org/patch/10509875/

Changes since RFC v1:
 - Fix confusing subsampling vs bit-depth X:X:X notation in
   descriptions (danvet)
 - Rename DRM_FORMAT_AVYU1101010 to DRM_FORMAT_Y410 (Lisa Wu)
 - Add drm_format_info structures for the new formats, using the
   new 'bpp' field for those with non-integer bytes-per-pixel
 - Rebase, including Juha-Pekka Heikkila's format definitions

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_fourcc.c      |  7 +++++++
 drivers/gpu/drm/drm_framebuffer.c | 33 +++++++++++++++++++++++++++++----
 include/uapi/drm/drm_fourcc.h     | 14 ++++++++++++++
 3 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index bb28919c32f3..f830acc3c3f0 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -172,16 +172,23 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_YVYU,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
 		{ .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
 		{ .format = DRM_FORMAT_VYUY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_Y210,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 2, .vsub = 1 },
 		{ .format = DRM_FORMAT_XYUV8888,	.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_VUY888,		.depth = 0,  .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_XVYU2101010,	.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_Y410,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
 		{ .format = DRM_FORMAT_Y0L0,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true },
 		{ .format = DRM_FORMAT_X0L0,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2 },
 		{ .format = DRM_FORMAT_Y0L2,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true },
 		{ .format = DRM_FORMAT_X0L2,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2 },
 		{ .format = DRM_FORMAT_AYUV,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
+		{ .format = DRM_FORMAT_P210,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 1 },
 		{ .format = DRM_FORMAT_P010,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2 },
 		{ .format = DRM_FORMAT_P012,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2 },
 		{ .format = DRM_FORMAT_P016,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_VUY101010,	.depth = 0,  .num_planes = 1, .cpp = { 0, 0, 0 }, .bpp = { 30, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_YUV420_8BIT,	.depth = 0,  .num_planes = 1, .cpp = { 0, 0, 0 }, .bpp = { 12, 0, 0 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_YUV420_10BIT,	.depth = 0,  .num_planes = 1, .cpp = { 0, 0, 0 }, .bpp = { 15, 0, 0 }, .hsub = 2, .vsub = 2 },
 	};
 
 	unsigned int i;
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 7e00360ff70d..6654ff7cb41e 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -152,6 +152,18 @@ static int fb_plane_height(int height,
 	return DIV_ROUND_UP(height, format->vsub);
 }
 
+static bool format_is_non_linear_only(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_VUY101010:
+	case DRM_FORMAT_YUV420_8BIT:
+	case DRM_FORMAT_YUV420_10BIT:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int framebuffer_check(struct drm_device *dev,
 			     const struct drm_mode_fb_cmd2 *r)
 {
@@ -209,10 +221,23 @@ static int framebuffer_check(struct drm_device *dev,
 			return -EINVAL;
 		}
 
-		if (r->flags & DRM_MODE_FB_MODIFIERS &&
-		    r->modifier[i] != r->modifier[0]) {
-			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
-				      r->modifier[i], i);
+		if (r->flags & DRM_MODE_FB_MODIFIERS) {
+			if (r->modifier[i] != r->modifier[0]) {
+				DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
+					      r->modifier[i], i);
+				return -EINVAL;
+			}
+		}
+
+		if ((!(r->flags & DRM_MODE_FB_MODIFIERS) ||
+		    r->modifier[i] == DRM_FORMAT_MOD_LINEAR) &&
+		    format_is_non_linear_only(r->pixel_format)) {
+			struct drm_format_name_buf format_name;
+
+			DRM_DEBUG_KMS("format %s cannot be linear\n",
+				      drm_get_format_name(r->pixel_format,
+							  &format_name));
+
 			return -EINVAL;
 		}
 
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 3bef399ffd8c..41f4610e471c 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -146,11 +146,15 @@ extern "C" {
 #define DRM_FORMAT_YVYU		fourcc_code('Y', 'V', 'Y', 'U') /* [31:0] Cb0:Y1:Cr0:Y0 8:8:8:8 little endian */
 #define DRM_FORMAT_UYVY		fourcc_code('U', 'Y', 'V', 'Y') /* [31:0] Y1:Cr0:Y0:Cb0 8:8:8:8 little endian */
 #define DRM_FORMAT_VYUY		fourcc_code('V', 'Y', 'U', 'Y') /* [31:0] Y1:Cb0:Y0:Cr0 8:8:8:8 little endian */
+#define DRM_FORMAT_Y210		fourcc_code('Y', '2', '1', '0') /* [63:0] Cr0:0:Y1:0:Cb0:0:Y0:0 10:6:10:6:10:6:10:6 little endian */
 
 /* packed YCbCr444 */
 #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */
 #define DRM_FORMAT_XYUV8888	fourcc_code('X', 'Y', 'U', 'V') /* [31:0] X:Y:Cb:Cr 8:8:8:8 little endian */
+#define DRM_FORMAT_VUY888	fourcc_code('V', 'U', '2', '4') /* [23:0] Cr:Cb:Y 8:8:8 little endian */
 #define DRM_FORMAT_XVYU2101010	fourcc_code('X', 'V', '3', '0') /* [31:0] X:Cr:Y:Cb 2:10:10:10 little endian */
+#define DRM_FORMAT_Y410 	fourcc_code('Y', '4', '1', '0') /* [31:0] A:Cr:Y:Cb 2:10:10:10 little endian */
+#define DRM_FORMAT_VUY101010    fourcc_code('V', 'U', '3', '0') /* Y followed by U then V, 10:10:10. Non-linear modifier only */
 
 /*
  * packed YCbCr420 2x2 tiled formats
@@ -167,6 +171,15 @@ extern "C" {
 /* [63:0]   X3:X2:Y3:Cr0:Y2:X1:X0:Y1:Cb0:Y0  1:1:10:10:10:1:1:10:10:10 little endian */
 #define DRM_FORMAT_X0L2		fourcc_code('X', '0', 'L', '2')
 
+/*
+ * 1-plane YUV 4:2:0
+ * In these formats, the component ordering is specified (Y, followed by U
+ * then V), but the exact Linear layout is undefined.
+ * These formats can only be used with a non-Linear modifier.
+ */
+#define DRM_FORMAT_YUV420_8BIT	fourcc_code('Y', 'U', '0', '8')
+#define DRM_FORMAT_YUV420_10BIT	fourcc_code('Y', 'U', '1', '0')
+
 /*
  * 2 plane RGB + A
  * index 0 = RGB plane, same format as the corresponding non _A8 format has
@@ -201,6 +214,7 @@ extern "C" {
  * component xxx msb Y [xxx:16-xxx]
  * index 1 = Cr:Cb plane, [31:0] Cr:Cb little endian [xxx:16-xxx:xxx:16-xxx]
  */
+#define DRM_FORMAT_P210		fourcc_code('P', '2', '1', '0') /* 2x1 subsampled Cr:Cb plane, 10 bit per channel */
 #define DRM_FORMAT_P010		fourcc_code('P', '0', '1', '0') /* 2x2 subsampled Cr:Cb plane, 10 bit per channel */
 #define DRM_FORMAT_P012		fourcc_code('P', '0', '1', '2') /* 2x2 subsampled Cr:Cb plane, 12 bit per channel */
 #define DRM_FORMAT_P016		fourcc_code('P', '0', '1', '6') /* 2x2 subsampled Cr:Cb plane, 16 bit per channel */
-- 
2.16.1

[RFC PATCH v2 3/3] drm/afbc: Add AFBC modifier usage documentation

[Brian Starkey]

AFBC is a flexible, proprietary, lossless compression protocol and
format, with a number of defined DRM format modifiers. To facilitate
consistency and compatibility between different AFBC producers and
consumers, document the expectations for usage of the AFBC DRM format
modifiers in a new .rst chapter.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 Documentation/gpu/afbc.rst    | 226 ++++++++++++++++++++++++++++++++++++++++++
 Documentation/gpu/drivers.rst |   1 +
 MAINTAINERS                   |   1 +
 include/uapi/drm/drm_fourcc.h |   3 +
 4 files changed, 231 insertions(+)
 create mode 100644 Documentation/gpu/afbc.rst

diff --git a/Documentation/gpu/afbc.rst b/Documentation/gpu/afbc.rst
new file mode 100644
index 000000000000..58d2b6b5d48b
--- /dev/null
+++ b/Documentation/gpu/afbc.rst
@@ -0,0 +1,226 @@
+===================================
+ Arm Framebuffer Compression (AFBC)
+===================================
+
+AFBC is a proprietary lossless image compression protocol and format.
+It provides fine-grained random access and minimizes the amount of
+data transferred between IP blocks.
+
+AFBC can be enabled on drivers which support it via use of the AFBC
+format modifiers defined in drm_fourcc.h. See DRM_FORMAT_MOD_ARM_AFBC(*).
+
+All users of the AFBC modifiers must follow the usage guidelines laid
+out in this document, to ensure compatibility across different AFBC
+producers and consumers.
+
+Components and Ordering
+=======================
+
+AFBC streams can contain several components - where a component
+corresponds to a color channel (i.e. R, G, B, X, A, Y, Cb, Cr).
+The assignment of input/output color channels must be consistent
+between the encoder and the decoder for correct operation, otherwise
+the consumer will interpret the decoded data incorrectly.
+
+Furthermore, when the lossless colorspace transform is used
+(AFBC_FORMAT_MOD_YTR, which should be enabled for RGB buffers for
+maximum compression efficiency), the component order must be:
+
+ * Component 0: R
+ * Component 1: G
+ * Component 2: B
+
+The component ordering is communicated via the fourcc code in the
+fourcc:modifier pair. In general, component '0' is considered to
+reside in the least-significant bits of the corresponding linear
+format. For example, COMP(bits):
+
+ * DRM_FORMAT_ABGR8888
+
+   * Component 0: R(8)
+   * Component 1: G(8)
+   * Component 2: B(8)
+   * Component 3: A(8)
+
+ * DRM_FORMAT_BGR888
+
+   * Component 0: R(8)
+   * Component 1: G(8)
+   * Component 2: B(8)
+
+ * DRM_FORMAT_YUYV
+
+   * Component 0: Y(8)
+   * Component 1: Cb(8, 2x1 subsampled)
+   * Component 2: Cr(8, 2x1 subsampled)
+
+In AFBC, 'X' components are not treated any differently from any other
+component. Therefore, an AFBC buffer with fourcc DRM_FORMAT_XBGR8888
+encodes with 4 components, like so:
+
+ * DRM_FORMAT_XBGR8888
+
+   * Component 0: R(8)
+   * Component 1: G(8)
+   * Component 2: B(8)
+   * Component 3: X(8)
+
+Please note, however, that the inclusion of a "wasted" 'X' channel is
+bad for compression efficiency, and so it's recommended to avoid
+formats containing 'X' bits. If a fourth component is
+required/expected by the encoder/decoder, then it is recommended to
+instead use an equivalent format with alpha, setting all alpha bits to
+'1'. If there is no requirement for a fourth component, then a format
+which doesn't include alpha can be used, e.g. DRM_FORMAT_BGR888.
+
+Number of Planes
+================
+
+Formats which are typically multi-planar in linear layouts (e.g. YUV
+420), can be encoded into one, or multiple, AFBC planes. As with
+component order, the encoder and decoder must agree about the number
+of planes in order to correctly decode the buffer. The fourcc code is
+used to determine the number of encoded planes in an AFBC buffer,
+matching the number of planes for the linear (unmodified) format.
+Within each plane, the component ordering also follows the fourcc
+code:
+
+For example:
+
+ * DRM_FORMAT_YUYV: nplanes = 1
+
+   * Plane 0:
+
+     * Component 0: Y(8)
+     * Component 1: Cb(8, 2x1 subsampled)
+     * Component 2: Cr(8, 2x1 subsampled)
+
+ * DRM_FORMAT_NV12: nplanes = 2
+
+   * Plane 0:
+
+     * Component 0: Y(8)
+
+   * Plane 1:
+
+     * Component 0: Cb(8, 2x1 subsampled)
+     * Component 1: Cr(8, 2x1 subsampled)
+
+Cross-device interoperability
+=============================
+
+For maximum compatibility across devices, the table below defines
+canonical formats for use between AFBC-enabled devices. Formats which
+are listed here must be used exactly as specified when using the AFBC
+modifiers. Formats which are not listed should be avoided.
+
+.. flat-table:: AFBC formats
+
+   * - Fourcc code
+     - Description
+     - Planes/Components
+
+   * - DRM_FORMAT_ABGR2101010
+     - 10-bit per component RGB, with 2-bit alpha
+     - Plane 0: 4 components
+              * Component 0: R(10)
+              * Component 1: G(10)
+              * Component 2: B(10)
+              * Component 3: A(2)
+
+   * - DRM_FORMAT_ABGR8888
+     - 8-bit per component RGB, with 8-bit alpha
+     - Plane 0: 4 components
+              * Component 0: R(8)
+              * Component 1: G(8)
+              * Component 2: B(8)
+              * Component 3: A(8)
+
+   * - DRM_FORMAT_BGR888
+     - 8-bit per component RGB
+     - Plane 0: 3 components
+              * Component 0: R(8)
+              * Component 1: G(8)
+              * Component 2: B(8)
+
+   * - DRM_FORMAT_BGR565
+     - 5/6-bit per component RGB
+     - Plane 0: 3 components
+              * Component 0: R(5)
+              * Component 1: G(6)
+              * Component 2: B(5)
+
+   * - DRM_FORMAT_ABGR1555
+     - 5-bit per component RGB, with 1-bit alpha
+     - Plane 0: 4 components
+              * Component 0: R(5)
+              * Component 1: G(5)
+              * Component 2: B(5)
+              * Component 3: A(1)
+
+   * - DRM_FORMAT_VUY888
+     - 8-bit per component YCbCr 444, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(8)
+              * Component 1: Cb(8)
+              * Component 2: Cr(8)
+
+   * - DRM_FORMAT_VUY101010
+     - 10-bit per component YCbCr 444, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(10)
+              * Component 1: Cb(10)
+              * Component 2: Cr(10)
+
+   * - DRM_FORMAT_YUYV
+     - 8-bit per component YCbCr 422, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(8)
+              * Component 1: Cb(8, 2x1 subsampled)
+              * Component 2: Cr(8, 2x1 subsampled)
+
+   * - DRM_FORMAT_Y210
+     - 10-bit per component YCbCr 422, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(10)
+              * Component 1: Cb(10, 2x1 subsampled)
+              * Component 2: Cr(10, 2x1 subsampled)
+
+   * - DRM_FORMAT_YUV420_8BIT
+     - 8-bit per component YCbCr 420, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(8)
+              * Component 1: Cb(8, 2x2 subsampled)
+              * Component 2: Cr(8, 2x2 subsampled)
+
+   * - DRM_FORMAT_YUV420_10BIT
+     - 10-bit per component YCbCr 420, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(10)
+              * Component 1: Cb(10, 2x2 subsampled)
+              * Component 2: Cr(10, 2x2 subsampled)
+
+   * - DRM_FORMAT_NV12
+     - 8-bit per component YCbCr 420, two plane
+     - Plane 0: 1 component
+              * Component 0: Y(8)
+       Plane 1: 2 components
+              * Component 0: Cb(8, 2x2 subsampled)
+              * Component 1: Cr(8, 2x2 subsampled)
+
+   * - DRM_FORMAT_P010
+     - 10-bit per component YCbCr 420, two plane
+     - Plane 0: 1 component
+              * Component 0: Y(10)
+       Plane 1: 2 components
+              * Component 0: Cb(10, 2x2 subsampled)
+              * Component 1: Cr(10, 2x2 subsampled)
+
+   * - DRM_FORMAT_YUV420
+     - 8-bit per component YCbCr 420, three plane
+     - Plane 0: 1 component
+              * Component 0: Y(8)
+       Plane 1: 1 component
+              * Component 0: Cb(8, 2x2 subsampled)
+       Plane 2: 1 component
+              * Component 0: Cr(8, 2x2 subsampled)
diff --git a/Documentation/gpu/drivers.rst b/Documentation/gpu/drivers.rst
index d3ab6abae838..304691cb90e8 100644
--- a/Documentation/gpu/drivers.rst
+++ b/Documentation/gpu/drivers.rst
@@ -13,6 +13,7 @@ GPU Driver Documentation
    vc4
    bridge/dw-hdmi
    xen-front
+   afbc
 
 .. only::  subproject and html
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 4af7f6119530..8fb92f162ae1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1036,6 +1036,7 @@ M:	Mali DP Maintainers <malidp@foss.arm.com>
 S:	Supported
 F:	drivers/gpu/drm/arm/
 F:	Documentation/devicetree/bindings/display/arm,malidp.txt
+F:	Documentation/gpu/afbc.rst
 
 ARM MFM AND FLOPPY DRIVERS
 M:	Ian Molton <spyro@f2s.com>
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 41f4610e471c..4ef685a02adc 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -494,6 +494,9 @@ extern "C" {
  * AFBC has several features which may be supported and/or used, which are
  * represented using bits in the modifier. Not all combinations are valid,
  * and different devices or use-cases may support different combinations.
+ *
+ * Further information on the use of AFBC modifiers can be found in
+ * Documentation/gpu/afbc.rst
  */
 #define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode)	fourcc_mod_code(ARM, __afbc_mode)
 
-- 
2.16.1