* [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
@ 2023-07-18 15:31 Michael Riesch
2023-07-18 15:31 ` [PATCH 1/4] dt-bindings: vendor-prefixes: add jasonic Michael Riesch
` (5 more replies)
0 siblings, 6 replies; 25+ messages in thread
From: Michael Riesch @ 2023-07-18 15:31 UTC (permalink / raw
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, David Airlie, Daniel Vetter, Maxime Ripard,
Miquel Raynal, Sebastian Reichel, Gerald Loacker
Cc: devicetree, linux-kernel, dri-devel, Michael Riesch
Hi all,
This series adds support for the partial display mode to the Sitronix
ST7789V panel driver. This is useful for panels that are partially
occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
for this particular panel is added as well.
Note: This series is already based on
https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
Looking forward to your comments!
---
Michael Riesch (4):
dt-bindings: vendor-prefixes: add jasonic
dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display
drm/panel: sitronix-st7789v: add support for partial mode
drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support
.../bindings/display/panel/sitronix,st7789v.yaml | 1 +
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 67 +++++++++++++++++++++-
3 files changed, 68 insertions(+), 2 deletions(-)
---
base-commit: b43dae411767f34288aa347f26b5ed2dade39469
change-id: 20230718-feature-lcd-panel-26d9f29a7830
Best regards,
--
Michael Riesch <michael.riesch@wolfvision.net>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/4] dt-bindings: vendor-prefixes: add jasonic
2023-07-18 15:31 [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
@ 2023-07-18 15:31 ` Michael Riesch
2023-07-18 15:56 ` Conor Dooley
2023-07-18 15:31 ` [PATCH 2/4] dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display Michael Riesch
` (4 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: Michael Riesch @ 2023-07-18 15:31 UTC (permalink / raw
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, David Airlie, Daniel Vetter, Maxime Ripard,
Miquel Raynal, Sebastian Reichel, Gerald Loacker
Cc: devicetree, linux-kernel, dri-devel, Michael Riesch
Add vendor prefix for Jasonic Technology Ltd., a manufacturer
of custom LCD panels.
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 1e2e51401dc5..1dfafc339ddd 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -677,6 +677,8 @@ patternProperties:
description: iWave Systems Technologies Pvt. Ltd.
"^jadard,.*":
description: Jadard Technology Inc.
+ "^jasonic,.*":
+ description: Jasonic Technology Ltd.
"^jdi,.*":
description: Japan Display Inc.
"^jedec,.*":
--
2.30.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/4] dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display
2023-07-18 15:31 [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
2023-07-18 15:31 ` [PATCH 1/4] dt-bindings: vendor-prefixes: add jasonic Michael Riesch
@ 2023-07-18 15:31 ` Michael Riesch
2023-07-18 16:04 ` Conor Dooley
2023-07-18 15:31 ` [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
` (3 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: Michael Riesch @ 2023-07-18 15:31 UTC (permalink / raw
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, David Airlie, Daniel Vetter, Maxime Ripard,
Miquel Raynal, Sebastian Reichel, Gerald Loacker
Cc: devicetree, linux-kernel, dri-devel, Michael Riesch
Add compatible for the Jasonic Technology Ltd. JT240MHQS-HWT-EK-E3
display.
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
index 905c064cd106..eb1a7256ac32 100644
--- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
+++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
@@ -18,6 +18,7 @@ properties:
enum:
- edt,et028013dma
- inanbo,t28cp45tn89-v17
+ - jasonic,jt240mhqs-hwt-ek-e3
- sitronix,st7789v
reg: true
--
2.30.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-07-18 15:31 [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
2023-07-18 15:31 ` [PATCH 1/4] dt-bindings: vendor-prefixes: add jasonic Michael Riesch
2023-07-18 15:31 ` [PATCH 2/4] dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display Michael Riesch
@ 2023-07-18 15:31 ` Michael Riesch
2023-07-19 6:39 ` Maxime Ripard
2023-08-04 8:41 ` Neil Armstrong
2023-07-18 15:31 ` [PATCH 4/4] drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support Michael Riesch
` (2 subsequent siblings)
5 siblings, 2 replies; 25+ messages in thread
From: Michael Riesch @ 2023-07-18 15:31 UTC (permalink / raw
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, David Airlie, Daniel Vetter, Maxime Ripard,
Miquel Raynal, Sebastian Reichel, Gerald Loacker
Cc: devicetree, linux-kernel, dri-devel, Michael Riesch
The ST7789V controller features support for the partial mode. Here,
the area to be displayed can be restricted in one direction (by default,
in vertical direction). This is useful for panels that are partially
occluded by design. Add support for the partial mode.
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 38 ++++++++++++++++++++++++--
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index d16d17f21d92..729d8d7dbf7f 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -118,6 +118,9 @@ struct st7789_panel_info {
u32 bus_format;
u32 bus_flags;
bool invert_mode;
+ bool partial_mode;
+ u16 partial_start;
+ u16 partial_end;
};
struct st7789v {
@@ -330,9 +333,14 @@ static int st7789v_get_modes(struct drm_panel *panel,
static int st7789v_prepare(struct drm_panel *panel)
{
struct st7789v *ctx = panel_to_st7789v(panel);
- u8 pixel_fmt, polarity;
+ u8 mode, pixel_fmt, polarity;
int ret;
+ if (!ctx->info->partial_mode)
+ mode = ST7789V_RGBCTRL_WO;
+ else
+ mode = 0;
+
switch (ctx->info->bus_format) {
case MEDIA_BUS_FMT_RGB666_1X18:
pixel_fmt = MIPI_DCS_PIXEL_FMT_18BIT;
@@ -472,6 +480,32 @@ static int st7789v_prepare(struct drm_panel *panel)
MIPI_DCS_EXIT_INVERT_MODE));
}
+ if (ctx->info->partial_mode) {
+ u8 area_data[4] = {
+ (ctx->info->partial_start >> 8) & 0xff,
+ (ctx->info->partial_start >> 0) & 0xff,
+ ((ctx->info->partial_end - 1) >> 8) & 0xff,
+ ((ctx->info->partial_end - 1) >> 0) & 0xff,
+ };
+
+ ST7789V_TEST(ret, st7789v_write_command(
+ ctx, MIPI_DCS_ENTER_PARTIAL_MODE));
+
+ ST7789V_TEST(ret, st7789v_write_command(
+ ctx, MIPI_DCS_SET_PAGE_ADDRESS));
+ ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
+ ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
+ ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
+ ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
+
+ ST7789V_TEST(ret, st7789v_write_command(
+ ctx, MIPI_DCS_SET_PARTIAL_ROWS));
+ ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
+ ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
+ ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
+ ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
+ }
+
ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RAMCTRL_CMD));
ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RAMCTRL_DM_RGB |
ST7789V_RAMCTRL_RM_RGB));
@@ -479,7 +513,7 @@ static int st7789v_prepare(struct drm_panel *panel)
ST7789V_RAMCTRL_MAGIC));
ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RGBCTRL_CMD));
- ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_WO |
+ ST7789V_TEST(ret, st7789v_write_data(ctx, mode |
ST7789V_RGBCTRL_RCM(2) |
polarity));
ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
--
2.30.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 4/4] drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support
2023-07-18 15:31 [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
` (2 preceding siblings ...)
2023-07-18 15:31 ` [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
@ 2023-07-18 15:31 ` Michael Riesch
2023-08-02 14:25 ` [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Daniel Vetter
2023-08-03 8:11 ` Neil Armstrong
5 siblings, 0 replies; 25+ messages in thread
From: Michael Riesch @ 2023-07-18 15:31 UTC (permalink / raw
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, David Airlie, Daniel Vetter, Maxime Ripard,
Miquel Raynal, Sebastian Reichel, Gerald Loacker
Cc: devicetree, linux-kernel, dri-devel, Michael Riesch
The Jasonic JT240MHQS-HWT-EK-E3 is a custom panel using the Sitronix
ST7789V controller. While the controller features a resolution of
320x240, only an area of 280x240 is visible by design.
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 29 ++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 729d8d7dbf7f..4c6aed993ba1 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -278,6 +278,21 @@ static const struct drm_display_mode et028013dma_mode = {
.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
};
+static const struct drm_display_mode jt240mhqs_hwt_ek_e3_mode = {
+ .clock = 6000,
+ .hdisplay = 240,
+ .hsync_start = 240 + 28,
+ .hsync_end = 240 + 28 + 10,
+ .htotal = 240 + 28 + 10 + 10,
+ .vdisplay = 280,
+ .vsync_start = 280 + 8,
+ .vsync_end = 280 + 8 + 4,
+ .vtotal = 280 + 8 + 4 + 4,
+ .width_mm = 43,
+ .height_mm = 37,
+ .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
+};
+
static const struct st7789_panel_info default_panel = {
.mode = &default_mode,
.invert_mode = true,
@@ -302,6 +317,17 @@ static const struct st7789_panel_info et028013dma_panel = {
DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
};
+static const struct st7789_panel_info jt240mhqs_hwt_ek_e3_panel = {
+ .mode = &jt240mhqs_hwt_ek_e3_mode,
+ .invert_mode = true,
+ .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+ .bus_flags = DRM_BUS_FLAG_DE_HIGH |
+ DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
+ .partial_mode = true,
+ .partial_start = 38,
+ .partial_end = 318,
+};
+
static int st7789v_get_modes(struct drm_panel *panel,
struct drm_connector *connector)
{
@@ -612,6 +638,7 @@ static const struct spi_device_id st7789v_spi_id[] = {
{ "st7789v", (unsigned long) &default_panel },
{ "t28cp45tn89-v17", (unsigned long) &t28cp45tn89_panel },
{ "et028013dma", (unsigned long) &et028013dma_panel },
+ { "jt240mhqs-hwt-ek-e3", (unsigned long) &jt240mhqs_hwt_ek_e3_panel },
{ }
};
MODULE_DEVICE_TABLE(spi, st7789v_spi_id);
@@ -620,6 +647,8 @@ static const struct of_device_id st7789v_of_match[] = {
{ .compatible = "sitronix,st7789v", .data = &default_panel },
{ .compatible = "inanbo,t28cp45tn89-v17", .data = &t28cp45tn89_panel },
{ .compatible = "edt,et028013dma", .data = &et028013dma_panel },
+ { .compatible = "jasonic,jt240mhqs-hwt-ek-e3",
+ .data = &jt240mhqs_hwt_ek_e3_panel },
{ }
};
MODULE_DEVICE_TABLE(of, st7789v_of_match);
--
2.30.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/4] dt-bindings: vendor-prefixes: add jasonic
2023-07-18 15:31 ` [PATCH 1/4] dt-bindings: vendor-prefixes: add jasonic Michael Riesch
@ 2023-07-18 15:56 ` Conor Dooley
0 siblings, 0 replies; 25+ messages in thread
From: Conor Dooley @ 2023-07-18 15:56 UTC (permalink / raw
To: Michael Riesch
Cc: Neil Armstrong, Conor Dooley, devicetree, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, Maxime Ripard, linux-kernel,
Rob Herring, dri-devel, Krzysztof Kozlowski, Miquel Raynal
[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]
On Tue, Jul 18, 2023 at 05:31:50PM +0200, Michael Riesch wrote:
> Add vendor prefix for Jasonic Technology Ltd., a manufacturer
> of custom LCD panels.
>
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 1e2e51401dc5..1dfafc339ddd 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -677,6 +677,8 @@ patternProperties:
> description: iWave Systems Technologies Pvt. Ltd.
> "^jadard,.*":
> description: Jadard Technology Inc.
> + "^jasonic,.*":
> + description: Jasonic Technology Ltd.
> "^jdi,.*":
> description: Japan Display Inc.
> "^jedec,.*":
>
> --
> 2.30.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display
2023-07-18 15:31 ` [PATCH 2/4] dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display Michael Riesch
@ 2023-07-18 16:04 ` Conor Dooley
0 siblings, 0 replies; 25+ messages in thread
From: Conor Dooley @ 2023-07-18 16:04 UTC (permalink / raw
To: Michael Riesch
Cc: Neil Armstrong, Conor Dooley, devicetree, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, Maxime Ripard, linux-kernel,
Rob Herring, dri-devel, Krzysztof Kozlowski, Miquel Raynal
[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]
On Tue, Jul 18, 2023 at 05:31:51PM +0200, Michael Riesch wrote:
> Add compatible for the Jasonic Technology Ltd. JT240MHQS-HWT-EK-E3
> display.
>
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
> Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> index 905c064cd106..eb1a7256ac32 100644
> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> @@ -18,6 +18,7 @@ properties:
> enum:
> - edt,et028013dma
> - inanbo,t28cp45tn89-v17
> + - jasonic,jt240mhqs-hwt-ek-e3
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Cheers,
Conor.
> - sitronix,st7789v
>
> reg: true
>
> --
> 2.30.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-07-18 15:31 ` [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
@ 2023-07-19 6:39 ` Maxime Ripard
2023-08-02 12:34 ` Michael Riesch
2023-08-04 8:41 ` Neil Armstrong
1 sibling, 1 reply; 25+ messages in thread
From: Maxime Ripard @ 2023-07-19 6:39 UTC (permalink / raw
To: Michael Riesch
Cc: Neil Armstrong, Conor Dooley, devicetree, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Krzysztof Kozlowski, Miquel Raynal
[-- Attachment #1: Type: text/plain, Size: 547 bytes --]
Hi,
On Tue, Jul 18, 2023 at 05:31:52PM +0200, Michael Riesch wrote:
> The ST7789V controller features support for the partial mode. Here,
> the area to be displayed can be restricted in one direction (by default,
> in vertical direction). This is useful for panels that are partially
> occluded by design. Add support for the partial mode.
>
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
We already had that discussion, but I think we shouldn't treat this any
differently than overscan for other output.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-07-19 6:39 ` Maxime Ripard
@ 2023-08-02 12:34 ` Michael Riesch
2023-08-02 12:47 ` Maxime Ripard
0 siblings, 1 reply; 25+ messages in thread
From: Michael Riesch @ 2023-08-02 12:34 UTC (permalink / raw
To: Maxime Ripard
Cc: Neil Armstrong, Conor Dooley, devicetree, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Krzysztof Kozlowski, Miquel Raynal
Hi Maxime,
On 7/19/23 08:39, Maxime Ripard wrote:
> Hi,
>
> On Tue, Jul 18, 2023 at 05:31:52PM +0200, Michael Riesch wrote:
>> The ST7789V controller features support for the partial mode. Here,
>> the area to be displayed can be restricted in one direction (by default,
>> in vertical direction). This is useful for panels that are partially
>> occluded by design. Add support for the partial mode.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>
> We already had that discussion, but I think we shouldn't treat this any
> differently than overscan for other output.
Indeed we had that discussion. For reference, it can be found here:
https://lore.kernel.org/dri-devel/20230329091636.mu6ml3gvw5mvkhm4@penduick/#t
The thing is that I am still clueless how the overscan approach could work.
I found some DRM properties related to overscan/margins and I guess
userspace needs to set those. On my system weston is running. Is weston
in charge of configuring the corresponding output so that the correct
margins are applied? If so, how can this be achieved?
Will DRM handle the properties generically or does the driver need to do
some work as well?
In any case it could make sense to write the partial mode registers and
enter the effective dimensions. At least I have seen this in other panel
drivers.
Thanks and best regards,
Michael
>
> Maxime
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-02 12:34 ` Michael Riesch
@ 2023-08-02 12:47 ` Maxime Ripard
2023-08-02 15:03 ` Michael Riesch
0 siblings, 1 reply; 25+ messages in thread
From: Maxime Ripard @ 2023-08-02 12:47 UTC (permalink / raw
To: Michael Riesch
Cc: Neil Armstrong, Conor Dooley, devicetree, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Krzysztof Kozlowski, Miquel Raynal
[-- Attachment #1: Type: text/plain, Size: 1780 bytes --]
Hi,
On Wed, Aug 02, 2023 at 02:34:28PM +0200, Michael Riesch wrote:
> On 7/19/23 08:39, Maxime Ripard wrote:
> > On Tue, Jul 18, 2023 at 05:31:52PM +0200, Michael Riesch wrote:
> >> The ST7789V controller features support for the partial mode. Here,
> >> the area to be displayed can be restricted in one direction (by default,
> >> in vertical direction). This is useful for panels that are partially
> >> occluded by design. Add support for the partial mode.
> >>
> >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> >
> > We already had that discussion, but I think we shouldn't treat this any
> > differently than overscan for other output.
>
> Indeed we had that discussion. For reference, it can be found here:
> https://lore.kernel.org/dri-devel/20230329091636.mu6ml3gvw5mvkhm4@penduick/#t
> The thing is that I am still clueless how the overscan approach could work.
>
> I found some DRM properties related to overscan/margins and I guess
> userspace needs to set those. On my system weston is running. Is weston
> in charge of configuring the corresponding output so that the correct
> margins are applied? If so, how can this be achieved?
I don't really know Weston, but my guess would be based on some
configuration or user feedback, depending on which case we're in.
We also set the default using some kernel command-line options.
> Will DRM handle the properties generically or does the driver need to do
> some work as well?
What do you mean by generically?
> In any case it could make sense to write the partial mode registers and
> enter the effective dimensions. At least I have seen this in other panel
> drivers.
Sure, it makes sense. It shouldn't come from the DT and be fixed though.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-07-18 15:31 [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
` (3 preceding siblings ...)
2023-07-18 15:31 ` [PATCH 4/4] drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support Michael Riesch
@ 2023-08-02 14:25 ` Daniel Vetter
2023-08-03 8:11 ` Neil Armstrong
5 siblings, 0 replies; 25+ messages in thread
From: Daniel Vetter @ 2023-08-02 14:25 UTC (permalink / raw
To: Michael Riesch
Cc: Neil Armstrong, Conor Dooley, devicetree, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, Maxime Ripard, linux-kernel,
Rob Herring, dri-devel, Krzysztof Kozlowski, Miquel Raynal
On Tue, Jul 18, 2023 at 05:31:49PM +0200, Michael Riesch wrote:
> Hi all,
>
> This series adds support for the partial display mode to the Sitronix
> ST7789V panel driver. This is useful for panels that are partially
> occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> for this particular panel is added as well.
>
> Note: This series is already based on
> https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
>
> Looking forward to your comments!
Summary of my take from a fairly long (and kinda still on-going) irc
discussion:
- Where we do know the exact overscan, the kernel should expose correct
modes and adjust the display pipeline to match if needed when
programming the hardware. Meaning the approach in this patch series.
- For hdmi overscan there's a lot of automagic overscan happening by
default. Drivers can mostly fix this by setting all the right
infoframes, but unfortuantely a very big pile of infoframes is needed.
Assuming drivers actually use the helpers I think only i915 gets them
all, so intel_hdmi_compute_config() at the bottom would be the example
to follow, and maybe some more code to extract from and share.
- That /should/ only leave the really old analog TV and similar horrors
leftover. For those we simply can't even guess the right amount of
overscan (because back then no one cared back then about really seeing
everything), and so that's the only case where we should rely on the
overscan properties. And that case only works when the compositor stack
passes these properties all the way to the user, since only they can
check when the settings are good.
The overscan properties should _not_ be used to fix issues of the previous
kind, that really should all work out of the box as much as possible.
Cheers, Sima
>
> ---
> Michael Riesch (4):
> dt-bindings: vendor-prefixes: add jasonic
> dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display
> drm/panel: sitronix-st7789v: add support for partial mode
> drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support
>
> .../bindings/display/panel/sitronix,st7789v.yaml | 1 +
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 67 +++++++++++++++++++++-
> 3 files changed, 68 insertions(+), 2 deletions(-)
> ---
> base-commit: b43dae411767f34288aa347f26b5ed2dade39469
> change-id: 20230718-feature-lcd-panel-26d9f29a7830
>
> Best regards,
> --
> Michael Riesch <michael.riesch@wolfvision.net>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-02 12:47 ` Maxime Ripard
@ 2023-08-02 15:03 ` Michael Riesch
0 siblings, 0 replies; 25+ messages in thread
From: Michael Riesch @ 2023-08-02 15:03 UTC (permalink / raw
To: Maxime Ripard
Cc: Neil Armstrong, Conor Dooley, devicetree, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Krzysztof Kozlowski, Miquel Raynal
Hi all,
In order to avoid spamming the list, I sparked a discussion in
#dri-devel. FTR the log can be found here:
https://oftc.irclog.whitequark.org/dri-devel/2023-08-02#32360491;
On 8/2/23 14:47, Maxime Ripard wrote:
> Hi,
>
> On Wed, Aug 02, 2023 at 02:34:28PM +0200, Michael Riesch wrote:
>> On 7/19/23 08:39, Maxime Ripard wrote:
>>> On Tue, Jul 18, 2023 at 05:31:52PM +0200, Michael Riesch wrote:
>>>> The ST7789V controller features support for the partial mode. Here,
>>>> the area to be displayed can be restricted in one direction (by default,
>>>> in vertical direction). This is useful for panels that are partially
>>>> occluded by design. Add support for the partial mode.
>>>>
>>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>>>
>>> We already had that discussion, but I think we shouldn't treat this any
>>> differently than overscan for other output.
>>
>> Indeed we had that discussion. For reference, it can be found here:
>> https://lore.kernel.org/dri-devel/20230329091636.mu6ml3gvw5mvkhm4@penduick/#t
>> The thing is that I am still clueless how the overscan approach could work.
>>
>> I found some DRM properties related to overscan/margins and I guess
>> userspace needs to set those. On my system weston is running. Is weston
>> in charge of configuring the corresponding output so that the correct
>> margins are applied? If so, how can this be achieved?
>
> I don't really know Weston, but my guess would be based on some
> configuration or user feedback, depending on which case we're in.
>
> We also set the default using some kernel command-line options.
>
>> Will DRM handle the properties generically or does the driver need to do
>> some work as well?
>
> What do you mean by generically?
I guess my question can be reduced to "What does the driver have to do
to support this overscan thingy?" If the overscan approach is the
preferred one, then I'd appreciate some pointers as to how this could work.
>> In any case it could make sense to write the partial mode registers and
>> enter the effective dimensions. At least I have seen this in other panel
>> drivers.
>
> Sure, it makes sense. It shouldn't come from the DT and be fixed though.
However, as indicated in Daniel Vetter's summary of the IRC discussion,
the overscan properties may not be the preferred solution in this case.
Looking forward to further comments (alternatively, to seeing this patch
series getting applied :-))
Best regards,
Michael
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-07-18 15:31 [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
` (4 preceding siblings ...)
2023-08-02 14:25 ` [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Daniel Vetter
@ 2023-08-03 8:11 ` Neil Armstrong
2023-08-03 8:48 ` Maxime Ripard
5 siblings, 1 reply; 25+ messages in thread
From: Neil Armstrong @ 2023-08-03 8:11 UTC (permalink / raw
To: Maxime Ripard, Daniel Vetter, Michael Riesch, Sam Ravnborg,
Sebastian Reichel, Gerald Loacker
Cc: devicetree, Conor Dooley, linux-kernel, dri-devel, Rob Herring,
Krzysztof Kozlowski, Miquel Raynal
Hi,
On 18/07/2023 17:31, Michael Riesch wrote:
> Hi all,
>
> This series adds support for the partial display mode to the Sitronix
> ST7789V panel driver. This is useful for panels that are partially
> occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> for this particular panel is added as well.
>
> Note: This series is already based on
> https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
I understand Maxime's arguments, but by looking closely at the code,
this doesn't look like an hack at all and uses capabilities of the
panel controller to expose a smaller area without depending on any
changes or hacks on the display controller side which is coherent.
Following's Daniel's summary we cannot compare it to TV overscan
because overscan is only on *some* displays, we can still get 100%
of the picture from the signal.
While here, we cannot, there's physically less pixels on the panel.
If there's no more still a strong nack or pending comments,
I plan to apply those tomorrow.
Thanks,
Neil
>
> Looking forward to your comments!
>
> ---
> Michael Riesch (4):
> dt-bindings: vendor-prefixes: add jasonic
> dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display
> drm/panel: sitronix-st7789v: add support for partial mode
> drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support
>
> .../bindings/display/panel/sitronix,st7789v.yaml | 1 +
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 67 +++++++++++++++++++++-
> 3 files changed, 68 insertions(+), 2 deletions(-)
> ---
> base-commit: b43dae411767f34288aa347f26b5ed2dade39469
> change-id: 20230718-feature-lcd-panel-26d9f29a7830
>
> Best regards,
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 8:11 ` Neil Armstrong
@ 2023-08-03 8:48 ` Maxime Ripard
2023-08-03 8:51 ` Daniel Vetter
0 siblings, 1 reply; 25+ messages in thread
From: Maxime Ripard @ 2023-08-03 8:48 UTC (permalink / raw
To: Neil Armstrong
Cc: devicetree, Conor Dooley, Krzysztof Kozlowski, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Michael Riesch, Miquel Raynal
[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]
On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
> Hi,
>
> On 18/07/2023 17:31, Michael Riesch wrote:
> > Hi all,
> >
> > This series adds support for the partial display mode to the Sitronix
> > ST7789V panel driver. This is useful for panels that are partially
> > occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> > for this particular panel is added as well.
> >
> > Note: This series is already based on
> > https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
>
> I understand Maxime's arguments, but by looking closely at the code,
> this doesn't look like an hack at all and uses capabilities of the
> panel controller to expose a smaller area without depending on any
> changes or hacks on the display controller side which is coherent.
>
> Following's Daniel's summary we cannot compare it to TV overscan
> because overscan is only on *some* displays, we can still get 100%
> of the picture from the signal.
Still disagree on the fact that it only affects some display. But it's
not really relevant for that series.
I think I'll still like to have something clarified before we merge it:
if userspace forces a mode, does it contain the margins or not? I don't
have an opinion there, I just think it should be documented.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 8:48 ` Maxime Ripard
@ 2023-08-03 8:51 ` Daniel Vetter
2023-08-03 9:22 ` Maxime Ripard
0 siblings, 1 reply; 25+ messages in thread
From: Daniel Vetter @ 2023-08-03 8:51 UTC (permalink / raw
To: Maxime Ripard
Cc: Neil Armstrong, Conor Dooley, Krzysztof Kozlowski, devicetree,
Gerald Loacker, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Michael Riesch, Miquel Raynal, Sam Ravnborg
On Thu, Aug 03, 2023 at 10:48:57AM +0200, Maxime Ripard wrote:
> On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
> > Hi,
> >
> > On 18/07/2023 17:31, Michael Riesch wrote:
> > > Hi all,
> > >
> > > This series adds support for the partial display mode to the Sitronix
> > > ST7789V panel driver. This is useful for panels that are partially
> > > occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> > > for this particular panel is added as well.
> > >
> > > Note: This series is already based on
> > > https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
> >
> > I understand Maxime's arguments, but by looking closely at the code,
> > this doesn't look like an hack at all and uses capabilities of the
> > panel controller to expose a smaller area without depending on any
> > changes or hacks on the display controller side which is coherent.
> >
> > Following's Daniel's summary we cannot compare it to TV overscan
> > because overscan is only on *some* displays, we can still get 100%
> > of the picture from the signal.
>
> Still disagree on the fact that it only affects some display. But it's
> not really relevant for that series.
See my 2nd point, from a quick grep aside from i915 hdmi support, no one
else sets all the required hdmi infoframes correctly. Which means on a
compliant hdmi tv, you _should_ get overscan. That's how that stuff is
speced.
Iirc you need to at least set both the VIC and the content type, maybe
even more stuff.
Unless all that stuff is set I'd say it's a kms driver bug if you get
overscan on a hdmi TV.
> I think I'll still like to have something clarified before we merge it:
> if userspace forces a mode, does it contain the margins or not? I don't
> have an opinion there, I just think it should be documented.
The mode comes with the margins, so if userspace does something really
funny then either it gets garbage (as in, part of it's crtc area isn't
visible, or maybe black bars on the screen), or the driver rejects it
(which I think is the case for panels, they only take their mode and
nothing else).
Cheers, Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 8:51 ` Daniel Vetter
@ 2023-08-03 9:22 ` Maxime Ripard
2023-08-03 9:30 ` Neil Armstrong
2023-08-03 10:26 ` Daniel Vetter
0 siblings, 2 replies; 25+ messages in thread
From: Maxime Ripard @ 2023-08-03 9:22 UTC (permalink / raw
To: Neil Armstrong, Michael Riesch, Sam Ravnborg, Sebastian Reichel,
Gerald Loacker, David Airlie, Miquel Raynal, Conor Dooley,
Krzysztof Kozlowski, Rob Herring, devicetree, linux-kernel,
dri-devel
[-- Attachment #1: Type: text/plain, Size: 2925 bytes --]
On Thu, Aug 03, 2023 at 10:51:57AM +0200, Daniel Vetter wrote:
> On Thu, Aug 03, 2023 at 10:48:57AM +0200, Maxime Ripard wrote:
> > On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
> > > Hi,
> > >
> > > On 18/07/2023 17:31, Michael Riesch wrote:
> > > > Hi all,
> > > >
> > > > This series adds support for the partial display mode to the Sitronix
> > > > ST7789V panel driver. This is useful for panels that are partially
> > > > occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> > > > for this particular panel is added as well.
> > > >
> > > > Note: This series is already based on
> > > > https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
> > >
> > > I understand Maxime's arguments, but by looking closely at the code,
> > > this doesn't look like an hack at all and uses capabilities of the
> > > panel controller to expose a smaller area without depending on any
> > > changes or hacks on the display controller side which is coherent.
> > >
> > > Following's Daniel's summary we cannot compare it to TV overscan
> > > because overscan is only on *some* displays, we can still get 100%
> > > of the picture from the signal.
> >
> > Still disagree on the fact that it only affects some display. But it's
> > not really relevant for that series.
>
> See my 2nd point, from a quick grep aside from i915 hdmi support, no one
> else sets all the required hdmi infoframes correctly. Which means on a
> compliant hdmi tv, you _should_ get overscan. That's how that stuff is
> speced.
>
> Iirc you need to at least set both the VIC and the content type, maybe
> even more stuff.
>
> Unless all that stuff is set I'd say it's a kms driver bug if you get
> overscan on a hdmi TV.
I have no doubt that i915 works there. The source of my disagreement is
that if all drivers but one don't do that, then userspace will have to
care. You kind of said it yourself, i915 is kind of the exception there.
The exception can be (and I'm sure it is) right, but still, it deviates
from the norm.
> > I think I'll still like to have something clarified before we merge it:
> > if userspace forces a mode, does it contain the margins or not? I don't
> > have an opinion there, I just think it should be documented.
>
> The mode comes with the margins, so if userspace does something really
> funny then either it gets garbage (as in, part of it's crtc area isn't
> visible, or maybe black bars on the screen), or the driver rejects it
> (which I think is the case for panels, they only take their mode and
> nothing else).
Panels can usually be quite flexible when it comes to the timings they
accept, and we could actually use that to our advantage, but even if we
assume that they have a single mode, I don't think we have anything that
enforces that, either at the framework or documentation levels?
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 9:22 ` Maxime Ripard
@ 2023-08-03 9:30 ` Neil Armstrong
2023-08-03 9:38 ` Maxime Ripard
2023-08-03 10:26 ` Daniel Vetter
1 sibling, 1 reply; 25+ messages in thread
From: Neil Armstrong @ 2023-08-03 9:30 UTC (permalink / raw
To: Maxime Ripard, Michael Riesch, Sam Ravnborg, Sebastian Reichel,
Gerald Loacker, David Airlie, Miquel Raynal, Conor Dooley,
Krzysztof Kozlowski, Rob Herring, devicetree, linux-kernel,
dri-devel
On 03/08/2023 11:22, Maxime Ripard wrote:
> On Thu, Aug 03, 2023 at 10:51:57AM +0200, Daniel Vetter wrote:
>> On Thu, Aug 03, 2023 at 10:48:57AM +0200, Maxime Ripard wrote:
>>> On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
>>>> Hi,
>>>>
>>>> On 18/07/2023 17:31, Michael Riesch wrote:
>>>>> Hi all,
>>>>>
>>>>> This series adds support for the partial display mode to the Sitronix
>>>>> ST7789V panel driver. This is useful for panels that are partially
>>>>> occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
>>>>> for this particular panel is added as well.
>>>>>
>>>>> Note: This series is already based on
>>>>> https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
>>>>
>>>> I understand Maxime's arguments, but by looking closely at the code,
>>>> this doesn't look like an hack at all and uses capabilities of the
>>>> panel controller to expose a smaller area without depending on any
>>>> changes or hacks on the display controller side which is coherent.
>>>>
>>>> Following's Daniel's summary we cannot compare it to TV overscan
>>>> because overscan is only on *some* displays, we can still get 100%
>>>> of the picture from the signal.
>>>
>>> Still disagree on the fact that it only affects some display. But it's
>>> not really relevant for that series.
>>
>> See my 2nd point, from a quick grep aside from i915 hdmi support, no one
>> else sets all the required hdmi infoframes correctly. Which means on a
>> compliant hdmi tv, you _should_ get overscan. That's how that stuff is
>> speced.
>>
>> Iirc you need to at least set both the VIC and the content type, maybe
>> even more stuff.
>>
>> Unless all that stuff is set I'd say it's a kms driver bug if you get
>> overscan on a hdmi TV.
>
> I have no doubt that i915 works there. The source of my disagreement is
> that if all drivers but one don't do that, then userspace will have to
> care. You kind of said it yourself, i915 is kind of the exception there.
>
> The exception can be (and I'm sure it is) right, but still, it deviates
> from the norm.
HDMI spec is hidden behind a paywall, HDMI testing is a mess, HDMI real
implementation on TVs and Displays is mostly broken, and HDMI certification
devices are too expensive... this is mainly why only i915 handles it correctly.
>
>>> I think I'll still like to have something clarified before we merge it:
>>> if userspace forces a mode, does it contain the margins or not? I don't
>>> have an opinion there, I just think it should be documented.
>>
>> The mode comes with the margins, so if userspace does something really
>> funny then either it gets garbage (as in, part of it's crtc area isn't
>> visible, or maybe black bars on the screen), or the driver rejects it
>> (which I think is the case for panels, they only take their mode and
>> nothing else).
>
> Panels can usually be quite flexible when it comes to the timings they
> accept, and we could actually use that to our advantage, but even if we
> assume that they have a single mode, I don't think we have anything that
> enforces that, either at the framework or documentation levels?
Yep, this is why we would need a better atomic based panel API that would
permit us handling dynamic timings for panel and get out of the single-mode
for modern panels.
Neil
>
> Maxime
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 9:30 ` Neil Armstrong
@ 2023-08-03 9:38 ` Maxime Ripard
0 siblings, 0 replies; 25+ messages in thread
From: Maxime Ripard @ 2023-08-03 9:38 UTC (permalink / raw
To: Neil Armstrong
Cc: devicetree, Conor Dooley, Gerald Loacker, Sam Ravnborg,
Sebastian Reichel, dri-devel, linux-kernel, Rob Herring,
Michael Riesch, Krzysztof Kozlowski, Miquel Raynal
[-- Attachment #1: Type: text/plain, Size: 4377 bytes --]
On Thu, Aug 03, 2023 at 11:30:52AM +0200, Neil Armstrong wrote:
> On 03/08/2023 11:22, Maxime Ripard wrote:
> > On Thu, Aug 03, 2023 at 10:51:57AM +0200, Daniel Vetter wrote:
> > > On Thu, Aug 03, 2023 at 10:48:57AM +0200, Maxime Ripard wrote:
> > > > On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
> > > > > Hi,
> > > > >
> > > > > On 18/07/2023 17:31, Michael Riesch wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > This series adds support for the partial display mode to the Sitronix
> > > > > > ST7789V panel driver. This is useful for panels that are partially
> > > > > > occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> > > > > > for this particular panel is added as well.
> > > > > >
> > > > > > Note: This series is already based on
> > > > > > https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
> > > > >
> > > > > I understand Maxime's arguments, but by looking closely at the code,
> > > > > this doesn't look like an hack at all and uses capabilities of the
> > > > > panel controller to expose a smaller area without depending on any
> > > > > changes or hacks on the display controller side which is coherent.
> > > > >
> > > > > Following's Daniel's summary we cannot compare it to TV overscan
> > > > > because overscan is only on *some* displays, we can still get 100%
> > > > > of the picture from the signal.
> > > >
> > > > Still disagree on the fact that it only affects some display. But it's
> > > > not really relevant for that series.
> > >
> > > See my 2nd point, from a quick grep aside from i915 hdmi support, no one
> > > else sets all the required hdmi infoframes correctly. Which means on a
> > > compliant hdmi tv, you _should_ get overscan. That's how that stuff is
> > > speced.
> > >
> > > Iirc you need to at least set both the VIC and the content type, maybe
> > > even more stuff.
> > >
> > > Unless all that stuff is set I'd say it's a kms driver bug if you get
> > > overscan on a hdmi TV.
> >
> > I have no doubt that i915 works there. The source of my disagreement is
> > that if all drivers but one don't do that, then userspace will have to
> > care. You kind of said it yourself, i915 is kind of the exception there.
> >
> > The exception can be (and I'm sure it is) right, but still, it deviates
> > from the norm.
>
> HDMI spec is hidden behind a paywall, HDMI testing is a mess, HDMI real
> implementation on TVs and Displays is mostly broken, and HDMI certification
> devices are too expensive... this is mainly why only i915 handles it correctly.
Sure, I know all that, it's why I was disagreeing with the fact that
it's mostly old news and we shouldn't care anymore. And it could largely
be fixed if i915 was using more helpers in general.
But that's a separate discussion entirely. The point I was trying to
make is that it's still very much the current situation for the vast
majority of drivers, for whatever reason, so we can't really treat as if
it isn't anymore.
> > > > I think I'll still like to have something clarified before we merge it:
> > > > if userspace forces a mode, does it contain the margins or not? I don't
> > > > have an opinion there, I just think it should be documented.
> > >
> > > The mode comes with the margins, so if userspace does something really
> > > funny then either it gets garbage (as in, part of it's crtc area isn't
> > > visible, or maybe black bars on the screen), or the driver rejects it
> > > (which I think is the case for panels, they only take their mode and
> > > nothing else).
> >
> > Panels can usually be quite flexible when it comes to the timings they
> > accept, and we could actually use that to our advantage, but even if we
> > assume that they have a single mode, I don't think we have anything that
> > enforces that, either at the framework or documentation levels?
>
> Yep, this is why we would need a better atomic based panel API that would
> permit us handling dynamic timings for panel and get out of the single-mode
> for modern panels.
Again, I definitely agree on the new API, but that seems a bit out of
scope :)
My point was that we can't expect modes to be the one we provided in
.get_modes. And that's for all the drivers, even i915 doesn't do it (as
far as I could see)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 9:22 ` Maxime Ripard
2023-08-03 9:30 ` Neil Armstrong
@ 2023-08-03 10:26 ` Daniel Vetter
2023-08-03 11:43 ` Maxime Ripard
1 sibling, 1 reply; 25+ messages in thread
From: Daniel Vetter @ 2023-08-03 10:26 UTC (permalink / raw
To: Maxime Ripard
Cc: Neil Armstrong, Conor Dooley, devicetree, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Michael Riesch, Krzysztof Kozlowski, Miquel Raynal
On Thu, 3 Aug 2023 at 11:22, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Thu, Aug 03, 2023 at 10:51:57AM +0200, Daniel Vetter wrote:
> > On Thu, Aug 03, 2023 at 10:48:57AM +0200, Maxime Ripard wrote:
> > > On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
> > > > Hi,
> > > >
> > > > On 18/07/2023 17:31, Michael Riesch wrote:
> > > > > Hi all,
> > > > >
> > > > > This series adds support for the partial display mode to the Sitronix
> > > > > ST7789V panel driver. This is useful for panels that are partially
> > > > > occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> > > > > for this particular panel is added as well.
> > > > >
> > > > > Note: This series is already based on
> > > > > https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
> > > >
> > > > I understand Maxime's arguments, but by looking closely at the code,
> > > > this doesn't look like an hack at all and uses capabilities of the
> > > > panel controller to expose a smaller area without depending on any
> > > > changes or hacks on the display controller side which is coherent.
> > > >
> > > > Following's Daniel's summary we cannot compare it to TV overscan
> > > > because overscan is only on *some* displays, we can still get 100%
> > > > of the picture from the signal.
> > >
> > > Still disagree on the fact that it only affects some display. But it's
> > > not really relevant for that series.
> >
> > See my 2nd point, from a quick grep aside from i915 hdmi support, no one
> > else sets all the required hdmi infoframes correctly. Which means on a
> > compliant hdmi tv, you _should_ get overscan. That's how that stuff is
> > speced.
> >
> > Iirc you need to at least set both the VIC and the content type, maybe
> > even more stuff.
> >
> > Unless all that stuff is set I'd say it's a kms driver bug if you get
> > overscan on a hdmi TV.
>
> I have no doubt that i915 works there. The source of my disagreement is
> that if all drivers but one don't do that, then userspace will have to
> care. You kind of said it yourself, i915 is kind of the exception there.
>
> The exception can be (and I'm sure it is) right, but still, it deviates
> from the norm.
The right fix for these is sending the right infoframes, _not_ trying
to fiddle with overscan margins. Only the kernel can make sure the
right infoframes are sent out. If you try to paper over this in
userspace, you'll make the situation worse, not better (because
fiddling with overscan means you get scaling, and so rescaling
artifacts, and for hard contrasts along pixel lines that'll look like
crap).
So yeah this is a case of "most upstream hdmi drivers are broken".
Please don't try to fix kernel bugs in userspace.
> > > I think I'll still like to have something clarified before we merge it:
> > > if userspace forces a mode, does it contain the margins or not? I don't
> > > have an opinion there, I just think it should be documented.
> >
> > The mode comes with the margins, so if userspace does something really
> > funny then either it gets garbage (as in, part of it's crtc area isn't
> > visible, or maybe black bars on the screen), or the driver rejects it
> > (which I think is the case for panels, they only take their mode and
> > nothing else).
>
> Panels can usually be quite flexible when it comes to the timings they
> accept, and we could actually use that to our advantage, but even if we
> assume that they have a single mode, I don't think we have anything that
> enforces that, either at the framework or documentation levels?
Maybe more bugs? We've been slowly filling out all kinds of atomic kms
validation bugs in core/helper code because as a rule of thumb,
drivers get it wrong. Developers test until things work, then call it
good enough, and very few driver teams make a serious effort in trying
to really validate all invalid input. Because doing that is an
enormous amount of work.
I think for clear-cut cases like drm_panel the fix is to just put more
stricter validation into shared code (and then if we break something,
figure out how we can be sufficiently lenient again).
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 10:26 ` Daniel Vetter
@ 2023-08-03 11:43 ` Maxime Ripard
2023-08-03 12:34 ` Neil Armstrong
2023-08-03 14:55 ` Daniel Vetter
0 siblings, 2 replies; 25+ messages in thread
From: Maxime Ripard @ 2023-08-03 11:43 UTC (permalink / raw
To: Daniel Vetter
Cc: Neil Armstrong, Conor Dooley, devicetree, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Michael Riesch, Krzysztof Kozlowski, Miquel Raynal
[-- Attachment #1: Type: text/plain, Size: 5169 bytes --]
On Thu, Aug 03, 2023 at 12:26:03PM +0200, Daniel Vetter wrote:
> On Thu, 3 Aug 2023 at 11:22, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Thu, Aug 03, 2023 at 10:51:57AM +0200, Daniel Vetter wrote:
> > > On Thu, Aug 03, 2023 at 10:48:57AM +0200, Maxime Ripard wrote:
> > > > On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
> > > > > Hi,
> > > > >
> > > > > On 18/07/2023 17:31, Michael Riesch wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > This series adds support for the partial display mode to the Sitronix
> > > > > > ST7789V panel driver. This is useful for panels that are partially
> > > > > > occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> > > > > > for this particular panel is added as well.
> > > > > >
> > > > > > Note: This series is already based on
> > > > > > https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
> > > > >
> > > > > I understand Maxime's arguments, but by looking closely at the code,
> > > > > this doesn't look like an hack at all and uses capabilities of the
> > > > > panel controller to expose a smaller area without depending on any
> > > > > changes or hacks on the display controller side which is coherent.
> > > > >
> > > > > Following's Daniel's summary we cannot compare it to TV overscan
> > > > > because overscan is only on *some* displays, we can still get 100%
> > > > > of the picture from the signal.
> > > >
> > > > Still disagree on the fact that it only affects some display. But it's
> > > > not really relevant for that series.
> > >
> > > See my 2nd point, from a quick grep aside from i915 hdmi support, no one
> > > else sets all the required hdmi infoframes correctly. Which means on a
> > > compliant hdmi tv, you _should_ get overscan. That's how that stuff is
> > > speced.
> > >
> > > Iirc you need to at least set both the VIC and the content type, maybe
> > > even more stuff.
> > >
> > > Unless all that stuff is set I'd say it's a kms driver bug if you get
> > > overscan on a hdmi TV.
> >
> > I have no doubt that i915 works there. The source of my disagreement is
> > that if all drivers but one don't do that, then userspace will have to
> > care. You kind of said it yourself, i915 is kind of the exception there.
> >
> > The exception can be (and I'm sure it is) right, but still, it deviates
> > from the norm.
>
> The right fix for these is sending the right infoframes, _not_ trying
> to fiddle with overscan margins. Only the kernel can make sure the
> right infoframes are sent out. If you try to paper over this in
> userspace, you'll make the situation worse, not better (because
> fiddling with overscan means you get scaling, and so rescaling
> artifacts, and for hard contrasts along pixel lines that'll look like
> crap).
>
> So yeah this is a case of "most upstream hdmi drivers are broken".
> Please don't try to fix kernel bugs in userspace.
ACK.
> > > > I think I'll still like to have something clarified before we merge it:
> > > > if userspace forces a mode, does it contain the margins or not? I don't
> > > > have an opinion there, I just think it should be documented.
> > >
> > > The mode comes with the margins, so if userspace does something really
> > > funny then either it gets garbage (as in, part of it's crtc area isn't
> > > visible, or maybe black bars on the screen), or the driver rejects it
> > > (which I think is the case for panels, they only take their mode and
> > > nothing else).
> >
> > Panels can usually be quite flexible when it comes to the timings they
> > accept, and we could actually use that to our advantage, but even if we
> > assume that they have a single mode, I don't think we have anything that
> > enforces that, either at the framework or documentation levels?
>
> Maybe more bugs? We've been slowly filling out all kinds of atomic kms
> validation bugs in core/helper code because as a rule of thumb,
> drivers get it wrong. Developers test until things work, then call it
> good enough, and very few driver teams make a serious effort in trying
> to really validate all invalid input. Because doing that is an
> enormous amount of work.
>
> I think for clear-cut cases like drm_panel the fix is to just put more
> stricter validation into shared code (and then if we break something,
> figure out how we can be sufficiently lenient again).
Panels are kind of weird, since they essentially don't exist at all in
the framework so it's difficult to make it handle them or their state.
It's typically handled by encoders directly, so each and every driver
would need to make that check, and from a quick grep, none of them are
(for the reasons you said).
Just like for HDMI, even though we can commit to changing those facts,
it won't happen overnight, so to circle back to that series, I'd like a
comment in the driver when the partial mode is enabled that if userspace
ever pushes a mode different from the expected one, we'll add the margins.
That way, if and when we come back to it, we'll know what the original
intent and semantics were.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 11:43 ` Maxime Ripard
@ 2023-08-03 12:34 ` Neil Armstrong
2023-08-03 12:44 ` Maxime Ripard
2023-08-03 14:55 ` Daniel Vetter
1 sibling, 1 reply; 25+ messages in thread
From: Neil Armstrong @ 2023-08-03 12:34 UTC (permalink / raw
To: Maxime Ripard, Daniel Vetter
Cc: devicetree, Conor Dooley, Gerald Loacker, Sam Ravnborg,
Sebastian Reichel, dri-devel, linux-kernel, Rob Herring,
Michael Riesch, Krzysztof Kozlowski, Miquel Raynal
On 03/08/2023 13:43, Maxime Ripard wrote:
> On Thu, Aug 03, 2023 at 12:26:03PM +0200, Daniel Vetter wrote:
>> On Thu, 3 Aug 2023 at 11:22, Maxime Ripard <mripard@kernel.org> wrote:
>>>
>>> On Thu, Aug 03, 2023 at 10:51:57AM +0200, Daniel Vetter wrote:
>>>> On Thu, Aug 03, 2023 at 10:48:57AM +0200, Maxime Ripard wrote:
>>>>> On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 18/07/2023 17:31, Michael Riesch wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> This series adds support for the partial display mode to the Sitronix
>>>>>>> ST7789V panel driver. This is useful for panels that are partially
>>>>>>> occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
>>>>>>> for this particular panel is added as well.
>>>>>>>
>>>>>>> Note: This series is already based on
>>>>>>> https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
>>>>>>
>>>>>> I understand Maxime's arguments, but by looking closely at the code,
>>>>>> this doesn't look like an hack at all and uses capabilities of the
>>>>>> panel controller to expose a smaller area without depending on any
>>>>>> changes or hacks on the display controller side which is coherent.
>>>>>>
>>>>>> Following's Daniel's summary we cannot compare it to TV overscan
>>>>>> because overscan is only on *some* displays, we can still get 100%
>>>>>> of the picture from the signal.
>>>>>
>>>>> Still disagree on the fact that it only affects some display. But it's
>>>>> not really relevant for that series.
>>>>
>>>> See my 2nd point, from a quick grep aside from i915 hdmi support, no one
>>>> else sets all the required hdmi infoframes correctly. Which means on a
>>>> compliant hdmi tv, you _should_ get overscan. That's how that stuff is
>>>> speced.
>>>>
>>>> Iirc you need to at least set both the VIC and the content type, maybe
>>>> even more stuff.
>>>>
>>>> Unless all that stuff is set I'd say it's a kms driver bug if you get
>>>> overscan on a hdmi TV.
>>>
>>> I have no doubt that i915 works there. The source of my disagreement is
>>> that if all drivers but one don't do that, then userspace will have to
>>> care. You kind of said it yourself, i915 is kind of the exception there.
>>>
>>> The exception can be (and I'm sure it is) right, but still, it deviates
>>> from the norm.
>>
>> The right fix for these is sending the right infoframes, _not_ trying
>> to fiddle with overscan margins. Only the kernel can make sure the
>> right infoframes are sent out. If you try to paper over this in
>> userspace, you'll make the situation worse, not better (because
>> fiddling with overscan means you get scaling, and so rescaling
>> artifacts, and for hard contrasts along pixel lines that'll look like
>> crap).
>>
>> So yeah this is a case of "most upstream hdmi drivers are broken".
>> Please don't try to fix kernel bugs in userspace.
>
> ACK.
>
>>>>> I think I'll still like to have something clarified before we merge it:
>>>>> if userspace forces a mode, does it contain the margins or not? I don't
>>>>> have an opinion there, I just think it should be documented.
>>>>
>>>> The mode comes with the margins, so if userspace does something really
>>>> funny then either it gets garbage (as in, part of it's crtc area isn't
>>>> visible, or maybe black bars on the screen), or the driver rejects it
>>>> (which I think is the case for panels, they only take their mode and
>>>> nothing else).
>>>
>>> Panels can usually be quite flexible when it comes to the timings they
>>> accept, and we could actually use that to our advantage, but even if we
>>> assume that they have a single mode, I don't think we have anything that
>>> enforces that, either at the framework or documentation levels?
>>
>> Maybe more bugs? We've been slowly filling out all kinds of atomic kms
>> validation bugs in core/helper code because as a rule of thumb,
>> drivers get it wrong. Developers test until things work, then call it
>> good enough, and very few driver teams make a serious effort in trying
>> to really validate all invalid input. Because doing that is an
>> enormous amount of work.
>>
>> I think for clear-cut cases like drm_panel the fix is to just put more
>> stricter validation into shared code (and then if we break something,
>> figure out how we can be sufficiently lenient again).
>
> Panels are kind of weird, since they essentially don't exist at all in
> the framework so it's difficult to make it handle them or their state.
>
> It's typically handled by encoders directly, so each and every driver
> would need to make that check, and from a quick grep, none of them are
> (for the reasons you said).
>
> Just like for HDMI, even though we can commit to changing those facts,
> it won't happen overnight, so to circle back to that series, I'd like a
> comment in the driver when the partial mode is enabled that if userspace
> ever pushes a mode different from the expected one, we'll add the margins.
To be fair, a majority of the panel drivers would do the wrong
init of the controller with a different mode because:
- mainly the controller model is unknown
- when it's known the datasheet is missing
- when the datasheet is here, most of the registers are missing
- and most of the time the timings are buried in the init sequence
It's sad but it's the real situation.
Only a few drivers can handle a different mode, and we should perhaps
add a flag when not set rejecting a different mode for those controllers and
mark the few ones who can handle that...
And this should be a first step before adding an atomic Panel API.
Neil
>
> That way, if and when we come back to it, we'll know what the original
> intent and semantics were.
>
> Maxime
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 12:34 ` Neil Armstrong
@ 2023-08-03 12:44 ` Maxime Ripard
0 siblings, 0 replies; 25+ messages in thread
From: Maxime Ripard @ 2023-08-03 12:44 UTC (permalink / raw
To: Neil Armstrong
Cc: devicetree, Conor Dooley, Krzysztof Kozlowski, Gerald Loacker,
Sam Ravnborg, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Michael Riesch, Miquel Raynal
[-- Attachment #1: Type: text/plain, Size: 3491 bytes --]
On Thu, Aug 03, 2023 at 02:34:58PM +0200, Neil Armstrong wrote:
> > > > > > I think I'll still like to have something clarified before we merge it:
> > > > > > if userspace forces a mode, does it contain the margins or not? I don't
> > > > > > have an opinion there, I just think it should be documented.
> > > > >
> > > > > The mode comes with the margins, so if userspace does something really
> > > > > funny then either it gets garbage (as in, part of it's crtc area isn't
> > > > > visible, or maybe black bars on the screen), or the driver rejects it
> > > > > (which I think is the case for panels, they only take their mode and
> > > > > nothing else).
> > > >
> > > > Panels can usually be quite flexible when it comes to the timings they
> > > > accept, and we could actually use that to our advantage, but even if we
> > > > assume that they have a single mode, I don't think we have anything that
> > > > enforces that, either at the framework or documentation levels?
> > >
> > > Maybe more bugs? We've been slowly filling out all kinds of atomic kms
> > > validation bugs in core/helper code because as a rule of thumb,
> > > drivers get it wrong. Developers test until things work, then call it
> > > good enough, and very few driver teams make a serious effort in trying
> > > to really validate all invalid input. Because doing that is an
> > > enormous amount of work.
> > >
> > > I think for clear-cut cases like drm_panel the fix is to just put more
> > > stricter validation into shared code (and then if we break something,
> > > figure out how we can be sufficiently lenient again).
> >
> > Panels are kind of weird, since they essentially don't exist at all in
> > the framework so it's difficult to make it handle them or their state.
> >
> > It's typically handled by encoders directly, so each and every driver
> > would need to make that check, and from a quick grep, none of them are
> > (for the reasons you said).
> >
> > Just like for HDMI, even though we can commit to changing those facts,
> > it won't happen overnight, so to circle back to that series, I'd like a
> > comment in the driver when the partial mode is enabled that if userspace
> > ever pushes a mode different from the expected one, we'll add the margins.
>
> To be fair, a majority of the panel drivers would do the wrong
> init of the controller with a different mode because:
> - mainly the controller model is unknown
> - when it's known the datasheet is missing
> - when the datasheet is here, most of the registers are missing
> - and most of the time the timings are buried in the init sequence
>
> It's sad but it's the real situation.
Again, I agree. As far as I'm aware, none of them add arbitrary numbers
to timings though, so it's easy enough to figure out what the mode is
meant to be: it's the mode. Here, we add some numbers to the mode, so
the interaction with the userspace forcing a mode is less clear.
> Only a few drivers can handle a different mode, and we should perhaps
> add a flag when not set rejecting a different mode for those controllers and
> mark the few ones who can handle that...
> And this should be a first step before adding an atomic Panel API.
I'm really just asking for a comment in the code here.
Everything that you mentioned are improvements that we should have on
our todo list, but I don't see them as pre-requisite for this series and
we get to it later on.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-03 11:43 ` Maxime Ripard
2023-08-03 12:34 ` Neil Armstrong
@ 2023-08-03 14:55 ` Daniel Vetter
1 sibling, 0 replies; 25+ messages in thread
From: Daniel Vetter @ 2023-08-03 14:55 UTC (permalink / raw
To: Maxime Ripard
Cc: Neil Armstrong, Conor Dooley, Krzysztof Kozlowski, devicetree,
Gerald Loacker, Sebastian Reichel, dri-devel, linux-kernel,
Rob Herring, Michael Riesch, Miquel Raynal, Sam Ravnborg
On Thu, Aug 03, 2023 at 01:43:08PM +0200, Maxime Ripard wrote:
> On Thu, Aug 03, 2023 at 12:26:03PM +0200, Daniel Vetter wrote:
> > On Thu, 3 Aug 2023 at 11:22, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Thu, Aug 03, 2023 at 10:51:57AM +0200, Daniel Vetter wrote:
> > > > On Thu, Aug 03, 2023 at 10:48:57AM +0200, Maxime Ripard wrote:
> > > > > On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On 18/07/2023 17:31, Michael Riesch wrote:
> > > > > > > Hi all,
> > > > > > >
> > > > > > > This series adds support for the partial display mode to the Sitronix
> > > > > > > ST7789V panel driver. This is useful for panels that are partially
> > > > > > > occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> > > > > > > for this particular panel is added as well.
> > > > > > >
> > > > > > > Note: This series is already based on
> > > > > > > https://lore.kernel.org/lkml/20230714013756.1546769-1-sre@kernel.org/
> > > > > >
> > > > > > I understand Maxime's arguments, but by looking closely at the code,
> > > > > > this doesn't look like an hack at all and uses capabilities of the
> > > > > > panel controller to expose a smaller area without depending on any
> > > > > > changes or hacks on the display controller side which is coherent.
> > > > > >
> > > > > > Following's Daniel's summary we cannot compare it to TV overscan
> > > > > > because overscan is only on *some* displays, we can still get 100%
> > > > > > of the picture from the signal.
> > > > >
> > > > > Still disagree on the fact that it only affects some display. But it's
> > > > > not really relevant for that series.
> > > >
> > > > See my 2nd point, from a quick grep aside from i915 hdmi support, no one
> > > > else sets all the required hdmi infoframes correctly. Which means on a
> > > > compliant hdmi tv, you _should_ get overscan. That's how that stuff is
> > > > speced.
> > > >
> > > > Iirc you need to at least set both the VIC and the content type, maybe
> > > > even more stuff.
> > > >
> > > > Unless all that stuff is set I'd say it's a kms driver bug if you get
> > > > overscan on a hdmi TV.
> > >
> > > I have no doubt that i915 works there. The source of my disagreement is
> > > that if all drivers but one don't do that, then userspace will have to
> > > care. You kind of said it yourself, i915 is kind of the exception there.
> > >
> > > The exception can be (and I'm sure it is) right, but still, it deviates
> > > from the norm.
> >
> > The right fix for these is sending the right infoframes, _not_ trying
> > to fiddle with overscan margins. Only the kernel can make sure the
> > right infoframes are sent out. If you try to paper over this in
> > userspace, you'll make the situation worse, not better (because
> > fiddling with overscan means you get scaling, and so rescaling
> > artifacts, and for hard contrasts along pixel lines that'll look like
> > crap).
> >
> > So yeah this is a case of "most upstream hdmi drivers are broken".
> > Please don't try to fix kernel bugs in userspace.
>
> ACK.
>
> > > > > I think I'll still like to have something clarified before we merge it:
> > > > > if userspace forces a mode, does it contain the margins or not? I don't
> > > > > have an opinion there, I just think it should be documented.
> > > >
> > > > The mode comes with the margins, so if userspace does something really
> > > > funny then either it gets garbage (as in, part of it's crtc area isn't
> > > > visible, or maybe black bars on the screen), or the driver rejects it
> > > > (which I think is the case for panels, they only take their mode and
> > > > nothing else).
> > >
> > > Panels can usually be quite flexible when it comes to the timings they
> > > accept, and we could actually use that to our advantage, but even if we
> > > assume that they have a single mode, I don't think we have anything that
> > > enforces that, either at the framework or documentation levels?
> >
> > Maybe more bugs? We've been slowly filling out all kinds of atomic kms
> > validation bugs in core/helper code because as a rule of thumb,
> > drivers get it wrong. Developers test until things work, then call it
> > good enough, and very few driver teams make a serious effort in trying
> > to really validate all invalid input. Because doing that is an
> > enormous amount of work.
> >
> > I think for clear-cut cases like drm_panel the fix is to just put more
> > stricter validation into shared code (and then if we break something,
> > figure out how we can be sufficiently lenient again).
>
> Panels are kind of weird, since they essentially don't exist at all in
> the framework so it's difficult to make it handle them or their state.
>
> It's typically handled by encoders directly, so each and every driver
> would need to make that check, and from a quick grep, none of them are
> (for the reasons you said).
I think the panel bridge driver infra is the right spot to put this, and
then push drivers a bit more towards using that.
Because yeah if they hand-roll the panel integration, they'll probably
miss a lot of these details :-/
-Sima
>
> Just like for HDMI, even though we can commit to changing those facts,
> it won't happen overnight, so to circle back to that series, I'd like a
> comment in the driver when the partial mode is enabled that if userspace
> ever pushes a mode different from the expected one, we'll add the margins.
>
> That way, if and when we come back to it, we'll know what the original
> intent and semantics were.
>
> Maxime
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-07-18 15:31 ` [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
2023-07-19 6:39 ` Maxime Ripard
@ 2023-08-04 8:41 ` Neil Armstrong
2023-08-04 9:45 ` Michael Riesch
1 sibling, 1 reply; 25+ messages in thread
From: Neil Armstrong @ 2023-08-04 8:41 UTC (permalink / raw
To: Michael Riesch, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Sam Ravnborg, David Airlie, Daniel Vetter, Maxime Ripard,
Miquel Raynal, Sebastian Reichel, Gerald Loacker
Cc: devicetree, linux-kernel, dri-devel
Hi Michael,
On 18/07/2023 17:31, Michael Riesch wrote:
> The ST7789V controller features support for the partial mode. Here,
> the area to be displayed can be restricted in one direction (by default,
> in vertical direction). This is useful for panels that are partial > occluded by design. Add support for the partial mode.
Could you send a v2 with a comment in the code as Maxime suggests ?
Thanks,
Neil
>
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 38 ++++++++++++++++++++++++--
> 1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index d16d17f21d92..729d8d7dbf7f 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -118,6 +118,9 @@ struct st7789_panel_info {
> u32 bus_format;
> u32 bus_flags;
> bool invert_mode;
> + bool partial_mode;
> + u16 partial_start;
> + u16 partial_end;
> };
>
> struct st7789v {
> @@ -330,9 +333,14 @@ static int st7789v_get_modes(struct drm_panel *panel,
> static int st7789v_prepare(struct drm_panel *panel)
> {
> struct st7789v *ctx = panel_to_st7789v(panel);
> - u8 pixel_fmt, polarity;
> + u8 mode, pixel_fmt, polarity;
> int ret;
>
> + if (!ctx->info->partial_mode)
> + mode = ST7789V_RGBCTRL_WO;
> + else
> + mode = 0;
> +
> switch (ctx->info->bus_format) {
> case MEDIA_BUS_FMT_RGB666_1X18:
> pixel_fmt = MIPI_DCS_PIXEL_FMT_18BIT;
> @@ -472,6 +480,32 @@ static int st7789v_prepare(struct drm_panel *panel)
> MIPI_DCS_EXIT_INVERT_MODE));
> }
>
> + if (ctx->info->partial_mode) {
> + u8 area_data[4] = {
> + (ctx->info->partial_start >> 8) & 0xff,
> + (ctx->info->partial_start >> 0) & 0xff,
> + ((ctx->info->partial_end - 1) >> 8) & 0xff,
> + ((ctx->info->partial_end - 1) >> 0) & 0xff,
> + };
> +
> + ST7789V_TEST(ret, st7789v_write_command(
> + ctx, MIPI_DCS_ENTER_PARTIAL_MODE));
> +
> + ST7789V_TEST(ret, st7789v_write_command(
> + ctx, MIPI_DCS_SET_PAGE_ADDRESS));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
> +
> + ST7789V_TEST(ret, st7789v_write_command(
> + ctx, MIPI_DCS_SET_PARTIAL_ROWS));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
> + }
> +
> ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RAMCTRL_CMD));
> ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RAMCTRL_DM_RGB |
> ST7789V_RAMCTRL_RM_RGB));
> @@ -479,7 +513,7 @@ static int st7789v_prepare(struct drm_panel *panel)
> ST7789V_RAMCTRL_MAGIC));
>
> ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RGBCTRL_CMD));
> - ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_WO |
> + ST7789V_TEST(ret, st7789v_write_data(ctx, mode |
> ST7789V_RGBCTRL_RCM(2) |
> polarity));
> ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
2023-08-04 8:41 ` Neil Armstrong
@ 2023-08-04 9:45 ` Michael Riesch
0 siblings, 0 replies; 25+ messages in thread
From: Michael Riesch @ 2023-08-04 9:45 UTC (permalink / raw
To: neil.armstrong, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Sam Ravnborg, David Airlie, Daniel Vetter, Maxime Ripard,
Miquel Raynal, Sebastian Reichel, Gerald Loacker
Cc: devicetree, linux-kernel, dri-devel
Hi Neil,
On 8/4/23 10:41, Neil Armstrong wrote:
> Hi Michael,
>
> On 18/07/2023 17:31, Michael Riesch wrote:
>> The ST7789V controller features support for the partial mode. Here,
>> the area to be displayed can be restricted in one direction (by default,
>> in vertical direction). This is useful for panels that are partial >
>> occluded by design. Add support for the partial mode.
>
> Could you send a v2 with a comment in the code as Maxime suggests ?
Sure thing! I must admit that I do not understand his concerns exactly,
though.
@Maxime: I can prepare a suggestion but feel free to tell me the exact
wording at the preferred position.
Best regards,
Michael
>
> Thanks,
> Neil
>
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 38
>> ++++++++++++++++++++++++--
>> 1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> index d16d17f21d92..729d8d7dbf7f 100644
>> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> @@ -118,6 +118,9 @@ struct st7789_panel_info {
>> u32 bus_format;
>> u32 bus_flags;
>> bool invert_mode;
>> + bool partial_mode;
>> + u16 partial_start;
>> + u16 partial_end;
>> };
>> struct st7789v {
>> @@ -330,9 +333,14 @@ static int st7789v_get_modes(struct drm_panel
>> *panel,
>> static int st7789v_prepare(struct drm_panel *panel)
>> {
>> struct st7789v *ctx = panel_to_st7789v(panel);
>> - u8 pixel_fmt, polarity;
>> + u8 mode, pixel_fmt, polarity;
>> int ret;
>> + if (!ctx->info->partial_mode)
>> + mode = ST7789V_RGBCTRL_WO;
>> + else
>> + mode = 0;
>> +
>> switch (ctx->info->bus_format) {
>> case MEDIA_BUS_FMT_RGB666_1X18:
>> pixel_fmt = MIPI_DCS_PIXEL_FMT_18BIT;
>> @@ -472,6 +480,32 @@ static int st7789v_prepare(struct drm_panel *panel)
>> MIPI_DCS_EXIT_INVERT_MODE));
>> }
>> + if (ctx->info->partial_mode) {
>> + u8 area_data[4] = {
>> + (ctx->info->partial_start >> 8) & 0xff,
>> + (ctx->info->partial_start >> 0) & 0xff,
>> + ((ctx->info->partial_end - 1) >> 8) & 0xff,
>> + ((ctx->info->partial_end - 1) >> 0) & 0xff,
>> + };
>> +
>> + ST7789V_TEST(ret, st7789v_write_command(
>> + ctx, MIPI_DCS_ENTER_PARTIAL_MODE));
>> +
>> + ST7789V_TEST(ret, st7789v_write_command(
>> + ctx, MIPI_DCS_SET_PAGE_ADDRESS));
>> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
>> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
>> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
>> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
>> +
>> + ST7789V_TEST(ret, st7789v_write_command(
>> + ctx, MIPI_DCS_SET_PARTIAL_ROWS));
>> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
>> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
>> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
>> + ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
>> + }
>> +
>> ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RAMCTRL_CMD));
>> ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RAMCTRL_DM_RGB |
>> ST7789V_RAMCTRL_RM_RGB));
>> @@ -479,7 +513,7 @@ static int st7789v_prepare(struct drm_panel *panel)
>> ST7789V_RAMCTRL_MAGIC));
>> ST7789V_TEST(ret, st7789v_write_command(ctx,
>> ST7789V_RGBCTRL_CMD));
>> - ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_WO |
>> + ST7789V_TEST(ret, st7789v_write_data(ctx, mode |
>> ST7789V_RGBCTRL_RCM(2) |
>> polarity));
>> ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2023-08-04 9:45 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-18 15:31 [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
2023-07-18 15:31 ` [PATCH 1/4] dt-bindings: vendor-prefixes: add jasonic Michael Riesch
2023-07-18 15:56 ` Conor Dooley
2023-07-18 15:31 ` [PATCH 2/4] dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display Michael Riesch
2023-07-18 16:04 ` Conor Dooley
2023-07-18 15:31 ` [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode Michael Riesch
2023-07-19 6:39 ` Maxime Ripard
2023-08-02 12:34 ` Michael Riesch
2023-08-02 12:47 ` Maxime Ripard
2023-08-02 15:03 ` Michael Riesch
2023-08-04 8:41 ` Neil Armstrong
2023-08-04 9:45 ` Michael Riesch
2023-07-18 15:31 ` [PATCH 4/4] drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support Michael Riesch
2023-08-02 14:25 ` [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode Daniel Vetter
2023-08-03 8:11 ` Neil Armstrong
2023-08-03 8:48 ` Maxime Ripard
2023-08-03 8:51 ` Daniel Vetter
2023-08-03 9:22 ` Maxime Ripard
2023-08-03 9:30 ` Neil Armstrong
2023-08-03 9:38 ` Maxime Ripard
2023-08-03 10:26 ` Daniel Vetter
2023-08-03 11:43 ` Maxime Ripard
2023-08-03 12:34 ` Neil Armstrong
2023-08-03 12:44 ` Maxime Ripard
2023-08-03 14:55 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).