drm/virtio: Don't reinvent a flipping wheel
diff mbox

Message ID 1465510073-20951-1-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter June 9, 2016, 10:07 p.m. UTC
Now that the core helpers support nonblocking atomic commits there's
no need to invent that wheel separately (instead of fixing the bug in
the atomic implementation of virtio, as it should have been done!).

v2: Rebased on top of

commit e7cf0963f816fa44190caaf51aeffaa614c340c6
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Tue May 31 08:50:47 2016 +0200

virtio-gpu: add atomic_commit function

Cc: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Gerd Hoffmann <kraxel@redhat.com> (v1)
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/virtio/virtgpu_display.c | 71 ++++++--------------------------
 1 file changed, 13 insertions(+), 58 deletions(-)

Comments

Daniel Vetter June 10, 2016, 3:20 p.m. UTC | #1
On Fri, Jun 10, 2016 at 12:07:53AM +0200, Daniel Vetter wrote:
> Now that the core helpers support nonblocking atomic commits there's
> no need to invent that wheel separately (instead of fixing the bug in
> the atomic implementation of virtio, as it should have been done!).
> 
> v2: Rebased on top of
> 
> commit e7cf0963f816fa44190caaf51aeffaa614c340c6
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Tue May 31 08:50:47 2016 +0200
> 
> virtio-gpu: add atomic_commit function
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com> (v1)
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Gerd, can you pls retest? I think due to your change in the above
referenced commit to switch to active_only=true in commit_planes() this is
now broken.
-Daniel

> ---
>  drivers/gpu/drm/virtio/virtgpu_display.c | 71 ++++++--------------------------
>  1 file changed, 13 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
> index 3d0fa049b34c..a09dc57fea9c 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -38,56 +38,11 @@
>  #define XRES_MAX  8192
>  #define YRES_MAX  8192
>  
> -static int virtio_gpu_page_flip(struct drm_crtc *crtc,
> -				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event,
> -				uint32_t flags)
> -{
> -	struct virtio_gpu_device *vgdev = crtc->dev->dev_private;
> -	struct virtio_gpu_output *output =
> -		container_of(crtc, struct virtio_gpu_output, crtc);
> -	struct drm_plane *plane = crtc->primary;
> -	struct virtio_gpu_framebuffer *vgfb;
> -	struct virtio_gpu_object *bo;
> -	unsigned long irqflags;
> -	uint32_t handle;
> -
> -	plane->fb = fb;
> -	vgfb = to_virtio_gpu_framebuffer(plane->fb);
> -	bo = gem_to_virtio_gpu_obj(vgfb->obj);
> -	handle = bo->hw_res_handle;
> -
> -	DRM_DEBUG("handle 0x%x%s, crtc %dx%d\n", handle,
> -		  bo->dumb ? ", dumb" : "",
> -		  crtc->mode.hdisplay, crtc->mode.vdisplay);
> -	if (bo->dumb) {
> -		virtio_gpu_cmd_transfer_to_host_2d
> -			(vgdev, handle, 0,
> -			 cpu_to_le32(crtc->mode.hdisplay),
> -			 cpu_to_le32(crtc->mode.vdisplay),
> -			 0, 0, NULL);
> -	}
> -	virtio_gpu_cmd_set_scanout(vgdev, output->index, handle,
> -				   crtc->mode.hdisplay,
> -				   crtc->mode.vdisplay, 0, 0);
> -	virtio_gpu_cmd_resource_flush(vgdev, handle, 0, 0,
> -				      crtc->mode.hdisplay,
> -				      crtc->mode.vdisplay);
> -
> -	if (event) {
> -		spin_lock_irqsave(&crtc->dev->event_lock, irqflags);
> -		drm_crtc_send_vblank_event(crtc, event);
> -		spin_unlock_irqrestore(&crtc->dev->event_lock, irqflags);
> -	}
> -
> -	return 0;
> -}
> -
>  static const struct drm_crtc_funcs virtio_gpu_crtc_funcs = {
>  	.set_config             = drm_atomic_helper_set_config,
>  	.destroy                = drm_crtc_cleanup,
>  
> -	.page_flip              = virtio_gpu_page_flip,
> +	.page_flip              = drm_atomic_helper_page_flip,
>  	.reset                  = drm_atomic_helper_crtc_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>  	.atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
> @@ -185,6 +140,7 @@ static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc,
>  	spin_lock_irqsave(&crtc->dev->event_lock, flags);
>  	if (crtc->state->event)
>  		drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +	crtc->state->event = NULL;
>  	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>  }
>  
> @@ -388,30 +344,28 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev,
>  	return &virtio_gpu_fb->base;
>  }
>  
> -static int vgdev_atomic_commit(struct drm_device *dev,
> -			       struct drm_atomic_state *state,
> -			       bool nonblock)
> +static void vgdev_atomic_commit_tail(struct drm_atomic_state *state)
>  {
> -	if (nonblock)
> -		return -EBUSY;
> -
> -	drm_atomic_helper_swap_state(state, true);
> -	drm_atomic_helper_wait_for_fences(dev, 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, true);
>  
> +	drm_atomic_helper_commit_hw_done(state);
> +
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> -	drm_atomic_state_free(state);
> -	return 0;
>  }
>  
> +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,
> -	.atomic_commit = vgdev_atomic_commit,
> +	.atomic_commit = drm_atomic_helper_commit,
>  };
>  
>  int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
> @@ -419,7 +373,8 @@ int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
>  	int i;
>  
>  	drm_mode_config_init(vgdev->ddev);
> -	vgdev->ddev->mode_config.funcs = (void *)&virtio_gpu_mode_funcs;
> +	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.8.1
>
Gerd Hoffmann June 13, 2016, 9:20 a.m. UTC | #2
On Fr, 2016-06-10 at 17:20 +0200, Daniel Vetter wrote:
> On Fri, Jun 10, 2016 at 12:07:53AM +0200, Daniel Vetter wrote:
> > Now that the core helpers support nonblocking atomic commits there's
> > no need to invent that wheel separately (instead of fixing the bug in
> > the atomic implementation of virtio, as it should have been done!).
> > 
> > v2: Rebased on top of
> > 
> > commit e7cf0963f816fa44190caaf51aeffaa614c340c6
> > Author: Gerd Hoffmann <kraxel@redhat.com>
> > Date:   Tue May 31 08:50:47 2016 +0200
> > 
> > virtio-gpu: add atomic_commit function
> > 
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Tested-by: Gerd Hoffmann <kraxel@redhat.com> (v1)
> > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> (v1)
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Gerd, can you pls retest? I think due to your change in the above
> referenced commit to switch to active_only=true in commit_planes() this is
> now broken.

Yes, probably it'll break things.

Any branch I can test?  Your "stuff" branch seems to not yet have the
commit above.

I guess virtio-gpu should switch over to override .atomic_commit_tail
instead of .atomic_commit?

cheers,
  Gerd
Daniel Vetter June 13, 2016, 1:34 p.m. UTC | #3
On Mon, Jun 13, 2016 at 11:20 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Fr, 2016-06-10 at 17:20 +0200, Daniel Vetter wrote:
>> On Fri, Jun 10, 2016 at 12:07:53AM +0200, Daniel Vetter wrote:
>> > Now that the core helpers support nonblocking atomic commits there's
>> > no need to invent that wheel separately (instead of fixing the bug in
>> > the atomic implementation of virtio, as it should have been done!).
>> >
>> > v2: Rebased on top of
>> >
>> > commit e7cf0963f816fa44190caaf51aeffaa614c340c6
>> > Author: Gerd Hoffmann <kraxel@redhat.com>
>> > Date:   Tue May 31 08:50:47 2016 +0200
>> >
>> > virtio-gpu: add atomic_commit function
>> >
>> > Cc: Gerd Hoffmann <kraxel@redhat.com>
>> > Tested-by: Gerd Hoffmann <kraxel@redhat.com> (v1)
>> > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> (v1)
>> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>
>> Gerd, can you pls retest? I think due to your change in the above
>> referenced commit to switch to active_only=true in commit_planes() this is
>> now broken.
>
> Yes, probably it'll break things.
>
> Any branch I can test?  Your "stuff" branch seems to not yet have the
> commit above.
>
> I guess virtio-gpu should switch over to override .atomic_commit_tail
> instead of .atomic_commit?

See v2 that I've sent out - it contains these changes already. Patch
applies on top of topic/drm-misc, but I'll be pushing out a new
version of stuff asap.
-Daniel
Gerd Hoffmann June 14, 2016, 2:25 p.m. UTC | #4
> >> Gerd, can you pls retest? I think due to your change in the above
> >> referenced commit to switch to active_only=true in commit_planes() this is
> >> now broken.
> >
> > Yes, probably it'll break things.
> >
> > Any branch I can test?  Your "stuff" branch seems to not yet have the
> > commit above.
> >
> > I guess virtio-gpu should switch over to override .atomic_commit_tail
> > instead of .atomic_commit?
> 
> See v2 that I've sent out - it contains these changes already. Patch
> applies on top of topic/drm-misc, but I'll be pushing out a new
> version of stuff asap.

Tested refreshed "stuff" branch now, seems to work just fine.
Anything specific to look out for?

cheers
  Gerd
Daniel Vetter June 14, 2016, 2:33 p.m. UTC | #5
On Tue, Jun 14, 2016 at 04:25:45PM +0200, Gerd Hoffmann wrote:
> > >> Gerd, can you pls retest? I think due to your change in the above
> > >> referenced commit to switch to active_only=true in commit_planes() this is
> > >> now broken.
> > >
> > > Yes, probably it'll break things.
> > >
> > > Any branch I can test?  Your "stuff" branch seems to not yet have the
> > > commit above.
> > >
> > > I guess virtio-gpu should switch over to override .atomic_commit_tail
> > > instead of .atomic_commit?
> > 
> > See v2 that I've sent out - it contains these changes already. Patch
> > applies on top of topic/drm-misc, but I'll be pushing out a new
> > version of stuff asap.
> 
> Tested refreshed "stuff" branch now, seems to work just fine.
> Anything specific to look out for?

If no noise in dmesg and not stalls and no issues, then it should be all
fine. I've removed the (v1) disclaimer and applied the patch to drm-misc,
thanks a lot for testing and reviewing.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
index 3d0fa049b34c..a09dc57fea9c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -38,56 +38,11 @@ 
 #define XRES_MAX  8192
 #define YRES_MAX  8192
 
-static int virtio_gpu_page_flip(struct drm_crtc *crtc,
-				struct drm_framebuffer *fb,
-				struct drm_pending_vblank_event *event,
-				uint32_t flags)
-{
-	struct virtio_gpu_device *vgdev = crtc->dev->dev_private;
-	struct virtio_gpu_output *output =
-		container_of(crtc, struct virtio_gpu_output, crtc);
-	struct drm_plane *plane = crtc->primary;
-	struct virtio_gpu_framebuffer *vgfb;
-	struct virtio_gpu_object *bo;
-	unsigned long irqflags;
-	uint32_t handle;
-
-	plane->fb = fb;
-	vgfb = to_virtio_gpu_framebuffer(plane->fb);
-	bo = gem_to_virtio_gpu_obj(vgfb->obj);
-	handle = bo->hw_res_handle;
-
-	DRM_DEBUG("handle 0x%x%s, crtc %dx%d\n", handle,
-		  bo->dumb ? ", dumb" : "",
-		  crtc->mode.hdisplay, crtc->mode.vdisplay);
-	if (bo->dumb) {
-		virtio_gpu_cmd_transfer_to_host_2d
-			(vgdev, handle, 0,
-			 cpu_to_le32(crtc->mode.hdisplay),
-			 cpu_to_le32(crtc->mode.vdisplay),
-			 0, 0, NULL);
-	}
-	virtio_gpu_cmd_set_scanout(vgdev, output->index, handle,
-				   crtc->mode.hdisplay,
-				   crtc->mode.vdisplay, 0, 0);
-	virtio_gpu_cmd_resource_flush(vgdev, handle, 0, 0,
-				      crtc->mode.hdisplay,
-				      crtc->mode.vdisplay);
-
-	if (event) {
-		spin_lock_irqsave(&crtc->dev->event_lock, irqflags);
-		drm_crtc_send_vblank_event(crtc, event);
-		spin_unlock_irqrestore(&crtc->dev->event_lock, irqflags);
-	}
-
-	return 0;
-}
-
 static const struct drm_crtc_funcs virtio_gpu_crtc_funcs = {
 	.set_config             = drm_atomic_helper_set_config,
 	.destroy                = drm_crtc_cleanup,
 
-	.page_flip              = virtio_gpu_page_flip,
+	.page_flip              = drm_atomic_helper_page_flip,
 	.reset                  = drm_atomic_helper_crtc_reset,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
@@ -185,6 +140,7 @@  static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc,
 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
 	if (crtc->state->event)
 		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+	crtc->state->event = NULL;
 	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 }
 
@@ -388,30 +344,28 @@  virtio_gpu_user_framebuffer_create(struct drm_device *dev,
 	return &virtio_gpu_fb->base;
 }
 
-static int vgdev_atomic_commit(struct drm_device *dev,
-			       struct drm_atomic_state *state,
-			       bool nonblock)
+static void vgdev_atomic_commit_tail(struct drm_atomic_state *state)
 {
-	if (nonblock)
-		return -EBUSY;
-
-	drm_atomic_helper_swap_state(state, true);
-	drm_atomic_helper_wait_for_fences(dev, 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, true);
 
+	drm_atomic_helper_commit_hw_done(state);
+
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
-	drm_atomic_state_free(state);
-	return 0;
 }
 
+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,
-	.atomic_commit = vgdev_atomic_commit,
+	.atomic_commit = drm_atomic_helper_commit,
 };
 
 int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
@@ -419,7 +373,8 @@  int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
 	int i;
 
 	drm_mode_config_init(vgdev->ddev);
-	vgdev->ddev->mode_config.funcs = (void *)&virtio_gpu_mode_funcs;
+	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;