drm/amd: DC pull request review
diff mbox

Message ID 20170927101550.23176-1-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Sept. 27, 2017, 10:15 a.m. UTC
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>
---
 drivers/gpu/drm/amd/display/TODO | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Harry Wentland Sept. 27, 2017, 1:47 p.m. UTC | #1
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
Daniel Vetter Sept. 27, 2017, 1:49 p.m. UTC | #2
On Wed, Sep 27, 2017 at 3:47 PM, Harry Wentland <harry.wentland@amd.com> wrote:
> On 2017-09-27 06:15 AM, Daniel Vetter wrote:
>> Ok, here's one more attempt at scrolling through 130k diff.
>> +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.

Plenty of complaints for sure, but I don't think anyone has looked
into it seriously. The big trouble is backwards compat with existing
drm.debug (too many people know about that one), and with dynamic
printk specifically, that non-debug builds don't build it (so useless
as-is for debugging issues users have). That was at least the
conclusion from the last few discussions I remember.
-Daniel
Sean Paul Sept. 27, 2017, 4:38 p.m. UTC | #3
On Wed, Sep 27, 2017 at 6:15 AM, Daniel Vetter <daniel.vetter@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@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@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Sept. 27, 2017, 4:48 p.m. UTC | #4
On Wed, Sep 27, 2017 at 6:38 PM, Sean Paul <seanpaul@chromium.org> wrote:
> Any chance we can address the i2c/gpio [re-]implementations as well?

It's already on the list. Part of this is code that's probably dead,
the other is a bit too much layer cake still left, and the final bits
are not fully using drm helpers for edid parsing and tons of other
stuff. But afaics all the i2c stuff does go through the i2c_adapter
abstraction in a proper fashion, which is at least the foundational
work. The other bits should be all captured in the todo already.
-Daniel
Harry Wentland Sept. 27, 2017, 6:12 p.m. UTC | #5
On 2017-09-27 12:48 PM, Daniel Vetter wrote:
> On Wed, Sep 27, 2017 at 6:38 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> Any chance we can address the i2c/gpio [re-]implementations as well?
> 
> It's already on the list. Part of this is code that's probably dead,
> the other is a bit too much layer cake still left, and the final bits
> are not fully using drm helpers for edid parsing and tons of other
> stuff. But afaics all the i2c stuff does go through the i2c_adapter
> abstraction in a proper fashion, which is at least the foundational
> work. The other bits should be all captured in the todo already.

There might still be some dead code around i2c stuff. I'll take a look
and see what I can rip out.

If there's any specific function, struct, etc. that's of concern let me
know as well.

Harry

> -Daniel
>
Harry Wentland Sept. 27, 2017, 7:25 p.m. UTC | #6
On 2017-09-27 02:12 PM, Harry Wentland wrote:
> On 2017-09-27 12:48 PM, Daniel Vetter wrote:
>> On Wed, Sep 27, 2017 at 6:38 PM, Sean Paul <seanpaul@chromium.org> wrote:
>>> Any chance we can address the i2c/gpio [re-]implementations as well?
>>
>> It's already on the list. Part of this is code that's probably dead,
>> the other is a bit too much layer cake still left, and the final bits
>> are not fully using drm helpers for edid parsing and tons of other
>> stuff. But afaics all the i2c stuff does go through the i2c_adapter
>> abstraction in a proper fashion, which is at least the foundational
>> work. The other bits should be all captured in the todo already.
> 
> There might still be some dead code around i2c stuff. I'll take a look
> and see what I can rip out.
> 

Looks like there currently are no easy pickings. What we have is:

1) Connector info read

See get_ext_display_connection_info

On some boards the connector information has to be read through a
special I2C channel. This line is only used for this purpose and only on
driver init.


2) SCDC stuff

This should all be reworked to go through DRM's SCDC code. When this is
done some unnecessary I2C code can be retired as well.


3) Max TMDS clock read

See dal_ddc_service_i2c_query_dp_dual_mode_adaptor

This should happen in DRM as well. I haven't checked if there's
currently functionality in DRM. If not we can propose something.


4) HDMI retimer programming

Some boards have an HDMI retimer that we need to program to pass PHY
compliance.

It would take a bit of time to address all of them. I'll add them on the
TODO.

1 & 3 might be a good exercise if someone is looking for things to do.

Harry


> If there's any specific function, struct, etc. that's of concern let me
> know as well.
> 
> Harry
> 
>> -Daniel
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Daniel Vetter Sept. 28, 2017, 6:28 a.m. UTC | #7
On Wed, Sep 27, 2017 at 9:25 PM, Harry Wentland <harry.wentland@amd.com> wrote:
> On 2017-09-27 02:12 PM, Harry Wentland wrote:
>> On 2017-09-27 12:48 PM, Daniel Vetter wrote:
>>> On Wed, Sep 27, 2017 at 6:38 PM, Sean Paul <seanpaul@chromium.org> wrote:
>>>> Any chance we can address the i2c/gpio [re-]implementations as well?
>>>
>>> It's already on the list. Part of this is code that's probably dead,
>>> the other is a bit too much layer cake still left, and the final bits
>>> are not fully using drm helpers for edid parsing and tons of other
>>> stuff. But afaics all the i2c stuff does go through the i2c_adapter
>>> abstraction in a proper fashion, which is at least the foundational
>>> work. The other bits should be all captured in the todo already.
>>
>> There might still be some dead code around i2c stuff. I'll take a look
>> and see what I can rip out.
>>
>
> Looks like there currently are no easy pickings. What we have is:
>
> 1) Connector info read
>
> See get_ext_display_connection_info
>
> On some boards the connector information has to be read through a
> special I2C channel. This line is only used for this purpose and only on
> driver init.
>
>
> 2) SCDC stuff
>
> This should all be reworked to go through DRM's SCDC code. When this is
> done some unnecessary I2C code can be retired as well.
>
>
> 3) Max TMDS clock read
>
> See dal_ddc_service_i2c_query_dp_dual_mode_adaptor
>
> This should happen in DRM as well. I haven't checked if there's
> currently functionality in DRM. If not we can propose something.
>
>
> 4) HDMI retimer programming
>
> Some boards have an HDMI retimer that we need to program to pass PHY
> compliance.

I spotted this too, but I'm not aware of any other driver/hw using
this. If it's some standard we might indeed want to move it into the
helpers, but not as a priority. That's why I didn't add it as a todo
item.

All the other bits should be in the TODO already (with my latest patch
at least). You might want to extend/clarify those entries though.

> It would take a bit of time to address all of them. I'll add them on the
> TODO.
>
> 1 & 3 might be a good exercise if someone is looking for things to do.

Yeah, some of this stuff looks like good gsoc/evoc/outreachy stuff.
-Daniel

>
> Harry
>
>
>> If there's any specific function, struct, etc. that's of concern let me
>> know as well.
>>
>> Harry
>>
>>> -Daniel
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>

Patch
diff mbox

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.