On Tue, Sep 04, 2018 at 04:03:25PM -0700, Jeykumar Sankaran wrote:
> On 2018-08-31 09:08, Sean Paul wrote:
> > 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 <jsanka@xxxxxxxxxxxxxx>
> > > ---
> > > 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
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > @@ -355,13 +355,14 @@ static void
> > > dpu_encoder_phys_vid_underrun_irq(void
> > *arg, int irq_idx)
> > >
> > > static bool _dpu_encoder_phys_is_dual_ctl(struct dpu_encoder_phys
> > *phys_enc)
> > > {
> > > + struct dpu_crtc_state *dpu_cstate;
> > > +
> > > if (!phys_enc)
> > > return false;
> > >
> > > - if (phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE)
> > > - return true;
> > > + dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
> > >
> > > - return false;
> > > + return (dpu_cstate->num_ctls > 1);
> >
> > Unnecessary parens
> >
> > > }
> > >
> > > static bool dpu_encoder_phys_vid_needs_single_flush(
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > index 13c0a36..fbd489f 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > @@ -33,7 +33,6 @@
> > > (t).num_intf == (r).num_intf)
> > >
> > > struct dpu_rm_topology_def {
> > > - enum dpu_rm_topology_name top_name;
> > > int num_lm;
> > > int num_comp_enc;
> > > int num_intf;
> > > @@ -42,10 +41,10 @@ struct dpu_rm_topology_def {
> > > };
> > >
> > > static const struct dpu_rm_topology_def g_top_table[] = {
> > > - { DPU_RM_TOPOLOGY_NONE, 0, 0, 0, 0, false },
> > > - { DPU_RM_TOPOLOGY_SINGLEPIPE, 1, 0, 1, 1, false },
> > > - { DPU_RM_TOPOLOGY_DUALPIPE, 2, 0, 2, 2, true },
> > > - { DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE, 2, 0, 1, 1, false },
> > > + { 0, 0, 0, 0, false },
> > > + { 1, 0, 1, 1, false },
> > > + { 2, 0, 2, 2, true },
> > > + { 2, 0, 1, 1, false },
> >
> > You don't need this. The data it provides can be deduced already:
> > num_ctl == num_intf
> > needs_split_display == (num_intf > 1)
> >
> > > };
> > >
> > > /**
> > > @@ -72,13 +71,11 @@ struct dpu_rm_requirements {
> > > * @enc_id: Reservations are tracked by Encoder DRM object ID.
> > > * CRTCs may be connected to multiple Encoders.
> > > * An encoder or connector id identifies the display path.
> > > - * @topology DRM<->HW topology use case
> > > */
> > > struct dpu_rm_rsvp {
> > > struct list_head list;
> > > uint32_t seq;
> > > uint32_t enc_id;
> > > - enum dpu_rm_topology_name topology;
> > > };
> > >
> > > /**
> > > @@ -122,8 +119,8 @@ static void _dpu_rm_print_rsvps(
> > > DPU_DEBUG("%d\n", stage);
> > >
> > > list_for_each_entry(rsvp, &rm->rsvps, list) {
> > > - DRM_DEBUG_KMS("%d rsvp[s%ue%u] topology %d\n", stage,
> > rsvp->seq,
> > > - rsvp->enc_id, rsvp->topology);
> > > + DRM_DEBUG_KMS("%d rsvp[s%ue%u]\n", stage, rsvp->seq,
> > > + rsvp->enc_id);
> > > }
> > >
> > > for (type = 0; type < DPU_HW_BLK_MAX; type++) {
> > > @@ -146,18 +143,6 @@ struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm
> > *rm)
> > > return rm->hw_mdp;
> > > }
> > >
> > > -enum dpu_rm_topology_name
> > > -dpu_rm_get_topology_name(struct msm_display_topology topology)
> > > -{
> > > - int i;
> > > -
> > > - for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++)
> > > - if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], topology))
> > > - return g_top_table[i].top_name;
> > > -
> > > - return DPU_RM_TOPOLOGY_NONE;
> > > -}
> > > -
> > > void dpu_rm_init_hw_iter(
> > > struct dpu_rm_hw_iter *iter,
> > > uint32_t enc_id,
> > > @@ -760,7 +745,6 @@ static int _dpu_rm_make_next_rsvp(
> > > /* Create reservation info, tag reserved blocks with it as we go
> > */
> > > rsvp->seq = ++rm->rsvp_next_seq;
> > > rsvp->enc_id = enc->base.id;
> > > - rsvp->topology = reqs->topology->top_name;
> > > list_add_tail(&rsvp->list, &rm->rsvps);
> > >
> > > ret = _dpu_rm_reserve_lms(rm, rsvp, reqs);
> > > @@ -807,7 +791,7 @@ static int _dpu_rm_populate_requirements(
> > >
> > > dpu_encoder_get_hw_resources(enc, &reqs->hw_res, conn_state);
> > >
> > > - for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++) {
> > > + for (i = 0; i < ARRAY_SIZE(g_top_table); i++) {
> > > if (RM_IS_TOPOLOGY_MATCH(g_top_table[i],
> > > req_topology)) {
> > > reqs->topology = &g_top_table[i];
> >
> > Since you can get rid of g_top_table, you can get rid of reqs->topology
> > too.
> >
> DPU RM is using reqs->topology (ie. struct dpu_rm_topology_def) to represent
> the hw_block requirements for a display. Removing the structure
> might require changes in rest of RM alloc functions.
>
> If you can hold off on this, I am planning to address your comment in the
> RM refactor series.
I'd rather it be introduced when it's used. That way it can evalutated on its
own merits when we have the full picture. There's a bunch of unused or
duplicated code surrounding the RM, and I think it'd be best to clean it up
while we all have full context in our heads.
Sean
>
> Jeykumar S.
>
> > > @@ -831,9 +815,8 @@ static int _dpu_rm_populate_requirements(
> > >
> > > 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("num_lm: %d num_ctl: %d topology: %d split_display:
> > %d\n",
> > > + DRM_DEBUG_KMS("num_lm: %d num_ctl: %d split_display: %d\n",
> > > reqs->topology->num_lm, reqs->topology->num_ctl,
> > > - reqs->topology->top_name,
> > > reqs->topology->needs_split_display);
> > >
> > > return 0;
> > > @@ -969,8 +952,7 @@ static int _dpu_rm_commit_rsvp(
> > > }
> > >
> > > if (!ret)
> > > - DRM_DEBUG_KMS("rsrv enc %d topology %d\n", rsvp->enc_id,
> > > - rsvp->topology);
> > > + DRM_DEBUG_KMS("rsrv enc %d\n", rsvp->enc_id);
> >
> > This debug message doesn't provide any value any longer.
> >
> > >
> > > return ret;
> > > }
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > index ffd1841..6fd9e7b 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > @@ -21,21 +21,6 @@
> > > #include "dpu_hw_top.h"
> > >
> > > /**
> > > - * enum dpu_rm_topology_name - HW resource use case in use by
> > > connector
> > > - * @DPU_RM_TOPOLOGY_NONE: No topology in use
> > > currently
> > > - * @DPU_RM_TOPOLOGY_SINGLEPIPE: 1 LM, 1 PP, 1 INTF/WB
> > > - * @DPU_RM_TOPOLOGY_DUALPIPE: 2 LM, 2 PP, 2 INTF/WB
> > > - * @DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE: 2 LM, 2 PP, 3DMux, 1
> > > INTF/WB
> > > - */
> > > -enum dpu_rm_topology_name {
> > > - DPU_RM_TOPOLOGY_NONE = 0,
> > > - DPU_RM_TOPOLOGY_SINGLEPIPE,
> > > - DPU_RM_TOPOLOGY_DUALPIPE,
> > > - DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,
> > > - DPU_RM_TOPOLOGY_MAX,
> > > -};
> > > -
> > > -/**
> > > * enum dpu_rm_topology_control - HW resource use case in use by
> > connector
> > > * @DPU_RM_TOPCTL_RESERVE_LOCK: If set, in AtomicTest phase, after a
> > successful
> > > * test, reserve the resources for this
> > display.
> > > @@ -187,13 +172,4 @@ void dpu_rm_init_hw_iter(
> > > */
> > > int dpu_rm_check_property_topctl(uint64_t val);
> > >
> > > -/**
> > > - * dpu_rm_get_topology_name - returns the name of the the given
> > topology
> > > - * definition
> > > - * @topology: topology definition
> > > - * @Return: name of the topology
> > > - */
> > > -enum dpu_rm_topology_name
> > > -dpu_rm_get_topology_name(struct msm_display_topology topology);
> > > -
> > > #endif /* __DPU_RM_H__ */
> > > --
> > > 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
