Re: [PATCH RFC v3 3/6] drm: fourcc: Add drm_format_plane_width_bytes()

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 09, 2018 at 05:35:53PM -0800, Hyun Kwon wrote:
> drm_format_plane_width_bytes() calculates and returns the number of bytes
> for given width of specified format. The calculation uses @cpp
> in drm format info for byte-aligned formats. If the format isn't
> byte-aligned, @cpp should 0, and the macro pixel information is used.
> This avoids bit level rounding.
> 
> Use this drm_fb_cma_get_gem_addr() for offset calculation.
> 
> Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>

We also need to adjust the validation in framebuffer_check, that's using
raw cpp. I think that's all there is for the core code, but please also
grep for cpp anywhere in core code and make sure we have them all. You
probably want to use the newly introduced helper function in all these
core places (like you do for cma helpers already).

> ---
> v3
> - Update according to member changes
> - Use @cpp for byte-aligned formats, and macro-pixel for non byte-aligned ones
> - Squash a change in drm_fb_cma_helper.c into this
> v2
> - This function is added
> ---
> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c |  3 ++-
>  drivers/gpu/drm/drm_fourcc.c        | 35 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_fourcc.h            |  2 ++
>  3 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 186d00a..271175e 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -124,7 +124,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
>  		return 0;
>  
>  	paddr = obj->paddr + fb->offsets[plane];
> -	paddr += fb->format->cpp[plane] * (state->src_x >> 16);
> +	paddr += drm_format_plane_width_bytes(fb->format, plane,
> +					      state->src_x >> 16);
>  	paddr += fb->pitches[plane] * (state->src_y >> 16);
>  
>  	return paddr;
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 9c0152d..ed95de2 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -348,3 +348,38 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
>  	return height / info->vsub;
>  }
>  EXPORT_SYMBOL(drm_format_plane_height);
> +
> +/**
> + * drm_format_plane_width_bytes - bytes of the given width of the plane
> + * @info: DRM format information
> + * @plane: plane index
> + * @width: width to get the number of bytes
> + *
> + * This returns the number of bytes for given @width and @plane.
> + * The @cpp or macro pixel information should be valid.
> + *
> + * Returns:
> + * The bytes of @width of @plane. 0 for invalid format info.
> + */
> +int drm_format_plane_width_bytes(const struct drm_format_info *info,
> +				 int plane, int width)

We need to extend the kernel code for drm_format_plane_cpp to explain that
it can't handle formats with macropixels (for obvious reasons) and that
generic code must use drm_format_plane_width_bytes instead of cpp. Drivers
can keep using drm_format_plane_cpp as long as they don't support a 4CC
code with macro pixels.

> +{
> +	if (!info || plane >= info->num_planes)
> +		return 0;
> +
> +	if (info->cpp[plane])

I think a WARN_ON(bytes_per_macropixel || pixels_per_macropixel); here
would be good, to make sure only one or the other is set (to force
consistency and avoid silly bugs).

With the above details addressed I think this patch looks good. I'll
triple-check the new version for any missing cpp conversions in the drm
core once more though.
-Daniel

> +		return info->cpp[plane] * width;
> +
> +	if (WARN_ON(!info->bytes_per_macropixel[plane] ||
> +		    !info->pixels_per_macropixel[plane])) {
> +		struct drm_format_name_buf buf;
> +
> +		DRM_WARN("Either cpp or macro-pixel info should be valid: %s\n",
> +			 drm_get_format_name(info->format, &buf));
> +		return 0;
> +	}
> +
> +	return DIV_ROUND_UP(width * info->bytes_per_macropixel[plane],
> +			    info->pixels_per_macropixel[plane]);
> +}
> +EXPORT_SYMBOL(drm_format_plane_width_bytes);
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index ce59329..8158290 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -119,6 +119,8 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
>  int drm_format_vert_chroma_subsampling(uint32_t format);
>  int drm_format_plane_width(int width, uint32_t format, int plane);
>  int drm_format_plane_height(int height, uint32_t format, int plane);
> +int drm_format_plane_width_bytes(const struct drm_format_info *info,
> +				 int plane, int width);
>  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>  
>  #endif /* __DRM_FOURCC_H__ */
> -- 
> 2.7.4
> 

-- 
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