Re: [PATCH v3 02/20] drm: omapdrm: fb: Use format information provided by the DRM core

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

On Tuesday 20 Sep 2016 15:47:57 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > The driver stores in a custom structure named format several pieces of
> > information about the format that are available in the DRM core. Remove
> > them and get the information from the DRM core instead.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> > 
> > @@ -128,13 +122,13 @@ static const struct drm_framebuffer_funcs
> > omap_framebuffer_funcs = {
> >  };
> >  
> >  static uint32_t get_linear_addr(struct plane *plane,
> > -		const struct format *format, int n, int x, int y)
> > +		const struct drm_format_info *format, int n, int x, int y)
> >  {
> >  	uint32_t offset;
> > 
> > -	offset = plane->offset +
> > -			(x * format->planes[n].stride_bpp) +
> > -			(y * plane->pitch / format->planes[n].sub_y);
> > +	offset = plane->offset
> > +	       + (x * format->cpp[n] / (n == 1 ? 1 : format->hsub))
> > +	       + (y * plane->pitch / (n == 1 ? 1 : format->vsub));
> 
> n is the plane number? Shouldn't the above be (n == 0 ? 1 : format->hsub)?

Oops. I'll fix this.

> > @@ -413,28 +410,32 @@ struct drm_framebuffer *omap_framebuffer_init(struct
> > drm_device *dev,
> > 
> >  	fb = &omap_fb->base;
> >  	omap_fb->format = format;
> > +	omap_fb->dss_format = dss_format;
> > 
> >  	mutex_init(&omap_fb->lock);
> > 
> > -	for (i = 0; i < n; i++) {
> > +	for (i = 0; i < format->num_planes; i++) {
> >  		struct plane *plane = &omap_fb->planes[i];
> > -		int size, pitch = mode_cmd->pitches[i];
> > +		unsigned int pitch = mode_cmd->pitches[i];
> > +		unsigned int hsub = i == 0 ? 1 : format->hsub;
> > +		unsigned int vsub = i == 0 ? 1 : format->vsub;
> 
> This makes me wonder... Will all drivers do something like the above?
> It's a bit laborious way to get the pixel subsampling factor, and I
> presume something like above is quite common so that all the
> calculations can be more generic (and not specific to a UV plane).

Helper functions could be useful. That would require auditing drivers to 
locate common code patterns, and I'm afraid I don't have time to do so at the 
moment. Patches are of course welcome ;-)

Some of this code can also be removed as the DRM core performs checks in 
framebuffer_check() too. I'll add a patch to fix that.

-- 
Regards,

Laurent Pinchart

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