Re: [PATCH 4/4] drm/msm/mdp5: Make the intf connection in config module

[Archit Taneja <architt@xxxxxxxxxxxxxx>]

On 03/04/2015 09:14 PM, "Stéphane Viau" wrote:
> > Hi,
>
> Hi Archit,
>
> > On 03/04/2015 12:06 AM, Stephane Viau wrote:
> > > Up until now, we assume that eDP is tight to intf_0 and HDMI to
> > > intf_3. This information shall actually come from the mdp5_cfg
> > > module since it can change from one chip to another.
> > >
> > > Signed-off-by: Stephane Viau <sviau@xxxxxxxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.c |   8 +++
> > >    drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.h |   4 ++
> > >    drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 111
> > > ++++++++++++++++++--------------
> > >    3 files changed, 74 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.c
> > > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.c
> > > index 72c075a..8bee023 100644
> > > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.c
> > > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.c
> > > @@ -62,6 +62,10 @@ const struct mdp5_cfg_hw msm8x74_config = {
> > >    		.count = 4,
> > >    		.base = { 0x12500, 0x12700, 0x12900, 0x12b00 },
> > >    	},
> > > +	.intfs = {
> > > +		[0] = INTF_eDP,
> > > +		[3] = INTF_HDMI,
> > > +	},
> > >    	.max_clk = 200000000,
> > >    };
> > >
> > > @@ -111,6 +115,10 @@ const struct mdp5_cfg_hw apq8084_config = {
> > >    		.count = 5,
> > >    		.base = { 0x12500, 0x12700, 0x12900, 0x12b00, 0x12d00 },
> > >    	},
> > > +	.intfs = {
> > > +		[0] = INTF_eDP,
> > > +		[3] = INTF_HDMI,
> > > +	},
> > >    	.max_clk = 320000000,
> > >    };
> > >
> > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.h
> > > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.h
> > > index be149b3..4e91f14 100644
> > > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.h
> > > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_cfg.h
> > > @@ -58,6 +58,8 @@ struct mdp5_smp_block {
> > >    	int reserved[MAX_CLIENTS];	/* # of MMBs allocated per client */
> > >    };
> > >
> > > +#define MDP5_INTF_NUM_MAX	5
> > > +
> > >    struct mdp5_cfg_hw {
> > >    	char  *name;
> > >
> > > @@ -71,6 +73,8 @@ struct mdp5_cfg_hw {
> > >    	struct mdp5_sub_block ad;
> > >    	struct mdp5_sub_block intf;
> > >
> > > +	u32 intfs[MDP5_INTF_NUM_MAX]; /* array of enum mdp5_intf_type */
> > > +
> >
> > The array type could be "enum mdp5_intf_type" rather than u32.
>
> The problem here is that mdp5_cfg.h must be included before mdp5.xml.h
> (see mdp5_kms.h #24) so that mdp5_cfg pointer can be resolved in the
> latter.
> mdp5_cfg.h hence cannot use any types defined in mdp5.xml.h, including
> 'enum mdp5_intf_type'.

Ah okay.

> > >    	uint32_t max_clk;
> > >    };
> > >
> > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> > > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> > > index 390d9d2..8cec00e 100644
> > > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> > > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> > > @@ -161,6 +161,44 @@ int mdp5_enable(struct mdp5_kms *mdp5_kms)
> > >    	return 0;
> > >    }
> > >
> > > +static int construct_encoder(struct mdp5_kms *mdp5_kms,
> > > +		enum mdp5_intf_type intf_type, int intf_num)
> > > +{
> > > +	struct drm_device *dev = mdp5_kms->dev;
> > > +	struct msm_drm_private *priv = dev->dev_private;
> > > +	struct drm_encoder *encoder;
> > > +	struct mdp5_interface intf = {
> > > +			.num	= intf_num,
> > > +			.type	= intf_type,
> > > +			.mode	= MDP5_INTF_MODE_NONE,
> > > +	};
> > > +	int ret = 0;
> > > +
> > > +	encoder = mdp5_encoder_init(dev, &intf);
> > > +	if (IS_ERR(encoder)) {
> > > +		ret = PTR_ERR(encoder);
> > > +		dev_err(dev->dev, "failed to construct encoder: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
> > > +	priv->encoders[priv->num_encoders++] = encoder;
> > > +
> > > +	if (intf_type == INTF_HDMI) {
> > > +		ret = hdmi_modeset_init(priv->hdmi, dev, encoder);
> > > +		if (ret)
> > > +			dev_err(dev->dev, "failed to init HDMI: %d\n", ret);
> > > +
> > > +	} else if (intf_type == INTF_eDP) {
> > > +		/* Construct bridge/connector for eDP: */
> > > +		ret = msm_edp_modeset_init(priv->edp, dev, encoder);
> > > +		if (ret)
> > > +			dev_err(dev->dev, "failed to init eDP: %d\n", ret);
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >    static int modeset_init(struct mdp5_kms *mdp5_kms)
> > >    {
> > >    	static const enum mdp5_pipe crtcs[] = {
> > > @@ -171,7 +209,6 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
> > >    	};
> > >    	struct drm_device *dev = mdp5_kms->dev;
> > >    	struct msm_drm_private *priv = dev->dev_private;
> > > -	struct drm_encoder *encoder;
> > >    	const struct mdp5_cfg_hw *hw_cfg;
> > >    	int i, ret;
> > >
> > > @@ -222,56 +259,29 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
> > >    		}
> > >    	}
> > >
> > > -	if (priv->hdmi) {
> > > -		struct mdp5_interface intf = {
> > > -				.num	= 3,
> > > -				.type	= INTF_HDMI,
> > > -				.mode	= MDP5_INTF_MODE_NONE,
> > > -		};
> > > -
> > > -		/* Construct encoder for HDMI: */
> > > -		encoder = mdp5_encoder_init(dev, &intf);
> > > -		if (IS_ERR(encoder)) {
> > > -			dev_err(dev->dev, "failed to construct encoder\n");
> > > -			ret = PTR_ERR(encoder);
> > > -			goto fail;
> > > -		}
> > > -
> > > -		encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;;
> > > -		priv->encoders[priv->num_encoders++] = encoder;
> > > -
> > > -		ret = hdmi_modeset_init(priv->hdmi, dev, encoder);
> > > -		if (ret) {
> > > -			dev_err(dev->dev, "failed to initialize HDMI: %d\n", ret);
> > > -			goto fail;
> > > -		}
> > > -	}
> > > -
> > > -	if (priv->edp) {
> > > -		struct mdp5_interface intf = {
> > > -				.num	= 0,
> > > -				.type	= INTF_eDP,
> > > -				.mode	= MDP5_INTF_MODE_NONE,
> > > -		};
> > > -
> > > -		/* Construct encoder for eDP: */
> > > -		encoder = mdp5_encoder_init(dev, &intf);
> > > -		if (IS_ERR(encoder)) {
> > > -			dev_err(dev->dev, "failed to construct eDP encoder\n");
> > > -			ret = PTR_ERR(encoder);
> > > -			goto fail;
> > > +	/* Construct external display interfaces' encoders: */
> > > +	for (i = 0; i < ARRAY_SIZE(hw_cfg->intfs); i++) {
> > > +		enum mdp5_intf_type intf_type = hw_cfg->intfs[i];
> > > +
> > > +		switch (intf_type) {
> > > +		case INTF_DISABLED:
> > > +			break;
> > > +		case INTF_eDP:
> > > +			if (priv->edp)
> > > +				ret = construct_encoder(mdp5_kms, INTF_eDP, i);
> > > +			break;
> > > +		case INTF_HDMI:
> > > +			if (priv->hdmi)
> > > +				ret = construct_encoder(mdp5_kms, INTF_HDMI, i);
> > > +			break;
> > > +		default:
> > > +			dev_err(dev->dev, "unknown intf: %d\n", intf_type);
> > > +			ret = -EINVAL;
> > > +			break;
> > >    		}
> > >
> > > -		encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
> > > -		priv->encoders[priv->num_encoders++] = encoder;
> > > -
> > > -		/* Construct bridge/connector for eDP: */
> > > -		ret = msm_edp_modeset_init(priv->edp, dev, encoder);
> > > -		if (ret) {
> > > -			dev_err(dev->dev, "failed to initialize eDP: %d\n",
> > > -									ret);
> > > +		if (ret)
> > >    			goto fail;
> > > -		}
> > >    	}
> > >
> > >    	return 0;
> > > @@ -415,8 +425,11 @@ struct msm_kms *mdp5_kms_init(struct drm_device
> > > *dev)
> > >    	 * we don't disable):
> > >    	 */
> > >    	mdp5_enable(mdp5_kms);
> > > -	for (i = 0; i < config->hw->intf.count; i++)
> > > +	for (i = 0; i < MDP5_INTF_NUM_MAX; i++) {
> > > +		if (MDP5_INTF_IS_VIRTUAL_DISPLAY(config->hw->intfs[i]))
> > > +			continue;
> >
> > This may not be sufficient to prevent register writes to interfaces that
> > don't exist. We'd probably need:
> >
> > 	if(MDP5_INTF_IS_VIRTUAL_DISPLAY(config->hw->intfs[i]) ||
> > 			config->hw->intfs[i] == INTF_DISABLED)
> > 		continue;
>
> We actually need to disable all display interfaces even though they are
> not listed as "enabled". If we do not do so, we get faults[1].
> As you mentioned, it shouldn't harm to write 0 in non existent interfaces'
> registers.
>
> [1] http://www.hastebin.com/raw/dekaqaboju

Thanks for the clarification.

Archit

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html