Message-ID: <20180112162535.GF10981@intel.com>
Date:   Fri, 12 Jan 2018 18:25:35 +0200
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Ayan Halder <ayan.halder@....com>
Cc:     liviu.dudau@....com, brian.starkey@....com,
        daniel.vetter@...el.com, jani.nikula@...ux.intel.com,
        seanpaul@...omium.org, airlied@...ux.ie,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        maxime.ripard@...e-electrons.com, nd@....com
Subject: Re: [PATCH] drm: add drm_format_alpha_bits

On Fri, Jan 12, 2018 at 04:11:38PM +0000, Ayan Halder wrote:
> On Fri, Jan 12, 2018 at 05:53:33PM +0200, Ville Syrj?l? wrote:
> > On Fri, Jan 12, 2018 at 03:43:49PM +0000, Ayan Halder wrote:
> > > On Fri, Jan 12, 2018 at 04:28:34PM +0200, Ville Syrj?l? wrote:
> > > > On Fri, Jan 12, 2018 at 02:21:16PM +0000, Ayan Halder wrote:
> > > > > drm_format_info does not describe the number of bits used for the alpha
> > > > > channel. That information is useful in a central place like drm_fourcc.c
> > > > > where it can be queried by the drivers that want to determine if 'alpha
> > > > > blending' is to be enabled or not.
> > > > > 
> > > > > Signed-off-by: Ayan Kumar Halder <ayan.halder@....com>
> > > > > Reviewed-by: Liviu Dudau <liviu.dudau@....com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_fourcc.c | 154 ++++++++++++++++++++++++-------------------
> > > > >  include/drm/drm_fourcc.h     |   3 +
> > > > >  2 files changed, 89 insertions(+), 68 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > > > index 9c0152d..073001b 100644
> > > > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > > <snip>
> > > > > @@ -348,3 +348,21 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
> > > > >  	return height / info->vsub;
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_format_plane_height);
> > > > > +
> > > > > +/**
> > > > > + * drm_format_alpha_bits - get the number of bits per pixel
> > > > > + * representing alpha for format
> > > > > + * @format: pixel format (DRM_FORMAT_*)
> > > > > + *
> > > > > + * Returns:
> > > > > + * The number of bits per pixel representing alpha used by the
> > > > > + * specified pixel format.
> > > > > + */
> > > > > +int drm_format_alpha_bits(uint32_t format)
> > > > > +{
> > > > > +	const struct drm_format_info *info;
> > > > > +
> > > > > +	info = drm_format_info(format);
> > > > > +	return info ? info->alpha : 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_format_alpha_bits);
> > > > 
> > > > Do you have an actual use for this function somewhere?
> > > Currently, we do not have a usage for this function.
> > 
> > No point in adding the function then IMO.
> We have helper functions for the other fields so I followed the same
> and added the helper function for the new field ('alpha') too.

Those helpers are mostly there for legacy reasons. Ideally someone would
go over all the code and clean up the cases where using those helpers
makes no sense (ie. when you already have the format structure around)
and then remove the helpers if/when they're no longer needed. This sort
of stuff tends to linger around for a while after bigger mechanical
conversions like the addition of the format info structure.

> > 
> > > We need 'alpha'
> > > field for each entry in 'drm_format_info' so as to determine whether
> > > to enable/disable alpha blending in Mali display processor for the
> > > particular color format. 
> > 
> > I think that's OK. Not sure if it really helps much though since you
> > generally have to configure other things based on the format as well, so
> > I would expect almost everyone will end up with some kind of
> > swicth/table to configure the hardware based on the format. Based on a
> > quick look this doesn't really help i915 at least.
> 
> My understanding was based on the fact that alpha blending could be a
> common feature for all the drivers (I might be wrong). So, if we keep
> the number of 'alpha' bits in 'drm_format_info', it would be useful to
> fetch this info similar to the existing fields.

I'm just saying that ut doesn't really provide much extra help if
the driver already has eg. a switch statement for all the formats
anyway.

> > > > 
> > > > > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > > > > index 6942e84..5513510 100644
> > > > > --- a/include/drm/drm_fourcc.h
> > > > > +++ b/include/drm/drm_fourcc.h
> > > > > @@ -38,6 +38,7 @@ struct drm_mode_fb_cmd2;
> > > > >   * @cpp: Number of bytes per pixel (per plane)
> > > > >   * @hsub: Horizontal chroma subsampling factor
> > > > >   * @vsub: Vertical chroma subsampling factor
> > > > > + * @alpha: Number of bits per pixel representing alpha
> > > > >   */
> > > > >  struct drm_format_info {
> > > > >  	u32 format;
> > > > > @@ -46,6 +47,7 @@ struct drm_format_info {
> > > > >  	u8 cpp[3];
> > > > >  	u8 hsub;
> > > > >  	u8 vsub;
> > > > > +	u8 alpha;
> > > > >  };
> > > > >  
> > > > >  /**
> > > > > @@ -57,6 +59,7 @@ struct drm_format_name_buf {
> > > > >  };
> > > > >  
> > > > >  const struct drm_format_info *__drm_format_info(u32 format);
> > > > > +int drm_format_alpha_bits(u32 format);
> > > > >  const struct drm_format_info *drm_format_info(u32 format);
> > > > >  const struct drm_format_info *
> > > > >  drm_get_format_info(struct drm_device *dev,
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@...ts.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > 
> > > > -- 
> > > > Ville Syrj??l??
> > > > Intel OTC
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@...ts.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Ville Syrj?l?
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC

Powered by blists - more mailing lists