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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <29888a4c-b745-3975-f330-d3a4033cd2a4@linaro.org>
Date:   Tue, 30 May 2023 11:29:51 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Marijn Suijten <marijn.suijten@...ainline.org>,
        Neil Armstrong <neil.armstrong@...aro.org>
Cc:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Caleb Connolly <caleb@...nolly.tech>,
        Jessica Zhang <quic_jesszhan@...cinc.com>,
        Sam Ravnborg <sam@...nborg.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        ~postmarketos/upstreaming@...ts.sr.ht,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>,
        Martin Botka <martin.botka@...ainline.org>,
        Jami Kettunen <jami.kettunen@...ainline.org>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Kuogee Hsieh <quic_khsieh@...cinc.com>
Subject: Re: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia
 XZ3



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