| Message ID | 20190902113121.31323-1-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/dp: Various helper improvements and cleanups | expand |
On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Hi, > > this series of patches improves the DP helpers a bit and cleans up some > inconsistencies along the way. > > v2 incorporates all review comments add collects Reviewed-bys from v1. > > Thierry > > Thierry Reding (21): > drm/dp: Sort includes alphabetically > drm/dp: Add missing kerneldoc for struct drm_dp_link > drm/dp: Add drm_dp_link_reset() implementation > drm/dp: Track link capabilities alongside settings > drm/dp: Turn link capabilities into booleans > drm/dp: Probe link using existing parsing helpers > drm/dp: Read fast training capability from link > drm/dp: Read TPS3 capability from sink > drm/dp: Read channel coding capability from sink > drm/dp: Read alternate scrambler reset capability from sink > drm/dp: Read eDP version from DPCD > drm/dp: Read AUX read interval from DPCD > drm/dp: Do not busy-loop during link training > drm/dp: Use drm_dp_aux_rd_interval() > drm/dp: Add helper to get post-cursor adjustments > drm/dp: Set channel coding on link configuration > drm/dp: Enable alternate scrambler reset when supported > drm/dp: Add drm_dp_link_choose() helper > drm/dp: Add support for eDP link rates > drm/dp: Remove a gratuituous blank line > drm/bridge: tc358767: Use DP nomenclature Anyone interested in reviewing these? Thierry > > drivers/gpu/drm/bridge/tc358767.c | 22 +- > drivers/gpu/drm/drm_dp_helper.c | 327 ++++++++++++++++++++++--- > drivers/gpu/drm/msm/edp/edp_ctrl.c | 12 +- > drivers/gpu/drm/rockchip/cdn-dp-core.c | 8 +- > drivers/gpu/drm/rockchip/cdn-dp-reg.c | 13 +- > drivers/gpu/drm/tegra/dpaux.c | 8 +- > drivers/gpu/drm/tegra/sor.c | 32 +-- > include/drm/drm_dp_helper.h | 124 +++++++++- > 8 files changed, 459 insertions(+), 87 deletions(-) > > -- > 2.22.0 >
On Fri, 20 Sep 2019, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote: >> From: Thierry Reding <treding@nvidia.com> >> >> Hi, >> >> this series of patches improves the DP helpers a bit and cleans up some >> inconsistencies along the way. >> >> v2 incorporates all review comments add collects Reviewed-bys from v1. >> >> Thierry >> >> Thierry Reding (21): >> drm/dp: Sort includes alphabetically >> drm/dp: Add missing kerneldoc for struct drm_dp_link >> drm/dp: Add drm_dp_link_reset() implementation >> drm/dp: Track link capabilities alongside settings >> drm/dp: Turn link capabilities into booleans >> drm/dp: Probe link using existing parsing helpers >> drm/dp: Read fast training capability from link >> drm/dp: Read TPS3 capability from sink >> drm/dp: Read channel coding capability from sink >> drm/dp: Read alternate scrambler reset capability from sink >> drm/dp: Read eDP version from DPCD >> drm/dp: Read AUX read interval from DPCD >> drm/dp: Do not busy-loop during link training >> drm/dp: Use drm_dp_aux_rd_interval() >> drm/dp: Add helper to get post-cursor adjustments >> drm/dp: Set channel coding on link configuration >> drm/dp: Enable alternate scrambler reset when supported >> drm/dp: Add drm_dp_link_choose() helper >> drm/dp: Add support for eDP link rates >> drm/dp: Remove a gratuituous blank line >> drm/bridge: tc358767: Use DP nomenclature > > Anyone interested in reviewing these? Thierry, I don't quite know how to put this nicely, but I also don't think it's nice to not reply at all. So I'll try to be fair but it'll be blunt. Fair enough? I've glanced over the series, already before you pinged for reviews. It looks like you've put effort into it, and it all looks nice. However, it does not look like we could use this in i915, without significant effort both on top of this work and in i915. It does not feel like there's any incentive for us to review this in detail. It also feels like there's an increasing disconnect between "small" and "big" drivers (*) when it comes to handling DP link and training. It scares me a bit that this work is being done on the terms of the "small" drivers, and that later in time this might be considered the One True Way of handling DP. One of the technical observations is that you fill the struct drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear that the link caps really are an intersection of the source and sink caps. The eDP 1.4 link rates are the prime example. I think you should have sets of source and sink rates, and you should intersect those to find out the available link rates. The max rate is the highest number in that set. Similarly for many things, like training pattern support. I think it's only going to get more complicated with DP 2.0. Another pain point is the caching of the caps as bits in drm_dp_link_caps. How far are you going to take it? There's an insane and growing amount of things in the DPCD that describe the link in one way or another. Should they all be added to caps? Where do you draw the line? Do we add both the bit and the helper for getting that bit from the DPCD? And are you then going to add support for intersecting all those cap bits between the source and the sink? --- Overall I think there is value in unifying how we handle DP in drm. Even if just by providing helpers to simplify things in drivers. It's just that I feel this series isn't taking us closer to that goal, except for a subset of drivers. In the big picture, it may be increasing the divide. If we get a confirmation from our drm overlords that drivers doing things the way they see fit in this regard is fine, then I'm okay with this. But I'm definitely not committing to switching to using the drm_dp_link structures and helpers in i915, quite the opposite actually. BR, Jani. (*) Please don't read too much into "small" and "big", just two groups of drivers handling things differently. > > Thierry > >> >> drivers/gpu/drm/bridge/tc358767.c | 22 +- >> drivers/gpu/drm/drm_dp_helper.c | 327 ++++++++++++++++++++++--- >> drivers/gpu/drm/msm/edp/edp_ctrl.c | 12 +- >> drivers/gpu/drm/rockchip/cdn-dp-core.c | 8 +- >> drivers/gpu/drm/rockchip/cdn-dp-reg.c | 13 +- >> drivers/gpu/drm/tegra/dpaux.c | 8 +- >> drivers/gpu/drm/tegra/sor.c | 32 +-- >> include/drm/drm_dp_helper.h | 124 +++++++++- >> 8 files changed, 459 insertions(+), 87 deletions(-) >> >> -- >> 2.22.0 >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Sep 23, 2019 at 04:52:50PM +0300, Jani Nikula wrote: > On Fri, 20 Sep 2019, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote: > >> From: Thierry Reding <treding@nvidia.com> > >> > >> Hi, > >> > >> this series of patches improves the DP helpers a bit and cleans up some > >> inconsistencies along the way. > >> > >> v2 incorporates all review comments add collects Reviewed-bys from v1. > >> > >> Thierry > >> > >> Thierry Reding (21): > >> drm/dp: Sort includes alphabetically > >> drm/dp: Add missing kerneldoc for struct drm_dp_link > >> drm/dp: Add drm_dp_link_reset() implementation > >> drm/dp: Track link capabilities alongside settings > >> drm/dp: Turn link capabilities into booleans > >> drm/dp: Probe link using existing parsing helpers > >> drm/dp: Read fast training capability from link > >> drm/dp: Read TPS3 capability from sink > >> drm/dp: Read channel coding capability from sink > >> drm/dp: Read alternate scrambler reset capability from sink > >> drm/dp: Read eDP version from DPCD > >> drm/dp: Read AUX read interval from DPCD > >> drm/dp: Do not busy-loop during link training > >> drm/dp: Use drm_dp_aux_rd_interval() > >> drm/dp: Add helper to get post-cursor adjustments > >> drm/dp: Set channel coding on link configuration > >> drm/dp: Enable alternate scrambler reset when supported > >> drm/dp: Add drm_dp_link_choose() helper > >> drm/dp: Add support for eDP link rates > >> drm/dp: Remove a gratuituous blank line > >> drm/bridge: tc358767: Use DP nomenclature > > > > Anyone interested in reviewing these? > > Thierry, I don't quite know how to put this nicely, but I also don't > think it's nice to not reply at all. So I'll try to be fair but it'll be > blunt. Fair enough? Fair enough. > I've glanced over the series, already before you pinged for reviews. It > looks like you've put effort into it, and it all looks nice. However, it > does not look like we could use this in i915, without significant effort > both on top of this work and in i915. It does not feel like there's any > incentive for us to review this in detail. > > It also feels like there's an increasing disconnect between "small" and > "big" drivers (*) when it comes to handling DP link and training. It > scares me a bit that this work is being done on the terms of the "small" > drivers, and that later in time this might be considered the One True > Way of handling DP. I'm not sure I understand your concern here. The goal of the series is primarily to extend the existing support for DP. It follows the pattern established by existing helpers and then goes one step further and provides some common way of actually storing the values that are being read from the sink so that they can be used. These are meant to be helpers and in no way should anyone feel obliged to use them. If you've got this all figured out already, great! If you do this already much better in i915, by all means stay away from this. At the same time it seems counter-productive to write all of this code as part of the Tegra DRM driver. In my opinion subsystems should provide generic helpers that can help multiple drivers share code. This is especially true for things that are defined in a specification because there's not a lot of room for interpretation. The helpers in these patches are meant to be that kind of helpers. > One of the technical observations is that you fill the struct > drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear > that the link caps really are an intersection of the source and sink > caps. The eDP 1.4 link rates are the prime example. I think you should > have sets of source and sink rates, and you should intersect those to > find out the available link rates. The max rate is the highest number in > that set. Similarly for many things, like training pattern support. I > think it's only going to get more complicated with DP 2.0. The idea here was to provide only helpers to collect the DPCD data defined by the specification. Anything specific to the source is meant to be handled by display driver. In case of eDP 1.4 link rates the code will only add the rates read from DPCD. It's up to the driver to filter out rates that it doesn't support from that list. I think the fact that things will keep getting more complicated is an argument in favour of sharing code rather than keep doing the same (complicated) thing over and over again in every driver. > Another pain point is the caching of the caps as bits in > drm_dp_link_caps. How far are you going to take it? There's an insane > and growing amount of things in the DPCD that describe the link in one > way or another. Should they all be added to caps? Where do you draw the > line? Do we add both the bit and the helper for getting that bit from > the DPCD? And are you then going to add support for intersecting all > those cap bits between the source and the sink? Like I said the primary goal here is to have common code to read the common values from DPCD. Once the link has been "probed" it is up to the driver to do whatever it wants with that data. Originally I had intended this shared code to do much more, but this was shot down during review (I think by Daniel and yourself) for many of the same reasons that you're pointing out. Initially there was code in this series to standardize the link training sequence, for example. This was all strictly according to the specification, so I thought that would give us enough common ground for shared code. But you guys didn't agree, so I've moved that out into Tegra specific patches since then. As for how far to take this, I think the most sensible is to do what we do everywhere else. We add to this whatever is needed on an on-demand basis. The current series here adds what I found to be necessary to support DP on Tegra. There's not a lot of fancy stuff here, I know, but that doesn't mean this code is useless for everyone else. So, can i915 use this? Probably yes. Would that be a good idea? Probably not. And that's perfectly fine. But I could imagine that others may very well want to use some shared code to avoid having to copy/paste code and then later fix up cargo-culted bugs in every driver. I'm also fully aware that this is not a lot and it may not be perfect. But most helpers aren't initially. The point here is to start collecting the common bits in one location and evolve them, just like we do for so many other helpers. Note also that I haven't made any attempt here to convert any drivers to these helpers. That's because these are meant to be opt-in to simplify drivers. If you want to do everything yourself, feel free to do that. It is perfectly legit to do everything yourself if these helpers aren't flexible enough to do what you want. The better option would be to help improve the helpers to make them work for a wider range of drivers, but if you don't want to, then don't. > Overall I think there is value in unifying how we handle DP in drm. Even > if just by providing helpers to simplify things in drivers. It's just > that I feel this series isn't taking us closer to that goal, except for > a subset of drivers. In the big picture, it may be increasing the > divide. So the bulk of this series is stuff that's purely parsing values from DPCD, which is very much in line with existing helpers. I don't think those are in any way contentious. There's also a bit of cleanup here where new helpers are used to simplify existing ones. Maybe a handful of these patches are what you claim might be "increasing the divide". But I really don't understand where this is coming from. i915 doesn't use a myriad of the other helpers (TTM, CMA, simple KMS, ...) and yet these are not increasing any divide, are they? Why do you think these helpers here are different? Again, if you've got all of this implemented in i915 (or any of the other "big" drivers), you probably want to stay away from these. But does that mean everyone else has to go and figure all of this out from scratch? Shouldn't we at least attempt to write common code? Or should we all go and write our own DP stacks like the big drivers? I don't see any advantage in that. > If we get a confirmation from our drm overlords that drivers doing > things the way they see fit in this regard is fine, then I'm okay with > this. But I'm definitely not committing to switching to using the > drm_dp_link structures and helpers in i915, quite the opposite actually. I don't think anyone's going to force you to convert to the drm_dp_link helpers if you don't want to. It's definitely not my intention to make this "The One And Only Way To Do DP in DRM". The goal here is to create helpers that can simplify adding DP support. Now, if everyone else thinks the drm_dp_link helpers are a bad idea, I will get over it and move this into Tegra code. But since we're being blunt, I'd like to get a third (and ideally fourth) opinion on whether we really want this stuff to be reinvented in every driver or whether we want to try and come up with something that works for more than one driver. Thierry > BR, > Jani. > > > (*) Please don't read too much into "small" and "big", just two groups > of drivers handling things differently. > > > > > > > > Thierry > > > >> > >> drivers/gpu/drm/bridge/tc358767.c | 22 +- > >> drivers/gpu/drm/drm_dp_helper.c | 327 ++++++++++++++++++++++--- > >> drivers/gpu/drm/msm/edp/edp_ctrl.c | 12 +- > >> drivers/gpu/drm/rockchip/cdn-dp-core.c | 8 +- > >> drivers/gpu/drm/rockchip/cdn-dp-reg.c | 13 +- > >> drivers/gpu/drm/tegra/dpaux.c | 8 +- > >> drivers/gpu/drm/tegra/sor.c | 32 +-- > >> include/drm/drm_dp_helper.h | 124 +++++++++- > >> 8 files changed, 459 insertions(+), 87 deletions(-) > >> > >> -- > >> 2.22.0 > >> > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Jani Nikula, Intel Open Source Graphics Center
On Mon, Sep 23, 2019 at 04:52:02PM +0200, Thierry Reding wrote: > On Mon, Sep 23, 2019 at 04:52:50PM +0300, Jani Nikula wrote: > > On Fri, 20 Sep 2019, Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote: > > >> From: Thierry Reding <treding@nvidia.com> > > >> > > >> Hi, > > >> > > >> this series of patches improves the DP helpers a bit and cleans up some > > >> inconsistencies along the way. > > >> > > >> v2 incorporates all review comments add collects Reviewed-bys from v1. > > >> > > >> Thierry > > >> > > >> Thierry Reding (21): > > >> drm/dp: Sort includes alphabetically > > >> drm/dp: Add missing kerneldoc for struct drm_dp_link > > >> drm/dp: Add drm_dp_link_reset() implementation > > >> drm/dp: Track link capabilities alongside settings > > >> drm/dp: Turn link capabilities into booleans > > >> drm/dp: Probe link using existing parsing helpers > > >> drm/dp: Read fast training capability from link > > >> drm/dp: Read TPS3 capability from sink > > >> drm/dp: Read channel coding capability from sink > > >> drm/dp: Read alternate scrambler reset capability from sink > > >> drm/dp: Read eDP version from DPCD > > >> drm/dp: Read AUX read interval from DPCD > > >> drm/dp: Do not busy-loop during link training > > >> drm/dp: Use drm_dp_aux_rd_interval() > > >> drm/dp: Add helper to get post-cursor adjustments > > >> drm/dp: Set channel coding on link configuration > > >> drm/dp: Enable alternate scrambler reset when supported > > >> drm/dp: Add drm_dp_link_choose() helper > > >> drm/dp: Add support for eDP link rates > > >> drm/dp: Remove a gratuituous blank line > > >> drm/bridge: tc358767: Use DP nomenclature > > > > > > Anyone interested in reviewing these? > > > > Thierry, I don't quite know how to put this nicely, but I also don't > > think it's nice to not reply at all. So I'll try to be fair but it'll be > > blunt. Fair enough? > > Fair enough. > > > I've glanced over the series, already before you pinged for reviews. It > > looks like you've put effort into it, and it all looks nice. However, it > > does not look like we could use this in i915, without significant effort > > both on top of this work and in i915. It does not feel like there's any > > incentive for us to review this in detail. > > > > It also feels like there's an increasing disconnect between "small" and > > "big" drivers (*) when it comes to handling DP link and training. It > > scares me a bit that this work is being done on the terms of the "small" > > drivers, and that later in time this might be considered the One True > > Way of handling DP. > > I'm not sure I understand your concern here. The goal of the series is > primarily to extend the existing support for DP. It follows the pattern > established by existing helpers and then goes one step further and > provides some common way of actually storing the values that are being > read from the sink so that they can be used. > > These are meant to be helpers and in no way should anyone feel obliged > to use them. If you've got this all figured out already, great! If you > do this already much better in i915, by all means stay away from this. > > At the same time it seems counter-productive to write all of this code > as part of the Tegra DRM driver. In my opinion subsystems should provide > generic helpers that can help multiple drivers share code. This is > especially true for things that are defined in a specification because > there's not a lot of room for interpretation. The helpers in these > patches are meant to be that kind of helpers. > > > One of the technical observations is that you fill the struct > > drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear > > that the link caps really are an intersection of the source and sink > > caps. The eDP 1.4 link rates are the prime example. I think you should > > have sets of source and sink rates, and you should intersect those to > > find out the available link rates. The max rate is the highest number in > > that set. Similarly for many things, like training pattern support. I > > think it's only going to get more complicated with DP 2.0. > > The idea here was to provide only helpers to collect the DPCD data > defined by the specification. Anything specific to the source is meant > to be handled by display driver. In case of eDP 1.4 link rates the code > will only add the rates read from DPCD. It's up to the driver to filter > out rates that it doesn't support from that list. > > I think the fact that things will keep getting more complicated is an > argument in favour of sharing code rather than keep doing the same > (complicated) thing over and over again in every driver. > > > Another pain point is the caching of the caps as bits in > > drm_dp_link_caps. How far are you going to take it? There's an insane > > and growing amount of things in the DPCD that describe the link in one > > way or another. Should they all be added to caps? Where do you draw the > > line? Do we add both the bit and the helper for getting that bit from > > the DPCD? And are you then going to add support for intersecting all > > those cap bits between the source and the sink? > > Like I said the primary goal here is to have common code to read the > common values from DPCD. Once the link has been "probed" it is up to the > driver to do whatever it wants with that data. > > Originally I had intended this shared code to do much more, but this was > shot down during review (I think by Daniel and yourself) for many of the > same reasons that you're pointing out. Initially there was code in this > series to standardize the link training sequence, for example. This was > all strictly according to the specification, so I thought that would > give us enough common ground for shared code. But you guys didn't agree, > so I've moved that out into Tegra specific patches since then. > > As for how far to take this, I think the most sensible is to do what we > do everywhere else. We add to this whatever is needed on an on-demand > basis. The current series here adds what I found to be necessary to > support DP on Tegra. There's not a lot of fancy stuff here, I know, but > that doesn't mean this code is useless for everyone else. > > So, can i915 use this? Probably yes. Would that be a good idea? Probably > not. And that's perfectly fine. But I could imagine that others may very > well want to use some shared code to avoid having to copy/paste code and > then later fix up cargo-culted bugs in every driver. > > I'm also fully aware that this is not a lot and it may not be perfect. > But most helpers aren't initially. The point here is to start collecting > the common bits in one location and evolve them, just like we do for so > many other helpers. > > Note also that I haven't made any attempt here to convert any drivers to > these helpers. That's because these are meant to be opt-in to simplify > drivers. If you want to do everything yourself, feel free to do that. It > is perfectly legit to do everything yourself if these helpers aren't > flexible enough to do what you want. The better option would be to help > improve the helpers to make them work for a wider range of drivers, but > if you don't want to, then don't. > > > Overall I think there is value in unifying how we handle DP in drm. Even > > if just by providing helpers to simplify things in drivers. It's just > > that I feel this series isn't taking us closer to that goal, except for > > a subset of drivers. In the big picture, it may be increasing the > > divide. > > So the bulk of this series is stuff that's purely parsing values from > DPCD, which is very much in line with existing helpers. I don't think > those are in any way contentious. There's also a bit of cleanup here > where new helpers are used to simplify existing ones. Maybe a handful > of these patches are what you claim might be "increasing the divide". > > But I really don't understand where this is coming from. i915 doesn't > use a myriad of the other helpers (TTM, CMA, simple KMS, ...) and yet > these are not increasing any divide, are they? Why do you think these > helpers here are different? > > Again, if you've got all of this implemented in i915 (or any of the > other "big" drivers), you probably want to stay away from these. But > does that mean everyone else has to go and figure all of this out from > scratch? Shouldn't we at least attempt to write common code? Or should > we all go and write our own DP stacks like the big drivers? I don't see > any advantage in that. > > > If we get a confirmation from our drm overlords that drivers doing > > things the way they see fit in this regard is fine, then I'm okay with > > this. But I'm definitely not committing to switching to using the > > drm_dp_link structures and helpers in i915, quite the opposite actually. > > I don't think anyone's going to force you to convert to the drm_dp_link > helpers if you don't want to. It's definitely not my intention to make > this "The One And Only Way To Do DP in DRM". The goal here is to create > helpers that can simplify adding DP support. > > Now, if everyone else thinks the drm_dp_link helpers are a bad idea, I > will get over it and move this into Tegra code. But since we're being > blunt, I'd like to get a third (and ideally fourth) opinion on whether > we really want this stuff to be reinvented in every driver or whether > we want to try and come up with something that works for more than one > driver. > > Thierry > > > BR, > > Jani. > > > > > > (*) Please don't read too much into "small" and "big", just two groups > > of drivers handling things differently. Dave, Daniel, how can we make progress on this? Is it okay to go forward with this series if we agree that it's not going to be required for drivers to adopt it if they already have a working DP implementation? If we can't agree on the struct drm_dp_link helpers, perhaps I should go and at least merge the additional DPCD parsing helpers. Those are very much in line with existing helpers. I could then move the drm_dp_link helpers into the Tegra driver and add replacement code to the other drivers that already use struct drm_dp_link. It'd be a shame to duplicate the code, but I'm willing to invest that additional work so that I can finally make progress on this series and the Tegra DP support that depends on this. Thierry > > > > > > > > > > > > > > Thierry > > > > > >> > > >> drivers/gpu/drm/bridge/tc358767.c | 22 +- > > >> drivers/gpu/drm/drm_dp_helper.c | 327 ++++++++++++++++++++++--- > > >> drivers/gpu/drm/msm/edp/edp_ctrl.c | 12 +- > > >> drivers/gpu/drm/rockchip/cdn-dp-core.c | 8 +- > > >> drivers/gpu/drm/rockchip/cdn-dp-reg.c | 13 +- > > >> drivers/gpu/drm/tegra/dpaux.c | 8 +- > > >> drivers/gpu/drm/tegra/sor.c | 32 +-- > > >> include/drm/drm_dp_helper.h | 124 +++++++++- > > >> 8 files changed, 459 insertions(+), 87 deletions(-) > > >> > > >> -- > > >> 2.22.0 > > >> > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Jani Nikula, Intel Open Source Graphics Center
On Wed, Oct 02, 2019 at 06:14:19PM +0200, Thierry Reding wrote: > On Mon, Sep 23, 2019 at 04:52:02PM +0200, Thierry Reding wrote: > > On Mon, Sep 23, 2019 at 04:52:50PM +0300, Jani Nikula wrote: > > > On Fri, 20 Sep 2019, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote: > > > >> From: Thierry Reding <treding@nvidia.com> > > > >> > > > >> Hi, > > > >> > > > >> this series of patches improves the DP helpers a bit and cleans up some > > > >> inconsistencies along the way. > > > >> > > > >> v2 incorporates all review comments add collects Reviewed-bys from v1. > > > >> > > > >> Thierry > > > >> > > > >> Thierry Reding (21): > > > >> drm/dp: Sort includes alphabetically > > > >> drm/dp: Add missing kerneldoc for struct drm_dp_link > > > >> drm/dp: Add drm_dp_link_reset() implementation > > > >> drm/dp: Track link capabilities alongside settings > > > >> drm/dp: Turn link capabilities into booleans > > > >> drm/dp: Probe link using existing parsing helpers > > > >> drm/dp: Read fast training capability from link > > > >> drm/dp: Read TPS3 capability from sink > > > >> drm/dp: Read channel coding capability from sink > > > >> drm/dp: Read alternate scrambler reset capability from sink > > > >> drm/dp: Read eDP version from DPCD > > > >> drm/dp: Read AUX read interval from DPCD > > > >> drm/dp: Do not busy-loop during link training > > > >> drm/dp: Use drm_dp_aux_rd_interval() > > > >> drm/dp: Add helper to get post-cursor adjustments > > > >> drm/dp: Set channel coding on link configuration > > > >> drm/dp: Enable alternate scrambler reset when supported > > > >> drm/dp: Add drm_dp_link_choose() helper > > > >> drm/dp: Add support for eDP link rates > > > >> drm/dp: Remove a gratuituous blank line > > > >> drm/bridge: tc358767: Use DP nomenclature > > > > > > > > Anyone interested in reviewing these? > > > > > > Thierry, I don't quite know how to put this nicely, but I also don't > > > think it's nice to not reply at all. So I'll try to be fair but it'll be > > > blunt. Fair enough? > > > > Fair enough. > > > > > I've glanced over the series, already before you pinged for reviews. It > > > looks like you've put effort into it, and it all looks nice. However, it > > > does not look like we could use this in i915, without significant effort > > > both on top of this work and in i915. It does not feel like there's any > > > incentive for us to review this in detail. > > > > > > It also feels like there's an increasing disconnect between "small" and > > > "big" drivers (*) when it comes to handling DP link and training. It > > > scares me a bit that this work is being done on the terms of the "small" > > > drivers, and that later in time this might be considered the One True > > > Way of handling DP. > > > > I'm not sure I understand your concern here. The goal of the series is > > primarily to extend the existing support for DP. It follows the pattern > > established by existing helpers and then goes one step further and > > provides some common way of actually storing the values that are being > > read from the sink so that they can be used. > > > > These are meant to be helpers and in no way should anyone feel obliged > > to use them. If you've got this all figured out already, great! If you > > do this already much better in i915, by all means stay away from this. > > > > At the same time it seems counter-productive to write all of this code > > as part of the Tegra DRM driver. In my opinion subsystems should provide > > generic helpers that can help multiple drivers share code. This is > > especially true for things that are defined in a specification because > > there's not a lot of room for interpretation. The helpers in these > > patches are meant to be that kind of helpers. > > > > > One of the technical observations is that you fill the struct > > > drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear > > > that the link caps really are an intersection of the source and sink > > > caps. The eDP 1.4 link rates are the prime example. I think you should > > > have sets of source and sink rates, and you should intersect those to > > > find out the available link rates. The max rate is the highest number in > > > that set. Similarly for many things, like training pattern support. I > > > think it's only going to get more complicated with DP 2.0. > > > > The idea here was to provide only helpers to collect the DPCD data > > defined by the specification. Anything specific to the source is meant > > to be handled by display driver. In case of eDP 1.4 link rates the code > > will only add the rates read from DPCD. It's up to the driver to filter > > out rates that it doesn't support from that list. > > > > I think the fact that things will keep getting more complicated is an > > argument in favour of sharing code rather than keep doing the same > > (complicated) thing over and over again in every driver. > > > > > Another pain point is the caching of the caps as bits in > > > drm_dp_link_caps. How far are you going to take it? There's an insane > > > and growing amount of things in the DPCD that describe the link in one > > > way or another. Should they all be added to caps? Where do you draw the > > > line? Do we add both the bit and the helper for getting that bit from > > > the DPCD? And are you then going to add support for intersecting all > > > those cap bits between the source and the sink? > > > > Like I said the primary goal here is to have common code to read the > > common values from DPCD. Once the link has been "probed" it is up to the > > driver to do whatever it wants with that data. > > > > Originally I had intended this shared code to do much more, but this was > > shot down during review (I think by Daniel and yourself) for many of the > > same reasons that you're pointing out. Initially there was code in this > > series to standardize the link training sequence, for example. This was > > all strictly according to the specification, so I thought that would > > give us enough common ground for shared code. But you guys didn't agree, > > so I've moved that out into Tegra specific patches since then. > > > > As for how far to take this, I think the most sensible is to do what we > > do everywhere else. We add to this whatever is needed on an on-demand > > basis. The current series here adds what I found to be necessary to > > support DP on Tegra. There's not a lot of fancy stuff here, I know, but > > that doesn't mean this code is useless for everyone else. > > > > So, can i915 use this? Probably yes. Would that be a good idea? Probably > > not. And that's perfectly fine. But I could imagine that others may very > > well want to use some shared code to avoid having to copy/paste code and > > then later fix up cargo-culted bugs in every driver. > > > > I'm also fully aware that this is not a lot and it may not be perfect. > > But most helpers aren't initially. The point here is to start collecting > > the common bits in one location and evolve them, just like we do for so > > many other helpers. > > > > Note also that I haven't made any attempt here to convert any drivers to > > these helpers. That's because these are meant to be opt-in to simplify > > drivers. If you want to do everything yourself, feel free to do that. It > > is perfectly legit to do everything yourself if these helpers aren't > > flexible enough to do what you want. The better option would be to help > > improve the helpers to make them work for a wider range of drivers, but > > if you don't want to, then don't. > > > > > Overall I think there is value in unifying how we handle DP in drm. Even > > > if just by providing helpers to simplify things in drivers. It's just > > > that I feel this series isn't taking us closer to that goal, except for > > > a subset of drivers. In the big picture, it may be increasing the > > > divide. > > > > So the bulk of this series is stuff that's purely parsing values from > > DPCD, which is very much in line with existing helpers. I don't think > > those are in any way contentious. There's also a bit of cleanup here > > where new helpers are used to simplify existing ones. Maybe a handful > > of these patches are what you claim might be "increasing the divide". > > > > But I really don't understand where this is coming from. i915 doesn't > > use a myriad of the other helpers (TTM, CMA, simple KMS, ...) and yet > > these are not increasing any divide, are they? Why do you think these > > helpers here are different? > > > > Again, if you've got all of this implemented in i915 (or any of the > > other "big" drivers), you probably want to stay away from these. But > > does that mean everyone else has to go and figure all of this out from > > scratch? Shouldn't we at least attempt to write common code? Or should > > we all go and write our own DP stacks like the big drivers? I don't see > > any advantage in that. > > > > > If we get a confirmation from our drm overlords that drivers doing > > > things the way they see fit in this regard is fine, then I'm okay with > > > this. But I'm definitely not committing to switching to using the > > > drm_dp_link structures and helpers in i915, quite the opposite actually. > > > > I don't think anyone's going to force you to convert to the drm_dp_link > > helpers if you don't want to. It's definitely not my intention to make > > this "The One And Only Way To Do DP in DRM". The goal here is to create > > helpers that can simplify adding DP support. > > > > Now, if everyone else thinks the drm_dp_link helpers are a bad idea, I > > will get over it and move this into Tegra code. But since we're being > > blunt, I'd like to get a third (and ideally fourth) opinion on whether > > we really want this stuff to be reinvented in every driver or whether > > we want to try and come up with something that works for more than one > > driver. > > > > Thierry > > > > > BR, > > > Jani. > > > > > > > > > (*) Please don't read too much into "small" and "big", just two groups > > > of drivers handling things differently. > > Dave, Daniel, > > how can we make progress on this? Is it okay to go forward with this > series if we agree that it's not going to be required for drivers to > adopt it if they already have a working DP implementation? > > If we can't agree on the struct drm_dp_link helpers, perhaps I should go > and at least merge the additional DPCD parsing helpers. Those are very > much in line with existing helpers. I could then move the drm_dp_link > helpers into the Tegra driver and add replacement code to the other > drivers that already use struct drm_dp_link. It'd be a shame to > duplicate the code, but I'm willing to invest that additional work so > that I can finally make progress on this series and the Tegra DP support > that depends on this. I ... don't know. In general I think helpers are totally ok with being optional (that's the point after all), but not for color choice reasons. And from very much afar this feels a bit like that. I also think that if we want smaller helpers for simpler drivers, it's better to build that on top of the full-featured helpers and dumb them down. This worked very well for simple display pipe (built on top of atomic helpers) and simple vram support (built on top of ttm). Having two distinct set of helpers for small and big drivers seems wrong. I also think that the functions/control-flow/callbacks are the important part of helpers, not really the data structures. Just having common data structures gives you as much a mess as having a pile of distinct implementations. Aside from all these generic thoughts on helpers, for dp specifically I think the proof of the pudding is in how it integrates with mst. If we have dp helpers that work for mst (the mst vs sst case decision and transitions are especially nasty in all drivers), then we can dumb it down for simple drivers and modularize it for special cases like we do with other helpers. Without that I fear we're just ending up in a dead-end (but won't realize it until it's too late). Finally I'm ok with no helpers in areas where we haven't figured out a good solution yet. Bunch of copypasta is imo better than going the wrong way collectively (since it prevents the experiments that might shine a light on better solutions). tldr; Still don't know. Cheers, Daniel
Hi! a couple people poked me to take a look at this, hopefully I can provide some helpful insight here. So: I've tried a _lot_ of various redesigns with MST and some portions of the DP helpers. I actually was going to try to write up some common helpers for handling link training across drivers, but when I started trying to implement them (ironically, I think it was in i915!) I realized I wasn't getting rid of nearly as much code as I thought I was going to. Now-I'd love to tell you if this series is good or bad, but I'm not entirely sure myself either. Sometimes I wonder myself if I'm overcomplicating things with MST. The only way I've really found of figuring out whether or not helpers like this are overkill is to just implement them everywhere. With my atomic VCPI helpers for MST, I tried implementing them everywhere I could (except for amdgpu, but I _did_ try there originally!) to see how awkward they were to use. I think if there's questions to how useful this series is, it'd probably be good to try implementing these helpers in drivers like i915 to see how things play out. It should also be noted that I did actually try to come up with common link rate helpers like this one, but I ended up realizing I was adding far more code then I was getting rid of after I tried implementing them in i915 (ironic, huh?). Things got even more complicated when I looked at how nouveau/nvidia hardware does link training. On Fri, 2019-09-20 at 18:00 +0200, Thierry Reding wrote: > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > Hi, > > > > this series of patches improves the DP helpers a bit and cleans up some > > inconsistencies along the way. > > > > v2 incorporates all review comments add collects Reviewed-bys from v1. > > > > Thierry > > > > Thierry Reding (21): > > drm/dp: Sort includes alphabetically > > drm/dp: Add missing kerneldoc for struct drm_dp_link > > drm/dp: Add drm_dp_link_reset() implementation > > drm/dp: Track link capabilities alongside settings > > drm/dp: Turn link capabilities into booleans > > drm/dp: Probe link using existing parsing helpers > > drm/dp: Read fast training capability from link > > drm/dp: Read TPS3 capability from sink > > drm/dp: Read channel coding capability from sink > > drm/dp: Read alternate scrambler reset capability from sink > > drm/dp: Read eDP version from DPCD > > drm/dp: Read AUX read interval from DPCD > > drm/dp: Do not busy-loop during link training > > drm/dp: Use drm_dp_aux_rd_interval() > > drm/dp: Add helper to get post-cursor adjustments > > drm/dp: Set channel coding on link configuration > > drm/dp: Enable alternate scrambler reset when supported > > drm/dp: Add drm_dp_link_choose() helper > > drm/dp: Add support for eDP link rates > > drm/dp: Remove a gratuituous blank line > > drm/bridge: tc358767: Use DP nomenclature > > Anyone interested in reviewing these? > > Thierry > > > drivers/gpu/drm/bridge/tc358767.c | 22 +- > > drivers/gpu/drm/drm_dp_helper.c | 327 ++++++++++++++++++++++--- > > drivers/gpu/drm/msm/edp/edp_ctrl.c | 12 +- > > drivers/gpu/drm/rockchip/cdn-dp-core.c | 8 +- > > drivers/gpu/drm/rockchip/cdn-dp-reg.c | 13 +- > > drivers/gpu/drm/tegra/dpaux.c | 8 +- > > drivers/gpu/drm/tegra/sor.c | 32 +-- > > include/drm/drm_dp_helper.h | 124 +++++++++- > > 8 files changed, 459 insertions(+), 87 deletions(-) > > > > -- > > 2.22.0 > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Thierry Reding <treding@nvidia.com> Hi, this series of patches improves the DP helpers a bit and cleans up some inconsistencies along the way. v2 incorporates all review comments add collects Reviewed-bys from v1. Thierry Thierry Reding (21): drm/dp: Sort includes alphabetically drm/dp: Add missing kerneldoc for struct drm_dp_link drm/dp: Add drm_dp_link_reset() implementation drm/dp: Track link capabilities alongside settings drm/dp: Turn link capabilities into booleans drm/dp: Probe link using existing parsing helpers drm/dp: Read fast training capability from link drm/dp: Read TPS3 capability from sink drm/dp: Read channel coding capability from sink drm/dp: Read alternate scrambler reset capability from sink drm/dp: Read eDP version from DPCD drm/dp: Read AUX read interval from DPCD drm/dp: Do not busy-loop during link training drm/dp: Use drm_dp_aux_rd_interval() drm/dp: Add helper to get post-cursor adjustments drm/dp: Set channel coding on link configuration drm/dp: Enable alternate scrambler reset when supported drm/dp: Add drm_dp_link_choose() helper drm/dp: Add support for eDP link rates drm/dp: Remove a gratuituous blank line drm/bridge: tc358767: Use DP nomenclature drivers/gpu/drm/bridge/tc358767.c | 22 +- drivers/gpu/drm/drm_dp_helper.c | 327 ++++++++++++++++++++++--- drivers/gpu/drm/msm/edp/edp_ctrl.c | 12 +- drivers/gpu/drm/rockchip/cdn-dp-core.c | 8 +- drivers/gpu/drm/rockchip/cdn-dp-reg.c | 13 +- drivers/gpu/drm/tegra/dpaux.c | 8 +- drivers/gpu/drm/tegra/sor.c | 32 +-- include/drm/drm_dp_helper.h | 124 +++++++++- 8 files changed, 459 insertions(+), 87 deletions(-)