Re: [PATCH v2 06/18] drm: Add drm_format_plane_width() and drm_format_plane_height()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jan 25, 2016 at 06:08:23PM +0100, Daniel Vetter wrote:
> On Wed, Jan 20, 2016 at 09:05:27PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a few helpers to get the dimensions of the chroma plane(s).
> > 
> > v2: Add kernel-doc (Daniel)
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  include/drm/drm_crtc.h | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index c65a212db77e..91195c403422 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -2482,6 +2482,36 @@ extern int drm_format_num_planes(uint32_t format);
> >  extern int drm_format_plane_cpp(uint32_t format, int plane);
> >  extern int drm_format_horz_chroma_subsampling(uint32_t format);
> >  extern int drm_format_vert_chroma_subsampling(uint32_t format);
> > +/**
> > + * drm_format_plane_width - width of the plane given the first plane
> > + * @width: width of the first plane
> > + * @format: pixel format
> > + * @plane: plane index
> > + *
> 
> kerneldoc style is
> 
> + * Returns:
> + * blabla

Hmm. I guess I should read some kind of manual of style or whatever.

> 
> > + * Returns the width of @plane, given that the width of the first plane
> > + * is @width.
> > + */
> > +static inline int drm_format_plane_width(int width, uint32_t format, int plane)
> > +{
> 
> For consistency with other helpers I think we should put an
> 
> 	if (plane >= drm_format_num_planes(format))
> 		return 0;
> 
> here. Also I think static inline is overkill for these, and grouping them
> together with the others in drm_crtc.c will make it easier to extract them
> into a new file (since drm_crtc.c is kinda sprawling a bit).

Well with the static inline I was aiming for the compiler to eliminate
the function calls entirely for the typical plane==0 constant case. But
if we add the num_planes check, then it's pretty much going to do that
function call anyway.

> 
> With those nitpicks applied: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > +	if (plane == 0)
> > +		return width;
> > +	return width / drm_format_horz_chroma_subsampling(format);
> > +}
> > +/**
> > + * drm_format_plane_height - height of the plane given the first plane
> > + * @height: height of the first plane
> > + * @format: pixel format
> > + * @plane: plane index
> > + *
> > + * Returns the height of @plane, given that the height of the first plane
> > + * is @height.
> > + */
> > +static inline int drm_format_plane_height(int height, uint32_t format, int plane)
> > +{
> > +	if (plane == 0)
> > +		return height;
> > +	return height / drm_format_vert_chroma_subsampling(format);
> > +}
> >  extern const char *drm_get_format_name(uint32_t format);
> >  extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
> >  							      unsigned int supported_rotations);
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx