[PATCH 0/3] drm/omapdrm: Remove CONFIG_DRM_OMAP_NUM_CRTCS and cleanup

[PATCH 0/3] drm/omapdrm: Remove CONFIG_DRM_OMAP_NUM_CRTCS and cleanup

[Jyri Sarha]

The first patch removes CONFIG_DRM_OMAP_NUM_CRTCS config option. The
rest is just adding comments and making the code more readable.

Jyri Sarha (3):
  drm/omapdrm: Get rid of DRM_OMAP_NUM_CRTCS config option
  drm/omapdrm: Change possible_crtcs to possible_crtcs_for_planes
  drm/omapdrm: Separate ids for planes and CRTCs in omap_modeset_init()

 drivers/gpu/drm/omapdrm/Kconfig    |  9 ----
 drivers/gpu/drm/omapdrm/omap_drv.c | 93 ++++++++++++++------------------------
 2 files changed, 35 insertions(+), 67 deletions(-)

-- 
1.9.1

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

[PATCH 1/3] drm/omapdrm: Get rid of DRM_OMAP_NUM_CRTCS config option

[Jyri Sarha]

Allocate one CRTC for each connected output and get rid of
DRM_OMAP_NUM_CRTCS config option. We still can not create more CRTCs
than we have DSS display managers. We also reserve one overlay per
CRTC for primary plane so we can not have more CRTCs than we have
overlays either.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/Kconfig    |  9 ------
 drivers/gpu/drm/omapdrm/omap_drv.c | 59 +++++++++++---------------------------
 2 files changed, 16 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig
index 556f81f..b3d08c5 100644
--- a/drivers/gpu/drm/omapdrm/Kconfig
+++ b/drivers/gpu/drm/omapdrm/Kconfig
@@ -10,15 +10,6 @@ config DRM_OMAP
 
 if DRM_OMAP
 
-config DRM_OMAP_NUM_CRTCS
-	int "Number of CRTCs"
-	range 1 10
-	default 1  if ARCH_OMAP2 || ARCH_OMAP3
-	default 2  if ARCH_OMAP4
-	help
-	  Select the number of video overlays which can be used as framebuffers.
-	  The remaining overlays are reserved for video.
-
 source "drivers/gpu/drm/omapdrm/dss/Kconfig"
 source "drivers/gpu/drm/omapdrm/displays/Kconfig"
 
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 79a4aad..17c40aa 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -34,11 +34,6 @@
 #define DRIVER_MINOR		0
 #define DRIVER_PATCHLEVEL	0
 
-static int num_crtc = CONFIG_DRM_OMAP_NUM_CRTCS;
-
-MODULE_PARM_DESC(num_crtc, "Number of overlays to use as CRTCs");
-module_param(num_crtc, int, 0600);
-
 /*
  * mode config funcs
  */
@@ -322,7 +317,7 @@ static int omap_modeset_init(struct drm_device *dev)
 	struct omap_dss_device *dssdev = NULL;
 	int num_ovls = dss_feat_get_num_ovls();
 	int num_mgrs = dss_feat_get_num_mgrs();
-	int num_crtcs;
+	int num_crtcs = 0;
 	int i, id = 0;
 	int ret;
 	u32 possible_crtcs;
@@ -334,12 +329,15 @@ static int omap_modeset_init(struct drm_device *dev)
 		return ret;
 
 	/*
-	 * We usually don't want to create a CRTC for each manager, at least
-	 * not until we have a way to expose private planes to userspace.
-	 * Otherwise there would not be enough video pipes left for drm planes.
-	 * We use the num_crtc argument to limit the number of crtcs we create.
+	 * Let's create one CRTC for each connected DSS device if we
+	 * have display managers and overlays (for primary planes) for
+	 * them.
 	 */
-	num_crtcs = min3(num_crtc, num_mgrs, num_ovls);
+	for_each_dss_dev(dssdev)
+		if (omapdss_device_is_connected(dssdev))
+			num_crtcs++;
+
+	num_crtcs = min3(num_crtcs, num_mgrs, num_ovls);
 	possible_crtcs = (1 << num_crtcs) - 1;
 
 	dssdev = NULL;
@@ -379,11 +377,9 @@ static int omap_modeset_init(struct drm_device *dev)
 		drm_mode_connector_attach_encoder(connector, encoder);
 
 		/*
-		 * if we have reached the limit of the crtcs we are allowed to
-		 * create, let's not try to look for a crtc for this
-		 * panel/encoder and onwards, we will, of course, populate the
-		 * the possible_crtcs field for all the encoders with the final
-		 * set of crtcs we create
+		 * if we have reached the limit of the crtcs we can
+		 * create, let's not try to create a crtc for this
+		 * panel/encoder and onwards.
 		 */
 		if (id == num_crtcs)
 			continue;
@@ -418,33 +414,6 @@ static int omap_modeset_init(struct drm_device *dev)
 	}
 
 	/*
-	 * we have allocated crtcs according to the need of the panels/encoders,
-	 * adding more crtcs here if needed
-	 */
-	for (; id < num_crtcs; id++) {
-
-		/* find a free manager for this crtc */
-		for (i = 0; i < num_mgrs; i++) {
-			if (!channel_used(dev, i))
-				break;
-		}
-
-		if (i == num_mgrs) {
-			/* this shouldn't really happen */
-			dev_err(dev->dev, "no managers left for crtc\n");
-			return -ENOMEM;
-		}
-
-		ret = omap_modeset_create_crtc(dev, id, i,
-			possible_crtcs);
-		if (ret < 0) {
-			dev_err(dev->dev,
-				"could not create CRTC (channel %u)\n", i);
-			return ret;
-		}
-	}
-
-	/*
 	 * Create normal planes for the remaining overlays:
 	 */
 	for (; id < num_ovls; id++) {
@@ -459,6 +428,10 @@ static int omap_modeset_init(struct drm_device *dev)
 		priv->planes[priv->num_planes++] = plane;
 	}
 
+	/*
+	 * populate the the possible_crtcs field for all the encoders
+	 * we created.
+	 */
 	for (i = 0; i < priv->num_encoders; i++) {
 		struct drm_encoder *encoder = priv->encoders[i];
 		struct omap_dss_device *dssdev =
-- 
1.9.1

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

[PATCH 2/3] drm/omapdrm: Change possible_crtcs to possible_crtcs_for_planes

[Jyri Sarha]

Rename possible_crtcs local variable to possible_crtcs_for_planes in
omap_modeset_init() and add a comment about its initialization. This
is to make it more explicit what the variable is used for.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 17c40aa..3d30f29 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -320,7 +320,7 @@ static int omap_modeset_init(struct drm_device *dev)
 	int num_crtcs = 0;
 	int i, id = 0;
 	int ret;
-	u32 possible_crtcs;
+	u32 possible_crtcs_for_planes;
 
 	drm_mode_config_init(dev);
 
@@ -338,7 +338,8 @@ static int omap_modeset_init(struct drm_device *dev)
 			num_crtcs++;
 
 	num_crtcs = min3(num_crtcs, num_mgrs, num_ovls);
-	possible_crtcs = (1 << num_crtcs) - 1;
+	/* All planes can be put to any CRTC */
+	possible_crtcs_for_planes = (1 << num_crtcs) - 1;
 
 	dssdev = NULL;
 
@@ -401,7 +402,7 @@ static int omap_modeset_init(struct drm_device *dev)
 		 */
 		if (!channel_used(dev, channel)) {
 			ret = omap_modeset_create_crtc(dev, id, channel,
-				possible_crtcs);
+				possible_crtcs_for_planes);
 			if (ret < 0) {
 				dev_err(dev->dev,
 					"could not create CRTC (channel %u)\n",
@@ -420,7 +421,7 @@ static int omap_modeset_init(struct drm_device *dev)
 		struct drm_plane *plane;
 
 		plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_OVERLAY,
-			possible_crtcs);
+			possible_crtcs_for_planes);
 		if (IS_ERR(plane))
 			return PTR_ERR(plane);
 
-- 
1.9.1

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

[PATCH 3/3] drm/omapdrm: Separate ids for planes and CRTCs in omap_modeset_init()

[Jyri Sarha]

Add separate local id variables for indexing planes and CRTCs in
omap_modeset_init(). This is to make it more explicit what each local
variable is used for.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 3d30f29..9b67906 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -275,7 +275,8 @@ static int omap_connect_dssdevs(void)
 	return r;
 }
 
-static int omap_modeset_create_crtc(struct drm_device *dev, int id,
+static int omap_modeset_create_crtc(struct drm_device *dev,
+				    int crtc_id, int plane_id,
 				    enum omap_channel channel,
 				    u32 possible_crtcs)
 {
@@ -283,18 +284,18 @@ static int omap_modeset_create_crtc(struct drm_device *dev, int id,
 	struct drm_plane *plane;
 	struct drm_crtc *crtc;
 
-	plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_PRIMARY,
+	plane = omap_plane_init(dev, plane_id, DRM_PLANE_TYPE_PRIMARY,
 		possible_crtcs);
 	if (IS_ERR(plane))
 		return PTR_ERR(plane);
 
-	crtc = omap_crtc_init(dev, plane, channel, id);
+	crtc = omap_crtc_init(dev, plane, channel, crtc_id);
 
 	BUG_ON(priv->num_crtcs >= ARRAY_SIZE(priv->crtcs));
-	priv->crtcs[id] = crtc;
+	priv->crtcs[crtc_id] = crtc;
 	priv->num_crtcs++;
 
-	priv->planes[id] = plane;
+	priv->planes[plane_id] = plane;
 	priv->num_planes++;
 
 	return 0;
@@ -318,7 +319,7 @@ static int omap_modeset_init(struct drm_device *dev)
 	int num_ovls = dss_feat_get_num_ovls();
 	int num_mgrs = dss_feat_get_num_mgrs();
 	int num_crtcs = 0;
-	int i, id = 0;
+	int i, crtc_id = 0, plane_id = 0;
 	int ret;
 	u32 possible_crtcs_for_planes;
 
@@ -382,7 +383,7 @@ static int omap_modeset_init(struct drm_device *dev)
 		 * create, let's not try to create a crtc for this
 		 * panel/encoder and onwards.
 		 */
-		if (id == num_crtcs)
+		if (crtc_id == num_crtcs)
 			continue;
 
 		/*
@@ -401,8 +402,8 @@ static int omap_modeset_init(struct drm_device *dev)
 		 * allocated crtc, we create a new crtc for it
 		 */
 		if (!channel_used(dev, channel)) {
-			ret = omap_modeset_create_crtc(dev, id, channel,
-				possible_crtcs_for_planes);
+			ret = omap_modeset_create_crtc(dev, crtc_id, plane_id,
+				channel, possible_crtcs_for_planes);
 			if (ret < 0) {
 				dev_err(dev->dev,
 					"could not create CRTC (channel %u)\n",
@@ -410,17 +411,18 @@ static int omap_modeset_init(struct drm_device *dev)
 				return ret;
 			}
 
-			id++;
+			crtc_id++;
+			plane_id++;
 		}
 	}
 
 	/*
 	 * Create normal planes for the remaining overlays:
 	 */
-	for (; id < num_ovls; id++) {
+	for (; plane_id < num_ovls; plane_id++) {
 		struct drm_plane *plane;
 
-		plane = omap_plane_init(dev, id, DRM_PLANE_TYPE_OVERLAY,
+		plane = omap_plane_init(dev, plane_id, DRM_PLANE_TYPE_OVERLAY,
 			possible_crtcs_for_planes);
 		if (IS_ERR(plane))
 			return PTR_ERR(plane);
@@ -438,6 +440,7 @@ static int omap_modeset_init(struct drm_device *dev)
 		struct omap_dss_device *dssdev =
 					omap_encoder_get_dssdev(encoder);
 		struct omap_dss_device *output;
+		int id;
 
 		output = omapdss_find_output_from_display(dssdev);
 
-- 
1.9.1

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

Re: [PATCH 3/3] drm/omapdrm: Separate ids for planes and CRTCs in omap_modeset_init()

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1322 bytes --]

Hi,

On 14/03/17 22:35, Jyri Sarha wrote:
> Add separate local id variables for indexing planes and CRTCs in
> omap_modeset_init(). This is to make it more explicit what each local
> variable is used for.

"id" should be used for DRM object ids. I think here plane_id and
crtc_id are really idx.

And the code still mixes up at least plane index and DSS plane (enum
omap_plane from omapdss.h). For crtcs I think we already separate the
DRM crtc idx and the DSS "enum omap_channel".

But I belive omap_drv.c shouldn't even care about those omap_channels,
it should all be in omap_crtc.c.

These patches are in the correct direction, but see if you can take it
further and get the code to be something like this:

For each display, we create a crtc.

Each crtc will figure out the omap_channel to use, based on the display
(out->dispc_channel), in omap_crtc.c.

Each crtc will get a primary plane. omap_plane.c will figure out which
"enum omap_plane" to to use for a plane. I think it can be a direct
mapping from plane idx to enum omap_plane, but that has to be explicit
in the code.

Each crtc will also get an encoder and a connector. We can configure an
encoder and a connector to be usable only with a single crtc.

The rest of the planes will be created as overlay planes.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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: [PATCH 2/3] drm/omapdrm: Change possible_crtcs to possible_crtcs_for_planes

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 846 bytes --]

On 14/03/17 22:35, Jyri Sarha wrote:
> Rename possible_crtcs local variable to possible_crtcs_for_planes in
> omap_modeset_init() and add a comment about its initialization. This
> is to make it more explicit what the variable is used for.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 17c40aa..3d30f29 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -320,7 +320,7 @@ static int omap_modeset_init(struct drm_device *dev)
>  	int num_crtcs = 0;
>  	int i, id = 0;
>  	int ret;
> -	u32 possible_crtcs;
> +	u32 possible_crtcs_for_planes;

Maybe "crtc_mask"?

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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