Re: [PATCH 3/4] drm/msm: Initial add DSI connector support

[hali@codeaurora.org]

Hi Archit,

> Hi Hai,
>
> On 03/19/2015 02:35 AM, hali@codeaurora.org wrote:
>> Hi Archit,
>>
>> Thanks for your comments. Please see my response for some comments
>> below.
>> Comments without response will be addressed in patch version 2. I will
>> wait for other comments if any to push patch V2.
>>
>>>> +static int dsi_gpio_init(struct msm_dsi_host *msm_host)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	msm_host->disp_en_gpio = devm_gpiod_get(&msm_host->pdev->dev,
>>>> +						"disp-enable");
>>>> +	if (IS_ERR(msm_host->disp_en_gpio)) {
>>>> +		pr_warn("%s: cannot get disp-enable-gpios %ld\n",
>>>> +			__func__, PTR_ERR(msm_host->disp_en_gpio));
>>>> +		msm_host->disp_en_gpio = NULL;
>>>> +	}
>>>> +	if (msm_host->disp_en_gpio) {
>>>> +		ret = gpiod_direction_output(msm_host->disp_en_gpio, 0);
>>>> +		if (ret) {
>>>> +			pr_err("cannot set dir to disp-en-gpios %d\n", ret);
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	msm_host->te_gpio = devm_gpiod_get(&msm_host->pdev->dev, "disp-te");
>>>> +	if (IS_ERR(msm_host->te_gpio)) {
>>>> +		pr_warn("%s: cannot get disp-te-gpios %ld\n",
>>>> +			__func__, PTR_ERR(msm_host->te_gpio));
>>>
>>> Video mode panels won't have te_gpio, we could probably make this
>>> pr_debug()
>>>
>>>> +		msm_host->te_gpio = NULL;
>>>> +	}
>>>> +
>>>> +	if (msm_host->te_gpio) {
>>>> +		ret = gpiod_direction_input(msm_host->te_gpio);
>>>> +		if (ret) {
>>>> +			pr_err("%s: cannot set dir to disp-te-gpios, %d\n",
>>>> +				__func__, ret);
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>
>>> These gpios currently need to be declared under the dsi DT node. Even
>>> if
>>> these are controlled via the dsi host, the gpios should still come
>>> under
>>> the panel DT node.
>>>
>>> We shout get the panel's of_node here and look for these.
>>>
>>
>>>> +static void dsi_sw_reset(struct msm_dsi_host *msm_host)
>>>> +{
>>>> +	dsi_write(msm_host, REG_DSI_CLK_CTRL,
>>>> +		DSI_CLK_CTRL_AHBS_HCLK_ON | DSI_CLK_CTRL_AHBM_SCLK_ON |
>>>> +		DSI_CLK_CTRL_PCLK_ON | DSI_CLK_CTRL_DSICLK_ON |
>>>> +		DSI_CLK_CTRL_BYTECLK_ON | DSI_CLK_CTRL_ESCCLK_ON |
>>>> +		DSI_CLK_CTRL_FORCE_ON_DYN_AHBM_HCLK);
>>>
>>> The same 7 bits seem to be set elsewhere, maybe make this a macro
>>> DSI_ENABLE_CLKS or something similar?
>>>
>>
>>>> +int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>>>> +{
>>>> +	struct msm_dsi_host *msm_host = NULL;
>>>> +	struct platform_device *pdev = msm_dsi->pdev;
>>>> +	int ret = 0;
>>>> +
>>>> +	msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
>>>> +	if (!msm_host) {
>>>> +		pr_err("%s: FAILED: cannot alloc dsi host\n",
>>>> +		       __func__);
>>>> +		ret = -ENOMEM;
>>>> +		goto fail;
>>>> +	}
>>>> +
>>>> +	ret = of_property_read_u32(pdev->dev.of_node,
>>>> +				"cell-index", &msm_host->id);
>>>
>>> retrieving the instance number of a peripheral via a DT property like
>>> 'cell-index' has been debated quite a bit in the past. I suppose it's
>>> not the best thing to do.
>>>
>>> However, since the DSI instances in MDSS aren't completely
>>> identical(one
>>> acts a master and other slave in dual dsi mode), maybe we can get away
>>> with having a property like "qcom,dsi-master;", and that can be further
>>> used to identify whether this node is DSI_0 or DSI_1
>>>
>>
>> 2 DSIs are not always master-slave mode. It is possible that a single
>> panel is connected to any of the hosts, or 2 panels are controlled
>> independently. If 'cell-index' is not allowed to specify the instance
>> number, i would prefer to have a simple property like
>> "qcom,dsi-host-index".
>
> Okay, thanks for that clarification.
>
>>
>>>> +int msm_dsi_host_register(struct mipi_dsi_host *host, bool
>>>> check_defer)
>>>> +{
>>>> +	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>>> +	struct device_node *node;
>>>> +	int ret;
>>>> +
>>>> +	/* Register mipi dsi host */
>>>> +	if (!msm_host->registered) {
>>>> +		host->dev = &msm_host->pdev->dev;
>>>> +		host->ops = &dsi_host_ops;
>>>> +		ret = mipi_dsi_host_register(host);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		msm_host->registered = true;
>>>> +
>>>> +		/* If the panel driver has not been probed after host register,
>>>> +		 * we should defer the host's probe.
>>>> +		 * It makes sure panel is connected when fbcon detects
>>>> +		 * connector status and gets the proper display mode to
>>>> +		 * create framebuffer.
>>>> +		 */
>>>> +		if (check_defer) {
>>>> +			node = of_parse_phandle(msm_host->pdev->dev.of_node,
>>>> +						"qcom,panel", 0);
>>>> +			if (node) {
>>>> +				if (!of_drm_find_panel(node))
>>>> +					return -EPROBE_DEFER;
>>>> +			}
>>>> +		}
>>>> +	}
>>>
>>> We might have to defer probe multiple times before another dependency
>>> is
>>> met. The above approach will let the driver defer only once without the
>>> panel driver registered. During the second probe attempt, we'd just
>>> return since 'msm_host->registered' would be true.
>>>
>>> I think we could move this check to end of dsi_init().
>>>
>>
>> Once probe deferred, the private data structures will be cleaned up and
>> re-allocated at the next probe. It should support multiple times probe
>> deferral.
>
> Ah right! I forgot about that. However, I don't think the panel would be
> a phandle entry under the dsi device node, right? Also, "qcom,panel"
> seems more like a compatible string to me. Should it rather be:
>
> node = of_get_child_by_name(msm_host->pdev->dev.of_node, "panel");
> if (node)
> 	...
> 	...
>

I was referring to what's done in other drm drivers, but i agree
of_get_child_by_name is a better way to avoid adding another entry in host
node. Thanks!
>>
>>>> +static int dsi_mgr_connector_mode_valid(struct drm_connector
>>>> *connector,
>>>> +				struct drm_display_mode *mode)
>>>> +{
>>>> +	int id = dsi_mgr_connector_get_id(connector);
>>>> +	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>> +	struct drm_encoder *encoder =
>>>> +		(msm_dsi->panel_flags & MIPI_DSI_MODE_VIDEO) ?
>>>> +		msm_dsi->base.encoders[MSM_DSI_VIDEO_ENCODER_ID] :
>>>> +		msm_dsi->base.encoders[MSM_DSI_CMD_ENCODER_ID];
>>>
>>> Maybe you could make a helper 'msm_dsi_get_encoder(msm_dsi)' out of
>>> this? It seems to be used elsewhere too.
>>>
>>
>>>> +int msm_dsi_manager_register(struct msm_dsi *msm_dsi)
>>>> +{
>>>> +	struct msm_dsi_manager *msm_dsim = &msm_dsim_glb;
>>>> +	int id = msm_dsi->id;
>>>> +	struct msm_dsi *other_dsi = dsi_mgr_get_other_dsi(id);
>>>> +	int ret;
>>>> +
>>>> +	if (id > DSI_MAX) {
>>>> +		pr_err("%s: invalid id %d\n", __func__, id);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if (msm_dsim->dsi[id]) {
>>>> +		pr_err("%s: dsi%d already registered\n", __func__, id);
>>>> +		return -EBUSY;
>>>> +	}
>>>> +
>>>> +	msm_dsim->dsi[id] = msm_dsi;
>>>> +
>>>> +	ret = dsi_mgr_parse_dual_panel(msm_dsi->pdev->dev.of_node, id);
>>>> +	if (ret) {
>>>> +		pr_err("%s: failed to parse dual panel info\n", __func__);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	if (!IS_DUAL_PANEL()) {
>>>> +		ret = msm_dsi_host_register(msm_dsi->host, true);
>>>> +	} else if (!other_dsi) {
>>>> +		return 0;
>>>> +	} else {
>>>> +		struct msm_dsi *mdsi = IS_MASTER_PANEL(id) ?
>>>> +					msm_dsi : other_dsi;
>>>> +		struct msm_dsi *sdsi = IS_MASTER_PANEL(id) ?
>>>> +					other_dsi : msm_dsi;
>>>> +		/* Register slave host first, so that slave DSI device
>>>> +		 * has a chance to probe, and do not block the master
>>>> +		 * DSI device's probe.
>>>> +		 * Also, do not check defer for the slave host,
>>>> +		 * because only master DSI device adds the panel to global
>>>> +		 * panel list. The panel's device is the master DSI device.
>>>> +		 */
>>>> +		ret = msm_dsi_host_register(sdsi->host, false);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +		ret = msm_dsi_host_register(mdsi->host, true);
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>> The dual panel checks in these functions are quite intrusive at the
>>> moment. We'd use DSI later where there won't be a panel at all. That
>>> would result in ever more complicated checks.
>>>
>>> Is it possible to separate out the dual panel functionality such that
>>> it
>>> becomes cleaner?
>>>
>>> For example msm_dsi_manager_phy_disable can look like:
>>>
>>> void msm_dsi_manager_phy_disable(int id)
>>> {
>>> 	...
>>> 	...
>>>
>>> 	if (msm_dual_dsi_mode())
>>> 		msm_dual_dsi_phy_disable(id);
>>> 	else
>>> 		msm_dsi_phy_disable(phy);
>>>
>>> 	...
>>> }
>>>
>>> There might be repetition of some code between the normal and dual mode
>>> case, but it will make things quite legible.
>>>
>>
>> I think even we separate out the dual DSI functionality, we still need
>> to
>> check dual DSI mode like the code above. The purpose of dsi_manager
>> module
>> is to centralize dual DSI handler, so there has to be many checks.
>>
>> The current code should work with different cases,
>> single-host-single-panel, dual-host-single-panel, dual-host-dual-panel
>> and
>> dual-host-independent-two-panels. If there is no panel for the host, we
>> will report disconnected connector. If we want to convert to other
>> interface type, i would prefer to have a fake dsi panel driver, to
>> follow
>> the current framework.
>>
>
> I think it will be a bit hard to incorporate fake panel support. I liked
> what's done in exynos/exynos_dp_core.c:
>
> The driver figures out if the device node supports a panel connected via
> a bridge, or a DP connector. Based on this info, it registers either a
> bridge driver, or a usual DP connector.
>
> I think it is fine to get this pulled as is. We can think later about
> whether the above or a fake panel approach would make more sense.
>

The problem of using bridge is that bridge has been mapped to DSI host,
like HDMI and eDP drivers, to set_mode/enable/disable to the host, as the
encoder is mapped to INTF in MDP.
Yes, we can think it later.

> Thanks,
> Archit
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Thanks,
Hai