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

[Laurent Pinchart]

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