Messages in this thread | | | Date | Tue, 30 May 2023 15:36:04 +0300 | Subject | Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3) | From | Dmitry Baryshkov <> |
| |
On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote: > Il 30/05/23 13:44, Dmitry Baryshkov ha scritto: >> On Tue, 30 May 2023 at 10:24, Neil Armstrong >> <neil.armstrong@linaro.org> 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 think this would damage the reusability of the drivers. The panel >> driver does not actually care if the SoC is SM8350, sunxi-something or >> RCar. >> Instead it cares about host capabilities. >> >> I think instead we should extend mipi_dsi_host: >> >> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0) >> #define MIPI_DSI_HOST_MODE_CMD BIT(1) >> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2) >> // FIXME: do we need to provide additional caps here ? >> >> #define MIPI_DSI_DSC_1_1 BIT(0) >> #define MIPI_DSI_DSC_1_2 BIT(1) >> #define MIPI_DSI_DSC_NATIVE_422 BIT(2) >> #define MIPI_DSI_DSC_NATIVE_420 BIT(3) >> #define MIPI_DSI_DSC_FRAC_BPP BIT(4) >> // etc. >> >> struct mipi_dsi_host { >> // new fields only >> unsigned long mode_flags; >> unsigned long dsc_flags; >> }; >> >> Then the panel driver can adapt itself to the host capabilities and >> (possibly) select one of the internally supported DSC profiles. >> > > I completely agree about extending mipi_dsi_host, other SoCs could reuse > that and > support for DSC panels would become a lot cleaner.
Sounds good. I will wait for one or two more days (to get the possible feedback on fields/flags/etc) and post an RFC patch to dri-devel.
> > For example, on MediaTek DRM there's some support for DSC, more or less > the same > for SPRD DRM and some DSI bridge drivers... having a clean > infrastructure would > definitely help. > > I'm sad I cannot offer testing in that case because despite being sure > that there > are MTK smartphones around with DSI panels using DSC, I have none... and > all of the > Chromebooks are not using DSC anyway (but using DisplayPort compression, > which is > obviously an entirely different beast). > >>> >>> ... >>> 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. >>> > > Usually DDICs support both DSC and non-DSC modes, depending on the initial > programming (read: init commands)... but the usual issue is that many DDICs > are not publicly documented for reasons, so yes, bailing out if DSC is not > supported would be the only option, and would be fine at this point. > > Cheers, > Angelo > >>> We could alternatively match on the DSI controller's dsi->host->dev >>> instead of the SoC root compatible. >>> >>> Neil >> >
-- With best wishes Dmitry
| |