Hi Daniel,
On Thursday 22 Sep 2016 07:31:24 Daniel Vetter wrote:
> On Wed, Sep 21, 2016 at 3:59 PM, Laurent Pinchart wrote:
> > 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.
It would, but the multiplication is done *after* that block of code, isn't it
?
aligned += pitch_mask;
aligned &= ~pitch_mask;
- return aligned;
+ return aligned * cpp
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
