[PATCH] drm/amd: DC pull request review

[Sean Paul]

On Wed, Sep 27, 2017 at 6:15 AM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> Ok, here's one more attempt at scrolling through 130k diff.
>
> Overall verdict from me is that DC is big project, and like any big
> project it's never done. So at least for me the goal isn't to make
> things perfect, becaue if that's the hoop to jump through we wouldn't
> have any gpu drivers at all. More important is whether merging a new
> driver base will benefit the overall subsystem, and here this
> primarily means whether the DC team understands how upstream works and
> is designed, and whether the code is largely aligned with upstream
> (especially the atomic modeset) architecture.
>
> Looking back over the last two years I think that's the case now, so
>
> Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> for merging this pull.

I'll echo the above. Perfect is a hard target to hit, even more so if
the target is moving.

AFAICS, the biggest issue DC faces is not a quality issue, but rather
a volume issue. The driver's origin being from Windows means that they
have a lot of things built from first principles instead of leveraging
existing infrastructure. While this is a real issue, I've seen (and
worked on) drm drivers in much worse shape than this.

Given the amount of effort AMD have already put into upstreaming and
community involvement, I have no doubt they'll continue to trim things
down after pull. I hope the community will also kick in to help them
out.

So,

Acked-by: Sean Paul <seanpaul at chromium.org>


>
> While scrolling through the pull I spotted a bunch more things that
> should be refactored, but most of these will be a real pain with DC
> is out of tree, and much easier in tree since in many of these areas
> the in-tree helpers aren't up to snuff yet for what DC needs. That
> kind of work is best done when there's one tree with everything
> integrated.
>
> That's also why I think we should merge DC into drm-next directly, so
> we can get started on the integration polish right away. That has a
> bit higher risk of Linus having a spazz, so here's my recommendation
> for merging:
>
> - There's a few additions to drm_dp_helper.h sprinkled all over the
>   pull. I think those should be put into a patch of it's own, and
>   merged first. No need to rebase DC, git merge will dtrt and not end
>   up with duplicates.
>
> - dm_alloc/realloc/free is something Dave Airlie noticed, and I agree
>   it's an easy red flag that might upset Linus. cocci can fix this
>   easy, so no real problem I think to patch up in one big patch (I
>   thought we've had a "remove malloc wrappers" todo item in the very
>   first review, apparently there was more than one such wrapper).
>
> - The history is huge, but AMD folks want to keep it if possible, and
>   I see the value in that. Would be good to get an ack from Linus for
>   that (but shouldn't be an issue, not the first time we've merged the
>   full history of out-of-tree work).

Any chance we can address the i2c/gpio [re-]implementations as well?

Sean


>
> Short&longer term TODO items are still tracked, might be a good idea
> to integrate those the overall drm todo in our gpu documentation, for
> more visibility.
>
> So in a way this is kinda like staging, except not with the horribly
> broken process of having an entirely separate tree for staging drivers
> which just makes refactoring needlessly painful (which defeats the
> point of staging really). So staging-within-the-subsystem. We've had
> that before, with early nouveau.
>
> And yes some of the files are utterly horrible to read and not
> anything close to kernel coding style standards. But that's the point,
> they're essentially gospel from hw engineers that happens to be
> parseable by gcc.
>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/gpu/drm/amd/display/TODO | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/TODO b/drivers/gpu/drm/amd/display/TODO
> index 2737873db12d..eea645b102a1 100644
> --- a/drivers/gpu/drm/amd/display/TODO
> +++ b/drivers/gpu/drm/amd/display/TODO
> @@ -79,3 +79,34 @@ TODOs
>
>  12. drm_modeset_lock in MST should no longer be needed in recent kernels
>      * Adopt appropriate locking scheme
> +
> +13. get_modes and best_encoder callbacks look a bit funny. Can probably rip out
> +a few indirections, and consider removing entirely and using the
> +drm_atomic_helper_best_encoder default behaviour.
> +
> +14. core/dc_debug.c, consider switching to the atomic state debug helpers and
> +moving all your driver state printing into the various atomic_print_state
> +callbacks. There's also plans to expose this stuff in a standard way across all
> +drivers, to make debugging userspace compositors easier across different hw.
> +
> +15. Move DP/HDMI dual mode adaptors to drm_dp_dual_mode_helper.c.
> +
> +16. Move to core SCDC helpers (I think those are new since initial DC review).
> +
> +17. There's still a pretty massive layer cake around dp aux and DPCD handling,
> +with like 3 levels of abstraction and using your own structures instead of the
> +stuff in drm_dp_helper.h. drm_dp_helper.h isn't really great and already has 2
> +incompatible styles, just means more reasons not to add a third (or well third
> +one gets to do the cleanup refactor).
> +
> +18. There's a pile of sink handling code, both for DP and HDMI where I didn't
> +immediately recognize the standard. I think long term it'd be best for the drm
> +subsystem if we try to move as much of that into helpers/core as possible, and
> +share it with drivers. But that's a very long term goal, and by far not just an
> +issue with DC - other drivers, especially around DP sink handling, are equally
> +guilty.
> +
> +19. The DC logger is still a rather sore thing, but I know that the DRM_DEBUG
> +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.
> --
> 2.14.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel