Re: [PATCH v3 09/15] drm: tilcdc: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

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

Hi Tomi,

On Friday 10 Jun 2016 14:51:45 Tomi Valkeinen wrote:
> On 09/06/16 02:32, Laurent Pinchart wrote:
> > The driver needs the number of bytes per pixel, not the bpp and depth
> > info meant for fbdev compatibility. Use the right API.
> > 
> > In the tilcdc_crtc_mode_set() function compute the hardware register
> > value directly from the pixel format instead of computing the number of
> > bits per pixels first.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 23 ++++++++++-------------
> >  1 file changed, 10 insertions(+), 13 deletions(-)
> > 
> > Cc: Jyri Sarha <jsarha@ti.com>
> > 
> > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 79027b1c64d3..d63c7363dabc
> > 100644
> > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > @@ -65,15 +65,13 @@ static void set_scanout(struct drm_crtc *crtc, struct
> > drm_framebuffer *fb)> 
> >  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_gem_cma_object *gem;
> > -	unsigned int depth, bpp;
> >  	dma_addr_t start, end;
> > 
> > -	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> >  	gem = drm_fb_cma_get_gem_obj(fb, 0);
> >  	start = gem->paddr + fb->offsets[0] +
> >  		crtc->y * fb->pitches[0] +
> > -		crtc->x * bpp / 8;
> > +		crtc->x * drm_format_plane_cpp(fb->pixel_format, 0);
> > 
> >  	end = start + (crtc->mode.vdisplay * fb->pitches[0]);
> > 
> > @@ -132,11 +130,12 @@ static void tilcdc_crtc_destroy(struct drm_crtc
> > *crtc)
> >  static int tilcdc_verify_fb(struct drm_crtc *crtc, struct drm_framebuffer
> >  *fb) {
> >  	struct drm_device *dev = crtc->dev;
> > -	unsigned int depth, bpp;
> > +	unsigned int min_pitch;
> > 
> > -	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> > +	min_pitch = crtc->mode.hdisplay
> > +		  * drm_format_plane_cpp(fb->pixel_format, 0);
> 
> Why 'min_pitch'? Plain 'pitch' should be fine here.

You're right, the hardware doesn't seem to support configurable strides, I'll 
rename this.

> > -	if (fb->pitches[0] != crtc->mode.hdisplay * bpp / 8) {
> > +	if (fb->pitches[0] != min_pitch) {
> >  		dev_err(dev->dev,
> >  			"Invalid pitch: fb and crtc widths must be the same");
> >  		return -EINVAL;
> > @@ -401,16 +400,14 @@ static int tilcdc_crtc_mode_set(struct drm_crtc
> > *crtc,
> >  	if (info->tft_alt_mode)
> >  		reg |= LCDC_TFT_ALT_ENABLE;
> >  	
> >  	if (priv->rev == 2) {
> > -		unsigned int depth, bpp;
> > -
> > -		drm_fb_get_bpp_depth(crtc->primary->fb->pixel_format, &depth, &bpp);
> > -		switch (bpp) {
> > -		case 16:
> > +		switch (crtc->primary->fb->pixel_format) {
> > +		case DRM_FORMAT_RGB565:
> >  			break;
> > -		case 32:
> > +		case DRM_FORMAT_XRGB8888:
> > +		case DRM_FORMAT_ARGB8888:
>
> I'm not sure what's the common way, but tilcdc doesn't support alpha.
> ARGB works, of course, by ignoring A, but... If an userspace app creates
> ARGB buffer, does the app expect alpha to work?

The driver currently accepts DRM_FORMAT_ARGB8888 and ignores the alpha 
component. I didn't want to risk breaking userspace by removing that, so I 
decided to take a conservative approach. If you think we can safely drop 
DRM_FORMAT_ARGB8888 then we can do so in a separate patch.

-- 
Regards,

Laurent Pinchart

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