On Wed, Sep 27, 2017 at 6:15 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxxx> > > 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@xxxxxxxxxxxx> > > 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@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
