[PATCH 01/15] staging: vboxvideo: Convert to atomic modesetting API

[PATCH 01/15] staging: vboxvideo: Convert to atomic modesetting API

[Hans de Goede]

Hi All,

This series converts the vboxvideo driver to the modesetting API, once this
is merged I will submit a patch to move the driver out of staging and into
drivers/gpu/drm.

This series consists of 3 parts:

1) More cleanups / prep work for atomic conversion
2) The actual atomic conversion, this is modelled after the atomic conversion
   of the qxl driver done by Gabriel Krisman Bertazi. Gabriel if you are
   reading this, thank you for providing a roadmap how to go about this.
3) Restore some features which were lost by the atomic conversion +
   more cleanups

Regards,

Hans

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/15] staging: vboxvideo: Cleanup vbox_set_up_input_mapping()

[Hans de Goede]

This cleanups 2 things:

1) The first time we loop over the crtc-s, to compare framebuffers, fb1 may
get set to NULL by the fb1 = CRTC_FB(crtci); statement and then we call
to_vbox_framebuffer() on it. The result of this call is only used for
an address comparison, so we don't end up dereferencing the bad pointer,
but still it is better to not do this.

2) Since we already figure out the first crtc with a fb in the first loop
and store that in fb1, there is no need to loop over the crtc-s again just
to find the first crtc with a fb again.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_mode.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index bef99664d030..4f5d28aeca95 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -169,7 +169,7 @@ static bool vbox_set_up_input_mapping(struct vbox_private *vbox)
 {
 	struct drm_crtc *crtci;
 	struct drm_connector *connectori;
-	struct drm_framebuffer *fb1 = NULL;
+	struct drm_framebuffer *fb, *fb1 = NULL;
 	bool single_framebuffer = true;
 	bool old_single_framebuffer = vbox->single_framebuffer;
 	u16 width = 0, height = 0;
@@ -180,25 +180,25 @@ static bool vbox_set_up_input_mapping(struct vbox_private *vbox)
 	 * Same fall-back if this is the fbdev frame-buffer.
 	 */
 	list_for_each_entry(crtci, &vbox->ddev.mode_config.crtc_list, head) {
+		fb = CRTC_FB(crtci);
+		if (!fb)
+			continue;
+
 		if (!fb1) {
-			fb1 = CRTC_FB(crtci);
+			fb1 = fb;
 			if (to_vbox_framebuffer(fb1) == &vbox->fbdev->afb)
 				break;
-		} else if (CRTC_FB(crtci) && fb1 != CRTC_FB(crtci)) {
+		} else if (fb != fb1) {
 			single_framebuffer = false;
 		}
 	}
+	if (!fb1)
+		return false;
+
 	if (single_framebuffer) {
 		vbox->single_framebuffer = true;
-		list_for_each_entry(crtci, &vbox->ddev.mode_config.crtc_list,
-				    head) {
-			if (!CRTC_FB(crtci))
-				continue;
-
-			vbox->input_mapping_width = CRTC_FB(crtci)->width;
-			vbox->input_mapping_height = CRTC_FB(crtci)->height;
-			break;
-		}
+		vbox->input_mapping_width = fb1->width;
+		vbox->input_mapping_height = fb1->height;
 		return old_single_framebuffer != vbox->single_framebuffer;
 	}
 	/* Otherwise calculate the total span of all screens. */
-- 
2.19.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/15] staging: vboxvideo: Cleanup vbox_set_up_input_mapping()

[Greg Kroah-Hartman]

On Wed, Sep 26, 2018 at 09:41:52PM +0200, Hans de Goede wrote:
> This cleanups 2 things:
> 
> 1) The first time we loop over the crtc-s, to compare framebuffers, fb1 may
> get set to NULL by the fb1 = CRTC_FB(crtci); statement and then we call
> to_vbox_framebuffer() on it. The result of this call is only used for
> an address comparison, so we don't end up dereferencing the bad pointer,
> but still it is better to not do this.
> 
> 2) Since we already figure out the first crtc with a fb in the first loop
> and store that in fb1, there is no need to loop over the crtc-s again just
> to find the first crtc with a fb again.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I have two patch 1/15 in this series, making it 16 patches?

Something went odd on your end, can you please resend these properly?

thanks,

greg k-h

Re: [PATCH 01/15] staging: vboxvideo: Cleanup vbox_set_up_input_mapping()

[Hans de Goede]

Hi,

On 28-09-18 14:38, Greg Kroah-Hartman wrote:
> On Wed, Sep 26, 2018 at 09:41:52PM +0200, Hans de Goede wrote:
>> This cleanups 2 things:
>>
>> 1) The first time we loop over the crtc-s, to compare framebuffers, fb1 may
>> get set to NULL by the fb1 = CRTC_FB(crtci); statement and then we call
>> to_vbox_framebuffer() on it. The result of this call is only used for
>> an address comparison, so we don't end up dereferencing the bad pointer,
>> but still it is better to not do this.
>>
>> 2) Since we already figure out the first crtc with a fb in the first loop
>> and store that in fb1, there is no need to loop over the crtc-s again just
>> to find the first crtc with a fb again.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> I have two patch 1/15 in this series, making it 16 patches?

It seems like I've accidentally numbered my cover-letter
as 01/15 instead of 00/15. Could that it be that that was the problem?

> Something went odd on your end, can you please resend these properly?

Done.

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/15] staging: vboxvideo: Cleanup vbox_set_up_input_mapping()

[Greg Kroah-Hartman]

On Sat, Sep 29, 2018 at 02:23:37PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 28-09-18 14:38, Greg Kroah-Hartman wrote:
> > On Wed, Sep 26, 2018 at 09:41:52PM +0200, Hans de Goede wrote:
> > > This cleanups 2 things:
> > > 
> > > 1) The first time we loop over the crtc-s, to compare framebuffers, fb1 may
> > > get set to NULL by the fb1 = CRTC_FB(crtci); statement and then we call
> > > to_vbox_framebuffer() on it. The result of this call is only used for
> > > an address comparison, so we don't end up dereferencing the bad pointer,
> > > but still it is better to not do this.
> > > 
> > > 2) Since we already figure out the first crtc with a fb in the first loop
> > > and store that in fb1, there is no need to loop over the crtc-s again just
> > > to find the first crtc with a fb again.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > I have two patch 1/15 in this series, making it 16 patches?
> 
> It seems like I've accidentally numbered my cover-letter
> as 01/15 instead of 00/15. Could that it be that that was the problem?

Maybe, I didn't look too hard at it :)

> > Something went odd on your end, can you please resend these properly?
> 
> Done.

Now applied, thanks!

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/15] staging: vboxvideo: Remove empty encoder_helper_funcs

[Hans de Goede]

All the encoder_helper_funcs are optional, and even setting the
drm_encoder_helper_funcs vtable itself is optional and may be left out
when not using any of the helper funcs, so lets drop all of this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_mode.c | 34 ---------------------------
 1 file changed, 34 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index 4f5d28aeca95..babb02a1ebf2 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -466,39 +466,6 @@ static const struct drm_encoder_funcs vbox_enc_funcs = {
 	.destroy = vbox_encoder_destroy,
 };
 
-static void vbox_encoder_dpms(struct drm_encoder *encoder, int mode)
-{
-}
-
-static bool vbox_mode_fixup(struct drm_encoder *encoder,
-			    const struct drm_display_mode *mode,
-			    struct drm_display_mode *adjusted_mode)
-{
-	return true;
-}
-
-static void vbox_encoder_mode_set(struct drm_encoder *encoder,
-				  struct drm_display_mode *mode,
-				  struct drm_display_mode *adjusted_mode)
-{
-}
-
-static void vbox_encoder_prepare(struct drm_encoder *encoder)
-{
-}
-
-static void vbox_encoder_commit(struct drm_encoder *encoder)
-{
-}
-
-static const struct drm_encoder_helper_funcs vbox_enc_helper_funcs = {
-	.dpms = vbox_encoder_dpms,
-	.mode_fixup = vbox_mode_fixup,
-	.prepare = vbox_encoder_prepare,
-	.commit = vbox_encoder_commit,
-	.mode_set = vbox_encoder_mode_set,
-};
-
 static struct drm_encoder *vbox_encoder_init(struct drm_device *dev,
 					     unsigned int i)
 {
@@ -510,7 +477,6 @@ static struct drm_encoder *vbox_encoder_init(struct drm_device *dev,
 
 	drm_encoder_init(dev, &vbox_encoder->base, &vbox_enc_funcs,
 			 DRM_MODE_ENCODER_DAC, NULL);
-	drm_encoder_helper_add(&vbox_encoder->base, &vbox_enc_helper_funcs);
 
 	vbox_encoder->base.possible_crtcs = 1 << i;
 	return &vbox_encoder->base;
-- 
2.19.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/15] staging: vboxvideo: Temporarily remove page_flip support

[Hans de Goede]

drm_mode_page_flip_ioctl() cannot deal with the in between phase of
the transitioning to atomic modeset support. Once we start using
drm_helper_crtc_mode_set(), we start setting plane->state on the primary
plane. But we are not fully atomic yet so then set both plane-state->fb
and plane->fb.

If both plane-state->fb and plane->fb are set drm_mode_page_flip_ioctl()
gets confused and stops calling drm_framebuffer_get() on the new fb while
still calling drm_framebuffer_put() on the old fb.

The current page_flip implementation expects drm_mode_page_flip_ioctl()
to take care of both and once we switch to drm_atomic_helper_page_flip()
that will expect neither to be done, taking care of both itself.

So for the transition we need to remove page_flip support and then after
the transition is complete and we set DRIVER_ATOMIC in our driver_features,
we can start using drm_atomic_helper_page_flip().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_mode.c | 35 ---------------------------
 1 file changed, 35 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index babb02a1ebf2..adb6bcf989d1 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -282,40 +282,6 @@ static int vbox_crtc_mode_set(struct drm_crtc *crtc,
 	return 0;
 }
 
-static int vbox_crtc_page_flip(struct drm_crtc *crtc,
-			       struct drm_framebuffer *fb,
-			       struct drm_pending_vblank_event *event,
-			       uint32_t page_flip_flags,
-			       struct drm_modeset_acquire_ctx *ctx)
-{
-	struct vbox_bo *bo = gem_to_vbox_bo(to_vbox_framebuffer(fb)->obj);
-	struct drm_framebuffer *old_fb = CRTC_FB(crtc);
-	unsigned long flags;
-	int rc;
-
-	rc = vbox_bo_pin(bo, TTM_PL_FLAG_VRAM);
-	if (rc) {
-		DRM_WARN("Error %d pinning new fb, out of video mem?\n", rc);
-		return rc;
-	}
-
-	vbox_crtc_set_base_and_mode(crtc, fb, NULL, 0, 0);
-
-	if (old_fb) {
-		bo = gem_to_vbox_bo(to_vbox_framebuffer(old_fb)->obj);
-		vbox_bo_unpin(bo);
-	}
-
-	spin_lock_irqsave(&crtc->dev->event_lock, flags);
-
-	if (event)
-		drm_crtc_send_vblank_event(crtc, event);
-
-	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
-
-	return 0;
-}
-
 static void vbox_crtc_disable(struct drm_crtc *crtc)
 {
 }
@@ -353,7 +319,6 @@ static const struct drm_crtc_funcs vbox_crtc_funcs = {
 	.reset = vbox_crtc_reset,
 	.set_config = drm_crtc_helper_set_config,
 	/* .gamma_set = vbox_crtc_gamma_set, */
-	.page_flip = vbox_crtc_page_flip,
 	.destroy = vbox_crtc_destroy,
 };
 
-- 
2.19.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/15] staging: vboxvideo: Cache mode width, height and crtc panning in vbox_crtc

[Hans de Goede]

When setting a mode we not only pass the mode to the hypervisor,
but also information on how to map / translate input coordinates
for the emulated USB tablet.  This input-mapping may change when
the mode on *another* crtc changes.

This means that sometimes we must do a modeset on other crtc-s then
the one being changed to update the input-mapping. Including crtc-s
which may be disabled inside the guest (shown as a black window
on the host unless closed by the user).

With atomic modesetting the mode-info of disabled crtcs gets zeroed
yet we need it when updating the input-map to avoid resizing the
window as a side effect of a mode_set on another crtc.

This commit adds caching of the mode info into out private vbox_crtc
struct so that we always have the info at hand when we need it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_drv.h  | 20 +++++++++++++++++++
 drivers/staging/vboxvideo/vbox_mode.c | 28 +++++++++++++++------------
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
index 6c52cbd9e91e..34c4d7fc6c8e 100644
--- a/drivers/staging/vboxvideo/vbox_drv.h
+++ b/drivers/staging/vboxvideo/vbox_drv.h
@@ -151,6 +151,26 @@ struct vbox_crtc {
 	bool cursor_enabled;
 	u32 x_hint;
 	u32 y_hint;
+	/*
+	 * When setting a mode we not only pass the mode to the hypervisor,
+	 * but also information on how to map / translate input coordinates
+	 * for the emulated USB tablet.  This input-mapping may change when
+	 * the mode on *another* crtc changes.
+	 *
+	 * This means that sometimes we must do a modeset on other crtc-s then
+	 * the one being changed to update the input-mapping. Including crtc-s
+	 * which may be disabled inside the guest (shown as a black window
+	 * on the host unless closed by the user).
+	 *
+	 * With atomic modesetting the mode-info of disabled crtcs gets zeroed
+	 * yet we need it when updating the input-map to avoid resizing the
+	 * window as a side effect of a mode_set on another crtc. Therefor we
+	 * cache the info of the last mode below.
+	 */
+	u32 width;
+	u32 height;
+	u32 x;
+	u32 y;
 };
 
 struct vbox_encoder {
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index adb6bcf989d1..e85b27f95def 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -48,8 +48,7 @@ static int vbox_cursor_move(struct drm_crtc *crtc, int x, int y);
  * Set a graphics mode.  Poke any required values into registers, do an HGSMI
  * mode set and tell the host we support advanced graphics functions.
  */
-static void vbox_do_modeset(struct drm_crtc *crtc,
-			    const struct drm_display_mode *mode)
+static void vbox_do_modeset(struct drm_crtc *crtc)
 {
 	struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
 	struct vbox_private *vbox;
@@ -58,12 +57,12 @@ static void vbox_do_modeset(struct drm_crtc *crtc,
 	s32 x_offset, y_offset;
 
 	vbox = crtc->dev->dev_private;
-	width = mode->hdisplay ? mode->hdisplay : 640;
-	height = mode->vdisplay ? mode->vdisplay : 480;
+	width = vbox_crtc->width ? vbox_crtc->width : 640;
+	height = vbox_crtc->height ? vbox_crtc->height : 480;
 	bpp = crtc->enabled ? CRTC_FB(crtc)->format->cpp[0] * 8 : 32;
 	pitch = crtc->enabled ? CRTC_FB(crtc)->pitches[0] : width * bpp / 8;
-	x_offset = vbox->single_framebuffer ? crtc->x : vbox_crtc->x_hint;
-	y_offset = vbox->single_framebuffer ? crtc->y : vbox_crtc->y_hint;
+	x_offset = vbox->single_framebuffer ? vbox_crtc->x : vbox_crtc->x_hint;
+	y_offset = vbox->single_framebuffer ? vbox_crtc->y : vbox_crtc->y_hint;
 
 	/*
 	 * This is the old way of setting graphics modes.  It assumed one screen
@@ -82,9 +81,9 @@ static void vbox_do_modeset(struct drm_crtc *crtc,
 		vbox_write_ioport(VBE_DISPI_INDEX_ENABLE, VBE_DISPI_ENABLED);
 		vbox_write_ioport(
 			VBE_DISPI_INDEX_X_OFFSET,
-			vbox_crtc->fb_offset % pitch / bpp * 8 + crtc->x);
+			vbox_crtc->fb_offset % pitch / bpp * 8 + vbox_crtc->x);
 		vbox_write_ioport(VBE_DISPI_INDEX_Y_OFFSET,
-				  vbox_crtc->fb_offset / pitch + crtc->y);
+				  vbox_crtc->fb_offset / pitch + vbox_crtc->y);
 	}
 
 	flags = VBVA_SCREEN_F_ACTIVE;
@@ -93,7 +92,8 @@ static void vbox_do_modeset(struct drm_crtc *crtc,
 	flags |= vbox_crtc->disconnected ? VBVA_SCREEN_F_DISABLED : 0;
 	hgsmi_process_display_info(vbox->guest_pool, vbox_crtc->crtc_id,
 				   x_offset, y_offset,
-				   crtc->x * bpp / 8 + crtc->y * pitch,
+				   vbox_crtc->x * bpp / 8 +
+							vbox_crtc->y * pitch,
 				   pitch, width, height,
 				   vbox_crtc->blanked ? 0 : bpp, flags);
 }
@@ -149,7 +149,7 @@ static void vbox_crtc_dpms(struct drm_crtc *crtc, int mode)
 	}
 
 	mutex_lock(&vbox->hw_mutex);
-	vbox_do_modeset(crtc, &crtc->hwmode);
+	vbox_do_modeset(crtc);
 	mutex_unlock(&vbox->hw_mutex);
 }
 
@@ -232,6 +232,10 @@ static void vbox_crtc_set_base_and_mode(struct drm_crtc *crtc,
 
 	mutex_lock(&vbox->hw_mutex);
 
+	vbox_crtc->width = mode->hdisplay;
+	vbox_crtc->height = mode->vdisplay;
+	vbox_crtc->x = x;
+	vbox_crtc->y = y;
 	vbox_crtc->fb_offset = vbox_bo_gpu_offset(bo);
 
 	/* vbox_do_modeset() checks vbox->single_framebuffer so update it now */
@@ -242,12 +246,12 @@ static void vbox_crtc_set_base_and_mode(struct drm_crtc *crtc,
 				    head) {
 			if (crtci == crtc)
 				continue;
-			vbox_do_modeset(crtci, &crtci->mode);
+			vbox_do_modeset(crtci);
 		}
 	}
 
 	vbox_set_view(crtc);
-	vbox_do_modeset(crtc, mode ? mode : &crtc->mode);
+	vbox_do_modeset(crtc);
 
 	if (mode)
 		hgsmi_update_input_mapping(vbox->guest_pool, 0, 0,
-- 
2.19.0

[PATCH 05/15] staging: vboxvideo: Atomic phase 1: convert cursor to universal plane

[Hans de Goede]

In preparation for atomic conversion, let's use the transitional atomic
helpers drm_plane_helper_update/disable.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_drv.h  |   5 -
 drivers/staging/vboxvideo/vbox_mode.c | 386 +++++++++++++-------------
 2 files changed, 186 insertions(+), 205 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
index 34c4d7fc6c8e..7fc668ff4465 100644
--- a/drivers/staging/vboxvideo/vbox_drv.h
+++ b/drivers/staging/vboxvideo/vbox_drv.h
@@ -116,11 +116,6 @@ struct vbox_private {
 	 * encompassing all screen ones or is the fbdev console active?
 	 */
 	bool single_framebuffer;
-	u32 cursor_width;
-	u32 cursor_height;
-	u32 cursor_hot_x;
-	u32 cursor_hot_y;
-	size_t cursor_data_size;
 	u8 cursor_data[CURSOR_DATA_SIZE];
 };
 
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index e85b27f95def..75e112d33cf0 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -34,16 +34,12 @@
 #include <linux/export.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "vbox_drv.h"
 #include "vboxvideo.h"
 #include "hgsmi_channels.h"
 
-static int vbox_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv,
-			    u32 handle, u32 width, u32 height,
-			    s32 hot_x, s32 hot_y);
-static int vbox_cursor_move(struct drm_crtc *crtc, int x, int y);
-
 /**
  * Set a graphics mode.  Poke any required values into registers, do an HGSMI
  * mode set and tell the host we support advanced graphics functions.
@@ -318,14 +314,166 @@ static void vbox_crtc_destroy(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_funcs vbox_crtc_funcs = {
-	.cursor_move = vbox_cursor_move,
-	.cursor_set2 = vbox_cursor_set2,
 	.reset = vbox_crtc_reset,
 	.set_config = drm_crtc_helper_set_config,
 	/* .gamma_set = vbox_crtc_gamma_set, */
 	.destroy = vbox_crtc_destroy,
 };
 
+static int vbox_cursor_atomic_check(struct drm_plane *plane,
+				    struct drm_plane_state *new_state)
+{
+	u32 width = new_state->crtc_w;
+	u32 height = new_state->crtc_h;
+
+	if (!new_state->fb)
+		return 0;
+
+	if (width > VBOX_MAX_CURSOR_WIDTH || height > VBOX_MAX_CURSOR_HEIGHT ||
+	    width == 0 || height == 0)
+		return -EINVAL;
+
+	return 0;
+}
+
+/**
+ * Copy the ARGB image and generate the mask, which is needed in case the host
+ * does not support ARGB cursors.  The mask is a 1BPP bitmap with the bit set
+ * if the corresponding alpha value in the ARGB image is greater than 0xF0.
+ */
+static void copy_cursor_image(u8 *src, u8 *dst, u32 width, u32 height,
+			      size_t mask_size)
+{
+	size_t line_size = (width + 7) / 8;
+	u32 i, j;
+
+	memcpy(dst + mask_size, src, width * height * 4);
+	for (i = 0; i < height; ++i)
+		for (j = 0; j < width; ++j)
+			if (((u32 *)src)[i * width + j] > 0xf0000000)
+				dst[i * line_size + j / 8] |= (0x80 >> (j % 8));
+}
+
+static void vbox_cursor_atomic_update(struct drm_plane *plane,
+				      struct drm_plane_state *old_state)
+{
+	struct vbox_private *vbox =
+		container_of(plane->dev, struct vbox_private, ddev);
+	struct vbox_crtc *vbox_crtc = to_vbox_crtc(plane->state->crtc);
+	struct drm_framebuffer *fb = plane->state->fb;
+	struct vbox_bo *bo = gem_to_vbox_bo(to_vbox_framebuffer(fb)->obj);
+	u32 width = plane->state->crtc_w;
+	u32 height = plane->state->crtc_h;
+	size_t data_size, mask_size;
+	u32 flags;
+	u8 *src;
+
+	/*
+	 * VirtualBox uses the host windowing system to draw the cursor so
+	 * moves are a no-op, we only need to upload new cursor sprites.
+	 */
+	if (fb == old_state->fb)
+		return;
+
+	mutex_lock(&vbox->hw_mutex);
+
+	vbox_crtc->cursor_enabled = true;
+
+	/* pinning is done in prepare/cleanup framebuffer */
+	src = vbox_bo_kmap(bo);
+	if (IS_ERR(src)) {
+		DRM_WARN("Could not kmap cursor bo, skipping update\n");
+		return;
+	}
+
+	/*
+	 * The mask must be calculated based on the alpha
+	 * channel, one bit per ARGB word, and must be 32-bit
+	 * padded.
+	 */
+	mask_size = ((width + 7) / 8 * height + 3) & ~3;
+	data_size = width * height * 4 + mask_size;
+
+	copy_cursor_image(src, vbox->cursor_data, width, height, mask_size);
+	vbox_bo_kunmap(bo);
+
+	flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE |
+		VBOX_MOUSE_POINTER_ALPHA;
+	hgsmi_update_pointer_shape(vbox->guest_pool, flags,
+				   min_t(u32, max(fb->hot_x, 0), width),
+				   min_t(u32, max(fb->hot_y, 0), height),
+				   width, height, vbox->cursor_data, data_size);
+
+	mutex_unlock(&vbox->hw_mutex);
+}
+
+void vbox_cursor_atomic_disable(struct drm_plane *plane,
+				struct drm_plane_state *old_state)
+{
+	struct vbox_private *vbox =
+		container_of(plane->dev, struct vbox_private, ddev);
+	struct vbox_crtc *vbox_crtc = to_vbox_crtc(old_state->crtc);
+	bool cursor_enabled = false;
+	struct drm_crtc *crtci;
+
+	mutex_lock(&vbox->hw_mutex);
+
+	vbox_crtc->cursor_enabled = false;
+
+	list_for_each_entry(crtci, &vbox->ddev.mode_config.crtc_list, head) {
+		if (to_vbox_crtc(crtci)->cursor_enabled)
+			cursor_enabled = true;
+	}
+
+	if (!cursor_enabled)
+		hgsmi_update_pointer_shape(vbox->guest_pool, 0, 0, 0,
+					   0, 0, NULL, 0);
+
+	mutex_unlock(&vbox->hw_mutex);
+}
+
+static int vbox_cursor_prepare_fb(struct drm_plane *plane,
+				  struct drm_plane_state *new_state)
+{
+	struct vbox_bo *bo;
+
+	if (!new_state->fb)
+		return 0;
+
+	bo = gem_to_vbox_bo(to_vbox_framebuffer(new_state->fb)->obj);
+	return vbox_bo_pin(bo, TTM_PL_FLAG_SYSTEM);
+}
+
+static void vbox_cursor_cleanup_fb(struct drm_plane *plane,
+				   struct drm_plane_state *old_state)
+{
+	struct vbox_bo *bo;
+
+	if (!plane->state->fb)
+		return;
+
+	bo = gem_to_vbox_bo(to_vbox_framebuffer(plane->state->fb)->obj);
+	vbox_bo_unpin(bo);
+}
+
+static const uint32_t vbox_cursor_plane_formats[] = {
+	DRM_FORMAT_ARGB8888,
+};
+
+static const struct drm_plane_helper_funcs vbox_cursor_helper_funcs = {
+	.atomic_check	= vbox_cursor_atomic_check,
+	.atomic_update	= vbox_cursor_atomic_update,
+	.atomic_disable	= vbox_cursor_atomic_disable,
+	.prepare_fb	= vbox_cursor_prepare_fb,
+	.cleanup_fb	= vbox_cursor_cleanup_fb,
+};
+
+static const struct drm_plane_funcs vbox_cursor_plane_funcs = {
+	.update_plane	= drm_plane_helper_update,
+	.disable_plane	= drm_plane_helper_disable,
+	.destroy	= drm_primary_helper_destroy,
+};
+
 static const uint32_t vbox_primary_plane_formats[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
@@ -352,6 +500,11 @@ static struct drm_plane *vbox_create_plane(struct vbox_private *vbox,
 		funcs = &vbox_primary_plane_funcs;
 		formats = vbox_primary_plane_formats;
 		num_formats = ARRAY_SIZE(vbox_primary_plane_formats);
+	} else if (type == DRM_PLANE_TYPE_CURSOR) {
+		funcs = &vbox_cursor_plane_funcs;
+		formats = vbox_cursor_plane_formats;
+		helper_funcs = &vbox_cursor_helper_funcs;
+		num_formats = ARRAY_SIZE(vbox_cursor_plane_formats);
 	} else {
 		return ERR_PTR(-EINVAL);
 	}
@@ -379,10 +532,17 @@ static struct vbox_crtc *vbox_crtc_init(struct drm_device *dev, unsigned int i)
 {
 	struct vbox_private *vbox =
 		container_of(dev, struct vbox_private, ddev);
+	struct drm_plane *cursor = NULL;
 	struct vbox_crtc *vbox_crtc;
 	struct drm_plane *primary;
+	u32 caps = 0;
 	int ret;
 
+	ret = hgsmi_query_conf(vbox->guest_pool,
+			       VBOX_VBVA_CONF32_CURSOR_CAPABILITIES, &caps);
+	if (ret)
+		return ERR_PTR(ret);
+
 	vbox_crtc = kzalloc(sizeof(*vbox_crtc), GFP_KERNEL);
 	if (!vbox_crtc)
 		return ERR_PTR(-ENOMEM);
@@ -393,18 +553,33 @@ static struct vbox_crtc *vbox_crtc_init(struct drm_device *dev, unsigned int i)
 		goto free_mem;
 	}
 
+	if ((caps & VBOX_VBVA_CURSOR_CAPABILITY_HARDWARE)) {
+		cursor = vbox_create_plane(vbox, 1 << i, DRM_PLANE_TYPE_CURSOR);
+		if (IS_ERR(cursor)) {
+			ret = PTR_ERR(cursor);
+			goto clean_primary;
+		}
+	} else {
+		DRM_WARN("VirtualBox host is too old, no cursor support\n");
+	}
+
 	vbox_crtc->crtc_id = i;
 
-	ret = drm_crtc_init_with_planes(dev, &vbox_crtc->base, primary, NULL,
+	ret = drm_crtc_init_with_planes(dev, &vbox_crtc->base, primary, cursor,
 					&vbox_crtc_funcs, NULL);
 	if (ret)
-		goto clean_primary;
+		goto clean_cursor;
 
 	drm_mode_crtc_set_gamma_size(&vbox_crtc->base, 256);
 	drm_crtc_helper_add(&vbox_crtc->base, &vbox_crtc_helper_funcs);
 
 	return vbox_crtc;
 
+clean_cursor:
+	if (cursor) {
+		drm_plane_cleanup(cursor);
+		kfree(cursor);
+	}
 clean_primary:
 	drm_plane_cleanup(primary);
 	kfree(primary);
@@ -721,13 +896,12 @@ int vbox_mode_init(struct vbox_private *vbox)
 	drm_mode_config_init(dev);
 
 	dev->mode_config.funcs = (void *)&vbox_mode_funcs;
-	dev->mode_config.min_width = 64;
-	dev->mode_config.min_height = 64;
+	dev->mode_config.min_width = 0;
+	dev->mode_config.min_height = 0;
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.max_width = VBE_DISPI_MAX_XRES;
 	dev->mode_config.max_height = VBE_DISPI_MAX_YRES;
 
-	/* vbox_cursor_init(dev); */
 	for (i = 0; i < vbox->num_crtcs; ++i) {
 		vbox_crtc = vbox_crtc_init(dev, i);
 		if (IS_ERR(vbox_crtc)) {
@@ -754,192 +928,4 @@ int vbox_mode_init(struct vbox_private *vbox)
 void vbox_mode_fini(struct vbox_private *vbox)
 {
 	drm_mode_config_cleanup(&vbox->ddev);
-	/* vbox_cursor_fini(dev); */
-}
-
-/**
- * Copy the ARGB image and generate the mask, which is needed in case the host
- * does not support ARGB cursors.  The mask is a 1BPP bitmap with the bit set
- * if the corresponding alpha value in the ARGB image is greater than 0xF0.
- */
-static void copy_cursor_image(u8 *src, u8 *dst, u32 width, u32 height,
-			      size_t mask_size)
-{
-	size_t line_size = (width + 7) / 8;
-	u32 i, j;
-
-	memcpy(dst + mask_size, src, width * height * 4);
-	for (i = 0; i < height; ++i)
-		for (j = 0; j < width; ++j)
-			if (((u32 *)src)[i * width + j] > 0xf0000000)
-				dst[i * line_size + j / 8] |= (0x80 >> (j % 8));
-}
-
-static int vbox_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv,
-			    u32 handle, u32 width, u32 height,
-			    s32 hot_x, s32 hot_y)
-{
-	struct vbox_private *vbox = crtc->dev->dev_private;
-	struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
-	struct ttm_bo_kmap_obj uobj_map;
-	size_t data_size, mask_size;
-	struct drm_gem_object *obj;
-	u32 flags, caps = 0;
-	struct vbox_bo *bo;
-	bool src_isiomem;
-	u8 *dst = NULL;
-	u8 *src;
-	int ret;
-
-	/*
-	 * Re-set this regularly as in 5.0.20 and earlier the information was
-	 * lost on save and restore.
-	 */
-	hgsmi_update_input_mapping(vbox->guest_pool, 0, 0,
-				   vbox->input_mapping_width,
-				   vbox->input_mapping_height);
-	if (!handle) {
-		bool cursor_enabled = false;
-		struct drm_crtc *crtci;
-
-		/* Hide cursor. */
-		vbox_crtc->cursor_enabled = false;
-		list_for_each_entry(crtci, &vbox->ddev.mode_config.crtc_list,
-				    head) {
-			if (to_vbox_crtc(crtci)->cursor_enabled)
-				cursor_enabled = true;
-		}
-
-		if (!cursor_enabled)
-			hgsmi_update_pointer_shape(vbox->guest_pool, 0, 0, 0,
-						   0, 0, NULL, 0);
-		return 0;
-	}
-
-	vbox_crtc->cursor_enabled = true;
-
-	if (width > VBOX_MAX_CURSOR_WIDTH || height > VBOX_MAX_CURSOR_HEIGHT ||
-	    width == 0 || height == 0)
-		return -EINVAL;
-
-	ret = hgsmi_query_conf(vbox->guest_pool,
-			       VBOX_VBVA_CONF32_CURSOR_CAPABILITIES, &caps);
-	if (ret)
-		return ret;
-
-	if (!(caps & VBOX_VBVA_CURSOR_CAPABILITY_HARDWARE)) {
-		/*
-		 * -EINVAL means cursor_set2() not supported, -EAGAIN means
-		 * retry at once.
-		 */
-		return -EBUSY;
-	}
-
-	obj = drm_gem_object_lookup(file_priv, handle);
-	if (!obj) {
-		DRM_ERROR("Cannot find cursor object %x for crtc\n", handle);
-		return -ENOENT;
-	}
-
-	bo = gem_to_vbox_bo(obj);
-	ret = vbox_bo_reserve(bo, false);
-	if (ret)
-		goto out_unref_obj;
-
-	/*
-	 * The mask must be calculated based on the alpha
-	 * channel, one bit per ARGB word, and must be 32-bit
-	 * padded.
-	 */
-	mask_size = ((width + 7) / 8 * height + 3) & ~3;
-	data_size = width * height * 4 + mask_size;
-	vbox->cursor_hot_x = min_t(u32, max(hot_x, 0), width);
-	vbox->cursor_hot_y = min_t(u32, max(hot_y, 0), height);
-	vbox->cursor_width = width;
-	vbox->cursor_height = height;
-	vbox->cursor_data_size = data_size;
-	dst = vbox->cursor_data;
-
-	ret = ttm_bo_kmap(&bo->bo, 0, bo->bo.num_pages, &uobj_map);
-	if (ret) {
-		vbox->cursor_data_size = 0;
-		goto out_unreserve_bo;
-	}
-
-	src = ttm_kmap_obj_virtual(&uobj_map, &src_isiomem);
-	if (src_isiomem) {
-		DRM_ERROR("src cursor bo not in main memory\n");
-		ret = -EIO;
-		goto out_unmap_bo;
-	}
-
-	copy_cursor_image(src, dst, width, height, mask_size);
-
-	flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE |
-		VBOX_MOUSE_POINTER_ALPHA;
-	ret = hgsmi_update_pointer_shape(vbox->guest_pool, flags,
-					 vbox->cursor_hot_x, vbox->cursor_hot_y,
-					 width, height, dst, data_size);
-out_unmap_bo:
-	ttm_bo_kunmap(&uobj_map);
-out_unreserve_bo:
-	vbox_bo_unreserve(bo);
-out_unref_obj:
-	drm_gem_object_put_unlocked(obj);
-
-	return ret;
-}
-
-static int vbox_cursor_move(struct drm_crtc *crtc, int x, int y)
-{
-	struct vbox_private *vbox = crtc->dev->dev_private;
-	u32 flags = VBOX_MOUSE_POINTER_VISIBLE |
-	    VBOX_MOUSE_POINTER_SHAPE | VBOX_MOUSE_POINTER_ALPHA;
-	s32 crtc_x =
-	    vbox->single_framebuffer ? crtc->x : to_vbox_crtc(crtc)->x_hint;
-	s32 crtc_y =
-	    vbox->single_framebuffer ? crtc->y : to_vbox_crtc(crtc)->y_hint;
-	u32 host_x, host_y;
-	u32 hot_x = 0;
-	u32 hot_y = 0;
-	int ret;
-
-	/*
-	 * We compare these to unsigned later and don't
-	 * need to handle negative.
-	 */
-	if (x + crtc_x < 0 || y + crtc_y < 0 || vbox->cursor_data_size == 0)
-		return 0;
-
-	ret = hgsmi_cursor_position(vbox->guest_pool, true, x + crtc_x,
-				    y + crtc_y, &host_x, &host_y);
-
-	/*
-	 * The only reason we have vbox_cursor_move() is that some older clients
-	 * might use DRM_IOCTL_MODE_CURSOR instead of DRM_IOCTL_MODE_CURSOR2 and
-	 * use DRM_MODE_CURSOR_MOVE to set the hot-spot.
-	 *
-	 * However VirtualBox 5.0.20 and earlier has a bug causing it to return
-	 * 0,0 as host cursor location after a save and restore.
-	 *
-	 * To work around this we ignore a 0, 0 return, since missing the odd
-	 * time when it legitimately happens is not going to hurt much.
-	 */
-	if (ret || (host_x == 0 && host_y == 0))
-		return ret;
-
-	if (x + crtc_x < host_x)
-		hot_x = min(host_x - x - crtc_x, vbox->cursor_width);
-	if (y + crtc_y < host_y)
-		hot_y = min(host_y - y - crtc_y, vbox->cursor_height);
-
-	if (hot_x == vbox->cursor_hot_x && hot_y == vbox->cursor_hot_y)
-		return 0;
-
-	vbox->cursor_hot_x = hot_x;
-	vbox->cursor_hot_y = hot_y;
-
-	return hgsmi_update_pointer_shape(vbox->guest_pool, flags,
-			hot_x, hot_y, vbox->cursor_width, vbox->cursor_height,
-			vbox->cursor_data, vbox->cursor_data_size);
 }
-- 
2.19.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/15] staging: vboxvideo: Atomic phase 1: Use drm_plane_helpers for primary plane

[Hans de Goede]

Use drm_plane_helpers for the primary plane and replace our custom
mode_set callback with drm_helper_crtc_mode_set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_mode.c | 116 ++++++++++++++++++++------
 1 file changed, 90 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index 75e112d33cf0..e560e36e7953 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -257,50 +257,48 @@ static void vbox_crtc_set_base_and_mode(struct drm_crtc *crtc,
 	mutex_unlock(&vbox->hw_mutex);
 }
 
-static int vbox_crtc_mode_set(struct drm_crtc *crtc,
-			      struct drm_display_mode *mode,
-			      struct drm_display_mode *adjusted_mode,
-			      int x, int y, struct drm_framebuffer *old_fb)
+static void vbox_crtc_disable(struct drm_crtc *crtc)
 {
-	struct drm_framebuffer *new_fb = CRTC_FB(crtc);
-	struct vbox_bo *bo = gem_to_vbox_bo(to_vbox_framebuffer(new_fb)->obj);
-	int ret;
-
-	ret = vbox_bo_pin(bo, TTM_PL_FLAG_VRAM);
-	if (ret) {
-		DRM_WARN("Error %d pinning new fb, out of video mem?\n", ret);
-		return ret;
-	}
-
-	vbox_crtc_set_base_and_mode(crtc, new_fb, mode, x, y);
-
-	if (old_fb) {
-		bo = gem_to_vbox_bo(to_vbox_framebuffer(old_fb)->obj);
-		vbox_bo_unpin(bo);
-	}
+}
 
-	return 0;
+static void vbox_crtc_prepare(struct drm_crtc *crtc)
+{
 }
 
-static void vbox_crtc_disable(struct drm_crtc *crtc)
+static void vbox_crtc_commit(struct drm_crtc *crtc)
 {
 }
 
-static void vbox_crtc_prepare(struct drm_crtc *crtc)
+static void vbox_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
+	/* We always set the mode when we set the fb/base */
 }
 
-static void vbox_crtc_commit(struct drm_crtc *crtc)
+static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
+				   struct drm_crtc_state *old_crtc_state)
 {
+	struct drm_pending_vblank_event *event;
+	unsigned long flags;
+
+	if (crtc->state && crtc->state->event) {
+		event = crtc->state->event;
+		crtc->state->event = NULL;
+
+		spin_lock_irqsave(&crtc->dev->event_lock, flags);
+		drm_crtc_send_vblank_event(crtc, event);
+		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+	}
 }
 
 static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
 	.dpms = vbox_crtc_dpms,
 	.mode_fixup = vbox_crtc_mode_fixup,
-	.mode_set = vbox_crtc_mode_set,
+	.mode_set = drm_helper_crtc_mode_set,
+	.mode_set_nofb = vbox_crtc_mode_set_nofb,
 	.disable = vbox_crtc_disable,
 	.prepare = vbox_crtc_prepare,
 	.commit = vbox_crtc_commit,
+	.atomic_flush = vbox_crtc_atomic_flush,
 };
 
 static void vbox_crtc_reset(struct drm_crtc *crtc)
@@ -320,6 +318,63 @@ static const struct drm_crtc_funcs vbox_crtc_funcs = {
 	.destroy = vbox_crtc_destroy,
 };
 
+static int vbox_primary_atomic_check(struct drm_plane *plane,
+				     struct drm_plane_state *new_state)
+{
+	return 0;
+}
+
+static void vbox_primary_atomic_update(struct drm_plane *plane,
+				       struct drm_plane_state *old_state)
+{
+	struct drm_crtc *crtc = plane->state->crtc;
+	struct drm_framebuffer *fb = plane->state->fb;
+
+	vbox_crtc_set_base_and_mode(crtc, fb, &crtc->state->mode,
+				    plane->state->src_x >> 16,
+				    plane->state->src_y >> 16);
+}
+
+void vbox_primary_atomic_disable(struct drm_plane *plane,
+				 struct drm_plane_state *old_state)
+{
+	struct drm_crtc *crtc = old_state->crtc;
+
+	/* vbox_do_modeset checks plane->state->fb and will disable if NULL */
+	vbox_crtc_set_base_and_mode(crtc, old_state->fb, &crtc->state->mode,
+				    old_state->src_x >> 16,
+				    old_state->src_y >> 16);
+}
+
+static int vbox_primary_prepare_fb(struct drm_plane *plane,
+				   struct drm_plane_state *new_state)
+{
+	struct vbox_bo *bo;
+	int ret;
+
+	if (!new_state->fb)
+		return 0;
+
+	bo = gem_to_vbox_bo(to_vbox_framebuffer(new_state->fb)->obj);
+	ret = vbox_bo_pin(bo, TTM_PL_FLAG_VRAM);
+	if (ret)
+		DRM_WARN("Error %d pinning new fb, out of video mem?\n", ret);
+
+	return ret;
+}
+
+static void vbox_primary_cleanup_fb(struct drm_plane *plane,
+				    struct drm_plane_state *old_state)
+{
+	struct vbox_bo *bo;
+
+	if (!old_state->fb)
+		return;
+
+	bo = gem_to_vbox_bo(to_vbox_framebuffer(old_state->fb)->obj);
+	vbox_bo_unpin(bo);
+}
+
 static int vbox_cursor_atomic_check(struct drm_plane *plane,
 				    struct drm_plane_state *new_state)
 {
@@ -479,8 +534,16 @@ static const uint32_t vbox_primary_plane_formats[] = {
 	DRM_FORMAT_ARGB8888,
 };
 
+static const struct drm_plane_helper_funcs vbox_primary_helper_funcs = {
+	.atomic_check = vbox_primary_atomic_check,
+	.atomic_update = vbox_primary_atomic_update,
+	.atomic_disable = vbox_primary_atomic_disable,
+	.prepare_fb = vbox_primary_prepare_fb,
+	.cleanup_fb = vbox_primary_cleanup_fb,
+};
+
 static const struct drm_plane_funcs vbox_primary_plane_funcs = {
-	.update_plane	= drm_primary_helper_update,
+	.update_plane	= drm_plane_helper_update,
 	.disable_plane	= drm_primary_helper_disable,
 	.destroy	= drm_primary_helper_destroy,
 };
@@ -499,6 +562,7 @@ static struct drm_plane *vbox_create_plane(struct vbox_private *vbox,
 	if (type == DRM_PLANE_TYPE_PRIMARY) {
 		funcs = &vbox_primary_plane_funcs;
 		formats = vbox_primary_plane_formats;
+		helper_funcs = &vbox_primary_helper_funcs;
 		num_formats = ARRAY_SIZE(vbox_primary_plane_formats);
 	} else if (type == DRM_PLANE_TYPE_CURSOR) {
 		funcs = &vbox_cursor_plane_funcs;
-- 
2.19.0

[PATCH 07/15] staging: vboxvideo: Atomic phase 2: Wire up state object handlers

[Hans de Goede]

Wire up state object handlers for the crtc-s and the planes, call
drm_mode_config_reset() after creating all the crtc-s and encoders and
remove the legacy drm_helper_disable_unused_functions() call.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_fb.c   |  3 ---
 drivers/staging/vboxvideo/vbox_mode.c | 15 ++++++++++-----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_fb.c b/drivers/staging/vboxvideo/vbox_fb.c
index b8b42f9aafae..8a9d56762659 100644
--- a/drivers/staging/vboxvideo/vbox_fb.c
+++ b/drivers/staging/vboxvideo/vbox_fb.c
@@ -216,9 +216,6 @@ int vbox_fbdev_init(struct vbox_private *vbox)
 	if (ret)
 		goto err_fini;
 
-	/* disable all the possible outputs/crtcs before entering KMS mode */
-	drm_helper_disable_unused_functions(dev);
-
 	ret = drm_fb_helper_initial_config(&fbdev->helper, 32);
 	if (ret)
 		goto err_fini;
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index e560e36e7953..cb897b672752 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -301,10 +301,6 @@ static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
 	.atomic_flush = vbox_crtc_atomic_flush,
 };
 
-static void vbox_crtc_reset(struct drm_crtc *crtc)
-{
-}
-
 static void vbox_crtc_destroy(struct drm_crtc *crtc)
 {
 	drm_crtc_cleanup(crtc);
@@ -312,10 +308,12 @@ static void vbox_crtc_destroy(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_funcs vbox_crtc_funcs = {
-	.reset = vbox_crtc_reset,
 	.set_config = drm_crtc_helper_set_config,
 	/* .gamma_set = vbox_crtc_gamma_set, */
 	.destroy = vbox_crtc_destroy,
+	.reset = drm_atomic_helper_crtc_reset,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
 static int vbox_primary_atomic_check(struct drm_plane *plane,
@@ -527,6 +525,9 @@ static const struct drm_plane_funcs vbox_cursor_plane_funcs = {
 	.update_plane	= drm_plane_helper_update,
 	.disable_plane	= drm_plane_helper_disable,
 	.destroy	= drm_primary_helper_destroy,
+	.reset		= drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
 };
 
 static const uint32_t vbox_primary_plane_formats[] = {
@@ -546,6 +547,9 @@ static const struct drm_plane_funcs vbox_primary_plane_funcs = {
 	.update_plane	= drm_plane_helper_update,
 	.disable_plane	= drm_primary_helper_disable,
 	.destroy	= drm_primary_helper_destroy,
+	.reset		= drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
 };
 
 static struct drm_plane *vbox_create_plane(struct vbox_private *vbox,
@@ -982,6 +986,7 @@ int vbox_mode_init(struct vbox_private *vbox)
 			goto err_drm_mode_cleanup;
 	}
 
+	drm_mode_config_reset(dev);
 	return 0;
 
 err_drm_mode_cleanup:
-- 
2.19.0

[PATCH 08/15] staging: vboxvideo: Atomic phase 2: Stop using plane->fb and crtc->*

[Hans de Goede]

Once we are fully atomic plane->fb will always be NULL and we also
should not access things like crtc->enabled and crt->[hw]mode.

Now that we've wired up the state object handlers, we always have a
plane_state and crtc_state so change the code referencing plane->fb and
crtc->* to use the data from the plane_state and crt_state instead.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_drv.h  |  1 -
 drivers/staging/vboxvideo/vbox_main.c | 16 +++++++++++-----
 drivers/staging/vboxvideo/vbox_mode.c | 15 +++++++--------
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
index 7fc668ff4465..fccb3851d6a3 100644
--- a/drivers/staging/vboxvideo/vbox_drv.h
+++ b/drivers/staging/vboxvideo/vbox_drv.h
@@ -199,7 +199,6 @@ int vbox_mode_init(struct vbox_private *vbox);
 void vbox_mode_fini(struct vbox_private *vbox);
 
 #define DRM_MODE_FB_CMD drm_mode_fb_cmd2
-#define CRTC_FB(crtc) ((crtc)->primary->fb)
 
 void vbox_enable_accel(struct vbox_private *vbox);
 void vbox_disable_accel(struct vbox_private *vbox);
diff --git a/drivers/staging/vboxvideo/vbox_main.c b/drivers/staging/vboxvideo/vbox_main.c
index 95100c5976e4..3b82d483ab51 100644
--- a/drivers/staging/vboxvideo/vbox_main.c
+++ b/drivers/staging/vboxvideo/vbox_main.c
@@ -102,24 +102,30 @@ void vbox_framebuffer_dirty_rectangles(struct drm_framebuffer *fb,
 				       unsigned int num_rects)
 {
 	struct vbox_private *vbox = fb->dev->dev_private;
+	struct drm_display_mode *mode;
 	struct drm_crtc *crtc;
+	int crtc_x, crtc_y;
 	unsigned int i;
 
 	mutex_lock(&vbox->hw_mutex);
 	list_for_each_entry(crtc, &fb->dev->mode_config.crtc_list, head) {
-		if (CRTC_FB(crtc) != fb)
+		if (crtc->primary->state->fb != fb)
 			continue;
 
+		mode = &crtc->state->mode;
+		crtc_x = crtc->primary->state->src_x >> 16;
+		crtc_y = crtc->primary->state->src_y >> 16;
+
 		vbox_enable_accel(vbox);
 
 		for (i = 0; i < num_rects; ++i) {
 			struct vbva_cmd_hdr cmd_hdr;
 			unsigned int crtc_id = to_vbox_crtc(crtc)->crtc_id;
 
-			if ((rects[i].x1 > crtc->x + crtc->hwmode.hdisplay) ||
-			    (rects[i].y1 > crtc->y + crtc->hwmode.vdisplay) ||
-			    (rects[i].x2 < crtc->x) ||
-			    (rects[i].y2 < crtc->y))
+			if ((rects[i].x1 > crtc_x + mode->hdisplay) ||
+			    (rects[i].y1 > crtc_y + mode->vdisplay) ||
+			    (rects[i].x2 < crtc_x) ||
+			    (rects[i].y2 < crtc_y))
 				continue;
 
 			cmd_hdr.x = (s16)rects[i].x1;
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index cb897b672752..54e6aac784f7 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -46,6 +46,7 @@
  */
 static void vbox_do_modeset(struct drm_crtc *crtc)
 {
+	struct drm_framebuffer *fb = crtc->primary->state->fb;
 	struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
 	struct vbox_private *vbox;
 	int width, height, bpp, pitch;
@@ -55,8 +56,8 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
 	vbox = crtc->dev->dev_private;
 	width = vbox_crtc->width ? vbox_crtc->width : 640;
 	height = vbox_crtc->height ? vbox_crtc->height : 480;
-	bpp = crtc->enabled ? CRTC_FB(crtc)->format->cpp[0] * 8 : 32;
-	pitch = crtc->enabled ? CRTC_FB(crtc)->pitches[0] : width * bpp / 8;
+	bpp = fb ? fb->format->cpp[0] * 8 : 32;
+	pitch = fb ? fb->pitches[0] : width * bpp / 8;
 	x_offset = vbox->single_framebuffer ? vbox_crtc->x : vbox_crtc->x_hint;
 	y_offset = vbox->single_framebuffer ? vbox_crtc->y : vbox_crtc->y_hint;
 
@@ -66,14 +67,13 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
 	 * VirtualBox, certain parts of the code still assume that the first
 	 * screen is programmed this way, so try to fake it.
 	 */
-	if (vbox_crtc->crtc_id == 0 && crtc->enabled &&
+	if (vbox_crtc->crtc_id == 0 && fb &&
 	    vbox_crtc->fb_offset / pitch < 0xffff - crtc->y &&
 	    vbox_crtc->fb_offset % (bpp / 8) == 0) {
 		vbox_write_ioport(VBE_DISPI_INDEX_XRES, width);
 		vbox_write_ioport(VBE_DISPI_INDEX_YRES, height);
 		vbox_write_ioport(VBE_DISPI_INDEX_VIRT_WIDTH, pitch * 8 / bpp);
-		vbox_write_ioport(VBE_DISPI_INDEX_BPP,
-				  CRTC_FB(crtc)->format->cpp[0] * 8);
+		vbox_write_ioport(VBE_DISPI_INDEX_BPP, bpp);
 		vbox_write_ioport(VBE_DISPI_INDEX_ENABLE, VBE_DISPI_ENABLED);
 		vbox_write_ioport(
 			VBE_DISPI_INDEX_X_OFFSET,
@@ -83,8 +83,7 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
 	}
 
 	flags = VBVA_SCREEN_F_ACTIVE;
-	flags |= (crtc->enabled && !vbox_crtc->blanked) ?
-		 0 : VBVA_SCREEN_F_BLANK;
+	flags |= (fb && !vbox_crtc->blanked) ? 0 : VBVA_SCREEN_F_BLANK;
 	flags |= vbox_crtc->disconnected ? VBVA_SCREEN_F_DISABLED : 0;
 	hgsmi_process_display_info(vbox->guest_pool, vbox_crtc->crtc_id,
 				   x_offset, y_offset,
@@ -176,7 +175,7 @@ static bool vbox_set_up_input_mapping(struct vbox_private *vbox)
 	 * Same fall-back if this is the fbdev frame-buffer.
 	 */
 	list_for_each_entry(crtci, &vbox->ddev.mode_config.crtc_list, head) {
-		fb = CRTC_FB(crtci);
+		fb = crtci->primary->state->fb;
 		if (!fb)
 			continue;
 
-- 
2.19.0

[PATCH 09/15] staging: vboxvideo: Atomic phase 3: Switch last bits over to atomic

[Hans de Goede]

Now that the state objects are wired up, we can:

1) Move to the final atomic handlers
2) Wire up atomic set_config helper
3) Switch to drm_mode_config_helper_suspend/resume for suspend/resume
4) Enable atomic modesetting ioctl

This is all done in one commit because doing this piecemeal leads to
an intermediate state which triggers WARN_ONs in the atomic code because
e.g. plane->fb is still being set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/TODO        |  1 -
 drivers/staging/vboxvideo/vbox_drv.c  | 40 +++++----------------------
 drivers/staging/vboxvideo/vbox_mode.c | 35 +++++++----------------
 3 files changed, 17 insertions(+), 59 deletions(-)

diff --git a/drivers/staging/vboxvideo/TODO b/drivers/staging/vboxvideo/TODO
index 468eea856ca6..2e0f99c3f10c 100644
--- a/drivers/staging/vboxvideo/TODO
+++ b/drivers/staging/vboxvideo/TODO
@@ -1,5 +1,4 @@
 TODO:
--Move the driver over to the atomic API
 -Get a full review from the drm-maintainers on dri-devel done on this driver
 -Extend this TODO with the results of that review
 
diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c
index c6a53b0ad66c..c3e14107da0d 100644
--- a/drivers/staging/vboxvideo/vbox_drv.c
+++ b/drivers/staging/vboxvideo/vbox_drv.c
@@ -132,35 +132,16 @@ static void vbox_pci_remove(struct pci_dev *pdev)
 	drm_dev_put(&vbox->ddev);
 }
 
-static int vbox_drm_freeze(struct vbox_private *vbox)
-{
-	drm_kms_helper_poll_disable(&vbox->ddev);
-
-	pci_save_state(vbox->ddev.pdev);
-
-	drm_fb_helper_set_suspend_unlocked(&vbox->fbdev->helper, true);
-
-	return 0;
-}
-
-static int vbox_drm_thaw(struct vbox_private *vbox)
-{
-	drm_mode_config_reset(&vbox->ddev);
-	drm_helper_resume_force_mode(&vbox->ddev);
-	drm_fb_helper_set_suspend_unlocked(&vbox->fbdev->helper, false);
-
-	return 0;
-}
-
 static int vbox_pm_suspend(struct device *dev)
 {
 	struct vbox_private *vbox = dev_get_drvdata(dev);
 	int error;
 
-	error = vbox_drm_freeze(vbox);
+	error = drm_mode_config_helper_suspend(&vbox->ddev);
 	if (error)
 		return error;
 
+	pci_save_state(vbox->ddev.pdev);
 	pci_disable_device(vbox->ddev.pdev);
 	pci_set_power_state(vbox->ddev.pdev, PCI_D3hot);
 
@@ -170,39 +151,32 @@ static int vbox_pm_suspend(struct device *dev)
 static int vbox_pm_resume(struct device *dev)
 {
 	struct vbox_private *vbox = dev_get_drvdata(dev);
-	int ret;
 
 	if (pci_enable_device(vbox->ddev.pdev))
 		return -EIO;
 
-	ret = vbox_drm_thaw(vbox);
-	if (ret)
-		return ret;
-
-	drm_kms_helper_poll_enable(&vbox->ddev);
-
-	return 0;
+	return drm_mode_config_helper_resume(&vbox->ddev);
 }
 
 static int vbox_pm_freeze(struct device *dev)
 {
 	struct vbox_private *vbox = dev_get_drvdata(dev);
 
-	return vbox_drm_freeze(vbox);
+	return drm_mode_config_helper_suspend(&vbox->ddev);
 }
 
 static int vbox_pm_thaw(struct device *dev)
 {
 	struct vbox_private *vbox = dev_get_drvdata(dev);
 
-	return vbox_drm_thaw(vbox);
+	return drm_mode_config_helper_resume(&vbox->ddev);
 }
 
 static int vbox_pm_poweroff(struct device *dev)
 {
 	struct vbox_private *vbox = dev_get_drvdata(dev);
 
-	return vbox_drm_freeze(vbox);
+	return drm_mode_config_helper_suspend(&vbox->ddev);
 }
 
 static const struct dev_pm_ops vbox_pm_ops = {
@@ -280,7 +254,7 @@ static void vbox_master_drop(struct drm_device *dev, struct drm_file *file_priv)
 static struct drm_driver driver = {
 	.driver_features =
 	    DRIVER_MODESET | DRIVER_GEM | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED |
-	    DRIVER_PRIME,
+	    DRIVER_PRIME | DRIVER_ATOMIC,
 	.dev_priv_size = 0,
 
 	.lastclose = vbox_driver_lastclose,
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index 54e6aac784f7..69a1e6c163b9 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -148,13 +148,6 @@ static void vbox_crtc_dpms(struct drm_crtc *crtc, int mode)
 	mutex_unlock(&vbox->hw_mutex);
 }
 
-static bool vbox_crtc_mode_fixup(struct drm_crtc *crtc,
-				 const struct drm_display_mode *mode,
-				 struct drm_display_mode *adjusted_mode)
-{
-	return true;
-}
-
 /*
  * Try to map the layout of virtual screens to the range of the input device.
  * Return true if we need to re-set the crtc modes due to screen offset
@@ -260,19 +253,10 @@ static void vbox_crtc_disable(struct drm_crtc *crtc)
 {
 }
 
-static void vbox_crtc_prepare(struct drm_crtc *crtc)
-{
-}
-
 static void vbox_crtc_commit(struct drm_crtc *crtc)
 {
 }
 
-static void vbox_crtc_mode_set_nofb(struct drm_crtc *crtc)
-{
-	/* We always set the mode when we set the fb/base */
-}
-
 static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
@@ -291,11 +275,7 @@ static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
 
 static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
 	.dpms = vbox_crtc_dpms,
-	.mode_fixup = vbox_crtc_mode_fixup,
-	.mode_set = drm_helper_crtc_mode_set,
-	.mode_set_nofb = vbox_crtc_mode_set_nofb,
 	.disable = vbox_crtc_disable,
-	.prepare = vbox_crtc_prepare,
 	.commit = vbox_crtc_commit,
 	.atomic_flush = vbox_crtc_atomic_flush,
 };
@@ -307,7 +287,7 @@ static void vbox_crtc_destroy(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_funcs vbox_crtc_funcs = {
-	.set_config = drm_crtc_helper_set_config,
+	.set_config = drm_atomic_helper_set_config,
 	/* .gamma_set = vbox_crtc_gamma_set, */
 	.destroy = vbox_crtc_destroy,
 	.reset = drm_atomic_helper_crtc_reset,
@@ -521,8 +501,8 @@ static const struct drm_plane_helper_funcs vbox_cursor_helper_funcs = {
 };
 
 static const struct drm_plane_funcs vbox_cursor_plane_funcs = {
-	.update_plane	= drm_plane_helper_update,
-	.disable_plane	= drm_plane_helper_disable,
+	.update_plane	= drm_atomic_helper_update_plane,
+	.disable_plane	= drm_atomic_helper_disable_plane,
 	.destroy	= drm_primary_helper_destroy,
 	.reset		= drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
@@ -543,8 +523,8 @@ static const struct drm_plane_helper_funcs vbox_primary_helper_funcs = {
 };
 
 static const struct drm_plane_funcs vbox_primary_plane_funcs = {
-	.update_plane	= drm_plane_helper_update,
-	.disable_plane	= drm_primary_helper_disable,
+	.update_plane	= drm_atomic_helper_update_plane,
+	.disable_plane	= drm_atomic_helper_disable_plane,
 	.destroy	= drm_primary_helper_destroy,
 	.reset		= drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
@@ -882,6 +862,9 @@ static const struct drm_connector_funcs vbox_connector_funcs = {
 	.detect = vbox_connector_detect,
 	.fill_modes = vbox_fill_modes,
 	.destroy = vbox_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int vbox_connector_init(struct drm_device *dev,
@@ -950,6 +933,8 @@ static struct drm_framebuffer *vbox_user_framebuffer_create(
 
 static const struct drm_mode_config_funcs vbox_mode_funcs = {
 	.fb_create = vbox_user_framebuffer_create,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
 };
 
 int vbox_mode_init(struct vbox_private *vbox)
-- 
2.19.0

[PATCH 10/15] staging: vboxvideo: Restore page-flip support

[Hans de Goede]

Restore page-flip support now that the atomic conversion is complete.

Since the mode parameter to vbox_crtc_set_base_and_mode() now never
is NULL call drm_atomic_crtc_needs_modeset() to check if we need to
check for input-mapping changes, to avoid doing unnecesarry work on
a flip. And hookup the drm_atomic_helper_page_flip helper to implement
the page_flip callback.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_mode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index 69a1e6c163b9..c4ec3fa49782 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -32,6 +32,7 @@
  *          Hans de Goede <hdegoede@redhat.com>
  */
 #include <linux/export.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_atomic_helper.h>
@@ -217,6 +218,7 @@ static void vbox_crtc_set_base_and_mode(struct drm_crtc *crtc,
 	struct vbox_bo *bo = gem_to_vbox_bo(to_vbox_framebuffer(fb)->obj);
 	struct vbox_private *vbox = crtc->dev->dev_private;
 	struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
+	bool needs_modeset = drm_atomic_crtc_needs_modeset(crtc->state);
 
 	mutex_lock(&vbox->hw_mutex);
 
@@ -227,7 +229,7 @@ static void vbox_crtc_set_base_and_mode(struct drm_crtc *crtc,
 	vbox_crtc->fb_offset = vbox_bo_gpu_offset(bo);
 
 	/* vbox_do_modeset() checks vbox->single_framebuffer so update it now */
-	if (mode && vbox_set_up_input_mapping(vbox)) {
+	if (needs_modeset && vbox_set_up_input_mapping(vbox)) {
 		struct drm_crtc *crtci;
 
 		list_for_each_entry(crtci, &vbox->ddev.mode_config.crtc_list,
@@ -241,7 +243,7 @@ static void vbox_crtc_set_base_and_mode(struct drm_crtc *crtc,
 	vbox_set_view(crtc);
 	vbox_do_modeset(crtc);
 
-	if (mode)
+	if (needs_modeset)
 		hgsmi_update_input_mapping(vbox->guest_pool, 0, 0,
 					   vbox->input_mapping_width,
 					   vbox->input_mapping_height);
@@ -288,6 +290,7 @@ static void vbox_crtc_destroy(struct drm_crtc *crtc)
 
 static const struct drm_crtc_funcs vbox_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
 	/* .gamma_set = vbox_crtc_gamma_set, */
 	.destroy = vbox_crtc_destroy,
 	.reset = drm_atomic_helper_crtc_reset,
-- 
2.19.0

[PATCH 11/15] staging: vboxvideo: Fix DPMS support after atomic conversion

[Hans de Goede]

Atomic modesetting does not use the traditional dpms call backs, instead
we should check crtc_state->active.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_drv.h  |  1 -
 drivers/staging/vboxvideo/vbox_mode.c | 28 ++-------------------------
 2 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
index fccb3851d6a3..9cc20c182df1 100644
--- a/drivers/staging/vboxvideo/vbox_drv.h
+++ b/drivers/staging/vboxvideo/vbox_drv.h
@@ -139,7 +139,6 @@ struct vbox_connector {
 
 struct vbox_crtc {
 	struct drm_crtc base;
-	bool blanked;
 	bool disconnected;
 	unsigned int crtc_id;
 	u32 fb_offset;
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index c4ec3fa49782..49ff9c4a6302 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -84,14 +84,13 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
 	}
 
 	flags = VBVA_SCREEN_F_ACTIVE;
-	flags |= (fb && !vbox_crtc->blanked) ? 0 : VBVA_SCREEN_F_BLANK;
+	flags |= (fb && crtc->state->active) ? 0 : VBVA_SCREEN_F_BLANK;
 	flags |= vbox_crtc->disconnected ? VBVA_SCREEN_F_DISABLED : 0;
 	hgsmi_process_display_info(vbox->guest_pool, vbox_crtc->crtc_id,
 				   x_offset, y_offset,
 				   vbox_crtc->x * bpp / 8 +
 							vbox_crtc->y * pitch,
-				   pitch, width, height,
-				   vbox_crtc->blanked ? 0 : bpp, flags);
+				   pitch, width, height, bpp, flags);
 }
 
 static int vbox_set_view(struct drm_crtc *crtc)
@@ -128,27 +127,6 @@ static int vbox_set_view(struct drm_crtc *crtc)
 	return 0;
 }
 
-static void vbox_crtc_dpms(struct drm_crtc *crtc, int mode)
-{
-	struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
-	struct vbox_private *vbox = crtc->dev->dev_private;
-
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
-		vbox_crtc->blanked = false;
-		break;
-	case DRM_MODE_DPMS_STANDBY:
-	case DRM_MODE_DPMS_SUSPEND:
-	case DRM_MODE_DPMS_OFF:
-		vbox_crtc->blanked = true;
-		break;
-	}
-
-	mutex_lock(&vbox->hw_mutex);
-	vbox_do_modeset(crtc);
-	mutex_unlock(&vbox->hw_mutex);
-}
-
 /*
  * Try to map the layout of virtual screens to the range of the input device.
  * Return true if we need to re-set the crtc modes due to screen offset
@@ -276,7 +254,6 @@ static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
-	.dpms = vbox_crtc_dpms,
 	.disable = vbox_crtc_disable,
 	.commit = vbox_crtc_commit,
 	.atomic_flush = vbox_crtc_atomic_flush,
@@ -861,7 +838,6 @@ static const struct drm_connector_helper_funcs vbox_connector_helper_funcs = {
 };
 
 static const struct drm_connector_funcs vbox_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
 	.detect = vbox_connector_detect,
 	.fill_modes = vbox_fill_modes,
 	.destroy = vbox_connector_destroy,
-- 
2.19.0

Re: [PATCH 11/15] staging: vboxvideo: Fix DPMS support after atomic conversion

[Daniel Vetter]

On Wed, Sep 26, 2018 at 09:42:02PM +0200, Hans de Goede wrote:
> Atomic modesetting does not use the traditional dpms call backs, instead
> we should check crtc_state->active.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Are you sure this does what you want it to do? Atomic helpers fully shut
down the screen when you do a dpms off, "just blanked" kinda doesn't exist
as a state by default.
-Daniel

> ---
>  drivers/staging/vboxvideo/vbox_drv.h  |  1 -
>  drivers/staging/vboxvideo/vbox_mode.c | 28 ++-------------------------
>  2 files changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
> index fccb3851d6a3..9cc20c182df1 100644
> --- a/drivers/staging/vboxvideo/vbox_drv.h
> +++ b/drivers/staging/vboxvideo/vbox_drv.h
> @@ -139,7 +139,6 @@ struct vbox_connector {
>  
>  struct vbox_crtc {
>  	struct drm_crtc base;
> -	bool blanked;
>  	bool disconnected;
>  	unsigned int crtc_id;
>  	u32 fb_offset;
> diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
> index c4ec3fa49782..49ff9c4a6302 100644
> --- a/drivers/staging/vboxvideo/vbox_mode.c
> +++ b/drivers/staging/vboxvideo/vbox_mode.c
> @@ -84,14 +84,13 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
>  	}
>  
>  	flags = VBVA_SCREEN_F_ACTIVE;
> -	flags |= (fb && !vbox_crtc->blanked) ? 0 : VBVA_SCREEN_F_BLANK;
> +	flags |= (fb && crtc->state->active) ? 0 : VBVA_SCREEN_F_BLANK;
>  	flags |= vbox_crtc->disconnected ? VBVA_SCREEN_F_DISABLED : 0;
>  	hgsmi_process_display_info(vbox->guest_pool, vbox_crtc->crtc_id,
>  				   x_offset, y_offset,
>  				   vbox_crtc->x * bpp / 8 +
>  							vbox_crtc->y * pitch,
> -				   pitch, width, height,
> -				   vbox_crtc->blanked ? 0 : bpp, flags);
> +				   pitch, width, height, bpp, flags);
>  }
>  
>  static int vbox_set_view(struct drm_crtc *crtc)
> @@ -128,27 +127,6 @@ static int vbox_set_view(struct drm_crtc *crtc)
>  	return 0;
>  }
>  
> -static void vbox_crtc_dpms(struct drm_crtc *crtc, int mode)
> -{
> -	struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
> -	struct vbox_private *vbox = crtc->dev->dev_private;
> -
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		vbox_crtc->blanked = false;
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		vbox_crtc->blanked = true;
> -		break;
> -	}
> -
> -	mutex_lock(&vbox->hw_mutex);
> -	vbox_do_modeset(crtc);
> -	mutex_unlock(&vbox->hw_mutex);
> -}
> -
>  /*
>   * Try to map the layout of virtual screens to the range of the input device.
>   * Return true if we need to re-set the crtc modes due to screen offset
> @@ -276,7 +254,6 @@ static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
> -	.dpms = vbox_crtc_dpms,
>  	.disable = vbox_crtc_disable,
>  	.commit = vbox_crtc_commit,
>  	.atomic_flush = vbox_crtc_atomic_flush,
> @@ -861,7 +838,6 @@ static const struct drm_connector_helper_funcs vbox_connector_helper_funcs = {
>  };
>  
>  static const struct drm_connector_funcs vbox_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
>  	.detect = vbox_connector_detect,
>  	.fill_modes = vbox_fill_modes,
>  	.destroy = vbox_connector_destroy,
> -- 
> 2.19.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/15] staging: vboxvideo: Fix DPMS support after atomic conversion

[Hans de Goede]

Hi,

On 01-10-18 09:53, Daniel Vetter wrote:
> On Wed, Sep 26, 2018 at 09:42:02PM +0200, Hans de Goede wrote:
>> Atomic modesetting does not use the traditional dpms call backs, instead
>> we should check crtc_state->active.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Are you sure this does what you want it to do? Atomic helpers fully shut
> down the screen when you do a dpms off, "just blanked" kinda doesn't exist
> as a state by default.

Yes I'm sure I tested "xset dpms force off" and before this would
result in in the virtual monitors (just windows on the host) to resize to
640x480 and in that 640x480 still show part of the old contents.

After this patch they become black instead.

Note somewhat related, Virtualbox does not allow closing a window from
within the guest, if the user wants to stop using an (extra) virtual monitor
it needs to be disabled in the VMs UI. Turning of a monitor through e.g.
"xrandr --output foo --off" just makes the window black.

Regards,

Hans




> -Daniel
> 
>> ---
>>   drivers/staging/vboxvideo/vbox_drv.h  |  1 -
>>   drivers/staging/vboxvideo/vbox_mode.c | 28 ++-------------------------
>>   2 files changed, 2 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
>> index fccb3851d6a3..9cc20c182df1 100644
>> --- a/drivers/staging/vboxvideo/vbox_drv.h
>> +++ b/drivers/staging/vboxvideo/vbox_drv.h
>> @@ -139,7 +139,6 @@ struct vbox_connector {
>>   
>>   struct vbox_crtc {
>>   	struct drm_crtc base;
>> -	bool blanked;
>>   	bool disconnected;
>>   	unsigned int crtc_id;
>>   	u32 fb_offset;
>> diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
>> index c4ec3fa49782..49ff9c4a6302 100644
>> --- a/drivers/staging/vboxvideo/vbox_mode.c
>> +++ b/drivers/staging/vboxvideo/vbox_mode.c
>> @@ -84,14 +84,13 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
>>   	}
>>   
>>   	flags = VBVA_SCREEN_F_ACTIVE;
>> -	flags |= (fb && !vbox_crtc->blanked) ? 0 : VBVA_SCREEN_F_BLANK;
>> +	flags |= (fb && crtc->state->active) ? 0 : VBVA_SCREEN_F_BLANK;
>>   	flags |= vbox_crtc->disconnected ? VBVA_SCREEN_F_DISABLED : 0;
>>   	hgsmi_process_display_info(vbox->guest_pool, vbox_crtc->crtc_id,
>>   				   x_offset, y_offset,
>>   				   vbox_crtc->x * bpp / 8 +
>>   							vbox_crtc->y * pitch,
>> -				   pitch, width, height,
>> -				   vbox_crtc->blanked ? 0 : bpp, flags);
>> +				   pitch, width, height, bpp, flags);
>>   }
>>   
>>   static int vbox_set_view(struct drm_crtc *crtc)
>> @@ -128,27 +127,6 @@ static int vbox_set_view(struct drm_crtc *crtc)
>>   	return 0;
>>   }
>>   
>> -static void vbox_crtc_dpms(struct drm_crtc *crtc, int mode)
>> -{
>> -	struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
>> -	struct vbox_private *vbox = crtc->dev->dev_private;
>> -
>> -	switch (mode) {
>> -	case DRM_MODE_DPMS_ON:
>> -		vbox_crtc->blanked = false;
>> -		break;
>> -	case DRM_MODE_DPMS_STANDBY:
>> -	case DRM_MODE_DPMS_SUSPEND:
>> -	case DRM_MODE_DPMS_OFF:
>> -		vbox_crtc->blanked = true;
>> -		break;
>> -	}
>> -
>> -	mutex_lock(&vbox->hw_mutex);
>> -	vbox_do_modeset(crtc);
>> -	mutex_unlock(&vbox->hw_mutex);
>> -}
>> -
>>   /*
>>    * Try to map the layout of virtual screens to the range of the input device.
>>    * Return true if we need to re-set the crtc modes due to screen offset
>> @@ -276,7 +254,6 @@ static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
>>   }
>>   
>>   static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
>> -	.dpms = vbox_crtc_dpms,
>>   	.disable = vbox_crtc_disable,
>>   	.commit = vbox_crtc_commit,
>>   	.atomic_flush = vbox_crtc_atomic_flush,
>> @@ -861,7 +838,6 @@ static const struct drm_connector_helper_funcs vbox_connector_helper_funcs = {
>>   };
>>   
>>   static const struct drm_connector_funcs vbox_connector_funcs = {
>> -	.dpms = drm_helper_connector_dpms,
>>   	.detect = vbox_connector_detect,
>>   	.fill_modes = vbox_fill_modes,
>>   	.destroy = vbox_connector_destroy,
>> -- 
>> 2.19.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/15] staging: vboxvideo: Fix DPMS support after atomic conversion

[Daniel Vetter]

On Mon, Oct 01, 2018 at 11:37:29AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 01-10-18 09:53, Daniel Vetter wrote:
> > On Wed, Sep 26, 2018 at 09:42:02PM +0200, Hans de Goede wrote:
> > > Atomic modesetting does not use the traditional dpms call backs, instead
> > > we should check crtc_state->active.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Are you sure this does what you want it to do? Atomic helpers fully shut
> > down the screen when you do a dpms off, "just blanked" kinda doesn't exist
> > as a state by default.
> 
> Yes I'm sure I tested "xset dpms force off" and before this would
> result in in the virtual monitors (just windows on the host) to resize to
> 640x480 and in that 640x480 still show part of the old contents.

Hm, this sounds a bit like a bug in your code somewhere, or at least not
100% converted over to atomic. From a driver pov it should be 100%
equivalent code between dpms off and the xrandr --off. If dpms shows some
garbage without this, then something is wrong.

The only difference is that for dpms off the helpers don't call
->cleanup_plane (and then ->prepare_plane on re-enabling), since the
framebuffers are supposed to stick around. Are you perchance doing some
modeset programming in there? That would be a bug in the atomic
implementation.

crtc_state->active should only be consulted from atomic_check callbacks.
I've added some pretty lengthy kerneldoc for this just recently, to
explain better how this is supposed to work.

> After this patch they become black instead.
> 
> Note somewhat related, Virtualbox does not allow closing a window from
> within the guest, if the user wants to stop using an (extra) virtual monitor
> it needs to be disabled in the VMs UI. Turning of a monitor through e.g.
> "xrandr --output foo --off" just makes the window black.

Ok that's funny, but shouldn't be related to what's going on here.
-Daniel

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > -Daniel
> > 
> > > ---
> > >   drivers/staging/vboxvideo/vbox_drv.h  |  1 -
> > >   drivers/staging/vboxvideo/vbox_mode.c | 28 ++-------------------------
> > >   2 files changed, 2 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
> > > index fccb3851d6a3..9cc20c182df1 100644
> > > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > > @@ -139,7 +139,6 @@ struct vbox_connector {
> > >   struct vbox_crtc {
> > >   	struct drm_crtc base;
> > > -	bool blanked;
> > >   	bool disconnected;
> > >   	unsigned int crtc_id;
> > >   	u32 fb_offset;
> > > diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
> > > index c4ec3fa49782..49ff9c4a6302 100644
> > > --- a/drivers/staging/vboxvideo/vbox_mode.c
> > > +++ b/drivers/staging/vboxvideo/vbox_mode.c
> > > @@ -84,14 +84,13 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
> > >   	}
> > >   	flags = VBVA_SCREEN_F_ACTIVE;
> > > -	flags |= (fb && !vbox_crtc->blanked) ? 0 : VBVA_SCREEN_F_BLANK;
> > > +	flags |= (fb && crtc->state->active) ? 0 : VBVA_SCREEN_F_BLANK;
> > >   	flags |= vbox_crtc->disconnected ? VBVA_SCREEN_F_DISABLED : 0;
> > >   	hgsmi_process_display_info(vbox->guest_pool, vbox_crtc->crtc_id,
> > >   				   x_offset, y_offset,
> > >   				   vbox_crtc->x * bpp / 8 +
> > >   							vbox_crtc->y * pitch,
> > > -				   pitch, width, height,
> > > -				   vbox_crtc->blanked ? 0 : bpp, flags);
> > > +				   pitch, width, height, bpp, flags);
> > >   }
> > >   static int vbox_set_view(struct drm_crtc *crtc)
> > > @@ -128,27 +127,6 @@ static int vbox_set_view(struct drm_crtc *crtc)
> > >   	return 0;
> > >   }
> > > -static void vbox_crtc_dpms(struct drm_crtc *crtc, int mode)
> > > -{
> > > -	struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
> > > -	struct vbox_private *vbox = crtc->dev->dev_private;
> > > -
> > > -	switch (mode) {
> > > -	case DRM_MODE_DPMS_ON:
> > > -		vbox_crtc->blanked = false;
> > > -		break;
> > > -	case DRM_MODE_DPMS_STANDBY:
> > > -	case DRM_MODE_DPMS_SUSPEND:
> > > -	case DRM_MODE_DPMS_OFF:
> > > -		vbox_crtc->blanked = true;
> > > -		break;
> > > -	}
> > > -
> > > -	mutex_lock(&vbox->hw_mutex);
> > > -	vbox_do_modeset(crtc);
> > > -	mutex_unlock(&vbox->hw_mutex);
> > > -}
> > > -
> > >   /*
> > >    * Try to map the layout of virtual screens to the range of the input device.
> > >    * Return true if we need to re-set the crtc modes due to screen offset
> > > @@ -276,7 +254,6 @@ static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
> > >   }
> > >   static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
> > > -	.dpms = vbox_crtc_dpms,
> > >   	.disable = vbox_crtc_disable,
> > >   	.commit = vbox_crtc_commit,
> > >   	.atomic_flush = vbox_crtc_atomic_flush,
> > > @@ -861,7 +838,6 @@ static const struct drm_connector_helper_funcs vbox_connector_helper_funcs = {
> > >   };
> > >   static const struct drm_connector_funcs vbox_connector_funcs = {
> > > -	.dpms = drm_helper_connector_dpms,
> > >   	.detect = vbox_connector_detect,
> > >   	.fill_modes = vbox_fill_modes,
> > >   	.destroy = vbox_connector_destroy,
> > > -- 
> > > 2.19.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/15] staging: vboxvideo: Fix DPMS support after atomic conversion

[Hans de Goede]

Hi,

On 01-10-18 18:52, Daniel Vetter wrote:
> On Mon, Oct 01, 2018 at 11:37:29AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 01-10-18 09:53, Daniel Vetter wrote:
>>> On Wed, Sep 26, 2018 at 09:42:02PM +0200, Hans de Goede wrote:
>>>> Atomic modesetting does not use the traditional dpms call backs, instead
>>>> we should check crtc_state->active.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Are you sure this does what you want it to do? Atomic helpers fully shut
>>> down the screen when you do a dpms off, "just blanked" kinda doesn't exist
>>> as a state by default.
>>
>> Yes I'm sure I tested "xset dpms force off" and before this would
>> result in in the virtual monitors (just windows on the host) to resize to
>> 640x480 and in that 640x480 still show part of the old contents.
> 
> Hm, this sounds a bit like a bug in your code somewhere, or at least not
> 100% converted over to atomic. From a driver pov it should be 100%
> equivalent code between dpms off and the xrandr --off. If dpms shows some
> garbage without this, then something is wrong.

I believe the 2 paths are different, xrandr --off actually does a modeset
disabling the crtc, where as xset dpms force off just changes the DPMS
property on the connector using the non atomic property ioctls leading to
the following call graph:

drm_mode_obj_set_property_ioctl()
set_property_atomic()
drm_atomic_connector_commit_dpms()

With the last one iterating over all connectors of the crtc to which
the connector in question is connected and then setting
crtc_state->active = false if all connectors have their dpms value
set to a value != DRM_MODE_DPMS_ON.

Followed by a drm_atomic_commit() so all that changes in this code
path is the active property of the crtc_state. Where as with a --off the
primary plane_state will get its fb (and crtc) set to NULL.

> The only difference is that for dpms off the helpers don't call
> ->cleanup_plane (and then ->prepare_plane on re-enabling), since the
> framebuffers are supposed to stick around. Are you perchance doing some
> modeset programming in there? That would be a bug in the atomic
> implementation.
> 
> crtc_state->active should only be consulted from atomic_check callbacks.
> I've added some pretty lengthy kerneldoc for this just recently, to
> explain better how this is supposed to work.

See above, I believe that the code path which I point out changes
crtc_state->active without making any further changes.

I'm pretty new to all this, so I could be wrong, but this is what
I believe is happening.

Regards,

Hans




> 
>> After this patch they become black instead.
>>
>> Note somewhat related, Virtualbox does not allow closing a window from
>> within the guest, if the user wants to stop using an (extra) virtual monitor
>> it needs to be disabled in the VMs UI. Turning of a monitor through e.g.
>> "xrandr --output foo --off" just makes the window black.
> 
> Ok that's funny, but shouldn't be related to what's going on here.
> -Daniel
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>> -Daniel
>>>
>>>> ---
>>>>    drivers/staging/vboxvideo/vbox_drv.h  |  1 -
>>>>    drivers/staging/vboxvideo/vbox_mode.c | 28 ++-------------------------
>>>>    2 files changed, 2 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
>>>> index fccb3851d6a3..9cc20c182df1 100644
>>>> --- a/drivers/staging/vboxvideo/vbox_drv.h
>>>> +++ b/drivers/staging/vboxvideo/vbox_drv.h
>>>> @@ -139,7 +139,6 @@ struct vbox_connector {
>>>>    struct vbox_crtc {
>>>>    	struct drm_crtc base;
>>>> -	bool blanked;
>>>>    	bool disconnected;
>>>>    	unsigned int crtc_id;
>>>>    	u32 fb_offset;
>>>> diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
>>>> index c4ec3fa49782..49ff9c4a6302 100644
>>>> --- a/drivers/staging/vboxvideo/vbox_mode.c
>>>> +++ b/drivers/staging/vboxvideo/vbox_mode.c
>>>> @@ -84,14 +84,13 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
>>>>    	}
>>>>    	flags = VBVA_SCREEN_F_ACTIVE;
>>>> -	flags |= (fb && !vbox_crtc->blanked) ? 0 : VBVA_SCREEN_F_BLANK;
>>>> +	flags |= (fb && crtc->state->active) ? 0 : VBVA_SCREEN_F_BLANK;
>>>>    	flags |= vbox_crtc->disconnected ? VBVA_SCREEN_F_DISABLED : 0;
>>>>    	hgsmi_process_display_info(vbox->guest_pool, vbox_crtc->crtc_id,
>>>>    				   x_offset, y_offset,
>>>>    				   vbox_crtc->x * bpp / 8 +
>>>>    							vbox_crtc->y * pitch,
>>>> -				   pitch, width, height,
>>>> -				   vbox_crtc->blanked ? 0 : bpp, flags);
>>>> +				   pitch, width, height, bpp, flags);
>>>>    }
>>>>    static int vbox_set_view(struct drm_crtc *crtc)
>>>> @@ -128,27 +127,6 @@ static int vbox_set_view(struct drm_crtc *crtc)
>>>>    	return 0;
>>>>    }
>>>> -static void vbox_crtc_dpms(struct drm_crtc *crtc, int mode)
>>>> -{
>>>> -	struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
>>>> -	struct vbox_private *vbox = crtc->dev->dev_private;
>>>> -
>>>> -	switch (mode) {
>>>> -	case DRM_MODE_DPMS_ON:
>>>> -		vbox_crtc->blanked = false;
>>>> -		break;
>>>> -	case DRM_MODE_DPMS_STANDBY:
>>>> -	case DRM_MODE_DPMS_SUSPEND:
>>>> -	case DRM_MODE_DPMS_OFF:
>>>> -		vbox_crtc->blanked = true;
>>>> -		break;
>>>> -	}
>>>> -
>>>> -	mutex_lock(&vbox->hw_mutex);
>>>> -	vbox_do_modeset(crtc);
>>>> -	mutex_unlock(&vbox->hw_mutex);
>>>> -}
>>>> -
>>>>    /*
>>>>     * Try to map the layout of virtual screens to the range of the input device.
>>>>     * Return true if we need to re-set the crtc modes due to screen offset
>>>> @@ -276,7 +254,6 @@ static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
>>>>    }
>>>>    static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
>>>> -	.dpms = vbox_crtc_dpms,
>>>>    	.disable = vbox_crtc_disable,
>>>>    	.commit = vbox_crtc_commit,
>>>>    	.atomic_flush = vbox_crtc_atomic_flush,
>>>> @@ -861,7 +838,6 @@ static const struct drm_connector_helper_funcs vbox_connector_helper_funcs = {
>>>>    };
>>>>    static const struct drm_connector_funcs vbox_connector_funcs = {
>>>> -	.dpms = drm_helper_connector_dpms,
>>>>    	.detect = vbox_connector_detect,
>>>>    	.fill_modes = vbox_fill_modes,
>>>>    	.destroy = vbox_connector_destroy,
>>>> -- 
>>>> 2.19.0
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/15] staging: vboxvideo: Fix DPMS support after atomic conversion

[Daniel Vetter]

On Mon, Oct 1, 2018 at 11:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 01-10-18 18:52, Daniel Vetter wrote:
> > On Mon, Oct 01, 2018 at 11:37:29AM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 01-10-18 09:53, Daniel Vetter wrote:
> >>> On Wed, Sep 26, 2018 at 09:42:02PM +0200, Hans de Goede wrote:
> >>>> Atomic modesetting does not use the traditional dpms call backs, instead
> >>>> we should check crtc_state->active.
> >>>>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>
> >>> Are you sure this does what you want it to do? Atomic helpers fully shut
> >>> down the screen when you do a dpms off, "just blanked" kinda doesn't exist
> >>> as a state by default.
> >>
> >> Yes I'm sure I tested "xset dpms force off" and before this would
> >> result in in the virtual monitors (just windows on the host) to resize to
> >> 640x480 and in that 640x480 still show part of the old contents.
> >
> > Hm, this sounds a bit like a bug in your code somewhere, or at least not
> > 100% converted over to atomic. From a driver pov it should be 100%
> > equivalent code between dpms off and the xrandr --off. If dpms shows some
> > garbage without this, then something is wrong.
>
> I believe the 2 paths are different, xrandr --off actually does a modeset
> disabling the crtc, where as xset dpms force off just changes the DPMS
> property on the connector using the non atomic property ioctls leading to
> the following call graph:
>
> drm_mode_obj_set_property_ioctl()
> set_property_atomic()
> drm_atomic_connector_commit_dpms()
>
> With the last one iterating over all connectors of the crtc to which
> the connector in question is connected and then setting
> crtc_state->active = false if all connectors have their dpms value
> set to a value != DRM_MODE_DPMS_ON.
>
> Followed by a drm_atomic_commit() so all that changes in this code
> path is the active property of the crtc_state. Where as with a --off the
> primary plane_state will get its fb (and crtc) set to NULL.

You're supposed to scan out black in this case, that's correct. But
you're also supposed to switch of the screen (well, scan out black
since that's all vbox allows from the guest side) if your
crtc_funcs->atomic_disable is called.

I think the correct fix is to just shut down the display
unconditionally in atomic_disable, and ignore ->active. The helpers
should take care of things for you already.

Also: plane going NULL is a side effect of the SETCRTC legacy2atomic
code only, for an atomic CRTC OFF you can see a disabled CRTC, but the
primary plane still having an attached framebuffer. Iirc
drm_plane_state->crtc will be set to NULL. So you need this code in
both cases, not only when active changes (but active will be clamped
to false when disabling the CRTC, so checking for ->active is simply
redundant).

> > The only difference is that for dpms off the helpers don't call
> > ->cleanup_plane (and then ->prepare_plane on re-enabling), since the
> > framebuffers are supposed to stick around. Are you perchance doing some
> > modeset programming in there? That would be a bug in the atomic
> > implementation.
> >
> > crtc_state->active should only be consulted from atomic_check callbacks.
> > I've added some pretty lengthy kerneldoc for this just recently, to
> > explain better how this is supposed to work.
>
> See above, I believe that the code path which I point out changes
> crtc_state->active without making any further changes.

Yup, that's all correct. It's just that your driver code shouldn't
need to look at this. See the kernel-doc for drm_crtc->active in
latest upstream.

> I'm pretty new to all this, so I could be wrong, but this is what
> I believe is happening.

No worries. Imo questions = great opportunity to improve the docs :-)

Cheers, Daniel


>
> Regards,
>
> Hans
>
>
>
>
> >
> >> After this patch they become black instead.
> >>
> >> Note somewhat related, Virtualbox does not allow closing a window from
> >> within the guest, if the user wants to stop using an (extra) virtual monitor
> >> it needs to be disabled in the VMs UI. Turning of a monitor through e.g.
> >> "xrandr --output foo --off" just makes the window black.
> >
> > Ok that's funny, but shouldn't be related to what's going on here.
> > -Daniel
> >
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>
> >>> -Daniel
> >>>
> >>>> ---
> >>>>    drivers/staging/vboxvideo/vbox_drv.h  |  1 -
> >>>>    drivers/staging/vboxvideo/vbox_mode.c | 28 ++-------------------------
> >>>>    2 files changed, 2 insertions(+), 27 deletions(-)
> >>>>
> >>>> diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
> >>>> index fccb3851d6a3..9cc20c182df1 100644
> >>>> --- a/drivers/staging/vboxvideo/vbox_drv.h
> >>>> +++ b/drivers/staging/vboxvideo/vbox_drv.h
> >>>> @@ -139,7 +139,6 @@ struct vbox_connector {
> >>>>    struct vbox_crtc {
> >>>>            struct drm_crtc base;
> >>>> -  bool blanked;
> >>>>            bool disconnected;
> >>>>            unsigned int crtc_id;
> >>>>            u32 fb_offset;
> >>>> diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
> >>>> index c4ec3fa49782..49ff9c4a6302 100644
> >>>> --- a/drivers/staging/vboxvideo/vbox_mode.c
> >>>> +++ b/drivers/staging/vboxvideo/vbox_mode.c
> >>>> @@ -84,14 +84,13 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
> >>>>            }
> >>>>            flags = VBVA_SCREEN_F_ACTIVE;
> >>>> -  flags |= (fb && !vbox_crtc->blanked) ? 0 : VBVA_SCREEN_F_BLANK;
> >>>> +  flags |= (fb && crtc->state->active) ? 0 : VBVA_SCREEN_F_BLANK;
> >>>>            flags |= vbox_crtc->disconnected ? VBVA_SCREEN_F_DISABLED : 0;
> >>>>            hgsmi_process_display_info(vbox->guest_pool, vbox_crtc->crtc_id,
> >>>>                                       x_offset, y_offset,
> >>>>                                       vbox_crtc->x * bpp / 8 +
> >>>>                                                            vbox_crtc->y * pitch,
> >>>> -                             pitch, width, height,
> >>>> -                             vbox_crtc->blanked ? 0 : bpp, flags);
> >>>> +                             pitch, width, height, bpp, flags);
> >>>>    }
> >>>>    static int vbox_set_view(struct drm_crtc *crtc)
> >>>> @@ -128,27 +127,6 @@ static int vbox_set_view(struct drm_crtc *crtc)
> >>>>            return 0;
> >>>>    }
> >>>> -static void vbox_crtc_dpms(struct drm_crtc *crtc, int mode)
> >>>> -{
> >>>> -  struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
> >>>> -  struct vbox_private *vbox = crtc->dev->dev_private;
> >>>> -
> >>>> -  switch (mode) {
> >>>> -  case DRM_MODE_DPMS_ON:
> >>>> -          vbox_crtc->blanked = false;
> >>>> -          break;
> >>>> -  case DRM_MODE_DPMS_STANDBY:
> >>>> -  case DRM_MODE_DPMS_SUSPEND:
> >>>> -  case DRM_MODE_DPMS_OFF:
> >>>> -          vbox_crtc->blanked = true;
> >>>> -          break;
> >>>> -  }
> >>>> -
> >>>> -  mutex_lock(&vbox->hw_mutex);
> >>>> -  vbox_do_modeset(crtc);
> >>>> -  mutex_unlock(&vbox->hw_mutex);
> >>>> -}
> >>>> -
> >>>>    /*
> >>>>     * Try to map the layout of virtual screens to the range of the input device.
> >>>>     * Return true if we need to re-set the crtc modes due to screen offset
> >>>> @@ -276,7 +254,6 @@ static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
> >>>>    }
> >>>>    static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
> >>>> -  .dpms = vbox_crtc_dpms,
> >>>>            .disable = vbox_crtc_disable,
> >>>>            .commit = vbox_crtc_commit,
> >>>>            .atomic_flush = vbox_crtc_atomic_flush,
> >>>> @@ -861,7 +838,6 @@ static const struct drm_connector_helper_funcs vbox_connector_helper_funcs = {
> >>>>    };
> >>>>    static const struct drm_connector_funcs vbox_connector_funcs = {
> >>>> -  .dpms = drm_helper_connector_dpms,
> >>>>            .detect = vbox_connector_detect,
> >>>>            .fill_modes = vbox_fill_modes,
> >>>>            .destroy = vbox_connector_destroy,
> >>>> --
> >>>> 2.19.0
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/15] staging: vboxvideo: Fix DPMS support after atomic conversion

[Hans de Goede]

Hi,

On 02-10-18 00:01, Daniel Vetter wrote:
> On Mon, Oct 1, 2018 at 11:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 01-10-18 18:52, Daniel Vetter wrote:
>>> On Mon, Oct 01, 2018 at 11:37:29AM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 01-10-18 09:53, Daniel Vetter wrote:
>>>>> On Wed, Sep 26, 2018 at 09:42:02PM +0200, Hans de Goede wrote:
>>>>>> Atomic modesetting does not use the traditional dpms call backs, instead
>>>>>> we should check crtc_state->active.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>
>>>>> Are you sure this does what you want it to do? Atomic helpers fully shut
>>>>> down the screen when you do a dpms off, "just blanked" kinda doesn't exist
>>>>> as a state by default.
>>>>
>>>> Yes I'm sure I tested "xset dpms force off" and before this would
>>>> result in in the virtual monitors (just windows on the host) to resize to
>>>> 640x480 and in that 640x480 still show part of the old contents.
>>>
>>> Hm, this sounds a bit like a bug in your code somewhere, or at least not
>>> 100% converted over to atomic. From a driver pov it should be 100%
>>> equivalent code between dpms off and the xrandr --off. If dpms shows some
>>> garbage without this, then something is wrong.
>>
>> I believe the 2 paths are different, xrandr --off actually does a modeset
>> disabling the crtc, where as xset dpms force off just changes the DPMS
>> property on the connector using the non atomic property ioctls leading to
>> the following call graph:
>>
>> drm_mode_obj_set_property_ioctl()
>> set_property_atomic()
>> drm_atomic_connector_commit_dpms()
>>
>> With the last one iterating over all connectors of the crtc to which
>> the connector in question is connected and then setting
>> crtc_state->active = false if all connectors have their dpms value
>> set to a value != DRM_MODE_DPMS_ON.
>>
>> Followed by a drm_atomic_commit() so all that changes in this code
>> path is the active property of the crtc_state. Where as with a --off the
>> primary plane_state will get its fb (and crtc) set to NULL.
> 
> You're supposed to scan out black in this case, that's correct. But
> you're also supposed to switch of the screen (well, scan out black
> since that's all vbox allows from the guest side) if your
> crtc_funcs->atomic_disable is called.
> 
> I think the correct fix is to just shut down the display
> unconditionally in atomic_disable, and ignore ->active. The helpers
> should take care of things for you already.

Currently my atomic_disable for the crtc is empty, so that may well be
the culprit. Can I just make this point to drm_atomic_helper_disable_planes_on_crtc ?
and do I need to do anything in atomic_enable to undo this then or will
the planes get re-enabled by the atomic core?

> Also: plane going NULL is a side effect of the SETCRTC legacy2atomic
> code only, for an atomic CRTC OFF you can see a disabled CRTC, but the
> primary plane still having an attached framebuffer. Iirc
> drm_plane_state->crtc will be set to NULL.

I do not think that that is correct, from include/drm/drm_atomic_helper.h:

static inline bool
drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
                            struct drm_plane_state *new_plane_state)
{
         /*
          * When disabling a plane, CRTC and FB should always be NULL together.
          * Anything else should be considered a bug in the atomic core, so we
          * gently warn about it.
          */
         WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) |
                 (new_plane_state->crtc != NULL && new_plane_state->fb == NULL));

         return old_plane_state->crtc && !new_plane_state->crtc;
}

So crtc->primary->state->fb will be NULL when crtc->primary->state->crtc is NULL.

I guess both could be non NULL but crtc->state->enable is false ?

> So you need this code in
> both cases, not only when active changes (but active will be clamped
> to false when disabling the CRTC, so checking for ->active is simply
> redundant).

>>> The only difference is that for dpms off the helpers don't call
>>> ->cleanup_plane (and then ->prepare_plane on re-enabling), since the
>>> framebuffers are supposed to stick around. Are you perchance doing some
>>> modeset programming in there? That would be a bug in the atomic
>>> implementation.
>>>
>>> crtc_state->active should only be consulted from atomic_check callbacks.
>>> I've added some pretty lengthy kerneldoc for this just recently, to
>>> explain better how this is supposed to work.
>>
>> See above, I believe that the code path which I point out changes
>> crtc_state->active without making any further changes.
> 
> Yup, that's all correct. It's just that your driver code shouldn't
> need to look at this. See the kernel-doc for drm_crtc->active in
> latest upstream.

Ok, I will take a look at this, I probably will not get around to this next
week. ATM I'm fixing some high prio bootsplash (plymouth) bugs related to
the flickerfree work for Fedora 29: https://hansdegoede.livejournal.com/19224.html

>> I'm pretty new to all this, so I could be wrong, but this is what
>> I believe is happening.
> 
> No worries. Imo questions = great opportunity to improve the docs :-)

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/15] staging: vboxvideo: Fix DPMS support after atomic conversion

[Daniel Vetter]

O
n Tue, Oct 2, 2018 at 11:25 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 02-10-18 00:01, Daniel Vetter wrote:
> > On Mon, Oct 1, 2018 at 11:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 01-10-18 18:52, Daniel Vetter wrote:
> >>> On Mon, Oct 01, 2018 at 11:37:29AM +0200, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 01-10-18 09:53, Daniel Vetter wrote:
> >>>>> On Wed, Sep 26, 2018 at 09:42:02PM +0200, Hans de Goede wrote:
> >>>>>> Atomic modesetting does not use the traditional dpms call backs, instead
> >>>>>> we should check crtc_state->active.
> >>>>>>
> >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>>
> >>>>> Are you sure this does what you want it to do? Atomic helpers fully shut
> >>>>> down the screen when you do a dpms off, "just blanked" kinda doesn't exist
> >>>>> as a state by default.
> >>>>
> >>>> Yes I'm sure I tested "xset dpms force off" and before this would
> >>>> result in in the virtual monitors (just windows on the host) to resize to
> >>>> 640x480 and in that 640x480 still show part of the old contents.
> >>>
> >>> Hm, this sounds a bit like a bug in your code somewhere, or at least not
> >>> 100% converted over to atomic. From a driver pov it should be 100%
> >>> equivalent code between dpms off and the xrandr --off. If dpms shows some
> >>> garbage without this, then something is wrong.
> >>
> >> I believe the 2 paths are different, xrandr --off actually does a modeset
> >> disabling the crtc, where as xset dpms force off just changes the DPMS
> >> property on the connector using the non atomic property ioctls leading to
> >> the following call graph:
> >>
> >> drm_mode_obj_set_property_ioctl()
> >> set_property_atomic()
> >> drm_atomic_connector_commit_dpms()
> >>
> >> With the last one iterating over all connectors of the crtc to which
> >> the connector in question is connected and then setting
> >> crtc_state->active = false if all connectors have their dpms value
> >> set to a value != DRM_MODE_DPMS_ON.
> >>
> >> Followed by a drm_atomic_commit() so all that changes in this code
> >> path is the active property of the crtc_state. Where as with a --off the
> >> primary plane_state will get its fb (and crtc) set to NULL.
> >
> > You're supposed to scan out black in this case, that's correct. But
> > you're also supposed to switch of the screen (well, scan out black
> > since that's all vbox allows from the guest side) if your
> > crtc_funcs->atomic_disable is called.
> >
> > I think the correct fix is to just shut down the display
> > unconditionally in atomic_disable, and ignore ->active. The helpers
> > should take care of things for you already.
>
> Currently my atomic_disable for the crtc is empty, so that may well be
> the culprit. Can I just make this point to drm_atomic_helper_disable_planes_on_crtc ?
> and do I need to do anything in atomic_enable to undo this then or will
> the planes get re-enabled by the atomic core?

Yup, that should work, assuming your plane->atomic_update code will
(re)enable the plane.

> > Also: plane going NULL is a side effect of the SETCRTC legacy2atomic
> > code only, for an atomic CRTC OFF you can see a disabled CRTC, but the
> > primary plane still having an attached framebuffer. Iirc
> > drm_plane_state->crtc will be set to NULL.
>
> I do not think that that is correct, from include/drm/drm_atomic_helper.h:
>
> static inline bool
> drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
>                             struct drm_plane_state *new_plane_state)
> {
>          /*
>           * When disabling a plane, CRTC and FB should always be NULL together.
>           * Anything else should be considered a bug in the atomic core, so we
>           * gently warn about it.
>           */
>          WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) |
>                  (new_plane_state->crtc != NULL && new_plane_state->fb == NULL));
>
>          return old_plane_state->crtc && !new_plane_state->crtc;
> }
>
> So crtc->primary->state->fb will be NULL when crtc->primary->state->crtc is NULL.
>
> I guess both could be non NULL but crtc->state->enable is false ?

Ah right, that's the case that can happen. crtc_state->enable == false
still implies crtc_state->active == false, so would still work. But
the ->active check is still redundant.

Cheers, Daniel

> > So you need this code in
> > both cases, not only when active changes (but active will be clamped
> > to false when disabling the CRTC, so checking for ->active is simply
> > redundant).
>
> >>> The only difference is that for dpms off the helpers don't call
> >>> ->cleanup_plane (and then ->prepare_plane on re-enabling), since the
> >>> framebuffers are supposed to stick around. Are you perchance doing some
> >>> modeset programming in there? That would be a bug in the atomic
> >>> implementation.
> >>>
> >>> crtc_state->active should only be consulted from atomic_check callbacks.
> >>> I've added some pretty lengthy kerneldoc for this just recently, to
> >>> explain better how this is supposed to work.
> >>
> >> See above, I believe that the code path which I point out changes
> >> crtc_state->active without making any further changes.
> >
> > Yup, that's all correct. It's just that your driver code shouldn't
> > need to look at this. See the kernel-doc for drm_crtc->active in
> > latest upstream.
>
> Ok, I will take a look at this, I probably will not get around to this next
> week. ATM I'm fixing some high prio bootsplash (plymouth) bugs related to
> the flickerfree work for Fedora 29: https://hansdegoede.livejournal.com/19224.html
>
> >> I'm pretty new to all this, so I could be wrong, but this is what
> >> I believe is happening.
> >
> > No worries. Imo questions = great opportunity to improve the docs :-)
>
> Regards,
>
> Hans



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/15] staging: vboxvideo: Replace crtc_helper enable/disable functions

[Hans de Goede]

Replace vbox_crtc_commit and vbox_crtc_disable with
vbox_crtc_atomic_[en|dis]able which are the preferred callbacks for
these for atomic drivers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_mode.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index 49ff9c4a6302..72fc6614179a 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -229,11 +229,13 @@ static void vbox_crtc_set_base_and_mode(struct drm_crtc *crtc,
 	mutex_unlock(&vbox->hw_mutex);
 }
 
-static void vbox_crtc_disable(struct drm_crtc *crtc)
+static void vbox_crtc_atomic_enable(struct drm_crtc *crtc,
+				    struct drm_crtc_state *old_crtc_state)
 {
 }
 
-static void vbox_crtc_commit(struct drm_crtc *crtc)
+static void vbox_crtc_atomic_disable(struct drm_crtc *crtc,
+				     struct drm_crtc_state *old_crtc_state)
 {
 }
 
@@ -254,8 +256,8 @@ static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
-	.disable = vbox_crtc_disable,
-	.commit = vbox_crtc_commit,
+	.atomic_enable = vbox_crtc_atomic_enable,
+	.atomic_disable = vbox_crtc_atomic_disable,
 	.atomic_flush = vbox_crtc_atomic_flush,
 };
 
-- 
2.19.0

[PATCH 13/15] staging: vboxvideo: Call drm_atomic_helper_check_plane_state from atomic_check

[Hans de Goede]

Extend our planes atomic_check callbacks to be more thorough by calling
the drm_atomic_helper_check_plane_state helper.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_mode.c | 30 ++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index 72fc6614179a..4993a6cf6770 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -280,7 +280,19 @@ static const struct drm_crtc_funcs vbox_crtc_funcs = {
 static int vbox_primary_atomic_check(struct drm_plane *plane,
 				     struct drm_plane_state *new_state)
 {
-	return 0;
+	struct drm_crtc_state *crtc_state = NULL;
+
+	if (new_state->crtc) {
+		crtc_state = drm_atomic_get_existing_crtc_state(
+					    new_state->state, new_state->crtc);
+		if (WARN_ON(!crtc_state))
+			return -EINVAL;
+	}
+
+	return drm_atomic_helper_check_plane_state(new_state, crtc_state,
+						   DRM_PLANE_HELPER_NO_SCALING,
+						   DRM_PLANE_HELPER_NO_SCALING,
+						   false, true);
 }
 
 static void vbox_primary_atomic_update(struct drm_plane *plane,
@@ -337,8 +349,24 @@ static void vbox_primary_cleanup_fb(struct drm_plane *plane,
 static int vbox_cursor_atomic_check(struct drm_plane *plane,
 				    struct drm_plane_state *new_state)
 {
+	struct drm_crtc_state *crtc_state = NULL;
 	u32 width = new_state->crtc_w;
 	u32 height = new_state->crtc_h;
+	int ret;
+
+	if (new_state->crtc) {
+		crtc_state = drm_atomic_get_existing_crtc_state(
+					    new_state->state, new_state->crtc);
+		if (WARN_ON(!crtc_state))
+			return -EINVAL;
+	}
+
+	ret = drm_atomic_helper_check_plane_state(new_state, crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  true, true);
+	if (ret)
+		return ret;
 
 	if (!new_state->fb)
 		return 0;
-- 
2.19.0

[PATCH 14/15] staging: vboxvideo: Drop unnecessary drm_connector_helper_funcs callbacks

[Hans de Goede]

vbox_mode_valid always returns MODE_OK, which is also the default if no
mode_valid callback is defined.

vbox_best_single_encoder chains to drm_encoder_find, the drm-core will
call drm_encoder_find itself if there is no best_encoder call back.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_mode.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index 4993a6cf6770..756544b53600 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -651,18 +651,6 @@ static void vbox_encoder_destroy(struct drm_encoder *encoder)
 	kfree(encoder);
 }
 
-static struct drm_encoder *vbox_best_single_encoder(struct drm_connector
-						    *connector)
-{
-	int enc_id = connector->encoder_ids[0];
-
-	/* pick the encoder ids */
-	if (enc_id)
-		return drm_encoder_find(connector->dev, NULL, enc_id);
-
-	return NULL;
-}
-
 static const struct drm_encoder_funcs vbox_enc_funcs = {
 	.destroy = vbox_encoder_destroy,
 };
@@ -820,12 +808,6 @@ static int vbox_get_modes(struct drm_connector *connector)
 	return num_modes;
 }
 
-static enum drm_mode_status vbox_mode_valid(struct drm_connector *connector,
-			   struct drm_display_mode *mode)
-{
-	return MODE_OK;
-}
-
 static void vbox_connector_destroy(struct drm_connector *connector)
 {
 	drm_connector_unregister(connector);
@@ -862,9 +844,7 @@ static int vbox_fill_modes(struct drm_connector *connector, u32 max_x,
 }
 
 static const struct drm_connector_helper_funcs vbox_connector_helper_funcs = {
-	.mode_valid = vbox_mode_valid,
 	.get_modes = vbox_get_modes,
-	.best_encoder = vbox_best_single_encoder,
 };
 
 static const struct drm_connector_funcs vbox_connector_funcs = {
-- 
2.19.0

[PATCH 15/15] staging: vboxvideo: Use more drm_fb_helper functions

[Hans de Goede]

Store fbhelper and afb struct directly in vbox_private and use
drm_fb_helper_fbdev_setup to replace vbox_fbdev_init, note we cannot use
drm_fb_helper_fbdev_teardown since we use a private framebuffer for the
fbdev.

And replace vbox_driver_lastclose with drm_fb_helper_lastclose.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/vbox_drv.c  | 10 +++-
 drivers/staging/vboxvideo/vbox_drv.h  | 28 +++--------
 drivers/staging/vboxvideo/vbox_fb.c   | 70 ++++++---------------------
 drivers/staging/vboxvideo/vbox_main.c | 12 -----
 drivers/staging/vboxvideo/vbox_mode.c |  2 +-
 5 files changed, 31 insertions(+), 91 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c
index c3e14107da0d..257030460fb6 100644
--- a/drivers/staging/vboxvideo/vbox_drv.c
+++ b/drivers/staging/vboxvideo/vbox_drv.c
@@ -49,6 +49,10 @@ static const struct pci_device_id pciidlist[] = {
 };
 MODULE_DEVICE_TABLE(pci, pciidlist);
 
+static struct drm_fb_helper_funcs vbox_fb_helper_funcs = {
+	.fb_probe = vboxfb_create,
+};
+
 static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct vbox_private *vbox;
@@ -92,7 +96,9 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		goto err_mode_fini;
 
-	ret = vbox_fbdev_init(vbox);
+	ret = drm_fb_helper_fbdev_setup(&vbox->ddev, &vbox->fb_helper,
+					&vbox_fb_helper_funcs, 32,
+					vbox->num_crtcs);
 	if (ret)
 		goto err_irq_fini;
 
@@ -257,7 +263,7 @@ static struct drm_driver driver = {
 	    DRIVER_PRIME | DRIVER_ATOMIC,
 	.dev_priv_size = 0,
 
-	.lastclose = vbox_driver_lastclose,
+	.lastclose = drm_fb_helper_lastclose,
 	.master_set = vbox_master_set,
 	.master_drop = vbox_master_drop,
 
diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
index 9cc20c182df1..73395a7536c5 100644
--- a/drivers/staging/vboxvideo/vbox_drv.h
+++ b/drivers/staging/vboxvideo/vbox_drv.h
@@ -72,11 +72,16 @@
 				sizeof(struct hgsmi_host_flags))
 #define HOST_FLAGS_OFFSET GUEST_HEAP_USABLE_SIZE
 
-struct vbox_fbdev;
+struct vbox_framebuffer {
+	struct drm_framebuffer base;
+	struct drm_gem_object *obj;
+};
 
 struct vbox_private {
 	/* Must be first; or we must define our own release callback */
 	struct drm_device ddev;
+	struct drm_fb_helper fb_helper;
+	struct vbox_framebuffer afb;
 
 	u8 __iomem *guest_heap;
 	u8 __iomem *vbva_buffers;
@@ -91,8 +96,6 @@ struct vbox_private {
 	/** Array of structures for receiving mode hints. */
 	struct vbva_modehint *last_mode_hints;
 
-	struct vbox_fbdev *fbdev;
-
 	int fb_mtrr;
 
 	struct {
@@ -122,8 +125,6 @@ struct vbox_private {
 #undef CURSOR_PIXEL_COUNT
 #undef CURSOR_DATA_SIZE
 
-void vbox_driver_lastclose(struct drm_device *dev);
-
 struct vbox_gem_object;
 
 struct vbox_connector {
@@ -171,20 +172,6 @@ struct vbox_encoder {
 	struct drm_encoder base;
 };
 
-struct vbox_framebuffer {
-	struct drm_framebuffer base;
-	struct drm_gem_object *obj;
-};
-
-struct vbox_fbdev {
-	struct drm_fb_helper helper;
-	struct vbox_framebuffer afb;
-	int size;
-	struct ttm_bo_kmap_obj mapping;
-	int x1, y1, x2, y2;	/* dirty rect */
-	spinlock_t dirty_lock;
-};
-
 #define to_vbox_crtc(x) container_of(x, struct vbox_crtc, base)
 #define to_vbox_connector(x) container_of(x, struct vbox_connector, base)
 #define to_vbox_encoder(x) container_of(x, struct vbox_encoder, base)
@@ -212,7 +199,8 @@ int vbox_framebuffer_init(struct vbox_private *vbox,
 			  const struct DRM_MODE_FB_CMD *mode_cmd,
 			  struct drm_gem_object *obj);
 
-int vbox_fbdev_init(struct vbox_private *vbox);
+int vboxfb_create(struct drm_fb_helper *helper,
+		  struct drm_fb_helper_surface_size *sizes);
 void vbox_fbdev_fini(struct vbox_private *vbox);
 
 struct vbox_bo {
diff --git a/drivers/staging/vboxvideo/vbox_fb.c b/drivers/staging/vboxvideo/vbox_fb.c
index 8a9d56762659..ee25f3a03934 100644
--- a/drivers/staging/vboxvideo/vbox_fb.c
+++ b/drivers/staging/vboxvideo/vbox_fb.c
@@ -66,13 +66,11 @@ static struct fb_ops vboxfb_ops = {
 	.fb_debug_leave = drm_fb_helper_debug_leave,
 };
 
-static int vboxfb_create(struct drm_fb_helper *helper,
-			 struct drm_fb_helper_surface_size *sizes)
+int vboxfb_create(struct drm_fb_helper *helper,
+		  struct drm_fb_helper_surface_size *sizes)
 {
-	struct vbox_fbdev *fbdev =
-	    container_of(helper, struct vbox_fbdev, helper);
-	struct vbox_private *vbox = container_of(fbdev->helper.dev,
-						 struct vbox_private, ddev);
+	struct vbox_private *vbox =
+		container_of(helper, struct vbox_private, fb_helper);
 	struct pci_dev *pdev = vbox->ddev.pdev;
 	struct DRM_MODE_FB_CMD mode_cmd;
 	struct drm_framebuffer *fb;
@@ -98,7 +96,7 @@ static int vboxfb_create(struct drm_fb_helper *helper,
 		return ret;
 	}
 
-	ret = vbox_framebuffer_init(vbox, &fbdev->afb, &mode_cmd, gobj);
+	ret = vbox_framebuffer_init(vbox, &vbox->afb, &mode_cmd, gobj);
 	if (ret)
 		return ret;
 
@@ -117,12 +115,10 @@ static int vboxfb_create(struct drm_fb_helper *helper,
 	if (IS_ERR(info->screen_base))
 		return PTR_ERR(info->screen_base);
 
-	info->par = fbdev;
+	info->par = helper;
 
-	fbdev->size = size;
-
-	fb = &fbdev->afb.base;
-	fbdev->helper.fb = fb;
+	fb = &vbox->afb.base;
+	helper->fb = fb;
 
 	strcpy(info->fix.id, "vboxdrmfb");
 
@@ -142,7 +138,7 @@ static int vboxfb_create(struct drm_fb_helper *helper,
 	info->apertures->ranges[0].size = pci_resource_len(pdev, 0);
 
 	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth);
-	drm_fb_helper_fill_var(info, &fbdev->helper, sizes->fb_width,
+	drm_fb_helper_fill_var(info, helper, sizes->fb_width,
 			       sizes->fb_height);
 
 	gpu_addr = vbox_bo_gpu_offset(bo);
@@ -161,21 +157,16 @@ static int vboxfb_create(struct drm_fb_helper *helper,
 	return 0;
 }
 
-static struct drm_fb_helper_funcs vbox_fb_helper_funcs = {
-	.fb_probe = vboxfb_create,
-};
-
 void vbox_fbdev_fini(struct vbox_private *vbox)
 {
-	struct vbox_fbdev *fbdev = vbox->fbdev;
-	struct vbox_framebuffer *afb = &fbdev->afb;
+	struct vbox_framebuffer *afb = &vbox->afb;
 
 #ifdef CONFIG_DRM_KMS_FB_HELPER
-	if (fbdev->helper.fbdev && fbdev->helper.fbdev->fbdefio)
-		fb_deferred_io_cleanup(fbdev->helper.fbdev);
+	if (vbox->fb_helper.fbdev && vbox->fb_helper.fbdev->fbdefio)
+		fb_deferred_io_cleanup(vbox->fb_helper.fbdev);
 #endif
 
-	drm_fb_helper_unregister_fbi(&fbdev->helper);
+	drm_fb_helper_unregister_fbi(&vbox->fb_helper);
 
 	if (afb->obj) {
 		struct vbox_bo *bo = gem_to_vbox_bo(afb->obj);
@@ -188,41 +179,8 @@ void vbox_fbdev_fini(struct vbox_private *vbox)
 		drm_gem_object_put_unlocked(afb->obj);
 		afb->obj = NULL;
 	}
-	drm_fb_helper_fini(&fbdev->helper);
+	drm_fb_helper_fini(&vbox->fb_helper);
 
 	drm_framebuffer_unregister_private(&afb->base);
 	drm_framebuffer_cleanup(&afb->base);
 }
-
-int vbox_fbdev_init(struct vbox_private *vbox)
-{
-	struct drm_device *dev = &vbox->ddev;
-	struct vbox_fbdev *fbdev;
-	int ret;
-
-	fbdev = devm_kzalloc(dev->dev, sizeof(*fbdev), GFP_KERNEL);
-	if (!fbdev)
-		return -ENOMEM;
-
-	vbox->fbdev = fbdev;
-	spin_lock_init(&fbdev->dirty_lock);
-
-	drm_fb_helper_prepare(dev, &fbdev->helper, &vbox_fb_helper_funcs);
-	ret = drm_fb_helper_init(dev, &fbdev->helper, vbox->num_crtcs);
-	if (ret)
-		return ret;
-
-	ret = drm_fb_helper_single_add_all_connectors(&fbdev->helper);
-	if (ret)
-		goto err_fini;
-
-	ret = drm_fb_helper_initial_config(&fbdev->helper, 32);
-	if (ret)
-		goto err_fini;
-
-	return 0;
-
-err_fini:
-	drm_fb_helper_fini(&fbdev->helper);
-	return ret;
-}
diff --git a/drivers/staging/vboxvideo/vbox_main.c b/drivers/staging/vboxvideo/vbox_main.c
index 3b82d483ab51..7466c1103ff6 100644
--- a/drivers/staging/vboxvideo/vbox_main.c
+++ b/drivers/staging/vboxvideo/vbox_main.c
@@ -322,18 +322,6 @@ void vbox_hw_fini(struct vbox_private *vbox)
 	pci_iounmap(vbox->ddev.pdev, vbox->guest_heap);
 }
 
-/**
- * @note this is described in the DRM framework documentation.  AST does not
- * have it, but we get an oops on driver unload if it is not present.
- */
-void vbox_driver_lastclose(struct drm_device *dev)
-{
-	struct vbox_private *vbox = dev->dev_private;
-
-	if (vbox->fbdev)
-		drm_fb_helper_restore_fbdev_mode_unlocked(&vbox->fbdev->helper);
-}
-
 int vbox_gem_create(struct vbox_private *vbox,
 		    u32 size, bool iskernel, struct drm_gem_object **obj)
 {
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index 756544b53600..042e4f384df9 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -153,7 +153,7 @@ static bool vbox_set_up_input_mapping(struct vbox_private *vbox)
 
 		if (!fb1) {
 			fb1 = fb;
-			if (to_vbox_framebuffer(fb1) == &vbox->fbdev->afb)
+			if (to_vbox_framebuffer(fb1) == &vbox->afb)
 				break;
 		} else if (fb != fb1) {
 			single_framebuffer = false;
-- 
2.19.0

Re: [PATCH 01/15] staging: vboxvideo: Convert to atomic modesetting API

[Daniel Vetter]

On Wed, Sep 26, 2018 at 09:41:51PM +0200, Hans de Goede wrote:
> Hi All,
> 
> This series converts the vboxvideo driver to the modesetting API, once this
> is merged I will submit a patch to move the driver out of staging and into
> drivers/gpu/drm.
> 
> This series consists of 3 parts:
> 
> 1) More cleanups / prep work for atomic conversion
> 2) The actual atomic conversion, this is modelled after the atomic conversion
>    of the qxl driver done by Gabriel Krisman Bertazi. Gabriel if you are
>    reading this, thank you for providing a roadmap how to go about this.
> 3) Restore some features which were lost by the atomic conversion +
>    more cleanups

Given how tiny vbox is it's easier if I just review the overall diff I
think. Overall = patch that adds the entire driver to drm, ignoring what's
been going on in staging. Please reply with that (I can do it locally, but
that doesn't allow me to conveniently reply&comment).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/15] staging: vboxvideo: Convert to atomic modesetting API

[Hans de Goede]

Hi,

On 01-10-18 09:51, Daniel Vetter wrote:
> On Wed, Sep 26, 2018 at 09:41:51PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> This series converts the vboxvideo driver to the modesetting API, once this
>> is merged I will submit a patch to move the driver out of staging and into
>> drivers/gpu/drm.
>>
>> This series consists of 3 parts:
>>
>> 1) More cleanups / prep work for atomic conversion
>> 2) The actual atomic conversion, this is modelled after the atomic conversion
>>     of the qxl driver done by Gabriel Krisman Bertazi. Gabriel if you are
>>     reading this, thank you for providing a roadmap how to go about this.
>> 3) Restore some features which were lost by the atomic conversion +
>>     more cleanups
> 
> Given how tiny vbox is it's easier if I just review the overall diff I
> think. Overall = patch that adds the entire driver to drm, ignoring what's
> been going on in staging. Please reply with that (I can do it locally, but
> that doesn't allow me to conveniently reply&comment).

Yes I was planning on submitting a patch to move this out of staging and I
was expecting you (or other drm people) to review that.

I was waiting for things to be merged in staging first :)

I need to do one more bugfix for an issue kbuild caught and then I will
send the patch to move things.

Regards,

Hans

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/15] staging: vboxvideo: Convert to atomic modesetting API

[Daniel Vetter]

On Mon, Oct 01, 2018 at 11:17:57AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 01-10-18 09:51, Daniel Vetter wrote:
> > On Wed, Sep 26, 2018 at 09:41:51PM +0200, Hans de Goede wrote:
> > > Hi All,
> > > 
> > > This series converts the vboxvideo driver to the modesetting API, once this
> > > is merged I will submit a patch to move the driver out of staging and into
> > > drivers/gpu/drm.
> > > 
> > > This series consists of 3 parts:
> > > 
> > > 1) More cleanups / prep work for atomic conversion
> > > 2) The actual atomic conversion, this is modelled after the atomic conversion
> > >     of the qxl driver done by Gabriel Krisman Bertazi. Gabriel if you are
> > >     reading this, thank you for providing a roadmap how to go about this.
> > > 3) Restore some features which were lost by the atomic conversion +
> > >     more cleanups
> > 
> > Given how tiny vbox is it's easier if I just review the overall diff I
> > think. Overall = patch that adds the entire driver to drm, ignoring what's
> > been going on in staging. Please reply with that (I can do it locally, but
> > that doesn't allow me to conveniently reply&comment).
> 
> Yes I was planning on submitting a patch to move this out of staging and I
> was expecting you (or other drm people) to review that.
> 
> I was waiting for things to be merged in staging first :)
> 
> I need to do one more bugfix for an issue kbuild caught and then I will
> send the patch to move things.

Ok, sounds like a plan. I'll ignore this series here (except for the dpms
thing, which jumped out from all the churn to me).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel