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
