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 <jsanka@xxxxxxxxxxxxxx>
> > > ---
> > > 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 =
> > &crtc_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 = &dpu_kms->rm;
> > > struct dpu_crtc_mixer *mixer;
> > > @@ -542,8 +542,8 @@ static void _dpu_crtc_setup_mixer_for_encoder(
> > > dpu_rm_init_hw_iter(&ctl_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 = &dpu_crtc->mixers[i];
> > > + for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++)
> > {
> > > + mixer = &cstate->mixers[i];
> > >
> > > if (!dpu_rm_get_hw(rm, &lm_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 *cstate = to_dpu_crtc_state(crtc->state);
> > > struct drm_encoder *enc;
> > >
> > > - dpu_crtc->num_mixers = 0;
> > > - dpu_crtc->mixers_swapped = false;
> > > - memset(dpu_crtc->mixers, 0, sizeof(dpu_crtc->mixers));
> > > + cstate->num_mixers = 0;
> > > + memset(cstate->mixers, 0, sizeof(cstate->mixers));
> >
> > Why is this necessary?
> >
> > >
> > > mutex_lock(&dpu_crtc->crtc_lock);
> > > /* Check for mixers on all encoders attached to this crtc */
> > > @@ -618,7 +618,7 @@ static void _dpu_crtc_setup_lm_bounds(struct
> > drm_crtc *crtc,
> > > crtc_split_width = _dpu_crtc_get_mixer_width(dpu_crtc, cstate,
> > > adj_mode);
> > >
> > > - for (i = 0; i < dpu_crtc->num_mixers; i++) {
> > > + for (i = 0; i < cstate->num_mixers; i++) {
> > > struct drm_rect *r = &cstate->lm_bounds[i];
> > > r->x1 = crtc_split_width * i;
> > > r->y1 = 0;
> > > @@ -635,6 +635,7 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> > *crtc,
> > > struct drm_crtc_state *old_state)
> > > {
> > > struct dpu_crtc *dpu_crtc;
> > > + struct dpu_crtc_state *cstate;
> > > struct drm_encoder *encoder;
> > > struct drm_device *dev;
> > > unsigned long flags;
> > > @@ -654,10 +655,11 @@ static void dpu_crtc_atomic_begin(struct
> > > drm_crtc
> > *crtc,
> > > DPU_DEBUG("crtc%d\n", crtc->base.id);
> > >
> > > dpu_crtc = to_dpu_crtc(crtc);
> > > + cstate = to_dpu_crtc_state(crtc->state);
> > > dev = crtc->dev;
> > > smmu_state = &dpu_crtc->smmu_state;
> > >
> > > - if (!dpu_crtc->num_mixers) {
> > > + if (!cstate->num_mixers) {
> > > _dpu_crtc_setup_mixers(crtc);
> > > _dpu_crtc_setup_lm_bounds(crtc, crtc->state);
> > > }
> > > @@ -684,7 +686,7 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> > *crtc,
> > > * it means we are trying to flush a CRTC whose state is disabled:
> > > * nothing else needs to be done.
> > > */
> > > - if (unlikely(!dpu_crtc->num_mixers))
> > > + if (unlikely(!cstate->num_mixers))
> > > return;
> > >
> > > _dpu_crtc_blend_setup(crtc);
> > > @@ -748,7 +750,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc
> > *crtc,
> > > * it means we are trying to flush a CRTC whose state is disabled:
> > > * nothing else needs to be done.
> > > */
> > > - if (unlikely(!dpu_crtc->num_mixers))
> > > + if (unlikely(!cstate->num_mixers))
> > > return;
> > >
> > > /*
> > > @@ -863,7 +865,7 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > > *crtc)
> > > * it means we are trying to start a CRTC whose state is disabled:
> > > * nothing else needs to be done.
> > > */
> > > - if (unlikely(!dpu_crtc->num_mixers))
> > > + if (unlikely(!cstate->num_mixers))
> > > return;
> > >
> > > DPU_ATRACE_BEGIN("crtc_commit");
> > > @@ -1097,12 +1099,14 @@ static void dpu_crtc_handle_power_event(u32
> > event_type, void *arg)
> > > struct drm_crtc *crtc = arg;
> > > struct dpu_crtc *dpu_crtc;
> > > struct drm_encoder *encoder;
> > > + struct dpu_crtc_state *cstate;
> > >
> > > if (!crtc) {
> > > DPU_ERROR("invalid crtc\n");
> > > return;
> > > }
> > > dpu_crtc = to_dpu_crtc(crtc);
> > > + cstate = to_dpu_crtc_state(dpu_crtc->base.state);
> > >
> > > mutex_lock(&dpu_crtc->crtc_lock);
> > >
> > > @@ -1197,9 +1201,8 @@ static void dpu_crtc_disable(struct drm_crtc
> > *crtc)
> > > dpu_power_handle_unregister_event(dpu_crtc->phandle,
> > > dpu_crtc->power_event);
> > >
> > > - memset(dpu_crtc->mixers, 0, sizeof(dpu_crtc->mixers));
> > > - dpu_crtc->num_mixers = 0;
> > > - dpu_crtc->mixers_swapped = false;
> > > + memset(cstate->mixers, 0, sizeof(cstate->mixers));
> >
> > Same question here, why is this necessary?
> >
> > > + cstate->num_mixers = 0;
> > >
> > > /* disable clk & bw control until clk & bw properties are set */
> > > cstate->bw_control = false;
> > > @@ -1547,6 +1550,8 @@ static int _dpu_debugfs_status_show(struct
> > seq_file *s, void *data)
> > >
> > > dpu_crtc = s->private;
> > > crtc = &dpu_crtc->base;
> > > +
> > > + drm_modeset_lock(&crtc->mutex, NULL);
> >
> > When I suggested this, I was hoping you'd put it in a separate patch.
> > Additionally, you'll probably want modeset_lock_all instead.
>
> crtc state retrieval is introduced in this patch. So I thought it would make
> sense to add the locking as well a part of this patch.
>
> >
> > > cstate = to_dpu_crtc_state(crtc->state);
>From this line, it looks like crtc state retrieval already existed. Not a huge
deal either way as long as the locking is corrected.
Sean
> > >
> > > mutex_lock(&dpu_crtc->crtc_lock);
> > > @@ -1558,8 +1563,8 @@ static int _dpu_debugfs_status_show(struct
> > seq_file *s, void *data)
> > >
> > > seq_puts(s, "\n");
> > >
> > > - for (i = 0; i < dpu_crtc->num_mixers; ++i) {
> > > - m = &dpu_crtc->mixers[i];
> > > + for (i = 0; i < cstate->num_mixers; ++i) {
> > > + m = &cstate->mixers[i];
> > > if (!m->hw_lm)
> > > seq_printf(s, "\tmixer[%d] has no lm\n", i);
> > > else if (!m->hw_ctl)
> > > @@ -1639,6 +1644,7 @@ static int _dpu_debugfs_status_show(struct
> > seq_file *s, void *data)
> > > seq_printf(s, "vblank_enable:%d\n", dpu_crtc->vblank_requested);
> > >
> > > mutex_unlock(&dpu_crtc->crtc_lock);
> > > + drm_modeset_unlock(&crtc->mutex);
> > >
> > > return 0;
> > > }
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > index 5e4dc5c..7aa772f 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > @@ -121,11 +121,6 @@ struct dpu_crtc_frame_event {
> > > * struct dpu_crtc - virtualized CRTC data structure
> > > * @base : Base drm crtc structure
> > > * @name : ASCII description of this crtc
> > > - * @num_ctls : Number of ctl paths in use
> > > - * @num_mixers : Number of mixers in use
> > > - * @mixers_swapped: Whether the mixers have been swapped for
> > > left/right
> > update
> > > - * especially in the case of DSC Merge.
> > > - * @mixers : List of active mixers
> > > * @event : Pointer to last received drm vblank event. If
> > > there
> > is a
> > > * pending vblank event, this will be non-null.
> > > * @vsync_count : Running count of received vsync events
> > > @@ -164,12 +159,6 @@ struct dpu_crtc {
> > > struct drm_crtc base;
> > > char name[DPU_CRTC_NAME_SIZE];
> > >
> > > - /* HW Resources reserved for the crtc */
> > > - u32 num_ctls;
> > > - u32 num_mixers;
> > > - bool mixers_swapped;
> > > - struct dpu_crtc_mixer mixers[CRTC_DUAL_MIXERS];
> > > -
> > > struct drm_pending_vblank_event *event;
> > > u32 vsync_count;
> > >
> > > @@ -221,6 +210,10 @@ struct dpu_crtc {
> > > * @property_values: Current crtc property values
> > > * @input_fence_timeout_ns : Cached input fence timeout, in ns
> > > * @new_perf: new performance state being requested
> > > + * @num_mixers : Number of mixers in use
> > > + * @mixers : List of active mixers
> > > + * @num_ctls : Number of ctl paths in use
> > > + * @hw_ctls : List of active ctl paths
> > > */
> > > struct dpu_crtc_state {
> > > struct drm_crtc_state base;
> > > @@ -232,6 +225,13 @@ struct dpu_crtc_state {
> > > uint64_t input_fence_timeout_ns;
> > >
> > > struct dpu_core_perf_params new_perf;
> > > +
> > > + /* HW Resources reserved for the crtc */
> > > + u32 num_mixers;
> > > + struct dpu_crtc_mixer mixers[CRTC_DUAL_MIXERS];
> > > +
> > > + u32 num_ctls;
> > > + struct dpu_hw_ctl *hw_ctls[CRTC_DUAL_MIXERS];
> > > };
> > >
> > > #define to_dpu_crtc_state(x) \
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > Forum,
> > > a Linux Foundation Collaborative Project
> > >
>
> --
> Jeykumar S
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
