On Wed, Sep 21, 2016 at 3:59 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
