Re: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3

[Marijn Suijten <marijn.suijten@somainline.org>]

On 2023-05-30 09:24:24, Neil Armstrong wrote:
> Hi Marijn, Dmitry, Caleb, Jessica,
> 
> On 29/05/2023 23:11, Marijn Suijten wrote:
> > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> > <snip>
> >>> +	if (ctx->dsi->dsc) {
> >>
> >> dsi->dsc is always set, thus this condition can be dropped.
> > 
> > I want to leave room for possibly running the panel without DSC (at a
> > lower resolution/refresh rate, or at higher power consumption if there
> > is enough BW) by not assigning the pointer, if we get access to panel
> > documentation: probably one of the magic commands sent in this driver
> > controls it but we don't know which.
> 
> I'd like to investigate if DSC should perhaps only be enabled if we
> run non certain platforms/socs ?
> 
> I mean, we don't know if the controller supports DSC and those particular
> DSC parameters so we should probably start adding something like :
> 
> static drm_dsc_config dsc_params_qcom = {}
> 
> static const struct of_device_id panel_of_dsc_params[] = {
> 	{ .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
> 	{ .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
> 	{ .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
> 	{ .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
> };

I'd absolutely hate hardcoding a list of compatible SoC names in a panel
driver.  For one these lists will fall out of date really soon even if
we store this list in a generic place: even the current DPU catalog and
new entries floating on the lists weren't faithfully representing DSC
capabilities (but that's all being / been fixed now).

What's more, most of these panel drivers are "hardcoded" for a specific
(smartphone) device (and SoC...) since we don't (usually) have the
DrIC/panel name nor documentation to make the commands generic enough.
I don't think we should be specific on that end, while being generic on
the DSC side.

That does mean I'll remove the if (dsc) here, as Dmitry noted most of
this driver expects/requires it is enabled.

> ...
> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
> ...
> 	const struct of_device_id *match;
> 
> ...
> 	match = of_match_node(panel_of_dsc_params, of_root);
> 	if (match && match->data) {
> 		dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
> 		memcpy(dsi->dsc, match->data, sizeof(*dsc));
> 	} else {
> 		dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n");
> 	}
> ...
> }
> 
> and probably bail out if it's a DSC only panel.
> 
> We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible.

I'd much rather have the DSI host/controller state whether it is capable
of DSC (likely allowing us to expose different modes for panels that
support toggling DSC), but for starters also validate (in DPU?) that the
pointer is NULL when the hardware does not support it (but maybe that
already happens implicitly somewhere in e.g.
dpu_encoder_virt_atomic_mode_set when finding the DSC blocks).

- Marijn