[PATCH 00/10] drm/uapi: Validate mode flags/type, and deprecate some of them

[PATCH 00/10] drm/uapi: Validate mode flags/type, and deprecate some of them

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I recently realized that we're not validating the mode flags/type
passed in from userspace. Let's try to fix that.

I'd also like to entirely eliminate some of the more crazy mode flags.
PIXMUX and BCAST look pretty easy, so I've targetted them first.

Ideally I'd like to kill DBLCLK, CLKDIV2, HSKEW, DBLSCAN, and vscan
as well. IMO there's no good reason to expose any of that to userspace,
and instead it should all be handled internally by the drivers.
Unfortunately all of those seem to be used to some degree by a handful
of drivers.

I also tried to nuke some of the crazy mode types.

Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Keith Packard <keithp@keithp.com>

Ville Syrjälä (10):
  drm/modes: Move 3D stereo flag check into drm_mode_validate_basic()
  drm/uapi: Validate the mode flags/type
  drm/uapi: Deprecate DRM_MODE_FLAG_PIXMUX
  drm/uapi: Deprecate DRM_MODE_FLAG_BCAST
  drm/modes: Fix description of DRM_MODE_TYPE_USERDEF
  drm/modes: Kill off the oddball DRM_MODE_TYPE_CRTC_C vs.
    DRM_MODE_TYPE_BUILTIN handling
  drm/modes: Kill DRM_MODE_TYPE_CLOCK_CRTC_C define
  drm/uapi: Deprecate nonsense kms mode types
  drm/modes: Provide global mode_valid hook
  drm/i915: Provide a device level .mode_valid() hook

 drivers/gpu/drm/drm_atomic.c         |  2 +-
 drivers/gpu/drm/drm_crtc.c           |  2 +-
 drivers/gpu/drm/drm_modes.c          | 60 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/drm_probe_helper.c   |  2 +-
 drivers/gpu/drm/i915/intel_crt.c     |  3 --
 drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++
 drivers/gpu/drm/i915/intel_dsi.c     |  5 ---
 drivers/gpu/drm/i915/intel_dvo.c     |  3 --
 drivers/gpu/drm/i915/intel_fbc.c     |  3 +-
 drivers/gpu/drm/i915/intel_hdmi.c    |  3 --
 drivers/gpu/drm/i915/intel_sdvo.c    |  3 --
 include/drm/drm_mode_config.h        | 12 ++++++++
 include/drm/drm_modes.h              | 24 +++++++--------
 include/uapi/drm/drm_mode.h          | 30 ++++++++++++++----
 14 files changed, 120 insertions(+), 59 deletions(-)

-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/10] drm/modes: Move 3D stereo flag check into drm_mode_validate_basic()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we don't sanity check the 3D stereo flags for modes filled out
by the kernel. Move the check from drm_mode_convert_umode() into
drm_mode_validate_basic() so that we get the same check going both ways.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 4a3f68a33844..1a72883b836e 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1036,6 +1036,9 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 enum drm_mode_status
 drm_mode_validate_basic(const struct drm_display_mode *mode)
 {
+	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
+		return MODE_BAD;
+
 	if (mode->clock == 0)
 		return MODE_CLOCK_LOW;
 
@@ -1574,9 +1577,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
 		goto out;
 	}
 
-	if ((in->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
-		goto out;
-
 	out->clock = in->clock;
 	out->hdisplay = in->hdisplay;
 	out->hsync_start = in->hsync_start;
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/10] drm/uapi: Validate the mode flags/type

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently userspace is allowed to feed in any king of garbage in the
high bits of the mode flags/type, as are drivers when probing modes.
Reject any mode with bogus flags/type.

Hopefully this won't break any current userspace...

Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c |  4 ++++
 include/uapi/drm/drm_mode.h | 24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 1a72883b836e..f99ba963fb3e 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1036,6 +1036,10 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 enum drm_mode_status
 drm_mode_validate_basic(const struct drm_display_mode *mode)
 {
+	if (mode->type & ~DRM_MODE_TYPE_ALL ||
+	    mode->flags & ~DRM_MODE_FLAG_ALL)
+		return MODE_BAD;
+
 	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
 		return MODE_BAD;
 
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 5597a87154e5..004db470b477 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -46,6 +46,14 @@ extern "C" {
 #define DRM_MODE_TYPE_USERDEF	(1<<5)
 #define DRM_MODE_TYPE_DRIVER	(1<<6)
 
+#define DRM_MODE_TYPE_ALL	(DRM_MODE_TYPE_BUILTIN |	\
+				 DRM_MODE_TYPE_CLOCK_C |	\
+				 DRM_MODE_TYPE_CRTC_C |		\
+				 DRM_MODE_TYPE_PREFERRED |	\
+				 DRM_MODE_TYPE_DEFAULT |	\
+				 DRM_MODE_TYPE_USERDEF |	\
+				 DRM_MODE_TYPE_DRIVER)
+
 /* Video mode flags */
 /* bit compatible with the xrandr RR_ definitions (bits 0-13)
  *
@@ -99,6 +107,22 @@ extern "C" {
 #define  DRM_MODE_FLAG_PIC_AR_16_9 \
 			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
 
+#define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
+				 DRM_MODE_FLAG_NHSYNC |		\
+				 DRM_MODE_FLAG_PVSYNC |		\
+				 DRM_MODE_FLAG_NVSYNC |		\
+				 DRM_MODE_FLAG_INTERLACE |	\
+				 DRM_MODE_FLAG_DBLSCAN |	\
+				 DRM_MODE_FLAG_CSYNC |		\
+				 DRM_MODE_FLAG_PCSYNC |		\
+				 DRM_MODE_FLAG_NCSYNC |		\
+				 DRM_MODE_FLAG_HSKEW |		\
+				 DRM_MODE_FLAG_BCAST |		\
+				 DRM_MODE_FLAG_PIXMUX |		\
+				 DRM_MODE_FLAG_DBLCLK |		\
+				 DRM_MODE_FLAG_CLKDIV2 |	\
+				 DRM_MODE_FLAG_3D_MASK)
+
 /* DPMS flags */
 /* bit compatible with the xorg definitions. */
 #define DRM_MODE_DPMS_ON	0
-- 
2.13.6

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

[PATCH 03/10] drm/uapi: Deprecate DRM_MODE_FLAG_PIXMUX

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reject any mode with DRM_MODE_FLAG_PIXMUX. We have no code that even
checks for this flag hence it can't possibly do any good.

Looks like this flag had something to do the the controller<->ramdac
interface with some ancient S3 graphics adapters. Why someone though
it would be a good idea to expose it directly to users I don't know.
And later on it got copied into the randr protocol and kms uapi.

Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_modes.h     | 2 +-
 include/uapi/drm/drm_mode.h | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 9f3421c8efcd..bce573375dd8 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -300,7 +300,7 @@ struct drm_display_mode {
 	 *  - DRM_MODE_FLAG_NCSYNC: composite sync is active low.
 	 *  - DRM_MODE_FLAG_HSKEW: hskew provided (not used?).
 	 *  - DRM_MODE_FLAG_BCAST: not used?
-	 *  - DRM_MODE_FLAG_PIXMUX: not used?
+	 *  - DRM_MODE_FLAG_PIXMUX: <deprecated>
 	 *  - DRM_MODE_FLAG_DBLCLK: double-clocked mode.
 	 *  - DRM_MODE_FLAG_CLKDIV2: half-clocked mode.
 	 *
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 004db470b477..8d872e17223e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -75,7 +75,7 @@ extern "C" {
 #define DRM_MODE_FLAG_NCSYNC			(1<<8)
 #define DRM_MODE_FLAG_HSKEW			(1<<9) /* hskew provided */
 #define DRM_MODE_FLAG_BCAST			(1<<10)
-#define DRM_MODE_FLAG_PIXMUX			(1<<11)
+#define DRM_MODE_FLAG_PIXMUX			(1<<11) /* deprecated */
 #define DRM_MODE_FLAG_DBLCLK			(1<<12)
 #define DRM_MODE_FLAG_CLKDIV2			(1<<13)
  /*
@@ -118,7 +118,6 @@ extern "C" {
 				 DRM_MODE_FLAG_NCSYNC |		\
 				 DRM_MODE_FLAG_HSKEW |		\
 				 DRM_MODE_FLAG_BCAST |		\
-				 DRM_MODE_FLAG_PIXMUX |		\
 				 DRM_MODE_FLAG_DBLCLK |		\
 				 DRM_MODE_FLAG_CLKDIV2 |	\
 				 DRM_MODE_FLAG_3D_MASK)
-- 
2.13.6

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

[PATCH 04/10] drm/uapi: Deprecate DRM_MODE_FLAG_BCAST

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reject any mode with DRM_MODE_FLAG_BCAST. We have no code that even
checks for this flag hence it can't possibly do any good.

I think this maybe originated from fbdev where it was supposed to
indicate PAL/NTSC broadcast timings. I have no idea why those would
have to be identified by a flag rather than by just the timings
themselves. And then I assume it got copied into xfree86 for
fbdevhw, and later on it leaked into the randr protocol and kms uapi.

Since kms fbdev emulation never uses the corresponding fbdev flag
there should be no sane way for this to come back into kms via
userspace either.

Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_modes.h     | 2 +-
 include/uapi/drm/drm_mode.h | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index bce573375dd8..09773e766e1f 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -299,7 +299,7 @@ struct drm_display_mode {
 	 *  - DRM_MODE_FLAG_PCSYNC: composite sync is active high.
 	 *  - DRM_MODE_FLAG_NCSYNC: composite sync is active low.
 	 *  - DRM_MODE_FLAG_HSKEW: hskew provided (not used?).
-	 *  - DRM_MODE_FLAG_BCAST: not used?
+	 *  - DRM_MODE_FLAG_BCAST: <deprecated>
 	 *  - DRM_MODE_FLAG_PIXMUX: <deprecated>
 	 *  - DRM_MODE_FLAG_DBLCLK: double-clocked mode.
 	 *  - DRM_MODE_FLAG_CLKDIV2: half-clocked mode.
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 8d872e17223e..a7cded1c43e8 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -74,7 +74,7 @@ extern "C" {
 #define DRM_MODE_FLAG_PCSYNC			(1<<7)
 #define DRM_MODE_FLAG_NCSYNC			(1<<8)
 #define DRM_MODE_FLAG_HSKEW			(1<<9) /* hskew provided */
-#define DRM_MODE_FLAG_BCAST			(1<<10)
+#define DRM_MODE_FLAG_BCAST			(1<<10) /* deprecated */
 #define DRM_MODE_FLAG_PIXMUX			(1<<11) /* deprecated */
 #define DRM_MODE_FLAG_DBLCLK			(1<<12)
 #define DRM_MODE_FLAG_CLKDIV2			(1<<13)
@@ -117,7 +117,6 @@ extern "C" {
 				 DRM_MODE_FLAG_PCSYNC |		\
 				 DRM_MODE_FLAG_NCSYNC |		\
 				 DRM_MODE_FLAG_HSKEW |		\
-				 DRM_MODE_FLAG_BCAST |		\
 				 DRM_MODE_FLAG_DBLCLK |		\
 				 DRM_MODE_FLAG_CLKDIV2 |	\
 				 DRM_MODE_FLAG_3D_MASK)
-- 
2.13.6

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

[PATCH 05/10] drm/modes: Fix description of DRM_MODE_TYPE_USERDEF

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

These days DRM_MODE_TYPE_USERDEF is used to flag modes defined via the
kernel command line. Update the docs to reflect that fact.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_modes.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 09773e766e1f..8ddf7adb98df 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -253,6 +253,7 @@ struct drm_display_mode {
 	 *  - DRM_MODE_TYPE_DRIVER: Mode created by the driver, which is all of
 	 *    them really. Drivers must set this bit for all modes they create
 	 *    and expose to userspace.
+	 *  - DRM_MODE_TYPE_USERDEF: Mode defined via kernel command line
 	 *
 	 * Plus a big list of flags which shouldn't be used at all, but are
 	 * still around since these flags are also used in the userspace ABI:
@@ -262,9 +263,6 @@ struct drm_display_mode {
 	 *  - DRM_MODE_TYPE_CLOCK_C and DRM_MODE_TYPE_CRTC_C: Define leftovers
 	 *    which are stuck around for hysterical raisins only. No one has an
 	 *    idea what they were meant for. Don't use.
-	 *  - DRM_MODE_TYPE_USERDEF: Mode defined by userspace, again a vestige
-	 *    from older kms designs where userspace had to first add a custom
-	 *    mode to the kernel's mode list before it could use it. Don't use.
 	 */
 	unsigned int type;
 
-- 
2.13.6

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

[PATCH 06/10] drm/modes: Kill off the oddball DRM_MODE_TYPE_CRTC_C vs. DRM_MODE_TYPE_BUILTIN handling

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

For some reason drm_mode_set_crtcinfo() does nothing of the mode has
the DRM_MODE_TYPE_BUILTIN flag set without the other bit from
DRM_MODE_TYPE_CRTC_C also set. I have zero idea what that is supposed
to achieve, but since we have no users for neither flag bit let's kill
this nonsense off.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index f99ba963fb3e..68a8ba4c2c38 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -833,7 +833,7 @@ EXPORT_SYMBOL(drm_mode_get_hv_timing);
  */
 void drm_mode_set_crtcinfo(struct drm_display_mode *p, int adjust_flags)
 {
-	if ((p == NULL) || ((p->type & DRM_MODE_TYPE_CRTC_C) == DRM_MODE_TYPE_BUILTIN))
+	if (!p)
 		return;
 
 	p->crtc_clock = p->clock;
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/10] drm/modes: Kill DRM_MODE_TYPE_CLOCK_CRTC_C define

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

No idea what the DRM_MODE_TYPE_CLOCK_CRTC_C define is supposed to
achieve. Totally unused so kill if off.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_modes.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 8ddf7adb98df..99dd815269e9 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -131,9 +131,6 @@ enum drm_mode_status {
 	MODE_ERROR = -1
 };
 
-#define DRM_MODE_TYPE_CLOCK_CRTC_C (DRM_MODE_TYPE_CLOCK_C | \
-				    DRM_MODE_TYPE_CRTC_C)
-
 #define DRM_MODE(nm, t, c, hd, hss, hse, ht, hsk, vd, vss, vse, vt, vs, f) \
 	.name = nm, .status = 0, .type = (t), .clock = (c), \
 	.hdisplay = (hd), .hsync_start = (hss), .hsync_end = (hse), \
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/10] drm/uapi: Deprecate nonsense kms mode types

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

BUILTIN, CRTC_C, CLOCK_C, and DEFULT mode types are unused. Let's
refuse to generate them or accept them from userspace either. A
cursory check didn't reveal any userspace code that would depend
on these.

Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_modes.h     |  7 ++++---
 include/uapi/drm/drm_mode.h | 14 +++++---------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 99dd815269e9..7ac61858b54e 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -242,8 +242,6 @@ struct drm_display_mode {
 	 * A bitmask of flags, mostly about the source of a mode. Possible flags
 	 * are:
 	 *
-	 *  - DRM_MODE_TYPE_BUILTIN: Meant for hard-coded modes, effectively
-	 *    unused.
 	 *  - DRM_MODE_TYPE_PREFERRED: Preferred mode, usually the native
 	 *    resolution of an LCD panel. There should only be one preferred
 	 *    mode per connector at any given time.
@@ -253,8 +251,11 @@ struct drm_display_mode {
 	 *  - DRM_MODE_TYPE_USERDEF: Mode defined via kernel command line
 	 *
 	 * Plus a big list of flags which shouldn't be used at all, but are
-	 * still around since these flags are also used in the userspace ABI:
+	 * still around since these flags are also used in the userspace ABI.
+	 * We no longer accept modes with these types though:
 	 *
+	 *  - DRM_MODE_TYPE_BUILTIN: Meant for hard-coded modes, effectively
+	 *    unused.
 	 *  - DRM_MODE_TYPE_DEFAULT: Again a leftover, use
 	 *    DRM_MODE_TYPE_PREFERRED instead.
 	 *  - DRM_MODE_TYPE_CLOCK_C and DRM_MODE_TYPE_CRTC_C: Define leftovers
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a7cded1c43e8..eb9b68c7c218 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -38,19 +38,15 @@ extern "C" {
 #define DRM_DISPLAY_MODE_LEN	32
 #define DRM_PROP_NAME_LEN	32
 
-#define DRM_MODE_TYPE_BUILTIN	(1<<0)
-#define DRM_MODE_TYPE_CLOCK_C	((1<<1) | DRM_MODE_TYPE_BUILTIN)
-#define DRM_MODE_TYPE_CRTC_C	((1<<2) | DRM_MODE_TYPE_BUILTIN)
+#define DRM_MODE_TYPE_BUILTIN	(1<<0) /* deprecated */
+#define DRM_MODE_TYPE_CLOCK_C	((1<<1) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
+#define DRM_MODE_TYPE_CRTC_C	((1<<2) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
 #define DRM_MODE_TYPE_PREFERRED	(1<<3)
-#define DRM_MODE_TYPE_DEFAULT	(1<<4)
+#define DRM_MODE_TYPE_DEFAULT	(1<<4) /* deprecated */
 #define DRM_MODE_TYPE_USERDEF	(1<<5)
 #define DRM_MODE_TYPE_DRIVER	(1<<6)
 
-#define DRM_MODE_TYPE_ALL	(DRM_MODE_TYPE_BUILTIN |	\
-				 DRM_MODE_TYPE_CLOCK_C |	\
-				 DRM_MODE_TYPE_CRTC_C |		\
-				 DRM_MODE_TYPE_PREFERRED |	\
-				 DRM_MODE_TYPE_DEFAULT |	\
+#define DRM_MODE_TYPE_ALL	(DRM_MODE_TYPE_PREFERRED |	\
 				 DRM_MODE_TYPE_USERDEF |	\
 				 DRM_MODE_TYPE_DRIVER)
 
-- 
2.13.6

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

[PATCH 09/10] drm/modes: Provide global mode_valid hook

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Allow drivers to provide a device wide .mode_valid() hook in addition to
the already existing crtc/encoder/bridge/connector hooks. This can be
used to validate device/driver wide constraings without having to add
those to the other hooks. And since we call this hook also for user
modes later on in the modeset we don't have to worry about anything the
hook has already rejected.

I also have some further ideas for this hook. Eg. we could replace the
drm_mode_set_crtcinfo(HALVE_V) call in drm_mode_convert_umode()/etc.
with a driver specific variant via this hook. At least on i915 we would
like to pass CRTC_STEREO_DOUBLE to that function instead, and then
we could safely use the crtc_ timings in all our .mode_valid() hooks,
which would allow us to reuse those hooks for validating the
adjusted_mode during a modeset.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c       |  2 +-
 drivers/gpu/drm/drm_crtc.c         |  2 +-
 drivers/gpu/drm/drm_modes.c        | 48 +++++++++++++++++++++++++++-----------
 drivers/gpu/drm/drm_probe_helper.c |  2 +-
 include/drm/drm_mode_config.h      | 12 ++++++++++
 include/drm/drm_modes.h            |  6 +++--
 6 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 37445d50816a..1df2a50c25d5 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -387,7 +387,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
 
 	if (blob) {
 		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
-		    drm_mode_convert_umode(&state->mode,
+		    drm_mode_convert_umode(state->crtc->dev, &state->mode,
 		                           (const struct drm_mode_modeinfo *)
 		                            blob->data))
 			return -EINVAL;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f0556e654116..7eaa3c87761e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -610,7 +610,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			goto out;
 		}
 
-		ret = drm_mode_convert_umode(mode, &crtc_req->mode);
+		ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
 		if (ret) {
 			DRM_DEBUG_KMS("Invalid mode\n");
 			goto out;
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 68a8ba4c2c38..4eb12ff4df17 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1023,17 +1023,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 }
 EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 
-/**
- * drm_mode_validate_basic - make sure the mode is somewhat sane
- * @mode: mode to check
- *
- * Check that the mode timings are at least somewhat reasonable.
- * Any hardware specific limits are left up for each driver to check.
- *
- * Returns:
- * The mode status
- */
-enum drm_mode_status
+static enum drm_mode_status
 drm_mode_validate_basic(const struct drm_display_mode *mode)
 {
 	if (mode->type & ~DRM_MODE_TYPE_ALL ||
@@ -1060,7 +1050,35 @@ drm_mode_validate_basic(const struct drm_display_mode *mode)
 
 	return MODE_OK;
 }
-EXPORT_SYMBOL(drm_mode_validate_basic);
+
+/**
+ * drm_mode_validate_driver - make sure the mode is somewhat sane
+ * @dev: drm device
+ * @mode: mode to check
+ *
+ * First do basic validation on the mode, and then allow the driver
+ * to check for device/driver specific limitations via the optional
+ * &drm_mode_config_helper_funcs.mode_valid hook.
+ *
+ * Returns:
+ * The mode status
+ */
+enum drm_mode_status
+drm_mode_validate_driver(struct drm_device *dev,
+			const struct drm_display_mode *mode)
+{
+	enum drm_mode_status status;
+
+	status = drm_mode_validate_basic(mode);
+	if (status != MODE_OK)
+		return status;
+
+	if (dev->mode_config.funcs->mode_valid)
+		return dev->mode_config.funcs->mode_valid(dev, mode);
+	else
+		return MODE_OK;
+}
+EXPORT_SYMBOL(drm_mode_validate_driver);
 
 /**
  * drm_mode_validate_size - make sure modes adhere to size constraints
@@ -1562,6 +1580,7 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 
 /**
  * drm_crtc_convert_umode - convert a modeinfo into a drm_display_mode
+ * @dev: drm device
  * @out: drm_display_mode to return to the user
  * @in: drm_mode_modeinfo to use
  *
@@ -1571,7 +1590,8 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
  * Returns:
  * Zero on success, negative errno on failure.
  */
-int drm_mode_convert_umode(struct drm_display_mode *out,
+int drm_mode_convert_umode(struct drm_device *dev,
+			   struct drm_display_mode *out,
 			   const struct drm_mode_modeinfo *in)
 {
 	int ret = -EINVAL;
@@ -1598,7 +1618,7 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 
-	out->status = drm_mode_validate_basic(out);
+	out->status = drm_mode_validate_driver(dev, out);
 	if (out->status != MODE_OK)
 		goto out;
 
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 6dc2dde5b672..51303060898e 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -500,7 +500,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 
 	list_for_each_entry(mode, &connector->modes, head) {
 		if (mode->status == MODE_OK)
-			mode->status = drm_mode_validate_basic(mode);
+			mode->status = drm_mode_validate_driver(dev, mode);
 
 		if (mode->status == MODE_OK)
 			mode->status = drm_mode_validate_size(mode, maxX, maxY);
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 0b4ac2ebc610..e134a0682395 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -35,6 +35,7 @@ struct drm_device;
 struct drm_atomic_state;
 struct drm_mode_fb_cmd2;
 struct drm_format_info;
+struct drm_display_mode;
 
 /**
  * struct drm_mode_config_funcs - basic driver provided mode setting functions
@@ -101,6 +102,17 @@ struct drm_mode_config_funcs {
 	void (*output_poll_changed)(struct drm_device *dev);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * Device specific validation of display modes. Can be used to reject
+	 * modes that can never be supports. Only device wide constraints can
+	 * be checked here. crtc/encoder/bridge/connector specific constraints
+	 * can be checked in the .mode_valid() hook for each specific object.
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_device *dev,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @atomic_check:
 	 *
 	 * This is the only hook to validate an atomic modeset update. This
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 7ac61858b54e..f945afaf2313 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -444,7 +444,8 @@ struct drm_display_mode *drm_mode_create(struct drm_device *dev);
 void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode);
 void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 			       const struct drm_display_mode *in);
-int drm_mode_convert_umode(struct drm_display_mode *out,
+int drm_mode_convert_umode(struct drm_device *dev,
+			   struct drm_display_mode *out,
 			   const struct drm_mode_modeinfo *in);
 void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
 void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);
@@ -497,7 +498,8 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 					const struct drm_display_mode *mode2);
 
 /* for use by the crtc helper probe functions */
-enum drm_mode_status drm_mode_validate_basic(const struct drm_display_mode *mode);
+enum drm_mode_status drm_mode_validate_driver(struct drm_device *dev,
+					      const struct drm_display_mode *mode);
 enum drm_mode_status drm_mode_validate_size(const struct drm_display_mode *mode,
 					    int maxX, int maxY);
 enum drm_mode_status
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/10] drm/i915: Provide a device level .mode_valid() hook

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We never support certain mode flags etc. Reject those early on in the
mode_config.mode_valid() hook. That allows us to remove some duplicated
checks from the connector .mode_valid() hooks, and it guarantees that
we never see those flags even from user mode as the
mode_config.mode_valid() hooks gets executed for those as well.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c     |  3 ---
 drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dsi.c     |  5 -----
 drivers/gpu/drm/i915/intel_dvo.c     |  3 ---
 drivers/gpu/drm/i915/intel_fbc.c     |  3 +--
 drivers/gpu/drm/i915/intel_hdmi.c    |  3 ---
 drivers/gpu/drm/i915/intel_sdvo.c    |  3 ---
 7 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 9f31aea51dff..1cd4a7c22bd5 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -304,9 +304,6 @@ intel_crt_mode_valid(struct drm_connector *connector,
 	int max_dotclk = dev_priv->max_dotclk_freq;
 	int max_clock;
 
-	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
-		return MODE_NO_DBLESCAN;
-
 	if (mode->clock < 25000)
 		return MODE_CLOCK_LOW;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ed6a4a8d9273..fec76147d8e5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14151,10 +14151,37 @@ static void intel_atomic_state_free(struct drm_atomic_state *state)
 	kfree(state);
 }
 
+static enum drm_mode_status
+intel_mode_valid(struct drm_device *dev,
+		 const struct drm_display_mode *mode)
+{
+	if (mode->vscan > 1)
+		return MODE_NO_VSCAN;
+
+	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
+		return MODE_NO_DBLESCAN;
+
+	if (mode->flags & DRM_MODE_FLAG_HSKEW)
+		return MODE_H_ILLEGAL;
+
+	if (mode->flags & (DRM_MODE_FLAG_CSYNC |
+			   DRM_MODE_FLAG_NCSYNC |
+			   DRM_MODE_FLAG_PCSYNC))
+		return MODE_HSYNC;
+
+	if (mode->flags & (DRM_MODE_FLAG_BCAST |
+			   DRM_MODE_FLAG_PIXMUX |
+			   DRM_MODE_FLAG_CLKDIV2))
+		return MODE_BAD;
+
+	return MODE_OK;
+}
+
 static const struct drm_mode_config_funcs intel_mode_funcs = {
 	.fb_create = intel_user_framebuffer_create,
 	.get_format_info = intel_get_format_info,
 	.output_poll_changed = intel_fbdev_output_poll_changed,
+	.mode_valid = intel_mode_valid,
 	.atomic_check = intel_atomic_check,
 	.atomic_commit = intel_atomic_commit,
 	.atomic_state_alloc = intel_atomic_state_alloc,
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index f09474b0c4d3..c9b5a956c30a 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1266,11 +1266,6 @@ intel_dsi_mode_valid(struct drm_connector *connector,
 
 	DRM_DEBUG_KMS("\n");
 
-	if (mode->flags & DRM_MODE_FLAG_DBLSCAN) {
-		DRM_DEBUG_KMS("MODE_NO_DBLESCAN\n");
-		return MODE_NO_DBLESCAN;
-	}
-
 	if (fixed_mode) {
 		if (mode->hdisplay > fixed_mode->hdisplay)
 			return MODE_PANEL;
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 754baa00bea9..59c066ca14e5 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -219,9 +219,6 @@ intel_dvo_mode_valid(struct drm_connector *connector,
 	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
 	int target_clock = mode->clock;
 
-	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
-		return MODE_NO_DBLESCAN;
-
 	/* XXX: Validate clock range */
 
 	if (fixed_mode) {
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 1a0f5e0c8d10..f0e74477c678 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -789,8 +789,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 		return false;
 	}
 
-	if ((cache->crtc.mode_flags & DRM_MODE_FLAG_INTERLACE) ||
-	    (cache->crtc.mode_flags & DRM_MODE_FLAG_DBLSCAN)) {
+	if (cache->crtc.mode_flags & DRM_MODE_FLAG_INTERLACE) {
 		fbc->no_fbc_reason = "incompatible mode";
 		return false;
 	}
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 2d95db64cdf2..7ce3d14153b4 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1311,9 +1311,6 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
 	bool force_dvi =
 		READ_ONCE(to_intel_digital_connector_state(connector->state)->force_audio) == HDMI_AUDIO_OFF_DVI;
 
-	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
-		return MODE_NO_DBLESCAN;
-
 	clock = mode->clock;
 
 	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING)
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 2b8764897d68..0bf97ed5ffac 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1607,9 +1607,6 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
 	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
 	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
 
-	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
-		return MODE_NO_DBLESCAN;
-
 	if (intel_sdvo->pixel_clock_min > mode->clock)
 		return MODE_CLOCK_LOW;
 
-- 
2.13.6

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

Re: [Intel-gfx] [PATCH 02/10] drm/uapi: Validate the mode flags/type

[Chris Wilson]

Quoting Ville Syrjala (2017-11-14 18:32:50)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently userspace is allowed to feed in any king of garbage in the
> high bits of the mode flags/type, as are drivers when probing modes.
> Reject any mode with bogus flags/type.
> 
> Hopefully this won't break any current userspace...
> 
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_modes.c |  4 ++++
>  include/uapi/drm/drm_mode.h | 24 ++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 1a72883b836e..f99ba963fb3e 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1036,6 +1036,10 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
>  enum drm_mode_status
>  drm_mode_validate_basic(const struct drm_display_mode *mode)
>  {
> +       if (mode->type & ~DRM_MODE_TYPE_ALL ||
> +           mode->flags & ~DRM_MODE_FLAG_ALL)
> +               return MODE_BAD;

I had to read this twice to realise they were different masks. (If the
start and end of a word match expectations, the eye skips the middle.)
Can we split this up into two separate ifs, so the reader doesn't fall
into this trap :)
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/10] drm/modes: Kill off the oddball DRM_MODE_TYPE_CRTC_C vs. DRM_MODE_TYPE_BUILTIN handling

[Chris Wilson]

Quoting Ville Syrjala (2017-11-14 18:32:54)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> For some reason drm_mode_set_crtcinfo() does nothing of the mode has
s/of/if/

> the DRM_MODE_TYPE_BUILTIN flag set without the other bit from
> DRM_MODE_TYPE_CRTC_C also set. I have zero idea what that is supposed
> to achieve, but since we have no users for neither flag bit let's kill
> this nonsense off.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm/uapi: Validate the mode flags/type

[Ville Syrjälä]

On Tue, Nov 14, 2017 at 06:42:06PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-11-14 18:32:50)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently userspace is allowed to feed in any king of garbage in the
> > high bits of the mode flags/type, as are drivers when probing modes.
> > Reject any mode with bogus flags/type.
> > 
> > Hopefully this won't break any current userspace...
> > 
> > Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> > Cc: Adam Jackson <ajax@redhat.com>
> > Cc: Keith Packard <keithp@keithp.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_modes.c |  4 ++++
> >  include/uapi/drm/drm_mode.h | 24 ++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 1a72883b836e..f99ba963fb3e 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1036,6 +1036,10 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
> >  enum drm_mode_status
> >  drm_mode_validate_basic(const struct drm_display_mode *mode)
> >  {
> > +       if (mode->type & ~DRM_MODE_TYPE_ALL ||
> > +           mode->flags & ~DRM_MODE_FLAG_ALL)
> > +               return MODE_BAD;
> 
> I had to read this twice to realise they were different masks. (If the
> start and end of a word match expectations, the eye skips the middle.)
> Can we split this up into two separate ifs, so the reader doesn't fall
> into this trap :)

Sure, I can split that up.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/10] drm/uapi: Deprecate DRM_MODE_FLAG_PIXMUX

[Alex Deucher]

On Tue, Nov 14, 2017 at 1:32 PM, Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Reject any mode with DRM_MODE_FLAG_PIXMUX. We have no code that even
> checks for this flag hence it can't possibly do any good.
>
> Looks like this flag had something to do the the controller<->ramdac
> interface with some ancient S3 graphics adapters. Why someone though
> it would be a good idea to expose it directly to users I don't know.
> And later on it got copied into the randr protocol and kms uapi.
>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  include/drm/drm_modes.h     | 2 +-
>  include/uapi/drm/drm_mode.h | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 9f3421c8efcd..bce573375dd8 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -300,7 +300,7 @@ struct drm_display_mode {
>          *  - DRM_MODE_FLAG_NCSYNC: composite sync is active low.
>          *  - DRM_MODE_FLAG_HSKEW: hskew provided (not used?).
>          *  - DRM_MODE_FLAG_BCAST: not used?
> -        *  - DRM_MODE_FLAG_PIXMUX: not used?
> +        *  - DRM_MODE_FLAG_PIXMUX: <deprecated>
>          *  - DRM_MODE_FLAG_DBLCLK: double-clocked mode.
>          *  - DRM_MODE_FLAG_CLKDIV2: half-clocked mode.
>          *
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 004db470b477..8d872e17223e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -75,7 +75,7 @@ extern "C" {
>  #define DRM_MODE_FLAG_NCSYNC                   (1<<8)
>  #define DRM_MODE_FLAG_HSKEW                    (1<<9) /* hskew provided */
>  #define DRM_MODE_FLAG_BCAST                    (1<<10)
> -#define DRM_MODE_FLAG_PIXMUX                   (1<<11)
> +#define DRM_MODE_FLAG_PIXMUX                   (1<<11) /* deprecated */
>  #define DRM_MODE_FLAG_DBLCLK                   (1<<12)
>  #define DRM_MODE_FLAG_CLKDIV2                  (1<<13)
>   /*
> @@ -118,7 +118,6 @@ extern "C" {
>                                  DRM_MODE_FLAG_NCSYNC |         \
>                                  DRM_MODE_FLAG_HSKEW |          \
>                                  DRM_MODE_FLAG_BCAST |          \
> -                                DRM_MODE_FLAG_PIXMUX |         \
>                                  DRM_MODE_FLAG_DBLCLK |         \
>                                  DRM_MODE_FLAG_CLKDIV2 |        \
>                                  DRM_MODE_FLAG_3D_MASK)
> --
> 2.13.6
>
> _______________________________________________
> 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 04/10] drm/uapi: Deprecate DRM_MODE_FLAG_BCAST

[Alex Deucher]

On Tue, Nov 14, 2017 at 1:32 PM, Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Reject any mode with DRM_MODE_FLAG_BCAST. We have no code that even
> checks for this flag hence it can't possibly do any good.
>
> I think this maybe originated from fbdev where it was supposed to
> indicate PAL/NTSC broadcast timings. I have no idea why those would
> have to be identified by a flag rather than by just the timings
> themselves. And then I assume it got copied into xfree86 for
> fbdevhw, and later on it leaked into the randr protocol and kms uapi.
>
> Since kms fbdev emulation never uses the corresponding fbdev flag
> there should be no sane way for this to come back into kms via
> userspace either.
>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  include/drm/drm_modes.h     | 2 +-
>  include/uapi/drm/drm_mode.h | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index bce573375dd8..09773e766e1f 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -299,7 +299,7 @@ struct drm_display_mode {
>          *  - DRM_MODE_FLAG_PCSYNC: composite sync is active high.
>          *  - DRM_MODE_FLAG_NCSYNC: composite sync is active low.
>          *  - DRM_MODE_FLAG_HSKEW: hskew provided (not used?).
> -        *  - DRM_MODE_FLAG_BCAST: not used?
> +        *  - DRM_MODE_FLAG_BCAST: <deprecated>
>          *  - DRM_MODE_FLAG_PIXMUX: <deprecated>
>          *  - DRM_MODE_FLAG_DBLCLK: double-clocked mode.
>          *  - DRM_MODE_FLAG_CLKDIV2: half-clocked mode.
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 8d872e17223e..a7cded1c43e8 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -74,7 +74,7 @@ extern "C" {
>  #define DRM_MODE_FLAG_PCSYNC                   (1<<7)
>  #define DRM_MODE_FLAG_NCSYNC                   (1<<8)
>  #define DRM_MODE_FLAG_HSKEW                    (1<<9) /* hskew provided */
> -#define DRM_MODE_FLAG_BCAST                    (1<<10)
> +#define DRM_MODE_FLAG_BCAST                    (1<<10) /* deprecated */
>  #define DRM_MODE_FLAG_PIXMUX                   (1<<11) /* deprecated */
>  #define DRM_MODE_FLAG_DBLCLK                   (1<<12)
>  #define DRM_MODE_FLAG_CLKDIV2                  (1<<13)
> @@ -117,7 +117,6 @@ extern "C" {
>                                  DRM_MODE_FLAG_PCSYNC |         \
>                                  DRM_MODE_FLAG_NCSYNC |         \
>                                  DRM_MODE_FLAG_HSKEW |          \
> -                                DRM_MODE_FLAG_BCAST |          \
>                                  DRM_MODE_FLAG_DBLCLK |         \
>                                  DRM_MODE_FLAG_CLKDIV2 |        \
>                                  DRM_MODE_FLAG_3D_MASK)
> --
> 2.13.6
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/10] drm/modes: Kill off the oddball DRM_MODE_TYPE_CRTC_C vs. DRM_MODE_TYPE_BUILTIN handling

[Alex Deucher]

On Tue, Nov 14, 2017 at 1:43 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Ville Syrjala (2017-11-14 18:32:54)
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> For some reason drm_mode_set_crtcinfo() does nothing of the mode has
> s/of/if/

With the typo fixed:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
>> the DRM_MODE_TYPE_BUILTIN flag set without the other bit from
>> DRM_MODE_TYPE_CRTC_C also set. I have zero idea what that is supposed
>> to achieve, but since we have no users for neither flag bit let's kill
>> this nonsense off.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/10] drm/modes: Kill DRM_MODE_TYPE_CLOCK_CRTC_C define

[Alex Deucher]

On Tue, Nov 14, 2017 at 1:32 PM, Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> No idea what the DRM_MODE_TYPE_CLOCK_CRTC_C define is supposed to
> achieve. Totally unused so kill if off.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  include/drm/drm_modes.h | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 8ddf7adb98df..99dd815269e9 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -131,9 +131,6 @@ enum drm_mode_status {
>         MODE_ERROR = -1
>  };
>
> -#define DRM_MODE_TYPE_CLOCK_CRTC_C (DRM_MODE_TYPE_CLOCK_C | \
> -                                   DRM_MODE_TYPE_CRTC_C)
> -
>  #define DRM_MODE(nm, t, c, hd, hss, hse, ht, hsk, vd, vss, vse, vt, vs, f) \
>         .name = nm, .status = 0, .type = (t), .clock = (c), \
>         .hdisplay = (hd), .hsync_start = (hss), .hsync_end = (hse), \
> --
> 2.13.6
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/10] drm/uapi: Deprecate nonsense kms mode types

[Alex Deucher]

On Tue, Nov 14, 2017 at 1:32 PM, Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> BUILTIN, CRTC_C, CLOCK_C, and DEFULT mode types are unused. Let's
> refuse to generate them or accept them from userspace either. A
> cursory check didn't reveal any userspace code that would depend
> on these.
>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  include/drm/drm_modes.h     |  7 ++++---
>  include/uapi/drm/drm_mode.h | 14 +++++---------
>  2 files changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 99dd815269e9..7ac61858b54e 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -242,8 +242,6 @@ struct drm_display_mode {
>          * A bitmask of flags, mostly about the source of a mode. Possible flags
>          * are:
>          *
> -        *  - DRM_MODE_TYPE_BUILTIN: Meant for hard-coded modes, effectively
> -        *    unused.
>          *  - DRM_MODE_TYPE_PREFERRED: Preferred mode, usually the native
>          *    resolution of an LCD panel. There should only be one preferred
>          *    mode per connector at any given time.
> @@ -253,8 +251,11 @@ struct drm_display_mode {
>          *  - DRM_MODE_TYPE_USERDEF: Mode defined via kernel command line
>          *
>          * Plus a big list of flags which shouldn't be used at all, but are
> -        * still around since these flags are also used in the userspace ABI:
> +        * still around since these flags are also used in the userspace ABI.
> +        * We no longer accept modes with these types though:
>          *
> +        *  - DRM_MODE_TYPE_BUILTIN: Meant for hard-coded modes, effectively
> +        *    unused.
>          *  - DRM_MODE_TYPE_DEFAULT: Again a leftover, use
>          *    DRM_MODE_TYPE_PREFERRED instead.
>          *  - DRM_MODE_TYPE_CLOCK_C and DRM_MODE_TYPE_CRTC_C: Define leftovers
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index a7cded1c43e8..eb9b68c7c218 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -38,19 +38,15 @@ extern "C" {
>  #define DRM_DISPLAY_MODE_LEN   32
>  #define DRM_PROP_NAME_LEN      32
>
> -#define DRM_MODE_TYPE_BUILTIN  (1<<0)
> -#define DRM_MODE_TYPE_CLOCK_C  ((1<<1) | DRM_MODE_TYPE_BUILTIN)
> -#define DRM_MODE_TYPE_CRTC_C   ((1<<2) | DRM_MODE_TYPE_BUILTIN)
> +#define DRM_MODE_TYPE_BUILTIN  (1<<0) /* deprecated */
> +#define DRM_MODE_TYPE_CLOCK_C  ((1<<1) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
> +#define DRM_MODE_TYPE_CRTC_C   ((1<<2) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
>  #define DRM_MODE_TYPE_PREFERRED        (1<<3)
> -#define DRM_MODE_TYPE_DEFAULT  (1<<4)
> +#define DRM_MODE_TYPE_DEFAULT  (1<<4) /* deprecated */
>  #define DRM_MODE_TYPE_USERDEF  (1<<5)
>  #define DRM_MODE_TYPE_DRIVER   (1<<6)
>
> -#define DRM_MODE_TYPE_ALL      (DRM_MODE_TYPE_BUILTIN |        \
> -                                DRM_MODE_TYPE_CLOCK_C |        \
> -                                DRM_MODE_TYPE_CRTC_C |         \
> -                                DRM_MODE_TYPE_PREFERRED |      \
> -                                DRM_MODE_TYPE_DEFAULT |        \
> +#define DRM_MODE_TYPE_ALL      (DRM_MODE_TYPE_PREFERRED |      \
>                                  DRM_MODE_TYPE_USERDEF |        \
>                                  DRM_MODE_TYPE_DRIVER)
>
> --
> 2.13.6
>
> _______________________________________________
> 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 01/10] drm/modes: Move 3D stereo flag check into drm_mode_validate_basic()

[Alex Deucher]

On Tue, Nov 14, 2017 at 1:32 PM, Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently we don't sanity check the 3D stereo flags for modes filled out
> by the kernel. Move the check from drm_mode_convert_umode() into
> drm_mode_validate_basic() so that we get the same check going both ways.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_modes.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 4a3f68a33844..1a72883b836e 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1036,6 +1036,9 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
>  enum drm_mode_status
>  drm_mode_validate_basic(const struct drm_display_mode *mode)
>  {
> +       if ((mode->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
> +               return MODE_BAD;
> +
>         if (mode->clock == 0)
>                 return MODE_CLOCK_LOW;
>
> @@ -1574,9 +1577,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>                 goto out;
>         }
>
> -       if ((in->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
> -               goto out;
> -
>         out->clock = in->clock;
>         out->hdisplay = in->hdisplay;
>         out->hsync_start = in->hsync_start;
> --
> 2.13.6
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/10] drm/modes: Fix description of DRM_MODE_TYPE_USERDEF

[Alex Deucher]

On Tue, Nov 14, 2017 at 1:32 PM, Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> These days DRM_MODE_TYPE_USERDEF is used to flag modes defined via the
> kernel command line. Update the docs to reflect that fact.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  include/drm/drm_modes.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 09773e766e1f..8ddf7adb98df 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -253,6 +253,7 @@ struct drm_display_mode {
>          *  - DRM_MODE_TYPE_DRIVER: Mode created by the driver, which is all of
>          *    them really. Drivers must set this bit for all modes they create
>          *    and expose to userspace.
> +        *  - DRM_MODE_TYPE_USERDEF: Mode defined via kernel command line
>          *
>          * Plus a big list of flags which shouldn't be used at all, but are
>          * still around since these flags are also used in the userspace ABI:
> @@ -262,9 +263,6 @@ struct drm_display_mode {
>          *  - DRM_MODE_TYPE_CLOCK_C and DRM_MODE_TYPE_CRTC_C: Define leftovers
>          *    which are stuck around for hysterical raisins only. No one has an
>          *    idea what they were meant for. Don't use.
> -        *  - DRM_MODE_TYPE_USERDEF: Mode defined by userspace, again a vestige
> -        *    from older kms designs where userspace had to first add a custom
> -        *    mode to the kernel's mode list before it could use it. Don't use.
>          */
>         unsigned int type;
>
> --
> 2.13.6
>
> _______________________________________________
> 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 08/10] drm/uapi: Deprecate nonsense kms mode types

[Adam Jackson]

On Tue, 2017-11-14 at 20:32 +0200, Ville Syrjala wrote:

> @@ -253,8 +251,11 @@ struct drm_display_mode {
>  	 *  - DRM_MODE_TYPE_USERDEF: Mode defined via kernel command line
>  	 *
>  	 * Plus a big list of flags which shouldn't be used at all, but are
> -	 * still around since these flags are also used in the userspace ABI:
> +	 * still around since these flags are also used in the userspace ABI.
> +	 * We no longer accept modes with these types though:
>  	 *
> +	 *  - DRM_MODE_TYPE_BUILTIN: Meant for hard-coded modes, effectively
> +	 *    unused.

This should suggest _TYPE_DRIVER, I think. The intent with the xfree86
mode type was that "built-in" modes were known quantities of the
hardware, either because the hardware only had one mode or due to
strapping pins or the like. These should be considered "supplied by the
driver" as with EDID-derived modes.

With or without that, patches 2 3 4 and 8 are:

Reviewed-by: Adam Jackson <ajax@redhat.com>

- ajax
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 02/10] drm/uapi: Validate the mode flags/type

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently userspace is allowed to feed in any king of garbage in the
high bits of the mode flags/type, as are drivers when probing modes.
Reject any mode with bogus flags/type.

Hopefully this won't break any current userspace...

v2: Split the type and flags checks to separates ifs (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
---
 drivers/gpu/drm/drm_modes.c |  6 ++++++
 include/uapi/drm/drm_mode.h | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 1a72883b836e..34b5123ebfc0 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1036,6 +1036,12 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 enum drm_mode_status
 drm_mode_validate_basic(const struct drm_display_mode *mode)
 {
+	if (mode->type & ~DRM_MODE_TYPE_ALL)
+		return MODE_BAD;
+
+	if (mode->flags & ~DRM_MODE_FLAG_ALL)
+		return MODE_BAD;
+
 	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
 		return MODE_BAD;
 
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 5597a87154e5..004db470b477 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -46,6 +46,14 @@ extern "C" {
 #define DRM_MODE_TYPE_USERDEF	(1<<5)
 #define DRM_MODE_TYPE_DRIVER	(1<<6)
 
+#define DRM_MODE_TYPE_ALL	(DRM_MODE_TYPE_BUILTIN |	\
+				 DRM_MODE_TYPE_CLOCK_C |	\
+				 DRM_MODE_TYPE_CRTC_C |		\
+				 DRM_MODE_TYPE_PREFERRED |	\
+				 DRM_MODE_TYPE_DEFAULT |	\
+				 DRM_MODE_TYPE_USERDEF |	\
+				 DRM_MODE_TYPE_DRIVER)
+
 /* Video mode flags */
 /* bit compatible with the xrandr RR_ definitions (bits 0-13)
  *
@@ -99,6 +107,22 @@ extern "C" {
 #define  DRM_MODE_FLAG_PIC_AR_16_9 \
 			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
 
+#define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
+				 DRM_MODE_FLAG_NHSYNC |		\
+				 DRM_MODE_FLAG_PVSYNC |		\
+				 DRM_MODE_FLAG_NVSYNC |		\
+				 DRM_MODE_FLAG_INTERLACE |	\
+				 DRM_MODE_FLAG_DBLSCAN |	\
+				 DRM_MODE_FLAG_CSYNC |		\
+				 DRM_MODE_FLAG_PCSYNC |		\
+				 DRM_MODE_FLAG_NCSYNC |		\
+				 DRM_MODE_FLAG_HSKEW |		\
+				 DRM_MODE_FLAG_BCAST |		\
+				 DRM_MODE_FLAG_PIXMUX |		\
+				 DRM_MODE_FLAG_DBLCLK |		\
+				 DRM_MODE_FLAG_CLKDIV2 |	\
+				 DRM_MODE_FLAG_3D_MASK)
+
 /* DPMS flags */
 /* bit compatible with the xorg definitions. */
 #define DRM_MODE_DPMS_ON	0
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 02/10] drm/uapi: Validate the mode flags/type

[Alex Deucher]

On Wed, Nov 15, 2017 at 10:49 AM, Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently userspace is allowed to feed in any king of garbage in the
> high bits of the mode flags/type, as are drivers when probing modes.
> Reject any mode with bogus flags/type.
>
> Hopefully this won't break any current userspace...
>
> v2: Split the type and flags checks to separates ifs (Chris)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Adam Jackson <ajax@redhat.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_modes.c |  6 ++++++
>  include/uapi/drm/drm_mode.h | 24 ++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 1a72883b836e..34b5123ebfc0 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1036,6 +1036,12 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
>  enum drm_mode_status
>  drm_mode_validate_basic(const struct drm_display_mode *mode)
>  {
> +       if (mode->type & ~DRM_MODE_TYPE_ALL)
> +               return MODE_BAD;
> +
> +       if (mode->flags & ~DRM_MODE_FLAG_ALL)
> +               return MODE_BAD;
> +
>         if ((mode->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
>                 return MODE_BAD;
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 5597a87154e5..004db470b477 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -46,6 +46,14 @@ extern "C" {
>  #define DRM_MODE_TYPE_USERDEF  (1<<5)
>  #define DRM_MODE_TYPE_DRIVER   (1<<6)
>
> +#define DRM_MODE_TYPE_ALL      (DRM_MODE_TYPE_BUILTIN |        \
> +                                DRM_MODE_TYPE_CLOCK_C |        \
> +                                DRM_MODE_TYPE_CRTC_C |         \
> +                                DRM_MODE_TYPE_PREFERRED |      \
> +                                DRM_MODE_TYPE_DEFAULT |        \
> +                                DRM_MODE_TYPE_USERDEF |        \
> +                                DRM_MODE_TYPE_DRIVER)
> +
>  /* Video mode flags */
>  /* bit compatible with the xrandr RR_ definitions (bits 0-13)
>   *
> @@ -99,6 +107,22 @@ extern "C" {
>  #define  DRM_MODE_FLAG_PIC_AR_16_9 \
>                         (DRM_MODE_PICTURE_ASPECT_16_9<<19)
>
> +#define  DRM_MODE_FLAG_ALL     (DRM_MODE_FLAG_PHSYNC |         \
> +                                DRM_MODE_FLAG_NHSYNC |         \
> +                                DRM_MODE_FLAG_PVSYNC |         \
> +                                DRM_MODE_FLAG_NVSYNC |         \
> +                                DRM_MODE_FLAG_INTERLACE |      \
> +                                DRM_MODE_FLAG_DBLSCAN |        \
> +                                DRM_MODE_FLAG_CSYNC |          \
> +                                DRM_MODE_FLAG_PCSYNC |         \
> +                                DRM_MODE_FLAG_NCSYNC |         \
> +                                DRM_MODE_FLAG_HSKEW |          \
> +                                DRM_MODE_FLAG_BCAST |          \
> +                                DRM_MODE_FLAG_PIXMUX |         \
> +                                DRM_MODE_FLAG_DBLCLK |         \
> +                                DRM_MODE_FLAG_CLKDIV2 |        \
> +                                DRM_MODE_FLAG_3D_MASK)
> +
>  /* DPMS flags */
>  /* bit compatible with the xorg definitions. */
>  #define DRM_MODE_DPMS_ON       0
> --
> 2.13.6
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 02/10] drm/uapi: Validate the mode flags/type

[Jose Abreu]

Hi Ville,

On 15-11-2017 15:49, Ville Syrjala wrote:
>  
> +#define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
> +				 DRM_MODE_FLAG_NHSYNC |		\
> +				 DRM_MODE_FLAG_PVSYNC |		\
> +				 DRM_MODE_FLAG_NVSYNC |		\
> +				 DRM_MODE_FLAG_INTERLACE |	\
> +				 DRM_MODE_FLAG_DBLSCAN |	\
> +				 DRM_MODE_FLAG_CSYNC |		\
> +				 DRM_MODE_FLAG_PCSYNC |		\
> +				 DRM_MODE_FLAG_NCSYNC |		\
> +				 DRM_MODE_FLAG_HSKEW |		\
> +				 DRM_MODE_FLAG_BCAST |		\
> +				 DRM_MODE_FLAG_PIXMUX |		\
> +				 DRM_MODE_FLAG_DBLCLK |		\
> +				 DRM_MODE_FLAG_CLKDIV2 |	\
> +				 DRM_MODE_FLAG_3D_MASK)
> +
>  

I see this doesn't include the picture aspect ratio flags.
Shouldn't we add this now so that userspace can start using them?

Best Regards,
Jose Miguel Abreu
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 08/10] drm/uapi: Deprecate nonsense kms mode types

[Jose Abreu]

On 15-11-2017 15:45, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> BUILTIN, CRTC_C, CLOCK_C, and DEFULT mode types are unused. Let's
> refuse to generate them or accept them from userspace either. A
> cursory check didn't reveal any userspace code that would depend
> on these.
>
> v2: Recommend DRIVER instead of BUILTIN (ajax)
>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Adam Jackson <ajax@redhat.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>

Reviewed-by: Jose Abreu <joabreu@synopsys.com>

Best Regards,
Jose Miguel Abreu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/10] drm/uapi: Deprecate DRM_MODE_FLAG_BCAST

[Jose Abreu]

On 14-11-2017 18:32, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Reject any mode with DRM_MODE_FLAG_BCAST. We have no code that even
> checks for this flag hence it can't possibly do any good.
>
> I think this maybe originated from fbdev where it was supposed to
> indicate PAL/NTSC broadcast timings. I have no idea why those would
> have to be identified by a flag rather than by just the timings
> themselves. And then I assume it got copied into xfree86 for
> fbdevhw, and later on it leaked into the randr protocol and kms uapi.
>
> Since kms fbdev emulation never uses the corresponding fbdev flag
> there should be no sane way for this to come back into kms via
> userspace either.
>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>

Reviewed-by: Jose Abreu <joabreu@synopsys.com>

Best Regards,
Jose Miguel Abreu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/10] drm/uapi: Deprecate DRM_MODE_FLAG_PIXMUX

[Jose Abreu]

On 14-11-2017 18:32, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Reject any mode with DRM_MODE_FLAG_PIXMUX. We have no code that even
> checks for this flag hence it can't possibly do any good.
>
> Looks like this flag had something to do the the controller<->ramdac
> interface with some ancient S3 graphics adapters. Why someone though
> it would be a good idea to expose it directly to users I don't know.
> And later on it got copied into the randr protocol and kms uapi.
>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>

Reviewed-by: Jose Abreu <joabreu@synopsys.com>

Best Regards,
Jose Miguel Abreu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 02/10] drm/uapi: Validate the mode flags/type

[Ville Syrjälä]

On Wed, Nov 15, 2017 at 06:02:59PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> On 15-11-2017 15:49, Ville Syrjala wrote:
> >  
> > +#define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
> > +				 DRM_MODE_FLAG_NHSYNC |		\
> > +				 DRM_MODE_FLAG_PVSYNC |		\
> > +				 DRM_MODE_FLAG_NVSYNC |		\
> > +				 DRM_MODE_FLAG_INTERLACE |	\
> > +				 DRM_MODE_FLAG_DBLSCAN |	\
> > +				 DRM_MODE_FLAG_CSYNC |		\
> > +				 DRM_MODE_FLAG_PCSYNC |		\
> > +				 DRM_MODE_FLAG_NCSYNC |		\
> > +				 DRM_MODE_FLAG_HSKEW |		\
> > +				 DRM_MODE_FLAG_BCAST |		\
> > +				 DRM_MODE_FLAG_PIXMUX |		\
> > +				 DRM_MODE_FLAG_DBLCLK |		\
> > +				 DRM_MODE_FLAG_CLKDIV2 |	\
> > +				 DRM_MODE_FLAG_3D_MASK)
> > +
> >  
> 
> I see this doesn't include the picture aspect ratio flags.
> Shouldn't we add this now so that userspace can start using them?

Not until we have the client cap to keep them hidden from userspace that
isn't prepared to see them.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/10] drm/modes: Provide global mode_valid hook

[Daniel Vetter]

On Tue, Nov 14, 2017 at 08:32:57PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Allow drivers to provide a device wide .mode_valid() hook in addition to
> the already existing crtc/encoder/bridge/connector hooks. This can be
> used to validate device/driver wide constraings without having to add
> those to the other hooks. And since we call this hook also for user
> modes later on in the modeset we don't have to worry about anything the
> hook has already rejected.
> 
> I also have some further ideas for this hook. Eg. we could replace the
> drm_mode_set_crtcinfo(HALVE_V) call in drm_mode_convert_umode()/etc.
> with a driver specific variant via this hook. At least on i915 we would
> like to pass CRTC_STEREO_DOUBLE to that function instead, and then
> we could safely use the crtc_ timings in all our .mode_valid() hooks,
> which would allow us to reuse those hooks for validating the
> adjusted_mode during a modeset.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c       |  2 +-
>  drivers/gpu/drm/drm_crtc.c         |  2 +-
>  drivers/gpu/drm/drm_modes.c        | 48 +++++++++++++++++++++++++++-----------
>  drivers/gpu/drm/drm_probe_helper.c |  2 +-
>  include/drm/drm_mode_config.h      | 12 ++++++++++
>  include/drm/drm_modes.h            |  6 +++--
>  6 files changed, 53 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..1df2a50c25d5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -387,7 +387,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>  
>  	if (blob) {
>  		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
> -		    drm_mode_convert_umode(&state->mode,
> +		    drm_mode_convert_umode(state->crtc->dev, &state->mode,
>  		                           (const struct drm_mode_modeinfo *)
>  		                            blob->data))
>  			return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f0556e654116..7eaa3c87761e 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -610,7 +610,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			goto out;
>  		}
>  
> -		ret = drm_mode_convert_umode(mode, &crtc_req->mode);
> +		ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
>  		if (ret) {
>  			DRM_DEBUG_KMS("Invalid mode\n");
>  			goto out;
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 68a8ba4c2c38..4eb12ff4df17 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1023,17 +1023,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  }
>  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
>  
> -/**
> - * drm_mode_validate_basic - make sure the mode is somewhat sane
> - * @mode: mode to check
> - *
> - * Check that the mode timings are at least somewhat reasonable.
> - * Any hardware specific limits are left up for each driver to check.
> - *
> - * Returns:
> - * The mode status
> - */
> -enum drm_mode_status
> +static enum drm_mode_status
>  drm_mode_validate_basic(const struct drm_display_mode *mode)
>  {
>  	if (mode->type & ~DRM_MODE_TYPE_ALL ||
> @@ -1060,7 +1050,35 @@ drm_mode_validate_basic(const struct drm_display_mode *mode)
>  
>  	return MODE_OK;
>  }
> -EXPORT_SYMBOL(drm_mode_validate_basic);
> +
> +/**
> + * drm_mode_validate_driver - make sure the mode is somewhat sane
> + * @dev: drm device
> + * @mode: mode to check
> + *
> + * First do basic validation on the mode, and then allow the driver
> + * to check for device/driver specific limitations via the optional
> + * &drm_mode_config_helper_funcs.mode_valid hook.
> + *
> + * Returns:
> + * The mode status
> + */
> +enum drm_mode_status
> +drm_mode_validate_driver(struct drm_device *dev,
> +			const struct drm_display_mode *mode)
> +{
> +	enum drm_mode_status status;
> +
> +	status = drm_mode_validate_basic(mode);
> +	if (status != MODE_OK)
> +		return status;
> +
> +	if (dev->mode_config.funcs->mode_valid)
> +		return dev->mode_config.funcs->mode_valid(dev, mode);
> +	else
> +		return MODE_OK;
> +}
> +EXPORT_SYMBOL(drm_mode_validate_driver);
>  
>  /**
>   * drm_mode_validate_size - make sure modes adhere to size constraints
> @@ -1562,6 +1580,7 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>  
>  /**
>   * drm_crtc_convert_umode - convert a modeinfo into a drm_display_mode
> + * @dev: drm device
>   * @out: drm_display_mode to return to the user
>   * @in: drm_mode_modeinfo to use
>   *
> @@ -1571,7 +1590,8 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>   * Returns:
>   * Zero on success, negative errno on failure.
>   */
> -int drm_mode_convert_umode(struct drm_display_mode *out,
> +int drm_mode_convert_umode(struct drm_device *dev,
> +			   struct drm_display_mode *out,
>  			   const struct drm_mode_modeinfo *in)
>  {
>  	int ret = -EINVAL;
> @@ -1598,7 +1618,7 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  
> -	out->status = drm_mode_validate_basic(out);
> +	out->status = drm_mode_validate_driver(dev, out);
>  	if (out->status != MODE_OK)
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 6dc2dde5b672..51303060898e 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -500,7 +500,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  
>  	list_for_each_entry(mode, &connector->modes, head) {
>  		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_basic(mode);
> +			mode->status = drm_mode_validate_driver(dev, mode);
>  
>  		if (mode->status == MODE_OK)
>  			mode->status = drm_mode_validate_size(mode, maxX, maxY);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 0b4ac2ebc610..e134a0682395 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -35,6 +35,7 @@ struct drm_device;
>  struct drm_atomic_state;
>  struct drm_mode_fb_cmd2;
>  struct drm_format_info;
> +struct drm_display_mode;
>  
>  /**
>   * struct drm_mode_config_funcs - basic driver provided mode setting functions
> @@ -101,6 +102,17 @@ struct drm_mode_config_funcs {
>  	void (*output_poll_changed)(struct drm_device *dev);
>  
>  	/**
> +	 * @mode_valid:
> +	 *
> +	 * Device specific validation of display modes. Can be used to reject
> +	 * modes that can never be supports. Only device wide constraints can

s/supports/supported/

> +	 * be checked here. crtc/encoder/bridge/connector specific constraints
> +	 * can be checked in the .mode_valid() hook for each specific object.

s/can/should/ I think, for my understanding of English at least.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Some igts that throw bs modes at the kernel would be nice, to complement
your work here (I didn't check for those patches, mail backlog and all).
-Daniel


> +	 */
> +	enum drm_mode_status (*mode_valid)(struct drm_device *dev,
> +					   const struct drm_display_mode *mode);
> +
> +	/**
>  	 * @atomic_check:
>  	 *
>  	 * This is the only hook to validate an atomic modeset update. This
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 7ac61858b54e..f945afaf2313 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -444,7 +444,8 @@ struct drm_display_mode *drm_mode_create(struct drm_device *dev);
>  void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode);
>  void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>  			       const struct drm_display_mode *in);
> -int drm_mode_convert_umode(struct drm_display_mode *out,
> +int drm_mode_convert_umode(struct drm_device *dev,
> +			   struct drm_display_mode *out,
>  			   const struct drm_mode_modeinfo *in);
>  void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
>  void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);
> @@ -497,7 +498,8 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  					const struct drm_display_mode *mode2);
>  
>  /* for use by the crtc helper probe functions */
> -enum drm_mode_status drm_mode_validate_basic(const struct drm_display_mode *mode);
> +enum drm_mode_status drm_mode_validate_driver(struct drm_device *dev,
> +					      const struct drm_display_mode *mode);
>  enum drm_mode_status drm_mode_validate_size(const struct drm_display_mode *mode,
>  					    int maxX, int maxY);
>  enum drm_mode_status
> -- 
> 2.13.6
> 
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/10] drm/i915: Provide a device level .mode_valid() hook

[Daniel Vetter]

On Tue, Nov 14, 2017 at 08:32:58PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We never support certain mode flags etc. Reject those early on in the
> mode_config.mode_valid() hook. That allows us to remove some duplicated
> checks from the connector .mode_valid() hooks, and it guarantees that
> we never see those flags even from user mode as the
> mode_config.mode_valid() hooks gets executed for those as well.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c     |  3 ---
>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dsi.c     |  5 -----
>  drivers/gpu/drm/i915/intel_dvo.c     |  3 ---
>  drivers/gpu/drm/i915/intel_fbc.c     |  3 +--
>  drivers/gpu/drm/i915/intel_hdmi.c    |  3 ---
>  drivers/gpu/drm/i915/intel_sdvo.c    |  3 ---
>  7 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 9f31aea51dff..1cd4a7c22bd5 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -304,9 +304,6 @@ intel_crt_mode_valid(struct drm_connector *connector,
>  	int max_dotclk = dev_priv->max_dotclk_freq;
>  	int max_clock;
>  
> -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> -		return MODE_NO_DBLESCAN;
> -
>  	if (mode->clock < 25000)
>  		return MODE_CLOCK_LOW;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ed6a4a8d9273..fec76147d8e5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14151,10 +14151,37 @@ static void intel_atomic_state_free(struct drm_atomic_state *state)
>  	kfree(state);
>  }
>  
> +static enum drm_mode_status
> +intel_mode_valid(struct drm_device *dev,
> +		 const struct drm_display_mode *mode)
> +{
> +	if (mode->vscan > 1)
> +		return MODE_NO_VSCAN;
> +
> +	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> +		return MODE_NO_DBLESCAN;
> +
> +	if (mode->flags & DRM_MODE_FLAG_HSKEW)
> +		return MODE_H_ILLEGAL;
> +
> +	if (mode->flags & (DRM_MODE_FLAG_CSYNC |
> +			   DRM_MODE_FLAG_NCSYNC |
> +			   DRM_MODE_FLAG_PCSYNC))
> +		return MODE_HSYNC;
> +
> +	if (mode->flags & (DRM_MODE_FLAG_BCAST |
> +			   DRM_MODE_FLAG_PIXMUX |
> +			   DRM_MODE_FLAG_CLKDIV2))
> +		return MODE_BAD;
> +
> +	return MODE_OK;
> +}

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  static const struct drm_mode_config_funcs intel_mode_funcs = {
>  	.fb_create = intel_user_framebuffer_create,
>  	.get_format_info = intel_get_format_info,
>  	.output_poll_changed = intel_fbdev_output_poll_changed,
> +	.mode_valid = intel_mode_valid,
>  	.atomic_check = intel_atomic_check,
>  	.atomic_commit = intel_atomic_commit,
>  	.atomic_state_alloc = intel_atomic_state_alloc,
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index f09474b0c4d3..c9b5a956c30a 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -1266,11 +1266,6 @@ intel_dsi_mode_valid(struct drm_connector *connector,
>  
>  	DRM_DEBUG_KMS("\n");
>  
> -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN) {
> -		DRM_DEBUG_KMS("MODE_NO_DBLESCAN\n");
> -		return MODE_NO_DBLESCAN;
> -	}
> -
>  	if (fixed_mode) {
>  		if (mode->hdisplay > fixed_mode->hdisplay)
>  			return MODE_PANEL;
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 754baa00bea9..59c066ca14e5 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -219,9 +219,6 @@ intel_dvo_mode_valid(struct drm_connector *connector,
>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>  	int target_clock = mode->clock;
>  
> -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> -		return MODE_NO_DBLESCAN;
> -
>  	/* XXX: Validate clock range */
>  
>  	if (fixed_mode) {
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 1a0f5e0c8d10..f0e74477c678 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -789,8 +789,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> -	if ((cache->crtc.mode_flags & DRM_MODE_FLAG_INTERLACE) ||
> -	    (cache->crtc.mode_flags & DRM_MODE_FLAG_DBLSCAN)) {
> +	if (cache->crtc.mode_flags & DRM_MODE_FLAG_INTERLACE) {
>  		fbc->no_fbc_reason = "incompatible mode";
>  		return false;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 2d95db64cdf2..7ce3d14153b4 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1311,9 +1311,6 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>  	bool force_dvi =
>  		READ_ONCE(to_intel_digital_connector_state(connector->state)->force_audio) == HDMI_AUDIO_OFF_DVI;
>  
> -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> -		return MODE_NO_DBLESCAN;
> -
>  	clock = mode->clock;
>  
>  	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING)
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 2b8764897d68..0bf97ed5ffac 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1607,9 +1607,6 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
>  	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>  
> -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> -		return MODE_NO_DBLESCAN;
> -
>  	if (intel_sdvo->pixel_clock_min > mode->clock)
>  		return MODE_CLOCK_LOW;
>  
> -- 
> 2.13.6
> 
> _______________________________________________
> 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 10/10] drm/i915: Provide a device level .mode_valid() hook

[Daniel Vetter]

On Mon, Nov 20, 2017 at 09:01:07AM +0100, Daniel Vetter wrote:
> On Tue, Nov 14, 2017 at 08:32:58PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We never support certain mode flags etc. Reject those early on in the
> > mode_config.mode_valid() hook. That allows us to remove some duplicated
> > checks from the connector .mode_valid() hooks, and it guarantees that
> > we never see those flags even from user mode as the
> > mode_config.mode_valid() hooks gets executed for those as well.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_crt.c     |  3 ---
> >  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dsi.c     |  5 -----
> >  drivers/gpu/drm/i915/intel_dvo.c     |  3 ---
> >  drivers/gpu/drm/i915/intel_fbc.c     |  3 +--
> >  drivers/gpu/drm/i915/intel_hdmi.c    |  3 ---
> >  drivers/gpu/drm/i915/intel_sdvo.c    |  3 ---
> >  7 files changed, 28 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > index 9f31aea51dff..1cd4a7c22bd5 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -304,9 +304,6 @@ intel_crt_mode_valid(struct drm_connector *connector,
> >  	int max_dotclk = dev_priv->max_dotclk_freq;
> >  	int max_clock;
> >  
> > -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > -		return MODE_NO_DBLESCAN;
> > -
> >  	if (mode->clock < 25000)
> >  		return MODE_CLOCK_LOW;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ed6a4a8d9273..fec76147d8e5 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14151,10 +14151,37 @@ static void intel_atomic_state_free(struct drm_atomic_state *state)
> >  	kfree(state);
> >  }
> >  
> > +static enum drm_mode_status
> > +intel_mode_valid(struct drm_device *dev,
> > +		 const struct drm_display_mode *mode)
> > +{
> > +	if (mode->vscan > 1)
> > +		return MODE_NO_VSCAN;
> > +
> > +	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > +		return MODE_NO_DBLESCAN;
> > +
> > +	if (mode->flags & DRM_MODE_FLAG_HSKEW)
> > +		return MODE_H_ILLEGAL;
> > +
> > +	if (mode->flags & (DRM_MODE_FLAG_CSYNC |
> > +			   DRM_MODE_FLAG_NCSYNC |
> > +			   DRM_MODE_FLAG_PCSYNC))
> > +		return MODE_HSYNC;
> > +
> > +	if (mode->flags & (DRM_MODE_FLAG_BCAST |
> > +			   DRM_MODE_FLAG_PIXMUX |
> > +			   DRM_MODE_FLAG_CLKDIV2))
> > +		return MODE_BAD;
> > +
> > +	return MODE_OK;
> > +}
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

And I'm kinda leaning towards moving the rejection of the old crud into
the core, but that's a different thing.
-Daniel

> 
> > +
> >  static const struct drm_mode_config_funcs intel_mode_funcs = {
> >  	.fb_create = intel_user_framebuffer_create,
> >  	.get_format_info = intel_get_format_info,
> >  	.output_poll_changed = intel_fbdev_output_poll_changed,
> > +	.mode_valid = intel_mode_valid,
> >  	.atomic_check = intel_atomic_check,
> >  	.atomic_commit = intel_atomic_commit,
> >  	.atomic_state_alloc = intel_atomic_state_alloc,
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index f09474b0c4d3..c9b5a956c30a 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -1266,11 +1266,6 @@ intel_dsi_mode_valid(struct drm_connector *connector,
> >  
> >  	DRM_DEBUG_KMS("\n");
> >  
> > -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN) {
> > -		DRM_DEBUG_KMS("MODE_NO_DBLESCAN\n");
> > -		return MODE_NO_DBLESCAN;
> > -	}
> > -
> >  	if (fixed_mode) {
> >  		if (mode->hdisplay > fixed_mode->hdisplay)
> >  			return MODE_PANEL;
> > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > index 754baa00bea9..59c066ca14e5 100644
> > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > @@ -219,9 +219,6 @@ intel_dvo_mode_valid(struct drm_connector *connector,
> >  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> >  	int target_clock = mode->clock;
> >  
> > -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > -		return MODE_NO_DBLESCAN;
> > -
> >  	/* XXX: Validate clock range */
> >  
> >  	if (fixed_mode) {
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index 1a0f5e0c8d10..f0e74477c678 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -789,8 +789,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> >  		return false;
> >  	}
> >  
> > -	if ((cache->crtc.mode_flags & DRM_MODE_FLAG_INTERLACE) ||
> > -	    (cache->crtc.mode_flags & DRM_MODE_FLAG_DBLSCAN)) {
> > +	if (cache->crtc.mode_flags & DRM_MODE_FLAG_INTERLACE) {
> >  		fbc->no_fbc_reason = "incompatible mode";
> >  		return false;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 2d95db64cdf2..7ce3d14153b4 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1311,9 +1311,6 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
> >  	bool force_dvi =
> >  		READ_ONCE(to_intel_digital_connector_state(connector->state)->force_audio) == HDMI_AUDIO_OFF_DVI;
> >  
> > -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > -		return MODE_NO_DBLESCAN;
> > -
> >  	clock = mode->clock;
> >  
> >  	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING)
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 2b8764897d68..0bf97ed5ffac 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -1607,9 +1607,6 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
> >  	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
> >  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> >  
> > -	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > -		return MODE_NO_DBLESCAN;
> > -
> >  	if (intel_sdvo->pixel_clock_min > mode->clock)
> >  		return MODE_CLOCK_LOW;
> >  
> > -- 
> > 2.13.6
> > 
> > _______________________________________________
> > 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

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

Re: [PATCH 00/10] drm/uapi: Validate mode flags/type, and deprecate some of them

[Ville Syrjälä]

On Tue, Nov 14, 2017 at 08:32:48PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I recently realized that we're not validating the mode flags/type
> passed in from userspace. Let's try to fix that.
> 
> I'd also like to entirely eliminate some of the more crazy mode flags.
> PIXMUX and BCAST look pretty easy, so I've targetted them first.
> 
> Ideally I'd like to kill DBLCLK, CLKDIV2, HSKEW, DBLSCAN, and vscan
> as well. IMO there's no good reason to expose any of that to userspace,
> and instead it should all be handled internally by the drivers.
> Unfortunately all of those seem to be used to some degree by a handful
> of drivers.
> 
> I also tried to nuke some of the crazy mode types.
> 
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Keith Packard <keithp@keithp.com>
> 
> Ville Syrjälä (10):
>   drm/modes: Move 3D stereo flag check into drm_mode_validate_basic()
>   drm/uapi: Validate the mode flags/type
>   drm/uapi: Deprecate DRM_MODE_FLAG_PIXMUX
>   drm/uapi: Deprecate DRM_MODE_FLAG_BCAST
>   drm/modes: Fix description of DRM_MODE_TYPE_USERDEF
>   drm/modes: Kill off the oddball DRM_MODE_TYPE_CRTC_C vs.
>     DRM_MODE_TYPE_BUILTIN handling
>   drm/modes: Kill DRM_MODE_TYPE_CLOCK_CRTC_C define
>   drm/uapi: Deprecate nonsense kms mode types
>   drm/modes: Provide global mode_valid hook
>   drm/i915: Provide a device level .mode_valid() hook

Entire series pushed to drm-misc-next. Thanks for the reviews.

> 
>  drivers/gpu/drm/drm_atomic.c         |  2 +-
>  drivers/gpu/drm/drm_crtc.c           |  2 +-
>  drivers/gpu/drm/drm_modes.c          | 60 +++++++++++++++++++++++++-----------
>  drivers/gpu/drm/drm_probe_helper.c   |  2 +-
>  drivers/gpu/drm/i915/intel_crt.c     |  3 --
>  drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_dsi.c     |  5 ---
>  drivers/gpu/drm/i915/intel_dvo.c     |  3 --
>  drivers/gpu/drm/i915/intel_fbc.c     |  3 +-
>  drivers/gpu/drm/i915/intel_hdmi.c    |  3 --
>  drivers/gpu/drm/i915/intel_sdvo.c    |  3 --
>  include/drm/drm_mode_config.h        | 12 ++++++++
>  include/drm/drm_modes.h              | 24 +++++++--------
>  include/uapi/drm/drm_mode.h          | 30 ++++++++++++++----
>  14 files changed, 120 insertions(+), 59 deletions(-)
> 
> -- 
> 2.13.6

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

drm-next xf86-video-vmware breakage Was [PATCH 02/10] drm/uapi: Validate the mode flags/type

[Thomas Hellstrom]

Hi, Ville,

On 11/14/2017 07:32 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently userspace is allowed to feed in any king of garbage in the
> high bits of the mode flags/type, as are drivers when probing modes.
> Reject any mode with bogus flags/type.
>
> Hopefully this won't break any current userspace...

Unfortunately this breaks xf86-video-vmware which currently leaves the 
mode->type uninitialized :(.
That code is inherited from the old gallium xorg state tracker. I don't 
know whether it has spread elsewhere but the symptoms are that the X 
server frequently dies failing to set screen resources, a very 
uninformative error.

I'll fix the xf86-video-vmware driver, but I guess we need to back out 
the mode->type check.

/Thomas

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

Re: drm-next xf86-video-vmware breakage Was [PATCH 02/10] drm/uapi: Validate the mode flags/type

[Ville Syrjälä]

On Wed, Mar 21, 2018 at 09:45:09PM +0100, Thomas Hellstrom wrote:
> Hi, Ville,
> 
> On 11/14/2017 07:32 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Currently userspace is allowed to feed in any king of garbage in the
> > high bits of the mode flags/type, as are drivers when probing modes.
> > Reject any mode with bogus flags/type.
> >
> > Hopefully this won't break any current userspace...
> 
> Unfortunately this breaks xf86-video-vmware which currently leaves the 
> mode->type uninitialized :(.
> That code is inherited from the old gallium xorg state tracker. I don't 
> know whether it has spread elsewhere but the symptoms are that the X 
> server frequently dies failing to set screen resources, a very 
> uninformative error.
> 
> I'll fix the xf86-video-vmware driver, but I guess we need to back out 
> the mode->type check.

Dang. So the flags check is fine but type check is not?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm-next xf86-video-vmware breakage Was [PATCH 02/10] drm/uapi: Validate the mode flags/type

[Thomas Hellstrom]

On 03/21/2018 09:51 PM, Ville Syrjälä wrote:
> On Wed, Mar 21, 2018 at 09:45:09PM +0100, Thomas Hellstrom wrote:
>> Hi, Ville,
>>
>> On 11/14/2017 07:32 PM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Currently userspace is allowed to feed in any king of garbage in the
>>> high bits of the mode flags/type, as are drivers when probing modes.
>>> Reject any mode with bogus flags/type.
>>>
>>> Hopefully this won't break any current userspace...
>> Unfortunately this breaks xf86-video-vmware which currently leaves the
>> mode->type uninitialized :(.
>> That code is inherited from the old gallium xorg state tracker. I don't
>> know whether it has spread elsewhere but the symptoms are that the X
>> server frequently dies failing to set screen resources, a very
>> uninformative error.
>>
>> I'll fix the xf86-video-vmware driver, but I guess we need to back out
>> the mode->type check.
> Dang. So the flags check is fine but type check is not?
>
Yes, we copy the flags field untranslated from xorg's 
DisplayModePtr::Flags field which seems to be what 
xf86-video-modesetting does as well, so I guess we should be OK there.

/Thomas


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