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 <jsa...@codeaurora.org>
---
 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);

        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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Reply via email to