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 },
};
...
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.
Neil
+ drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc);
+
+ ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
+ if (ret < 0) {
+ dev_err(panel->dev, "failed to transmit PPS: %d\n",
ret);
+ goto fail;
+ }
+ ret = mipi_dsi_compression_mode(ctx->dsi, true);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable compression mode: %d\n",
ret);
+ goto fail;
+ }
+
+ msleep(28);
+ }
+
+ ctx->prepared = true;
+ return 0;
+
+fail:
+ gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+ regulator_disable(ctx->vddio);
+ return ret;
+}
<snip>
+ /* This panel only supports DSC; unconditionally enable it */
On that note I should perhaps reword this.
+ dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
I think double assignments are frowned upon.
Ack.
+ if (!dsc)
+ return -ENOMEM;
+
+ dsc->dsc_version_major = 1;
+ dsc->dsc_version_minor = 1;
+
+ dsc->slice_height = 32;
+ dsc->slice_count = 2;
+ // TODO: Get hdisplay from the mode
Would you like to fix the TODO?
I can't unless either migrating to drm_bridge (is that doable?) or
expand drm_panel. That's a larger task, but I don't think this driver
is the right place to track that desire. Should I drop the comment
entirely or reword it?
+ WARN_ON(1440 % dsc->slice_count);
+ dsc->slice_width = 1440 / dsc->slice_count;
<snip>
- Marijn
