On Wed, May 03, 2023 at 10:36:42AM +0300, Lisovskiy, Stanislav wrote:
On Tue, May 02, 2023 at 05:38:57PM +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
The MST DSC code has a myriad of issues:
- Platform checks are wrong (MST+DSC is TGL+ only IIRC)
- Return values of .mode_valid_ctx() are wrong
- .mode_valid_ctx() assumes bigjoiner might be used, but ther rest of the code doesn't agree
- compressed bpp calculations don't make sense
- FEC handling needs to consider the entire link as opposed to just the single stream. Currently FEC would only get enabled if the first enabled stream is compressed. Also I'm not seeing anything that would account for the FEC overhead in any bandwidth calculations
- PPS SDP is only handled for the first stream via the dig_port hooks, other streams will not be transmittitng any PPS SDPs
- PPS SDP readout is missing (also missing for SST!)
- VDSC readout is missing (also missing for SST!)
The FEC issues is really the big one since we have no way currently to apply such link wide configuration constraints. Changing that is going to require a much bigger rework of the higher level modeset .compute_config() logic. We will also need such a rework to properly distribute the available bandwidth across all the streams on the same link (which is a must to eg. enable deep color).
Also all the things you mentioned are subject for discussion, for example I see that FEC overhead is actually accounted for bpp calculation for instance.
AFAICS FEC is only accounted for in the data M/N calculations, assuming that particular stream happened to be compressed. I'm not sure if that actually matters since at least the link M/N are not even used by the MST sink. I suppose the data M/N might still be used for something though. For any uncompressed stream on the same link the data M/N values will be calculated incorrectly without FEC.
And as mentioned, the FEC bandwidth overhead doesn't seem to be accounted anywhere so no guarantee that we won't try to oversubcribe the link.
And FEC will only be enabled if the first stream to be enabled is compressed, otherwise we will enable the link without FEC and still try to cram other compressed streams through it (albeit without the PPS SDP so who knows what will happen) and that is illegal.
We usually improve things by gradually fixing, because if we act same way towards all wrong code in the driver, we could end up removing the whole i915.
We ususally don't merge code that has this many obvious and/or fundemental issues.
Now, most of the issues I listed above are probably fixable in a way that could be backported to stable kernels, but unfortunately the FEC issue is not one of those. That one will likely need massive amounts of work all over the driver modeset code, making a backport impossible.
So from my side I would nack it, at least until you have a code which handles all of this better - I have no doubt you probably have some ideas in your mind, so lets be constructive at least and propose something better first. This code doesn't cause any regressions, but still provides "some" support to DP MST DSC to say the least and even if that would be removed, if some of those users refer to me, I would probably then just point to this mail discussion everytime.
It seems very likely that it will cause regressions at some point, it just needs a specific multi-display MST setup. The resulting problems will be very confusing to debug since the order in which you enable/disable the outputs will have an impact on what actually goes wrong on account of the FEC and PPS SDP issues. The longer we wait disabling this the harder it will be to deal with those regressions since we the probably can't revert anymore (a straight revert was already not possible) but also can't fix it in a way that can be backported (due to the FEC issues in particular).