Re: [PATCH v4 0/16] Add Analogix Core Display Port Driver

[Yakir Yang <ykk@rock-chips.com>]

Hi Thierry,

Thanks for your suggest :)

On 09/21/2015 05:15 PM, Thierry Reding wrote:
> On Mon, Sep 21, 2015 at 04:45:44PM +0800, Yakir Yang wrote:
>> Hi Heiko,
>>
>> On 09/02/2015 10:15 AM, Yakir Yang wrote:
>>> Hi Heiko,
>>>
>>> 在 09/02/2015 05:47 AM, Heiko Stuebner 写道:
>>>> Hi Yakir,
>>>>
>>>> Am Dienstag, 1. September 2015, 13:46:11 schrieb Yakir Yang:
>>>>>     The Samsung Exynos eDP controller and Rockchip RK3288 eDP
>>>>> controller
>>>>> share the same IP, so a lot of parts can be re-used. I split the common
>>>>> code into bridge directory, then rk3288 and exynos only need to keep
>>>>> some platform code. Cause I can't find the exact IP name of exynos dp
>>>>> controller, so I decide to name dp core driver with "analogix" which I
>>>>> find in rk3288 eDP TRM ;)
>>>>>
>>>>> Beyond that, there are three light registers setting differents bewteen
>>>>> exynos and rk3288.
>>>>> 1. RK3288 have five special pll resigters which not indicata in exynos
>>>>>     dp controller.
>>>>> 2. The address of DP_PHY_PD(dp phy power manager register) are
>>>>> different
>>>>>     between rk3288 and exynos.
>>>>> 3. Rk3288 and exynos have different setting with AUX_HW_RETRY_CTL(dp
>>>>> debug
>>>>>     register).
>>>>>
>>>>> I have verified this series on two kinds of rockchip platform board,
>>>>> one
>>>>> is rk3288 sdk board which connect with a 2K display port monitor, the
>>>>> other
>>>>> is google jerry chromebook which connect with a eDP screen
>>>>> "cnm,n116bgeea2",
>>>>> both of them works rightlly.
>>>> it looks like during the rebase something did go wrong and I found some
>>>> issues
>>>> I mentioned in the replies to individual patches.
>>>>
>>>> I did prepare a branch based on mainline [0] with both the old and the
>>>> new edp
>>>> driver - rk3288_veyron_defconfig build both drivers into the image.
>>>>
>>>> While the old driver still works, I wasn't able to make the new one work
>>>> yet
>>>> ... the drm core does find the connector, but not that anything is
>>>> connected
>>>> to it. I'll try to dig deeper tomorrow, but maybe you'll see anything
>>>> interesting before then.
>>> Many thanks for your comment and debug, I would rebase on your
>>> "edp-with-veyron" branch and fix the broken, make sure v6 would
>>> work rightly at least in your side and my side.
>> Just like we talk off line, I guess there are two tricky questions which
>> make analogix_dp just crash/failed on rockchip platform:
>>
>> -  One is how to reach a agreement with the common way to register
>> connector. There would be a conflict with Exynos & IMX & Rockchip.
>>       On analogix_dp thread, Exynos want to register connector when that
>> connector is ready.
>>       On dw_hdmi thread, IMX want to register connector when all component is
>> already.
>>       So Exynos & IMX & Rockchip should reach a common way to register
>> connector to fix this issue.
>>
>> -  The other is atomic API.
>>        The rockchip drm haven't implemented the atomic API, but the original
>> exynos_dp have used the atomic API on connector helper function. That's why
>> analogix_dp just keep crash on your side.
> There's really no reason not to convert Rockchip to atomic. It will have
> to happen eventually anyway.

Do agree on this point, and I see Tomasz Figa have done some WIP
works on implementing the atomic_commit, maybe would upstream
in further.(https://chromium-review.googlesource.com/#/c/284560/1)


> That said, there's another option that would allow you to side-step both
> of the above problems at the same time. If you turn the common code into
> a helper library that should give you enough flexibility to integrate it
> into all existing users. For example you could leave out the connector
> registration and let the drivers do that. Similarly since the helpers
> are only hooked up at registration time you could probably find a way to
> share the low-level code but again leave it up to the drivers to glue it
> all together at registration time (drivers could wrap the low-level code
> with atomic or non-atomic callbacks).

Wow, sounds good, but I'm not sure I understand this rightly. Do you
mean that I could support two kinds of callbacks in analogix_dp_core
driver, and export them out. And move the connector registration code
into the helper driver (like exynos_dp.c), so helper driver could chose to
use the atomic or non-atomic callbacks. like:

-- analogix_dp_core.c 
--------------------------------------------------------------------
...
struct drm_connector_funcs analogix_dp_connector_atomic_funcs = {
         .dpms = drm_atomic_helper_connector_dpms,
         .fill_modes = drm_helper_probe_single_connector_modes,
         .detect = analogix_dp_detect,
         .destroy = analogix_dp_connector_destroy,
         .reset = drm_atomic_helper_connector_reset,
         .atomic_duplicate_state = 
drm_atomic_helper_connector_duplicate_state,
         .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};
EXPORT_SYMBOL(analogix_dp_connector_atomic_funcs);

struct drm_connector_funcs analogix_dp_connector_funcs = {
         .dpms = drm_helper_connector_dpms,
         .fill_modes = drm_helper_probe_single_connector_modes,
         .detect = analogix_dp_detect,
         .destroy = analogix_dp_connector_destroy,
};
EXPORT_SYMBOL(analogix_dp_connector_funcs);

struct drm_connector_helper_funcs analogix_dp_connector_helper_funcs = {
         .get_modes = analogix_dp_get_modes,
         .best_encoder = analogix_dp_best_encoder,
};
EXPORT_SYMBOL(analogix_dp_connector_helper_funcs);
...

-- exynos_dp 
----------------------------------------------------------------------------
         ret = drm_connector_init(dp->drm_dev, connector,
&analogix_dp_connector_atomic_funcs,
                                  DRM_MODE_CONNECTOR_eDP);
         if (ret) {
                 DRM_ERROR("Failed to initialize connector with drm\n");
                 return ret;
         }

         drm_connector_helper_add(connector, 
&analogix_dp_connector_helper_funcs);
         drm_connector_register(connector);
         drm_mode_connector_attach_encoder(connector, encoder);

-- analogix_dp-rockchip 
----------------------------------------------------------------------------
         ret = drm_connector_init(dp->drm_dev, connector,
                                  &analogix_dp_connector_funcs,
                                  DRM_MODE_CONNECTOR_eDP);
         if (ret) {
                 DRM_ERROR("Failed to initialize connector with drm\n");
                 return ret;
         }

         drm_connector_helper_add(connector, 
&analogix_dp_connector_helper_funcs);
         drm_mode_connector_attach_encoder(connector, encoder);


Are those code corresponding to your suggestion.   :)

Thanks
- Yakir

> This option may also have the benefit of loosening the coupling between
> DRM drivers and the helper code for this IP, which may be handy in case
> the drivers diverge again in the future, or ease transitions to new API.
>
> Thierry