Re: [PATCH 3/3] drm/amd/display: DC I2C review

[Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>]

On Wed, Sep 27, 2017 at 9:46 PM, Harry Wentland <harry.wentland@amd.com> wrote:
> While reviewing I2C in DC identified a few places. Added a couple to the
> TODO list.
>
> 1) Connector info read
>
> See get_ext_display_connection_info
>
> On some boards the connector information has to be read through a
> special I2C channel. This line is only used for this purpose and only on
> driver init.
>
> 2) SCDC stuff
>
> This should all be reworked to go through DRM's SCDC code. When this is
> done some unnecessary I2C code can be retired as well.
>
> 3) Max TMDS clock read
>
> See dal_ddc_service_i2c_query_dp_dual_mode_adaptor
>
> This should happen in DRM as well. I haven't checked if there's
> currently functionality in DRM. If not we can propose something.
>
> 4) HDMI retimer programming
>
> Some boards have an HDMI retimer that we need to program to pass PHY
> compliance.
>
> 1 & 3 might be a good exercise if someone is looking for things to do.
>
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>  drivers/gpu/drm/amd/display/TODO | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/TODO b/drivers/gpu/drm/amd/display/TODO
> index eea645b102a1..981352bc95f0 100644
> --- a/drivers/gpu/drm/amd/display/TODO
> +++ b/drivers/gpu/drm/amd/display/TODO
> @@ -62,20 +62,10 @@ TODOs
>      ~ Daniel Vetter
>
>
> -11. Remove existing i2c implementation from DC
> -
> -    "Similar story for i2c, it uses the kernel's i2c code now, but there's
> -    still a full i2c implementation hidden beneath that in
> -    display/dc/i2caux. Kinda not cool, but imo ok if you fix that
> -    post-merging (perhaps by not including any of this in the linux DC
> -    code in the upstream kernel, but as an aux module in your internal
> -    codebase since there you probably need that, same applies to the edid
> -    parsing DC still does. For both cases I assume that the minimal shim
> -    you need on linux (bit banging and edid parsing isn't rocket since) is
> -    a lot less than the glue code to interface with the dc-provided
> -    abstraction."
> -    ~ Daniel Vetter
> -
> +11. Remove dc/i2caux. This folder can be somewhat misleading. It's basically an
> +overy complicated HW programming function for sendind and receiving i2c/aux
> +commands. We can greatly simplify that and move it into dc/dceXYZ like other
> +HW blocks.

Best case I think would be if you directly implement the i2c_adapter
in there. It's a tiny abstraction/api, so should be trivial to
reimplement for the windows side. Or at least align really closely.
Even more so for the gpio bit-banging case, that should use the linux
implementation I think. Might be good to clarify.

Anyway, ack on this.

>  12. drm_modeset_lock in MST should no longer be needed in recent kernels
>      * Adopt appropriate locking scheme
> @@ -110,3 +100,11 @@ guilty.
>  stuff just isn't up to the challenges either. We need to figure out something
>  that integrates better with DRM and linux debug printing, while not being
>  useless with filtering output. dynamic debug printing might be an option.
> +
> +20. Move Max TMDS clock read to DRM. See
> +dal_ddc_service_i2c_query_dp_dual_mode_adaptor. I haven't checked if there's
> +currently functionality in DRM. If not we can propose something.

We already have dual_mode helpers. It's one of the todo's I've added,
merged this with point 15?

> +21. Use kernel i2c device to program HDMI retimer. Some boards have an HDMI
> +retimer that we need to program to pass PHY compliance. Currently that's
> +bypassing the i2c device and goes directly to HW. This should be changed.

I thought it eventually goes through the i2c stuff, after a few layers
at least. Maybe I got derailed. Anyway, makes sense.

With 20 merged into 15, ack on the patch from me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx