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