mbox series

[RFC,00/10] drm/panel: Drivers for four Sony CMD-mode (and DSC) panels

Message ID 20230521-drm-panels-sony-v1-0-541c341d6bee@somainline.org
Headers show
Series drm/panel: Drivers for four Sony CMD-mode (and DSC) panels | expand

Message

Marijn Suijten May 21, 2023, 9:23 p.m. UTC
Sending in a bunch of almost-finished (hence RFC) Sony panel drivers
upon drm/msm request, to further discussions around DSC panels and DSI
pclk calculations.  This brings support for the following Sony Xperia
devices:

- Xperia XZ3 (DSC)
- Xperia 1 (DSC)
- Xperia 5
- Xperia 10 II (shared with Xperia 5)
- Xperia 5 II (DSC, 120Hz)

And, since the XZ3 already has all the DT in place to enable the panels
on its smaller XZ2(c) siblings, an extra patch is included to add the
new compatible string and properties to this device .dts.  DTS for other
boards/platforms will come later, after cleaning up preliminary patches
(e.g. DPU catalog additions, SoC/board DTS for the MDSS subsystem, etc).

- File- and compatible names:

  None of my downstream sources describe the product name of the panels
  used here; in few cases the Display-IC is known but for the Xperia XZ3
  Xperia 1 we have to make-do with a vendor name only.

  Naming suggestions definitely welcome; i.e. I'm especially not fond of
  sony-griffin-samsung.c :)

- Panels/drivers featuring multiple modes

  As brought up earlier in #freedreno drm_panel drivers can provide
  multiple modes, but the selected mode is never communicated back to
  the panel.  This either has to be added to the driver, or the drivers
  in question have to be converted to drm_bridges (suggestion from
  #freedreno).  That should allow us to select a mode at runtime, and
  downstream even defines "fast paths" to switch from one mode to the
  next (e.g. when only adjusting the refresh rate) without powering the
  panel off and on again, which we can hopefully support too.

  For now the choice between either mode has been hardcoded behind a
  static const bool.

- DSC

  Not much to discuss here except that "it works" after piecing together
  various series on the lists.  No dependencies to make this series
  apply/compile, though.

- pclk

  The brunt of the discussion is around getting these command mode
  panels functioning at their desired 60Hz or 120Hz refresh rate without
  tearing/artifacts, and without hacks.  Part of that discussion around
  DSC-specific timing adjustments is happening in [1], but the sofef01
  (non-DSC) Driver-IC is also struggling on the Xperia 5 specifically,
  as outlined in that specific patch.  That is currently "addressed"
  with a "porch hack" but should probably have some sort of overhead /
  transfer time taken into account in the MSM DSI driver.

  Let me know what the best place is to collate all the relevant info
  (links to downstream panel DTS, outcomes with different patches and
  tweaks, etc).  A new fd.o drm/msm issue?

[1]: https://gitlab.freedesktop.org/drm/msm/-/issues/24#note_1917707

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
Marijn Suijten (10):
      drm/panel: Clean up SOFEF00 config dependencies
      dt-bindings: display: panel: Describe Sony Xperia XZ3's LGD panel
      drm/panel: Add LGD panel driver for Sony Xperia XZ3
      arm64: dts: qcom: sdm845-akatsuki: Configure OLED panel
      dt-bindings: display: panel: Describe Samsung SOFEF01-M Display-IC
      drm/panel/samsung-sofef01: Add panel driver for Sony Xperia 5 / 10 II
      dt-bindings: display: panel: Describe Samsung SOFEF03-M Display-IC
      drm/panel/samsung-sofef03: Add panel driver for Sony Xperia 5 II
      dt-bindings: display: panel: Describe Sony Xperia 1 display
      drm/panel/sony-griffin-samsung: Add panel driver for Sony Xperia 1

 .../bindings/display/panel/samsung,sofef01-m.yaml  | 109 ++++++
 .../bindings/display/panel/samsung,sofef03-m.yaml  |  73 ++++
 .../bindings/display/panel/sony,akatsuki-lgd.yaml  |  71 ++++
 .../display/panel/sony,griffin-samsung.yaml        |  73 ++++
 .../dts/qcom/sdm845-sony-xperia-tama-akatsuki.dts  |   9 +
 drivers/gpu/drm/panel/Kconfig                      |  52 ++-
 drivers/gpu/drm/panel/Makefile                     |   4 +
 drivers/gpu/drm/panel/panel-samsung-sofef01.c      | 360 ++++++++++++++++++
 drivers/gpu/drm/panel/panel-samsung-sofef03.c      | 423 +++++++++++++++++++++
 drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c    | 362 ++++++++++++++++++
 drivers/gpu/drm/panel/panel-sony-griffin-samsung.c | 410 ++++++++++++++++++++
 11 files changed, 1945 insertions(+), 1 deletion(-)
---
base-commit: dbd91ef4e91c1ce3a24429f5fb3876b7a0306733
change-id: 20230521-drm-panels-sony-3c5ac3218427

Best regards,

Comments

Marijn Suijten May 22, 2023, 10:32 p.m. UTC | #1
On 2023-05-22 04:19:45, Dmitry Baryshkov wrote:
> On 22/05/2023 00:23, Marijn Suijten wrote:
> > This SOFEF01-M Display-IC driver supports two modes with different
> > compatibles to differentiate between slightly different physical sizes
> > (panels) found on the Xperia 5 (6.1") and 10 II (6.0").
> > 
> > It is currently also used to hardcode significantly higher fake porches
> > for the Xperia 5, which are unused in transfers due to this being a
> > command-mode panel but do have an effect on the clock rates set by
> > dsi_host.c.  Without higher clock rates this panel fails to achieve
> > 60fps and has significant tearing artifacts, while the same calculated
> > clock rate works perfectly fine on the Xperia 10 II.

<snip>

> > +/* Sony Xperia 5 (kumano bahamut) */
> > +static const struct drm_display_mode samsung_sofef01_m_bahamut_mode = {
> > +	/*
> > +	 * WARNING: These massive porches are wrong/useless for CMDmode
> > +	 * (and not defined in downstream DTS) but necessary to bump dsi
> > +	 * clocks higher, so that we can achieve proper 60fps without tearing.
> > +	 */
> > +	.clock = (1080 + 156 + 8 + 8) * (2520 + 2393 + 8 + 8) * 60 / 1000,
> > +	.hdisplay = 1080,
> > +	.hsync_start = 1080 + 156,
> > +	.hsync_end = 1080 + 156 + 8,
> > +	.htotal = 1080 + 156 + 8 + 8,
> > +	.vdisplay = 2520,
> > +	.vsync_start = 2520 + 2393,
> > +	.vsync_end = 2520 + 2393 + 8,
> > +	.vtotal = 2520 + 2393 + 8 + 8,
> > +	.width_mm = 61,
> > +	.height_mm = 142,
> > +};
> > +
> > +/* Sony Xperia 10 II (seine pdx201) */
> > +static const struct drm_display_mode samsung_sofef01_m_pdx201_mode = {
> > +	.clock = (1080 + 8 + 8 + 8) * (2520 + 8 + 8 + 8) * 60 / 1000,
> > +	.hdisplay = 1080,
> > +	.hsync_start = 1080 + 8,
> > +	.hsync_end = 1080 + 8 + 8,
> > +	.htotal = 1080 + 8 + 8 + 8,
> > +	.vdisplay = 2520,
> > +	.vsync_start = 2520 + 8,
> > +	.vsync_end = 2520 + 8 + 8,
> > +	.vtotal = 2520 + 8 + 8 + 8,
> > +	.width_mm = 60,
> > +	.height_mm = 139,
> > +};
> > +
> > +static const struct of_device_id samsung_sofef01_m_of_match[] = {
> > +	{ .compatible = "samsung,sofef01-m-bahamut", .data = &samsung_sofef01_m_bahamut_mode },
> > +	{ .compatible = "samsung,sofef01-m-pdx201", .data = &samsung_sofef01_m_pdx201_mode },
> 
> Are there really two panels? Can we use one mode for both usecases?

See the commit description where I explained exactly this: the panels
have different dimensions (6.1" vs 6.0", hence different DPI) and I also
abuse this to hack in higher clock rates via fake porches.

I just ended up on a scary website that supposedly contains the panel
names:

- Xperia 5 (bahamut, 6.1"): AMB609TC01
- Xperia 10 II (pdx201, 6.0"): AMS597UT01

- Marijn
Dmitry Baryshkov May 22, 2023, 10:56 p.m. UTC | #2
On Tue, 23 May 2023 at 01:32, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2023-05-22 04:19:45, Dmitry Baryshkov wrote:
> > On 22/05/2023 00:23, Marijn Suijten wrote:
> > > This SOFEF01-M Display-IC driver supports two modes with different
> > > compatibles to differentiate between slightly different physical sizes
> > > (panels) found on the Xperia 5 (6.1") and 10 II (6.0").
> > >
> > > It is currently also used to hardcode significantly higher fake porches
> > > for the Xperia 5, which are unused in transfers due to this being a
> > > command-mode panel but do have an effect on the clock rates set by
> > > dsi_host.c.  Without higher clock rates this panel fails to achieve
> > > 60fps and has significant tearing artifacts, while the same calculated
> > > clock rate works perfectly fine on the Xperia 10 II.
>
> <snip>
>
> > > +/* Sony Xperia 5 (kumano bahamut) */
> > > +static const struct drm_display_mode samsung_sofef01_m_bahamut_mode = {
> > > +   /*
> > > +    * WARNING: These massive porches are wrong/useless for CMDmode
> > > +    * (and not defined in downstream DTS) but necessary to bump dsi
> > > +    * clocks higher, so that we can achieve proper 60fps without tearing.
> > > +    */
> > > +   .clock = (1080 + 156 + 8 + 8) * (2520 + 2393 + 8 + 8) * 60 / 1000,
> > > +   .hdisplay = 1080,
> > > +   .hsync_start = 1080 + 156,
> > > +   .hsync_end = 1080 + 156 + 8,
> > > +   .htotal = 1080 + 156 + 8 + 8,
> > > +   .vdisplay = 2520,
> > > +   .vsync_start = 2520 + 2393,
> > > +   .vsync_end = 2520 + 2393 + 8,
> > > +   .vtotal = 2520 + 2393 + 8 + 8,
> > > +   .width_mm = 61,
> > > +   .height_mm = 142,
> > > +};
> > > +
> > > +/* Sony Xperia 10 II (seine pdx201) */
> > > +static const struct drm_display_mode samsung_sofef01_m_pdx201_mode = {
> > > +   .clock = (1080 + 8 + 8 + 8) * (2520 + 8 + 8 + 8) * 60 / 1000,
> > > +   .hdisplay = 1080,
> > > +   .hsync_start = 1080 + 8,
> > > +   .hsync_end = 1080 + 8 + 8,
> > > +   .htotal = 1080 + 8 + 8 + 8,
> > > +   .vdisplay = 2520,
> > > +   .vsync_start = 2520 + 8,
> > > +   .vsync_end = 2520 + 8 + 8,
> > > +   .vtotal = 2520 + 8 + 8 + 8,
> > > +   .width_mm = 60,
> > > +   .height_mm = 139,
> > > +};
> > > +
> > > +static const struct of_device_id samsung_sofef01_m_of_match[] = {
> > > +   { .compatible = "samsung,sofef01-m-bahamut", .data = &samsung_sofef01_m_bahamut_mode },
> > > +   { .compatible = "samsung,sofef01-m-pdx201", .data = &samsung_sofef01_m_pdx201_mode },
> >
> > Are there really two panels? Can we use one mode for both usecases?
>
> See the commit description where I explained exactly this: the panels
> have different dimensions (6.1" vs 6.0", hence different DPI) and I also
> abuse this to hack in higher clock rates via fake porches.
>
> I just ended up on a scary website that supposedly contains the panel
> names:
>
> - Xperia 5 (bahamut, 6.1"): AMB609TC01
> - Xperia 10 II (pdx201, 6.0"): AMS597UT01

Great! From the patch description it was not obvious if those are two
different panels or a single panel with slight difference in the glass
cover. With these names in place (well, with two distinct names in
place) it makes sense.
Caleb Connolly May 28, 2023, 10 p.m. UTC | #3
On 21/05/2023 22:23, Marijn Suijten wrote:
> As per the config name this Display IC features a DSI command-mode
> interface (or the command to switch to video mode is not
> known/documented) and does not use any of the video-mode helper
> utilities, hence should not select VIDEOMODE_HELPERS.  In addition it
> uses devm_gpiod_get() and related functions from GPIOLIB.
> 
> Fixes: 5933baa36e26 ("drm/panel/samsung-sofef00: Add panel for OnePlus 6/T devices")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

Reviewed-by: Caleb Connolly <caleb@connolly.tech>
> ---
>  drivers/gpu/drm/panel/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 2b9d6db7860ba..67ef898d133f2 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -608,10 +608,10 @@ config DRM_PANEL_SAMSUNG_S6E8AA0
> 
>  config DRM_PANEL_SAMSUNG_SOFEF00
>  	tristate "Samsung sofef00/s6e3fc2x01 OnePlus 6/6T DSI cmd mode panels"
> +	depends on GPIOLIB
>  	depends on OF
>  	depends on DRM_MIPI_DSI
>  	depends on BACKLIGHT_CLASS_DEVICE
> -	select VIDEOMODE_HELPERS
>  	help
>  	  Say Y or M here if you want to enable support for the Samsung AMOLED
>  	  command mode panels found in the OnePlus 6/6T smartphones.
> 
> --
> 2.40.1
>
Dmitry Baryshkov May 29, 2023, 10:17 p.m. UTC | #4
On 30/05/2023 00: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.
> 
>>> +		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?

I'd say, reword it, so that it becomes more obvious why this TODO can 
not be fixed at this moment.

> 
>>> +	WARN_ON(1440 % dsc->slice_count);
>>> +	dsc->slice_width = 1440 / dsc->slice_count;
> 
> <snip>
> 
> - Marijn
Dmitry Baryshkov May 29, 2023, 10:22 p.m. UTC | #5
On 30/05/2023 00:29, Konrad Dybcio wrote:
> 
> 
> On 29.05.2023 23:21, Marijn Suijten wrote:
>> On 2023-05-22 11:08:12, Neil Armstrong wrote:
>>> On 22/05/2023 03:23, Dmitry Baryshkov wrote:
>>>> On 22/05/2023 00:23, Marijn Suijten wrote:
>>>>> The SOFEF03-M Display-IC paired with an unknown panel in the Sony Xperia
>>>>> 5 II always uses Display Stream Compression 1.1 and features a 60hz and
>>>>> 120hz refresh-rate mode.
>>>>>
>>>>> Co-developed-by: Konrad Dybcio <konrad.dybcio@somainline.org>
>>>>
>>>> Konrad's S-o-b is also required then
>>
>> I am unsure what to include here, since Konrad originally "authored" the
>> commit but I believe it was nothing more than a completely broken and
>> unusable driver spit out by "The mdss panel generator".  This needed
>> enough rewriting that I don't feel like giving it much credit ;)
> Might have been. I won't be mad if you drop this!

I'd say, either add S-o-B, or drop C-D-B. The Co-developed-by should 
always come with the Signed-of-by, otherwise one can not be sure that 
the co-developer didn't copy-paste some super-proprietary stolen code.

> 
> Konrad
>>
>>>>
>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Marijn Suijten May 29, 2023, 10:35 p.m. UTC | #6
On 2023-05-30 01:20:17, Dmitry Baryshkov wrote:
> On 29/05/2023 23:58, Marijn Suijten wrote:
> > On 2023-05-23 01:56:46, Dmitry Baryshkov wrote:
> >> On Tue, 23 May 2023 at 01:32, Marijn Suijten
> >> <marijn.suijten@somainline.org> wrote:
> >>>
> >>> On 2023-05-22 04:19:45, Dmitry Baryshkov wrote:
> >>>> On 22/05/2023 00:23, Marijn Suijten wrote:
> >>>>> This SOFEF01-M Display-IC driver supports two modes with different
> >>>>> compatibles to differentiate between slightly different physical sizes
> >>>>> (panels) found on the Xperia 5 (6.1") and 10 II (6.0").
> >>>>>
> >>>>> It is currently also used to hardcode significantly higher fake porches
> >>>>> for the Xperia 5, which are unused in transfers due to this being a
> >>>>> command-mode panel but do have an effect on the clock rates set by
> >>>>> dsi_host.c.  Without higher clock rates this panel fails to achieve
> >>>>> 60fps and has significant tearing artifacts, while the same calculated
> >>>>> clock rate works perfectly fine on the Xperia 10 II.
> >>>
> >>> <snip>
> >>>
> >>>>> +/* Sony Xperia 5 (kumano bahamut) */
> >>>>> +static const struct drm_display_mode samsung_sofef01_m_bahamut_mode = {
> >>>>> +   /*
> >>>>> +    * WARNING: These massive porches are wrong/useless for CMDmode
> >>>>> +    * (and not defined in downstream DTS) but necessary to bump dsi
> >>>>> +    * clocks higher, so that we can achieve proper 60fps without tearing.
> >>>>> +    */
> >>>>> +   .clock = (1080 + 156 + 8 + 8) * (2520 + 2393 + 8 + 8) * 60 / 1000,
> >>>>> +   .hdisplay = 1080,
> >>>>> +   .hsync_start = 1080 + 156,
> >>>>> +   .hsync_end = 1080 + 156 + 8,
> >>>>> +   .htotal = 1080 + 156 + 8 + 8,
> >>>>> +   .vdisplay = 2520,
> >>>>> +   .vsync_start = 2520 + 2393,
> >>>>> +   .vsync_end = 2520 + 2393 + 8,
> >>>>> +   .vtotal = 2520 + 2393 + 8 + 8,
> >>>>> +   .width_mm = 61,
> >>>>> +   .height_mm = 142,
> >>>>> +};
> >>>>> +
> >>>>> +/* Sony Xperia 10 II (seine pdx201) */
> >>>>> +static const struct drm_display_mode samsung_sofef01_m_pdx201_mode = {
> >>>>> +   .clock = (1080 + 8 + 8 + 8) * (2520 + 8 + 8 + 8) * 60 / 1000,
> >>>>> +   .hdisplay = 1080,
> >>>>> +   .hsync_start = 1080 + 8,
> >>>>> +   .hsync_end = 1080 + 8 + 8,
> >>>>> +   .htotal = 1080 + 8 + 8 + 8,
> >>>>> +   .vdisplay = 2520,
> >>>>> +   .vsync_start = 2520 + 8,
> >>>>> +   .vsync_end = 2520 + 8 + 8,
> >>>>> +   .vtotal = 2520 + 8 + 8 + 8,
> >>>>> +   .width_mm = 60,
> >>>>> +   .height_mm = 139,
> >>>>> +};
> >>>>> +
> >>>>> +static const struct of_device_id samsung_sofef01_m_of_match[] = {
> >>>>> +   { .compatible = "samsung,sofef01-m-bahamut", .data = &samsung_sofef01_m_bahamut_mode },
> >>>>> +   { .compatible = "samsung,sofef01-m-pdx201", .data = &samsung_sofef01_m_pdx201_mode },
> >>>>
> >>>> Are there really two panels? Can we use one mode for both usecases?
> >>>
> >>> See the commit description where I explained exactly this: the panels
> >>> have different dimensions (6.1" vs 6.0", hence different DPI) and I also
> >>> abuse this to hack in higher clock rates via fake porches.
> >>>
> >>> I just ended up on a scary website that supposedly contains the panel
> >>> names:
> >>>
> >>> - Xperia 5 (bahamut, 6.1"): AMB609TC01
> >>> - Xperia 10 II (pdx201, 6.0"): AMS597UT01
> >>
> >> Great! From the patch description it was not obvious if those are two
> >> different panels or a single panel with slight difference in the glass
> >> cover. With these names in place (well, with two distinct names in
> >> place) it makes sense.
> > 
> > For completeness: keep the current single file but embed these panel
> > names as suffix (eg. `samsung,sofef-01-m-am[bs]...`) to the compatible
> > (and document these more explicitly elsewhere)?
> 
> Where do the sofef parts of the name come from? Glancing at other 
> panels, I'd expect something simpler. Maybe:

That is the Driver-IC.  Sorry, I meant sofef01-m without the first dash,
matching the original compatibles.  But can also drop the dash in 01-m
if desired.

> samsung,sofef01m-amb60...

- Marijn
Marijn Suijten May 29, 2023, 10:37 p.m. UTC | #7
On 2023-05-30 01:18:40, Dmitry Baryshkov wrote:
<snip>
> >>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
> >>>>> +    if (ret < 0) {
> >>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
> >>>>> +        return ret;
> >>>>> +    }
> >>>>
> >>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
> >>>
> >>>
> >>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
> >>>
> >>
> >> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
> > 
> > I have never investigated what it takes to split these functions, but
> > some of these panels do show some corruption at startup which may be
> > circumvented by powering the panel on after starting the video stream?
> > 
> > I'm just not sure where to make the split: downstream does describe a
> > qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
> > the latter only contains set_display_on() (not exit_sleep_mode()).
> > It is documented like:
> > 
> >      same as "qcom,mdss-dsi-on-command" except commands are sent after
> >      displaying an image."
> > 
> > So this seems like the right way to split them up, I'll test this out on
> > all submitted panel drivers.
> 
> Interesting enough, Neil suggested that sending all the commands during 
> pre_enable() is the correct sequence (especially for VIDEO mode panels), 
> since not all DSI hosts can send commands after switching to the VIDEO mode.

Note that all these panels and Driver-ICs are command-mode, and/or
programmed to run in command-mode, so there shouldn't be any notion of a
VIDEO stream (any command-mode frame is just an "arbitrary command" as
far as I understood).

- Marijn
Dmitry Baryshkov May 29, 2023, 10:39 p.m. UTC | #8
On 30/05/2023 01:37, Marijn Suijten wrote:
> On 2023-05-30 01:18:40, Dmitry Baryshkov wrote:
> <snip>
>>>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
>>>>>>> +    if (ret < 0) {
>>>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
>>>>>>> +        return ret;
>>>>>>> +    }
>>>>>>
>>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
>>>>>
>>>>>
>>>>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
>>>>>
>>>>
>>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
>>>
>>> I have never investigated what it takes to split these functions, but
>>> some of these panels do show some corruption at startup which may be
>>> circumvented by powering the panel on after starting the video stream?
>>>
>>> I'm just not sure where to make the split: downstream does describe a
>>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
>>> the latter only contains set_display_on() (not exit_sleep_mode()).
>>> It is documented like:
>>>
>>>       same as "qcom,mdss-dsi-on-command" except commands are sent after
>>>       displaying an image."
>>>
>>> So this seems like the right way to split them up, I'll test this out on
>>> all submitted panel drivers.
>>
>> Interesting enough, Neil suggested that sending all the commands during
>> pre_enable() is the correct sequence (especially for VIDEO mode panels),
>> since not all DSI hosts can send commands after switching to the VIDEO mode.
> 
> Note that all these panels and Driver-ICs are command-mode, and/or
> programmed to run in command-mode, so there shouldn't be any notion of a
> VIDEO stream (any command-mode frame is just an "arbitrary command" as
> far as I understood).

Yes, from the data stream point of view. I was talking about the DSI 
host being able to send arbitrary commands or not after enabling the 
video/cmd stream.

> 
> - Marijn
Marijn Suijten May 30, 2023, 6:19 p.m. UTC | #9
On 2023-05-30 14:11:06, Dmitry Baryshkov wrote:
> On Tue, 30 May 2023 at 11:27, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > On 2023-05-30 01:39:10, Dmitry Baryshkov wrote:
> > > On 30/05/2023 01:37, Marijn Suijten wrote:
> > > > On 2023-05-30 01:18:40, Dmitry Baryshkov wrote:
> > > > <snip>
> > > >>>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
> > > >>>>>>> +    if (ret < 0) {
> > > >>>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
> > > >>>>>>> +        return ret;
> > > >>>>>>> +    }
> > > >>>>>>
> > > >>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
> > > >>>>>
> > > >>>>>
> > > >>>>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
> > > >>>>>
> > > >>>>
> > > >>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
> > > >>>
> > > >>> I have never investigated what it takes to split these functions, but
> > > >>> some of these panels do show some corruption at startup which may be
> > > >>> circumvented by powering the panel on after starting the video stream?
> > > >>>
> > > >>> I'm just not sure where to make the split: downstream does describe a
> > > >>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
> > > >>> the latter only contains set_display_on() (not exit_sleep_mode()).
> > > >>> It is documented like:
> > > >>>
> > > >>>       same as "qcom,mdss-dsi-on-command" except commands are sent after
> > > >>>       displaying an image."
> > > >>>
> > > >>> So this seems like the right way to split them up, I'll test this out on
> > > >>> all submitted panel drivers.
> > > >>
> > > >> Interesting enough, Neil suggested that sending all the commands during
> > > >> pre_enable() is the correct sequence (especially for VIDEO mode panels),
> > > >> since not all DSI hosts can send commands after switching to the VIDEO mode.
> > > >
> > > > Note that all these panels and Driver-ICs are command-mode, and/or
> > > > programmed to run in command-mode, so there shouldn't be any notion of a
> > > > VIDEO stream (any command-mode frame is just an "arbitrary command" as
> > > > far as I understood).
> > >
> > > Yes, from the data stream point of view. I was talking about the DSI
> > > host being able to send arbitrary commands or not after enabling the
> > > video/cmd stream.
> >
> > Is this a known limitation of SM8250 then?  Or is the brightness_set
> > issue more likely a "problem" with the panel that the downstream kernel
> > is "somehow" working around or aware of, and I just haven't found
> > how/where it deals with that?
> > (Alternatively we could be "doing it wrong" for other panels but it
> >  turns out to be working anyway)
> 
> Please excuse me for not being explicit enough. Qualcomm hardware
> doesn't have this problem. Thus I was completely unaware of it before
> talking to Neil.
> So, our hardware works in most of the cases.

Also excuse me for mocking the hardware here; it seems quite illogical
for it to not work on this specific device which is more likely a
failure in porting the panel DT to the driver than related to this
specific SoC. There's probably one of the hundred-or-so DT params
responsible for triggering a setting, delay, or other magic sequence
that gets the brightness toggle working.

- Marijn
Dmitry Baryshkov May 30, 2023, 11:16 p.m. UTC | #10
On 30/05/2023 21:13, Marijn Suijten wrote:
> On 2023-05-30 10:54:17, Abhinav Kumar wrote:
>>> On 30/05/2023 00:07, Marijn Suijten wrote:
>>>> On 2023-05-22 15:58:56, Dmitry Baryshkov wrote:
>>>>> On Mon, 22 May 2023 at 12:04, Neil Armstrong
>>>>> <neil.armstrong@linaro.org> wrote:
>>>>>>
>>>>>> On 22/05/2023 03:16, Dmitry Baryshkov wrote:
>>>>>>> On 22/05/2023 00:23, Marijn Suijten wrote:
>>>>>>>> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its
>>>>>>>> Xperia
>>>>>>>> XZ3 (tama akatsuki) phone, with custom DCS commands to match.
>>>>>>>>
>>>>>>>> This panel features Display Stream Compression 1.1.
>>>>>>>>
>>>>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/panel/Kconfig                   |  11 +
>>>>>>>>     drivers/gpu/drm/panel/Makefile                  |   1 +
>>>>>>>>     drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362
>>>>>>>> ++++++++++++++++++++++++
>>>>>>>>     3 files changed, 374 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/panel/Kconfig
>>>>>>>> b/drivers/gpu/drm/panel/Kconfig
>>>>>>>> index 67ef898d133f2..18bd116e78a71 100644
>>>>>>>> --- a/drivers/gpu/drm/panel/Kconfig
>>>>>>>> +++ b/drivers/gpu/drm/panel/Kconfig
>>>>>>>> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM
>>>>>>>>           Say Y here if you want to enable support for the Sony
>>>>>>>> ACX565AKM
>>>>>>>>           800x600 3.5" panel (found on the Nokia N900).
>>>>>>>> +config DRM_PANEL_SONY_AKATSUKI_LGD
>>>>>>>> +    tristate "Sony Xperia XZ3 LGD panel"
>>>>>>>> +    depends on GPIOLIB && OF
>>>>>>>> +    depends on DRM_MIPI_DSI
>>>>>>>> +    depends on BACKLIGHT_CLASS_DEVICE
>>>>>>>> +    help
>>>>>>>> +      Say Y here if you want to enable support for the Sony
>>>>>>>> Xperia XZ3
>>>>>>>> +      1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG
>>>>>>>> Display.
>>>>>>>> +
>>>>>>>> +      This panel uses Display Stream Compression 1.1.
>>>>>>>> +
>>>>>>>>     config DRM_PANEL_SONY_TD4353_JDI
>>>>>>>>         tristate "Sony TD4353 JDI panel"
>>>>>>>>         depends on GPIOLIB && OF
>>>>>>>> diff --git a/drivers/gpu/drm/panel/Makefile
>>>>>>>> b/drivers/gpu/drm/panel/Makefile
>>>>>>>> index ff169781e82d7..85133f73558f3 100644
>>>>>>>> --- a/drivers/gpu/drm/panel/Makefile
>>>>>>>> +++ b/drivers/gpu/drm/panel/Makefile
>>>>>>>> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) +=
>>>>>>>> panel-sitronix-st7701.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) +=
>>>>>>>> panel-sitronix-st7789v.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
>>>>>>>> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) +=
>>>>>>>> panel-sony-akatsuki-lgd.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) +=
>>>>>>>> panel-sony-tulip-truly-nt35521.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
>>>>>>>> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>>>>> b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000000..f55788f963dab
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>>>>> @@ -0,0 +1,362 @@
>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>>>> +/*
>>>>>>>> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
>>>>>>>> + *
>>>>>>>> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#include <linux/backlight.h>
>>>>>>>> +#include <linux/delay.h>
>>>>>>>> +#include <linux/gpio/consumer.h>
>>>>>>>> +#include <linux/module.h>
>>>>>>>> +#include <linux/of.h>
>>>>>>>> +#include <linux/of_device.h>
>>>>>>>> +#include <linux/regulator/consumer.h>
>>>>>>>> +
>>>>>>>> +#include <video/mipi_display.h>
>>>>>>>> +
>>>>>>>> +#include <drm/drm_mipi_dsi.h>
>>>>>>>> +#include <drm/drm_modes.h>
>>>>>>>> +#include <drm/drm_panel.h>
>>>>>>>> +#include <drm/drm_probe_helper.h>
>>>>>>>> +#include <drm/display/drm_dsc.h>
>>>>>>>> +#include <drm/display/drm_dsc_helper.h>
>>>>>>>> +
>>>>>>>> +struct sony_akatsuki_lgd {
>>>>>>>> +    struct drm_panel panel;
>>>>>>>> +    struct mipi_dsi_device *dsi;
>>>>>>>> +    struct regulator *vddio;
>>>>>>>> +    struct gpio_desc *reset_gpio;
>>>>>>>> +    bool prepared;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static inline struct sony_akatsuki_lgd
>>>>>>>> *to_sony_akatsuki_lgd(struct drm_panel *panel)
>>>>>>>> +{
>>>>>>>> +    return container_of(panel, struct sony_akatsuki_lgd, panel);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
>>>>>>>> +{
>>>>>>>> +    struct mipi_dsi_device *dsi = ctx->dsi;
>>>>>>>> +    struct device *dev = &dsi->dev;
>>>>>>>> +    int ret;
>>>>>>>> +
>>>>>>>> +    dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>>>>>>>> +
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
>>>>>>>> +    /* Enable backlight control */
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
>>>>>>>> BIT(5));
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to set column address: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to set page address: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_set_tear_on(dsi,
>>>>>>>> MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to set tear on: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +    msleep(120);
>>>>>>>> +
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>
>>>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() /
>>>>>>> mipi_dsi_dcs_set_display_on() be moved from prepare() to enable()
>>>>>>> part?
>>>>>>
>>>>>>
>>>>>> No, prepare is called before the video stream is started and when
>>>>>> display is still in LPM mode and the mode hasn't been set.
>>>>>>
>>>>>
>>>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting
>>>>> the stream?
>>>>
>>>> I have never investigated what it takes to split these functions, but
>>>> some of these panels do show some corruption at startup which may be
>>>> circumvented by powering the panel on after starting the video stream?
>>>>
>>>> I'm just not sure where to make the split: downstream does describe a
>>>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
>>>> the latter only contains set_display_on() (not exit_sleep_mode()).
>>>> It is documented like:
>>>>
>>>>       same as "qcom,mdss-dsi-on-command" except commands are sent after
>>>>       displaying an image."
>>>>
>>>> So this seems like the right way to split them up, I'll test this out on
>>>> all submitted panel drivers.
>>>
>>> Interesting enough, Neil suggested that sending all the commands during
>>> pre_enable() is the correct sequence (especially for VIDEO mode panels),
>>> since not all DSI hosts can send commands after switching to the VIDEO
>>> mode.
>>>
>>
>> I agree with Neil here.
>>
>> Yes, it does seem natural to think that sending the video stream before
>> sending the on commands would avoid any potential corruption / garbage
>> screen issues.
>>
>> But even from panel side should allow that. I have seen panel ON
>> sequences where some explicitly ask for ON commands before the video stream.
>>
>> So, we cannot really generalize it and needs to be treated on a
>> host-to-host and panel-to-panel basis.
> 
> All four panel drivers in this series have a `post-panel-on` separation
> (containing _just_ the panel_on DCS) and should be implemented with a
> separate .enable callback.
> 
> More importantly: is the API to enable/disable, and separately
> prepare/unprepare the panel exposed to userspace?  And is there an app
> (maybe as part of mesa/drm where modetest lives) that is able to trigger
> these calls to test adequate behaviour?  In the past I've seen panels
> working, until my userspace suspends and turn the panel off, where it
> never comes back up properly after.

No. The prepare/enable (and disable/unrepare) translate to the bridge's 
pre_enable/enable and disable/post_disable callbacks, which are a part 
of modesetting. One can not call them separately.
Maxime Ripard July 5, 2023, 1:29 p.m. UTC | #11
On Wed, Jul 05, 2023 at 03:05:33PM +0200, Neil Armstrong wrote:
> On 05/07/2023 14:04, Maxime Ripard wrote:
> > 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.
> 
> Except some panels supports DSC & non-DSC, Video and Command mode, and
> all that is runtime configurable. How do you handle that ?

In this case, most of the constraints are going to be on the encoder
still so it should be the one driving it. The panel will only care about
which mode has been selected, but it shouldn't be the one driving it,
and thus we still don't really need to expose the host capabilities.

This is very much like HDMI: the encoder knows what the monitor is
capable of, will take a decision based on its capabilities and the
monitor's and will then let the monitor know. But the monitor never
knows what the encoder is truly capable of, nor will it enforce
something.

Maxime
Dmitry Baryshkov July 5, 2023, 1:37 p.m. UTC | #12
On 05/07/2023 16:29, Maxime Ripard wrote:
> On Wed, Jul 05, 2023 at 03:05:33PM +0200, Neil Armstrong wrote:
>> On 05/07/2023 14:04, Maxime Ripard wrote:
>>> 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?

No, I got sidetracked by other issues.

>>>
>>> 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.
>>
>> Except some panels supports DSC & non-DSC, Video and Command mode, and
>> all that is runtime configurable. How do you handle that ?
> 
> In this case, most of the constraints are going to be on the encoder
> still so it should be the one driving it. The panel will only care about
> which mode has been selected, but it shouldn't be the one driving it,
> and thus we still don't really need to expose the host capabilities.

This is an interesting perspective. This means that we can and actually 
have to extend the drm_display_mode with the DSI data and compression 
information.

For example, the panel that supports all four types for the 1080p should 
export several modes:

1920x1080-command
1920x1080-command-DSC
1920x1080-video
1920x1080-video-DSC

where video/command and DSC are some kinds of flags and/or information 
in the drm_display_mode? Ideally DSC also has several sub-flags, which 
denote what kind of configuration is supported by the DSC sink (e.g. 
bpp, yuv, etc).

Another option would be to get this handled via the bus format 
negotiation, but that sounds like worse idea to me.

> This is very much like HDMI: the encoder knows what the monitor is
> capable of, will take a decision based on its capabilities and the
> monitor's and will then let the monitor know. But the monitor never
> knows what the encoder is truly capable of, nor will it enforce
> something.
> 
> Maxime