| Message ID | 20190822135741.12923-1-David.Francis@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | MST DSC support in drm-mst | expand |
Still in the process of reviewing this, but one minor change that should be done on all of the patches (which I didn't notice before, whoops): s:drm/dp-mst:drm/dp_mst:g On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote: > Add necessary support for MST DSC. > (Display Stream COmpression over Multi-Stream Transport) > > v4: Split patchset and rebase onto drm-tip > > David Francis (5): > drm/dp-mst: Add PBN calculation for DSC modes > drm/dp-mst: Parse FEC capability on MST ports > drm/dp-mst: Add MST support to DP DPCD R/W functions > drm/dp-mst: Fill branch->num_ports > drm/dp-mst: Add helpers for querying and enabling MST DSC > > drivers/gpu/drm/drm_dp_aux_dev.c | 12 +- > drivers/gpu/drm/drm_dp_helper.c | 10 +- > drivers/gpu/drm/drm_dp_mst_topology.c | 243 ++++++++++++++++++++++++++ > include/drm/drm_dp_mst_helper.h | 8 +- > 4 files changed, 260 insertions(+), 13 deletions(-) >
OK-done reviewing, but there's some stuff missing here. The PBN bandwidth checks in https://patchwork.freedesktop.org/patch/325604/?series=65423&rev=3 need to be moved into drm_dp_mst_atomic_check(), along with moving amdgpu over to using drm_dp_mst_atomic_check(). Doing so will also give us PBN bandwidth checks in both i915 and nouveau as well, and keeps the bandwidth calculation where it should be. Additionally, you still need to move the code here into an MST atomic helper or drm_dp_mst_atomic_check() as well: https://patchwork.freedesktop.org/patch/325611/?series=65423&rev=3 Unless I'm mistaken, adding each CRTC which has a connector whose PBN requires recalculation into the atomic state is something every DRM driver is going to need to do. And, you can do this more easily by adding PBN information into drm_dp_mst_topology_state. Yes-it's a lot of locks, but since every connector in an MST topology is sharing the bandwidth on the same link it's kind of expected. I already know I'm going to have to do basically the same thing with every driver once I've got the time to actually implement fallback link rate retraining with MST topologies. If you need help figuring out how to structure this in a way that works for all drivers, I'm willing to help and I'm sure Manasi is as well. On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote: > Add necessary support for MST DSC. > (Display Stream COmpression over Multi-Stream Transport) > > v4: Split patchset and rebase onto drm-tip > > David Francis (5): > drm/dp-mst: Add PBN calculation for DSC modes > drm/dp-mst: Parse FEC capability on MST ports > drm/dp-mst: Add MST support to DP DPCD R/W functions > drm/dp-mst: Fill branch->num_ports > drm/dp-mst: Add helpers for querying and enabling MST DSC > > drivers/gpu/drm/drm_dp_aux_dev.c | 12 +- > drivers/gpu/drm/drm_dp_helper.c | 10 +- > drivers/gpu/drm/drm_dp_mst_topology.c | 243 ++++++++++++++++++++++++++ > include/drm/drm_dp_mst_helper.h | 8 +- > 4 files changed, 260 insertions(+), 13 deletions(-) >
Adding DSC functionality to drm_dp_mst_atomic_check() is a good idea. However, until amdgpu switches over to that system, I wouldn't be able to test those changes. Making that switch is on our TODO list, and it would fix a number of problems with our current MST implementation, but it's going to be a major rewrite. MST DSC hardware is already on the market. It would be expedient to merge the patches we need for Navi support sooner and update drm_dp_mst_atomic_check when we're able to test it. David Francis
On Sat, 24 Aug 2019 at 06:24, Francis, David <David.Francis@amd.com> wrote: > > Adding DSC functionality to drm_dp_mst_atomic_check() is a good idea. > However, until amdgpu switches over to that system, I wouldn't be able > to test those changes. Making that switch is on our TODO list, and it would > fix a number of problems with our current MST implementation, but > it's going to be a major rewrite. > > MST DSC hardware is already on the market. It would be expedient to > merge the patches we need for Navi support sooner and update > drm_dp_mst_atomic_check when we're able to test it. Is there any commitment to rewriting it, a timeline or anything? The problem with this situation is there is always new hardware coming onto the market, and there is always pressure to support all the features of that new hardware, and the pressure always comes like this and being expedient. However I've found that a lot of the time the required refactor or work is never done, because the time is being allocated now to the next GPU that is coming on the market, and nobody ever cares enough to clean up their technical debt. How come the needs for MST DSC support wasn't identified earlier, blocked on refactoring of the code to use common code, and then that task made a higher priority? I'm sorta inclined to say no we shouldn't be merging any driver specific code here, because this is the only point we can push pressure on to refactor the MST implementation, which I guess otherwise we'll just keep avoiding until Lyude ends up doing it for you. Dave.
On 2019-08-26 3:50 p.m., Dave Airlie wrote: > On Sat, 24 Aug 2019 at 06:24, Francis, David <David.Francis@amd.com> wrote: >> >> Adding DSC functionality to drm_dp_mst_atomic_check() is a good idea. >> However, until amdgpu switches over to that system, I wouldn't be able >> to test those changes. Making that switch is on our TODO list, and it would >> fix a number of problems with our current MST implementation, but >> it's going to be a major rewrite. >> >> MST DSC hardware is already on the market. It would be expedient to >> merge the patches we need for Navi support sooner and update >> drm_dp_mst_atomic_check when we're able to test it. > > Is there any commitment to rewriting it, a timeline or anything? > > The problem with this situation is there is always new hardware coming > onto the market, and there is always pressure to support all the > features of that new hardware, and the pressure always comes like this > and being expedient. However I've found that a lot of the time the > required refactor or work is never done, because the time is being > allocated now to the next GPU that is coming on the market, and nobody > ever cares enough to clean up their technical debt. > > How come the needs for MST DSC support wasn't identified earlier, > blocked on refactoring of the code to use common code, and then that > task made a higher priority? > drm_dp_mst_atomic_check was introduced by Lyude back in January with https://patchwork.freedesktop.org/patch/276405/ as part of https://patchwork.freedesktop.org/series/54031/ At the time Lyude updated i915 and nouveau to use these helpers. amdgpu wasn't updated. > I'm sorta inclined to say no we shouldn't be merging any driver > specific code here, because this is the only point we can push > pressure on to refactor the MST implementation, which I guess > otherwise we'll just keep avoiding until Lyude ends up doing it for > you. > That's fair. I agree that the refactor should be done and I understand where you're coming from. Since David is heading back to school in less than a week I was inclined to see if we can push back a little so he can get his change in. Other than that I don't mind holding off on the merge unless the refactor is done. Adding Mikita who'll pick up DSC stuff from David and will iterate on these patches if necessary and look at the MST refactor. Thanks for keeping us honest. Harry > Dave. > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >