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.vet...@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.vet...@intel.com>
Thanks for the feedback, ack and help along the way. This patch is Reviewed-by: Harry Wentland <harry.wentl...@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
