Re: [PATCH v3 1/6] drm/komeda: Add side by side assembling

[Mihail Atanassov <Mihail.Atanassov@arm.com>]

Hi James,

On Thursday, 14 November 2019 08:37:24 GMT james qian wang (Arm Technology China) wrote:
> Komeda HW can support side by side, which splits the internal display
> processing to two single halves (LEFT/RIGHT) and handle them by two
> pipelines separately.
> komeda "side by side" is enabled by DT property: "side_by_side_master",
> once DT configured side by side, komeda need to verify it with HW's
> configuration, and assemble it for the further usage.

A few problems I see with this approach:
 - This property doesn't scale to >2 pipes;
 - Our HW is capable of dynamically switching between SBS and non-SBS
modes, with this DT property you're effectively denying the opportunity
to use the second pipe when the first one can be satisfied with
4 planes and 1px/clk.

If we only want to fix the first problem, then at least we need this
to be a property of the pipeline node with a phandle linking slave to
master (or bidirectional).

For the second, why not do the SBS decision at modeset time?
If the first CRTC has dual-link output and the commit:
 - only drives one CRTC
 - uses up to 4 planes
 - doesn't meet clk requirements without SBS but does with SBS
then we can switch SBS on dynamically.

And we can tweak that decision with power use in mind later on since
there's no user-visible knob.

We can still keep a DT property if we have a use case for it (e.g.
forcing SBS on for some reason), but we might want to name it slightly
more conservatively then, so it doesn't imply that we never do SBS
when it's not there.

Lastly, maintaining that property in combination with the dynamic
modeset-time SBS decision tree means extra code for more or less the
same functionality. <2c>I'm not 100% sure it's worth it.</2c>

> 
> v3: Correct a typo.
> 
> Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> ---
>  .../gpu/drm/arm/display/komeda/komeda_crtc.c  | 13 ++++-
>  .../gpu/drm/arm/display/komeda/komeda_dev.c   |  3 ++
>  .../gpu/drm/arm/display/komeda/komeda_dev.h   |  9 ++++
>  .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 ++
>  .../drm/arm/display/komeda/komeda_pipeline.c  | 50 +++++++++++++++++--
>  .../drm/arm/display/komeda/komeda_pipeline.h  |  1 +
>  6 files changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 1c452ea75999..cee9a1692e71 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -561,21 +561,30 @@ int komeda_kms_setup_crtcs(struct komeda_kms_dev *kms,
>  	kms->n_crtcs = 0;
>  
>  	for (i = 0; i < mdev->n_pipelines; i++) {
> +		/* if sbs, one komeda_dev only can represent one CRTC */
> +		if (mdev->side_by_side && i != mdev->side_by_side_master)
> +			continue;
> +
>  		crtc = &kms->crtcs[kms->n_crtcs];
>  		master = mdev->pipelines[i];
>  
>  		crtc->master = master;
>  		crtc->slave  = komeda_pipeline_get_slave(master);
> +		crtc->side_by_side = mdev->side_by_side;
>  
>  		if (crtc->slave)
>  			sprintf(str, "pipe-%d", crtc->slave->id);
>  		else
>  			sprintf(str, "None");
>  
> -		DRM_INFO("CRTC-%d: master(pipe-%d) slave(%s).\n",
> -			 kms->n_crtcs, master->id, str);
> +		DRM_INFO("CRTC-%d: master(pipe-%d) slave(%s) sbs(%s).\n",
> +			 kms->n_crtcs, master->id, str,
> +			 crtc->side_by_side ? "On" : "Off");
>  
>  		kms->n_crtcs++;
> +
> +		if (mdev->side_by_side)
> +			break;
>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> index 4e46f650fddf..c3fa4835cb8d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> @@ -178,6 +178,9 @@ static int komeda_parse_dt(struct device *dev, struct komeda_dev *mdev)
>  		}
>  	}
>  
> +	mdev->side_by_side = !of_property_read_u32(np, "side_by_side_master",
> +						   &mdev->side_by_side_master);
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> index d406a4d83352..471604b42431 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> @@ -183,6 +183,15 @@ struct komeda_dev {
>  
>  	/** @irq: irq number */
>  	int irq;
> +	/**
> +	 * @side_by_side:
> +	 *
> +	 * on sbs the whole display frame will be split to two halves (1:2),
> +	 * master pipeline handles the left part, slave for the right part
> +	 */
> +	bool side_by_side;

That's a duplicate of the one in komeda_crtc. You don't need both.

> +	/** @side_by_side_master: master pipe id for side by side */
> +	int side_by_side_master;

As I detailed above, this should be on the crtc, otherwise we can't
scale to >2 pipes.

>  
>  	/** @lock: used to protect dpmode */
>  	struct mutex lock;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index 456f3c435719..ae6654fe95e2 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -76,6 +76,9 @@ struct komeda_crtc {
>  	 */
>  	struct komeda_pipeline *slave;
>  
> +	/** @side_by_side: if the master and slave works on side by side mode */
> +	bool side_by_side;
> +
>  	/** @slave_planes: komeda slave planes mask */
>  	u32 slave_planes;
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> index 452e505a1fd3..104e27cc1dc3 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> @@ -326,14 +326,56 @@ static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
>  struct komeda_pipeline *
>  komeda_pipeline_get_slave(struct komeda_pipeline *master)
>  {
> -	struct komeda_component *slave;
> +	struct komeda_dev *mdev = master->mdev;
> +	struct komeda_component *comp, *slave;
> +	u32 avail_inputs;
> +
> +	/* on SBS, slave pipeline merge to master via image processor */
> +	if (mdev->side_by_side) {
> +		comp = &master->improc->base;
> +		avail_inputs = KOMEDA_PIPELINE_IMPROCS;
> +	} else {
> +		comp = &master->compiz->base;
> +		avail_inputs = KOMEDA_PIPELINE_COMPIZS;
> +	}
>  
> -	slave = komeda_component_pickup_input(&master->compiz->base,
> -					      KOMEDA_PIPELINE_COMPIZS);
> +	slave = komeda_component_pickup_input(comp, avail_inputs);
>  
>  	return slave ? slave->pipeline : NULL;
>  }
>  
> +static int komeda_assemble_side_by_side(struct komeda_dev *mdev)
> +{
> +	struct komeda_pipeline *master, *slave;
> +	int i;
> +
> +	if (!mdev->side_by_side)
> +		return 0;
> +
> +	if (mdev->side_by_side_master >= mdev->n_pipelines) {
> +		DRM_ERROR("DT configured side by side master-%d is invalid.\n",
> +			  mdev->side_by_side_master);
> +		return -EINVAL;
> +	}
> +
> +	master = mdev->pipelines[mdev->side_by_side_master];
> +	slave = komeda_pipeline_get_slave(master);
> +	if (!slave || slave->n_layers != master->n_layers) {
> +		DRM_ERROR("Current HW doesn't support side by side.\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!master->dual_link) {
> +		DRM_DEBUG_ATOMIC("SBS can not work without dual link.\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < master->n_layers; i++)
> +		master->layers[i]->sbs_slave = slave->layers[i];
> +
> +	return 0;
> +}
> +
>  int komeda_assemble_pipelines(struct komeda_dev *mdev)
>  {
>  	struct komeda_pipeline *pipe;
> @@ -346,7 +388,7 @@ int komeda_assemble_pipelines(struct komeda_dev *mdev)
>  		komeda_pipeline_dump(pipe);
>  	}
>  
> -	return 0;
> +	return komeda_assemble_side_by_side(mdev);
>  }
>  
>  void komeda_pipeline_dump_register(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 ac8725e24853..20a076cce635 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> @@ -237,6 +237,7 @@ struct komeda_layer {
>  	 * not the source buffer.
>  	 */
>  	struct komeda_layer *right;
> +	struct komeda_layer *sbs_slave;
>  };
>  
>  struct komeda_layer_state {
> 


-- 
Mihail