| Message ID | 1473345868-25453-11-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 08, 2016 at 05:44:24PM +0300, Laurent Pinchart wrote: > The driver needs the number of bytes per pixel, not the bpp and depth > info meant for fbdev compatibility. Use the right API. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v3: > > - Renamed bpp to cpp > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 14 +++++++------- > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++- > 2 files changed, 9 insertions(+), 8 deletions(-) > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Michel Dänzer <michel@daenzer.net> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > index bf033b58056c..0727946db189 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > @@ -62,12 +62,12 @@ static struct fb_ops amdgpufb_ops = { > }; > > > -int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, bool tiled) > +int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int cpp, bool tiled) > { > int aligned = width; > int pitch_mask = 0; > > - switch (bpp / 8) { > + switch (cpp) { > case 1: > pitch_mask = 255; > break; > @@ -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. Otherwise looks reasonable. -Daniel > } > > static void amdgpufb_destroy_pinned_object(struct drm_gem_object *gobj) > @@ -111,13 +111,13 @@ static int amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev, > int ret; > int aligned_size, size; > int height = mode_cmd->height; > - u32 bpp, depth; > + u32 cpp; > > - drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp); > + cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0); > > /* need to align pitch with crtc limits */ > - mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp, > - fb_tiled) * ((bpp + 1) / 8); > + mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp, > + fb_tiled); > > height = ALIGN(mode_cmd->height, 8); > size = mode_cmd->pitches[0] * height; > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > index 88fbed2389c0..20a4e569b245 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > @@ -704,7 +704,8 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv, > uint32_t handle; > int r; > > - 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); > args->size = (u64)args->pitch * args->height; > args->size = ALIGN(args->size, PAGE_SIZE); > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, Thank you for the review. On Wednesday 21 Sep 2016 09:51:44 Daniel Vetter wrote: > On Thu, Sep 08, 2016 at 05:44:24PM +0300, Laurent Pinchart wrote: > > The driver needs the number of bytes per pixel, not the bpp and depth > > info meant for fbdev compatibility. Use the right API. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v3: > > > > - Renamed bpp to cpp > > --- > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 14 +++++++------- > > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++- > > 2 files changed, 9 insertions(+), 8 deletions(-) > > > > Cc: Alex Deucher <alexander.deucher@amd.com> > > Cc: Christian König <christian.koenig@amd.com> > > Cc: Michel Dänzer <michel@daenzer.net> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c index bf033b58056c..0727946db189 > > 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > > @@ -62,12 +62,12 @@ static struct fb_ops amdgpufb_ops = { > > > > }; > > > > -int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, > > bool tiled) > > +int amdgpu_align_pitch(struct amdgpu_device *adev, int> width, int cpp, > > bool tiled) > > { > > int aligned = width; > > int pitch_mask = 0; > > > > - switch (bpp / 8) { > > + switch (cpp) { > > case 1: > > pitch_mask = 255; > > break; > > @@ -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. > Otherwise looks reasonable. > -Daniel > > > } > > > > static void amdgpufb_destroy_pinned_object(struct drm_gem_object *gobj) > > @@ -111,13 +111,13 @@ static int amdgpufb_create_pinned_object(struct > > amdgpu_fbdev *rfbdev, > > int ret; > > int aligned_size, size; > > int height = mode_cmd->height; > > - u32 bpp, depth; > > + u32 cpp; > > > > - drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp); > > + cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0); > > > > /* need to align pitch with crtc limits */ > > > > - mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp, > > - fb_tiled) * ((bpp + 1) / 8); > > + mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp, > > + fb_tiled); > > > > height = ALIGN(mode_cmd->height, 8); > > size = mode_cmd->pitches[0] * height; > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index > > 88fbed2389c0..20a4e569b245 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > > @@ -704,7 +704,8 @@ int amdgpu_mode_dumb_create(struct drm_file > > *file_priv, > > uint32_t handle; > > int r; > > > > - 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); > > > > args->size = (u64)args->pitch * args->height; > > args->size = ALIGN(args->size, PAGE_SIZE);
On Wed, Sep 21, 2016 at 2:39 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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). -Daniel
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.
On Wed, Sep 21, 2016 at 3:59 PM, Laurent Pinchart <laurent.pinchart@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
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
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c index bf033b58056c..0727946db189 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c @@ -62,12 +62,12 @@ static struct fb_ops amdgpufb_ops = { }; -int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, bool tiled) +int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int cpp, bool tiled) { int aligned = width; int pitch_mask = 0; - switch (bpp / 8) { + switch (cpp) { case 1: pitch_mask = 255; break; @@ -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; } static void amdgpufb_destroy_pinned_object(struct drm_gem_object *gobj) @@ -111,13 +111,13 @@ static int amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev, int ret; int aligned_size, size; int height = mode_cmd->height; - u32 bpp, depth; + u32 cpp; - drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp); + cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0); /* need to align pitch with crtc limits */ - mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp, - fb_tiled) * ((bpp + 1) / 8); + mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp, + fb_tiled); height = ALIGN(mode_cmd->height, 8); size = mode_cmd->pitches[0] * height; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 88fbed2389c0..20a4e569b245 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -704,7 +704,8 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv, uint32_t handle; int r; - 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); args->size = (u64)args->pitch * args->height; args->size = ALIGN(args->size, PAGE_SIZE);
The driver needs the number of bytes per pixel, not the bpp and depth info meant for fbdev compatibility. Use the right API. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v3: - Renamed bpp to cpp --- drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 14 +++++++------- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++- 2 files changed, 9 insertions(+), 8 deletions(-) Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Christian König <christian.koenig@amd.com> Cc: Michel Dänzer <michel@daenzer.net>