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

[Daniel Vetter]

On Wed, Sep 21, 2016 at 3:59 PM, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
> 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.

All correct. The trouble is that you move the * cpp over the following code:

aligned += pitch_mask;
aligned &= ~pitch_mask;

Multiplying by 4 (for xrgb8888) before or after aligning to pitch_mask
does make a difference.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch