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

From: "Stéphane Viau"
Date: Wed Mar 04 2015 - 10:45:19 EST

> 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'.

>> 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

>> mdp5_write(mdp5_kms, REG_MDP5_INTF_TIMING_ENGINE_EN(i), 0);
>> + }
>
> Writing to registers for a non existent interface shouldn't do any harm,
> but it's best to be safe :)
>
>> mdp5_disable(mdp5_kms);
>> mdelay(16);
>>
>>
>
> Thanks,
> 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
>

Thanks,
Stéphane.

--
Qualcomm Innovation Center, Inc.

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/