Re: [PATCH] drm/amd: DC pull request review

From: Harry Wentland <harry.wentland@amd.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH] drm/amd: DC pull request review
Date: Wed, 27 Sep 2017 09:47:26 -0400	[thread overview]
Message-ID: <ceb3a2b1-1a70-65ac-dfca-1367d27b44b4@amd.com> (raw)
In-Reply-To: <20170927101550.23176-1-daniel.vetter@ffwll.ch>

On 2017-09-27 06:15 AM, Daniel Vetter 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@ffwll.ch>
> 
> for merging this pull.
> 
> 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).
> 
> 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@intel.com>

Thanks for the feedback, ack and help along the way.

This patch is
Reviewed-by: Harry Wentland <harry.wentland@amd.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.
> 

Have there been any thoughts on improving log filtering in DRM? I think
our KFD guys (which hopefully go upstream soon as well) are using
dynamic debug prints for it but not sure if that's entirely the right
approach.

Harry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel