From patchwork Wed Sep 27 10:15:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 9973671 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 75CBE6037F for ; Wed, 27 Sep 2017 10:16:00 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 79F2029165 for ; Wed, 27 Sep 2017 10:16:00 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6EE4829167; Wed, 27 Sep 2017 10:16:00 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 02FD829165 for ; Wed, 27 Sep 2017 10:16:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6E0446E15F; Wed, 27 Sep 2017 10:15:59 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wm0-x230.google.com (mail-wm0-x230.google.com [IPv6:2a00:1450:400c:c09::230]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1CE956E15F for ; Wed, 27 Sep 2017 10:15:57 +0000 (UTC) Received: by mail-wm0-x230.google.com with SMTP id r74so16359455wme.4 for ; Wed, 27 Sep 2017 03:15:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=mq4RaAVCT2ECv49uKZE1BCbEr690jJ8GjBELxzdxFxQ=; b=iBkiebXjs8x+lPJYPTUEtPxSW1YH9dgwV6K7M/ucCC3VoYGvUw3ybFVbFQGRAFBS5V aPBtYDPO5igQS19FPmoQF9gPoY504ATtrdpUXe2iC7U71e1xGpOZoI+khQacMrxUHCuM a2hTMufirJ7nV6B40mJko9mJu/L1trXISHsV4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=mq4RaAVCT2ECv49uKZE1BCbEr690jJ8GjBELxzdxFxQ=; b=i0I0ex1mqNWqz3Xwd/u4mzduK1JA02Adci48ieVQRmWF7OaFIqiZReU12gX+E6mtGU QMh9FA9e3OCE8pQ6c87f7oTk3ENfg82FG5xpYP3D5JHnnpwiEWHtER79kbVrJtOLUbLv rUYckRM2kH/zGmCJO++J1yIOjXAyXHbJZ8KNmj7Cloa3UuesP0nTk2lMipAvF5LUluFv NK5paKivJceO51Pn9Is1009CC7D1AMvhV52VFC49Do33V6iWwuAwDCQwuU+chUB3aG8l wuCljQ2Chi1U13PE65BpAmwGZgwmxM9q/DC5zoNZu1X8cXtsyXdxLYKjohaoGa+EAiaz pv9g== X-Gm-Message-State: AHPjjUhTutARYCOA7zMOIqlCFYPhH9hcEvGAuvjid8T8QKs94oqCwMD9 1kWkyZ5n9o1U309aDBbdmNYzq/Hf X-Google-Smtp-Source: AOwi7QD5qrLcN7NQW8HeHyiEU/f1Pqxxy50rV93IBHeKoifgZmVwX2VPMjfT+BuQjg5NaKPUi3mBAg== X-Received: by 10.80.135.217 with SMTP id 25mr1277232edz.23.1506507356063; Wed, 27 Sep 2017 03:15:56 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:5635:0:39d2:f87e:2033:9f6]) by smtp.gmail.com with ESMTPSA id l50sm7424123eda.80.2017.09.27.03.15.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Sep 2017 03:15:55 -0700 (PDT) From: Daniel Vetter To: DRI Development Subject: [PATCH] drm/amd: DC pull request review Date: Wed, 27 Sep 2017 12:15:50 +0200 Message-Id: <20170927101550.23176-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.14.1 In-Reply-To: <1506476167-2637-1-git-send-email-alexander.deucher@amd.com> References: <1506476167-2637-1-git-send-email-alexander.deucher@amd.com> Cc: Daniel Vetter , Daniel Vetter X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 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 Reviewed-by: Harry Wentland Acked-by: Sean Paul --- 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.