Re: [Freedreno] [PATCH 10/14] drm/msm/dpu: move hw resource tracking to crtc state
On Fri, Aug 31, 2018 at 12:22:04PM -0700, Jeykumar Sankaran wrote:
> On 2018-08-31 07:56, Sean Paul wrote:
> > On Tue, Aug 28, 2018 at 05:39:59PM -0700, Jeykumar Sankaran wrote:
> > > Prep changes for state based resource management.
> > >
> > > Moves all the hw block tracking for the crtc to the state
> > > object.
> >
> > Changes in v4...
> >
> > >
> > > Signed-off-by: Jeykumar Sankaran
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 60
> > ++--
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 22 ++--
> > > 2 files changed, 44 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > index e061847..7ab320d 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > @@ -163,9 +163,9 @@ static void _dpu_crtc_program_lm_output_roi(struct
> > drm_crtc *crtc)
> > > crtc_state = to_dpu_crtc_state(crtc->state);
> > >
> > > lm_horiz_position = 0;
> > > - for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
> > > + for (lm_idx = 0; lm_idx < crtc_state->num_mixers; lm_idx++) {
> > > const struct drm_rect *lm_roi =
> > _state->lm_bounds[lm_idx];
> > > - struct dpu_hw_mixer *hw_lm =
> > dpu_crtc->mixers[lm_idx].hw_lm;
> > > + struct dpu_hw_mixer *hw_lm =
> > crtc_state->mixers[lm_idx].hw_lm;
> > > struct dpu_hw_mixer_cfg cfg;
> > >
> > > if (!lm_roi || !drm_rect_visible(lm_roi))
> > > @@ -246,7 +246,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
> > drm_crtc *crtc,
> > > fb ? fb->modifier : 0);
> > >
> > > /* blend config update */
> > > - for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++)
> > {
> > > + for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
> > > _dpu_crtc_setup_blend_cfg(mixer + lm_idx,
> > > pstate, format);
> > >
> > > @@ -270,7 +270,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
> > drm_crtc *crtc,
> > > static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
> > > {
> > > struct dpu_crtc *dpu_crtc;
> > > - struct dpu_crtc_state *dpu_crtc_state;
> > > + struct dpu_crtc_state *cstate;
> > > struct dpu_crtc_mixer *mixer;
> > > struct dpu_hw_ctl *ctl;
> > > struct dpu_hw_mixer *lm;
> > > @@ -281,17 +281,17 @@ static void _dpu_crtc_blend_setup(struct
> > > drm_crtc
> > *crtc)
> > > return;
> > >
> > > dpu_crtc = to_dpu_crtc(crtc);
> > > - dpu_crtc_state = to_dpu_crtc_state(crtc->state);
> > > - mixer = dpu_crtc->mixers;
> > > + cstate = to_dpu_crtc_state(crtc->state);
> > > + mixer = cstate->mixers;
> > >
> > > DPU_DEBUG("%s\n", dpu_crtc->name);
> > >
> > > - if (dpu_crtc->num_mixers > CRTC_DUAL_MIXERS) {
> > > - DPU_ERROR("invalid number mixers: %d\n",
> > dpu_crtc->num_mixers);
> > > + if (cstate->num_mixers > CRTC_DUAL_MIXERS) {
> >
> > This is not possible.
> >
> > > + DPU_ERROR("invalid number mixers: %d\n",
> > cstate->num_mixers);
> > > return;
> > > }
> > >
> > > - for (i = 0; i < dpu_crtc->num_mixers; i++) {
> > > + for (i = 0; i < cstate->num_mixers; i++) {
> > > if (!mixer[i].hw_lm || !mixer[i].hw_ctl) {
> > > DPU_ERROR("invalid lm or ctl assigned to
> > mixer\n");
> > > return;
> > > @@ -308,7 +308,7 @@ static void _dpu_crtc_blend_setup(struct drm_crtc
> > *crtc)
> > >
> > > _dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer);
> > >
> > > - for (i = 0; i < dpu_crtc->num_mixers; i++) {
> > > + for (i = 0; i < cstate->num_mixers; i++) {
> > > ctl = mixer[i].hw_ctl;
> > > lm = mixer[i].hw_lm;
> > >
> > > @@ -530,7 +530,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(
> > > struct drm_crtc *crtc,
> > > struct drm_encoder *enc)
> > > {
> > > - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> > > + struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> > > struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
> > > struct dpu_rm *rm = _kms->rm;
> > > struct dpu_crtc_mixer *mixer;
> > > @@ -542,8 +542,8 @@ static void _dpu_crtc_setup_mixer_for_encoder(
> > > dpu_rm_init_hw_iter(_iter, enc->base.id, DPU_HW_BLK_CTL);
> > >
> > > /* Set up all the mixers and ctls reserved by this encoder */
> > > - for (i = dpu_crtc->num_mixers; i < ARRAY_SIZE(dpu_crtc->mixers);
> > i++) {
> > > - mixer = _crtc->mixers[i];
> > > + for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++)
> > {
> > > + mixer = >mixers[i];
> > >
> > > if (!dpu_rm_get_hw(rm, _iter))
> > > break;
> > > @@ -568,7 +568,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(
> > >
> > > mixer->encoder = enc;
> > >
> > > - dpu_crtc->num_mixers++;
> > > + cstate->num_mixers++;
> > >
Re: [Freedreno] [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly
On Fri, Aug 31, 2018 at 12:16:46PM -0700, Jeykumar Sankaran wrote:
> On 2018-08-30 09:24, Sean Paul wrote:
> > On Tue, Aug 28, 2018 at 05:39:54PM -0700, Jeykumar Sankaran wrote:
> > > Identify slave-master encoders during initialization and enable
> > > the encoders explicitly as the current logic has redundant and
> > > ambiguous loops.
> >
> > Please include a "Changes in vX" section so I don't need to figure it
> > out
> > myself.
> >
> Since I split these clean up changes into a new series, I was not sure
> how to handle the versioning. With "changes in v4" log, Should I also
> change the prefix labeling to [PATCH v4 05/14]? A couple of changes
> are also introduced in this series.
Yeah, sometimes it's best just to choose an arbitrarily high version if multiple
series' are coming together. In this case it's more simple since we're just
adding patches to the series, so v4 would have worked. As far as the new
patches,
just add:
Changed in v4:
- Introduced patch (split from "blah blah")
or
Changed in v4:
- Introduced patch (suggested by Sean)
Sean
>
> Thanks,
> Jeykumar S.
>
> > >
> > > Signed-off-by: Jeykumar Sankaran
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 46
> > ++---
> > > 1 file changed, 15 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 991b22c..b223bd2 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -180,6 +180,7 @@ struct dpu_encoder_virt {
> > > unsigned int num_phys_encs;
> > > struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
> > > struct dpu_encoder_phys *cur_master;
> > > + struct dpu_encoder_phys *cur_slave;
> > > struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
> > >
> > > bool intfs_swapped;
> > > @@ -1141,7 +1142,8 @@ void dpu_encoder_virt_restore(struct drm_encoder
> > *drm_enc)
> > > static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
> > > {
> > > struct dpu_encoder_virt *dpu_enc = NULL;
> > > - int i, ret = 0;
> > > + struct dpu_encoder_phys *phys = NULL;
> > > + int ret = 0;
> > > struct drm_display_mode *cur_mode = NULL;
> > >
> > > if (!drm_enc) {
> > > @@ -1154,21 +1156,14 @@ static void dpu_encoder_virt_enable(struct
> > drm_encoder *drm_enc)
> > > trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
> > >cur_mode->vdisplay);
> > >
> > > - dpu_enc->cur_master = NULL;
> > > - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > > - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> > > + /* always enable slave encoder before master */
> > > + phys = dpu_enc->cur_slave;
> >
> > I think this variable makes things less readable. It made sense in the
> > loop since
> > you're indented an extra level and they're obfuscated anyways, but since
> > they're
> > explicit now, could you please just use dpu_enc->cur_slave/master
> > directly?
> >
> > With this nit and the added changelog in the commit addressed,
> >
> > Reviewed-by: Sean Paul
> >
> >
> > > + if (phys && phys->ops.enable)
> > > + phys->ops.enable(phys);
> > >
> > > - if (phys && phys->ops.is_master &&
> > phys->ops.is_master(phys)) {
> > > - DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n",
> > i);
> > > - dpu_enc->cur_master = phys;
> > > - break;
> > > - }
> > > - }
> > > -
> > > - if (!dpu_enc->cur_master) {
> > > - DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
> > > - return;
> > > - }
> > > + phys = dpu_enc->cur_master;
> > > + if (phys && phys->ops.enable)
> > > + phys->ops.enable(phys);
> > >
> > > ret = dpu_encoder_resource_control(drm_enc,
> > DPU_ENC_RC_EVENT_KICKOFF);
> > > if (ret) {
> > > @@ -1177,21 +1172,6 @@ static void dpu_encoder_virt_enable(struct
> > drm_encoder *drm_enc)
> > > return;
> > > }
> > >
> > > - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > > - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> > > -
> > > - if (!phys)
> > > - continue;
> > > -
> > > - if (phys != dpu_enc->cur_master) {
> > > - if (phys->ops.enable)
> > > - phys->ops.enable(phys);
> > > - }
> > > - }
> > > -
> > > - if (dpu_enc->cur_master->ops.enable)
> > > - dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
> > > -
> > > _dpu_encoder_virt_enable_helper(drm_enc);
> > > }
> > >
> > > @@ -2062,6 +2042,11 @@ static int dpu_encoder_virt_add_phys_encs(
> > > ++dpu_enc->num_phys_encs;
> > > }
> > >
> > > + if (params->split_role == ENC_ROLE_SLAVE)
> > > + dpu_enc->cur_slave = enc;
> > > + else
> > > + dpu_enc->cur_master = enc;
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -2228,7 +2213,6 @@ int
Re: [Freedreno] [PATCH 10/14] drm/msm/dpu: move hw resource tracking to crtc state
On 2018-08-31 07:56, Sean Paul wrote:
On Tue, Aug 28, 2018 at 05:39:59PM -0700, Jeykumar Sankaran wrote:
Prep changes for state based resource management.
Moves all the hw block tracking for the crtc to the state
object.
Changes in v4...
Signed-off-by: Jeykumar Sankaran
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 60
++--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 22 ++--
2 files changed, 44 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e061847..7ab320d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -163,9 +163,9 @@ static void _dpu_crtc_program_lm_output_roi(struct
drm_crtc *crtc)
crtc_state = to_dpu_crtc_state(crtc->state);
lm_horiz_position = 0;
- for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
+ for (lm_idx = 0; lm_idx < crtc_state->num_mixers; lm_idx++) {
const struct drm_rect *lm_roi =
_state->lm_bounds[lm_idx];
- struct dpu_hw_mixer *hw_lm =
dpu_crtc->mixers[lm_idx].hw_lm;
+ struct dpu_hw_mixer *hw_lm =
crtc_state->mixers[lm_idx].hw_lm;
struct dpu_hw_mixer_cfg cfg;
if (!lm_roi || !drm_rect_visible(lm_roi))
@@ -246,7 +246,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
fb ? fb->modifier : 0);
/* blend config update */
- for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++)
{
+ for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
_dpu_crtc_setup_blend_cfg(mixer + lm_idx,
pstate, format);
@@ -270,7 +270,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
{
struct dpu_crtc *dpu_crtc;
- struct dpu_crtc_state *dpu_crtc_state;
+ struct dpu_crtc_state *cstate;
struct dpu_crtc_mixer *mixer;
struct dpu_hw_ctl *ctl;
struct dpu_hw_mixer *lm;
@@ -281,17 +281,17 @@ static void _dpu_crtc_blend_setup(struct
drm_crtc
*crtc)
return;
dpu_crtc = to_dpu_crtc(crtc);
- dpu_crtc_state = to_dpu_crtc_state(crtc->state);
- mixer = dpu_crtc->mixers;
+ cstate = to_dpu_crtc_state(crtc->state);
+ mixer = cstate->mixers;
DPU_DEBUG("%s\n", dpu_crtc->name);
- if (dpu_crtc->num_mixers > CRTC_DUAL_MIXERS) {
- DPU_ERROR("invalid number mixers: %d\n",
dpu_crtc->num_mixers);
+ if (cstate->num_mixers > CRTC_DUAL_MIXERS) {
This is not possible.
+ DPU_ERROR("invalid number mixers: %d\n",
cstate->num_mixers);
return;
}
- for (i = 0; i < dpu_crtc->num_mixers; i++) {
+ for (i = 0; i < cstate->num_mixers; i++) {
if (!mixer[i].hw_lm || !mixer[i].hw_ctl) {
DPU_ERROR("invalid lm or ctl assigned to
mixer\n");
return;
@@ -308,7 +308,7 @@ static void _dpu_crtc_blend_setup(struct drm_crtc
*crtc)
_dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer);
- for (i = 0; i < dpu_crtc->num_mixers; i++) {
+ for (i = 0; i < cstate->num_mixers; i++) {
ctl = mixer[i].hw_ctl;
lm = mixer[i].hw_lm;
@@ -530,7 +530,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(
struct drm_crtc *crtc,
struct drm_encoder *enc)
{
- struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
+ struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
struct dpu_rm *rm = _kms->rm;
struct dpu_crtc_mixer *mixer;
@@ -542,8 +542,8 @@ static void _dpu_crtc_setup_mixer_for_encoder(
dpu_rm_init_hw_iter(_iter, enc->base.id, DPU_HW_BLK_CTL);
/* Set up all the mixers and ctls reserved by this encoder */
- for (i = dpu_crtc->num_mixers; i < ARRAY_SIZE(dpu_crtc->mixers);
i++) {
- mixer = _crtc->mixers[i];
+ for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++)
{
+ mixer = >mixers[i];
if (!dpu_rm_get_hw(rm, _iter))
break;
@@ -568,7 +568,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(
mixer->encoder = enc;
- dpu_crtc->num_mixers++;
+ cstate->num_mixers++;
DPU_DEBUG("setup mixer %d: lm %d\n",
i, mixer->hw_lm->idx - LM_0);
DPU_DEBUG("setup mixer %d: ctl %d\n",
@@ -579,11 +579,11 @@ static void _dpu_crtc_setup_mixer_for_encoder(
static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
{
struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
+ struct dpu_crtc_state
Re: [Freedreno] [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly
On 2018-08-30 09:24, Sean Paul wrote:
On Tue, Aug 28, 2018 at 05:39:54PM -0700, Jeykumar Sankaran wrote:
Identify slave-master encoders during initialization and enable
the encoders explicitly as the current logic has redundant and
ambiguous loops.
Please include a "Changes in vX" section so I don't need to figure it
out
myself.
Since I split these clean up changes into a new series, I was not sure
how to handle the versioning. With "changes in v4" log, Should I also
change the prefix labeling to [PATCH v4 05/14]? A couple of changes
are also introduced in this series.
Thanks,
Jeykumar S.
Signed-off-by: Jeykumar Sankaran
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 46
++---
1 file changed, 15 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 991b22c..b223bd2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -180,6 +180,7 @@ struct dpu_encoder_virt {
unsigned int num_phys_encs;
struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
struct dpu_encoder_phys *cur_master;
+ struct dpu_encoder_phys *cur_slave;
struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
bool intfs_swapped;
@@ -1141,7 +1142,8 @@ void dpu_encoder_virt_restore(struct drm_encoder
*drm_enc)
static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
{
struct dpu_encoder_virt *dpu_enc = NULL;
- int i, ret = 0;
+ struct dpu_encoder_phys *phys = NULL;
+ int ret = 0;
struct drm_display_mode *cur_mode = NULL;
if (!drm_enc) {
@@ -1154,21 +1156,14 @@ static void dpu_encoder_virt_enable(struct
drm_encoder *drm_enc)
trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
cur_mode->vdisplay);
- dpu_enc->cur_master = NULL;
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+ /* always enable slave encoder before master */
+ phys = dpu_enc->cur_slave;
I think this variable makes things less readable. It made sense in the
loop since
you're indented an extra level and they're obfuscated anyways, but
since
they're
explicit now, could you please just use dpu_enc->cur_slave/master
directly?
With this nit and the added changelog in the commit addressed,
Reviewed-by: Sean Paul
+ if (phys && phys->ops.enable)
+ phys->ops.enable(phys);
- if (phys && phys->ops.is_master &&
phys->ops.is_master(phys)) {
- DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n",
i);
- dpu_enc->cur_master = phys;
- break;
- }
- }
-
- if (!dpu_enc->cur_master) {
- DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
- return;
- }
+ phys = dpu_enc->cur_master;
+ if (phys && phys->ops.enable)
+ phys->ops.enable(phys);
ret = dpu_encoder_resource_control(drm_enc,
DPU_ENC_RC_EVENT_KICKOFF);
if (ret) {
@@ -1177,21 +1172,6 @@ static void dpu_encoder_virt_enable(struct
drm_encoder *drm_enc)
return;
}
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
- if (!phys)
- continue;
-
- if (phys != dpu_enc->cur_master) {
- if (phys->ops.enable)
- phys->ops.enable(phys);
- }
- }
-
- if (dpu_enc->cur_master->ops.enable)
- dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
-
_dpu_encoder_virt_enable_helper(drm_enc);
}
@@ -2062,6 +2042,11 @@ static int dpu_encoder_virt_add_phys_encs(
++dpu_enc->num_phys_encs;
}
+ if (params->split_role == ENC_ROLE_SLAVE)
+ dpu_enc->cur_slave = enc;
+ else
+ dpu_enc->cur_master = enc;
+
return 0;
}
@@ -2228,7 +2213,6 @@ int dpu_encoder_setup(struct drm_device *dev,
struct drm_encoder *enc,
if (ret)
goto fail;
- dpu_enc->cur_master = NULL;
spin_lock_init(_enc->enc_spinlock);
atomic_set(_enc->frame_done_timeout, 0);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
--
Jeykumar S
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm/msm/dpu: Remove dpu_mdss_isr when dpu_mdss_destroy is called
On Tue, Aug 28, 2018 at 03:23:04PM -0600, Jordan Crouse wrote:
> The MDSS device is created before the MSM driver attempts to bind the
> sub components. If any of the components return -EPROBE_DEFER the MDSS
> device is destroyed and tried again later.
>
> If this happens the "dpu_mdss_isr" interrupt created from the DPU MDSS
> is not freed when the MDSS device is destroyed and has a risk of
> triggering later and hitting a fault by accessing a mmio region that
> no longer exists. Even if the interrupt isn't triggered by
> accident when the device attempts to reprobe it would error out
> when it tries to re-register the interrupt so unconditionally removing
> it in the destroy is the right move.
>
> Switch the device managed "dpu_mdss_isr" to be unmanaged and add a
> free_irq() in the mdss destroy function.
>
> Signed-off-by: Jordan Crouse
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> index 9e533b86682c..2235ef8129f4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> @@ -158,6 +158,8 @@ static void dpu_mdss_destroy(struct drm_device *dev)
>
> _dpu_mdss_irq_domain_fini(dpu_mdss);
>
> + free_irq(platform_get_irq(pdev, 0), dpu_mdss);
> +
> msm_dss_put_clk(mp->clk_config, mp->num_clk);
> devm_kfree(>dev, mp->clk_config);
>
> @@ -215,7 +217,7 @@ int dpu_mdss_init(struct drm_device *dev)
> if (ret)
> goto irq_domain_error;
>
> - ret = devm_request_irq(dev->dev, platform_get_irq(pdev, 0),
> + ret = request_irq(platform_get_irq(pdev, 0),
> dpu_mdss_irq, 0, "dpu_mdss_isr", dpu_mdss);
> if (ret) {
> DPU_ERROR("failed to init irq: %d\n", ret);
> --
> 2.18.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 14/14] drm/msm/dpu: remove cdm block support from resource manager
On Tue, Aug 28, 2018 at 05:40:03PM -0700, Jeykumar Sankaran wrote:
> Support for CDM block is not present in DPU. Remove CDM
> handlers from resource manager.
I think there's some leftovers that can be cleaned up...
$ ack -i cdm
disp/dpu1/dpu_hw_mdss.h
103:DPU_HW_BLK_CDM,
192:enum dpu_cdm {
193:CDM_0 = 1,
194:CDM_1,
195:CDM_MAX
454:#define DPU_DBG_MASK_CDM (1 << 1)
disp/dpu1/dpu_hw_catalog.h
533: * struct dpu_cdm_cfg - information of chroma down blocks
537: * @intf_connect Bitmask of INTF IDs this CDM can connect to
539:struct dpu_cdm_cfg {
737:u32 cdm_count;
738:struct dpu_cdm_cfg *cdm;
776:#define BLK_CDM(s) ((s)->cdm)
disp/dpu1/dpu_hw_catalog.c
324: * CDM sub blocks config
326:static struct dpu_cdm_cfg sdm845_cdm[] = {
328:.name = "cdm_0", .id = CDM_0,
461:.cdm_count = ARRAY_SIZE(sdm845_cdm),
462:.cdm = sdm845_cdm,
>
> Signed-off-by: Jeykumar Sankaran
> ---
> drivers/gpu/drm/msm/Makefile | 1 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 2 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c | 323
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h | 139 --
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 14 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 4 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 18 --
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 17 --
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 68 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h| 4 -
> 11 files changed, 2 insertions(+), 593 deletions(-)
> delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
>
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 261fa79..19ab521 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -58,7 +58,6 @@ msm-y := \
> disp/dpu1/dpu_formats.o \
> disp/dpu1/dpu_hw_blk.o \
> disp/dpu1/dpu_hw_catalog.o \
> - disp/dpu1/dpu_hw_cdm.o \
> disp/dpu1/dpu_hw_ctl.o \
> disp/dpu1/dpu_hw_interrupts.o \
> disp/dpu1/dpu_hw_intf.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index e453271..e844afa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -32,11 +32,9 @@
> /**
> * Encoder functions and data types
> * @intfs: Interfaces this encoder is using, INTF_MODE_NONE if unused
> - * @needs_cdm: Encoder requests a CDM based on pixel format conversion
> needs
> */
> struct dpu_encoder_hw_resources {
> enum dpu_intf_mode intfs[INTF_MAX];
> - bool needs_cdm;
> };
>
> /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index b5fc65c..f796683 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -22,7 +22,6 @@
> #include "dpu_hw_pingpong.h"
> #include "dpu_hw_ctl.h"
> #include "dpu_hw_top.h"
> -#include "dpu_hw_cdm.h"
> #include "dpu_encoder.h"
> #include "dpu_crtc.h"
>
> @@ -205,8 +204,6 @@ struct dpu_encoder_irq {
> * @parent_ops: Callbacks exposed by the parent to the phys_enc
> * @hw_mdptop: Hardware interface to the top registers
> * @hw_ctl: Hardware interface to the ctl registers
> - * @hw_cdm: Hardware interface to the cdm registers
> - * @cdm_cfg: Chroma-down hardware configuration
> * @hw_pp: Hardware interface to the ping pong registers
> * @dpu_kms: Pointer to the dpu_kms top level
> * @cached_mode: DRM mode cached at mode_set time, acted on in enable
> @@ -235,8 +232,6 @@ struct dpu_encoder_phys {
> const struct dpu_encoder_virt_ops *parent_ops;
> struct dpu_hw_mdp *hw_mdptop;
> struct dpu_hw_ctl *hw_ctl;
> - struct dpu_hw_cdm *hw_cdm;
> - struct dpu_hw_cdm_cfg cdm_cfg;
> struct dpu_hw_pingpong *hw_pp;
> struct dpu_kms *dpu_kms;
> struct drm_display_mode cached_mode;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> deleted file mode 100644
> index 554874b..000
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> +++ /dev/null
> @@ -1,323 +0,0 @@
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 and
> - * only version 2 as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - *
Re: [Freedreno] [PATCH 13/14] drm/msm/dpu: remove display H_TILE from encoder
On Tue, Aug 28, 2018 at 05:40:02PM -0700, Jeykumar Sankaran wrote:
> Encoder H_TILE values are not used for allocating the hw blocks.
> no. of hw_intf blocks provides the info.
>
> Signed-off-by: Jeykumar Sankaran
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 3 +--
> 3 files changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 56ef349..bfc082d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -175,8 +175,6 @@ struct dpu_encoder_virt {
> spinlock_t enc_spinlock;
> uint32_t bus_scaling_client;
>
> - uint32_t display_num_of_h_tiles;
> -
> unsigned int num_phys_encs;
> struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
> struct dpu_encoder_phys *cur_master;
> @@ -455,7 +453,6 @@ void dpu_encoder_get_hw_resources(struct drm_encoder
> *drm_enc,
>
> /* Query resources used by phys encs, expected to be without overlap */
> memset(hw_res, 0, sizeof(*hw_res));
> - hw_res->display_num_of_h_tiles = dpu_enc->display_num_of_h_tiles;
>
> for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> @@ -2103,8 +2100,6 @@ static int dpu_encoder_setup_display(struct
> dpu_encoder_virt *dpu_enc,
>
> WARN_ON(disp_info->num_of_h_tiles < 1);
>
> - dpu_enc->display_num_of_h_tiles = disp_info->num_of_h_tiles;
> -
> DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", disp_info->num_of_h_tiles);
>
> if ((disp_info->capabilities & MSM_DISPLAY_CAP_CMD_MODE) ||
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index c5600e6..e453271 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -33,13 +33,10 @@
> * Encoder functions and data types
> * @intfs: Interfaces this encoder is using, INTF_MODE_NONE if unused
> * @needs_cdm: Encoder requests a CDM based on pixel format conversion
> needs
> - * @display_num_of_h_tiles: Number of horizontal tiles in case of split
> - * interface
> */
> struct dpu_encoder_hw_resources {
> enum dpu_intf_mode intfs[INTF_MAX];
> bool needs_cdm;
> - u32 display_num_of_h_tiles;
> };
>
> /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index fbd489f..388ae65 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -813,8 +813,7 @@ static int _dpu_rm_populate_requirements(
> conn_state->connector->connector_type == DRM_MODE_CONNECTOR_DSI)
> reqs->top_ctrl |= BIT(DPU_RM_TOPCTL_DS);
>
> - DRM_DEBUG_KMS("top_ctrl: 0x%llX num_h_tiles: %d\n", reqs->top_ctrl,
> - reqs->hw_res.display_num_of_h_tiles);
> + DRM_DEBUG_KMS("top_ctrl: 0x%llX\n", reqs->top_ctrl);
> DRM_DEBUG_KMS("num_lm: %d num_ctl: %d split_display: %d\n",
> reqs->topology->num_lm, reqs->topology->num_ctl,
> reqs->topology->needs_split_display);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 12/14] drm/msm/dpu: remove topology name
On Tue, Aug 28, 2018 at 05:40:01PM -0700, Jeykumar Sankaran wrote:
> Strip down the support for topology enums. It
> can be replaced with simple hw count checks.
>
Changes in v4:
...
> Signed-off-by: Jeykumar Sankaran
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 3 --
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 1 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 9 --
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 7 +++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 36
> ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 24 ---
> 6 files changed, 19 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index dbf669e..56ef349 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1013,7 +1013,6 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
> struct drm_connector *conn = NULL, *conn_iter;
> struct dpu_rm_hw_iter pp_iter, ctl_iter;
> struct msm_display_topology topology;
> - enum dpu_rm_topology_name topology_name;
> struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
> int i = 0, ret;
>
> @@ -1069,7 +1068,6 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
> hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
> }
>
> - topology_name = dpu_rm_get_topology_name(topology);
> for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>
> @@ -1089,7 +1087,6 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
> phys->hw_ctl = hw_ctl[i];
>
> phys->connector = conn->state->connector;
> - phys->topology_name = topology_name;
> if (phys->ops.mode_set)
> phys->ops.mode_set(phys, mode, adj_mode);
> }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 60f809f..c5600e6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -35,7 +35,6 @@
> * @needs_cdm: Encoder requests a CDM based on pixel format conversion
> needs
> * @display_num_of_h_tiles: Number of horizontal tiles in case of split
> * interface
> - * @topology: Topology of the display
> */
> struct dpu_encoder_hw_resources {
> enum dpu_intf_mode intfs[INTF_MAX];
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index b3917e0..b5fc65c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -24,6 +24,7 @@
> #include "dpu_hw_top.h"
> #include "dpu_hw_cdm.h"
> #include "dpu_encoder.h"
> +#include "dpu_crtc.h"
>
> #define DPU_ENCODER_NAME_MAX 16
>
> @@ -213,7 +214,6 @@ struct dpu_encoder_irq {
> * @split_role: Role to play in a split-panel configuration
> * @intf_mode: Interface mode
> * @intf_idx:Interface index on dpu hardware
> - * @topology_name: topology selected for the display
> * @enc_spinlock:Virtual-Encoder-Wide Spin Lock for IRQ purposes
> * @enable_state:Enable state tracking
> * @vblank_refcount: Reference count of vblank request
> @@ -243,7 +243,6 @@ struct dpu_encoder_phys {
> enum dpu_enc_split_role split_role;
> enum dpu_intf_mode intf_mode;
> enum dpu_intf intf_idx;
> - enum dpu_rm_topology_name topology_name;
> spinlock_t *enc_spinlock;
> enum dpu_enc_enable_state enable_state;
> atomic_t vblank_refcount;
> @@ -361,11 +360,15 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
> static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode(
> struct dpu_encoder_phys *phys_enc)
> {
> + struct dpu_crtc_state *dpu_cstate;
> +
> if (!phys_enc || phys_enc->enable_state == DPU_ENC_DISABLING)
> return BLEND_3D_NONE;
>
> + dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
> +
> if (phys_enc->split_role == ENC_ROLE_SOLO &&
> - phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE)
> + (dpu_cstate->num_mixers == 2))
I guess this should be CRTC_DUAL_MIXERS? Perhaps you should add a helper called
dpu_crtc_state_is_stereo() instead of littering these checks everywhere.
> return BLEND_3D_H_ROW_INT;
>
> return BLEND_3D_NONE;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index 6de13f4..1643e2f 100644
> ---
Re: [Freedreno] [PATCH 11/14] drm/msm/dpu: rename hw_ctl to lm_ctl
On Tue, Aug 28, 2018 at 05:40:00PM -0700, Jeykumar Sankaran wrote:
> Rename hw_ctl to lm_ctl to mean the ctl associated
> with the hw layer mixer block.
>
> sed -i 's/\([*@.>]\)hw_ctl\([^s]\)/\1lm_ctl\2/g' dpu_crtc.c dpu_crtc.h
>
> Signed-off-by: Jeykumar Sankaran
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 26 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 ++--
> 2 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 7ab320d..aeb0f1a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -202,7 +202,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
> *crtc,
> return;
> }
>
> - ctl = mixer->hw_ctl;
> + ctl = mixer->lm_ctl;
> lm = mixer->hw_lm;
> stage_cfg = _crtc->stage_cfg;
> cstate = to_dpu_crtc_state(crtc->state);
> @@ -292,15 +292,15 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
> }
>
> for (i = 0; i < cstate->num_mixers; i++) {
> - if (!mixer[i].hw_lm || !mixer[i].hw_ctl) {
> + if (!mixer[i].hw_lm || !mixer[i].lm_ctl) {
> DPU_ERROR("invalid lm or ctl assigned to mixer\n");
> return;
> }
> mixer[i].mixer_op_mode = 0;
> mixer[i].flush_mask = 0;
> - if (mixer[i].hw_ctl->ops.clear_all_blendstages)
> - mixer[i].hw_ctl->ops.clear_all_blendstages(
> - mixer[i].hw_ctl);
> + if (mixer[i].lm_ctl->ops.clear_all_blendstages)
> + mixer[i].lm_ctl->ops.clear_all_blendstages(
> + mixer[i].lm_ctl);
> }
>
> /* initialize stage cfg */
> @@ -309,7 +309,7 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
> _dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer);
>
> for (i = 0; i < cstate->num_mixers; i++) {
> - ctl = mixer[i].hw_ctl;
> + ctl = mixer[i].lm_ctl;
> lm = mixer[i].hw_lm;
>
> lm->ops.setup_alpha_out(lm, mixer[i].mixer_op_mode);
> @@ -553,14 +553,14 @@ static void _dpu_crtc_setup_mixer_for_encoder(
> if (!dpu_rm_get_hw(rm, _iter)) {
> DPU_DEBUG("no ctl assigned to lm %d, using previous\n",
> mixer->hw_lm->idx - LM_0);
> - mixer->hw_ctl = last_valid_ctl;
> + mixer->lm_ctl = last_valid_ctl;
> } else {
> - mixer->hw_ctl = (struct dpu_hw_ctl *)ctl_iter.hw;
> - last_valid_ctl = mixer->hw_ctl;
> + mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw;
> + last_valid_ctl = mixer->lm_ctl;
> }
>
> /* Shouldn't happen, mixers are always >= ctls */
> - if (!mixer->hw_ctl) {
> + if (!mixer->lm_ctl) {
> DPU_ERROR("no valid ctls found for lm %d\n",
> mixer->hw_lm->idx - LM_0);
> return;
> @@ -572,7 +572,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(
> DPU_DEBUG("setup mixer %d: lm %d\n",
> i, mixer->hw_lm->idx - LM_0);
> DPU_DEBUG("setup mixer %d: ctl %d\n",
> - i, mixer->hw_ctl->idx - CTL_0);
> + i, mixer->lm_ctl->idx - CTL_0);
> }
> }
>
> @@ -1567,11 +1567,11 @@ static int _dpu_debugfs_status_show(struct seq_file
> *s, void *data)
> m = >mixers[i];
> if (!m->hw_lm)
> seq_printf(s, "\tmixer[%d] has no lm\n", i);
> - else if (!m->hw_ctl)
> + else if (!m->lm_ctl)
> seq_printf(s, "\tmixer[%d] has no ctl\n", i);
> else
> seq_printf(s, "\tmixer:%d ctl:%d width:%d height:%d\n",
> - m->hw_lm->idx - LM_0, m->hw_ctl->idx - CTL_0,
> + m->hw_lm->idx - LM_0, m->lm_ctl->idx - CTL_0,
> out_width, mode->vdisplay);
> }
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 7aa772f..9b1056c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -83,14 +83,14 @@ struct dpu_crtc_smmu_state_data {
> /**
> * struct dpu_crtc_mixer: stores the map for each virtual pipeline in the
> CRTC
> * @hw_lm: LM HW Driver context
> - * @hw_ctl: CTL Path HW driver context
> + * @lm_ctl: CTL Path HW driver context
> * @encoder: Encoder attached to this lm & ctl
> * @mixer_op_mode: mixer blending operation mode
>
Re: [Freedreno] [PATCH 10/14] drm/msm/dpu: move hw resource tracking to crtc state
On Tue, Aug 28, 2018 at 05:39:59PM -0700, Jeykumar Sankaran wrote:
> Prep changes for state based resource management.
>
> Moves all the hw block tracking for the crtc to the state
> object.
Changes in v4...
>
> Signed-off-by: Jeykumar Sankaran
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 60
> ++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 22 ++--
> 2 files changed, 44 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index e061847..7ab320d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -163,9 +163,9 @@ static void _dpu_crtc_program_lm_output_roi(struct
> drm_crtc *crtc)
> crtc_state = to_dpu_crtc_state(crtc->state);
>
> lm_horiz_position = 0;
> - for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
> + for (lm_idx = 0; lm_idx < crtc_state->num_mixers; lm_idx++) {
> const struct drm_rect *lm_roi = _state->lm_bounds[lm_idx];
> - struct dpu_hw_mixer *hw_lm = dpu_crtc->mixers[lm_idx].hw_lm;
> + struct dpu_hw_mixer *hw_lm = crtc_state->mixers[lm_idx].hw_lm;
> struct dpu_hw_mixer_cfg cfg;
>
> if (!lm_roi || !drm_rect_visible(lm_roi))
> @@ -246,7 +246,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
> *crtc,
> fb ? fb->modifier : 0);
>
> /* blend config update */
> - for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
> + for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
> _dpu_crtc_setup_blend_cfg(mixer + lm_idx,
> pstate, format);
>
> @@ -270,7 +270,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
> *crtc,
> static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
> {
> struct dpu_crtc *dpu_crtc;
> - struct dpu_crtc_state *dpu_crtc_state;
> + struct dpu_crtc_state *cstate;
> struct dpu_crtc_mixer *mixer;
> struct dpu_hw_ctl *ctl;
> struct dpu_hw_mixer *lm;
> @@ -281,17 +281,17 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
> return;
>
> dpu_crtc = to_dpu_crtc(crtc);
> - dpu_crtc_state = to_dpu_crtc_state(crtc->state);
> - mixer = dpu_crtc->mixers;
> + cstate = to_dpu_crtc_state(crtc->state);
> + mixer = cstate->mixers;
>
> DPU_DEBUG("%s\n", dpu_crtc->name);
>
> - if (dpu_crtc->num_mixers > CRTC_DUAL_MIXERS) {
> - DPU_ERROR("invalid number mixers: %d\n", dpu_crtc->num_mixers);
> + if (cstate->num_mixers > CRTC_DUAL_MIXERS) {
This is not possible.
> + DPU_ERROR("invalid number mixers: %d\n", cstate->num_mixers);
> return;
> }
>
> - for (i = 0; i < dpu_crtc->num_mixers; i++) {
> + for (i = 0; i < cstate->num_mixers; i++) {
> if (!mixer[i].hw_lm || !mixer[i].hw_ctl) {
> DPU_ERROR("invalid lm or ctl assigned to mixer\n");
> return;
> @@ -308,7 +308,7 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
>
> _dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer);
>
> - for (i = 0; i < dpu_crtc->num_mixers; i++) {
> + for (i = 0; i < cstate->num_mixers; i++) {
> ctl = mixer[i].hw_ctl;
> lm = mixer[i].hw_lm;
>
> @@ -530,7 +530,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(
> struct drm_crtc *crtc,
> struct drm_encoder *enc)
> {
> - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> + struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
> struct dpu_rm *rm = _kms->rm;
> struct dpu_crtc_mixer *mixer;
> @@ -542,8 +542,8 @@ static void _dpu_crtc_setup_mixer_for_encoder(
> dpu_rm_init_hw_iter(_iter, enc->base.id, DPU_HW_BLK_CTL);
>
> /* Set up all the mixers and ctls reserved by this encoder */
> - for (i = dpu_crtc->num_mixers; i < ARRAY_SIZE(dpu_crtc->mixers); i++) {
> - mixer = _crtc->mixers[i];
> + for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++) {
> + mixer = >mixers[i];
>
> if (!dpu_rm_get_hw(rm, _iter))
> break;
> @@ -568,7 +568,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(
>
> mixer->encoder = enc;
>
> - dpu_crtc->num_mixers++;
> + cstate->num_mixers++;
> DPU_DEBUG("setup mixer %d: lm %d\n",
> i, mixer->hw_lm->idx - LM_0);
> DPU_DEBUG("setup mixer %d: ctl %d\n",
> @@ -579,11 +579,11 @@ static void _dpu_crtc_setup_mixer_for_encoder(
> static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
> {
> struct dpu_crtc *dpu_crtc =
Re: [Freedreno] [PATCH 09/14] drm/msm/dpu: make crtc get_mixer_width helper static
On Tue, Aug 28, 2018 at 05:39:58PM -0700, Jeykumar Sankaran wrote:
> Mark CRTC get_mixer_width helper API static as it is
> not used outside the file.
>
> Signed-off-by: Jeykumar Sankaran
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 21 ++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 18 --
> 2 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 92eda3e..e061847 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -47,6 +47,20 @@
> #define LEFT_MIXER 0
> #define RIGHT_MIXER 1
>
> +static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc *dpu_crtc,
> + struct dpu_crtc_state *cstate, struct drm_display_mode *mode)
> +{
> + u32 mixer_width;
> +
> + if (!dpu_crtc || !cstate || !mode)
> + return 0;
> +
> + mixer_width = (cstate->num_mixers == CRTC_DUAL_MIXERS ?
> + mode->hdisplay / CRTC_DUAL_MIXERS : mode->hdisplay);
> +
> + return mixer_width;
> +}
This function feels a little too beefy to be inline.
However(!!):
- None of the NULL checks are needed
- the mixer_width local variable isn't needed
- dpu_crtc isn't even used
- the divisor is always == cstate->num_mixers
So you can actually get an inline-worth function out of this, all of that
simplifies down to:
static inline int dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate,
struct drm_display_mode *mode)
{
return mode->hdisplay / cstate->num_mixers;
}
> +
> static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
> {
> struct msm_drm_private *priv;
> @@ -601,7 +615,8 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc
> *crtc,
> cstate = to_dpu_crtc_state(state);
>
> adj_mode = >adjusted_mode;
> - crtc_split_width = dpu_crtc_get_mixer_width(dpu_crtc, cstate, adj_mode);
> + crtc_split_width = _dpu_crtc_get_mixer_width(dpu_crtc, cstate,
> + adj_mode);
>
> for (i = 0; i < dpu_crtc->num_mixers; i++) {
> struct drm_rect *r = >lm_bounds[i];
> @@ -1299,7 +1314,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>
> memset(pipe_staged, 0, sizeof(pipe_staged));
>
> - mixer_width = dpu_crtc_get_mixer_width(dpu_crtc, cstate, mode);
> + mixer_width = _dpu_crtc_get_mixer_width(dpu_crtc, cstate, mode);
>
> _dpu_crtc_setup_lm_bounds(crtc, state);
>
> @@ -1536,7 +1551,7 @@ static int _dpu_debugfs_status_show(struct seq_file *s,
> void *data)
>
> mutex_lock(_crtc->crtc_lock);
> mode = >state->adjusted_mode;
> - out_width = dpu_crtc_get_mixer_width(dpu_crtc, cstate, mode);
> + out_width = _dpu_crtc_get_mixer_width(dpu_crtc, cstate, mode);
>
> seq_printf(s, "crtc:%d width:%d height:%d\n", crtc->base.id,
> mode->hdisplay, mode->vdisplay);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index ec9c538..5e4dc5c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -238,24 +238,6 @@ struct dpu_crtc_state {
> container_of(x, struct dpu_crtc_state, base)
>
> /**
> - * dpu_crtc_get_mixer_width - get the mixer width
> - * Mixer width will be same as panel width(/2 for split)
> - */
> -static inline int dpu_crtc_get_mixer_width(struct dpu_crtc *dpu_crtc,
> - struct dpu_crtc_state *cstate, struct drm_display_mode *mode)
> -{
> - u32 mixer_width;
> -
> - if (!dpu_crtc || !cstate || !mode)
> - return 0;
> -
> - mixer_width = (dpu_crtc->num_mixers == CRTC_DUAL_MIXERS ?
> - mode->hdisplay / CRTC_DUAL_MIXERS : mode->hdisplay);
> -
> - return mixer_width;
> -}
> -
> -/**
> * dpu_crtc_get_mixer_height - get the mixer height
> * Mixer height will be same as panel height
> */
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Patch v15 4/5] dt-bindings: arm-smmu: Add bindings for qcom, smmu-v2
Hi Rob, On 8/30/2018 6:13 AM, Rob Herring wrote: On Wed, Aug 29, 2018 at 6:23 AM Vivek Gautam wrote: On Wed, Aug 29, 2018 at 2:05 PM Vivek Gautam wrote: Hi Rob, On 8/29/2018 2:04 AM, Rob Herring wrote: On Mon, Aug 27, 2018 at 04:25:50PM +0530, Vivek Gautam wrote: Add bindings doc for Qcom's smmu-v2 implementation. Signed-off-by: Vivek Gautam Reviewed-by: Tomasz Figa Tested-by: Srinivas Kandagatla --- Changes since v14: - This is a new patch added in v15 after noticing the new checkpatch warning for separate dt-bindings doc. - This patch also addresses comments given by Rob and Robin to add a list of valid values of '' in "qcom,-smmu-v2" compatible string. .../devicetree/bindings/iommu/arm,smmu.txt | 47 ++ 1 file changed, 47 insertions(+) diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt index 8a6ffce12af5..52198a539606 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt @@ -17,10 +17,24 @@ conditions. "arm,mmu-401" "arm,mmu-500" "cavium,smmu-v2" +"qcom,-smmu-v2", "qcom,smmu-v2" The v2 in the compatible string is kind of redundant unless the SoC has other SMMU types. sdm845 has smmu-v2, and smmu-500 [1]. depending on the particular implementation and/or the version of the architecture implemented. + A number of Qcom SoCs use qcom,smmu-v2 version of the IP. + "qcom,-smmu-v2" represents a soc specific compatible + string that should be present along with the "qcom,smmu-v2" + to facilitate SoC specific clocks/power connections and to + address specific bug fixes. + '' string in "qcom,-smmu-v2" should be one of the + following: + msm8996 - for msm8996 Qcom SoC. + sdm845 - for sdm845 Qcom Soc. Rather than all this prose, it would be simpler to just add 2 lines with the full compatibles rather than . The thing is not going to work when/if we move bindings to json-schema also. then we keep adding "qcom,msm8996-smmu-v2", "qcom,smmu-v2" "qcom,msm8998-smmu-v2", "qcom,smmu-v2" "qcom,sdm845-smmu-v2", "qcom,smmu-v2", and from [1] "qcom,sdm845-smmu-500", "arm,mmu-500", etc. for each SoCs? How about following diff on top of this patch? diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt index 52198a539606..5e6c04876533 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt @@ -17,23 +17,18 @@ conditions. "arm,mmu-401" "arm,mmu-500" "cavium,smmu-v2" -"qcom,-smmu-v2", "qcom,smmu-v2" +"qcom,smmu-v2" depending on the particular implementation and/or the version of the architecture implemented. - A number of Qcom SoCs use qcom,smmu-v2 version of the IP. - "qcom,-smmu-v2" represents a soc specific compatible - string that should be present along with the "qcom,smmu-v2" - to facilitate SoC specific clocks/power connections and to - address specific bug fixes. - '' string in "qcom,-smmu-v2" should be one of the - following: - msm8996 - for msm8996 Qcom SoC. - sdm845 - for sdm845 Qcom Soc. - - An example string would be - - "qcom,msm8996-smmu-v2", "qcom,smmu-v2". + Qcom SoCs using qcom,smmu-v2 must have soc specific + compatible string attached to "qcom,smmu-v2" to take care + of SoC specific clocks/power connections and to address + specific bug fixes. + Precisely, it should be one of the following: + "qcom,msm8996-smmu-v2", "qcom,smmu-v2", + "qcom,sdm845-smmu-v2", "qcom,smmu-v2". We don't need an explanation of why we need specific compatibles in each binding document (though maybe we need a better explanation somewhere). We just need to know what are valid values for compatibles and this includes any combinations. Generally, this is just a list of combinations. [snip] Fixed this in v16. Thanks. Best regards Vivek ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
