[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

✓ Fi.CI.BAT: success for drm/uapi: Validate mode flags/type, and deprecate some of them

[Patchwork]

== Series Details ==

Series: drm/uapi: Validate mode flags/type, and deprecate some of them
URL   : https://patchwork.freedesktop.org/series/33810/
State : success

== Summary ==

Series 33810v1 drm/uapi: Validate mode flags/type, and deprecate some of them
https://patchwork.freedesktop.org/api/1.0/series/33810/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:444s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:451s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:379s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:536s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:274s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:504s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:502s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:496s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:488s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:429s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:264s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:543s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:428s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:438s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:426s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:467s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:480s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:528s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:529s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:569s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:457s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:545s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:564s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:493s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:462s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:557s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:419s
Blacklisted hosts:
fi-cfl-s         total:289  pass:254  dwarn:3   dfail:0   fail:0   skip:32  time:535s
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:559s
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:488s

c46476e24d6432b5792ef63596a985848d122d50 drm-tip: 2017y-11m-14d-17h-03m-32s UTC integration manifest
c49fbaf4c826 drm/i915: Provide a device level .mode_valid() hook
58718fce7a9c drm/modes: Provide global mode_valid hook
86a44b462856 drm/uapi: Deprecate nonsense kms mode types
96e09203df32 drm/modes: Kill DRM_MODE_TYPE_CLOCK_CRTC_C define
5cfe7bbdd757 drm/modes: Kill off the oddball DRM_MODE_TYPE_CRTC_C vs. DRM_MODE_TYPE_BUILTIN handling
b902fa8caaa7 drm/modes: Fix description of DRM_MODE_TYPE_USERDEF
51d53b1b26a8 drm/uapi: Deprecate DRM_MODE_FLAG_BCAST
6632fd6ce1c9 drm/uapi: Deprecate DRM_MODE_FLAG_PIXMUX
007aa8d5ff85 drm/uapi: Validate the mode flags/type
7ba984ac2279 drm/modes: Move 3D stereo flag check into drm_mode_validate_basic()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7123/
_______________________________________________
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

[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

✗ Fi.CI.IGT: failure for drm/uapi: Validate mode flags/type, and deprecate some of them

[Patchwork]

== Series Details ==

Series: drm/uapi: Validate mode flags/type, and deprecate some of them
URL   : https://patchwork.freedesktop.org/series/33810/
State : failure

== Summary ==

Test kms_busy:
        Subgroup extended-modeset-hang-newfb-with-reset-render-c:
                pass       -> DMESG-WARN (shard-hsw) fdo#102249
Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252
Test kms_atomic_transition:
        Subgroup plane-all-modeset-transition-fencing:
                pass       -> FAIL       (shard-hsw) fdo#102614
        Subgroup plane-all-modeset-transition:
                pass       -> FAIL       (shard-hsw)
Test kms_cursor_legacy:
        Subgroup flip-vs-cursor-varying-size:
                fail       -> PASS       (shard-hsw)

fdo#102249 https://bugs.freedesktop.org/show_bug.cgi?id=102249
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614

shard-hsw        total:2584 pass:1469 dwarn:3   dfail:1   fail:12  skip:1099 time:9480s
Blacklisted hosts:
shard-apl        total:2584 pass:1618 dwarn:2   dfail:1   fail:27  skip:936 time:13051s
shard-kbl        total:2565 pass:1611 dwarn:92  dfail:5   fail:27  skip:829 time:10598s
shard-snb        total:2584 pass:1208 dwarn:1   dfail:1   fail:15  skip:1359 time:7723s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7123/shards.html
_______________________________________________
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

✓ Fi.CI.BAT: success for drm/uapi: Validate mode flags/type, and deprecate some of them (rev2)

[Patchwork]

== Series Details ==

Series: drm/uapi: Validate mode flags/type, and deprecate some of them (rev2)
URL   : https://patchwork.freedesktop.org/series/33810/
State : success

== Summary ==

Series 33810v2 drm/uapi: Validate mode flags/type, and deprecate some of them
https://patchwork.freedesktop.org/api/1.0/series/33810/revisions/2/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                pass       -> FAIL       (fi-kbl-7500u) fdo#102514
Test gem_exec_reloc:
        Subgroup basic-cpu-read-active:
                pass       -> FAIL       (fi-gdg-551) fdo#102582 +1
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:441s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:453s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:383s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:534s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:277s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:509s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:503s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:498s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:487s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:431s
fi-gdg-551       total:289  pass:176  dwarn:1   dfail:0   fail:3   skip:109 time:265s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:541s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:430s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:439s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:430s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:459s
fi-kbl-7500u     total:289  pass:263  dwarn:1   dfail:0   fail:1   skip:24  time:469s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:537s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:542s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:580s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:548s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:564s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:494s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:468s
fi-snb-2520m     total:246  pass:212  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:428s
Blacklisted hosts:
fi-cfl-s         total:289  pass:254  dwarn:3   dfail:0   fail:0   skip:32  time:536s

06718a287f282ad31264560c5dc18a38474cb562 drm-tip: 2017y-11m-15d-11h-36m-39s UTC integration manifest
0f0dd0f89b9b drm/i915: Provide a device level .mode_valid() hook
5013fc3e277d drm/modes: Provide global mode_valid hook
0207542b258d drm/uapi: Deprecate nonsense kms mode types
cc3bcc4cafa3 drm/modes: Kill DRM_MODE_TYPE_CLOCK_CRTC_C define
58c5f7a01c38 drm/modes: Kill off the oddball DRM_MODE_TYPE_CRTC_C vs. DRM_MODE_TYPE_BUILTIN handling
7af2fa4b9d45 drm/modes: Fix description of DRM_MODE_TYPE_USERDEF
309956649688 drm/uapi: Deprecate DRM_MODE_FLAG_BCAST
c3e3d06d8f93 drm/uapi: Deprecate DRM_MODE_FLAG_PIXMUX
c282a3aa6661 drm/uapi: Validate the mode flags/type
e401ceb629ba drm/modes: Move 3D stereo flag check into drm_mode_validate_basic()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7145/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for drm/uapi: Validate mode flags/type, and deprecate some of them (rev2)

[Patchwork]

== Series Details ==

Series: drm/uapi: Validate mode flags/type, and deprecate some of them (rev2)
URL   : https://patchwork.freedesktop.org/series/33810/
State : failure

== Summary ==

Test kms_atomic_transition:
        Subgroup plane-all-modeset-transition-fencing:
                dmesg-warn -> FAIL       (shard-hsw) fdo#102614
        Subgroup plane-all-modeset-transition:
                pass       -> FAIL       (shard-hsw)
Test kms_cursor_crc:
        Subgroup cursor-64x64-suspend:
                pass       -> SKIP       (shard-hsw)
Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (shard-hsw) fdo#102707

fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707

shard-hsw        total:2584 pass:1468 dwarn:3   dfail:1   fail:12  skip:1100 time:9409s
Blacklisted hosts:
shard-apl        total:2584 pass:1618 dwarn:3   dfail:2   fail:25  skip:936 time:13371s
shard-kbl        total:2357 pass:1551 dwarn:8   dfail:2   fail:24  skip:769 time:9699s
shard-snb        total:2584 pass:1203 dwarn:2   dfail:1   fail:13  skip:1365 time:7788s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7145/shards.html
_______________________________________________
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