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
