* Re: [[RFC]DPU PATCH] drm/msm/dsi: Use one connector for dual DSI mode
2018-03-02 0:27 ` [[RFC]DPU PATCH] drm/msm/dsi: Use one connector for dual DSI mode chandanu-sgV2jX0FEOL9JmXXK+q4OQ
@ 2018-03-08 5:50 ` Archit Taneja
0 siblings, 0 replies; 2+ messages in thread
From: Archit Taneja @ 2018-03-08 5:50 UTC (permalink / raw)
To: chandanu, Sean Paul
Cc: linux-arm-msm, abhinavk, hoegsberg, dri-devel, freedreno
On Friday 02 March 2018 05:57 AM, chandanu@codeaurora.org wrote:
> On 2018-03-01 07:53, Sean Paul wrote:
>> On Wed, Feb 28, 2018 at 04:44:49PM -0800, Chandan Uddaraju wrote:
>>> Current DSI driver uses two connectors for dual DSI case even
>>> though we only have one panel. Fix this by implementing one
>>> connector/bridge for dual DSI use case.
>>>
>>> Current patch is not yet tested on dual-dsi setup.
>>
>> This seems like something that might benefit from being part of a
>> patch series
>> that can be tested end-to-end (ie: a patch series to add full dual-dsi
>> support
>> to the dsi driver). It's kind of hard to see where things are going
>> with just
>> this one small piece.
>>
>
> This is more like a initial patch to get early comments/suggestions
> regarding this approach on the DSI upstream driver.
> We are working on enabling dual-dsi using DPU with upstream dsi driver.
> This change will be based on archit's dual DSI changes on SDM845.
> Once we have verified, we will post all the relevant patches along with
> this one.
The approach of entirely bypassing msm_dsi_modeset_init() for DSI1
sounds like a decent idea. However, this may not work well if we need to
dynamically switch between dual DSI mode vs operating the 2 DSIs
independently. If we don't have this use case in mind, then we should
be okay.
Another approach could be to create 2 separate bridge/connectors for
irrespective of whether we are in dual DSI mode or not, but report one
of the pairs as unavailable (connector's detect() should report
disconnected).
>
>>>
>>> Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
>>> ---
>>> drivers/gpu/drm/msm/dsi/dsi.c | 3 +
>>> drivers/gpu/drm/msm/dsi/dsi.h | 1 +
>>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 100
>>> +++++++---------------------------
>>> 3 files changed, 24 insertions(+), 80 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c
>>> b/drivers/gpu/drm/msm/dsi/dsi.c
>>> index b744bcc..ff8164c 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>>> @@ -208,6 +208,9 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi,
>>> struct drm_device *dev,
>>> goto fail;
>>> }
>>>
>>> + if (!msm_dsi_manager_validate_current_config(msm_dsi->id))
>>> + goto fail;
>>> +
>>> msm_dsi->encoder = encoder;
>>>
>>> msm_dsi->bridge = msm_dsi_manager_bridge_init(msm_dsi->id);
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h
>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>> index 70d9a9a..2d9763f 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>> @@ -100,6 +100,7 @@ struct msm_dsi {
>>> void msm_dsi_manager_attach_dsi_device(int id, u32 device_flags);
>>> int msm_dsi_manager_register(struct msm_dsi *msm_dsi);
>>> void msm_dsi_manager_unregister(struct msm_dsi *msm_dsi);
>>> +bool msm_dsi_manager_validate_current_config(u8 id);
>>>
>>> /* msm dsi */
>>> static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi)
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> index 4cb1cb6..bf92f25 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> @@ -305,67 +305,6 @@ static void dsi_mgr_connector_destroy(struct
>>> drm_connector *connector)
>>> kfree(dsi_connector);
>>> }
>>>
>>> -static void dsi_dual_connector_fix_modes(struct drm_connector
>>> *connector)
>>> -{
>>> - struct drm_display_mode *mode, *m;
>>> -
>>> - /* Only support left-right mode */
>>> - list_for_each_entry_safe(mode, m, &connector->probed_modes, head) {
>>> - mode->clock >>= 1;
>>> - mode->hdisplay >>= 1;
>>> - mode->hsync_start >>= 1;
>>> - mode->hsync_end >>= 1;
>>> - mode->htotal >>= 1;
>>> - drm_mode_set_name(mode);
>>> - }
>>> -}
>>> -
>>> -static int dsi_dual_connector_tile_init(
>>> - struct drm_connector *connector, int id)
>>> -{
>>> - struct drm_display_mode *mode;
>>> - /* Fake topology id */
>>> - char topo_id[8] = {'M', 'S', 'M', 'D', 'U', 'D', 'S', 'I'};
>>> -
>>> - if (connector->tile_group) {
>>> - DBG("Tile property has been initialized");
>>> - return 0;
>>> - }
>>> -
>>> - /* Use the first mode only for now */
>>> - mode = list_first_entry(&connector->probed_modes,
>>> - struct drm_display_mode,
>>> - head);
>>> - if (!mode)
>>> - return -EINVAL;
>>> -
>>> - connector->tile_group = drm_mode_get_tile_group(
>>> - connector->dev, topo_id);
>>> - if (!connector->tile_group)
>>> - connector->tile_group = drm_mode_create_tile_group(
>>> - connector->dev, topo_id);
>>> - if (!connector->tile_group) {
>>> - pr_err("%s: failed to create tile group\n", __func__);
>>> - return -ENOMEM;
>>> - }
>>> -
>>> - connector->has_tile = true;
>>> - connector->tile_is_single_monitor = true;
>>> -
>>> - /* mode has been fixed */
>>> - connector->tile_h_size = mode->hdisplay;
>>> - connector->tile_v_size = mode->vdisplay;
>>> -
>>> - /* Only support left-right mode */
>>> - connector->num_h_tile = 2;
>>> - connector->num_v_tile = 1;
>>> -
>>> - connector->tile_v_loc = 0;
>>> - connector->tile_h_loc = (id == DSI_RIGHT) ? 1 : 0;
>>> -
>>> - return 0;
>>> -}
>>> -
>>> static int dsi_mgr_connector_get_modes(struct drm_connector *connector)
>>> {
>>> int id = dsi_mgr_connector_get_id(connector);
>>> @@ -376,31 +315,15 @@ static int dsi_mgr_connector_get_modes(struct
>>> drm_connector *connector)
>>> if (!panel)
>>> return 0;
>>>
>>> - /* Since we have 2 connectors, but only 1 drm_panel in dual DSI
>>> mode,
>>> - * panel should not attach to any connector.
>>> - * Only temporarily attach panel to the current connector here,
>>> - * to let panel set mode to this connector.
>>> + /*
>>> + * In dual DSI mode, we have one connector that can be
>>> + * attached to the drm_panel.
>>> */
>>> drm_panel_attach(panel, connector);
>>> num = drm_panel_get_modes(panel);
>>> - drm_panel_detach(panel);
>>> if (!num)
>>> return 0;
>>>
>>> - if (IS_DUAL_DSI()) {
>>> - /* report half resolution to user */
>>> - dsi_dual_connector_fix_modes(connector);
>>> - ret = dsi_dual_connector_tile_init(connector, id);
>>> - if (ret)
>>> - return ret;
>>> - ret = drm_mode_connector_set_tile_property(connector);
>>> - if (ret) {
>>> - pr_err("%s: set tile property failed, %d\n",
>>> - __func__, ret);
>>> - return ret;
>>> - }
>>> - }
>>> -
>>> return num;
>>> }
>>>
>>> @@ -689,6 +612,23 @@ struct drm_connector
>>> *msm_dsi_manager_connector_init(u8 id)
>>> return connector;
>>> }
>>>
>>> +bool msm_dsi_manager_validate_current_config(u8 id)
>>> +{
>>> + bool is_dual_dsi = IS_DUAL_DSI();
>>> +
>>> + /*
>>> + * For dual DSI, we only have one drm panel. For this
>>> + * use case, we register only one bridge/connector.
>>> + * Skip bridge/connector initialisation if it is
>>> + * DSI 1 in case of dual DSI.
>>
>> I think the other dual-dsi implementations take the master/slave cues
>> from
>> device tree as opposed to hard-coding one or the other (I know of at
>> least one
>> case where DSI_1 was connected to the panel's first channel, whereas
>> DSI_0 was
>> connected to the second channel.
>>
>> Sean
>
> You are right Sean. I need to add logic to check Master DSI controller
> instead
> of DSI ids so that the implementation become more generic. Thank you for
> sharing
> your thoughts.
Slightly unrelated: currently, the IS_DUAL_DSI() uses custom qcom
bindings, we should use more generic bindings. I'd posted a RFC for it:
https://lists.freedesktop.org/archives/dri-devel/2018-January/162913.html
Thanks,
Archit
>
> Chandan
>>
>>> + */
>>> + if (is_dual_dsi && (DSI_1 == id)) {
>>> + DBG("Skip DSI_1 bridge registration for dual DSI.\n");
>>> + return false;
>>> + }
>>> + return true;
>>> +}
>>> +
>>> /* initialize bridge */
>>> struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
>>> {
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum,
>>> a Linux Foundation Collaborative Project
>>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 2+ messages in thread