[RFC][PATCH 0/2] Add mode_valid drm_crtc_helper_funcs for HiKey

[RFC][PATCH 0/2] Add mode_valid drm_crtc_helper_funcs for HiKey

[John Stultz]

Currently, on the hikey board, we have the adv7511 bridge wired
up to the kirin ade drm driver. Unfortunately, the kirin ade
core cannot generate accurate byteclocks for all pixel clock
values.

Thus if a mode clock is selected that we cannot calculate a
matching byteclock, the device will boot with a blank screen.

Unfortunately, currently the only place we can properly check
potential modes for this issue in the connector mode_valid
helper. Again, hikey uses the adv7511 bridge, which is shared
between a number of different devices, so its improper to put
restrictions caused by the kirin drm driver in the adv7511
logic.

So this patch set tries to fix this by adding some
infrastructure and logic to the probe helpers so that drm_crtcs
can filter the probed modes. And then adds a whitelist of valid
modes for the kirin drm driver used on HiKey.

This is a first pass attempt here, implementing a suggestion from
Rob Clark on irc, so I'd really welcome any feedback or ideas for
how to best do this.

Thanks so much!
-john

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: dri-devel@lists.freedesktop.org

John Stultz (2):
  drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs
  drm: kirin: Restrict modes to known good mode clocks

 drivers/gpu/drm/drm_probe_helper.c              | 24 ++++++++++++++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 38 +++++++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h        | 26 +++++++++++++++++
 3 files changed, 88 insertions(+)

-- 
2.7.4

[RFC][PATCH 0/2] Add mode_valid drm_crtc_helper_funcs for HiKey

[John Stultz]

Currently, on the hikey board, we have the adv7511 bridge wired
up to the kirin ade drm driver. Unfortunately, the kirin ade
core cannot generate accurate byteclocks for all pixel clock
values.

Thus if a mode clock is selected that we cannot calculate a
matching byteclock, the device will boot with a blank screen.

Unfortunately, currently the only place we can properly check
potential modes for this issue in the connector mode_valid
helper. Again, hikey uses the adv7511 bridge, which is shared
between a number of different devices, so its improper to put
restrictions caused by the kirin drm driver in the adv7511
logic.

So this patch set tries to fix this by adding some
infrastructure and logic to the probe helpers so that drm_crtcs
can filter the probed modes. And then adds a whitelist of valid
modes for the kirin drm driver used on HiKey.

This is a first pass attempt here, implementing a suggestion from
Rob Clark on irc, so I'd really welcome any feedback or ideas for
how to best do this.

Thanks so much!
-john

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: dri-devel@lists.freedesktop.org

John Stultz (2):
  drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs
  drm: kirin: Restrict modes to known good mode clocks

 drivers/gpu/drm/drm_probe_helper.c              | 24 ++++++++++++++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 38 +++++++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h        | 26 +++++++++++++++++
 3 files changed, 88 insertions(+)

-- 
2.7.4

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

[RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

Currently, on the hikey board, we have the adv7511 bridge wired
up to the kirin ade drm driver. Unfortunately, the kirin ade
core cannot generate accurate byteclocks for all pixel clock
values.

Thus if a mode clock is selected that we cannot calculate a
matching byteclock, the device will boot with a blank screen.

Unfortunately, currently the only place we can properly check
potential modes for this issue in the connector mode_valid
helper. Again, hikey uses the adv7511 bridge, which is shared
between a number of different devices, so its improper to put
restrictions caused by the kirin drm driver in the adv7511
logic.

So this patch tries to correct for that, by adding some
infrastructure so that the drm_crtc_helper_funcs can optionally
implement a mode_valid check, so that the probe helpers can
check to make sure there are not any restrictions at the crtc
level as well.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/drm_probe_helper.c       | 24 ++++++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index cf8f012..a808348 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force)
 		connector_status_connected;
 }
 
+static enum drm_mode_status
+drm_connector_check_crtc_modes(struct drm_connector *connector,
+			       struct drm_display_mode *mode)
+{
+	struct drm_device *dev = connector->dev;
+	const struct drm_crtc_helper_funcs *crtc_funcs;
+	struct drm_crtc *c;
+
+	if (mode->status != MODE_OK)
+		return mode->status;
+
+	/* Check all the crtcs on a connector to make sure the mode is valid */
+	drm_for_each_crtc(c, dev) {
+		crtc_funcs = c->helper_private;
+		if (crtc_funcs && crtc_funcs->mode_valid)
+			mode->status = crtc_funcs->mode_valid(c, mode);
+		if (mode->status != MODE_OK)
+			break;
+	}
+	return mode->status;
+}
+
 /**
  * drm_helper_probe_single_connector_modes - get complete set of display modes
  * @connector: connector to probe
@@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		if (mode->status == MODE_OK && connector_funcs->mode_valid)
 			mode->status = connector_funcs->mode_valid(connector,
 								   mode);
+
+		mode->status = drm_connector_check_crtc_modes(connector, mode);
 	}
 
 prune:
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 69c3974..53ca0e4 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * Callback to validate a mode for a crtc, irrespective of the
+	 * specific display configuration.
+	 *
+	 * This callback is used by the probe helpers to filter the mode list
+	 * (which is usually derived from the EDID data block from the sink).
+	 * See e.g. drm_helper_probe_single_connector_modes().
+	 *
+	 * NOTE:
+	 *
+	 * This only filters the mode list supplied to userspace in the
+	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
+	 * ask the kernel to use them. It this case the atomic helpers or legacy
+	 * CRTC helpers will not call this function. Drivers therefore must
+	 * still fully validate any mode passed in in a modeset request.
+	 *
+	 * RETURNS:
+	 *
+	 * Either MODE_OK or one of the failure reasons in enum
+	 * &drm_mode_status.
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
+					   struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate a mode. The parameter mode is the
-- 
2.7.4

[RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

Currently, on the hikey board, we have the adv7511 bridge wired
up to the kirin ade drm driver. Unfortunately, the kirin ade
core cannot generate accurate byteclocks for all pixel clock
values.

Thus if a mode clock is selected that we cannot calculate a
matching byteclock, the device will boot with a blank screen.

Unfortunately, currently the only place we can properly check
potential modes for this issue in the connector mode_valid
helper. Again, hikey uses the adv7511 bridge, which is shared
between a number of different devices, so its improper to put
restrictions caused by the kirin drm driver in the adv7511
logic.

So this patch tries to correct for that, by adding some
infrastructure so that the drm_crtc_helper_funcs can optionally
implement a mode_valid check, so that the probe helpers can
check to make sure there are not any restrictions at the crtc
level as well.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/drm_probe_helper.c       | 24 ++++++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index cf8f012..a808348 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force)
 		connector_status_connected;
 }
 
+static enum drm_mode_status
+drm_connector_check_crtc_modes(struct drm_connector *connector,
+			       struct drm_display_mode *mode)
+{
+	struct drm_device *dev = connector->dev;
+	const struct drm_crtc_helper_funcs *crtc_funcs;
+	struct drm_crtc *c;
+
+	if (mode->status != MODE_OK)
+		return mode->status;
+
+	/* Check all the crtcs on a connector to make sure the mode is valid */
+	drm_for_each_crtc(c, dev) {
+		crtc_funcs = c->helper_private;
+		if (crtc_funcs && crtc_funcs->mode_valid)
+			mode->status = crtc_funcs->mode_valid(c, mode);
+		if (mode->status != MODE_OK)
+			break;
+	}
+	return mode->status;
+}
+
 /**
  * drm_helper_probe_single_connector_modes - get complete set of display modes
  * @connector: connector to probe
@@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		if (mode->status == MODE_OK && connector_funcs->mode_valid)
 			mode->status = connector_funcs->mode_valid(connector,
 								   mode);
+
+		mode->status = drm_connector_check_crtc_modes(connector, mode);
 	}
 
 prune:
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 69c3974..53ca0e4 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * Callback to validate a mode for a crtc, irrespective of the
+	 * specific display configuration.
+	 *
+	 * This callback is used by the probe helpers to filter the mode list
+	 * (which is usually derived from the EDID data block from the sink).
+	 * See e.g. drm_helper_probe_single_connector_modes().
+	 *
+	 * NOTE:
+	 *
+	 * This only filters the mode list supplied to userspace in the
+	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
+	 * ask the kernel to use them. It this case the atomic helpers or legacy
+	 * CRTC helpers will not call this function. Drivers therefore must
+	 * still fully validate any mode passed in in a modeset request.
+	 *
+	 * RETURNS:
+	 *
+	 * Either MODE_OK or one of the failure reasons in enum
+	 * &drm_mode_status.
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
+					   struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate a mode. The parameter mode is the
-- 
2.7.4

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

[RFC][PATCH 2/2] drm: kirin: Restrict modes to known good mode clocks

[John Stultz]

Currently the hikey dsi logic cannot generate accurate byte
clocks values for all pixel clock values. Thus if a mode clock
is selected that cannot match the calculated byte clock, the
device will boot with a blank screen.

This patch uses the new mode_valid callback to enforces known
good mode clocks for well known resolutions, which should allow
the display to work from given EDID options, and ensures for a
given resolution & refresh, the right mode clock is selected.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 38 +++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index afc2b5d..9161633 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -504,6 +504,43 @@ static void ade_crtc_disable(struct drm_crtc *crtc)
 	acrtc->enable = false;
 }
 
+static enum drm_mode_status ade_crtc_mode_valid(struct drm_crtc *crtc,
+						struct drm_display_mode *mode)
+{
+	/*
+	 * kirin_ade cannot generate all modes, so use the whitelist below
+	 */
+	DRM_DEBUG("Checking mode %ix%i@%i clock: %i...",
+		  mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), mode->clock);
+	if ((mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 148500) ||
+	    (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 80192)  ||
+	    (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 74250)  ||
+	    (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 61855)  ||
+	    (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 147116) ||
+	    (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 146250) ||
+	    (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 144589) ||
+	    (mode->hdisplay == 1600 && mode->vdisplay == 1200 && mode->clock == 160961) ||
+	    (mode->hdisplay == 1600 && mode->vdisplay == 900  && mode->clock == 118963) ||
+	    (mode->hdisplay == 1440 && mode->vdisplay == 900  && mode->clock == 126991) ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 128946) ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 98619)  ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 960  && mode->clock == 102081) ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 800  && mode->clock == 83496)  ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 720  && mode->clock == 74440)  ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 720  && mode->clock == 74250)  ||
+	    (mode->hdisplay == 1024 && mode->vdisplay == 768  && mode->clock == 78800)  ||
+	    (mode->hdisplay == 1024 && mode->vdisplay == 768  && mode->clock == 75000)  ||
+	    (mode->hdisplay == 1024 && mode->vdisplay == 768  && mode->clock == 81833)  ||
+	    (mode->hdisplay == 800  && mode->vdisplay == 600  && mode->clock == 48907)  ||
+	    (mode->hdisplay == 800  && mode->vdisplay == 600  && mode->clock == 40000)) {
+		mode->type |= DRM_MODE_TYPE_PREFERRED;
+		DRM_DEBUG("OK\n");
+		return MODE_OK;
+	}
+	DRM_DEBUG("BAD\n");
+	return MODE_BAD;
+}
+
 static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct ade_crtc *acrtc = to_ade_crtc(crtc);
@@ -557,6 +594,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc,
 static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = {
 	.enable		= ade_crtc_enable,
 	.disable	= ade_crtc_disable,
+	.mode_valid	= ade_crtc_mode_valid,
 	.mode_set_nofb	= ade_crtc_mode_set_nofb,
 	.atomic_begin	= ade_crtc_atomic_begin,
 	.atomic_flush	= ade_crtc_atomic_flush,
-- 
2.7.4

[RFC][PATCH 2/2] drm: kirin: Restrict modes to known good mode clocks

[John Stultz]

Currently the hikey dsi logic cannot generate accurate byte
clocks values for all pixel clock values. Thus if a mode clock
is selected that cannot match the calculated byte clock, the
device will boot with a blank screen.

This patch uses the new mode_valid callback to enforces known
good mode clocks for well known resolutions, which should allow
the display to work from given EDID options, and ensures for a
given resolution & refresh, the right mode clock is selected.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 38 +++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index afc2b5d..9161633 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -504,6 +504,43 @@ static void ade_crtc_disable(struct drm_crtc *crtc)
 	acrtc->enable = false;
 }
 
+static enum drm_mode_status ade_crtc_mode_valid(struct drm_crtc *crtc,
+						struct drm_display_mode *mode)
+{
+	/*
+	 * kirin_ade cannot generate all modes, so use the whitelist below
+	 */
+	DRM_DEBUG("Checking mode %ix%i@%i clock: %i...",
+		  mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), mode->clock);
+	if ((mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 148500) ||
+	    (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 80192)  ||
+	    (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 74250)  ||
+	    (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 61855)  ||
+	    (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 147116) ||
+	    (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 146250) ||
+	    (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 144589) ||
+	    (mode->hdisplay == 1600 && mode->vdisplay == 1200 && mode->clock == 160961) ||
+	    (mode->hdisplay == 1600 && mode->vdisplay == 900  && mode->clock == 118963) ||
+	    (mode->hdisplay == 1440 && mode->vdisplay == 900  && mode->clock == 126991) ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 128946) ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 98619)  ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 960  && mode->clock == 102081) ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 800  && mode->clock == 83496)  ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 720  && mode->clock == 74440)  ||
+	    (mode->hdisplay == 1280 && mode->vdisplay == 720  && mode->clock == 74250)  ||
+	    (mode->hdisplay == 1024 && mode->vdisplay == 768  && mode->clock == 78800)  ||
+	    (mode->hdisplay == 1024 && mode->vdisplay == 768  && mode->clock == 75000)  ||
+	    (mode->hdisplay == 1024 && mode->vdisplay == 768  && mode->clock == 81833)  ||
+	    (mode->hdisplay == 800  && mode->vdisplay == 600  && mode->clock == 48907)  ||
+	    (mode->hdisplay == 800  && mode->vdisplay == 600  && mode->clock == 40000)) {
+		mode->type |= DRM_MODE_TYPE_PREFERRED;
+		DRM_DEBUG("OK\n");
+		return MODE_OK;
+	}
+	DRM_DEBUG("BAD\n");
+	return MODE_BAD;
+}
+
 static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct ade_crtc *acrtc = to_ade_crtc(crtc);
@@ -557,6 +594,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc,
 static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = {
 	.enable		= ade_crtc_enable,
 	.disable	= ade_crtc_disable,
+	.mode_valid	= ade_crtc_mode_valid,
 	.mode_set_nofb	= ade_crtc_mode_set_nofb,
 	.atomic_begin	= ade_crtc_atomic_begin,
 	.atomic_flush	= ade_crtc_atomic_flush,
-- 
2.7.4

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> Currently, on the hikey board, we have the adv7511 bridge wired
> up to the kirin ade drm driver. Unfortunately, the kirin ade
> core cannot generate accurate byteclocks for all pixel clock
> values.
>
> Thus if a mode clock is selected that we cannot calculate a
> matching byteclock, the device will boot with a blank screen.
>
> Unfortunately, currently the only place we can properly check
> potential modes for this issue in the connector mode_valid
> helper. Again, hikey uses the adv7511 bridge, which is shared
> between a number of different devices, so its improper to put
> restrictions caused by the kirin drm driver in the adv7511
> logic.
>
> So this patch tries to correct for that, by adding some
> infrastructure so that the drm_crtc_helper_funcs can optionally
> implement a mode_valid check, so that the probe helpers can
> check to make sure there are not any restrictions at the crtc
> level as well.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Xinliang Liu <xinliang.liu@linaro.org>
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Cc: Chen Feng <puck.chen@hisilicon.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>

So I'm going to be super-annoying here and ask for a complete
solution. This here is defacto what ever driver already does (or has
too), but it doesn't really solve the overall issue of having entirely
separate validation paths for probe and atomic_check paths. I think if
we wan to solve this, we need to solve this properly, with a generic
solution. That would mean:
- still in helpers, to make it all opt-int
- covers crtc and encoders and bridges
- allows you to implement the current mode_valid in terms of the new
stuff (maybe as a default hook)
- allows you to implement the current assortment of mode_fixup and/or
atomic_check in terms of the new stuff, or at least to not have to
duplicate logic in there

Docs for all this, especially updating all the warnings on how to use
the existing hooks correctly.

I think just pushing this specific case into the helpers, without
rethinking the overall big picture, isn't gaining us all that much.
For just this I'd say just put it into drivers, until we have some
good clue about how the helpers should look like (maybe this time is
the time? ...).

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_probe_helper.c       | 24 ++++++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index cf8f012..a808348 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force)
>                 connector_status_connected;
>  }
>
> +static enum drm_mode_status
> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> +                              struct drm_display_mode *mode)
> +{
> +       struct drm_device *dev = connector->dev;
> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> +       struct drm_crtc *c;
> +
> +       if (mode->status != MODE_OK)
> +               return mode->status;
> +
> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> +       drm_for_each_crtc(c, dev) {
> +               crtc_funcs = c->helper_private;
> +               if (crtc_funcs && crtc_funcs->mode_valid)
> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> +               if (mode->status != MODE_OK)
> +                       break;
> +       }
> +       return mode->status;
> +}
> +
>  /**
>   * drm_helper_probe_single_connector_modes - get complete set of display modes
>   * @connector: connector to probe
> @@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>                 if (mode->status == MODE_OK && connector_funcs->mode_valid)
>                         mode->status = connector_funcs->mode_valid(connector,
>                                                                    mode);
> +
> +               mode->status = drm_connector_check_crtc_modes(connector, mode);
>         }
>
>  prune:
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 69c3974..53ca0e4 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs {
>         void (*commit)(struct drm_crtc *crtc);
>
>         /**
> +        * @mode_valid:
> +        *
> +        * Callback to validate a mode for a crtc, irrespective of the
> +        * specific display configuration.
> +        *
> +        * This callback is used by the probe helpers to filter the mode list
> +        * (which is usually derived from the EDID data block from the sink).
> +        * See e.g. drm_helper_probe_single_connector_modes().
> +        *
> +        * NOTE:
> +        *
> +        * This only filters the mode list supplied to userspace in the
> +        * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
> +        * ask the kernel to use them. It this case the atomic helpers or legacy
> +        * CRTC helpers will not call this function. Drivers therefore must
> +        * still fully validate any mode passed in in a modeset request.
> +        *
> +        * RETURNS:
> +        *
> +        * Either MODE_OK or one of the failure reasons in enum
> +        * &drm_mode_status.
> +        */
> +       enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> +                                          struct drm_display_mode *mode);
> +
> +       /**
>          * @mode_fixup:
>          *
>          * This callback is used to validate a mode. The parameter mode is the
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> Currently, on the hikey board, we have the adv7511 bridge wired
> up to the kirin ade drm driver. Unfortunately, the kirin ade
> core cannot generate accurate byteclocks for all pixel clock
> values.
>
> Thus if a mode clock is selected that we cannot calculate a
> matching byteclock, the device will boot with a blank screen.
>
> Unfortunately, currently the only place we can properly check
> potential modes for this issue in the connector mode_valid
> helper. Again, hikey uses the adv7511 bridge, which is shared
> between a number of different devices, so its improper to put
> restrictions caused by the kirin drm driver in the adv7511
> logic.
>
> So this patch tries to correct for that, by adding some
> infrastructure so that the drm_crtc_helper_funcs can optionally
> implement a mode_valid check, so that the probe helpers can
> check to make sure there are not any restrictions at the crtc
> level as well.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Xinliang Liu <xinliang.liu@linaro.org>
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Cc: Chen Feng <puck.chen@hisilicon.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>

So I'm going to be super-annoying here and ask for a complete
solution. This here is defacto what ever driver already does (or has
too), but it doesn't really solve the overall issue of having entirely
separate validation paths for probe and atomic_check paths. I think if
we wan to solve this, we need to solve this properly, with a generic
solution. That would mean:
- still in helpers, to make it all opt-int
- covers crtc and encoders and bridges
- allows you to implement the current mode_valid in terms of the new
stuff (maybe as a default hook)
- allows you to implement the current assortment of mode_fixup and/or
atomic_check in terms of the new stuff, or at least to not have to
duplicate logic in there

Docs for all this, especially updating all the warnings on how to use
the existing hooks correctly.

I think just pushing this specific case into the helpers, without
rethinking the overall big picture, isn't gaining us all that much.
For just this I'd say just put it into drivers, until we have some
good clue about how the helpers should look like (maybe this time is
the time? ...).

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_probe_helper.c       | 24 ++++++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index cf8f012..a808348 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force)
>                 connector_status_connected;
>  }
>
> +static enum drm_mode_status
> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> +                              struct drm_display_mode *mode)
> +{
> +       struct drm_device *dev = connector->dev;
> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> +       struct drm_crtc *c;
> +
> +       if (mode->status != MODE_OK)
> +               return mode->status;
> +
> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> +       drm_for_each_crtc(c, dev) {
> +               crtc_funcs = c->helper_private;
> +               if (crtc_funcs && crtc_funcs->mode_valid)
> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> +               if (mode->status != MODE_OK)
> +                       break;
> +       }
> +       return mode->status;
> +}
> +
>  /**
>   * drm_helper_probe_single_connector_modes - get complete set of display modes
>   * @connector: connector to probe
> @@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>                 if (mode->status == MODE_OK && connector_funcs->mode_valid)
>                         mode->status = connector_funcs->mode_valid(connector,
>                                                                    mode);
> +
> +               mode->status = drm_connector_check_crtc_modes(connector, mode);
>         }
>
>  prune:
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 69c3974..53ca0e4 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs {
>         void (*commit)(struct drm_crtc *crtc);
>
>         /**
> +        * @mode_valid:
> +        *
> +        * Callback to validate a mode for a crtc, irrespective of the
> +        * specific display configuration.
> +        *
> +        * This callback is used by the probe helpers to filter the mode list
> +        * (which is usually derived from the EDID data block from the sink).
> +        * See e.g. drm_helper_probe_single_connector_modes().
> +        *
> +        * NOTE:
> +        *
> +        * This only filters the mode list supplied to userspace in the
> +        * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
> +        * ask the kernel to use them. It this case the atomic helpers or legacy
> +        * CRTC helpers will not call this function. Drivers therefore must
> +        * still fully validate any mode passed in in a modeset request.
> +        *
> +        * RETURNS:
> +        *
> +        * Either MODE_OK or one of the failure reasons in enum
> +        * &drm_mode_status.
> +        */
> +       enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> +                                          struct drm_display_mode *mode);
> +
> +       /**
>          * @mode_fixup:
>          *
>          * This callback is used to validate a mode. The parameter mode is the
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

On Tue, Feb 14, 2017 at 11:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>> Currently, on the hikey board, we have the adv7511 bridge wired
>> up to the kirin ade drm driver. Unfortunately, the kirin ade
>> core cannot generate accurate byteclocks for all pixel clock
>> values.
>>
>> Thus if a mode clock is selected that we cannot calculate a
>> matching byteclock, the device will boot with a blank screen.
>>
>> Unfortunately, currently the only place we can properly check
>> potential modes for this issue in the connector mode_valid
>> helper. Again, hikey uses the adv7511 bridge, which is shared
>> between a number of different devices, so its improper to put
>> restrictions caused by the kirin drm driver in the adv7511
>> logic.
>>
>> So this patch tries to correct for that, by adding some
>> infrastructure so that the drm_crtc_helper_funcs can optionally
>> implement a mode_valid check, so that the probe helpers can
>> check to make sure there are not any restrictions at the crtc
>> level as well.
>>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Xinliang Liu <xinliang.liu@linaro.org>
>> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
>> Cc: Rongrong Zou <zourongrong@gmail.com>
>> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> Cc: Chen Feng <puck.chen@hisilicon.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>
> So I'm going to be super-annoying here and ask for a complete
> solution. This here is defacto what ever driver already does (or has
> too), but it doesn't really solve the overall issue of having entirely
> separate validation paths for probe and atomic_check paths. I think if
> we wan to solve this, we need to solve this properly, with a generic
> solution. That would mean:
> - still in helpers, to make it all opt-int

Just to be clear, I believe what I proposed is opt-in, but I assume
you want that in addition to the following, right?

> - covers crtc and encoders and bridges

So you'd like similar mode_valid() calls in the crtc/encoder/bridge
helpers? right?

> - allows you to implement the current mode_valid in terms of the new
> stuff (maybe as a default hook)

This bit I'm not sure I'm following. The current drm_connector's
mode_valid in terms of a new mode_valid call that also looks at
crtc/encoder/bridges? Or do you mean something else?

> - allows you to implement the current assortment of mode_fixup and/or
> atomic_check in terms of the new stuff, or at least to not have to
> duplicate logic in there

This is over my head, so I'll have to research to better understand.

> Docs for all this, especially updating all the warnings on how to use
> the existing hooks correctly.

That's fair.

> I think just pushing this specific case into the helpers, without
> rethinking the overall big picture, isn't gaining us all that much.
> For just this I'd say just put it into drivers, until we have some

Not following here. What do you mean by "put it into drivers"?  Where?

thanks
-john

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

On Tue, Feb 14, 2017 at 11:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
>> Currently, on the hikey board, we have the adv7511 bridge wired
>> up to the kirin ade drm driver. Unfortunately, the kirin ade
>> core cannot generate accurate byteclocks for all pixel clock
>> values.
>>
>> Thus if a mode clock is selected that we cannot calculate a
>> matching byteclock, the device will boot with a blank screen.
>>
>> Unfortunately, currently the only place we can properly check
>> potential modes for this issue in the connector mode_valid
>> helper. Again, hikey uses the adv7511 bridge, which is shared
>> between a number of different devices, so its improper to put
>> restrictions caused by the kirin drm driver in the adv7511
>> logic.
>>
>> So this patch tries to correct for that, by adding some
>> infrastructure so that the drm_crtc_helper_funcs can optionally
>> implement a mode_valid check, so that the probe helpers can
>> check to make sure there are not any restrictions at the crtc
>> level as well.
>>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Xinliang Liu <xinliang.liu@linaro.org>
>> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
>> Cc: Rongrong Zou <zourongrong@gmail.com>
>> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> Cc: Chen Feng <puck.chen@hisilicon.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>
> So I'm going to be super-annoying here and ask for a complete
> solution. This here is defacto what ever driver already does (or has
> too), but it doesn't really solve the overall issue of having entirely
> separate validation paths for probe and atomic_check paths. I think if
> we wan to solve this, we need to solve this properly, with a generic
> solution. That would mean:
> - still in helpers, to make it all opt-int

Just to be clear, I believe what I proposed is opt-in, but I assume
you want that in addition to the following, right?

> - covers crtc and encoders and bridges

So you'd like similar mode_valid() calls in the crtc/encoder/bridge
helpers? right?

> - allows you to implement the current mode_valid in terms of the new
> stuff (maybe as a default hook)

This bit I'm not sure I'm following. The current drm_connector's
mode_valid in terms of a new mode_valid call that also looks at
crtc/encoder/bridges? Or do you mean something else?

> - allows you to implement the current assortment of mode_fixup and/or
> atomic_check in terms of the new stuff, or at least to not have to
> duplicate logic in there

This is over my head, so I'll have to research to better understand.

> Docs for all this, especially updating all the warnings on how to use
> the existing hooks correctly.

That's fair.

> I think just pushing this specific case into the helpers, without
> rethinking the overall big picture, isn't gaining us all that much.
> For just this I'd say just put it into drivers, until we have some

Not following here. What do you mean by "put it into drivers"?  Where?

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Ville Syrjälä]

On Tue, Feb 14, 2017 at 08:38:40PM +0100, Daniel Vetter wrote:
> On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> > Currently, on the hikey board, we have the adv7511 bridge wired
> > up to the kirin ade drm driver. Unfortunately, the kirin ade
> > core cannot generate accurate byteclocks for all pixel clock
> > values.
> >
> > Thus if a mode clock is selected that we cannot calculate a
> > matching byteclock, the device will boot with a blank screen.
> >
> > Unfortunately, currently the only place we can properly check
> > potential modes for this issue in the connector mode_valid
> > helper. Again, hikey uses the adv7511 bridge, which is shared
> > between a number of different devices, so its improper to put
> > restrictions caused by the kirin drm driver in the adv7511
> > logic.
> >
> > So this patch tries to correct for that, by adding some
> > infrastructure so that the drm_crtc_helper_funcs can optionally
> > implement a mode_valid check, so that the probe helpers can
> > check to make sure there are not any restrictions at the crtc
> > level as well.
> >
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Xinliang Liu <xinliang.liu@linaro.org>
> > Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> > Cc: Rongrong Zou <zourongrong@gmail.com>
> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> > Cc: Chen Feng <puck.chen@hisilicon.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> 
> So I'm going to be super-annoying here and ask for a complete
> solution. This here is defacto what ever driver already does (or has
> too), but it doesn't really solve the overall issue of having entirely
> separate validation paths for probe and atomic_check paths. I think if
> we wan to solve this, we need to solve this properly, with a generic
> solution. That would mean:
> - still in helpers, to make it all opt-int
> - covers crtc and encoders and bridges
> - allows you to implement the current mode_valid in terms of the new
> stuff (maybe as a default hook)
> - allows you to implement the current assortment of mode_fixup and/or
> atomic_check in terms of the new stuff, or at least to not have to
> duplicate logic in there

Long ago I quickly looked at doing this for i915. IIRC the main
complication was the normal vs. the crtc_ timings stored in the
mode. I suppose one option would be to populate the crtc_ timings
in .mode_valid() as well and otherwise just ignore the normal timings.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Ville Syrjälä]

On Tue, Feb 14, 2017 at 08:38:40PM +0100, Daniel Vetter wrote:
> On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> > Currently, on the hikey board, we have the adv7511 bridge wired
> > up to the kirin ade drm driver. Unfortunately, the kirin ade
> > core cannot generate accurate byteclocks for all pixel clock
> > values.
> >
> > Thus if a mode clock is selected that we cannot calculate a
> > matching byteclock, the device will boot with a blank screen.
> >
> > Unfortunately, currently the only place we can properly check
> > potential modes for this issue in the connector mode_valid
> > helper. Again, hikey uses the adv7511 bridge, which is shared
> > between a number of different devices, so its improper to put
> > restrictions caused by the kirin drm driver in the adv7511
> > logic.
> >
> > So this patch tries to correct for that, by adding some
> > infrastructure so that the drm_crtc_helper_funcs can optionally
> > implement a mode_valid check, so that the probe helpers can
> > check to make sure there are not any restrictions at the crtc
> > level as well.
> >
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Xinliang Liu <xinliang.liu@linaro.org>
> > Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> > Cc: Rongrong Zou <zourongrong@gmail.com>
> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> > Cc: Chen Feng <puck.chen@hisilicon.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> 
> So I'm going to be super-annoying here and ask for a complete
> solution. This here is defacto what ever driver already does (or has
> too), but it doesn't really solve the overall issue of having entirely
> separate validation paths for probe and atomic_check paths. I think if
> we wan to solve this, we need to solve this properly, with a generic
> solution. That would mean:
> - still in helpers, to make it all opt-int
> - covers crtc and encoders and bridges
> - allows you to implement the current mode_valid in terms of the new
> stuff (maybe as a default hook)
> - allows you to implement the current assortment of mode_fixup and/or
> atomic_check in terms of the new stuff, or at least to not have to
> duplicate logic in there

Long ago I quickly looked at doing this for i915. IIRC the main
complication was the normal vs. the crtc_ timings stored in the
mode. I suppose one option would be to populate the crtc_ timings
in .mode_valid() as well and otherwise just ignore the normal timings.

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote:
> On Tue, Feb 14, 2017 at 11:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> >> Currently, on the hikey board, we have the adv7511 bridge wired
> >> up to the kirin ade drm driver. Unfortunately, the kirin ade
> >> core cannot generate accurate byteclocks for all pixel clock
> >> values.
> >>
> >> Thus if a mode clock is selected that we cannot calculate a
> >> matching byteclock, the device will boot with a blank screen.
> >>
> >> Unfortunately, currently the only place we can properly check
> >> potential modes for this issue in the connector mode_valid
> >> helper. Again, hikey uses the adv7511 bridge, which is shared
> >> between a number of different devices, so its improper to put
> >> restrictions caused by the kirin drm driver in the adv7511
> >> logic.
> >>
> >> So this patch tries to correct for that, by adding some
> >> infrastructure so that the drm_crtc_helper_funcs can optionally
> >> implement a mode_valid check, so that the probe helpers can
> >> check to make sure there are not any restrictions at the crtc
> >> level as well.
> >>
> >> Cc: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> Cc: Sean Paul <seanpaul@chromium.org>
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: Rob Clark <robdclark@gmail.com>
> >> Cc: Xinliang Liu <xinliang.liu@linaro.org>
> >> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> >> Cc: Rongrong Zou <zourongrong@gmail.com>
> >> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> >> Cc: Chen Feng <puck.chen@hisilicon.com>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
> >
> > So I'm going to be super-annoying here and ask for a complete
> > solution. This here is defacto what ever driver already does (or has
> > too), but it doesn't really solve the overall issue of having entirely
> > separate validation paths for probe and atomic_check paths. I think if
> > we wan to solve this, we need to solve this properly, with a generic
> > solution. That would mean:
> > - still in helpers, to make it all opt-int
> 
> Just to be clear, I believe what I proposed is opt-in, but I assume
> you want that in addition to the following, right?
> 
> > - covers crtc and encoders and bridges
> 
> So you'd like similar mode_valid() calls in the crtc/encoder/bridge
> helpers? right?
> 
> > - allows you to implement the current mode_valid in terms of the new
> > stuff (maybe as a default hook)
> 
> This bit I'm not sure I'm following. The current drm_connector's
> mode_valid in terms of a new mode_valid call that also looks at
> crtc/encoder/bridges? Or do you mean something else?
> 
> > - allows you to implement the current assortment of mode_fixup and/or
> > atomic_check in terms of the new stuff, or at least to not have to
> > duplicate logic in there
> 
> This is over my head, so I'll have to research to better understand.
> 
> > Docs for all this, especially updating all the warnings on how to use
> > the existing hooks correctly.
> 
> That's fair.
> 
> > I think just pushing this specific case into the helpers, without
> > rethinking the overall big picture, isn't gaining us all that much.
> > For just this I'd say just put it into drivers, until we have some
> 
> Not following here. What do you mean by "put it into drivers"?  Where?

In your driver callback for ->mode_valid, call into a shared function to
validate CRTC limits. Which you also call from the crtc's ->mode_fixup
function.

In short my complain here is that this is only a partial solution of the
larger problem, specific for your driver. That kind of code is better put
into drivers, until we have a clear understanding to type up something
complete in the helpers. Shared code is imo overrated :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote:
> On Tue, Feb 14, 2017 at 11:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> >> Currently, on the hikey board, we have the adv7511 bridge wired
> >> up to the kirin ade drm driver. Unfortunately, the kirin ade
> >> core cannot generate accurate byteclocks for all pixel clock
> >> values.
> >>
> >> Thus if a mode clock is selected that we cannot calculate a
> >> matching byteclock, the device will boot with a blank screen.
> >>
> >> Unfortunately, currently the only place we can properly check
> >> potential modes for this issue in the connector mode_valid
> >> helper. Again, hikey uses the adv7511 bridge, which is shared
> >> between a number of different devices, so its improper to put
> >> restrictions caused by the kirin drm driver in the adv7511
> >> logic.
> >>
> >> So this patch tries to correct for that, by adding some
> >> infrastructure so that the drm_crtc_helper_funcs can optionally
> >> implement a mode_valid check, so that the probe helpers can
> >> check to make sure there are not any restrictions at the crtc
> >> level as well.
> >>
> >> Cc: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> Cc: Sean Paul <seanpaul@chromium.org>
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: Rob Clark <robdclark@gmail.com>
> >> Cc: Xinliang Liu <xinliang.liu@linaro.org>
> >> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> >> Cc: Rongrong Zou <zourongrong@gmail.com>
> >> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> >> Cc: Chen Feng <puck.chen@hisilicon.com>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
> >
> > So I'm going to be super-annoying here and ask for a complete
> > solution. This here is defacto what ever driver already does (or has
> > too), but it doesn't really solve the overall issue of having entirely
> > separate validation paths for probe and atomic_check paths. I think if
> > we wan to solve this, we need to solve this properly, with a generic
> > solution. That would mean:
> > - still in helpers, to make it all opt-int
> 
> Just to be clear, I believe what I proposed is opt-in, but I assume
> you want that in addition to the following, right?
> 
> > - covers crtc and encoders and bridges
> 
> So you'd like similar mode_valid() calls in the crtc/encoder/bridge
> helpers? right?
> 
> > - allows you to implement the current mode_valid in terms of the new
> > stuff (maybe as a default hook)
> 
> This bit I'm not sure I'm following. The current drm_connector's
> mode_valid in terms of a new mode_valid call that also looks at
> crtc/encoder/bridges? Or do you mean something else?
> 
> > - allows you to implement the current assortment of mode_fixup and/or
> > atomic_check in terms of the new stuff, or at least to not have to
> > duplicate logic in there
> 
> This is over my head, so I'll have to research to better understand.
> 
> > Docs for all this, especially updating all the warnings on how to use
> > the existing hooks correctly.
> 
> That's fair.
> 
> > I think just pushing this specific case into the helpers, without
> > rethinking the overall big picture, isn't gaining us all that much.
> > For just this I'd say just put it into drivers, until we have some
> 
> Not following here. What do you mean by "put it into drivers"?  Where?

In your driver callback for ->mode_valid, call into a shared function to
validate CRTC limits. Which you also call from the crtc's ->mode_fixup
function.

In short my complain here is that this is only a partial solution of the
larger problem, specific for your driver. That kind of code is better put
into drivers, until we have a clear understanding to type up something
complete in the helpers. Shared code is imo overrated :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Stone]

Hi John,

On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote:
> +static enum drm_mode_status
> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> +                              struct drm_display_mode *mode)
> +{
> +       struct drm_device *dev = connector->dev;
> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> +       struct drm_crtc *c;
> +
> +       if (mode->status != MODE_OK)
> +               return mode->status;
> +
> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> +       drm_for_each_crtc(c, dev) {
> +               crtc_funcs = c->helper_private;
> +               if (crtc_funcs && crtc_funcs->mode_valid)
> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> +               if (mode->status != MODE_OK)
> +                       break;
> +       }
> +       return mode->status;
> +}

Hm, that's unfortunate: it limits the mode list for every connector,
to those which are supported by every single CRTC. So if you have one
CRTC serving low-res LVDS, and another serving higher-res HDMI,
suddenly you can't get bigger modes on HDMI. The idea seems sound
enough, but a little more nuance might be good ...

Cheers,
Daniel

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Stone]

Hi John,

On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote:
> +static enum drm_mode_status
> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> +                              struct drm_display_mode *mode)
> +{
> +       struct drm_device *dev = connector->dev;
> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> +       struct drm_crtc *c;
> +
> +       if (mode->status != MODE_OK)
> +               return mode->status;
> +
> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> +       drm_for_each_crtc(c, dev) {
> +               crtc_funcs = c->helper_private;
> +               if (crtc_funcs && crtc_funcs->mode_valid)
> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> +               if (mode->status != MODE_OK)
> +                       break;
> +       }
> +       return mode->status;
> +}

Hm, that's unfortunate: it limits the mode list for every connector,
to those which are supported by every single CRTC. So if you have one
CRTC serving low-res LVDS, and another serving higher-res HDMI,
suddenly you can't get bigger modes on HDMI. The idea seems sound
enough, but a little more nuance might be good ...

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

On Tue, Feb 14, 2017 at 12:22 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote:
>>
>> Not following here. What do you mean by "put it into drivers"?  Where?
>
> In your driver callback for ->mode_valid, call into a shared function to
> validate CRTC limits. Which you also call from the crtc's ->mode_fixup
> function.

So bascially have the adv7511 driver's mode_valid() have a special
callback to the kirin (and freedreno, and whatever other) drm driver
to check the modes? Or I guess the drm driver that uses that bridge
should register something w/ the adv7511 code?

Part of my confusion here is that the main issue is that its not just
one driver I'm dealing with, and they're all really just tied together
via device tree, so I'm not sure how to special case it in just one of
the drivers.

> In short my complain here is that this is only a partial solution of the
> larger problem, specific for your driver. That kind of code is better put
> into drivers, until we have a clear understanding to type up something
> complete in the helpers. Shared code is imo overrated :-)

Yea, apologies for my not seeing the larger problem at first (its
still a bit hazy, really), part of this submission is just trying to
get something to throw darts at and figure out the right thing.

But I'll try to figure out another approach here.

thanks
-john

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

On Tue, Feb 14, 2017 at 12:22 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote:
>>
>> Not following here. What do you mean by "put it into drivers"?  Where?
>
> In your driver callback for ->mode_valid, call into a shared function to
> validate CRTC limits. Which you also call from the crtc's ->mode_fixup
> function.

So bascially have the adv7511 driver's mode_valid() have a special
callback to the kirin (and freedreno, and whatever other) drm driver
to check the modes? Or I guess the drm driver that uses that bridge
should register something w/ the adv7511 code?

Part of my confusion here is that the main issue is that its not just
one driver I'm dealing with, and they're all really just tied together
via device tree, so I'm not sure how to special case it in just one of
the drivers.

> In short my complain here is that this is only a partial solution of the
> larger problem, specific for your driver. That kind of code is better put
> into drivers, until we have a clear understanding to type up something
> complete in the helpers. Shared code is imo overrated :-)

Yea, apologies for my not seeing the larger problem at first (its
still a bit hazy, really), part of this submission is just trying to
get something to throw darts at and figure out the right thing.

But I'll try to figure out another approach here.

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

On Tue, Feb 14, 2017 at 12:32 PM, Daniel Stone <daniel@fooishbar.org> wrote:
> Hi John,
>
> On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote:
>> +static enum drm_mode_status
>> +drm_connector_check_crtc_modes(struct drm_connector *connector,
>> +                              struct drm_display_mode *mode)
>> +{
>> +       struct drm_device *dev = connector->dev;
>> +       const struct drm_crtc_helper_funcs *crtc_funcs;
>> +       struct drm_crtc *c;
>> +
>> +       if (mode->status != MODE_OK)
>> +               return mode->status;
>> +
>> +       /* Check all the crtcs on a connector to make sure the mode is valid */
>> +       drm_for_each_crtc(c, dev) {
>> +               crtc_funcs = c->helper_private;
>> +               if (crtc_funcs && crtc_funcs->mode_valid)
>> +                       mode->status = crtc_funcs->mode_valid(c, mode);
>> +               if (mode->status != MODE_OK)
>> +                       break;
>> +       }
>> +       return mode->status;
>> +}
>
> Hm, that's unfortunate: it limits the mode list for every connector,
> to those which are supported by every single CRTC. So if you have one
> CRTC serving low-res LVDS, and another serving higher-res HDMI,
> suddenly you can't get bigger modes on HDMI. The idea seems sound
> enough, but a little more nuance might be good ...

Yea. That is not my intent at all I'm just trying to get the drm_crtc
attached to the connector that we're getting the EDID mode lines from.
I had tried going connector->encoder->crtc, but at the time this is
called, the encoder is null. So Rob suggested the for_each_crtc(), and
I guess I mistook that for being each crtc on the connector.

Thanks for pointing out this issue. From Daniel's feedback it looks
like I need to start over from scratch though, so little worry this
implementation will go much further.

thanks
-john

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

On Tue, Feb 14, 2017 at 12:32 PM, Daniel Stone <daniel@fooishbar.org> wrote:
> Hi John,
>
> On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote:
>> +static enum drm_mode_status
>> +drm_connector_check_crtc_modes(struct drm_connector *connector,
>> +                              struct drm_display_mode *mode)
>> +{
>> +       struct drm_device *dev = connector->dev;
>> +       const struct drm_crtc_helper_funcs *crtc_funcs;
>> +       struct drm_crtc *c;
>> +
>> +       if (mode->status != MODE_OK)
>> +               return mode->status;
>> +
>> +       /* Check all the crtcs on a connector to make sure the mode is valid */
>> +       drm_for_each_crtc(c, dev) {
>> +               crtc_funcs = c->helper_private;
>> +               if (crtc_funcs && crtc_funcs->mode_valid)
>> +                       mode->status = crtc_funcs->mode_valid(c, mode);
>> +               if (mode->status != MODE_OK)
>> +                       break;
>> +       }
>> +       return mode->status;
>> +}
>
> Hm, that's unfortunate: it limits the mode list for every connector,
> to those which are supported by every single CRTC. So if you have one
> CRTC serving low-res LVDS, and another serving higher-res HDMI,
> suddenly you can't get bigger modes on HDMI. The idea seems sound
> enough, but a little more nuance might be good ...

Yea. That is not my intent at all I'm just trying to get the drm_crtc
attached to the connector that we're getting the EDID mode lines from.
I had tried going connector->encoder->crtc, but at the time this is
called, the encoder is null. So Rob suggested the for_each_crtc(), and
I guess I mistook that for being each crtc on the connector.

Thanks for pointing out this issue. From Daniel's feedback it looks
like I need to start over from scratch though, so little worry this
implementation will go much further.

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Tue, Feb 14, 2017 at 01:03:27PM -0800, John Stultz wrote:
> On Tue, Feb 14, 2017 at 12:22 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote:
> >>
> >> Not following here. What do you mean by "put it into drivers"?  Where?
> >
> > In your driver callback for ->mode_valid, call into a shared function to
> > validate CRTC limits. Which you also call from the crtc's ->mode_fixup
> > function.
> 
> So bascially have the adv7511 driver's mode_valid() have a special
> callback to the kirin (and freedreno, and whatever other) drm driver
> to check the modes? Or I guess the drm driver that uses that bridge
> should register something w/ the adv7511 code?
> 
> Part of my confusion here is that the main issue is that its not just
> one driver I'm dealing with, and they're all really just tied together
> via device tree, so I'm not sure how to special case it in just one of
> the drivers.

This sounds you want to fix this for bridges, but your patch only adds a
crtc callback?

> > In short my complain here is that this is only a partial solution of the
> > larger problem, specific for your driver. That kind of code is better put
> > into drivers, until we have a clear understanding to type up something
> > complete in the helpers. Shared code is imo overrated :-)
> 
> Yea, apologies for my not seeing the larger problem at first (its
> still a bit hazy, really), part of this submission is just trying to
> get something to throw darts at and figure out the right thing.
> 
> But I'll try to figure out another approach here.

Latest kerneldoc in drm-tip should explain this, not sure you didn't spot
it or looked at an old kernel version. For drm docs, _always_ look at
drm-tip, they're moving real fast :-)

https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#modeset-helper-reference-for-common-vtables

See e.g. drm_crtc_helpers_funcs->mode_fixup docs, it's there. There's
explanations sprinkled at various places (mode_valid, plus the different
helper funcs), probably good to first hunt these all down. Then read a
bunch of driver callbacks so you have a better idea of the patterns of
checks, otoh _lots_ of kms drivers get this wrong :(

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Tue, Feb 14, 2017 at 01:03:27PM -0800, John Stultz wrote:
> On Tue, Feb 14, 2017 at 12:22 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote:
> >>
> >> Not following here. What do you mean by "put it into drivers"?  Where?
> >
> > In your driver callback for ->mode_valid, call into a shared function to
> > validate CRTC limits. Which you also call from the crtc's ->mode_fixup
> > function.
> 
> So bascially have the adv7511 driver's mode_valid() have a special
> callback to the kirin (and freedreno, and whatever other) drm driver
> to check the modes? Or I guess the drm driver that uses that bridge
> should register something w/ the adv7511 code?
> 
> Part of my confusion here is that the main issue is that its not just
> one driver I'm dealing with, and they're all really just tied together
> via device tree, so I'm not sure how to special case it in just one of
> the drivers.

This sounds you want to fix this for bridges, but your patch only adds a
crtc callback?

> > In short my complain here is that this is only a partial solution of the
> > larger problem, specific for your driver. That kind of code is better put
> > into drivers, until we have a clear understanding to type up something
> > complete in the helpers. Shared code is imo overrated :-)
> 
> Yea, apologies for my not seeing the larger problem at first (its
> still a bit hazy, really), part of this submission is just trying to
> get something to throw darts at and figure out the right thing.
> 
> But I'll try to figure out another approach here.

Latest kerneldoc in drm-tip should explain this, not sure you didn't spot
it or looked at an old kernel version. For drm docs, _always_ look at
drm-tip, they're moving real fast :-)

https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#modeset-helper-reference-for-common-vtables

See e.g. drm_crtc_helpers_funcs->mode_fixup docs, it's there. There's
explanations sprinkled at various places (mode_valid, plus the different
helper funcs), probably good to first hunt these all down. Then read a
bunch of driver callbacks so you have a better idea of the patterns of
checks, otoh _lots_ of kms drivers get this wrong :(

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Tue, Feb 14, 2017 at 01:07:21PM -0800, John Stultz wrote:
> On Tue, Feb 14, 2017 at 12:32 PM, Daniel Stone <daniel@fooishbar.org> wrote:
> > Hi John,
> >
> > On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote:
> >> +static enum drm_mode_status
> >> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> >> +                              struct drm_display_mode *mode)
> >> +{
> >> +       struct drm_device *dev = connector->dev;
> >> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> >> +       struct drm_crtc *c;
> >> +
> >> +       if (mode->status != MODE_OK)
> >> +               return mode->status;
> >> +
> >> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> >> +       drm_for_each_crtc(c, dev) {
> >> +               crtc_funcs = c->helper_private;
> >> +               if (crtc_funcs && crtc_funcs->mode_valid)
> >> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> >> +               if (mode->status != MODE_OK)
> >> +                       break;
> >> +       }
> >> +       return mode->status;
> >> +}
> >
> > Hm, that's unfortunate: it limits the mode list for every connector,
> > to those which are supported by every single CRTC. So if you have one
> > CRTC serving low-res LVDS, and another serving higher-res HDMI,
> > suddenly you can't get bigger modes on HDMI. The idea seems sound
> > enough, but a little more nuance might be good ...
> 
> Yea. That is not my intent at all I'm just trying to get the drm_crtc
> attached to the connector that we're getting the EDID mode lines from.
> I had tried going connector->encoder->crtc, but at the time this is
> called, the encoder is null. So Rob suggested the for_each_crtc(), and
> I guess I mistook that for being each crtc on the connector.
> 
> Thanks for pointing out this issue. From Daniel's feedback it looks
> like I need to start over from scratch though, so little worry this
> implementation will go much further.

Well your idea was somewhat right, but logic inverted. In ->mode_valid we
need to check whether any encoder/crtc combo could support the mode. Which
means you need to reject it only when there's no encoder/crtc combo that
could support the mode (you reject it if there's only one crtc which can't
handle it).

On the ->mode_fixup/atomic_check callbacks otoh we need to reject the mode
when it's not suitable for the current chain (as described in the atomic
states). That little difference is why this is not an entirely trivial
problem, and yes there's lots of hw out there where this matters.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Tue, Feb 14, 2017 at 01:07:21PM -0800, John Stultz wrote:
> On Tue, Feb 14, 2017 at 12:32 PM, Daniel Stone <daniel@fooishbar.org> wrote:
> > Hi John,
> >
> > On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote:
> >> +static enum drm_mode_status
> >> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> >> +                              struct drm_display_mode *mode)
> >> +{
> >> +       struct drm_device *dev = connector->dev;
> >> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> >> +       struct drm_crtc *c;
> >> +
> >> +       if (mode->status != MODE_OK)
> >> +               return mode->status;
> >> +
> >> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> >> +       drm_for_each_crtc(c, dev) {
> >> +               crtc_funcs = c->helper_private;
> >> +               if (crtc_funcs && crtc_funcs->mode_valid)
> >> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> >> +               if (mode->status != MODE_OK)
> >> +                       break;
> >> +       }
> >> +       return mode->status;
> >> +}
> >
> > Hm, that's unfortunate: it limits the mode list for every connector,
> > to those which are supported by every single CRTC. So if you have one
> > CRTC serving low-res LVDS, and another serving higher-res HDMI,
> > suddenly you can't get bigger modes on HDMI. The idea seems sound
> > enough, but a little more nuance might be good ...
> 
> Yea. That is not my intent at all I'm just trying to get the drm_crtc
> attached to the connector that we're getting the EDID mode lines from.
> I had tried going connector->encoder->crtc, but at the time this is
> called, the encoder is null. So Rob suggested the for_each_crtc(), and
> I guess I mistook that for being each crtc on the connector.
> 
> Thanks for pointing out this issue. From Daniel's feedback it looks
> like I need to start over from scratch though, so little worry this
> implementation will go much further.

Well your idea was somewhat right, but logic inverted. In ->mode_valid we
need to check whether any encoder/crtc combo could support the mode. Which
means you need to reject it only when there's no encoder/crtc combo that
could support the mode (you reject it if there's only one crtc which can't
handle it).

On the ->mode_fixup/atomic_check callbacks otoh we need to reject the mode
when it's not suitable for the current chain (as described in the atomic
states). That little difference is why this is not an entirely trivial
problem, and yes there's lots of hw out there where this matters.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Rob Clark]

On Tue, Feb 14, 2017 at 4:49 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 14, 2017 at 01:07:21PM -0800, John Stultz wrote:
>> On Tue, Feb 14, 2017 at 12:32 PM, Daniel Stone <daniel@fooishbar.org> wrote:
>> > Hi John,
>> >
>> > On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote:
>> >> +static enum drm_mode_status
>> >> +drm_connector_check_crtc_modes(struct drm_connector *connector,
>> >> +                              struct drm_display_mode *mode)
>> >> +{
>> >> +       struct drm_device *dev = connector->dev;
>> >> +       const struct drm_crtc_helper_funcs *crtc_funcs;
>> >> +       struct drm_crtc *c;
>> >> +
>> >> +       if (mode->status != MODE_OK)
>> >> +               return mode->status;
>> >> +
>> >> +       /* Check all the crtcs on a connector to make sure the mode is valid */
>> >> +       drm_for_each_crtc(c, dev) {
>> >> +               crtc_funcs = c->helper_private;
>> >> +               if (crtc_funcs && crtc_funcs->mode_valid)
>> >> +                       mode->status = crtc_funcs->mode_valid(c, mode);
>> >> +               if (mode->status != MODE_OK)
>> >> +                       break;
>> >> +       }
>> >> +       return mode->status;
>> >> +}
>> >
>> > Hm, that's unfortunate: it limits the mode list for every connector,
>> > to those which are supported by every single CRTC. So if you have one
>> > CRTC serving low-res LVDS, and another serving higher-res HDMI,
>> > suddenly you can't get bigger modes on HDMI. The idea seems sound
>> > enough, but a little more nuance might be good ...
>>
>> Yea. That is not my intent at all I'm just trying to get the drm_crtc
>> attached to the connector that we're getting the EDID mode lines from.
>> I had tried going connector->encoder->crtc, but at the time this is
>> called, the encoder is null. So Rob suggested the for_each_crtc(), and
>> I guess I mistook that for being each crtc on the connector.
>>
>> Thanks for pointing out this issue. From Daniel's feedback it looks
>> like I need to start over from scratch though, so little worry this
>> implementation will go much further.
>
> Well your idea was somewhat right, but logic inverted. In ->mode_valid we
> need to check whether any encoder/crtc combo could support the mode. Which
> means you need to reject it only when there's no encoder/crtc combo that
> could support the mode (you reject it if there's only one crtc which can't
> handle it).

sorry, I was probably not expressing my idea to John very well on IRC,
but yeah, the idea was for this to only reject modes that are
impossible for all CRTCs (so a bit different than the case that the
atomic_check callbacks would be validating)

and btw, yeah, this is specifically about fixing things for bridges or
situations where the connector is shared across multiple drivers.  It
isn't really something we can solve in-driver.  Maybe driver provided
callbacks to the bridge would do the trick, but that seemed a bit
weird.  The simple idea was to give the bridge a way to figure out
things that were completely unpossible and let the driver figure out
how to make the things that are possible work somehow.

BR,
-R

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Rob Clark]

On Tue, Feb 14, 2017 at 4:49 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 14, 2017 at 01:07:21PM -0800, John Stultz wrote:
>> On Tue, Feb 14, 2017 at 12:32 PM, Daniel Stone <daniel@fooishbar.org> wrote:
>> > Hi John,
>> >
>> > On 14 February 2017 at 19:25, John Stultz <john.stultz@linaro.org> wrote:
>> >> +static enum drm_mode_status
>> >> +drm_connector_check_crtc_modes(struct drm_connector *connector,
>> >> +                              struct drm_display_mode *mode)
>> >> +{
>> >> +       struct drm_device *dev = connector->dev;
>> >> +       const struct drm_crtc_helper_funcs *crtc_funcs;
>> >> +       struct drm_crtc *c;
>> >> +
>> >> +       if (mode->status != MODE_OK)
>> >> +               return mode->status;
>> >> +
>> >> +       /* Check all the crtcs on a connector to make sure the mode is valid */
>> >> +       drm_for_each_crtc(c, dev) {
>> >> +               crtc_funcs = c->helper_private;
>> >> +               if (crtc_funcs && crtc_funcs->mode_valid)
>> >> +                       mode->status = crtc_funcs->mode_valid(c, mode);
>> >> +               if (mode->status != MODE_OK)
>> >> +                       break;
>> >> +       }
>> >> +       return mode->status;
>> >> +}
>> >
>> > Hm, that's unfortunate: it limits the mode list for every connector,
>> > to those which are supported by every single CRTC. So if you have one
>> > CRTC serving low-res LVDS, and another serving higher-res HDMI,
>> > suddenly you can't get bigger modes on HDMI. The idea seems sound
>> > enough, but a little more nuance might be good ...
>>
>> Yea. That is not my intent at all I'm just trying to get the drm_crtc
>> attached to the connector that we're getting the EDID mode lines from.
>> I had tried going connector->encoder->crtc, but at the time this is
>> called, the encoder is null. So Rob suggested the for_each_crtc(), and
>> I guess I mistook that for being each crtc on the connector.
>>
>> Thanks for pointing out this issue. From Daniel's feedback it looks
>> like I need to start over from scratch though, so little worry this
>> implementation will go much further.
>
> Well your idea was somewhat right, but logic inverted. In ->mode_valid we
> need to check whether any encoder/crtc combo could support the mode. Which
> means you need to reject it only when there's no encoder/crtc combo that
> could support the mode (you reject it if there's only one crtc which can't
> handle it).

sorry, I was probably not expressing my idea to John very well on IRC,
but yeah, the idea was for this to only reject modes that are
impossible for all CRTCs (so a bit different than the case that the
atomic_check callbacks would be validating)

and btw, yeah, this is specifically about fixing things for bridges or
situations where the connector is shared across multiple drivers.  It
isn't really something we can solve in-driver.  Maybe driver provided
callbacks to the bridge would do the trick, but that seemed a bit
weird.  The simple idea was to give the bridge a way to figure out
things that were completely unpossible and let the driver figure out
how to make the things that are possible work somehow.

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]

Hi Daniel,

On Tue, Feb 14, 2017 at 10:42:53PM +0100, Daniel Vetter wrote:
> On Tue, Feb 14, 2017 at 01:03:27PM -0800, John Stultz wrote:
> > On Tue, Feb 14, 2017 at 12:22 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote:
> > >>
> > >> Not following here. What do you mean by "put it into drivers"?  Where?
> > >
> > > In your driver callback for ->mode_valid, call into a shared function to
> > > validate CRTC limits. Which you also call from the crtc's ->mode_fixup
> > > function.
> > 
> > So bascially have the adv7511 driver's mode_valid() have a special
> > callback to the kirin (and freedreno, and whatever other) drm driver
> > to check the modes? Or I guess the drm driver that uses that bridge
> > should register something w/ the adv7511 code?
> > 
> > Part of my confusion here is that the main issue is that its not just
> > one driver I'm dealing with, and they're all really just tied together
> > via device tree, so I'm not sure how to special case it in just one of
> > the drivers.
> 
> This sounds you want to fix this for bridges, but your patch only adds a
> crtc callback?

I've had kind of the same issue on sun4i actually. The issue is that a
bridge (through EDID) might report that some modes are supported, and
the bridge has a chance to say whether or not it can support such
resolutions and filter out the ones it cannot.

However, components higher in the pipeline cannot do so. In my case,
we had an RGB to HDMI that was definitely able to support up to
1080p60. However, our RGB encoder can only generate a pixel clock for
up to ~720p60, and this is not really the bridge's fault, so it
shouldn't be in the bridge driver. The same bridge attached to a
different RGB encoder (even a different SoC from the same family in
our case) will run just fine at 1080p.

We currently end up letting the user choose a resolution we knew very
well had not a chance to work. I came up with a similar solution than
John's, but for the encoders (since our clocks are per-encoder), but
unfortunately had no time to push it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2258 bytes --]

Hi Daniel,

On Tue, Feb 14, 2017 at 10:42:53PM +0100, Daniel Vetter wrote:
> On Tue, Feb 14, 2017 at 01:03:27PM -0800, John Stultz wrote:
> > On Tue, Feb 14, 2017 at 12:22 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote:
> > >>
> > >> Not following here. What do you mean by "put it into drivers"?  Where?
> > >
> > > In your driver callback for ->mode_valid, call into a shared function to
> > > validate CRTC limits. Which you also call from the crtc's ->mode_fixup
> > > function.
> > 
> > So bascially have the adv7511 driver's mode_valid() have a special
> > callback to the kirin (and freedreno, and whatever other) drm driver
> > to check the modes? Or I guess the drm driver that uses that bridge
> > should register something w/ the adv7511 code?
> > 
> > Part of my confusion here is that the main issue is that its not just
> > one driver I'm dealing with, and they're all really just tied together
> > via device tree, so I'm not sure how to special case it in just one of
> > the drivers.
> 
> This sounds you want to fix this for bridges, but your patch only adds a
> crtc callback?

I've had kind of the same issue on sun4i actually. The issue is that a
bridge (through EDID) might report that some modes are supported, and
the bridge has a chance to say whether or not it can support such
resolutions and filter out the ones it cannot.

However, components higher in the pipeline cannot do so. In my case,
we had an RGB to HDMI that was definitely able to support up to
1080p60. However, our RGB encoder can only generate a pixel clock for
up to ~720p60, and this is not really the bridge's fault, so it
shouldn't be in the bridge driver. The same bridge attached to a
different RGB encoder (even a different SoC from the same family in
our case) will run just fine at 1080p.

We currently end up letting the user choose a resolution we knew very
well had not a chance to work. I came up with a similar solution than
John's, but for the encoders (since our clocks are per-encoder), but
unfortunately had no time to push it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

I thought about this some more, I think what we need to fix this mess
properly is:
- mode_valid helper callbacks for crtc, encoder, bridges, with the
same interface as for connectors.
- calling all these new mode_valid hooks from the probe helpers, but
with the restriction that we only reject a mode if all possible
crtc/encoders combos reject it. We need to filter by
possible_encoders/crtcs for these checks. Bridges have a fixed routing
to their encoder, so those are easy.
- add calls to mode_valid in the atomic helpers, right before we call
mode_fixup or atomic_check in drm_atomic_helper_check_modesets.
- convert drivers to move code from mode_fixup into mode_valid
wherever possible, to make sure we can share as much of the check
logic between probe and atomic comit code.
- update docs for all the hooks, plus update the overview sections accordingly.

I think this should give us a good approximation of nirvana. For I
long time I thought we could get away without adding mode_valid
everywhere, but in the probe paths we really can't fake the
adjusted_mode (and other atomic state), so adding dedicated hooks
which are called from both places is probably the only option.
-Daniel

On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> Currently, on the hikey board, we have the adv7511 bridge wired
> up to the kirin ade drm driver. Unfortunately, the kirin ade
> core cannot generate accurate byteclocks for all pixel clock
> values.
>
> Thus if a mode clock is selected that we cannot calculate a
> matching byteclock, the device will boot with a blank screen.
>
> Unfortunately, currently the only place we can properly check
> potential modes for this issue in the connector mode_valid
> helper. Again, hikey uses the adv7511 bridge, which is shared
> between a number of different devices, so its improper to put
> restrictions caused by the kirin drm driver in the adv7511
> logic.
>
> So this patch tries to correct for that, by adding some
> infrastructure so that the drm_crtc_helper_funcs can optionally
> implement a mode_valid check, so that the probe helpers can
> check to make sure there are not any restrictions at the crtc
> level as well.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Xinliang Liu <xinliang.liu@linaro.org>
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Cc: Chen Feng <puck.chen@hisilicon.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/gpu/drm/drm_probe_helper.c       | 24 ++++++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index cf8f012..a808348 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force)
>                 connector_status_connected;
>  }
>
> +static enum drm_mode_status
> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> +                              struct drm_display_mode *mode)
> +{
> +       struct drm_device *dev = connector->dev;
> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> +       struct drm_crtc *c;
> +
> +       if (mode->status != MODE_OK)
> +               return mode->status;
> +
> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> +       drm_for_each_crtc(c, dev) {
> +               crtc_funcs = c->helper_private;
> +               if (crtc_funcs && crtc_funcs->mode_valid)
> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> +               if (mode->status != MODE_OK)
> +                       break;
> +       }
> +       return mode->status;
> +}
> +
>  /**
>   * drm_helper_probe_single_connector_modes - get complete set of display modes
>   * @connector: connector to probe
> @@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>                 if (mode->status == MODE_OK && connector_funcs->mode_valid)
>                         mode->status = connector_funcs->mode_valid(connector,
>                                                                    mode);
> +
> +               mode->status = drm_connector_check_crtc_modes(connector, mode);
>         }
>
>  prune:
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 69c3974..53ca0e4 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs {
>         void (*commit)(struct drm_crtc *crtc);
>
>         /**
> +        * @mode_valid:
> +        *
> +        * Callback to validate a mode for a crtc, irrespective of the
> +        * specific display configuration.
> +        *
> +        * This callback is used by the probe helpers to filter the mode list
> +        * (which is usually derived from the EDID data block from the sink).
> +        * See e.g. drm_helper_probe_single_connector_modes().
> +        *
> +        * NOTE:
> +        *
> +        * This only filters the mode list supplied to userspace in the
> +        * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
> +        * ask the kernel to use them. It this case the atomic helpers or legacy
> +        * CRTC helpers will not call this function. Drivers therefore must
> +        * still fully validate any mode passed in in a modeset request.
> +        *
> +        * RETURNS:
> +        *
> +        * Either MODE_OK or one of the failure reasons in enum
> +        * &drm_mode_status.
> +        */
> +       enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> +                                          struct drm_display_mode *mode);
> +
> +       /**
>          * @mode_fixup:
>          *
>          * This callback is used to validate a mode. The parameter mode is the
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

I thought about this some more, I think what we need to fix this mess
properly is:
- mode_valid helper callbacks for crtc, encoder, bridges, with the
same interface as for connectors.
- calling all these new mode_valid hooks from the probe helpers, but
with the restriction that we only reject a mode if all possible
crtc/encoders combos reject it. We need to filter by
possible_encoders/crtcs for these checks. Bridges have a fixed routing
to their encoder, so those are easy.
- add calls to mode_valid in the atomic helpers, right before we call
mode_fixup or atomic_check in drm_atomic_helper_check_modesets.
- convert drivers to move code from mode_fixup into mode_valid
wherever possible, to make sure we can share as much of the check
logic between probe and atomic comit code.
- update docs for all the hooks, plus update the overview sections accordingly.

I think this should give us a good approximation of nirvana. For I
long time I thought we could get away without adding mode_valid
everywhere, but in the probe paths we really can't fake the
adjusted_mode (and other atomic state), so adding dedicated hooks
which are called from both places is probably the only option.
-Daniel

On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> Currently, on the hikey board, we have the adv7511 bridge wired
> up to the kirin ade drm driver. Unfortunately, the kirin ade
> core cannot generate accurate byteclocks for all pixel clock
> values.
>
> Thus if a mode clock is selected that we cannot calculate a
> matching byteclock, the device will boot with a blank screen.
>
> Unfortunately, currently the only place we can properly check
> potential modes for this issue in the connector mode_valid
> helper. Again, hikey uses the adv7511 bridge, which is shared
> between a number of different devices, so its improper to put
> restrictions caused by the kirin drm driver in the adv7511
> logic.
>
> So this patch tries to correct for that, by adding some
> infrastructure so that the drm_crtc_helper_funcs can optionally
> implement a mode_valid check, so that the probe helpers can
> check to make sure there are not any restrictions at the crtc
> level as well.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Xinliang Liu <xinliang.liu@linaro.org>
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Cc: Chen Feng <puck.chen@hisilicon.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/gpu/drm/drm_probe_helper.c       | 24 ++++++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index cf8f012..a808348 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -170,6 +170,28 @@ drm_connector_detect(struct drm_connector *connector, bool force)
>                 connector_status_connected;
>  }
>
> +static enum drm_mode_status
> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> +                              struct drm_display_mode *mode)
> +{
> +       struct drm_device *dev = connector->dev;
> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> +       struct drm_crtc *c;
> +
> +       if (mode->status != MODE_OK)
> +               return mode->status;
> +
> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> +       drm_for_each_crtc(c, dev) {
> +               crtc_funcs = c->helper_private;
> +               if (crtc_funcs && crtc_funcs->mode_valid)
> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> +               if (mode->status != MODE_OK)
> +                       break;
> +       }
> +       return mode->status;
> +}
> +
>  /**
>   * drm_helper_probe_single_connector_modes - get complete set of display modes
>   * @connector: connector to probe
> @@ -338,6 +360,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>                 if (mode->status == MODE_OK && connector_funcs->mode_valid)
>                         mode->status = connector_funcs->mode_valid(connector,
>                                                                    mode);
> +
> +               mode->status = drm_connector_check_crtc_modes(connector, mode);
>         }
>
>  prune:
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 69c3974..53ca0e4 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -105,6 +105,32 @@ struct drm_crtc_helper_funcs {
>         void (*commit)(struct drm_crtc *crtc);
>
>         /**
> +        * @mode_valid:
> +        *
> +        * Callback to validate a mode for a crtc, irrespective of the
> +        * specific display configuration.
> +        *
> +        * This callback is used by the probe helpers to filter the mode list
> +        * (which is usually derived from the EDID data block from the sink).
> +        * See e.g. drm_helper_probe_single_connector_modes().
> +        *
> +        * NOTE:
> +        *
> +        * This only filters the mode list supplied to userspace in the
> +        * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
> +        * ask the kernel to use them. It this case the atomic helpers or legacy
> +        * CRTC helpers will not call this function. Drivers therefore must
> +        * still fully validate any mode passed in in a modeset request.
> +        *
> +        * RETURNS:
> +        *
> +        * Either MODE_OK or one of the failure reasons in enum
> +        * &drm_mode_status.
> +        */
> +       enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> +                                          struct drm_display_mode *mode);
> +
> +       /**
>          * @mode_fixup:
>          *
>          * This callback is used to validate a mode. The parameter mode is the
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

On Mon, Feb 20, 2017 at 2:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> I thought about this some more, I think what we need to fix this mess
> properly is:
> - mode_valid helper callbacks for crtc, encoder, bridges, with the
> same interface as for connectors.
> - calling all these new mode_valid hooks from the probe helpers, but
> with the restriction that we only reject a mode if all possible
> crtc/encoders combos reject it. We need to filter by
> possible_encoders/crtcs for these checks. Bridges have a fixed routing
> to their encoder, so those are easy.

So... my ignorance here my have complicated things a bit.

Xinliang can correct me here, if I get this wrong. :)

So in the past, when we ran into this issue, we just hacked in a mode
white list into the adv7511 bridge mode_valid check, as it was the
easiest place. But as I understood it, the limitation came from logic
in (what is now an early version of) the kirin ade driver.

However, since kirin made it upstream, the code was refactored and I
believe the specific code was moved to the dsi_calc_phy_rate() which
is part of the dsi encoder, not the crtc code.

So in my limited understanding I initially tried to add the mode_check
logic in the ade crtc driver, but to me (and I could have this wrong
again) it seems it really ought to be in the encoder (where we
probably could do better then using a white list by doing the mode
calculations and seeing if we get a value that matches what was
requested).

I'm not sure if that simplifies your proposal (possibly avoiding the
crtc/encoder combo checks?), or if you think we still need the big
grand solution?

> - add calls to mode_valid in the atomic helpers, right before we call
> mode_fixup or atomic_check in drm_atomic_helper_check_modesets.
> - convert drivers to move code from mode_fixup into mode_valid
> wherever possible, to make sure we can share as much of the check
> logic between probe and atomic comit code.
> - update docs for all the hooks, plus update the overview sections accordingly.
>
> I think this should give us a good approximation of nirvana. For I
> long time I thought we could get away without adding mode_valid
> everywhere, but in the probe paths we really can't fake the
> adjusted_mode (and other atomic state), so adding dedicated hooks
> which are called from both places is probably the only option.

So my mental map of the DRM code is all pretty hazy and limited here,
but the above sounds reasonable. I'll try to trace through and better
understand what specifically you're asking for and get something
started here.

Thanks again for the thoughts and feedback!
-john

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[John Stultz]

On Mon, Feb 20, 2017 at 2:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> I thought about this some more, I think what we need to fix this mess
> properly is:
> - mode_valid helper callbacks for crtc, encoder, bridges, with the
> same interface as for connectors.
> - calling all these new mode_valid hooks from the probe helpers, but
> with the restriction that we only reject a mode if all possible
> crtc/encoders combos reject it. We need to filter by
> possible_encoders/crtcs for these checks. Bridges have a fixed routing
> to their encoder, so those are easy.

So... my ignorance here my have complicated things a bit.

Xinliang can correct me here, if I get this wrong. :)

So in the past, when we ran into this issue, we just hacked in a mode
white list into the adv7511 bridge mode_valid check, as it was the
easiest place. But as I understood it, the limitation came from logic
in (what is now an early version of) the kirin ade driver.

However, since kirin made it upstream, the code was refactored and I
believe the specific code was moved to the dsi_calc_phy_rate() which
is part of the dsi encoder, not the crtc code.

So in my limited understanding I initially tried to add the mode_check
logic in the ade crtc driver, but to me (and I could have this wrong
again) it seems it really ought to be in the encoder (where we
probably could do better then using a white list by doing the mode
calculations and seeing if we get a value that matches what was
requested).

I'm not sure if that simplifies your proposal (possibly avoiding the
crtc/encoder combo checks?), or if you think we still need the big
grand solution?

> - add calls to mode_valid in the atomic helpers, right before we call
> mode_fixup or atomic_check in drm_atomic_helper_check_modesets.
> - convert drivers to move code from mode_fixup into mode_valid
> wherever possible, to make sure we can share as much of the check
> logic between probe and atomic comit code.
> - update docs for all the hooks, plus update the overview sections accordingly.
>
> I think this should give us a good approximation of nirvana. For I
> long time I thought we could get away without adding mode_valid
> everywhere, but in the probe paths we really can't fake the
> adjusted_mode (and other atomic state), so adding dedicated hooks
> which are called from both places is probably the only option.

So my mental map of the DRM code is all pretty hazy and limited here,
but the above sounds reasonable. I'll try to trace through and better
understand what specifically you're asking for and get something
started here.

Thanks again for the thoughts and feedback!
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Mark yao]

[-- Attachment #1: Type: text/html, Size: 3384 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Mon, Feb 27, 2017 at 10:03:20PM -0800, John Stultz wrote:
> On Mon, Feb 20, 2017 at 2:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > I thought about this some more, I think what we need to fix this mess
> > properly is:
> > - mode_valid helper callbacks for crtc, encoder, bridges, with the
> > same interface as for connectors.
> > - calling all these new mode_valid hooks from the probe helpers, but
> > with the restriction that we only reject a mode if all possible
> > crtc/encoders combos reject it. We need to filter by
> > possible_encoders/crtcs for these checks. Bridges have a fixed routing
> > to their encoder, so those are easy.
> 
> So... my ignorance here my have complicated things a bit.
> 
> Xinliang can correct me here, if I get this wrong. :)
> 
> So in the past, when we ran into this issue, we just hacked in a mode
> white list into the adv7511 bridge mode_valid check, as it was the
> easiest place. But as I understood it, the limitation came from logic
> in (what is now an early version of) the kirin ade driver.
> 
> However, since kirin made it upstream, the code was refactored and I
> believe the specific code was moved to the dsi_calc_phy_rate() which
> is part of the dsi encoder, not the crtc code.
> 
> So in my limited understanding I initially tried to add the mode_check
> logic in the ade crtc driver, but to me (and I could have this wrong
> again) it seems it really ought to be in the encoder (where we
> probably could do better then using a white list by doing the mode
> calculations and seeing if we get a value that matches what was
> requested).
> 
> I'm not sure if that simplifies your proposal (possibly avoiding the
> crtc/encoder combo checks?), or if you think we still need the big
> grand solution?

We'll need the grand plan for other drivers I think. Or at least that's
what I'm trying to volunteer you for :-)

If you just want to fix up your specific combo, then you can just hack up
that driver. You don't need any core/helper changes for that.

> > - add calls to mode_valid in the atomic helpers, right before we call
> > mode_fixup or atomic_check in drm_atomic_helper_check_modesets.
> > - convert drivers to move code from mode_fixup into mode_valid
> > wherever possible, to make sure we can share as much of the check
> > logic between probe and atomic comit code.
> > - update docs for all the hooks, plus update the overview sections accordingly.
> >
> > I think this should give us a good approximation of nirvana. For I
> > long time I thought we could get away without adding mode_valid
> > everywhere, but in the probe paths we really can't fake the
> > adjusted_mode (and other atomic state), so adding dedicated hooks
> > which are called from both places is probably the only option.
> 
> So my mental map of the DRM code is all pretty hazy and limited here,
> but the above sounds reasonable. I'll try to trace through and better
> understand what specifically you're asking for and get something
> started here.

Yeah, we're still lacking the nice overview graphs for the drm docs. I
have them here, but the kbuild scripts need polish :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Mon, Feb 27, 2017 at 10:03:20PM -0800, John Stultz wrote:
> On Mon, Feb 20, 2017 at 2:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > I thought about this some more, I think what we need to fix this mess
> > properly is:
> > - mode_valid helper callbacks for crtc, encoder, bridges, with the
> > same interface as for connectors.
> > - calling all these new mode_valid hooks from the probe helpers, but
> > with the restriction that we only reject a mode if all possible
> > crtc/encoders combos reject it. We need to filter by
> > possible_encoders/crtcs for these checks. Bridges have a fixed routing
> > to their encoder, so those are easy.
> 
> So... my ignorance here my have complicated things a bit.
> 
> Xinliang can correct me here, if I get this wrong. :)
> 
> So in the past, when we ran into this issue, we just hacked in a mode
> white list into the adv7511 bridge mode_valid check, as it was the
> easiest place. But as I understood it, the limitation came from logic
> in (what is now an early version of) the kirin ade driver.
> 
> However, since kirin made it upstream, the code was refactored and I
> believe the specific code was moved to the dsi_calc_phy_rate() which
> is part of the dsi encoder, not the crtc code.
> 
> So in my limited understanding I initially tried to add the mode_check
> logic in the ade crtc driver, but to me (and I could have this wrong
> again) it seems it really ought to be in the encoder (where we
> probably could do better then using a white list by doing the mode
> calculations and seeing if we get a value that matches what was
> requested).
> 
> I'm not sure if that simplifies your proposal (possibly avoiding the
> crtc/encoder combo checks?), or if you think we still need the big
> grand solution?

We'll need the grand plan for other drivers I think. Or at least that's
what I'm trying to volunteer you for :-)

If you just want to fix up your specific combo, then you can just hack up
that driver. You don't need any core/helper changes for that.

> > - add calls to mode_valid in the atomic helpers, right before we call
> > mode_fixup or atomic_check in drm_atomic_helper_check_modesets.
> > - convert drivers to move code from mode_fixup into mode_valid
> > wherever possible, to make sure we can share as much of the check
> > logic between probe and atomic comit code.
> > - update docs for all the hooks, plus update the overview sections accordingly.
> >
> > I think this should give us a good approximation of nirvana. For I
> > long time I thought we could get away without adding mode_valid
> > everywhere, but in the probe paths we really can't fake the
> > adjusted_mode (and other atomic state), so adding dedicated hooks
> > which are called from both places is probably the only option.
> 
> So my mental map of the DRM code is all pretty hazy and limited here,
> but the above sounds reasonable. I'll try to trace through and better
> understand what specifically you're asking for and get something
> started here.

Yeah, we're still lacking the nice overview graphs for the drm docs. I
have them here, but the kbuild scripts need polish :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel