Re: [Freedreno] [PATCH 22/25] drm/msm/dpu: make crtc and encoder specific HW reservation
On Tue, Oct 09, 2018 at 11:15:02PM -0700, Jeykumar Sankaran wrote:
> On 2018-10-09 13:41, Sean Paul wrote:
> > On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote:
> > > Instead of letting encoder make a centralized reservation for
> > > all of its display DRM components, this path splits the
> > > responsibility between CRTC and Encoder, each requesting
> > > RM for the HW mapping of its own domain.
> > >
> > > Signed-off-by: Jeykumar Sankaran
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 31 +
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69
> > -
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 36 +++
> > > 4 files changed, 119 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > index 0625f56..0536b8a 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > @@ -47,6 +47,8 @@
> > > #define LEFT_MIXER 0
> > > #define RIGHT_MIXER 1
> > >
> > > +#define MAX_VDISPLAY_SPLIT 1080
> > > +
> > > static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state
> > *cstate,
> > > struct drm_display_mode *mode)
> > > {
> > > @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct
> > drm_crtc *crtc,
> > >
> > > for (i = 0; i < cstate->num_mixers; i++) {
> > > struct drm_rect *r = >lm_bounds[i];
> > > +
> > > r->x1 = crtc_split_width * i;
> > > r->y1 = 0;
> > > r->x2 = r->x1 + crtc_split_width;
> > > @@ -885,6 +888,7 @@ static void dpu_crtc_disable(struct drm_crtc
> > > *crtc)
> > > struct drm_display_mode *mode;
> > > struct drm_encoder *encoder;
> > > struct msm_drm_private *priv;
> > > + struct dpu_kms *dpu_kms;
> > > unsigned long flags;
> > >
> > > if (!crtc || !crtc->dev || !crtc->dev->dev_private ||
> > !crtc->state) {
> > > @@ -895,6 +899,7 @@ static void dpu_crtc_disable(struct drm_crtc
> > > *crtc)
> > > cstate = to_dpu_crtc_state(crtc->state);
> > > mode = >base.adjusted_mode;
> > > priv = crtc->dev->dev_private;
> > > + dpu_kms = to_dpu_kms(priv->kms);
> > >
> > > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
> > >
> > > @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc
> > > *crtc)
> > > crtc->state->event = NULL;
> > > spin_unlock_irqrestore(>dev->event_lock, flags);
> > > }
> > > +
> > > + dpu_rm_crtc_release(_kms->rm, crtc->state);
> > > }
> > >
> > > static void dpu_crtc_enable(struct drm_crtc *crtc,
> > > @@ -1004,6 +1011,21 @@ struct plane_state {
> > > u32 pipe_id;
> > > };
> > >
> > > +static void _dpu_crtc_get_topology(
> > > + struct drm_crtc_state *crtc_state,
> > > + struct drm_display_mode *mode)
> > > +{
> > > + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> > > +
> > > + dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2
> > : 1;
> > > +
> > > + /**
> > > + * encoder->atomic_check is invoked before crtc->atomic_check.
> > > + * so dpu_cstate->num_intfs should have a non-zero value.
> > > + */
> > > + dpu_cstate->num_ctls = dpu_cstate->num_intfs;
> >
> > Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs
> > directly?
> > Also,
> > you don't really need these in their own function, especially if
> > num_ctls
> > goes
> > away.
> >
> Yes. I can live with just that. But since dpu_cstate maintains HW arrays
> for each type, I thought it would be more readable if I could use
> separate variables to track their counts instead of iterating over
> ctl arrays over dpu_cstate->num_intfs and leaving comments that both
> will be same for this version of hardware.
You could change the name to make it more generic. AFAICT,
num_h_tiles == num_phys_encs == num_intfs == num_ctls
So storing it as num_h_tiles might make more sense.
>
> Also, the counts need not be the same for all the Snapdragon variants.
This is probably a good thing. It doesn't seem like the current driver would
work if these values were different, so making it explicit is a good signal that
more invasive changes are needed.
Sean
>
> Thanks,
> Jeykumar S.
> > > +}
> > > +
> > > static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
> > > struct drm_crtc_state *state)
> > > {
> > > @@ -1014,6 +1036,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc
> > *crtc,
> > > const struct drm_plane_state *pstate;
> > > struct drm_plane *plane;
> > > struct drm_display_mode *mode;
> > > + struct msm_drm_private *priv;
> > > + struct dpu_kms *dpu_kms;
> > >
> > > int cnt = 0, rc = 0, mixer_width, i, z_pos;
> > >
> > > @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc
> > *crtc,
> > > goto end;
> > > }
> > >
> > > + priv =
Re: [Freedreno] [PATCH 22/25] drm/msm/dpu: make crtc and encoder specific HW reservation
On 2018-10-09 13:41, Sean Paul wrote:
On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote:
Instead of letting encoder make a centralized reservation for
all of its display DRM components, this path splits the
responsibility between CRTC and Encoder, each requesting
RM for the HW mapping of its own domain.
Signed-off-by: Jeykumar Sankaran
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 31 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69
-
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 36 +++
4 files changed, 119 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 0625f56..0536b8a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -47,6 +47,8 @@
#define LEFT_MIXER 0
#define RIGHT_MIXER 1
+#define MAX_VDISPLAY_SPLIT 1080
+
static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state
*cstate,
struct drm_display_mode *mode)
{
@@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct
drm_crtc *crtc,
for (i = 0; i < cstate->num_mixers; i++) {
struct drm_rect *r = >lm_bounds[i];
+
r->x1 = crtc_split_width * i;
r->y1 = 0;
r->x2 = r->x1 + crtc_split_width;
@@ -885,6 +888,7 @@ static void dpu_crtc_disable(struct drm_crtc
*crtc)
struct drm_display_mode *mode;
struct drm_encoder *encoder;
struct msm_drm_private *priv;
+ struct dpu_kms *dpu_kms;
unsigned long flags;
if (!crtc || !crtc->dev || !crtc->dev->dev_private ||
!crtc->state) {
@@ -895,6 +899,7 @@ static void dpu_crtc_disable(struct drm_crtc
*crtc)
cstate = to_dpu_crtc_state(crtc->state);
mode = >base.adjusted_mode;
priv = crtc->dev->dev_private;
+ dpu_kms = to_dpu_kms(priv->kms);
DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
@@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc
*crtc)
crtc->state->event = NULL;
spin_unlock_irqrestore(>dev->event_lock, flags);
}
+
+ dpu_rm_crtc_release(_kms->rm, crtc->state);
}
static void dpu_crtc_enable(struct drm_crtc *crtc,
@@ -1004,6 +1011,21 @@ struct plane_state {
u32 pipe_id;
};
+static void _dpu_crtc_get_topology(
+ struct drm_crtc_state *crtc_state,
+ struct drm_display_mode *mode)
+{
+ struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
+
+ dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2
: 1;
+
+ /**
+* encoder->atomic_check is invoked before crtc->atomic_check.
+* so dpu_cstate->num_intfs should have a non-zero value.
+*/
+ dpu_cstate->num_ctls = dpu_cstate->num_intfs;
Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs
directly?
Also,
you don't really need these in their own function, especially if
num_ctls
goes
away.
Yes. I can live with just that. But since dpu_cstate maintains HW arrays
for each type, I thought it would be more readable if I could use
separate variables to track their counts instead of iterating over
ctl arrays over dpu_cstate->num_intfs and leaving comments that both
will be same for this version of hardware.
Also, the counts need not be the same for all the Snapdragon variants.
Thanks,
Jeykumar S.
+}
+
static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)
{
@@ -1014,6 +1036,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc
*crtc,
const struct drm_plane_state *pstate;
struct drm_plane *plane;
struct drm_display_mode *mode;
+ struct msm_drm_private *priv;
+ struct dpu_kms *dpu_kms;
int cnt = 0, rc = 0, mixer_width, i, z_pos;
@@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc
*crtc,
goto end;
}
+ priv = crtc->dev->dev_private;
+ dpu_kms = to_dpu_kms(priv->kms);
+
mode = >adjusted_mode;
DPU_DEBUG("%s: check", dpu_crtc->name);
@@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct
drm_crtc
*crtc,
}
}
+ _dpu_crtc_get_topology(state, mode);
+ if (drm_atomic_crtc_needs_modeset(state))
+ rc = dpu_rm_crtc_reserve(_kms->rm, state);
+
end:
kfree(pstates);
return rc;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5d501c8..ce66309 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -67,8 +67,6 @@
#define IDLE_SHORT_TIMEOUT 1
-#define MAX_VDISPLAY_SPLIT 1080
-
/**
* enum dpu_enc_rc_events - events for resource
[Freedreno] [PATCH 22/25] drm/msm/dpu: make crtc and encoder specific HW reservation
Instead of letting encoder make a centralized reservation for
all of its display DRM components, this path splits the
responsibility between CRTC and Encoder, each requesting
RM for the HW mapping of its own domain.
Signed-off-by: Jeykumar Sankaran
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 31 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69 -
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 36 +++
4 files changed, 119 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 0625f56..0536b8a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -47,6 +47,8 @@
#define LEFT_MIXER 0
#define RIGHT_MIXER 1
+#define MAX_VDISPLAY_SPLIT 1080
+
static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate,
struct drm_display_mode *mode)
{
@@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
for (i = 0; i < cstate->num_mixers; i++) {
struct drm_rect *r = >lm_bounds[i];
+
r->x1 = crtc_split_width * i;
r->y1 = 0;
r->x2 = r->x1 + crtc_split_width;
@@ -885,6 +888,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
struct drm_display_mode *mode;
struct drm_encoder *encoder;
struct msm_drm_private *priv;
+ struct dpu_kms *dpu_kms;
unsigned long flags;
if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) {
@@ -895,6 +899,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
cstate = to_dpu_crtc_state(crtc->state);
mode = >base.adjusted_mode;
priv = crtc->dev->dev_private;
+ dpu_kms = to_dpu_kms(priv->kms);
DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
@@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
crtc->state->event = NULL;
spin_unlock_irqrestore(>dev->event_lock, flags);
}
+
+ dpu_rm_crtc_release(_kms->rm, crtc->state);
}
static void dpu_crtc_enable(struct drm_crtc *crtc,
@@ -1004,6 +1011,21 @@ struct plane_state {
u32 pipe_id;
};
+static void _dpu_crtc_get_topology(
+ struct drm_crtc_state *crtc_state,
+ struct drm_display_mode *mode)
+{
+ struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
+
+ dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1;
+
+ /**
+* encoder->atomic_check is invoked before crtc->atomic_check.
+* so dpu_cstate->num_intfs should have a non-zero value.
+*/
+ dpu_cstate->num_ctls = dpu_cstate->num_intfs;
+}
+
static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)
{
@@ -1014,6 +1036,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
const struct drm_plane_state *pstate;
struct drm_plane *plane;
struct drm_display_mode *mode;
+ struct msm_drm_private *priv;
+ struct dpu_kms *dpu_kms;
int cnt = 0, rc = 0, mixer_width, i, z_pos;
@@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
goto end;
}
+ priv = crtc->dev->dev_private;
+ dpu_kms = to_dpu_kms(priv->kms);
+
mode = >adjusted_mode;
DPU_DEBUG("%s: check", dpu_crtc->name);
@@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
}
}
+ _dpu_crtc_get_topology(state, mode);
+ if (drm_atomic_crtc_needs_modeset(state))
+ rc = dpu_rm_crtc_reserve(_kms->rm, state);
+
end:
kfree(pstates);
return rc;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5d501c8..ce66309 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -67,8 +67,6 @@
#define IDLE_SHORT_TIMEOUT 1
-#define MAX_VDISPLAY_SPLIT 1080
-
/**
* enum dpu_enc_rc_events - events for resource control state machine
* @DPU_ENC_RC_EVENT_KICKOFF:
@@ -557,14 +555,10 @@ static void _dpu_encoder_adjust_mode(struct drm_connector
*connector,
static void _dpu_encoder_get_topology(
struct dpu_encoder_virt *dpu_enc,
- struct drm_crtc_state *crtc_state,
- struct drm_display_mode *mode)
+ struct drm_crtc_state *crtc_state)
{
struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
- /* User split topology for width > 1080 */
- dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1;
- dpu_cstate->num_ctls = dpu_enc->num_phys_encs;
