| Message ID | 20190604154201.14460-7-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] drm/ast: Unpin cursor BO during cleanup | expand |
On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote: > The cursor handling in mgag200 is complicated to understand. It touches a > number of different BOs, but doesn't really use all of them. > > Rewriting the cursor update reduces the amount of cursor state. There are > two BOs for double-buffered HW updates. The source BO updates the one that > is currently not displayed and then switches buffers. Explicit BO locking > has been removed from the code. BOs are simply pinned and unpinned in video > RAM. Cursors are not that big after all, so maybe pin the two BOs for double-buffering permanently in vram to simplify things further? Also factoring out the code which updates the two BOs to a separate function should help making the code more readable. But even as-is the patch is a step into the right direction. Acked-by: Gerd Hoffmann <kraxel@redhat.com> cheers, Gerd
Hi Am 05.06.19 um 11:58 schrieb Gerd Hoffmann: > On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote: >> The cursor handling in mgag200 is complicated to understand. It touches a >> number of different BOs, but doesn't really use all of them. >> >> Rewriting the cursor update reduces the amount of cursor state. There are >> two BOs for double-buffered HW updates. The source BO updates the one that >> is currently not displayed and then switches buffers. Explicit BO locking >> has been removed from the code. BOs are simply pinned and unpinned in video >> RAM. > > Cursors are not that big after all, so maybe pin the two BOs for > double-buffering permanently in vram to simplify things further? > > Also factoring out the code which updates the two BOs to a separate > function should help making the code more readable. The cursor handling in the ast driver is similar, but uses one single BO to hold both cursor buffers. I'm thinking about how to turn both implementations into a generic helper for legacy cursors (i.e., low number of colors or palette). This would also be helpful for my work on fbdev porting. One idea is to adapt deferred I/O. DRM would expose an ARGB shadow buffer to userspace and let the mmap implementation update the HW buffers (including dithering, palette setup, etc.). Best regards Thomas > But even as-is the patch is a step into the right direction. > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > cheers, > Gerd >
On Tue, Jun 11, 2019 at 2:32 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 05.06.19 um 11:58 schrieb Gerd Hoffmann: > > On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote: > >> The cursor handling in mgag200 is complicated to understand. It touches a > >> number of different BOs, but doesn't really use all of them. > >> > >> Rewriting the cursor update reduces the amount of cursor state. There are > >> two BOs for double-buffered HW updates. The source BO updates the one that > >> is currently not displayed and then switches buffers. Explicit BO locking > >> has been removed from the code. BOs are simply pinned and unpinned in video > >> RAM. > > > > Cursors are not that big after all, so maybe pin the two BOs for > > double-buffering permanently in vram to simplify things further? > > > > Also factoring out the code which updates the two BOs to a separate > > function should help making the code more readable. > > The cursor handling in the ast driver is similar, but uses one single BO > to hold both cursor buffers. I'm thinking about how to turn both > implementations into a generic helper for legacy cursors (i.e., low > number of colors or palette). This would also be helpful for my work on > fbdev porting. > > One idea is to adapt deferred I/O. DRM would expose an ARGB shadow > buffer to userspace and let the mmap implementation update the HW > buffers (including dithering, palette setup, etc.). No mmap games needed with kms, we expect userspace to give us an ioctl call in all cases. fbdev is the legacy horrors that works differently. So for cursors, assuming you have universal cursors, you just need to update the shadowed copy in the prepare_fb plane hook. For non-universal plane drivers you need to do that somewhere in your set/move_cursor hooks (I think both of them). Aside: For non-cursors there's also the dirtyfb ioctl, which serves the same purpose. Cheers, Daniel > > Best regards > Thomas > > > But even as-is the patch is a step into the right direction. > > > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > > > cheers, > > Gerd > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah > HRB 21284 (AG Nürnberg) >
Hi Am 11.06.19 um 17:33 schrieb Daniel Vetter: > On Tue, Jun 11, 2019 at 2:32 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >> Am 05.06.19 um 11:58 schrieb Gerd Hoffmann: >>> On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote: >>>> The cursor handling in mgag200 is complicated to understand. It touches a >>>> number of different BOs, but doesn't really use all of them. >>>> >>>> Rewriting the cursor update reduces the amount of cursor state. There are >>>> two BOs for double-buffered HW updates. The source BO updates the one that >>>> is currently not displayed and then switches buffers. Explicit BO locking >>>> has been removed from the code. BOs are simply pinned and unpinned in video >>>> RAM. >>> >>> Cursors are not that big after all, so maybe pin the two BOs for >>> double-buffering permanently in vram to simplify things further? >>> >>> Also factoring out the code which updates the two BOs to a separate >>> function should help making the code more readable. >> >> The cursor handling in the ast driver is similar, but uses one single BO >> to hold both cursor buffers. I'm thinking about how to turn both >> implementations into a generic helper for legacy cursors (i.e., low >> number of colors or palette). This would also be helpful for my work on >> fbdev porting. >> >> One idea is to adapt deferred I/O. DRM would expose an ARGB shadow >> buffer to userspace and let the mmap implementation update the HW >> buffers (including dithering, palette setup, etc.). > > No mmap games needed with kms, we expect userspace to give us an ioctl > call in all cases. fbdev is the legacy horrors that works differently. Thanks for clarifying this. Conversion should be much easier this way. I saw the dirty callback and the DIRTYFB ioctl, but I don't saw anything in Weston that calls it. So I assumed that it's obsolete or optional. Best regards Thomas > So for cursors, assuming you have universal cursors, you just need to > update the shadowed copy in the prepare_fb plane hook. For > non-universal plane drivers you need to do that somewhere in your > set/move_cursor hooks (I think both of them). Aside: For non-cursors > there's also the dirtyfb ioctl, which serves the same purpose. > > Cheers, Daniel > >> >> Best regards >> Thomas >> >>> But even as-is the patch is a step into the right direction. >>> >>> Acked-by: Gerd Hoffmann <kraxel@redhat.com> >>> >>> cheers, >>> Gerd >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany >> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah >> HRB 21284 (AG Nürnberg) >> > >
On Wed, Jun 12, 2019 at 09:27:21AM +0200, Thomas Zimmermann wrote: > Hi > > Am 11.06.19 um 17:33 schrieb Daniel Vetter: > > On Tue, Jun 11, 2019 at 2:32 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> > >> Hi > >> > >> Am 05.06.19 um 11:58 schrieb Gerd Hoffmann: > >>> On Tue, Jun 04, 2019 at 05:41:59PM +0200, Thomas Zimmermann wrote: > >>>> The cursor handling in mgag200 is complicated to understand. It touches a > >>>> number of different BOs, but doesn't really use all of them. > >>>> > >>>> Rewriting the cursor update reduces the amount of cursor state. There are > >>>> two BOs for double-buffered HW updates. The source BO updates the one that > >>>> is currently not displayed and then switches buffers. Explicit BO locking > >>>> has been removed from the code. BOs are simply pinned and unpinned in video > >>>> RAM. > >>> > >>> Cursors are not that big after all, so maybe pin the two BOs for > >>> double-buffering permanently in vram to simplify things further? > >>> > >>> Also factoring out the code which updates the two BOs to a separate > >>> function should help making the code more readable. > >> > >> The cursor handling in the ast driver is similar, but uses one single BO > >> to hold both cursor buffers. I'm thinking about how to turn both > >> implementations into a generic helper for legacy cursors (i.e., low > >> number of colors or palette). This would also be helpful for my work on > >> fbdev porting. > >> > >> One idea is to adapt deferred I/O. DRM would expose an ARGB shadow > >> buffer to userspace and let the mmap implementation update the HW > >> buffers (including dithering, palette setup, etc.). > > > > No mmap games needed with kms, we expect userspace to give us an ioctl > > call in all cases. fbdev is the legacy horrors that works differently. > > Thanks for clarifying this. Conversion should be much easier this way. I > saw the dirty callback and the DIRTYFB ioctl, but I don't saw anything > in Weston that calls it. So I assumed that it's obsolete or optional. It's not optional nor obsolete, but you only need to call dirtyfb if you do frontbuffer rendering. Which weston doesnt do. As long as you pageflip, the pageflip will make sure the buffer contents gets updated. -Daniel > > Best regards > Thomas > > > So for cursors, assuming you have universal cursors, you just need to > > update the shadowed copy in the prepare_fb plane hook. For > > non-universal plane drivers you need to do that somewhere in your > > set/move_cursor hooks (I think both of them). Aside: For non-cursors > > there's also the dirtyfb ioctl, which serves the same purpose. > > > > Cheers, Daniel > > > >> > >> Best regards > >> Thomas > >> > >>> But even as-is the patch is a step into the right direction. > >>> > >>> Acked-by: Gerd Hoffmann <kraxel@redhat.com> > >>> > >>> cheers, > >>> Gerd > >>> > >> > >> -- > >> Thomas Zimmermann > >> Graphics Driver Developer > >> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany > >> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah > >> HRB 21284 (AG Nürnberg) > >> > > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah > HRB 21284 (AG Nürnberg) >
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index 06a8c076cb33..165895d00fcb 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -22,10 +22,9 @@ static void mga_hide_cursor(struct mga_device *mdev) { WREG8(MGA_CURPOSXL, 0); WREG8(MGA_CURPOSXH, 0); - if (mdev->cursor.pixels_1->pin_count) - drm_gem_vram_unpin_locked(mdev->cursor.pixels_1); - if (mdev->cursor.pixels_2->pin_count) - drm_gem_vram_unpin_locked(mdev->cursor.pixels_2); + if (mdev->cursor.pixels_current) + drm_gem_vram_unpin(mdev->cursor.pixels_current); + mdev->cursor.pixels_current = NULL; } int mga_crtc_cursor_set(struct drm_crtc *crtc, @@ -39,7 +38,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, struct drm_gem_vram_object *pixels_1 = mdev->cursor.pixels_1; struct drm_gem_vram_object *pixels_2 = mdev->cursor.pixels_2; struct drm_gem_vram_object *pixels_current = mdev->cursor.pixels_current; - struct drm_gem_vram_object *pixels_prev = mdev->cursor.pixels_prev; + struct drm_gem_vram_object *pixels_next; struct drm_gem_object *obj; struct drm_gem_vram_object *gbo = NULL; int ret = 0; @@ -52,6 +51,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, bool found = false; int colour_count = 0; s64 gpu_addr; + u64 dst_gpu; u8 reg_index; u8 this_row[48]; @@ -61,80 +61,66 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, return -ENOTSUPP; /* Didn't allocate space for cursors */ } - if ((width != 64 || height != 64) && handle) { - WREG8(MGA_CURPOSXL, 0); - WREG8(MGA_CURPOSXH, 0); - return -EINVAL; + if (WARN_ON(pixels_current && + pixels_1 != pixels_current && + pixels_2 != pixels_current)) { + return -ENOTSUPP; /* inconsistent state */ } - BUG_ON(pixels_1 != pixels_current && pixels_1 != pixels_prev); - BUG_ON(pixels_2 != pixels_current && pixels_2 != pixels_prev); - BUG_ON(pixels_current == pixels_prev); - if (!handle || !file_priv) { mga_hide_cursor(mdev); return 0; } - obj = drm_gem_object_lookup(file_priv, handle); - if (!obj) - return -ENOENT; - - ret = drm_gem_vram_lock(pixels_1, true); - if (ret) { + if (width != 64 || height != 64) { WREG8(MGA_CURPOSXL, 0); WREG8(MGA_CURPOSXH, 0); - goto out_unref; - } - ret = drm_gem_vram_lock(pixels_2, true); - if (ret) { - WREG8(MGA_CURPOSXL, 0); - WREG8(MGA_CURPOSXH, 0); - drm_gem_vram_unlock(pixels_1); - goto out_unlock1; + return -EINVAL; } - /* Move cursor buffers into VRAM if they aren't already */ - if (!pixels_1->pin_count) { - ret = drm_gem_vram_pin_locked(pixels_1, - DRM_GEM_VRAM_PL_FLAG_VRAM); - if (ret) - goto out1; - gpu_addr = drm_gem_vram_offset(pixels_1); - if (gpu_addr < 0) { - drm_gem_vram_unpin_locked(pixels_1); - goto out1; - } - mdev->cursor.pixels_1_gpu_addr = gpu_addr; - } - if (!pixels_2->pin_count) { - ret = drm_gem_vram_pin_locked(pixels_2, - DRM_GEM_VRAM_PL_FLAG_VRAM); - if (ret) { - drm_gem_vram_unpin_locked(pixels_1); - goto out1; - } - gpu_addr = drm_gem_vram_offset(pixels_2); - if (gpu_addr < 0) { - drm_gem_vram_unpin_locked(pixels_1); - drm_gem_vram_unpin_locked(pixels_2); - goto out1; - } - mdev->cursor.pixels_2_gpu_addr = gpu_addr; - } + if (pixels_current == pixels_1) + pixels_next = pixels_2; + else + pixels_next = pixels_1; + obj = drm_gem_object_lookup(file_priv, handle); + if (!obj) + return -ENOENT; gbo = drm_gem_vram_of_gem(obj); - ret = drm_gem_vram_lock(gbo, true); + ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_SYSTEM); if (ret) { dev_err(&dev->pdev->dev, "failed to lock user bo\n"); - goto out1; + goto err_drm_gem_object_put_unlocked; } src = drm_gem_vram_kmap(gbo, true, NULL); if (IS_ERR(src)) { ret = PTR_ERR(src); - dev_err(&dev->pdev->dev, "failed to kmap user buffer updates\n"); - goto out2; + dev_err(&dev->pdev->dev, + "failed to kmap user buffer updates\n"); + goto err_drm_gem_vram_unpin_src; + } + + /* Pin and map up-coming buffer to write colour indices */ + ret = drm_gem_vram_pin(pixels_next, DRM_GEM_VRAM_PL_FLAG_VRAM); + if (ret) + dev_err(&dev->pdev->dev, + "failed to pin cursor buffer: %d\n", ret); + goto err_drm_gem_vram_kunmap_src; + dst = drm_gem_vram_kmap(pixels_next, true, NULL); + if (IS_ERR(dst)) { + ret = PTR_ERR(dst); + dev_err(&dev->pdev->dev, + "failed to kmap cursor updates: %d\n", ret); + goto err_drm_gem_vram_unpin_dst; + } + gpu_addr = drm_gem_vram_offset(pixels_2); + if (gpu_addr < 0) { + ret = (int)gpu_addr; + dev_err(&dev->pdev->dev, + "failed to get cursor scanout address: %d\n", ret); + goto err_drm_gem_vram_kunmap_dst; } + dst_gpu = (u64)gpu_addr; memset(&colour_set[0], 0, sizeof(uint32_t)*16); /* width*height*4 = 16384 */ @@ -149,7 +135,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, warn_transparent = false; /* Only tell the user once. */ } ret = -EINVAL; - goto out3; + goto err_drm_gem_vram_kunmap_dst; } /* Don't need to store transparent pixels as colours */ if (this_colour>>24 == 0x0) @@ -171,7 +157,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, warn_palette = false; /* Only tell the user once. */ } ret = -EINVAL; - goto out3; + goto err_drm_gem_vram_kunmap_dst; } *next_space = this_colour; next_space++; @@ -190,14 +176,6 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, BUG_ON((colour_set[i]>>24 & 0xff) != 0xff); } - /* Map up-coming buffer to write colour indices */ - dst = drm_gem_vram_kmap(pixels_prev, true, NULL); - if (IS_ERR(dst)) { - ret = PTR_ERR(dst); - dev_err(&dev->pdev->dev, "failed to kmap cursor updates\n"); - goto out3; - } - /* now write colour indices into hardware cursor buffer */ for (row = 0; row < 64; row++) { memset(&this_row[0], 0, 48); @@ -224,42 +202,35 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, } /* Program gpu address of cursor buffer */ - if (pixels_prev == pixels_1) - gpu_addr = mdev->cursor.pixels_1_gpu_addr; - else - gpu_addr = mdev->cursor.pixels_2_gpu_addr; - WREG_DAC(MGA1064_CURSOR_BASE_ADR_LOW, (u8)((gpu_addr>>10) & 0xff)); - WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, (u8)((gpu_addr>>18) & 0x3f)); + WREG_DAC(MGA1064_CURSOR_BASE_ADR_LOW, (u8)((dst_gpu>>10) & 0xff)); + WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, (u8)((dst_gpu>>18) & 0x3f)); /* Adjust cursor control register to turn on the cursor */ WREG_DAC(MGA1064_CURSOR_CTL, 4); /* 16-colour palletized cursor mode */ - /* Now swap internal buffer pointers */ - if (mdev->cursor.pixels_1 == mdev->cursor.pixels_prev) { - mdev->cursor.pixels_prev = mdev->cursor.pixels_2; - mdev->cursor.pixels_current = mdev->cursor.pixels_1; - } else if (mdev->cursor.pixels_1 == mdev->cursor.pixels_current) { - mdev->cursor.pixels_prev = mdev->cursor.pixels_1; - mdev->cursor.pixels_current = mdev->cursor.pixels_2; - } else { - BUG(); - } - ret = 0; + /* Now update internal buffer pointers */ + if (pixels_current) + drm_gem_vram_unpin(pixels_current); + mdev->cursor.pixels_current = pixels_next; - drm_gem_vram_kunmap(pixels_prev); - out3: + drm_gem_vram_kunmap(pixels_next); + drm_gem_vram_unpin(pixels_next); drm_gem_vram_kunmap(gbo); - out2: - drm_gem_vram_unlock(gbo); - out1: - if (ret) - mga_hide_cursor(mdev); - drm_gem_vram_unlock(pixels_1); -out_unlock1: - drm_gem_vram_unlock(pixels_2); -out_unref: + drm_gem_vram_unpin(gbo); drm_gem_object_put_unlocked(obj); + return 0; + +err_drm_gem_vram_kunmap_dst: + drm_gem_vram_kunmap(pixels_next); +err_drm_gem_vram_unpin_dst: + drm_gem_vram_unpin(pixels_next); +err_drm_gem_vram_kunmap_src: + drm_gem_vram_kunmap(gbo); +err_drm_gem_vram_unpin_src: + drm_gem_vram_unpin(gbo); +err_drm_gem_object_put_unlocked: + drm_gem_object_put_unlocked(obj); return ret; } diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 6180acbca7ca..d39f2a567b3f 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -158,11 +158,8 @@ struct mga_cursor { */ struct drm_gem_vram_object *pixels_1; struct drm_gem_vram_object *pixels_2; - u64 pixels_1_gpu_addr, pixels_2_gpu_addr; /* The currently displayed icon, this points to one of pixels_1, or pixels_2 */ struct drm_gem_vram_object *pixels_current; - /* The previously displayed icon */ - struct drm_gem_vram_object *pixels_prev; }; struct mga_mc { diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index f3687fed4075..0d7fc00e5d8a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -241,10 +241,8 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags) mdev->cursor.pixels_2 = NULL; dev_warn(&dev->pdev->dev, "Could not allocate space for cursors. Not doing hardware cursors.\n"); - } else { - mdev->cursor.pixels_current = mdev->cursor.pixels_1; - mdev->cursor.pixels_prev = mdev->cursor.pixels_2; } + mdev->cursor.pixels_current = NULL; return 0;
The cursor handling in mgag200 is complicated to understand. It touches a number of different BOs, but doesn't really use all of them. Rewriting the cursor update reduces the amount of cursor state. There are two BOs for double-buffered HW updates. The source BO updates the one that is currently not displayed and then switches buffers. Explicit BO locking has been removed from the code. BOs are simply pinned and unpinned in video RAM. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 165 ++++++++++------------- drivers/gpu/drm/mgag200/mgag200_drv.h | 3 - drivers/gpu/drm/mgag200/mgag200_main.c | 4 +- 3 files changed, 69 insertions(+), 103 deletions(-)