Re: [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

[Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx>]

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 <jsanka@xxxxxxxxxxxxxx>
> > ---
> >  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 <seanpaul@xxxxxxxxxxxx>
>
>
> > +	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(&dpu_enc->enc_spinlock);
> >
> >  	atomic_set(&dpu_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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel