[PATCH v2 0/2] Adds slave pipeline support

[PATCH v2 0/2] Adds slave pipeline support

[Lowry Li (Arm Technology China)]

Hi,

This serie aims at adding the support for slave pipeline on Komeda
driver. Also adds drop_master to shutdown the device and make sure
all the komeda resources shared between crtcs have been released.

Change since v1:
Rebases the code and resolves the conflict.

This patch series depends on:
- https://patchwork.freedesktop.org/series/60856/

Regards,
Lowry

Lowry Li (Arm Technology China) (2):
  drm/komeda: Add slave pipeline support
  drm/komeda: Adds komeda_kms_drop_master

 drivers/gpu/drm/arm/display/komeda/komeda_crtc.c   | 41 ++++++++++++++++++++--
 drivers/gpu/drm/arm/display/komeda/komeda_kms.c    | 23 ++++++++++++
 drivers/gpu/drm/arm/display/komeda/komeda_kms.h    |  9 +++++
 .../gpu/drm/arm/display/komeda/komeda_pipeline.c   | 22 ++++++++++++
 .../gpu/drm/arm/display/komeda/komeda_pipeline.h   |  2 ++
 .../drm/arm/display/komeda/komeda_pipeline_state.c | 15 ++++++++
 drivers/gpu/drm/arm/display/komeda/komeda_plane.c  | 32 ++++++++++++++++-
 7 files changed, 141 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] drm/komeda: Add slave pipeline support

[Lowry Li (Arm Technology China)]

From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>

One crtc can use two komeda_pipeline, and one works as master and as
slave. the slave pipeline doesn't have its own output and timing
ctrlr, but pre-composite the input layer data flow and then feed the
result to master. the pipeline configuration like:

slave-layer-0 \
...            slave->CU
slave-layer-4 /         \
                        \
master-layer-0 --------> master->CU -> ...
 ...                  /
master-layer-4 ------>

Since komeda Compiz doesn't output alpha, so the slave->CU result
only can be used as bottom input when blend it with master input data
flows.

Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_crtc.c   | 41 ++++++++++++++++++++--
 drivers/gpu/drm/arm/display/komeda/komeda_kms.c    | 10 ++++++
 drivers/gpu/drm/arm/display/komeda/komeda_kms.h    |  9 +++++
 .../gpu/drm/arm/display/komeda/komeda_pipeline.c   | 22 ++++++++++++
 .../gpu/drm/arm/display/komeda/komeda_pipeline.h   |  2 ++
 .../drm/arm/display/komeda/komeda_pipeline_state.c | 15 ++++++++
 drivers/gpu/drm/arm/display/komeda/komeda_plane.c  | 32 ++++++++++++++++-
 7 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index a2d656f..cafb445 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -64,6 +64,10 @@ static void komeda_crtc_update_clock_ratio(struct komeda_crtc_state *kcrtc_st)
 	}
 
 	/* release unclaimed pipeline resources */
+	err = komeda_release_unclaimed_resources(kcrtc->slave, kcrtc_st);
+	if (err)
+		return err;
+
 	err = komeda_release_unclaimed_resources(kcrtc->master, kcrtc_st);
 	if (err)
 		return err;
@@ -226,6 +230,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 	struct komeda_crtc_state *kcrtc_st = to_kcrtc_st(crtc->state);
 	struct komeda_dev *mdev = kcrtc->base.dev->dev_private;
 	struct komeda_pipeline *master = kcrtc->master;
+	struct komeda_pipeline *slave = kcrtc->slave;
 	struct komeda_wb_connector *wb_conn = kcrtc->wb_conn;
 	struct drm_connector_state *conn_st;
 
@@ -237,6 +242,9 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 	if (has_bit(master->id, kcrtc_st->affected_pipes))
 		komeda_pipeline_update(master, old->state);
 
+	if (slave && has_bit(slave->id, kcrtc_st->affected_pipes))
+		komeda_pipeline_update(slave, old->state);
+
 	conn_st = wb_conn ? wb_conn->base.base.state : NULL;
 	if (conn_st && conn_st->writeback_job)
 		drm_writeback_queue_job(&wb_conn->base, conn_st);
@@ -262,6 +270,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 	struct komeda_crtc_state *old_st = to_kcrtc_st(old);
 	struct komeda_dev *mdev = crtc->dev->dev_private;
 	struct komeda_pipeline *master = kcrtc->master;
+	struct komeda_pipeline *slave  = kcrtc->slave;
 	struct completion *disable_done = &crtc->state->commit->flip_done;
 	struct completion temp;
 	int timeout;
@@ -270,6 +279,9 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 			 drm_crtc_index(crtc),
 			 old_st->active_pipes, old_st->affected_pipes);
 
+	if (slave && has_bit(slave->id, old_st->active_pipes))
+		komeda_pipeline_disable(slave, old->state);
+
 	if (has_bit(master->id, old_st->active_pipes))
 		komeda_pipeline_disable(master, old->state);
 
@@ -414,6 +426,7 @@ static void komeda_crtc_reset(struct drm_crtc *crtc)
 
 	new->affected_pipes = old->active_pipes;
 	new->clock_ratio = old->clock_ratio;
+	new->max_slave_zorder = old->max_slave_zorder;
 
 	return &new->base;
 }
@@ -488,7 +501,7 @@ int komeda_kms_setup_crtcs(struct komeda_kms_dev *kms,
 		master = mdev->pipelines[i];
 
 		crtc->master = master;
-		crtc->slave  = NULL;
+		crtc->slave  = komeda_pipeline_get_slave(master);
 
 		if (crtc->slave)
 			sprintf(str, "pipe-%d", crtc->slave->id);
@@ -522,6 +535,26 @@ static int komeda_crtc_create_clock_ratio_property(struct komeda_crtc *kcrtc)
 	return 0;
 }
 
+static int komeda_crtc_create_slave_planes_property(struct komeda_crtc *kcrtc)
+{
+	struct drm_crtc *crtc = &kcrtc->base;
+	struct drm_property *prop;
+
+	if (kcrtc->slave_planes == 0)
+		return 0;
+
+	prop = drm_property_create_range(crtc->dev, DRM_MODE_PROP_IMMUTABLE,
+					 "slave_planes", 0, U32_MAX);
+	if (!prop)
+		return -ENOMEM;
+
+	drm_object_attach_property(&crtc->base, prop, kcrtc->slave_planes);
+
+	kcrtc->slave_planes_property = prop;
+
+	return 0;
+}
+
 static struct drm_plane *
 get_crtc_primary(struct komeda_kms_dev *kms, struct komeda_crtc *crtc)
 {
@@ -562,7 +595,11 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
 	if (err)
 		return err;
 
-	return 0;
+	err = komeda_crtc_create_slave_planes_property(kcrtc);
+	if (err)
+		return err;
+
+	return err;
 }
 
 int komeda_kms_add_crtcs(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 5d10c55..8543860 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -143,6 +143,8 @@ static int komeda_crtc_normalize_zpos(struct drm_crtc *crtc,
 				      struct drm_crtc_state *crtc_st)
 {
 	struct drm_atomic_state *state = crtc_st->state;
+	struct komeda_crtc *kcrtc = to_kcrtc(crtc);
+	struct komeda_crtc_state *kcrtc_st = to_kcrtc_st(crtc_st);
 	struct komeda_plane_state *kplane_st;
 	struct drm_plane_state *plane_st;
 	struct drm_framebuffer *fb;
@@ -167,6 +169,8 @@ static int komeda_crtc_normalize_zpos(struct drm_crtc *crtc,
 			return err;
 	}
 
+	kcrtc_st->max_slave_zorder = 0;
+
 	list_for_each_entry(kplane_st, &zorder_list, zlist_node) {
 		plane_st = &kplane_st->base;
 		fb = plane_st->fb;
@@ -185,6 +189,12 @@ static int komeda_crtc_normalize_zpos(struct drm_crtc *crtc,
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] zpos:%d, normalized zpos: %d\n",
 				 plane->base.id, plane->name,
 				 plane_st->zpos, plane_st->normalized_zpos);
+
+		/* calculate max slave zorder */
+		if (has_bit(drm_plane_index(plane), kcrtc->slave_planes))
+			kcrtc_st->max_slave_zorder =
+				max(plane_st->normalized_zpos,
+				    kcrtc_st->max_slave_zorder);
 	}
 
 	crtc_st->zpos_changed = true;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 9dcfe5a..219fa3f 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -86,6 +86,9 @@ struct komeda_crtc {
 	 */
 	struct komeda_pipeline *slave;
 
+	/** @slave_planes: komeda slave planes mask */
+	u32 slave_planes;
+
 	/** @wb_conn: komeda write back connector */
 	struct komeda_wb_connector *wb_conn;
 
@@ -94,6 +97,9 @@ struct komeda_crtc {
 
 	/** @clock_ratio_property: property for ratio of (aclk << 32)/pxlclk */
 	struct drm_property *clock_ratio_property;
+
+	/** @slave_planes_property: property for slaves of the planes */
+	struct drm_property *slave_planes_property;
 };
 
 /**
@@ -119,6 +125,9 @@ struct komeda_crtc_state {
 
 	/** @clock_ratio: ratio of (aclk << 32)/pxlclk */
 	u64 clock_ratio;
+
+	/** @max_slave_zorder: the maximum of slave zorder */
+	u32 max_slave_zorder;
 };
 
 /** struct komeda_kms_dev - for gather KMS related things */
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index eb9e0c0..a3d90d8 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -142,6 +142,14 @@ struct komeda_component *
 	return c;
 }
 
+static struct komeda_component *
+komeda_component_pickup_input(struct komeda_component *c, u32 avail_comps)
+{
+	u32 avail_inputs = c->supported_inputs & (avail_comps);
+
+	return komeda_pipeline_get_first_component(c->pipeline, avail_inputs);
+}
+
 /** komeda_component_add - Add a component to &komeda_pipeline */
 struct komeda_component *
 komeda_component_add(struct komeda_pipeline *pipe,
@@ -296,6 +304,20 @@ static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
 	}
 }
 
+/* if pipeline_A accept another pipeline_B's component as input, treat
+ * pipeline_B as slave of pipeline_A.
+ */
+struct komeda_pipeline *
+komeda_pipeline_get_slave(struct komeda_pipeline *master)
+{
+	struct komeda_component *slave;
+
+	slave = komeda_component_pickup_input(&master->compiz->base,
+					      KOMEDA_PIPELINE_COMPIZS);
+
+	return slave ? slave->pipeline : NULL;
+}
+
 int komeda_assemble_pipelines(struct komeda_dev *mdev)
 {
 	struct komeda_pipeline *pipe;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
index f6a4a51..2a67c8a 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
@@ -452,6 +452,8 @@ struct komeda_pipeline *
 		    const struct komeda_pipeline_funcs *funcs);
 void komeda_pipeline_destroy(struct komeda_dev *mdev,
 			     struct komeda_pipeline *pipe);
+struct komeda_pipeline *
+komeda_pipeline_get_slave(struct komeda_pipeline *master);
 int komeda_assemble_pipelines(struct komeda_dev *mdev);
 struct komeda_component *
 komeda_pipeline_get_component(struct komeda_pipeline *pipe, int id);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index 23182ed..6c35afd 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1032,10 +1032,25 @@ int komeda_build_display_data_flow(struct komeda_crtc *kcrtc,
 				   struct komeda_crtc_state *kcrtc_st)
 {
 	struct komeda_pipeline *master = kcrtc->master;
+	struct komeda_pipeline *slave  = kcrtc->slave;
 	struct komeda_data_flow_cfg m_dflow; /* master data flow */
+	struct komeda_data_flow_cfg s_dflow; /* slave data flow */
 	int err;
 
 	memset(&m_dflow, 0, sizeof(m_dflow));
+	memset(&s_dflow, 0, sizeof(s_dflow));
+
+	if (slave && has_bit(slave->id, kcrtc_st->active_pipes)) {
+		err = komeda_compiz_validate(slave->compiz, kcrtc_st, &s_dflow);
+		if (err)
+			return err;
+
+		/* merge the slave dflow into master pipeline */
+		err = komeda_compiz_set_input(master->compiz, kcrtc_st,
+					      &s_dflow);
+		if (err)
+			return err;
+	}
 
 	err = komeda_compiz_validate(master->compiz, kcrtc_st, &m_dflow);
 	if (err)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
index d1c58a8..04b122f 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
@@ -14,15 +14,27 @@
 
 static int
 komeda_plane_init_data_flow(struct drm_plane_state *st,
+			    struct komeda_crtc_state *kcrtc_st,
 			    struct komeda_data_flow_cfg *dflow)
 {
+	struct komeda_plane *kplane = to_kplane(st->plane);
 	struct komeda_plane_state *kplane_st = to_kplane_st(st);
 	struct drm_framebuffer *fb = st->fb;
 	const struct komeda_format_caps *caps = to_kfb(fb)->format_caps;
+	struct komeda_pipeline *pipe = kplane->layer->base.pipeline;
 
 	memset(dflow, 0, sizeof(*dflow));
 
 	dflow->blending_zorder = st->normalized_zpos;
+	if (pipe == to_kcrtc(st->crtc)->master)
+		dflow->blending_zorder -= kcrtc_st->max_slave_zorder;
+	if (dflow->blending_zorder < 0) {
+		DRM_DEBUG_ATOMIC("%s zorder:%d < max_slave_zorder: %d.\n",
+				 st->plane->name, st->normalized_zpos,
+				 kcrtc_st->max_slave_zorder);
+		return -EINVAL;
+	}
+
 	dflow->pixel_blend_mode = st->pixel_blend_mode;
 	dflow->layer_alpha = st->alpha >> 8;
 
@@ -88,7 +100,7 @@
 
 	kcrtc_st = to_kcrtc_st(crtc_st);
 
-	err = komeda_plane_init_data_flow(state, &dflow);
+	err = komeda_plane_init_data_flow(state, kcrtc_st, &dflow);
 	if (err)
 		return err;
 
@@ -288,6 +300,22 @@ static u32 get_possible_crtcs(struct komeda_kms_dev *kms,
 	return possible_crtcs;
 }
 
+static void
+komeda_set_crtc_plane_mask(struct komeda_kms_dev *kms,
+			   struct komeda_pipeline *pipe,
+			   struct drm_plane *plane)
+{
+	struct komeda_crtc *kcrtc;
+	int i;
+
+	for (i = 0; i < kms->n_crtcs; i++) {
+		kcrtc = &kms->crtcs[i];
+
+		if (pipe == kcrtc->slave)
+			kcrtc->slave_planes |= BIT(drm_plane_index(plane));
+	}
+}
+
 /* use Layer0 as primary */
 static u32 get_plane_type(struct komeda_kms_dev *kms,
 			  struct komeda_component *c)
@@ -366,6 +394,8 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
 	if (err)
 		goto cleanup;
 
+	komeda_set_crtc_plane_mask(kms, c->pipeline, plane);
+
 	return 0;
 cleanup:
 	komeda_plane_destroy(plane);
-- 
1.9.1

[PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[Lowry Li (Arm Technology China)]

From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>

The komeda internal resources (pipelines) are shared between crtcs,
and resources release by disable_crtc. This commit is working for once
user forgot disabling crtc like app quit abnomally, and then the
resources can not be used by another crtc. Adds drop_master to
shutdown the device and make sure all the komeda resources have been
released and can be used for the next usage.

Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 8543860..647bce5 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
 	return status;
 }
 
+/* Komeda internal resources (pipelines) are shared between crtcs, and resources
+ * are released by disable_crtc. But if user forget disabling crtc like app quit
+ * abnormally, the resources can not be used by another crtc.
+ * Use drop_master to shutdown the device and make sure all the komeda resources
+ * have been released, and can be used for the next usage.
+ */
+static void komeda_kms_drop_master(struct drm_device *dev,
+				   struct drm_file *file_priv)
+{
+	drm_atomic_helper_shutdown(dev);
+}
+
 static struct drm_driver komeda_kms_driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
 			   DRIVER_PRIME | DRIVER_HAVE_IRQ,
 	.lastclose			= drm_fb_helper_lastclose,
 	.irq_handler			= komeda_kms_irq_handler,
+	.master_drop			= komeda_kms_drop_master,
 	.gem_free_object_unlocked	= drm_gem_cma_free_object,
 	.gem_vm_ops			= &drm_gem_cma_vm_ops,
 	.dumb_create			= komeda_gem_cma_dumb_create,
-- 
1.9.1

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[Daniel Vetter]

On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> 
> The komeda internal resources (pipelines) are shared between crtcs,
> and resources release by disable_crtc. This commit is working for once
> user forgot disabling crtc like app quit abnomally, and then the
> resources can not be used by another crtc. Adds drop_master to
> shutdown the device and make sure all the komeda resources have been
> released and can be used for the next usage.
> 
> Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index 8543860..647bce5 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
>  	return status;
>  }
>  
> +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> + * are released by disable_crtc. But if user forget disabling crtc like app quit
> + * abnormally, the resources can not be used by another crtc.
> + * Use drop_master to shutdown the device and make sure all the komeda resources
> + * have been released, and can be used for the next usage.
> + */

No. If we want this, we need to implement this across drivers, not with
per-vendor hacks.

The kerneldoc should have been a solid hint: "Only used by vmwgfx."
-Daniel

> +static void komeda_kms_drop_master(struct drm_device *dev,
> +				   struct drm_file *file_priv)
> +{
> +	drm_atomic_helper_shutdown(dev);
> +}
> +
>  static struct drm_driver komeda_kms_driver = {
>  	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
>  			   DRIVER_PRIME | DRIVER_HAVE_IRQ,
>  	.lastclose			= drm_fb_helper_lastclose,
>  	.irq_handler			= komeda_kms_irq_handler,
> +	.master_drop			= komeda_kms_drop_master,
>  	.gem_free_object_unlocked	= drm_gem_cma_free_object,
>  	.gem_vm_ops			= &drm_gem_cma_vm_ops,
>  	.dumb_create			= komeda_gem_cma_dumb_create,
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[james qian wang (Arm Technology China)]

On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> >
> > The komeda internal resources (pipelines) are shared between crtcs,
> > and resources release by disable_crtc. This commit is working for once
> > user forgot disabling crtc like app quit abnomally, and then the
> > resources can not be used by another crtc. Adds drop_master to
> > shutdown the device and make sure all the komeda resources have been
> > released and can be used for the next usage.
> >
> > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > ---
> >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > index 8543860..647bce5 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> >  return status;
> >  }
> >
> > +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> > + * are released by disable_crtc. But if user forget disabling crtc like app quit
> > + * abnormally, the resources can not be used by another crtc.
> > + * Use drop_master to shutdown the device and make sure all the komeda resources
> > + * have been released, and can be used for the next usage.
> > + */
>
> No. If we want this, we need to implement this across drivers, not with
> per-vendor hacks.
>
> The kerneldoc should have been a solid hint: "Only used by vmwgfx."
> -Daniel

Hi Daniel:
This drop_master is really what we want, can we update the doc and
add komeda as a user of this hacks like "used by vmwfgx and komeda",
or maybe directly promote this per-vendor hacks as an optional chip
function ?

James

> > +static void komeda_kms_drop_master(struct drm_device *dev,
> > +   struct drm_file *file_priv)
> > +{
> > +drm_atomic_helper_shutdown(dev);
> > +}
> > +
> >  static struct drm_driver komeda_kms_driver = {
> >  .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
> >     DRIVER_PRIME | DRIVER_HAVE_IRQ,
> >  .lastclose= drm_fb_helper_lastclose,
> >  .irq_handler= komeda_kms_irq_handler,
> > +.master_drop= komeda_kms_drop_master,
> >  .gem_free_object_unlocked= drm_gem_cma_free_object,
> >  .gem_vm_ops= &drm_gem_cma_vm_ops,
> >  .dumb_create= komeda_gem_cma_dumb_create,
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [v2,1/2] drm/komeda: Add slave pipeline support

[james qian wang (Arm Technology China)]

On Tue, Jun 11, 2019 at 11:13:39AM +0000, Lowry Li (Arm Technology China) wrote:
> From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> 
> One crtc can use two komeda_pipeline, and one works as master and as
> slave. the slave pipeline doesn't have its own output and timing
> ctrlr, but pre-composite the input layer data flow and then feed the
> result to master. the pipeline configuration like:
> 
> slave-layer-0 \
> ...            slave->CU
> slave-layer-4 /         \
>                         \
> master-layer-0 --------> master->CU -> ...
>  ...                  /
> master-layer-4 ------>
> 
> Since komeda Compiz doesn't output alpha, so the slave->CU result
> only can be used as bottom input when blend it with master input data
> flows.
> 
> Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c   | 41 ++++++++++++++++++++--
>  drivers/gpu/drm/arm/display/komeda/komeda_kms.c    | 10 ++++++
>  drivers/gpu/drm/arm/display/komeda/komeda_kms.h    |  9 +++++
>  .../gpu/drm/arm/display/komeda/komeda_pipeline.c   | 22 ++++++++++++
>  .../gpu/drm/arm/display/komeda/komeda_pipeline.h   |  2 ++
>  .../drm/arm/display/komeda/komeda_pipeline_state.c | 15 ++++++++
>  drivers/gpu/drm/arm/display/komeda/komeda_plane.c  | 32 ++++++++++++++++-
>  7 files changed, 128 insertions(+), 3 deletions(-)
> 
> -- 
> 1.9.1
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index a2d656f..cafb445 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -64,6 +64,10 @@ static void komeda_crtc_update_clock_ratio(struct komeda_crtc_state *kcrtc_st)
>  	}
>  
>  	/* release unclaimed pipeline resources */
> +	err = komeda_release_unclaimed_resources(kcrtc->slave, kcrtc_st);
> +	if (err)
> +		return err;
> +
>  	err = komeda_release_unclaimed_resources(kcrtc->master, kcrtc_st);
>  	if (err)
>  		return err;
> @@ -226,6 +230,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>  	struct komeda_crtc_state *kcrtc_st = to_kcrtc_st(crtc->state);
>  	struct komeda_dev *mdev = kcrtc->base.dev->dev_private;
>  	struct komeda_pipeline *master = kcrtc->master;
> +	struct komeda_pipeline *slave = kcrtc->slave;
>  	struct komeda_wb_connector *wb_conn = kcrtc->wb_conn;
>  	struct drm_connector_state *conn_st;
>  
> @@ -237,6 +242,9 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>  	if (has_bit(master->id, kcrtc_st->affected_pipes))
>  		komeda_pipeline_update(master, old->state);
>  
> +	if (slave && has_bit(slave->id, kcrtc_st->affected_pipes))
> +		komeda_pipeline_update(slave, old->state);
> +
>  	conn_st = wb_conn ? wb_conn->base.base.state : NULL;
>  	if (conn_st && conn_st->writeback_job)
>  		drm_writeback_queue_job(&wb_conn->base, conn_st);
> @@ -262,6 +270,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>  	struct komeda_crtc_state *old_st = to_kcrtc_st(old);
>  	struct komeda_dev *mdev = crtc->dev->dev_private;
>  	struct komeda_pipeline *master = kcrtc->master;
> +	struct komeda_pipeline *slave  = kcrtc->slave;
>  	struct completion *disable_done = &crtc->state->commit->flip_done;
>  	struct completion temp;
>  	int timeout;
> @@ -270,6 +279,9 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>  			 drm_crtc_index(crtc),
>  			 old_st->active_pipes, old_st->affected_pipes);
>  
> +	if (slave && has_bit(slave->id, old_st->active_pipes))
> +		komeda_pipeline_disable(slave, old->state);
> +
>  	if (has_bit(master->id, old_st->active_pipes))
>  		komeda_pipeline_disable(master, old->state);
>  
> @@ -414,6 +426,7 @@ static void komeda_crtc_reset(struct drm_crtc *crtc)
>  
>  	new->affected_pipes = old->active_pipes;
>  	new->clock_ratio = old->clock_ratio;
> +	new->max_slave_zorder = old->max_slave_zorder;
>  
>  	return &new->base;
>  }
> @@ -488,7 +501,7 @@ int komeda_kms_setup_crtcs(struct komeda_kms_dev *kms,
>  		master = mdev->pipelines[i];
>  
>  		crtc->master = master;
> -		crtc->slave  = NULL;
> +		crtc->slave  = komeda_pipeline_get_slave(master);
>  
>  		if (crtc->slave)
>  			sprintf(str, "pipe-%d", crtc->slave->id);
> @@ -522,6 +535,26 @@ static int komeda_crtc_create_clock_ratio_property(struct komeda_crtc *kcrtc)
>  	return 0;
>  }
>  
> +static int komeda_crtc_create_slave_planes_property(struct komeda_crtc *kcrtc)
> +{
> +	struct drm_crtc *crtc = &kcrtc->base;
> +	struct drm_property *prop;
> +
> +	if (kcrtc->slave_planes == 0)
> +		return 0;
> +
> +	prop = drm_property_create_range(crtc->dev, DRM_MODE_PROP_IMMUTABLE,
> +					 "slave_planes", 0, U32_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&crtc->base, prop, kcrtc->slave_planes);
> +
> +	kcrtc->slave_planes_property = prop;
> +
> +	return 0;
> +}
> +
>  static struct drm_plane *
>  get_crtc_primary(struct komeda_kms_dev *kms, struct komeda_crtc *crtc)
>  {
> @@ -562,7 +595,11 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
>  	if (err)
>  		return err;
>  
> -	return 0;
> +	err = komeda_crtc_create_slave_planes_property(kcrtc);
> +	if (err)
> +		return err;
> +
> +	return err;
>  }
>  
>  int komeda_kms_add_crtcs(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index 5d10c55..8543860 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -143,6 +143,8 @@ static int komeda_crtc_normalize_zpos(struct drm_crtc *crtc,
>  				      struct drm_crtc_state *crtc_st)
>  {
>  	struct drm_atomic_state *state = crtc_st->state;
> +	struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> +	struct komeda_crtc_state *kcrtc_st = to_kcrtc_st(crtc_st);
>  	struct komeda_plane_state *kplane_st;
>  	struct drm_plane_state *plane_st;
>  	struct drm_framebuffer *fb;
> @@ -167,6 +169,8 @@ static int komeda_crtc_normalize_zpos(struct drm_crtc *crtc,
>  			return err;
>  	}
>  
> +	kcrtc_st->max_slave_zorder = 0;
> +
>  	list_for_each_entry(kplane_st, &zorder_list, zlist_node) {
>  		plane_st = &kplane_st->base;
>  		fb = plane_st->fb;
> @@ -185,6 +189,12 @@ static int komeda_crtc_normalize_zpos(struct drm_crtc *crtc,
>  		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] zpos:%d, normalized zpos: %d\n",
>  				 plane->base.id, plane->name,
>  				 plane_st->zpos, plane_st->normalized_zpos);
> +
> +		/* calculate max slave zorder */
> +		if (has_bit(drm_plane_index(plane), kcrtc->slave_planes))
> +			kcrtc_st->max_slave_zorder =
> +				max(plane_st->normalized_zpos,
> +				    kcrtc_st->max_slave_zorder);
>  	}
>  
>  	crtc_st->zpos_changed = true;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index 9dcfe5a..219fa3f 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -86,6 +86,9 @@ struct komeda_crtc {
>  	 */
>  	struct komeda_pipeline *slave;
>  
> +	/** @slave_planes: komeda slave planes mask */
> +	u32 slave_planes;
> +
>  	/** @wb_conn: komeda write back connector */
>  	struct komeda_wb_connector *wb_conn;
>  
> @@ -94,6 +97,9 @@ struct komeda_crtc {
>  
>  	/** @clock_ratio_property: property for ratio of (aclk << 32)/pxlclk */
>  	struct drm_property *clock_ratio_property;
> +
> +	/** @slave_planes_property: property for slaves of the planes */
> +	struct drm_property *slave_planes_property;
>  };
>  
>  /**
> @@ -119,6 +125,9 @@ struct komeda_crtc_state {
>  
>  	/** @clock_ratio: ratio of (aclk << 32)/pxlclk */
>  	u64 clock_ratio;
> +
> +	/** @max_slave_zorder: the maximum of slave zorder */
> +	u32 max_slave_zorder;
>  };
>  
>  /** struct komeda_kms_dev - for gather KMS related things */
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> index eb9e0c0..a3d90d8 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> @@ -142,6 +142,14 @@ struct komeda_component *
>  	return c;
>  }
>  
> +static struct komeda_component *
> +komeda_component_pickup_input(struct komeda_component *c, u32 avail_comps)
> +{
> +	u32 avail_inputs = c->supported_inputs & (avail_comps);
> +
> +	return komeda_pipeline_get_first_component(c->pipeline, avail_inputs);
> +}
> +
>  /** komeda_component_add - Add a component to &komeda_pipeline */
>  struct komeda_component *
>  komeda_component_add(struct komeda_pipeline *pipe,
> @@ -296,6 +304,20 @@ static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
>  	}
>  }
>  
> +/* if pipeline_A accept another pipeline_B's component as input, treat
> + * pipeline_B as slave of pipeline_A.
> + */
> +struct komeda_pipeline *
> +komeda_pipeline_get_slave(struct komeda_pipeline *master)
> +{
> +	struct komeda_component *slave;
> +
> +	slave = komeda_component_pickup_input(&master->compiz->base,
> +					      KOMEDA_PIPELINE_COMPIZS);
> +
> +	return slave ? slave->pipeline : NULL;
> +}
> +
>  int komeda_assemble_pipelines(struct komeda_dev *mdev)
>  {
>  	struct komeda_pipeline *pipe;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> index f6a4a51..2a67c8a 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> @@ -452,6 +452,8 @@ struct komeda_pipeline *
>  		    const struct komeda_pipeline_funcs *funcs);
>  void komeda_pipeline_destroy(struct komeda_dev *mdev,
>  			     struct komeda_pipeline *pipe);
> +struct komeda_pipeline *
> +komeda_pipeline_get_slave(struct komeda_pipeline *master);
>  int komeda_assemble_pipelines(struct komeda_dev *mdev);
>  struct komeda_component *
>  komeda_pipeline_get_component(struct komeda_pipeline *pipe, int id);
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index 23182ed..6c35afd 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -1032,10 +1032,25 @@ int komeda_build_display_data_flow(struct komeda_crtc *kcrtc,
>  				   struct komeda_crtc_state *kcrtc_st)
>  {
>  	struct komeda_pipeline *master = kcrtc->master;
> +	struct komeda_pipeline *slave  = kcrtc->slave;
>  	struct komeda_data_flow_cfg m_dflow; /* master data flow */
> +	struct komeda_data_flow_cfg s_dflow; /* slave data flow */
>  	int err;
>  
>  	memset(&m_dflow, 0, sizeof(m_dflow));
> +	memset(&s_dflow, 0, sizeof(s_dflow));
> +
> +	if (slave && has_bit(slave->id, kcrtc_st->active_pipes)) {
> +		err = komeda_compiz_validate(slave->compiz, kcrtc_st, &s_dflow);
> +		if (err)
> +			return err;
> +
> +		/* merge the slave dflow into master pipeline */
> +		err = komeda_compiz_set_input(master->compiz, kcrtc_st,
> +					      &s_dflow);
> +		if (err)
> +			return err;
> +	}
>  
>  	err = komeda_compiz_validate(master->compiz, kcrtc_st, &m_dflow);
>  	if (err)
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> index d1c58a8..04b122f 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> @@ -14,15 +14,27 @@
>  
>  static int
>  komeda_plane_init_data_flow(struct drm_plane_state *st,
> +			    struct komeda_crtc_state *kcrtc_st,
>  			    struct komeda_data_flow_cfg *dflow)
>  {
> +	struct komeda_plane *kplane = to_kplane(st->plane);
>  	struct komeda_plane_state *kplane_st = to_kplane_st(st);
>  	struct drm_framebuffer *fb = st->fb;
>  	const struct komeda_format_caps *caps = to_kfb(fb)->format_caps;
> +	struct komeda_pipeline *pipe = kplane->layer->base.pipeline;
>  
>  	memset(dflow, 0, sizeof(*dflow));
>  
>  	dflow->blending_zorder = st->normalized_zpos;
> +	if (pipe == to_kcrtc(st->crtc)->master)
> +		dflow->blending_zorder -= kcrtc_st->max_slave_zorder;
> +	if (dflow->blending_zorder < 0) {
> +		DRM_DEBUG_ATOMIC("%s zorder:%d < max_slave_zorder: %d.\n",
> +				 st->plane->name, st->normalized_zpos,
> +				 kcrtc_st->max_slave_zorder);
> +		return -EINVAL;
> +	}
> +
>  	dflow->pixel_blend_mode = st->pixel_blend_mode;
>  	dflow->layer_alpha = st->alpha >> 8;
>  
> @@ -88,7 +100,7 @@
>  
>  	kcrtc_st = to_kcrtc_st(crtc_st);
>  
> -	err = komeda_plane_init_data_flow(state, &dflow);
> +	err = komeda_plane_init_data_flow(state, kcrtc_st, &dflow);
>  	if (err)
>  		return err;
>  
> @@ -288,6 +300,22 @@ static u32 get_possible_crtcs(struct komeda_kms_dev *kms,
>  	return possible_crtcs;
>  }
>  
> +static void
> +komeda_set_crtc_plane_mask(struct komeda_kms_dev *kms,
> +			   struct komeda_pipeline *pipe,
> +			   struct drm_plane *plane)
> +{
> +	struct komeda_crtc *kcrtc;
> +	int i;
> +
> +	for (i = 0; i < kms->n_crtcs; i++) {
> +		kcrtc = &kms->crtcs[i];
> +
> +		if (pipe == kcrtc->slave)
> +			kcrtc->slave_planes |= BIT(drm_plane_index(plane));
> +	}
> +}
> +
>  /* use Layer0 as primary */
>  static u32 get_plane_type(struct komeda_kms_dev *kms,
>  			  struct komeda_component *c)
> @@ -366,6 +394,8 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
>  	if (err)
>  		goto cleanup;
>  
> +	komeda_set_crtc_plane_mask(kms, c->pipeline, plane);
> +
>  	return 0;
>  cleanup:
>  	komeda_plane_destroy(plane);


Looks good to me.

Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[Daniel Vetter]

On Wed, Jun 12, 2019 at 02:26:24AM +0000, james qian wang (Arm Technology China) wrote:
> On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> > On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> > > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> > >
> > > The komeda internal resources (pipelines) are shared between crtcs,
> > > and resources release by disable_crtc. This commit is working for once
> > > user forgot disabling crtc like app quit abnomally, and then the
> > > resources can not be used by another crtc. Adds drop_master to
> > > shutdown the device and make sure all the komeda resources have been
> > > released and can be used for the next usage.
> > >
> > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > > ---
> > >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > index 8543860..647bce5 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> > >  return status;
> > >  }
> > >
> > > +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> > > + * are released by disable_crtc. But if user forget disabling crtc like app quit
> > > + * abnormally, the resources can not be used by another crtc.
> > > + * Use drop_master to shutdown the device and make sure all the komeda resources
> > > + * have been released, and can be used for the next usage.
> > > + */
> >
> > No. If we want this, we need to implement this across drivers, not with
> > per-vendor hacks.
> >
> > The kerneldoc should have been a solid hint: "Only used by vmwgfx."
> > -Daniel
> 
> Hi Daniel:
> This drop_master is really what we want, can we update the doc and
> add komeda as a user of this hacks like "used by vmwfgx and komeda",
> or maybe directly promote this per-vendor hacks as an optional chip
> function ?

Still no, because it would mean different behaviour for arm/komeda
compared to everyone else. And we really don't want this, because this
would completely break flicker-less vt-switching.

Currently the only fallback for this case is the lastclose handler, which
atm just restores fbcon/fbdev. If you want to change/extend that to work
without fbdev, then that's the place to do the change. And across _all_
drm kms drivers, so that we have consistent behaviour.

kms is a cross-vendor api, vendor hacks are very, very much not cool.
-Daniel

> 
> James
> 
> > > +static void komeda_kms_drop_master(struct drm_device *dev,
> > > +   struct drm_file *file_priv)
> > > +{
> > > +drm_atomic_helper_shutdown(dev);
> > > +}
> > > +
> > >  static struct drm_driver komeda_kms_driver = {
> > >  .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
> > >     DRIVER_PRIME | DRIVER_HAVE_IRQ,
> > >  .lastclose= drm_fb_helper_lastclose,
> > >  .irq_handler= komeda_kms_irq_handler,
> > > +.master_drop= komeda_kms_drop_master,
> > >  .gem_free_object_unlocked= drm_gem_cma_free_object,
> > >  .gem_vm_ops= &drm_gem_cma_vm_ops,
> > >  .dumb_create= komeda_gem_cma_dumb_create,
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[Liviu Dudau]

On Thu, Jun 13, 2019 at 10:17:27AM +0200, Daniel Vetter wrote:
> On Wed, Jun 12, 2019 at 02:26:24AM +0000, james qian wang (Arm Technology China) wrote:
> > On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> > > On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> > > > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> > > >
> > > > The komeda internal resources (pipelines) are shared between crtcs,
> > > > and resources release by disable_crtc. This commit is working for once
> > > > user forgot disabling crtc like app quit abnomally, and then the
> > > > resources can not be used by another crtc. Adds drop_master to
> > > > shutdown the device and make sure all the komeda resources have been
> > > > released and can be used for the next usage.
> > > >
> > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > > > ---
> > > >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > index 8543860..647bce5 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> > > >  return status;
> > > >  }
> > > >
> > > > +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> > > > + * are released by disable_crtc. But if user forget disabling crtc like app quit
> > > > + * abnormally, the resources can not be used by another crtc.
> > > > + * Use drop_master to shutdown the device and make sure all the komeda resources
> > > > + * have been released, and can be used for the next usage.
> > > > + */
> > >
> > > No. If we want this, we need to implement this across drivers, not with
> > > per-vendor hacks.
> > >
> > > The kerneldoc should have been a solid hint: "Only used by vmwgfx."
> > > -Daniel
> > 
> > Hi Daniel:
> > This drop_master is really what we want, can we update the doc and
> > add komeda as a user of this hacks like "used by vmwfgx and komeda",
> > or maybe directly promote this per-vendor hacks as an optional chip
> > function ?
> 
> Still no, because it would mean different behaviour for arm/komeda
> compared to everyone else. And we really don't want this, because this
> would completely break flicker-less vt-switching.
> 
> Currently the only fallback for this case is the lastclose handler, which
> atm just restores fbcon/fbdev. If you want to change/extend that to work
> without fbdev, then that's the place to do the change. And across _all_
> drm kms drivers, so that we have consistent behaviour.

Slightly unrelated, just thinking of a solution and wanted confirmation/double
checking: can a CRTC be instantiated without any planes (or without a primary
plane)?

Best regards,
Liviu

> 
> kms is a cross-vendor api, vendor hacks are very, very much not cool.
> -Daniel
> 
> > 
> > James
> > 
> > > > +static void komeda_kms_drop_master(struct drm_device *dev,
> > > > +   struct drm_file *file_priv)
> > > > +{
> > > > +drm_atomic_helper_shutdown(dev);
> > > > +}
> > > > +
> > > >  static struct drm_driver komeda_kms_driver = {
> > > >  .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
> > > >     DRIVER_PRIME | DRIVER_HAVE_IRQ,
> > > >  .lastclose= drm_fb_helper_lastclose,
> > > >  .irq_handler= komeda_kms_irq_handler,
> > > > +.master_drop= komeda_kms_drop_master,
> > > >  .gem_free_object_unlocked= drm_gem_cma_free_object,
> > > >  .gem_vm_ops= &drm_gem_cma_vm_ops,
> > > >  .dumb_create= komeda_gem_cma_dumb_create,
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[Daniel Vetter]

On Thu, Jun 13, 2019 at 09:28:13AM +0100, Liviu Dudau wrote:
> On Thu, Jun 13, 2019 at 10:17:27AM +0200, Daniel Vetter wrote:
> > On Wed, Jun 12, 2019 at 02:26:24AM +0000, james qian wang (Arm Technology China) wrote:
> > > On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> > > > On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> > > > > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> > > > >
> > > > > The komeda internal resources (pipelines) are shared between crtcs,
> > > > > and resources release by disable_crtc. This commit is working for once
> > > > > user forgot disabling crtc like app quit abnomally, and then the
> > > > > resources can not be used by another crtc. Adds drop_master to
> > > > > shutdown the device and make sure all the komeda resources have been
> > > > > released and can be used for the next usage.
> > > > >
> > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > > > > ---
> > > > >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
> > > > >  1 file changed, 13 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > index 8543860..647bce5 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> > > > >  return status;
> > > > >  }
> > > > >
> > > > > +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> > > > > + * are released by disable_crtc. But if user forget disabling crtc like app quit
> > > > > + * abnormally, the resources can not be used by another crtc.
> > > > > + * Use drop_master to shutdown the device and make sure all the komeda resources
> > > > > + * have been released, and can be used for the next usage.
> > > > > + */
> > > >
> > > > No. If we want this, we need to implement this across drivers, not with
> > > > per-vendor hacks.
> > > >
> > > > The kerneldoc should have been a solid hint: "Only used by vmwgfx."
> > > > -Daniel
> > > 
> > > Hi Daniel:
> > > This drop_master is really what we want, can we update the doc and
> > > add komeda as a user of this hacks like "used by vmwfgx and komeda",
> > > or maybe directly promote this per-vendor hacks as an optional chip
> > > function ?
> > 
> > Still no, because it would mean different behaviour for arm/komeda
> > compared to everyone else. And we really don't want this, because this
> > would completely break flicker-less vt-switching.
> > 
> > Currently the only fallback for this case is the lastclose handler, which
> > atm just restores fbcon/fbdev. If you want to change/extend that to work
> > without fbdev, then that's the place to do the change. And across _all_
> > drm kms drivers, so that we have consistent behaviour.
> 
> Slightly unrelated, just thinking of a solution and wanted confirmation/double
> checking: can a CRTC be instantiated without any planes (or without a primary
> plane)?

Without a primary plane maybe not so recommended, because it would break
all the legacy userspace. Might even result in some oopses, not sure we
check for crtc->primary != NULL.

I'm not sure what you mean about instantiating it without any plane at
all. That would be rather useless.
-Daniel

> 
> Best regards,
> Liviu
> 
> > 
> > kms is a cross-vendor api, vendor hacks are very, very much not cool.
> > -Daniel
> > 
> > > 
> > > James
> > > 
> > > > > +static void komeda_kms_drop_master(struct drm_device *dev,
> > > > > +   struct drm_file *file_priv)
> > > > > +{
> > > > > +drm_atomic_helper_shutdown(dev);
> > > > > +}
> > > > > +
> > > > >  static struct drm_driver komeda_kms_driver = {
> > > > >  .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
> > > > >     DRIVER_PRIME | DRIVER_HAVE_IRQ,
> > > > >  .lastclose= drm_fb_helper_lastclose,
> > > > >  .irq_handler= komeda_kms_irq_handler,
> > > > > +.master_drop= komeda_kms_drop_master,
> > > > >  .gem_free_object_unlocked= drm_gem_cma_free_object,
> > > > >  .gem_vm_ops= &drm_gem_cma_vm_ops,
> > > > >  .dumb_create= komeda_gem_cma_dumb_create,
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[Liviu Dudau]

On Thu, Jun 13, 2019 at 11:08:14AM +0200, Daniel Vetter wrote:
> On Thu, Jun 13, 2019 at 09:28:13AM +0100, Liviu Dudau wrote:
> > On Thu, Jun 13, 2019 at 10:17:27AM +0200, Daniel Vetter wrote:
> > > On Wed, Jun 12, 2019 at 02:26:24AM +0000, james qian wang (Arm Technology China) wrote:
> > > > On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> > > > > On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> > > > > > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> > > > > >
> > > > > > The komeda internal resources (pipelines) are shared between crtcs,
> > > > > > and resources release by disable_crtc. This commit is working for once
> > > > > > user forgot disabling crtc like app quit abnomally, and then the
> > > > > > resources can not be used by another crtc. Adds drop_master to
> > > > > > shutdown the device and make sure all the komeda resources have been
> > > > > > released and can be used for the next usage.
> > > > > >
> > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
> > > > > >  1 file changed, 13 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > index 8543860..647bce5 100644
> > > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> > > > > >  return status;
> > > > > >  }
> > > > > >
> > > > > > +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> > > > > > + * are released by disable_crtc. But if user forget disabling crtc like app quit
> > > > > > + * abnormally, the resources can not be used by another crtc.
> > > > > > + * Use drop_master to shutdown the device and make sure all the komeda resources
> > > > > > + * have been released, and can be used for the next usage.
> > > > > > + */
> > > > >
> > > > > No. If we want this, we need to implement this across drivers, not with
> > > > > per-vendor hacks.
> > > > >
> > > > > The kerneldoc should have been a solid hint: "Only used by vmwgfx."
> > > > > -Daniel
> > > > 
> > > > Hi Daniel:
> > > > This drop_master is really what we want, can we update the doc and
> > > > add komeda as a user of this hacks like "used by vmwfgx and komeda",
> > > > or maybe directly promote this per-vendor hacks as an optional chip
> > > > function ?
> > > 
> > > Still no, because it would mean different behaviour for arm/komeda
> > > compared to everyone else. And we really don't want this, because this
> > > would completely break flicker-less vt-switching.
> > > 
> > > Currently the only fallback for this case is the lastclose handler, which
> > > atm just restores fbcon/fbdev. If you want to change/extend that to work
> > > without fbdev, then that's the place to do the change. And across _all_
> > > drm kms drivers, so that we have consistent behaviour.
> > 
> > Slightly unrelated, just thinking of a solution and wanted confirmation/double
> > checking: can a CRTC be instantiated without any planes (or without a primary
> > plane)?
> 
> Without a primary plane maybe not so recommended, because it would break
> all the legacy userspace. Might even result in some oopses, not sure we
> check for crtc->primary != NULL.
> 
> I'm not sure what you mean about instantiating it without any plane at
> all. That would be rather useless.

Agree, and I think I have one way of solving the scenario Lowry and James are
trying to cover. Basically, komeda has 2 pipelines that are exposed as 2 crtcs.
However, layers (planes in DRM) can be associated with any of the pipelines and
it is possible to have a DRM master open up crtc0 and enable all possible planes,
which would leave crtc1 with no available layer to use (but technically still visible to
userspace, as it has been drm_crtc_init-ed. James and Lowry are trying to give
another master a chance of enabling crtc1 if previous master drops the
ownership of crtc0 without disabling it. So one solution I'm thinking of is to
tie one of the layers/planes to crtc1 regardless if that pipeline is enabled or
not.

Alternatively, we need a more generic solution for re-allocating resources
between CRTCs that might be enabled at different times. Ideas on how userspace
should handle it first are welcome as well.

Best regards,
Liviu

> -Daniel
> 
> > 
> > Best regards,
> > Liviu
> > 
> > > 
> > > kms is a cross-vendor api, vendor hacks are very, very much not cool.
> > > -Daniel
> > > 
> > > > 
> > > > James
> > > > 
> > > > > > +static void komeda_kms_drop_master(struct drm_device *dev,
> > > > > > +   struct drm_file *file_priv)
> > > > > > +{
> > > > > > +drm_atomic_helper_shutdown(dev);
> > > > > > +}
> > > > > > +
> > > > > >  static struct drm_driver komeda_kms_driver = {
> > > > > >  .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
> > > > > >     DRIVER_PRIME | DRIVER_HAVE_IRQ,
> > > > > >  .lastclose= drm_fb_helper_lastclose,
> > > > > >  .irq_handler= komeda_kms_irq_handler,
> > > > > > +.master_drop= komeda_kms_drop_master,
> > > > > >  .gem_free_object_unlocked= drm_gem_cma_free_object,
> > > > > >  .gem_vm_ops= &drm_gem_cma_vm_ops,
> > > > > >  .dumb_create= komeda_gem_cma_dumb_create,
> > > > > > --
> > > > > > 1.9.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[Daniel Vetter]

On Thu, Jun 13, 2019 at 02:24:37PM +0100, Liviu Dudau wrote:
> On Thu, Jun 13, 2019 at 11:08:14AM +0200, Daniel Vetter wrote:
> > On Thu, Jun 13, 2019 at 09:28:13AM +0100, Liviu Dudau wrote:
> > > On Thu, Jun 13, 2019 at 10:17:27AM +0200, Daniel Vetter wrote:
> > > > On Wed, Jun 12, 2019 at 02:26:24AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> > > > > > On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> > > > > > > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> > > > > > >
> > > > > > > The komeda internal resources (pipelines) are shared between crtcs,
> > > > > > > and resources release by disable_crtc. This commit is working for once
> > > > > > > user forgot disabling crtc like app quit abnomally, and then the
> > > > > > > resources can not be used by another crtc. Adds drop_master to
> > > > > > > shutdown the device and make sure all the komeda resources have been
> > > > > > > released and can be used for the next usage.
> > > > > > >
> > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
> > > > > > >  1 file changed, 13 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > index 8543860..647bce5 100644
> > > > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> > > > > > >  return status;
> > > > > > >  }
> > > > > > >
> > > > > > > +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> > > > > > > + * are released by disable_crtc. But if user forget disabling crtc like app quit
> > > > > > > + * abnormally, the resources can not be used by another crtc.
> > > > > > > + * Use drop_master to shutdown the device and make sure all the komeda resources
> > > > > > > + * have been released, and can be used for the next usage.
> > > > > > > + */
> > > > > >
> > > > > > No. If we want this, we need to implement this across drivers, not with
> > > > > > per-vendor hacks.
> > > > > >
> > > > > > The kerneldoc should have been a solid hint: "Only used by vmwgfx."
> > > > > > -Daniel
> > > > > 
> > > > > Hi Daniel:
> > > > > This drop_master is really what we want, can we update the doc and
> > > > > add komeda as a user of this hacks like "used by vmwfgx and komeda",
> > > > > or maybe directly promote this per-vendor hacks as an optional chip
> > > > > function ?
> > > > 
> > > > Still no, because it would mean different behaviour for arm/komeda
> > > > compared to everyone else. And we really don't want this, because this
> > > > would completely break flicker-less vt-switching.
> > > > 
> > > > Currently the only fallback for this case is the lastclose handler, which
> > > > atm just restores fbcon/fbdev. If you want to change/extend that to work
> > > > without fbdev, then that's the place to do the change. And across _all_
> > > > drm kms drivers, so that we have consistent behaviour.
> > > 
> > > Slightly unrelated, just thinking of a solution and wanted confirmation/double
> > > checking: can a CRTC be instantiated without any planes (or without a primary
> > > plane)?
> > 
> > Without a primary plane maybe not so recommended, because it would break
> > all the legacy userspace. Might even result in some oopses, not sure we
> > check for crtc->primary != NULL.
> > 
> > I'm not sure what you mean about instantiating it without any plane at
> > all. That would be rather useless.
> 
> Agree, and I think I have one way of solving the scenario Lowry and James are
> trying to cover. Basically, komeda has 2 pipelines that are exposed as 2 crtcs.
> However, layers (planes in DRM) can be associated with any of the pipelines and
> it is possible to have a DRM master open up crtc0 and enable all possible planes,
> which would leave crtc1 with no available layer to use (but technically still visible to
> userspace, as it has been drm_crtc_init-ed. James and Lowry are trying to give
> another master a chance of enabling crtc1 if previous master drops the
> ownership of crtc0 without disabling it. So one solution I'm thinking of is to
> tie one of the layers/planes to crtc1 regardless if that pipeline is enabled or
> not.
> 
> Alternatively, we need a more generic solution for re-allocating resources
> between CRTCs that might be enabled at different times. Ideas on how userspace
> should handle it first are welcome as well.

Uh, you can't have more than one active master. And that other master
needs to clean up the mess left behind by the previous one. Generally that
means disabling all the planes, like e.g. fbdev emulation does (but
there's a lot more we should probably clean up, current code is kinda just
good enough).

If you want multiple concurrent masters on the same hw, then you need drm
leases, and there you can limit the leases to however many planes you
think they need.
-Daniel

> 
> Best regards,
> Liviu
> 
> > -Daniel
> > 
> > > 
> > > Best regards,
> > > Liviu
> > > 
> > > > 
> > > > kms is a cross-vendor api, vendor hacks are very, very much not cool.
> > > > -Daniel
> > > > 
> > > > > 
> > > > > James
> > > > > 
> > > > > > > +static void komeda_kms_drop_master(struct drm_device *dev,
> > > > > > > +   struct drm_file *file_priv)
> > > > > > > +{
> > > > > > > +drm_atomic_helper_shutdown(dev);
> > > > > > > +}
> > > > > > > +
> > > > > > >  static struct drm_driver komeda_kms_driver = {
> > > > > > >  .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
> > > > > > >     DRIVER_PRIME | DRIVER_HAVE_IRQ,
> > > > > > >  .lastclose= drm_fb_helper_lastclose,
> > > > > > >  .irq_handler= komeda_kms_irq_handler,
> > > > > > > +.master_drop= komeda_kms_drop_master,
> > > > > > >  .gem_free_object_unlocked= drm_gem_cma_free_object,
> > > > > > >  .gem_vm_ops= &drm_gem_cma_vm_ops,
> > > > > > >  .dumb_create= komeda_gem_cma_dumb_create,
> > > > > > > --
> > > > > > > 1.9.1
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > dri-devel mailing list
> > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > 
> > > > -- 
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > 
> > > -- 
> > > ====================
> > > | I would like to |
> > > | fix the world,  |
> > > | but they're not |
> > > | giving me the   |
> > >  \ source code!  /
> > >   ---------------
> > >     ¯\_(ツ)_/¯
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[james qian wang (Arm Technology China)]

On Thu, Jun 13, 2019 at 04:30:08PM +0200, Daniel Vetter wrote:
> On Thu, Jun 13, 2019 at 02:24:37PM +0100, Liviu Dudau wrote:
> > On Thu, Jun 13, 2019 at 11:08:14AM +0200, Daniel Vetter wrote:
> > > On Thu, Jun 13, 2019 at 09:28:13AM +0100, Liviu Dudau wrote:
> > > > On Thu, Jun 13, 2019 at 10:17:27AM +0200, Daniel Vetter wrote:
> > > > > On Wed, Jun 12, 2019 at 02:26:24AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > > On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> > > > > > > On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> > > > > > > > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> > > > > > > >
> > > > > > > > The komeda internal resources (pipelines) are shared between crtcs,
> > > > > > > > and resources release by disable_crtc. This commit is working for once
> > > > > > > > user forgot disabling crtc like app quit abnomally, and then the
> > > > > > > > resources can not be used by another crtc. Adds drop_master to
> > > > > > > > shutdown the device and make sure all the komeda resources have been
> > > > > > > > released and can be used for the next usage.
> > > > > > > >
> > > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
> > > > > > > >  1 file changed, 13 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > index 8543860..647bce5 100644
> > > > > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> > > > > > > >  return status;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> > > > > > > > + * are released by disable_crtc. But if user forget disabling crtc like app quit
> > > > > > > > + * abnormally, the resources can not be used by another crtc.
> > > > > > > > + * Use drop_master to shutdown the device and make sure all the komeda resources
> > > > > > > > + * have been released, and can be used for the next usage.
> > > > > > > > + */
> > > > > > >
> > > > > > > No. If we want this, we need to implement this across drivers, not with
> > > > > > > per-vendor hacks.
> > > > > > >
> > > > > > > The kerneldoc should have been a solid hint: "Only used by vmwgfx."
> > > > > > > -Daniel
> > > > > >
> > > > > > Hi Daniel:
> > > > > > This drop_master is really what we want, can we update the doc and
> > > > > > add komeda as a user of this hacks like "used by vmwfgx and komeda",
> > > > > > or maybe directly promote this per-vendor hacks as an optional chip
> > > > > > function ?
> > > > >
> > > > > Still no, because it would mean different behaviour for arm/komeda
> > > > > compared to everyone else. And we really don't want this, because this
> > > > > would completely break flicker-less vt-switching.
> > > > >
> > > > > Currently the only fallback for this case is the lastclose handler, which
> > > > > atm just restores fbcon/fbdev. If you want to change/extend that to work
> > > > > without fbdev, then that's the place to do the change. And across _all_
> > > > > drm kms drivers, so that we have consistent behaviour.
> > > >
> > > > Slightly unrelated, just thinking of a solution and wanted confirmation/double
> > > > checking: can a CRTC be instantiated without any planes (or without a primary
> > > > plane)?
> > >
> > > Without a primary plane maybe not so recommended, because it would break
> > > all the legacy userspace. Might even result in some oopses, not sure we
> > > check for crtc->primary != NULL.
> > >
> > > I'm not sure what you mean about instantiating it without any plane at
> > > all. That would be rather useless.
> >
> > Agree, and I think I have one way of solving the scenario Lowry and James are
> > trying to cover. Basically, komeda has 2 pipelines that are exposed as 2 crtcs.
> > However, layers (planes in DRM) can be associated with any of the pipelines and
> > it is possible to have a DRM master open up crtc0 and enable all possible planes,
> > which would leave crtc1 with no available layer to use (but technically still visible to
> > userspace, as it has been drm_crtc_init-ed. James and Lowry are trying to give
> > another master a chance of enabling crtc1 if previous master drops the
> > ownership of crtc0 without disabling it. So one solution I'm thinking of is to
> > tie one of the layers/planes to crtc1 regardless if that pipeline is enabled or
> > not.
> >
> > Alternatively, we need a more generic solution for re-allocating resources
> > between CRTCs that might be enabled at different times. Ideas on how userspace
> > should handle it first are welcome as well.
>
> Uh, you can't have more than one active master. And that other master
> needs to clean up the mess left behind by the previous one. Generally that

Yes komeda needs such clean up (disableing all planes is
enough for such cleanup), but no matter it handles by old master or
the new master, just before the new master enable the crtc is enouth
for komeda.
I think the problem here is which place we should do such cleanup:
USER or KERNEL mode.

For kernel: set_master/drop_master is a good place.
For user: enter_vt and leave_vt.

Why I decide to add it to kernel:
- So many users: X/wayland/Android, avoid duplidated code.
- we also plan to support third-part user. they may not do such
  cleanup for komeda.

Thanks
James

> means disabling all the planes, like e.g. fbdev emulation does (but
> there's a lot more we should probably clean up, current code is kinda just
> good enough).
>
> If you want multiple concurrent masters on the same hw, then you need drm
> leases, and there you can limit the leases to however many planes you
> think they need.
> -Daniel
>
> >
> > Best regards,
> > Liviu
> >
> > > -Daniel
> > >
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > >
> > > > > kms is a cross-vendor api, vendor hacks are very, very much not cool.
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > James
> > > > > >
> > > > > > > > +static void komeda_kms_drop_master(struct drm_device *dev,
> > > > > > > > +   struct drm_file *file_priv)
> > > > > > > > +{
> > > > > > > > +drm_atomic_helper_shutdown(dev);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static struct drm_driver komeda_kms_driver = {
> > > > > > > >  .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
> > > > > > > >     DRIVER_PRIME | DRIVER_HAVE_IRQ,
> > > > > > > >  .lastclose= drm_fb_helper_lastclose,
> > > > > > > >  .irq_handler= komeda_kms_irq_handler,
> > > > > > > > +.master_drop= komeda_kms_drop_master,
> > > > > > > >  .gem_free_object_unlocked= drm_gem_cma_free_object,
> > > > > > > >  .gem_vm_ops= &drm_gem_cma_vm_ops,
> > > > > > > >  .dumb_create= komeda_gem_cma_dumb_create,
> > > > > > > > --
> > > > > > > > 1.9.1
> > > > > > > >
> > > > > > > > _______________________________________________
> > > > > > > > dri-devel mailing list
> > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > >
> > > > > > > --
> > > > > > > Daniel Vetter
> > > > > > > Software Engineer, Intel Corporation
> > > > > > > http://blog.ffwll.ch
> > > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > >
> > > > --
> > > > ====================
> > > > | I would like to |
> > > > | fix the world,  |
> > > > | but they're not |
> > > > | giving me the   |
> > > >  \ source code!  /
> > > >   ---------------
> > > >     ¯\_(ツ)_/¯
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

--
Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[Daniel Vetter]

On Fri, Jun 14, 2019 at 05:46:04AM +0000, james qian wang (Arm Technology China) wrote:
> On Thu, Jun 13, 2019 at 04:30:08PM +0200, Daniel Vetter wrote:
> > On Thu, Jun 13, 2019 at 02:24:37PM +0100, Liviu Dudau wrote:
> > > On Thu, Jun 13, 2019 at 11:08:14AM +0200, Daniel Vetter wrote:
> > > > On Thu, Jun 13, 2019 at 09:28:13AM +0100, Liviu Dudau wrote:
> > > > > On Thu, Jun 13, 2019 at 10:17:27AM +0200, Daniel Vetter wrote:
> > > > > > On Wed, Jun 12, 2019 at 02:26:24AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > > > On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> > > > > > > > On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> > > > > > > > > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> > > > > > > > >
> > > > > > > > > The komeda internal resources (pipelines) are shared between crtcs,
> > > > > > > > > and resources release by disable_crtc. This commit is working for once
> > > > > > > > > user forgot disabling crtc like app quit abnomally, and then the
> > > > > > > > > resources can not be used by another crtc. Adds drop_master to
> > > > > > > > > shutdown the device and make sure all the komeda resources have been
> > > > > > > > > released and can be used for the next usage.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
> > > > > > > > >  1 file changed, 13 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > index 8543860..647bce5 100644
> > > > > > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> > > > > > > > >  return status;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> > > > > > > > > + * are released by disable_crtc. But if user forget disabling crtc like app quit
> > > > > > > > > + * abnormally, the resources can not be used by another crtc.
> > > > > > > > > + * Use drop_master to shutdown the device and make sure all the komeda resources
> > > > > > > > > + * have been released, and can be used for the next usage.
> > > > > > > > > + */
> > > > > > > >
> > > > > > > > No. If we want this, we need to implement this across drivers, not with
> > > > > > > > per-vendor hacks.
> > > > > > > >
> > > > > > > > The kerneldoc should have been a solid hint: "Only used by vmwgfx."
> > > > > > > > -Daniel
> > > > > > >
> > > > > > > Hi Daniel:
> > > > > > > This drop_master is really what we want, can we update the doc and
> > > > > > > add komeda as a user of this hacks like "used by vmwfgx and komeda",
> > > > > > > or maybe directly promote this per-vendor hacks as an optional chip
> > > > > > > function ?
> > > > > >
> > > > > > Still no, because it would mean different behaviour for arm/komeda
> > > > > > compared to everyone else. And we really don't want this, because this
> > > > > > would completely break flicker-less vt-switching.
> > > > > >
> > > > > > Currently the only fallback for this case is the lastclose handler, which
> > > > > > atm just restores fbcon/fbdev. If you want to change/extend that to work
> > > > > > without fbdev, then that's the place to do the change. And across _all_
> > > > > > drm kms drivers, so that we have consistent behaviour.
> > > > >
> > > > > Slightly unrelated, just thinking of a solution and wanted confirmation/double
> > > > > checking: can a CRTC be instantiated without any planes (or without a primary
> > > > > plane)?
> > > >
> > > > Without a primary plane maybe not so recommended, because it would break
> > > > all the legacy userspace. Might even result in some oopses, not sure we
> > > > check for crtc->primary != NULL.
> > > >
> > > > I'm not sure what you mean about instantiating it without any plane at
> > > > all. That would be rather useless.
> > >
> > > Agree, and I think I have one way of solving the scenario Lowry and James are
> > > trying to cover. Basically, komeda has 2 pipelines that are exposed as 2 crtcs.
> > > However, layers (planes in DRM) can be associated with any of the pipelines and
> > > it is possible to have a DRM master open up crtc0 and enable all possible planes,
> > > which would leave crtc1 with no available layer to use (but technically still visible to
> > > userspace, as it has been drm_crtc_init-ed. James and Lowry are trying to give
> > > another master a chance of enabling crtc1 if previous master drops the
> > > ownership of crtc0 without disabling it. So one solution I'm thinking of is to
> > > tie one of the layers/planes to crtc1 regardless if that pipeline is enabled or
> > > not.
> > >
> > > Alternatively, we need a more generic solution for re-allocating resources
> > > between CRTCs that might be enabled at different times. Ideas on how userspace
> > > should handle it first are welcome as well.
> >
> > Uh, you can't have more than one active master. And that other master
> > needs to clean up the mess left behind by the previous one. Generally that
> 
> Yes komeda needs such clean up (disableing all planes is
> enough for such cleanup), but no matter it handles by old master or
> the new master, just before the new master enable the crtc is enouth
> for komeda.
> I think the problem here is which place we should do such cleanup:
> USER or KERNEL mode.
> 
> For kernel: set_master/drop_master is a good place.
> For user: enter_vt and leave_vt.

Well someone else already made that decision for you, userspace needs to
clean up.

If we want to change that, then we need to have a subsystem wide
discussion, and roll out the new behaviour (without breaking any
expectations of current userspace, which your patch does) for everyone.

For more context, I've written up what could all be possible new/existing
solutions in this space quite a while ago:

https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html

> Why I decide to add it to kernel:
> - So many users: X/wayland/Android, avoid duplidated code.
> - we also plan to support third-part user. they may not do such
>   cleanup for komeda.

Fix your userspace :-)

Cheers, Daniel

> 
> Thanks
> James
> 
> > means disabling all the planes, like e.g. fbdev emulation does (but
> > there's a lot more we should probably clean up, current code is kinda just
> > good enough).
> >
> > If you want multiple concurrent masters on the same hw, then you need drm
> > leases, and there you can limit the leases to however many planes you
> > think they need.
> > -Daniel
> >
> > >
> > > Best regards,
> > > Liviu
> > >
> > > > -Daniel
> > > >
> > > > >
> > > > > Best regards,
> > > > > Liviu
> > > > >
> > > > > >
> > > > > > kms is a cross-vendor api, vendor hacks are very, very much not cool.
> > > > > > -Daniel
> > > > > >
> > > > > > >
> > > > > > > James
> > > > > > >
> > > > > > > > > +static void komeda_kms_drop_master(struct drm_device *dev,
> > > > > > > > > +   struct drm_file *file_priv)
> > > > > > > > > +{
> > > > > > > > > +drm_atomic_helper_shutdown(dev);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static struct drm_driver komeda_kms_driver = {
> > > > > > > > >  .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
> > > > > > > > >     DRIVER_PRIME | DRIVER_HAVE_IRQ,
> > > > > > > > >  .lastclose= drm_fb_helper_lastclose,
> > > > > > > > >  .irq_handler= komeda_kms_irq_handler,
> > > > > > > > > +.master_drop= komeda_kms_drop_master,
> > > > > > > > >  .gem_free_object_unlocked= drm_gem_cma_free_object,
> > > > > > > > >  .gem_vm_ops= &drm_gem_cma_vm_ops,
> > > > > > > > >  .dumb_create= komeda_gem_cma_dumb_create,
> > > > > > > > > --
> > > > > > > > > 1.9.1
> > > > > > > > >
> > > > > > > > > _______________________________________________
> > > > > > > > > dri-devel mailing list
> > > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > > >
> > > > > > > > --
> > > > > > > > Daniel Vetter
> > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > http://blog.ffwll.ch
> > > > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > > > > > > _______________________________________________
> > > > > > > dri-devel mailing list
> > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > > >
> > > > > --
> > > > > ====================
> > > > > | I would like to |
> > > > > | fix the world,  |
> > > > > | but they're not |
> > > > > | giving me the   |
> > > > >  \ source code!  /
> > > > >   ---------------
> > > > >     ¯\_(ツ)_/¯
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > >
> > > --
> > > ====================
> > > | I would like to |
> > > | fix the world,  |
> > > | but they're not |
> > > | giving me the   |
> > >  \ source code!  /
> > >   ---------------
> > >     ¯\_(ツ)_/¯
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> --
> Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH v2 2/2] drm/komeda: Adds komeda_kms_drop_master

[james qian wang (Arm Technology China)]

On Fri, Jun 14, 2019 at 09:01:11AM +0200, Daniel Vetter wrote:
> On Fri, Jun 14, 2019 at 05:46:04AM +0000, james qian wang (Arm Technology China) wrote:
> > On Thu, Jun 13, 2019 at 04:30:08PM +0200, Daniel Vetter wrote:
> > > On Thu, Jun 13, 2019 at 02:24:37PM +0100, Liviu Dudau wrote:
> > > > On Thu, Jun 13, 2019 at 11:08:14AM +0200, Daniel Vetter wrote:
> > > > > On Thu, Jun 13, 2019 at 09:28:13AM +0100, Liviu Dudau wrote:
> > > > > > On Thu, Jun 13, 2019 at 10:17:27AM +0200, Daniel Vetter wrote:
> > > > > > > On Wed, Jun 12, 2019 at 02:26:24AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > > > > On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> > > > > > > > > On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm Technology China) wrote:
> > > > > > > > > > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
> > > > > > > > > >
> > > > > > > > > > The komeda internal resources (pipelines) are shared between crtcs,
> > > > > > > > > > and resources release by disable_crtc. This commit is working for once
> > > > > > > > > > user forgot disabling crtc like app quit abnomally, and then the
> > > > > > > > > > resources can not be used by another crtc. Adds drop_master to
> > > > > > > > > > shutdown the device and make sure all the komeda resources have been
> > > > > > > > > > released and can be used for the next usage.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13 +++++++++++++
> > > > > > > > > >  1 file changed, 13 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > > index 8543860..647bce5 100644
> > > > > > > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > > @@ -54,11 +54,24 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> > > > > > > > > >  return status;
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +/* Komeda internal resources (pipelines) are shared between crtcs, and resources
> > > > > > > > > > + * are released by disable_crtc. But if user forget disabling crtc like app quit
> > > > > > > > > > + * abnormally, the resources can not be used by another crtc.
> > > > > > > > > > + * Use drop_master to shutdown the device and make sure all the komeda resources
> > > > > > > > > > + * have been released, and can be used for the next usage.
> > > > > > > > > > + */
> > > > > > > > >
> > > > > > > > > No. If we want this, we need to implement this across drivers, not with
> > > > > > > > > per-vendor hacks.
> > > > > > > > >
> > > > > > > > > The kerneldoc should have been a solid hint: "Only used by vmwgfx."
> > > > > > > > > -Daniel
> > > > > > > >
> > > > > > > > Hi Daniel:
> > > > > > > > This drop_master is really what we want, can we update the doc and
> > > > > > > > add komeda as a user of this hacks like "used by vmwfgx and komeda",
> > > > > > > > or maybe directly promote this per-vendor hacks as an optional chip
> > > > > > > > function ?
> > > > > > >
> > > > > > > Still no, because it would mean different behaviour for arm/komeda
> > > > > > > compared to everyone else. And we really don't want this, because this
> > > > > > > would completely break flicker-less vt-switching.
> > > > > > >
> > > > > > > Currently the only fallback for this case is the lastclose handler, which
> > > > > > > atm just restores fbcon/fbdev. If you want to change/extend that to work
> > > > > > > without fbdev, then that's the place to do the change. And across _all_
> > > > > > > drm kms drivers, so that we have consistent behaviour.
> > > > > >
> > > > > > Slightly unrelated, just thinking of a solution and wanted confirmation/double
> > > > > > checking: can a CRTC be instantiated without any planes (or without a primary
> > > > > > plane)?
> > > > >
> > > > > Without a primary plane maybe not so recommended, because it would break
> > > > > all the legacy userspace. Might even result in some oopses, not sure we
> > > > > check for crtc->primary != NULL.
> > > > >
> > > > > I'm not sure what you mean about instantiating it without any plane at
> > > > > all. That would be rather useless.
> > > >
> > > > Agree, and I think I have one way of solving the scenario Lowry and James are
> > > > trying to cover. Basically, komeda has 2 pipelines that are exposed as 2 crtcs.
> > > > However, layers (planes in DRM) can be associated with any of the pipelines and
> > > > it is possible to have a DRM master open up crtc0 and enable all possible planes,
> > > > which would leave crtc1 with no available layer to use (but technically still visible to
> > > > userspace, as it has been drm_crtc_init-ed. James and Lowry are trying to give
> > > > another master a chance of enabling crtc1 if previous master drops the
> > > > ownership of crtc0 without disabling it. So one solution I'm thinking of is to
> > > > tie one of the layers/planes to crtc1 regardless if that pipeline is enabled or
> > > > not.
> > > >
> > > > Alternatively, we need a more generic solution for re-allocating resources
> > > > between CRTCs that might be enabled at different times. Ideas on how userspace
> > > > should handle it first are welcome as well.
> > >
> > > Uh, you can't have more than one active master. And that other master
> > > needs to clean up the mess left behind by the previous one. Generally that
> >
> > Yes komeda needs such clean up (disableing all planes is
> > enough for such cleanup), but no matter it handles by old master or
> > the new master, just before the new master enable the crtc is enouth
> > for komeda.
> > I think the problem here is which place we should do such cleanup:
> > USER or KERNEL mode.
> >
> > For kernel: set_master/drop_master is a good place.
> > For user: enter_vt and leave_vt.
>
> Well someone else already made that decision for you, userspace needs to
> clean up.
>
> If we want to change that, then we need to have a subsystem wide
> discussion, and roll out the new behaviour (without breaking any
> expectations of current userspace, which your patch does) for everyone.
>
> For more context, I've written up what could all be possible new/existing
> solutions in this space quite a while ago:
>
> https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
>
> > Why I decide to add it to kernel:
> > - So many users: X/wayland/Android, avoid duplidated code.
> > - we also plan to support third-part user. they may not do such
> >   cleanup for komeda.
>
> Fix your userspace :-)
>

Hi Daniel:
Thank you, it's very good article, very clear for why we adopt
user-mode to do save/restore for VT-switching.

I think I should follow.

Actually our komeda user has such logic already. :)

Hi Lowry:

Per Daniel, we should drop this patch.

Thanks
James

> Cheers, Daniel
>
> >
> > Thanks
> > James
> >
> > > means disabling all the planes, like e.g. fbdev emulation does (but
> > > there's a lot more we should probably clean up, current code is kinda just
> > > good enough).
> > >
> > > If you want multiple concurrent masters on the same hw, then you need drm
> > > leases, and there you can limit the leases to however many planes you
> > > think they need.
> > > -Daniel
> > >
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > Best regards,
> > > > > > Liviu
> > > > > >
> > > > > > >
> > > > > > > kms is a cross-vendor api, vendor hacks are very, very much not cool.
> > > > > > > -Daniel
> > > > > > >
> > > > > > > >
> > > > > > > > James
> > > > > > > >
> > > > > > > > > > +static void komeda_kms_drop_master(struct drm_device *dev,
> > > > > > > > > > +   struct drm_file *file_priv)
> > > > > > > > > > +{
> > > > > > > > > > +drm_atomic_helper_shutdown(dev);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  static struct drm_driver komeda_kms_driver = {
> > > > > > > > > >  .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC |
> > > > > > > > > >     DRIVER_PRIME | DRIVER_HAVE_IRQ,
> > > > > > > > > >  .lastclose= drm_fb_helper_lastclose,
> > > > > > > > > >  .irq_handler= komeda_kms_irq_handler,
> > > > > > > > > > +.master_drop= komeda_kms_drop_master,
> > > > > > > > > >  .gem_free_object_unlocked= drm_gem_cma_free_object,
> > > > > > > > > >  .gem_vm_ops= &drm_gem_cma_vm_ops,
> > > > > > > > > >  .dumb_create= komeda_gem_cma_dumb_create,
> > > > > > > > > > --
> > > > > > > > > > 1.9.1
> > > > > > > > > >
> > > > > > > > > > _______________________________________________
> > > > > > > > > > dri-devel mailing list
> > > > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Daniel Vetter
> > > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > > http://blog.ffwll.ch
> > > > > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > > > > > > > _______________________________________________
> > > > > > > > dri-devel mailing list
> > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > >
> > > > > > > --
> > > > > > > Daniel Vetter
> > > > > > > Software Engineer, Intel Corporation
> > > > > > > http://blog.ffwll.ch
> > > > > >
> > > > > > --
> > > > > > ====================
> > > > > > | I would like to |
> > > > > > | fix the world,  |
> > > > > > | but they're not |
> > > > > > | giving me the   |
> > > > > >  \ source code!  /
> > > > > >   ---------------
> > > > > >     ¯\_(ツ)_/¯
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > >
> > > > --
> > > > ====================
> > > > | I would like to |
> > > > | fix the world,  |
> > > > | but they're not |
> > > > | giving me the   |
> > > >  \ source code!  /
> > > >   ---------------
> > > >     ¯\_(ツ)_/¯
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

--
Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.