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

[Thierry Reding <treding@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 7521 bytes --]

On Mon, Sep 21, 2015 at 06:27:40PM +0800, Yakir Yang wrote:
> 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.   :)

Yes, that looks about right. You could also move the implementations
into the Exynos and Rockchip drivers, respectively, if they're only used
from one place. Then you can simply export the low-level analogix_dp_*()
functions. That might give you even more flexibility, but the above
would probably work well enough.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]