Re: [PATCH] Only align screen / scanout pixmap height where necessary

[Michel Dänzer]

On 02.10.2015 23:46, Alex Deucher wrote:
> On Fri, Oct 2, 2015 at 5:11 AM, Michel Dänzer  wrote:
>> From: Michel Dänzer 
>>
>> When using glamor acceleration, the pixmap's header has to have a height
>> that matches exactly what the actual height is minus the GPU memory
>> alignment. Otherwise CRTCs scanning out from the main scanout buffer
>> (e.g. every CRTC that isn't rotated or transformed in some way) won't
>> always work. This results in a bug where rotating one monitor in a
>> multi-monitor setup won't always work properly. Easiest way to reproduce
>> this:
>>
>> - Have two monitors (I've gotten this working with a 1920x1080 and
>>   1280x1024, along with two 1920x1080s)
>> - Rotate one of them from 0° to 90°, then rotate the same monitor from
>>   90° to 180°. The monitor that hasn't been rotated won't properly
>>   update, and will stay on a blank screen
>>
>> This doesn't seem to make any difference when using EXA for
>> acceleration.
>>
>> Compared to Stephen Chandler's patch, this drops the height alignment
>> in most places and only keeps it where it's really necessary.
>>
>> Reported-by: Stephen Chandler Paul 
>> Signed-off-by: Michel Dänzer 
> 
> Reviewed-by: Alex Deucher 

Thanks, pushed.


> Anyone want to port this to amdgpu?

I took a look at that, but the amdgpu driver doesn't have the spurious
alignment in the first place.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Only align screen / scanout pixmap height where necessary

[Stephen Chandler Paul]

Yep, patch works fine here in both EXA and glamor. Tested with a Radeon
7750 and 6850.

Cheers,
Lyude

On Fri, 2015-10-02 at 18:11 +0900, Michel Dänzer wrote:
> From: Michel Dänzer 
> 
> When using glamor acceleration, the pixmap's header has to have a
> height
> that matches exactly what the actual height is minus the GPU memory
> alignment. Otherwise CRTCs scanning out from the main scanout buffer
> (e.g. every CRTC that isn't rotated or transformed in some way) won't
> always work. This results in a bug where rotating one monitor in a
> multi-monitor setup won't always work properly. Easiest way to
> reproduce
> this:
> 
> - Have two monitors (I've gotten this working with a 1920x1080 and
>   1280x1024, along with two 1920x1080s)
> - Rotate one of them from 0° to 90°, then rotate the same monitor
> from
>   90° to 180°. The monitor that hasn't been rotated won't properly
>   update, and will stay on a blank screen
> 
> This doesn't seem to make any difference when using EXA for
> acceleration.
> 
> Compared to Stephen Chandler's patch, this drops the height alignment
> in most places and only keeps it where it's really necessary.
> 
> Reported-by: Stephen Chandler Paul 
> Signed-off-by: Michel Dänzer 
> ---
> 
> Stephen Chandler,
> 
> thanks for the good catch, but I think we can take this even further.
> Does this fix the problem for you as well?
> 
>  src/drmmode_display.c | 18 --
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index efc35f0..623124e 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -525,6 +525,7 @@ drmmode_crtc_scanout_allocate(xf86CrtcPtr crtc,
>   RADEONInfoPtr info = RADEONPTR(pScrn);
>   drmmode_crtc_private_ptr drmmode_crtc = crtc
> ->driver_private;
>   drmmode_ptr drmmode = drmmode_crtc->drmmode;
> + int aligned_height;
>   int size;
>   int ret;
>   unsigned long rotate_pitch;
> @@ -547,9 +548,9 @@ drmmode_crtc_scanout_allocate(xf86CrtcPtr crtc,
>   rotate_pitch =
>   RADEON_ALIGN(width, drmmode_get_pitch_align(pScrn,
> drmmode->cpp, 0))
>   * drmmode->cpp;
> - height = RADEON_ALIGN(height,
> drmmode_get_height_align(pScrn, 0));
> + aligned_height = RADEON_ALIGN(height,
> drmmode_get_height_align(pScrn, 0));
>   base_align = drmmode_get_base_align(pScrn, drmmode->cpp, 0);
> - size = RADEON_ALIGN(rotate_pitch * height,
> RADEON_GPU_PAGE_SIZE);
> + size = RADEON_ALIGN(rotate_pitch * aligned_height,
> RADEON_GPU_PAGE_SIZE);
>  
>   scanout->bo = radeon_bo_open(drmmode->bufmgr, 0, size,
> base_align,
>RADEON_GEM_DOMAIN_VRAM, 0);
> @@ -633,7 +634,6 @@ drmmode_set_mode_major(xf86CrtcPtr crtc,
> DisplayModePtr mode,
>   drmModeModeInfo kmode;
>   int pitch;
>   uint32_t tiling_flags = 0;
> - int height;
>  
>   if (info->allowColorTiling) {
>   if (info->ChipFamily >= CHIP_FAMILY_R600)
> @@ -644,14 +644,13 @@ drmmode_set_mode_major(xf86CrtcPtr crtc,
> DisplayModePtr mode,
>  
>   pitch = RADEON_ALIGN(pScrn->displayWidth,
> drmmode_get_pitch_align(pScrn, info->pixel_bytes, tiling_flags)) *
>   info->pixel_bytes;
> - height = RADEON_ALIGN(pScrn->virtualY,
> drmmode_get_height_align(pScrn, tiling_flags));
>   if (info->ChipFamily >= CHIP_FAMILY_R600) {
>   pitch = info->front_surface.level[0].pitch_bytes;
>   }
>  
>   if (drmmode->fb_id == 0) {
>   ret = drmModeAddFB(drmmode->fd,
> -pScrn->virtualX, height,
> +pScrn->virtualX, pScrn->virtualY,
> pScrn->depth, pScrn
> ->bitsPerPixel,
>  pitch,
>  info->front_bo->handle,
> @@ -1789,6 +1788,7 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int
> width, int height)
>   ScreenPtr   screen = xf86ScrnToScreen(scrn);
>   uint32_told_fb_id;
>   int i, pitch, old_width, old_height, old_pitch;
> + int aligned_height;
>   uint32_t screen_size;
>   int cpp = info->pixel_bytes;
>   struct radeon_bo *front_bo;
> @@ -1822,8 +1822,8 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int
> width, int height)
>   }
>  
>   pitch = RADEON_ALIGN(width, drmmode_get_pitch_align(scrn,
> cpp, tiling_flags)) * cpp;
> - height = RADEON_ALIGN(height, drmmode_get_height_align(scrn,
> tiling_flags));
> - screen_size = RADEON_ALIGN(pitch * height,
> RADEON_GPU_PAGE_SIZE);
> + aligned_height = RADEON_ALIGN(height,
> drmmode_get_height_align(scrn, tiling_flags));
> + screen_size = RADEON_ALIGN(pitch * aligned_height,
> RADEON_GPU_PAGE_SIZE);
>   base_align = 4096;
>   if (info->ChipFamily >= CHIP_FAMILY_R600) {
>   memset(, 0, sizeof(struct 

Re: [PATCH] Only align screen / scanout pixmap height where necessary

[Alex Deucher]

On Fri, Oct 2, 2015 at 5:11 AM, Michel Dänzer  wrote:
> From: Michel Dänzer 
>
> When using glamor acceleration, the pixmap's header has to have a height
> that matches exactly what the actual height is minus the GPU memory
> alignment. Otherwise CRTCs scanning out from the main scanout buffer
> (e.g. every CRTC that isn't rotated or transformed in some way) won't
> always work. This results in a bug where rotating one monitor in a
> multi-monitor setup won't always work properly. Easiest way to reproduce
> this:
>
> - Have two monitors (I've gotten this working with a 1920x1080 and
>   1280x1024, along with two 1920x1080s)
> - Rotate one of them from 0° to 90°, then rotate the same monitor from
>   90° to 180°. The monitor that hasn't been rotated won't properly
>   update, and will stay on a blank screen
>
> This doesn't seem to make any difference when using EXA for
> acceleration.
>
> Compared to Stephen Chandler's patch, this drops the height alignment
> in most places and only keeps it where it's really necessary.
>
> Reported-by: Stephen Chandler Paul 
> Signed-off-by: Michel Dänzer 

Reviewed-by: Alex Deucher 

Anyone want to port this to amdgpu?

Alex

> ---
>
> Stephen Chandler,
>
> thanks for the good catch, but I think we can take this even further.
> Does this fix the problem for you as well?
>
>  src/drmmode_display.c | 18 --
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index efc35f0..623124e 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -525,6 +525,7 @@ drmmode_crtc_scanout_allocate(xf86CrtcPtr crtc,
> RADEONInfoPtr info = RADEONPTR(pScrn);
> drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> drmmode_ptr drmmode = drmmode_crtc->drmmode;
> +   int aligned_height;
> int size;
> int ret;
> unsigned long rotate_pitch;
> @@ -547,9 +548,9 @@ drmmode_crtc_scanout_allocate(xf86CrtcPtr crtc,
> rotate_pitch =
> RADEON_ALIGN(width, drmmode_get_pitch_align(pScrn, 
> drmmode->cpp, 0))
> * drmmode->cpp;
> -   height = RADEON_ALIGN(height, drmmode_get_height_align(pScrn, 0));
> +   aligned_height = RADEON_ALIGN(height, drmmode_get_height_align(pScrn, 
> 0));
> base_align = drmmode_get_base_align(pScrn, drmmode->cpp, 0);
> -   size = RADEON_ALIGN(rotate_pitch * height, RADEON_GPU_PAGE_SIZE);
> +   size = RADEON_ALIGN(rotate_pitch * aligned_height, 
> RADEON_GPU_PAGE_SIZE);
>
> scanout->bo = radeon_bo_open(drmmode->bufmgr, 0, size, base_align,
>  RADEON_GEM_DOMAIN_VRAM, 0);
> @@ -633,7 +634,6 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr 
> mode,
> drmModeModeInfo kmode;
> int pitch;
> uint32_t tiling_flags = 0;
> -   int height;
>
> if (info->allowColorTiling) {
> if (info->ChipFamily >= CHIP_FAMILY_R600)
> @@ -644,14 +644,13 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr 
> mode,
>
> pitch = RADEON_ALIGN(pScrn->displayWidth, 
> drmmode_get_pitch_align(pScrn, info->pixel_bytes, tiling_flags)) *
> info->pixel_bytes;
> -   height = RADEON_ALIGN(pScrn->virtualY, 
> drmmode_get_height_align(pScrn, tiling_flags));
> if (info->ChipFamily >= CHIP_FAMILY_R600) {
> pitch = info->front_surface.level[0].pitch_bytes;
> }
>
> if (drmmode->fb_id == 0) {
> ret = drmModeAddFB(drmmode->fd,
> -  pScrn->virtualX, height,
> +  pScrn->virtualX, pScrn->virtualY,
> pScrn->depth, pScrn->bitsPerPixel,
>pitch,
>info->front_bo->handle,
> @@ -1789,6 +1788,7 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int width, 
> int height)
> ScreenPtr   screen = xf86ScrnToScreen(scrn);
> uint32_told_fb_id;
> int i, pitch, old_width, old_height, old_pitch;
> +   int aligned_height;
> uint32_t screen_size;
> int cpp = info->pixel_bytes;
> struct radeon_bo *front_bo;
> @@ -1822,8 +1822,8 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int width, 
> int height)
> }
>
> pitch = RADEON_ALIGN(width, drmmode_get_pitch_align(scrn, cpp, 
> tiling_flags)) * cpp;
> -   height = RADEON_ALIGN(height, drmmode_get_height_align(scrn, 
> tiling_flags));
> -   screen_size = RADEON_ALIGN(pitch * height, RADEON_GPU_PAGE_SIZE);
> +   aligned_height = RADEON_ALIGN(height, drmmode_get_height_align(scrn, 
> tiling_flags));
> +   screen_size = RADEON_ALIGN(pitch * aligned_height, 
> RADEON_GPU_PAGE_SIZE);
> base_align = 4096;
> if