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

[Hyun Kwon]

Hi Laurent,

Thanks for the comment.

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart at ideasonboard.com]
> Sent: Monday, December 11, 2017 6:02 AM
> To: Hyun Kwon <hyunk at xilinx.com>
> Cc: Daniel Vetter <daniel.vetter at intel.com>; Jani Nikula
> <jani.nikula at linux.intel.com>; Sean Paul <seanpaul at chromium.org>; David
> Airlie <airlied at linux.ie>; dri-devel at lists.freedesktop.org;
> monstr at monstr.eu; Jeff Mouroux <jmouroux at xilinx.com>; Satish Kumar
> Nagireddy <SATISHNA at xilinx.com>; Satish Kumar Nagireddy
> <SATISHNA at xilinx.com>
> Subject: Re: [PATCH 1/3] drm: drm_fourcc: Add scaling and padding factor
> to drm_format_info
> 
> 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 at 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 at xilinx.com>
> > Signed-off-by: Hyun Kwon <hyun.kwon at 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.

Bits per pixel would work. I was hesitant to modify other drivers, but I can still make it that way.

> 
> >   * @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 ?

I don't see how to model this with the number of padding bits. There can be arbitrary number of padding bits in every arbitrary number of components with arbitrary bpp. Just for example, 2 bits padding after 3 - 10bit components or 8 bit padding after 4 - 14bit components.

The currently expected usage is:

	bytes_per_line = width * info->cpp[i] * info->cpp_scale[0] / info->cpp_scale[1] * info->padding_scale[0] / info->padding_scale[1];

cpp_scale[] can be replaced with bpp / 8 per your suggestion. Let me know if you see any better way.

Thanks,
-hyun

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