| Message ID | 20200709123339.547390-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dma-buf.rst: Document why indefinite fences are a bad idea | expand |
On Thu, Jul 09, 2020 at 02:33:39PM +0200, Daniel Vetter wrote: > Exactly matches the one in the helpers. > > This avoids me having to roll out dma-fence critical section > annotations to this copy. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: virtualization@lists.linux-foundation.org Acked-by: Gerd Hoffmann <kraxel@redhat.com>
On Thu, Jul 09, 2020 at 02:33:39PM +0200, Daniel Vetter wrote: > Exactly matches the one in the helpers. > > This avoids me having to roll out dma-fence critical section > annotations to this copy. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: virtualization@lists.linux-foundation.org > --- > drivers/gpu/drm/virtio/virtgpu_display.c | 20 -------------------- > 1 file changed, 20 deletions(-) Very nice catch: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c > index f3ce49c5a34c..af55b334be2f 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_display.c > +++ b/drivers/gpu/drm/virtio/virtgpu_display.c > @@ -314,25 +314,6 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev, > return &virtio_gpu_fb->base; > } > > -static void vgdev_atomic_commit_tail(struct drm_atomic_state *state) > -{ > - struct drm_device *dev = state->dev; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - drm_atomic_helper_commit_modeset_enables(dev, state); > - drm_atomic_helper_commit_planes(dev, state, 0); > - > - drm_atomic_helper_fake_vblank(state); > - drm_atomic_helper_commit_hw_done(state); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - drm_atomic_helper_cleanup_planes(dev, state); > -} > - > -static const struct drm_mode_config_helper_funcs virtio_mode_config_helpers = { > - .atomic_commit_tail = vgdev_atomic_commit_tail, > -}; > - > static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = { > .fb_create = virtio_gpu_user_framebuffer_create, > .atomic_check = drm_atomic_helper_check, > @@ -346,7 +327,6 @@ void virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) > drm_mode_config_init(vgdev->ddev); > vgdev->ddev->mode_config.quirk_addfb_prefer_host_byte_order = true; > vgdev->ddev->mode_config.funcs = &virtio_gpu_mode_funcs; > - vgdev->ddev->mode_config.helper_private = &virtio_mode_config_helpers; > > /* modes will be validated against the framebuffer size */ > vgdev->ddev->mode_config.min_width = XRES_MIN; > -- > 2.27.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jul 09, 2020 at 04:05:31PM +0200, Sam Ravnborg wrote: > On Thu, Jul 09, 2020 at 02:33:39PM +0200, Daniel Vetter wrote: > > Exactly matches the one in the helpers. > > > > This avoids me having to roll out dma-fence critical section > > annotations to this copy. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: virtualization@lists.linux-foundation.org > > --- > > drivers/gpu/drm/virtio/virtgpu_display.c | 20 -------------------- > > 1 file changed, 20 deletions(-) > Very nice catch: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Patch applied, thanks for reviewing. > > > > diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c > > index f3ce49c5a34c..af55b334be2f 100644 > > --- a/drivers/gpu/drm/virtio/virtgpu_display.c > > +++ b/drivers/gpu/drm/virtio/virtgpu_display.c > > @@ -314,25 +314,6 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev, > > return &virtio_gpu_fb->base; > > } > > > > -static void vgdev_atomic_commit_tail(struct drm_atomic_state *state) > > -{ > > - struct drm_device *dev = state->dev; > > - > > - drm_atomic_helper_commit_modeset_disables(dev, state); > > - drm_atomic_helper_commit_modeset_enables(dev, state); > > - drm_atomic_helper_commit_planes(dev, state, 0); > > - > > - drm_atomic_helper_fake_vblank(state); > > - drm_atomic_helper_commit_hw_done(state); > > - > > - drm_atomic_helper_wait_for_vblanks(dev, state); > > - drm_atomic_helper_cleanup_planes(dev, state); > > -} > > - > > -static const struct drm_mode_config_helper_funcs virtio_mode_config_helpers = { > > - .atomic_commit_tail = vgdev_atomic_commit_tail, > > -}; > > - > > static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = { > > .fb_create = virtio_gpu_user_framebuffer_create, > > .atomic_check = drm_atomic_helper_check, > > @@ -346,7 +327,6 @@ void virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) > > drm_mode_config_init(vgdev->ddev); > > vgdev->ddev->mode_config.quirk_addfb_prefer_host_byte_order = true; > > vgdev->ddev->mode_config.funcs = &virtio_gpu_mode_funcs; > > - vgdev->ddev->mode_config.helper_private = &virtio_mode_config_helpers; > > > > /* modes will be validated against the framebuffer size */ > > vgdev->ddev->mode_config.min_width = XRES_MIN; > > -- > > 2.27.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 09. 07. 20, 14:33, Daniel Vetter wrote: > Exactly matches the one in the helpers. It's not that exact. The order of modeset_enables and planes is different. And this causes a regression -- no fb in qemu. So if I run drm-tip, no fb. If I revert 73f15a9, it works. If I then switch the two calls in vgdev_atomic_commit_tail, it doesn't work again. So the order apparently matters. > This avoids me having to roll out dma-fence critical section > annotations to this copy. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: virtualization@lists.linux-foundation.org > --- > drivers/gpu/drm/virtio/virtgpu_display.c | 20 -------------------- > 1 file changed, 20 deletions(-) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c > index f3ce49c5a34c..af55b334be2f 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_display.c > +++ b/drivers/gpu/drm/virtio/virtgpu_display.c > @@ -314,25 +314,6 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev, > return &virtio_gpu_fb->base; > } > > -static void vgdev_atomic_commit_tail(struct drm_atomic_state *state) > -{ > - struct drm_device *dev = state->dev; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - drm_atomic_helper_commit_modeset_enables(dev, state); > - drm_atomic_helper_commit_planes(dev, state, 0); > - > - drm_atomic_helper_fake_vblank(state); > - drm_atomic_helper_commit_hw_done(state); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - drm_atomic_helper_cleanup_planes(dev, state); > -} > - > -static const struct drm_mode_config_helper_funcs virtio_mode_config_helpers = { > - .atomic_commit_tail = vgdev_atomic_commit_tail, > -}; > - > static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = { > .fb_create = virtio_gpu_user_framebuffer_create, > .atomic_check = drm_atomic_helper_check, > @@ -346,7 +327,6 @@ void virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) > drm_mode_config_init(vgdev->ddev); > vgdev->ddev->mode_config.quirk_addfb_prefer_host_byte_order = true; > vgdev->ddev->mode_config.funcs = &virtio_gpu_mode_funcs; > - vgdev->ddev->mode_config.helper_private = &virtio_mode_config_helpers; > > /* modes will be validated against the framebuffer size */ > vgdev->ddev->mode_config.min_width = XRES_MIN; > thanks,
On 19. 08. 20, 14:43, Jiri Slaby wrote: > On 09. 07. 20, 14:33, Daniel Vetter wrote: >> Exactly matches the one in the helpers. > > It's not that exact. The order of modeset_enables and planes is > different. And this causes a regression -- no fb in qemu. > > So if I run drm-tip, no fb. > If I revert 73f15a9, it works. > If I then switch the two calls in vgdev_atomic_commit_tail, it doesn't > work again. > > So the order apparently matters. And it was the intention, it seems: commit e7cf0963f816fa44190caaf51aeffaa614c340c6 Author: Gerd Hoffmann <kraxel@redhat.com> Date: Tue May 31 08:50:47 2016 +0200 virtio-gpu: add atomic_commit function Do modesets first, then call drm_atomic_helper_commit_planes with active_only = true. That way the outputs doesn't get disabled temporarly on atomic commits. thanks,
On Wed, Aug 19, 2020 at 02:43:28PM +0200, Jiri Slaby wrote: > On 09. 07. 20, 14:33, Daniel Vetter wrote: > > Exactly matches the one in the helpers. > > It's not that exact. The order of modeset_enables and planes is > different. And this causes a regression -- no fb in qemu. Does https://patchwork.freedesktop.org/patch/385980/ help? take care, Gerd
On 19. 08. 20, 15:24, Gerd Hoffmann wrote: > On Wed, Aug 19, 2020 at 02:43:28PM +0200, Jiri Slaby wrote: >> On 09. 07. 20, 14:33, Daniel Vetter wrote: >>> Exactly matches the one in the helpers. >> >> It's not that exact. The order of modeset_enables and planes is >> different. And this causes a regression -- no fb in qemu. > > Does https://patchwork.freedesktop.org/patch/385980/ help? Yes, it does. thanks,
On Thu, Aug 20, 2020 at 08:32:51AM +0200, Jiri Slaby wrote: > On 19. 08. 20, 15:24, Gerd Hoffmann wrote: > > On Wed, Aug 19, 2020 at 02:43:28PM +0200, Jiri Slaby wrote: > >> On 09. 07. 20, 14:33, Daniel Vetter wrote: > >>> Exactly matches the one in the helpers. > >> > >> It's not that exact. The order of modeset_enables and planes is > >> different. And this causes a regression -- no fb in qemu. > > > > Does https://patchwork.freedesktop.org/patch/385980/ help? > > Yes, it does. Any chance you can send a tested-by & acked-by for the series so I can get it merged? thanks, Gerd
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index f3ce49c5a34c..af55b334be2f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -314,25 +314,6 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev, return &virtio_gpu_fb->base; } -static void vgdev_atomic_commit_tail(struct drm_atomic_state *state) -{ - struct drm_device *dev = state->dev; - - drm_atomic_helper_commit_modeset_disables(dev, state); - drm_atomic_helper_commit_modeset_enables(dev, state); - drm_atomic_helper_commit_planes(dev, state, 0); - - drm_atomic_helper_fake_vblank(state); - drm_atomic_helper_commit_hw_done(state); - - drm_atomic_helper_wait_for_vblanks(dev, state); - drm_atomic_helper_cleanup_planes(dev, state); -} - -static const struct drm_mode_config_helper_funcs virtio_mode_config_helpers = { - .atomic_commit_tail = vgdev_atomic_commit_tail, -}; - static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = { .fb_create = virtio_gpu_user_framebuffer_create, .atomic_check = drm_atomic_helper_check, @@ -346,7 +327,6 @@ void virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) drm_mode_config_init(vgdev->ddev); vgdev->ddev->mode_config.quirk_addfb_prefer_host_byte_order = true; vgdev->ddev->mode_config.funcs = &virtio_gpu_mode_funcs; - vgdev->ddev->mode_config.helper_private = &virtio_mode_config_helpers; /* modes will be validated against the framebuffer size */ vgdev->ddev->mode_config.min_width = XRES_MIN;
Exactly matches the one in the helpers. This avoids me having to roll out dma-fence critical section annotations to this copy. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: David Airlie <airlied@linux.ie> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: virtualization@lists.linux-foundation.org --- drivers/gpu/drm/virtio/virtgpu_display.c | 20 -------------------- 1 file changed, 20 deletions(-)