Re: [PATCH v9 02/13] drm/mediatek: Implement Display Stream Compression support

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 11/4/26 19:51, Roman Vivchar wrote:
> Hi Angelo,
> 
> I have tested this series with my out-of-tree mt6595 (command mode
> panel) and mt6768 (video mode) DRM. There's no regressions, but I
> spotted an issue with kconfig.
> 
> On 9/16/26 14:08, AngeloGioacchino Del Regno wrote:
>> Add a real driver for the Display Stream Compression (DSC) Display
>> Controller IP, implementing support for DSC v1.1 to v1.2.
>>
>> In order to do this, it was necessary to remove the basic DSC IP
>> bypass setup from mtk_ddp_comp: this functionality is retained in
>> the new mtk_disp_dsc driver, which checks if DSC was actually
>> requested by other components (with the only one that currently
>> supports this being DSI) and, if not, it will set BYPASS mode in
>> the DSC IP.
>>
>> Like before, the BYPASS mode is set before starting the DSC IP,
>> but unlike before, this is being done in the component start
>> callback instead of the config one.
>> Notably, the config callback is called by mtk_crtc always
>> immediately before the calling start callback, so the order of
>> register writes is retained.
>> The only real difference is that now this is being done through
>> CPU writes instead of CMDQ, but since that's called only once
>> and since it's just three registers, the performance impact will
>> not be minimal and not even measurable.
>>
>> As anticipated, DSC handling was also introduced in the mtk_dsi
>> driver: when performing dsi_host_attach, the driver now checks
>> if the DSI panel adds the DSC configuration structure to the
>> mipi_dsi_device structure and, if it does, it will store a
>> pointer in the driver-local mtk_dsi structure's `dsc` member.
>>
>> The DSI driver will then check whether the DSC configuration
>> that comes from the panel is valid (in regard to MediaTek DSI)
>> and will call the DRM API's DSC helpers to calculate and set
>> all of the const and RC parameters for the actual DSC setup.
>>
>> For the time being, even though the latest MediaTek SoCs do
>> support DSC v1.2, only DSC v1.1 pre-scr support is implemented
>> as an initial contribution (which is rather big, and 1.2 would
>> make it even bigger - but that can anyway be implemented later).
>>
>> As a last step for validation of DSC parameters in DSI, a check
>> for the hdisplay against DSC slice sidth and one for vdisplay
>> against DSC slice height was added to the mode_valid callback,
>> making sure that H/V are, as expected, multiples of slice W/H.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> ...
> 
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -18,6 +18,8 @@
>>    #include <video/mipi_display.h>
>>    #include <video/videomode.h>
>>    
>> +#include <drm/display/drm_dsc.h>
>> +#include <drm/display/drm_dsc_helper.h>
> 
> ...
> 
>>    static void mtk_dsi_config_vdo_timing_per_frame_lp(struct mtk_dsi *dsi)
>> @@ -565,26 +610,68 @@ static void mtk_dsi_config_vdo_timing_per_line_lp(struct mtk_dsi *dsi)
>>    	writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
>>    }
>>    
>> -static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>> +static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi)
>> +{
>> +	struct drm_dsc_config *dsc = dsi->dsc;
>> +	struct device *dev = dsi->host.dev;
>> +	int ret;
>> +
>> +	if (dsc->bits_per_pixel & GENMASK(3, 0)) {
>> +		dev_err(dev, "Fractional bits_per_pixel not supported\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (dsc->bits_per_component != 8) {
>> +		dev_err(dev, "%u bits per component is not supported\n",
>> +			dsc->bits_per_component);
>> +		return -EINVAL;
>> +	}
>> +
>> +	dsc->simple_422 = false;
>> +	dsc->convert_rgb = true;
>> +	dsc->vbr_enable = false;
>> +
>> +	drm_dsc_set_const_params(dsc);
>> +	drm_dsc_set_rc_buf_thresh(dsc);
>> +
>> +	ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_444);
> 
> If config doesn't have DRM_DISPLAY_DSC_HELPER (selected by few panels),
> linking will fail with "error: undefined symbol: drm_dsc_set_const_params".
> Should MediaTek DRM select it?
> 

Yes it should. I already spotted this a bit of time ago and fixed it locally, but
hey, thanks a lot for that.

Cheers,
Angelo