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

[Konrad Dybcio]

On 30.05.2023 10:41, Marijn Suijten wrote:
> 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).
Yes, a driver should behave predictably, regardless of the platform.

> 
> 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.
I'd say we could assume it's mandatory as of today.

Konrad
> 
>> ...
>> 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