Re: [PATCH 1/3] drm: drm_fourcc: Add scaling and padding factor to drm_format_info

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Hyun Kwon <hyun.kwon@xilinx.com>
Cc: Michal Simek <monstr@monstr.eu>, David Airlie <airlied@linux.ie>,
	dri-devel@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@intel.com>,
	Satish Kumar Nagireddy <satishna@xilinx.com>,
	Satish Kumar Nagireddy <satish.nagireddy.nagireddy@xilinx.com>,
	Jeffrey Mouroux <jmouroux@xilinx.com>
Subject: Re: [PATCH 1/3] drm: drm_fourcc: Add scaling and padding factor to drm_format_info
Date: Mon, 11 Dec 2017 16:02:08 +0200	[thread overview]
Message-ID: <3497764.a6yH058mm4@avalon> (raw)
In-Reply-To: <1511836053-9489-2-git-send-email-hyun.kwon@xilinx.com>

Hi Hyun,

Thank you for the patch.

On Tuesday, 28 November 2017 04:27:31 EET Hyun Kwon wrote:
> From: Satish Kumar Nagireddy <satish.nagireddy.nagireddy@xilinx.com>
> 
> 'cpp_scale' can be used as a multiplying factor to calculate
> bytes per component based on color format.
> For eg. scaling factor of YUV420 8 bit format is 1
>         so multiplying factor is 1 (8/8)
>         scaling factor of YUV420 10 bit format is 1.25 (10/8)
> 
> 'padding_scale' can be used as a multiplying factor to calculate
> actual width of video according to color format.
> For eg. padding factor of YUV420 8 bit format: 8 bits per 1 component
>         no padding bits here, so multiplying factor is 1
>         padding factor of YUV422 10 bit format: 32 bits per 3 components
>         each component is 10 bit and the factor is 32/30
> 
> Signed-off-by: Satish Kumar Nagireddy <satishna@xilinx.com>
> Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> ---
>  drivers/gpu/drm/drm_fourcc.c | 136 ++++++++++++++++++----------------------
>  include/drm/drm_fourcc.h     |   9 +++
>  2 files changed, 77 insertions(+), 68 deletions(-)

[snip]

> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 6942e84..0202d19 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -36,16 +36,25 @@ 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)
> + * @cpp_scale: { numerator, denominator }. Scaling factor for
> + *	non 8bit aligned formats. For instance, { 10, 8 } can be used for
> + *	10 bit component / pixel formats.

Stupid question, wouldn't it be better to replace cpp with a number of bits 
per pixel then ? Or possibly supplement it if we need both. A scaling factor 
seems difficult to use.

>   * @hsub: Horizontal chroma subsampling factor
>   * @vsub: Vertical chroma subsampling factor
> + * @padding_scale: { numerator, denominator }. Scaling factor for
> + *	padding. This can be used for formats with padding bits after
> + *	multiple pixels / components. For instance, if there are 2 bit
> + *	padding after 3 10bit components, the value should be { 32, 30 }.

Similarly, why don't you instead specify the number of padding bits directly ?

>   */
>  struct drm_format_info {
>  	u32 format;
>  	u8 depth;
>  	u8 num_planes;
>  	u8 cpp[3];
> +	u8 cpp_scale[2];
>  	u8 hsub;
>  	u8 vsub;
> +	u8 padding_scale[2];
>  };
> 
>  /**

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel