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 at codeaurora.org>
>>> ---
>>> 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
