Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)

From: AngeloGioacchino Del Regno
Date: Wed May 31 2023 - 04:02:41 EST

Il 30/05/23 17:44, Neil Armstrong ha scritto:
On 30/05/2023 14:36, Dmitry Baryshkov wrote:
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@xxxxxxxxxx> 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)

I assume all DSI controller supports Video mode, so it should be a negative here
if for a reason it's not the case.

Either all positive or all negative... and yes I agree that all DSI controllers
support video mode nowadays, but:
- Will that be true for future controllers? (likely yes, but you never know)
- Is there any controller driver not implementing video mode?
- Will there be one in the future?


There should also be a flag to tell if sending LP commands sending while
in HS Video mode is supported.


+1. This is the case for both qcom and mtk.

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

Good, I was waiting until a DSC panel appears on the list (and I failed to be the first), it's now the case.

For VTRD6130, the panel is capable of the 4 modes:
- video mode
- command mode
- video mode & DSC
- command mode & DSC

So it would need such info to enable one of the mode in some order to determine.


Dynamically determining is not trivial, as that depends on multiple variables:
- Availability of the modes (obviously)
- Available lanes
- Available bandwidth per lane
- Available total bandwidth
- Power consumption considerations (DSC IP may be using more or less power
depending on the actual SoC//controller)
- Thermal management: DSC may make no thermal sense as in, more heat output
vs thermal envelope (laptop vs embedded vs handset)
- Others

Hence, the implementation should also provide a way of choosing a preferred mode
on a per-controller basis (DSC or no compression).

Just a few considerations that came to mind with a good sleep.

Cheers!

Thanks,
Neil


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