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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3901 bytes --]

Hi,

On Tue, May 30, 2023 at 03:36:04PM +0300, 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@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.

I just came across that discussion, and couldn't find those patches, did
you ever send them?

Either way, I'm not really sure it's a good idea to multiply the
capabilities flags of the DSI host, and we should just stick to the
spec. If the spec says that we have to support DSC while video is
output, then that's what the panels should expect.

If a host isn't able to provide that, it's a bug and we should fix the
controller driver instead of creating a workaround in the core for
broken drivers.

Another concern I have is that, those broken drivers are usually the
undocumented ones that already have trouble supporting the most trivial
setup. Creating more combinations both at the controller and panel level
will just make it harder for those drivers.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]