[07/18] drm/vmwgfx: Add FIXME comments for customer page_flip handlers
diff mbox series

Message ID 20181002133526.13685-8-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • atomic helper cleanups
Related show

Commit Message

Daniel Vetter Oct. 2, 2018, 1:35 p.m. UTC
The idea behind allowing drivers to override legacy ioctls (instead of
using the generic implementations unconditionally) is to handle bugs
in old driver-specific userspace. Like e.g. vmw_kms_set_config does,
to work around some vmwgfx userspace not clearing its ioctl structs
properly.

But you can't use it to augment semantics and put in additional
checks, since from a correctly working userspace's pov there should
not be any difference in behaviour between the legacy and the atomic
paths.

vmwgfx seems to be doing some strange things in its page_flip
handlers. Since I'm not an expert of this codebase just wrap some
FIXME comments around the potentially problematic code.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
Cc: Sinclair Yeh <syeh@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 1 +
 2 files changed, 3 insertions(+)

Comments

Thomas Hellstrom Oct. 2, 2018, 4:49 p.m. UTC | #1
Hi, Daniel,

On 10/02/2018 03:35 PM, Daniel Vetter wrote:
> The idea behind allowing drivers to override legacy ioctls (instead of
> using the generic implementations unconditionally) is to handle bugs
> in old driver-specific userspace. Like e.g. vmw_kms_set_config does,
> to work around some vmwgfx userspace not clearing its ioctl structs
> properly.
>
> But you can't use it to augment semantics and put in additional
> checks, since from a correctly working userspace's pov there should
> not be any difference in behaviour between the legacy and the atomic
> paths.
>
> vmwgfx seems to be doing some strange things in its page_flip
> handlers. Since I'm not an expert of this codebase just wrap some
> FIXME comments around the potentially problematic code.
>

We've got proper patches for these. Apparently they got lost in my -next 
pull request, though.

/Thomas
Daniel Vetter Oct. 3, 2018, 9:11 a.m. UTC | #2
On Tue, Oct 02, 2018 at 04:49:30PM +0000, Thomas Hellstrom wrote:
> Hi, Daniel,
> 
> On 10/02/2018 03:35 PM, Daniel Vetter wrote:
> > The idea behind allowing drivers to override legacy ioctls (instead of
> > using the generic implementations unconditionally) is to handle bugs
> > in old driver-specific userspace. Like e.g. vmw_kms_set_config does,
> > to work around some vmwgfx userspace not clearing its ioctl structs
> > properly.
> >
> > But you can't use it to augment semantics and put in additional
> > checks, since from a correctly working userspace's pov there should
> > not be any difference in behaviour between the legacy and the atomic
> > paths.
> >
> > vmwgfx seems to be doing some strange things in its page_flip
> > handlers. Since I'm not an expert of this codebase just wrap some
> > FIXME comments around the potentially problematic code.
> >
> 
> We've got proper patches for these. Apparently they got lost in my -next 
> pull request, though.

Yeah I wondered why this one here didn't conflict yet, I can carry it
around a bit longer as a memo.
-Daniel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 4c68ad6f3605..8e692334c3b9 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -326,6 +326,7 @@  static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
 	struct vmw_private *dev_priv = vmw_priv(crtc->dev);
 	int ret;
 
+	/* FIMXE: This check needs to be moved into ->atomic_check code. */
 	if (!vmw_kms_crtc_flippable(dev_priv, crtc))
 		return -EINVAL;
 
@@ -335,6 +336,7 @@  static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
 		return ret;
 	}
 
+	/* FIMXE: This code needs to be moved into ->atomic_commit callbacks. */
 	if (vmw_crtc_to_du(crtc)->is_implicit)
 		vmw_kms_update_implicit_fb(dev_priv, crtc);
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index e28bb08114a5..199927f20e0a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -501,6 +501,7 @@  static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc,
 	struct vmw_screen_target_display_unit *stdu = vmw_crtc_to_stdu(crtc);
 	int ret;
 
+	/* FIMXE: This check needs to be moved into ->atomic_check code. */
 	if (!stdu->defined || !vmw_kms_crtc_flippable(dev_priv, crtc))
 		return -EINVAL;