Re: [PATCH v3 1/6] drm/komeda: Add side by side assembling
On Fri, Nov 15, 2019 at 12:02:00AM +, Mihail Atanassov wrote:
> 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).
I had consider this way before, but consider we have no product (now
and in next 2/3 years) can support >2 pipes. So for DT I decide to
focus on current, but you may see I add two side_by_side flags.
- mdev->side_by_side.
- crtc->side_by_side.
And beside the DT parse we use mdev->side_by_side, the real SBS
operation actually based on crtc->side_by_side, then once the HW
changed, we only need to update the SBS assemble/decision code, but no
need to update the real sbs logic.
thanks
James
> 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.
Yes, you're right, current implementation just use simplest way to
show the feature, and for dynamic enable/disable sbs will be added
when we have the real usage case.
> 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.
I think we'd add such support when we have the real use case. :)
> >
> > v3: Correct a typo.
> >
> > Signed-off-by: James Qian Wang (Arm Technology China)
> >
> > ---
> > .../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 = >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
Re: [PATCH v3 1/6] drm/komeda: Add side by side assembling
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.
>
> v3: Correct a typo.
>
> Signed-off-by: James Qian Wang (Arm Technology China)
>
> ---
> .../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 = >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",
> +>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
[PATCH v3 1/6] drm/komeda: Add side by side assembling
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.
v3: Correct a typo.
Signed-off-by: James Qian Wang (Arm Technology China)
---
.../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 = >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",
+ >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;
+ /** @side_by_side_master: master pipe id for side by side */
+ int side_by_side_master;
/** @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 = >improc->base;
+ avail_inputs =
