Re: [PATCH v4 10/14] drm: amdgpu: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

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

Hi Daniel,

On Wednesday 21 Sep 2016 14:46:07 Daniel Vetter wrote:
> On Wed, Sep 21, 2016 at 2:39 PM, Laurent Pinchart wrote:
> >> > @@ -82,7 +82,7 @@ int amdgpu_align_pitch(struct amdgpu_device *adev,
> >> > int width, int bpp, bool tile
> >> > 
> >> >     aligned += pitch_mask;
> >> >     aligned &= ~pitch_mask;
> >> > 
> >> > -   return aligned;
> >> > +   return aligned * cpp;
> >> 
> >> Now you multiply by cpp after the rounding.
> > 
> > That's right, but I don't think that's a problem, as all bpp values
> > returned by drm_fb_get_bpp_depth() are multiple of 8 bits.
> 
> Before we have ALIGN(width * cpp, pitch_mask + 1). With your patch we
> have ALIGN(width, pitch_mask + 1) * cpp. In short we overalign, and
> instead of e.g. aligning to 256bytes we now align to 256*4 (for
> xrgb8888).

The current code is

	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp,
						  fb_tiled) * ((bpp + 1) / 8);

As bpp is a multiple of 8, this is equivalent to

	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp,
						  fb_tiled) * (bpp / 8);

The patch replaces the code with

	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp,
						  fb_tiled);

with cpp = bpp / 8, and amdgpu_align_pitch() now returns

-	return aligned;
+	return aligned * cpp;

So the patch just moves * (bpp / 8) inside the amdgpu_align_pitch() function.

The other code path is changed as follows:

-	args->pitch = amdgpu_align_pitch(adev, args->width, args->bpp, 0) * 
((args->bpp + 1) / 8);
+	args->pitch = amdgpu_align_pitch(adev, args->width,
+					 DIV_ROUND_UP(args->bpp, 8), 0);

DIV_ROUND_UP(args->bpp, 8) is equal to ((args->bpp + 1) / 8) for all supported 
bpp values, so this also moves the multiplication inside the function, without 
changing the result as far as I can see.

Note that amdgpu_align_width() is also changed as follows

-       switch (bpp / 8) {
+       switch (cpp) {
	case 1:
		pitch_mask = 255;
		break;
	case 2:
		pitch_mask = 127;
		break;
	case 3:
	case 4:
		pitch_mask = 63;
		break;
	}

This will change the pitch mask if the bpp value isn't a multiple of 8. 
However, all the formats we support use multiples of 8 as bpp values, so I 
don't see a problem here.

-- 
Regards,

Laurent Pinchart

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