Re: [PATCH] drm/panel: sitronix-st7789v: Add check for of_drm_get_panel_orientation
Hi Chen Ni,
On 5/28/24 05:08, Chen Ni wrote:
> Add check for the return value of of_drm_get_panel_orientation() and
> return the error if it fails in order to catch the error.
>
> Fixes: b27c0f6d208d ("drm/panel: sitronix-st7789v: add panel orientation
> support")
> Signed-off-by: Chen Ni
> ---
> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index 88e80fe98112..8b15e225bf37 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -643,7 +643,9 @@ static int st7789v_probe(struct spi_device *spi)
> if (ret)
> return dev_err_probe(dev, ret, "Failed to get backlight\n");
>
> - of_drm_get_panel_orientation(spi->dev.of_node, >orientation);
> + ret = of_drm_get_panel_orientation(spi->dev.of_node, >orientation);
> + if (ret)
> + return dev_err_probe(>dev, ret, "Failed to get
> orientation\n");
>
> drm_panel_add(>panel);
I was worried that this effectively makes "rotation" a required property
(although it is not marked required in the bindings documentation) but
it turns out that of_drm_get_panel_orientation handles a missing
"rotation" property gracefully.
Also, all other panel drivers I checked handle the
of_drm_get_panel_orientation call as you suggested. Nice to see this
becoming aligned.
Reviewed-by: Michael Riesch
Thanks and best regards,
Michael
[PATCH v2 2/4] dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display
Add compatible for the Jasonic Technology Ltd. JT240MHQS-HWT-EK-E3 display. Acked-by: Conor Dooley Signed-off-by: Michael Riesch --- 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 0da4c7e05097..ef162b51d010 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.37.2
[PATCH v2 3/4] drm/panel: sitronix-st7789v: add support for partial mode
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
---
drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 43 --
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 0ded72ed2fcd..ebc9a3bd6db3 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 {
@@ -345,9 +348,14 @@ static enum drm_panel_orientation
st7789v_get_orientation(struct drm_panel *p)
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;
@@ -487,6 +495,37 @@ 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,
+ };
+
+ /* Caution: if userspace ever pushes a mode different from the
+* expected one (i.e., the one advertised by get_modes), we'll
+* add margins.
+*/
+
+ 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));
@@ -494,7 +533,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.37.2
[PATCH v2 4/4] drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support
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
---
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 ebc9a3bd6db3..88e80fe98112 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -279,6 +279,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 = _mode,
.invert_mode = true,
@@ -303,6 +318,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 = _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)
{
@@ -635,6 +661,7 @@ static const struct spi_device_id st7789v_spi_id[] = {
{ "st7789v", (unsigned long) _panel },
{ "t28cp45tn89-v17", (unsigned long) _panel },
{ "et028013dma", (unsigned long) _panel },
+ { "jt240mhqs-hwt-ek-e3", (unsigned long) _hwt_ek_e3_panel },
{ }
};
MODULE_DEVICE_TABLE(spi, st7789v_spi_id);
@@ -643,6 +670,8 @@ static const struct of_device_id st7789v_of_match[] = {
{ .compatible = "sitronix,st7789v", .data = _panel },
{ .compatible = "inanbo,t28cp45tn89-v17", .data = _panel },
{ .compatible = "edt,et028013dma", .data = _panel },
+ { .compatible = "jasonic,jt240mhqs-hwt-ek-e3",
+ .data = _hwt_ek_e3_panel },
{ }
};
MODULE_DEVICE_TABLE(of, st7789v_of_match);
--
2.37.2
[PATCH v2 0/4] drm/panel: sitronix-st7789v: add support for partial mode
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. Looking forward to your comments! --- Changes in v2: - Add comment w.r.t. modes (as requested by Maxime) - Link to v1: https://lore.kernel.org/r/20230718-feature-lcd-panel-v1-0-e9a85d537...@wolfvision.net --- 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 | 72 +- 3 files changed, 73 insertions(+), 2 deletions(-) --- base-commit: e83172ec548d420f2e0b01e80a9a8cbae39bbe29 change-id: 20230718-feature-lcd-panel-26d9f29a7830 Best regards, -- Michael Riesch
[PATCH v2 1/4] dt-bindings: vendor-prefixes: add jasonic
Add vendor prefix for Jasonic Technology Ltd., a manufacturer of custom LCD panels. Acked-by: Conor Dooley Signed-off-by: Michael Riesch --- 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.37.2
Re: [PATCH v2 2/3] drm/panel: sitronix-st7789v: add panel orientation support
Hi Neil,
On 8/4/23 10:40, Neil Armstrong wrote:
> Hi,
>
> On 03/08/2023 22:13, Michael Riesch wrote:
>> Determine the orientation of the display based on the device tree and
>> propagate it.
>>
>> Reviewed-by: Neil Armstrong
>> Signed-off-by: Michael Riesch
>> ---
>> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 18 ++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> index c7cbfe6ca82c..6575f07d49e3 100644
>> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> @@ -116,6 +116,7 @@ struct st7789v {
>> struct spi_device *spi;
>> struct gpio_desc *reset;
>> struct regulator *power;
>> + enum drm_panel_orientation orientation;
>> };
>> enum st7789v_prefix {
>> @@ -170,6 +171,7 @@ static const struct drm_display_mode default_mode = {
>> static int st7789v_get_modes(struct drm_panel *panel,
>> struct drm_connector *connector)
>> {
>> + struct st7789v *ctx = panel_to_st7789v(panel);
>> struct drm_display_mode *mode;
>> mode = drm_mode_duplicate(connector->dev, _mode);
>> @@ -188,9 +190,22 @@ static int st7789v_get_modes(struct drm_panel
>> *panel,
>> connector->display_info.width_mm = 61;
>> connector->display_info.height_mm = 103;
>> + /*
>> + * TODO: Remove once all drm drivers call
>> + * drm_connector_set_orientation_from_panel()
>> + */
>> + drm_connector_set_panel_orientation(connector, ctx->orientation);
>> +
>> return 1;
>> }
>> +static enum drm_panel_orientation st7789v_get_orientation(struct
>> drm_panel *p)
>> +{
>> + struct st7789v *ctx = panel_to_st7789v(p);
>> +
>> + return ctx->orientation;
>> +}
>> +
>> static int st7789v_prepare(struct drm_panel *panel)
>> {
>> struct st7789v *ctx = panel_to_st7789v(panel);
>> @@ -349,6 +364,7 @@ static const struct drm_panel_funcs
>> st7789v_drm_funcs = {
>> .disable = st7789v_disable,
>> .enable = st7789v_enable,
>> .get_modes = st7789v_get_modes,
>> + .get_orientation = st7789v_get_orientation,
>> .prepare = st7789v_prepare,
>> .unprepare = st7789v_unprepare,
>> };
>> @@ -382,6 +398,8 @@ static int st7789v_probe(struct spi_device *spi)
>> if (ret)
>> return ret;
>> + of_drm_get_panel_orientation(spi->dev.of_node, >orientation);
>> +
>> drm_panel_add(>panel);
>> return 0;
>>
>
> This patch doesn't apply clean on drm-misc-next, could you rebase and
> resend ?
Sure! v3 is out.
Best regards,
Michael
>
> Thanks,
> Neil
[PATCH v3 3/3] dt-bindings: display: add rotation property to sitronix,st7789v
The sitronix-st7789v driver now considers the rotation property. Add the property to the documentation. Acked-by: Conor Dooley Reviewed-by: Sebastian Reichel Signed-off-by: Michael Riesch --- Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml index 905c064cd106..0da4c7e05097 100644 --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml @@ -25,6 +25,7 @@ properties: power-supply: true backlight: true port: true + rotation: true spi-cpha: true spi-cpol: true @@ -58,6 +59,7 @@ examples: reset-gpios = < 6 11 GPIO_ACTIVE_LOW>; backlight = <_bl>; power-supply = <>; +rotation = <180>; spi-max-frequency = <10>; spi-cpol; spi-cpha; -- 2.37.2
[PATCH v3 0/3] drm/panel: sitronix-st7789v: add panel orientation support
Hi all, This series adds support for orientation specification in the device tree to the Sitronix ST7789V panel driver. This is can be seen as reduced version of [0] (some things of [0] have been implemented in more general fashion in the scope of [1], other things have been rejected). Looking forward to your comments! [0] https://lore.kernel.org/lkml/20230314115644.3775169-1-gerald.loac...@wolfvision.net/ [1] https://lore.kernel.org/lkml/20230714013756.1546769-1-...@kernel.org/ --- Changes in v3: - Rebase onto drm-misc-next. - Link to v2: https://lore.kernel.org/r/20230718-feature-st7789v-v2-0-207cb1bae...@wolfvision.net Changes in v2: - Move indentation fix to separate patch (as suggested by Neil) - Link to v1: https://lore.kernel.org/r/20230718-feature-st7789v-v1-0-76d6ca9b3...@wolfvision.net --- Michael Riesch (3): drm/panel: sitronix-st7789v: fix indentation in drm_panel_funcs drm/panel: sitronix-st7789v: add panel orientation support dt-bindings: display: add rotation property to sitronix,st7789v .../bindings/display/panel/sitronix,st7789v.yaml | 2 ++ drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 27 ++ 2 files changed, 24 insertions(+), 5 deletions(-) --- base-commit: 6db96c7703edd6e37da8ca571dfe5e1ecb6010c1 change-id: 20230718-feature-st7789v-4d0c2c6e2429 Best regards, -- Michael Riesch
[PATCH v3 2/3] drm/panel: sitronix-st7789v: add panel orientation support
Determine the orientation of the display based on the device tree and
propagate it.
Reviewed-by: Neil Armstrong
Reviewed-by: Sebastian Reichel
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index bc8df51224de..0ded72ed2fcd 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -126,6 +126,7 @@ struct st7789v {
struct spi_device *spi;
struct gpio_desc *reset;
struct regulator *power;
+ enum drm_panel_orientation orientation;
};
enum st7789v_prefix {
@@ -325,9 +326,22 @@ static int st7789v_get_modes(struct drm_panel *panel,
drm_display_info_set_bus_formats(>display_info,
>info->bus_format, 1);
+ /*
+* TODO: Remove once all drm drivers call
+* drm_connector_set_orientation_from_panel()
+*/
+ drm_connector_set_panel_orientation(connector, ctx->orientation);
+
return 1;
}
+static enum drm_panel_orientation st7789v_get_orientation(struct drm_panel *p)
+{
+ struct st7789v *ctx = panel_to_st7789v(p);
+
+ return ctx->orientation;
+}
+
static int st7789v_prepare(struct drm_panel *panel)
{
struct st7789v *ctx = panel_to_st7789v(panel);
@@ -522,6 +536,7 @@ static const struct drm_panel_funcs st7789v_drm_funcs = {
.disable = st7789v_disable,
.enable = st7789v_enable,
.get_modes = st7789v_get_modes,
+ .get_orientation = st7789v_get_orientation,
.prepare = st7789v_prepare,
.unprepare = st7789v_unprepare,
};
@@ -563,6 +578,8 @@ static int st7789v_probe(struct spi_device *spi)
if (ret)
return dev_err_probe(dev, ret, "Failed to get backlight\n");
+ of_drm_get_panel_orientation(spi->dev.of_node, >orientation);
+
drm_panel_add(>panel);
return 0;
--
2.37.2
[PATCH v3 1/3] drm/panel: sitronix-st7789v: fix indentation in drm_panel_funcs
Fix indentation of the callbacks in struct drm_panel_funcs.
No functional changes.
Reviewed-by: Sebastian Reichel
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index dc010d87a9ef..bc8df51224de 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -519,11 +519,11 @@ static int st7789v_unprepare(struct drm_panel *panel)
}
static const struct drm_panel_funcs st7789v_drm_funcs = {
- .disable= st7789v_disable,
- .enable = st7789v_enable,
- .get_modes = st7789v_get_modes,
- .prepare= st7789v_prepare,
- .unprepare = st7789v_unprepare,
+ .disable = st7789v_disable,
+ .enable = st7789v_enable,
+ .get_modes = st7789v_get_modes,
+ .prepare = st7789v_prepare,
+ .unprepare = st7789v_unprepare,
};
static int st7789v_probe(struct spi_device *spi)
--
2.37.2
Re: [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
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
>> ---
>> 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)));
>>
>
[PATCH v2 2/3] drm/panel: sitronix-st7789v: add panel orientation support
Determine the orientation of the display based on the device tree and
propagate it.
Reviewed-by: Neil Armstrong
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index c7cbfe6ca82c..6575f07d49e3 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -116,6 +116,7 @@ struct st7789v {
struct spi_device *spi;
struct gpio_desc *reset;
struct regulator *power;
+ enum drm_panel_orientation orientation;
};
enum st7789v_prefix {
@@ -170,6 +171,7 @@ static const struct drm_display_mode default_mode = {
static int st7789v_get_modes(struct drm_panel *panel,
struct drm_connector *connector)
{
+ struct st7789v *ctx = panel_to_st7789v(panel);
struct drm_display_mode *mode;
mode = drm_mode_duplicate(connector->dev, _mode);
@@ -188,9 +190,22 @@ static int st7789v_get_modes(struct drm_panel *panel,
connector->display_info.width_mm = 61;
connector->display_info.height_mm = 103;
+ /*
+* TODO: Remove once all drm drivers call
+* drm_connector_set_orientation_from_panel()
+*/
+ drm_connector_set_panel_orientation(connector, ctx->orientation);
+
return 1;
}
+static enum drm_panel_orientation st7789v_get_orientation(struct drm_panel *p)
+{
+ struct st7789v *ctx = panel_to_st7789v(p);
+
+ return ctx->orientation;
+}
+
static int st7789v_prepare(struct drm_panel *panel)
{
struct st7789v *ctx = panel_to_st7789v(panel);
@@ -349,6 +364,7 @@ static const struct drm_panel_funcs st7789v_drm_funcs = {
.disable = st7789v_disable,
.enable = st7789v_enable,
.get_modes = st7789v_get_modes,
+ .get_orientation = st7789v_get_orientation,
.prepare = st7789v_prepare,
.unprepare = st7789v_unprepare,
};
@@ -382,6 +398,8 @@ static int st7789v_probe(struct spi_device *spi)
if (ret)
return ret;
+ of_drm_get_panel_orientation(spi->dev.of_node, >orientation);
+
drm_panel_add(>panel);
return 0;
--
2.37.2
[PATCH v2 1/3] drm/panel: sitronix-st7789v: fix indentation in drm_panel_funcs
Fix indentation of the callbacks in struct drm_panel_funcs.
No functional changes.
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index bbc4569cbcdc..c7cbfe6ca82c 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -346,11 +346,11 @@ static int st7789v_unprepare(struct drm_panel *panel)
}
static const struct drm_panel_funcs st7789v_drm_funcs = {
- .disable= st7789v_disable,
- .enable = st7789v_enable,
- .get_modes = st7789v_get_modes,
- .prepare= st7789v_prepare,
- .unprepare = st7789v_unprepare,
+ .disable = st7789v_disable,
+ .enable = st7789v_enable,
+ .get_modes = st7789v_get_modes,
+ .prepare = st7789v_prepare,
+ .unprepare = st7789v_unprepare,
};
static int st7789v_probe(struct spi_device *spi)
--
2.37.2
[PATCH v2 3/3] dt-bindings: display: add rotation property to sitronix,st7789v
The sitronix-st7789v driver now considers the rotation property. Add the property to the documentation. Acked-by: Conor Dooley Signed-off-by: Michael Riesch --- Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml index fa6556363cca..694d7f771d0c 100644 --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml @@ -22,6 +22,7 @@ properties: power-supply: true backlight: true port: true + rotation: true spi-cpha: true spi-cpol: true @@ -52,6 +53,7 @@ examples: reset-gpios = < 6 11 GPIO_ACTIVE_LOW>; backlight = <_bl>; power-supply = <>; +rotation = <180>; spi-max-frequency = <10>; spi-cpol; spi-cpha; -- 2.37.2
[PATCH v2 0/3] drm/panel: sitronix-st7789v: add panel orientation support
Hi all, This series adds support for orientation specification in the device tree to the Sitronix ST7789V panel driver. This is can be seen as reduced version of [0] (some things of [0] have been implemented in more general fashion in the scope of [1], other things have been rejected). Looking forward to your comments! [0] https://lore.kernel.org/lkml/20230314115644.3775169-1-gerald.loac...@wolfvision.net/ [1] https://lore.kernel.org/lkml/20230714013756.1546769-1-...@kernel.org/ --- Changes in v2: - Move indentation fix to separate patch (as suggested by Neil) - Link to v1: https://lore.kernel.org/r/20230718-feature-st7789v-v1-0-76d6ca9b3...@wolfvision.net --- Michael Riesch (3): drm/panel: sitronix-st7789v: fix indentation in drm_panel_funcs drm/panel: sitronix-st7789v: add panel orientation support dt-bindings: display: add rotation property to sitronix,st7789v .../bindings/display/panel/sitronix,st7789v.yaml | 2 ++ drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 28 ++ 2 files changed, 25 insertions(+), 5 deletions(-) --- base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 change-id: 20230718-feature-st7789v-4d0c2c6e2429 Best regards, -- Michael Riesch
Re: [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
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 >>> >>> 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
Re: [PATCH 1/2] drm/panel: sitronix-st7789v: add panel orientation support
Hi Neil,
On 8/2/23 14:39, Neil Armstrong wrote:
> On 18/07/2023 17:12, Michael Riesch wrote:
>> Determine the orientation of the display based on the device tree and
>> propagate it.
>>
>> While at it, fix the indentation in the struct drm_panel_funcs.
>>
>> Signed-off-by: Michael Riesch
>> ---
>> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 28
>> +-
>> 1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> index bbc4569cbcdc..6575f07d49e3 100644
>> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> @@ -116,6 +116,7 @@ struct st7789v {
>> struct spi_device *spi;
>> struct gpio_desc *reset;
>> struct regulator *power;
>> + enum drm_panel_orientation orientation;
>> };
>> enum st7789v_prefix {
>> @@ -170,6 +171,7 @@ static const struct drm_display_mode default_mode = {
>> static int st7789v_get_modes(struct drm_panel *panel,
>> struct drm_connector *connector)
>> {
>> + struct st7789v *ctx = panel_to_st7789v(panel);
>> struct drm_display_mode *mode;
>> mode = drm_mode_duplicate(connector->dev, _mode);
>> @@ -188,9 +190,22 @@ static int st7789v_get_modes(struct drm_panel
>> *panel,
>> connector->display_info.width_mm = 61;
>> connector->display_info.height_mm = 103;
>> + /*
>> + * TODO: Remove once all drm drivers call
>> + * drm_connector_set_orientation_from_panel()
>> + */
>> + drm_connector_set_panel_orientation(connector, ctx->orientation);
>> +
>> return 1;
>> }
>> +static enum drm_panel_orientation st7789v_get_orientation(struct
>> drm_panel *p)
>> +{
>> + struct st7789v *ctx = panel_to_st7789v(p);
>> +
>> + return ctx->orientation;
>> +}
>> +
>> static int st7789v_prepare(struct drm_panel *panel)
>> {
>> struct st7789v *ctx = panel_to_st7789v(panel);
>> @@ -346,11 +361,12 @@ static int st7789v_unprepare(struct drm_panel
>> *panel)
>> }
>> static const struct drm_panel_funcs st7789v_drm_funcs = {
>> - .disable = st7789v_disable,
>> - .enable = st7789v_enable,
>> - .get_modes = st7789v_get_modes,
>> - .prepare = st7789v_prepare,
>> - .unprepare = st7789v_unprepare,
>> + .disable = st7789v_disable,
>> + .enable = st7789v_enable,
>> + .get_modes = st7789v_get_modes,
>> + .get_orientation = st7789v_get_orientation,
>> + .prepare = st7789v_prepare,
>> + .unprepare = st7789v_unprepare,
>
> Changing the indentation of the whole block is a spurious change,
> either change it in a separate patch or use the current indentation
> style...
OK, if we agree that the indentation should be changed I'll be happy to
move the change to an extra patch.
>> };
>> static int st7789v_probe(struct spi_device *spi)
>> @@ -382,6 +398,8 @@ static int st7789v_probe(struct spi_device *spi)
>> if (ret)
>> return ret;
>> + of_drm_get_panel_orientation(spi->dev.of_node, >orientation);
>> +
>> drm_panel_add(>panel);
>> return 0;
>>
>
> With this changed:
>
> Reviewed-by: Neil Armstrong
Thanks!
Best regards,
Michael
>
> Thanks,
> Neil
>
Re: [PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
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 > > 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
Re: [PATCH 0/2] drm/panel: sitronix-st7789v: add panel orientation support
Hi all, On 7/18/23 17:12, Michael Riesch wrote: > Hi all, > > This series adds support for orientation specification in the device > tree to the Sitronix ST7789V panel driver. > > This is can be seen as reduced version of [0] (some things of [0] have > been implemented in more general fashion in the scope of [1], other > things have been rejected). > > Looking forward to your comments! Gentle ping! The DT part has already received an Acked-by -- are there any objections from the DRM side? Thanks and best regards, Michael > > [0] > https://lore.kernel.org/lkml/20230314115644.3775169-1-gerald.loac...@wolfvision.net/ > [1] https://lore.kernel.org/lkml/20230714013756.1546769-1-...@kernel.org/ > > --- > Michael Riesch (2): > drm/panel: sitronix-st7789v: add panel orientation support > dt-bindings: display: add rotation property to sitronix,st7789v > > .../bindings/display/panel/sitronix,st7789v.yaml | 2 ++ > drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 28 > ++ > 2 files changed, 25 insertions(+), 5 deletions(-) > --- > base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 > change-id: 20230718-feature-st7789v-4d0c2c6e2429 > > Best regards,
[PATCH 2/4] dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display
Add compatible for the Jasonic Technology Ltd. JT240MHQS-HWT-EK-E3 display. Signed-off-by: Michael Riesch --- 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
[PATCH 4/4] drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support
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
---
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 = _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 = _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) _panel },
{ "t28cp45tn89-v17", (unsigned long) _panel },
{ "et028013dma", (unsigned long) _panel },
+ { "jt240mhqs-hwt-ek-e3", (unsigned long) _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 = _panel },
{ .compatible = "inanbo,t28cp45tn89-v17", .data = _panel },
{ .compatible = "edt,et028013dma", .data = _panel },
+ { .compatible = "jasonic,jt240mhqs-hwt-ek-e3",
+ .data = _hwt_ek_e3_panel },
{ }
};
MODULE_DEVICE_TABLE(of, st7789v_of_match);
--
2.30.2
[PATCH 3/4] drm/panel: sitronix-st7789v: add support for partial mode
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
---
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
[PATCH 1/4] dt-bindings: vendor-prefixes: add jasonic
Add vendor prefix for Jasonic Technology Ltd., a manufacturer of custom LCD panels. Signed-off-by: Michael Riesch --- 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
[PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode
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-...@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
[PATCH 1/2] drm/panel: sitronix-st7789v: add panel orientation support
Determine the orientation of the display based on the device tree and
propagate it.
While at it, fix the indentation in the struct drm_panel_funcs.
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 28 +-
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index bbc4569cbcdc..6575f07d49e3 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -116,6 +116,7 @@ struct st7789v {
struct spi_device *spi;
struct gpio_desc *reset;
struct regulator *power;
+ enum drm_panel_orientation orientation;
};
enum st7789v_prefix {
@@ -170,6 +171,7 @@ static const struct drm_display_mode default_mode = {
static int st7789v_get_modes(struct drm_panel *panel,
struct drm_connector *connector)
{
+ struct st7789v *ctx = panel_to_st7789v(panel);
struct drm_display_mode *mode;
mode = drm_mode_duplicate(connector->dev, _mode);
@@ -188,9 +190,22 @@ static int st7789v_get_modes(struct drm_panel *panel,
connector->display_info.width_mm = 61;
connector->display_info.height_mm = 103;
+ /*
+* TODO: Remove once all drm drivers call
+* drm_connector_set_orientation_from_panel()
+*/
+ drm_connector_set_panel_orientation(connector, ctx->orientation);
+
return 1;
}
+static enum drm_panel_orientation st7789v_get_orientation(struct drm_panel *p)
+{
+ struct st7789v *ctx = panel_to_st7789v(p);
+
+ return ctx->orientation;
+}
+
static int st7789v_prepare(struct drm_panel *panel)
{
struct st7789v *ctx = panel_to_st7789v(panel);
@@ -346,11 +361,12 @@ static int st7789v_unprepare(struct drm_panel *panel)
}
static const struct drm_panel_funcs st7789v_drm_funcs = {
- .disable= st7789v_disable,
- .enable = st7789v_enable,
- .get_modes = st7789v_get_modes,
- .prepare= st7789v_prepare,
- .unprepare = st7789v_unprepare,
+ .disable = st7789v_disable,
+ .enable = st7789v_enable,
+ .get_modes = st7789v_get_modes,
+ .get_orientation = st7789v_get_orientation,
+ .prepare = st7789v_prepare,
+ .unprepare = st7789v_unprepare,
};
static int st7789v_probe(struct spi_device *spi)
@@ -382,6 +398,8 @@ static int st7789v_probe(struct spi_device *spi)
if (ret)
return ret;
+ of_drm_get_panel_orientation(spi->dev.of_node, >orientation);
+
drm_panel_add(>panel);
return 0;
--
2.30.2
[PATCH 2/2] dt-bindings: display: add rotation property to sitronix,st7789v
The sitronix-st7789v driver now considers the rotation property. Add the property to the documentation. Signed-off-by: Michael Riesch --- Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml index fa6556363cca..694d7f771d0c 100644 --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml @@ -22,6 +22,7 @@ properties: power-supply: true backlight: true port: true + rotation: true spi-cpha: true spi-cpol: true @@ -52,6 +53,7 @@ examples: reset-gpios = < 6 11 GPIO_ACTIVE_LOW>; backlight = <_bl>; power-supply = <>; +rotation = <180>; spi-max-frequency = <10>; spi-cpol; spi-cpha; -- 2.30.2
[PATCH 0/2] drm/panel: sitronix-st7789v: add panel orientation support
Hi all, This series adds support for orientation specification in the device tree to the Sitronix ST7789V panel driver. This is can be seen as reduced version of [0] (some things of [0] have been implemented in more general fashion in the scope of [1], other things have been rejected). Looking forward to your comments! [0] https://lore.kernel.org/lkml/20230314115644.3775169-1-gerald.loac...@wolfvision.net/ [1] https://lore.kernel.org/lkml/20230714013756.1546769-1-...@kernel.org/ --- Michael Riesch (2): drm/panel: sitronix-st7789v: add panel orientation support dt-bindings: display: add rotation property to sitronix,st7789v .../bindings/display/panel/sitronix,st7789v.yaml | 2 ++ drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 28 ++ 2 files changed, 25 insertions(+), 5 deletions(-) --- base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 change-id: 20230718-feature-st7789v-4d0c2c6e2429 Best regards, -- Michael Riesch
Re: [PATCH 7/7] drm/panel: sitronix-st7789v: Check display ID
Hi Miquel,
On 6/9/23 16:59, Miquel Raynal wrote:
> A very basic debugging rule when a device is connected for the first
> time is to access a read-only register which contains known data in
> order to ensure the communication protocol is properly working. This
> driver lacked any read helper which is often a critical peace for
> fastening bring-ups.
I am afraid I don't get the last sentence. s/peace/piece? s/for
fastening/to speed up ? Only guessing here.
Best regards,
Michael
> Add a read helper and use it to verify the communication with the panel
> is working as soon as possible in order to fail early if this is not the
> case.
>
> Signed-off-by: Miquel Raynal
> ---
> .../gpu/drm/panel/panel-sitronix-st7789v.c| 78 +++
> 1 file changed, 78 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index 7de192a3a8aa..fb21d52a7a63 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -113,6 +113,9 @@
> return val; \
> } while (0)
>
> +#define ST7789V_IDS { 0x85, 0x85, 0x52 }
> +#define ST7789V_IDS_SIZE 3
> +
> struct st7789v_panel_info {
> const struct drm_display_mode *display_mode;
> u16 width_mm;
> @@ -165,6 +168,77 @@ static int st7789v_write_data(struct st7789v *ctx, u8
> cmd)
> return st7789v_spi_write(ctx, ST7789V_DATA, cmd);
> }
>
> +static int st7789v_read_data(struct st7789v *ctx, u8 cmd, u8 *buf,
> + unsigned int len)
> +{
> + struct spi_transfer xfer[2] = { };
> + struct spi_message msg;
> + u16 txbuf = ((ST7789V_COMMAND & 1) << 8) | cmd;
> + u16 rxbuf[4] = {};
> + u8 bit9 = 0;
> + int ret, i;
> +
> + switch (len) {
> + case 1:
> + case 3:
> + case 4:
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + spi_message_init();
> +
> + xfer[0].tx_buf =
> + xfer[0].len = sizeof(txbuf);
> + spi_message_add_tail([0], );
> +
> + xfer[1].rx_buf = rxbuf;
> + xfer[1].len = len * 2;
> + spi_message_add_tail([1], );
> +
> + ret = spi_sync(ctx->spi, );
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < len; i++) {
> + buf[i] = rxbuf[i] >> i | (bit9 << (9 - i));
> + if (i)
> + bit9 = rxbuf[i] & GENMASK(i - 1, 0);
> + }
> +
> + return 0;
> +}
> +
> +static int st7789v_check_id(struct drm_panel *panel)
> +{
> + const u8 st7789v_ids[ST7789V_IDS_SIZE] = ST7789V_IDS;
> + struct st7789v *ctx = panel_to_st7789v(panel);
> + bool invalid_ids = false;
> + int ret, i;
> + u8 ids[3];
> +
> + ret = st7789v_read_data(ctx, MIPI_DCS_GET_DISPLAY_ID, ids,
> ST7789V_IDS_SIZE);
> + if (ret) {
> + dev_err(panel->dev, "Failed to read IDs\n");
> + return ret;
> + }
> +
> + for (i = 0; i < ST7789V_IDS_SIZE; i++) {
> + if (ids[i] != st7789v_ids[i]) {
> + invalid_ids = true;
> + break;
> + }
> + }
> +
> + if (invalid_ids) {
> + dev_err(panel->dev, "Unrecognized panel IDs");
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> static const struct drm_display_mode default_mode = {
> .clock = 7000,
> .hdisplay = 240,
> @@ -237,6 +311,10 @@ static int st7789v_prepare(struct drm_panel *panel)
> gpiod_set_value(ctx->reset, 0);
> msleep(120);
>
> + ret = st7789v_check_id(panel);
> + if (ret)
> + return ret;
> +
> ST7789V_TEST(ret, st7789v_write_command(ctx, MIPI_DCS_EXIT_SLEEP_MODE));
>
> /* We need to wait 120ms after a sleep out command */
Re: [PATCH 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings
Hi Miquel,
On 6/9/23 16:59, Miquel Raynal wrote:
> The spi core warns us about using an of_device_id table without a
s/spi/SPI ?
> spi_device_id table aside for module utilities in orter to not rely on
s/in orter to/in order to ?
> OF modaliases. Just add this table using the device name without the
> vendor prefix (as it is usually done).
>
> Signed-off-by: Miquel Raynal
> ---
> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index bbc4569cbcdc..c67b9adb157f 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -400,9 +400,16 @@ static const struct of_device_id
st7789v_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, st7789v_of_match);
>
> +static const struct spi_device_id st7789v_ids[] = {
> + { "st7789v", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, st7789v_ids);
> +
> static struct spi_driver st7789v_driver = {
> .probe = st7789v_probe,
> .remove = st7789v_remove,
> + .id_table = st7789v_ids,
> .driver = {
> .name = "st7789v",
> .of_match_table = st7789v_of_match,
May I point to you Sebastian Reichel's series that features a partial
overlap with your work? [0]
For instance, the patch at hand is comparable to [1].
Cc: Sebastian to keep him in the loop.
Best regards,
Michael
[0]
https://lore.kernel.org/dri-devel/20230422205012.2464933-1-...@kernel.org/
[1]
https://lore.kernel.org/dri-devel/20230422205012.2464933-4-...@kernel.org/
Re: [PATCH v2 08/13] drm/panel: sitronix-st7789v: avoid hardcoding mode info
Hi Sebastian,
Thanks for the v2 of your series. Looks great!
One nitpick though: you seem to wrap the patch message lines at ~50
characters sometimes, which is awfully short.
Another comment below:
On 4/22/23 22:50, Sebastian Reichel wrote:
> Avoid hard-coding the default_mode and supply it from match data. One
> additional layer of abstraction has been introduced, which will be
> needed for specifying other panel information (e.g. bus flags) in the
> next steps.
>
> Signed-off-by: Sebastian Reichel
> ---
> .../gpu/drm/panel/panel-sitronix-st7789v.c| 24 ++-
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index a6d6155ef45c..29c2a91f8299 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -108,8 +108,13 @@
> return val; \
> } while (0)
>
> +struct st7789_panel_info {
> + const struct drm_display_mode *mode;
> +};
> +
> struct st7789v {
> struct drm_panel panel;
> + const struct st7789_panel_info *info;
> struct spi_device *spi;
> struct gpio_desc *reset;
> struct regulator *power;
> @@ -160,16 +165,21 @@ static const struct drm_display_mode default_mode = {
> .vtotal = 320 + 8 + 4 + 4,
> };
>
> +struct st7789_panel_info default_panel = {
> + .mode = _mode,
> +};
Shouldn't this be "static const struct st7789_panel_info default_panel"?
(Same holds for "struct st7789_panel_info t28cp45tn89_panel" in patch
13/13.)
With the comments above addressed, feel free to add my
Reviewed-by: Michael Riesch
to the whole v3 of your series.
Thanks and best regards,
Michael
> +
> static int st7789v_get_modes(struct drm_panel *panel,
>struct drm_connector *connector)
> {
> + struct st7789v *ctx = panel_to_st7789v(panel);
> struct drm_display_mode *mode;
>
> - mode = drm_mode_duplicate(connector->dev, _mode);
> + mode = drm_mode_duplicate(connector->dev, ctx->info->mode);
> if (!mode) {
> - dev_err(panel->dev, "failed to add mode %ux%ux@%u\n",
> - default_mode.hdisplay, default_mode.vdisplay,
> - drm_mode_vrefresh(_mode));
> + dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> + ctx->info->mode->hdisplay, ctx->info->mode->vdisplay,
> + drm_mode_vrefresh(ctx->info->mode));
> return -ENOMEM;
> }
>
> @@ -359,6 +369,8 @@ static int st7789v_probe(struct spi_device *spi)
> spi_set_drvdata(spi, ctx);
> ctx->spi = spi;
>
> + ctx->info = device_get_match_data(>dev);
> +
> drm_panel_init(>panel, dev, _drm_funcs,
> DRM_MODE_CONNECTOR_DPI);
>
> @@ -389,13 +401,13 @@ static void st7789v_remove(struct spi_device *spi)
> }
>
> static const struct spi_device_id st7789v_spi_id[] = {
> - { "st7789v" },
> + { "st7789v", (unsigned long) _panel },
> { }
> };
> MODULE_DEVICE_TABLE(spi, st7789v_spi_id);
>
> static const struct of_device_id st7789v_of_match[] = {
> - { .compatible = "sitronix,st7789v" },
> + { .compatible = "sitronix,st7789v", .data = _panel },
> { }
> };
> MODULE_DEVICE_TABLE(of, st7789v_of_match);
Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v
Hi Maxime, On 4/4/23 18:04, Maxime Ripard wrote: > On Fri, Mar 31, 2023 at 11:36:43AM +0200, Michael Riesch wrote: >> On 3/30/23 16:58, Maxime Ripard wrote: >>> On Wed, Mar 29, 2023 at 12:08:50PM +0200, Michael Riesch wrote: >>>> On 3/29/23 11:16, Maxime Ripard wrote: >>>>> On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote: >>>>>> Hi Rob, >>>>>> >>>>>> On 3/16/23 22:57, Rob Herring wrote: >>>>>>> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote: >>>>>>>> The sitronix-st7789v driver now considers the panel-timing property. >>>>>>> >>>>>>> I read the patch for that and still don't know 'why'. Commit messages >>>>>>> should answer why. >>>>>>> >>>>>>>> Add the property to the documentation. >>>>>>> >>>>>>> We generally don't put timings in DT for panels. Why is this one >>>>>>> special? >>>>>> >>>>>> For now, having the timings in the device tree allows for setting the >>>>>> hsync/vsync/de polarity. >>>>>> >>>>>> As a next step, we aim to implement the partial mode feature of this >>>>>> panel. It is possible to use only a certain region of the panel, which >>>>>> is helpful e.g., when a part of the panel is occluded and should not be >>>>>> considered by DRM. We thought that this could be specified as timing in >>>>>> DT. >>>>>> >>>>>> (The hactive and vactive properties serve as dimensions of this certain >>>>>> region, of course. We still need to specify somehow the position of the >>>>>> region. Maybe with additional properties hactive-start and >>>>>> vactive-start?) >>>>>> >>>>>> What do you think about that? >>>>> >>>>> I don't see why we would need the device tree to support that. What you >>>>> described is essentially what overscan is for HDMI/analog output, and we >>>>> already have everything to deal with overscan properly in KMS. >>>> >>>> Thanks for your response, but I am afraid I don't quite follow. >>>> >>>> How are we supposed to expose control over the hsync/vsync/data enable >>>> polarity? I only know that the display controller and the panel need to >>>> agree on a setting that works for both. What is the canonical way to do >>>> this? >>> >>> So typically, it would come from the panel datasheet and would thus be >>> exposed by the panel driver. st7789v is not a panel itself but a (pretty >>> flexible) panel controller so it's not fixed and I don't think we have a >>> good answer to that (yet). >> >> Then it seems to me that creating a panel driver (= st8879v panel >> controller driver with a new compatible) would make sense. > > I don't see why? The entire controller is the same except (maybe) for > some initialization data. Doing a new driver for it seems like taking > the easy way out? > >> By coincidence Sebastian Reichel has come up with this approach >> recently, see >> https://lore.kernel.org/dri-devel/20230317232355.1554980-1-...@kernel.org/ >> We just need a way to resolve the conflicts between the two series. >> >> Cc: Sebastian > > That's not a new driver though? That approach looks sane to me. Sorry for the ambiguity. The plan is now to add a new compatible to the st8879v panel controller driver. >>>> A different question is the partial mode, for which (IIUC) you suggest >>>> the overscan feature. As I have never heard of this before, it would be >>>> very nice if you could point me to some examples. Where would the >>>> effective resolution be set in this case? >>> >>> So, back when CRT were a thing the edges of the tube were masked by the >>> plastic case. HDMI inherited from that and that's why you still have >>> some UI on some devices (like consoles) to setup the active area of the >>> display. >>> >>> The underlying issue is exactly what you describe: the active area is >>> larger than what the plastic case allows to see. I don't think anyone >>> ever had the usecase you have, but it would be the right solution to me >>> to solve essentially the same issue the same way we do on other output >>> types. >> >> OK, we'll look into the overscan feature. But still the information >> about the active area should come from the driver, right? > > No, the userspace is in charge there. I'd prefer not to have the hardware description in user space. But we can continue this discussing once our v2 is out. Best regards, Michael
Re: [PATCHv1 3/7] drm/panel: sitronix-st7789v: add SPI ID table
Hi Sebastian,
On 3/18/23 00:23, Sebastian Reichel wrote:
> SPI device drivers should also have a SPI ID table.
>
> Signed-off-by: Sebastian Reichel
> ---
> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index bbc4569cbcdc..e4d8dea1db36 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -394,6 +394,12 @@ static void st7789v_remove(struct spi_device *spi)
> drm_panel_remove(>panel);
> }
>
> +static const struct spi_device_id st7789v_spi_id[] = {
> + { "st7789v" },
Minor suggestion: The format
static const struct spi_device_id st7789v_spi_id[] = {
{
.name = "st7789v",
},
{ }
};
is more verbose, but can be extended easily.
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, st7789v_spi_id);
> +
> static const struct of_device_id st7789v_of_match[] = {
> { .compatible = "sitronix,st7789v" },
> { }
The same holds for this structure here (you may want to consider this
when adding the .data field in patch 6/7.
Best regards,
Michael
> @@ -403,6 +409,7 @@ MODULE_DEVICE_TABLE(of, st7789v_of_match);
> static struct spi_driver st7789v_driver = {
> .probe = st7789v_probe,
> .remove = st7789v_remove,
> + .id_table = st7789v_spi_id,
> .driver = {
> .name = "st7789v",
> .of_match_table = st7789v_of_match,
Re: [PATCHv1 7/7] drm/panel: sitronix-st7789v: add Inanbo T28CP45TN89 support
Hi Sebastian,
Thanks for your work and for the beautiful timing :-)
On 3/18/23 00:23, Sebastian Reichel wrote:
> UNI-T UTi260b has a Inanbo T28CP45TN89 v17 panel. I could not find
> proper documentation for the panel apart from a technical drawing, but
> according to the vendor U-Boot it is based on a Sitronix st7789v chip.
> I generated the init sequence by modifying the default one until proper
> graphics output has been seen on the device.
I can spot only a few differences:
- bits per pixel: 18 (RGB666) vs. 16 (RGB565)
- invert mode
- sync/clk signal polarity
The init sequences are largely the same, which leads to vast code
duplication. Instead, the st7789v_prepare could be adjusted to consider
the st7789v_panel_info and apply the required settings accordingly.
For example, the polarities could be embedded into the drm_display_mode
structure...
> Signed-off-by: Sebastian Reichel
> ---
> .../gpu/drm/panel/panel-sitronix-st7789v.c| 137 ++
> 1 file changed, 137 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index a62a2f5737e4..90f70eb84f11 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -10,6 +10,7 @@
> #include
>
> #include
> +#include
>
> #include
> #include
> @@ -113,6 +114,8 @@ struct st7789v;
> struct st7789_panel_info {
> const struct drm_display_mode *mode;
> int (*init_sequence)(struct st7789v *ctx);
> + unsigned int bpc;
> + u32 bus_format;
... and here you introduce fields for the bits per pixel. Just a field
for the invert mode is missing.
BTW, I would introduce these fields in the previous patch. This patch
should be only about filling out the already existing fields for the new
panel.
> };
>
> struct st7789v {
> @@ -174,6 +177,20 @@ static const struct drm_display_mode default_mode = {
> .height_mm = 103,
> };
>
> +static const struct drm_display_mode t28cp45tn89_mode = {
> + .clock = 6008,
> + .hdisplay = 240,
> + .hsync_start = 240 + 38,
> + .hsync_end = 240 + 38 + 10,
> + .htotal = 240 + 38 + 10 + 10,
> + .vdisplay = 320,
> + .vsync_start = 320 + 8,
> + .vsync_end = 320 + 8 + 4,
> + .vtotal = 320 + 8 + 4 + 4,
> + .width_mm = 43,
> + .height_mm = 57,
> +};
> +
> static int init_sequence_default(struct st7789v *ctx) {
> int ret;
>
> @@ -283,11 +300,125 @@ static int init_sequence_default(struct st7789v *ctx) {
> return 0;
> }
>
> +static int init_sequence_t28cp45tn89(struct st7789v *ctx) {
> + int ret;
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx,
> + MIPI_DCS_SET_ADDRESS_MODE));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, 0));
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx,
> + MIPI_DCS_SET_PIXEL_FORMAT));
> + ST7789V_TEST(ret, st7789v_write_data(ctx,
> + (MIPI_DCS_PIXEL_FMT_16BIT << 4) |
> + (MIPI_DCS_PIXEL_FMT_16BIT)));
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_PORCTRL_CMD));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, 0xc));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, 0xc));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, 0));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_PORCTRL_IDLE_BP(3) |
> + ST7789V_PORCTRL_IDLE_FP(3)));
> + ST7789V_TEST(ret, st7789v_write_data(ctx,
> + ST7789V_PORCTRL_PARTIAL_BP(3) |
> + ST7789V_PORCTRL_PARTIAL_FP(3)));
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_GCTRL_CMD));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_GCTRL_VGLS(5) |
> + ST7789V_GCTRL_VGHS(3)));
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_VCOMS_CMD));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, 0x2b));
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_LCMCTRL_CMD));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_LCMCTRL_XMH |
> + ST7789V_LCMCTRL_XMX |
> + ST7789V_LCMCTRL_XBGR));
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_VDVVRHEN_CMD));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_VDVVRHEN_CMDEN));
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_VRHS_CMD));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, 0xf));
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_VDVS_CMD));
> + ST7789V_TEST(ret, st7789v_write_data(ctx, 0x20));
> +
> + ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_FRCTRL2_CMD));
> + ST7789V_TEST(ret,
Re: [PATCH 0/7] Add timing override to sitronix,st7789v
Hi all, On 3/14/23 12:56, Gerald Loacker wrote: > This patch set adds additional functionality to the sitronix,st7789v > driver. > > Patches 1,3 and 4 propagate useful flags to the drm subsystem. > Patch 2 adds the orientation property. If there are no objections, patches 1-4 and 6 could be applied from our point of view. Or should we spin a v2? > Patch 5 parses the device tree for a panel-timing and makes it possible to > override the default timing. > Patches 6 and 7 add the new properties to the dt-bindings. Parsing the timing from the device tree (patches 5 and 7) can be ignored, we'll come up with a different approach. Best regards, Michael > > Gerald Loacker (4): > drm/panel: sitronix-st7789v: propagate h/v-sync polarity > drm/panel: sitronix-st7789v: add bus_flags to connector > drm/panel: sitronix-st7789v: parse device tree to override timing mode > dt-bindings: display: add panel-timing property to sitronix,st7789v > > Michael Riesch (3): > drm/panel: sitronix-st7789v: propagate RGB666 format > drm/panel: sitronix-st7789v: add panel orientation support > dt-bindings: display: add rotation property to sitronix,st7789v > > .../display/panel/sitronix,st7789v.yaml | 19 ++ > .../gpu/drm/panel/panel-sitronix-st7789v.c| 204 +++--- > 2 files changed, 191 insertions(+), 32 deletions(-) >
Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v
Hi Maxime, On 3/30/23 16:58, Maxime Ripard wrote: > On Wed, Mar 29, 2023 at 12:08:50PM +0200, Michael Riesch wrote: >> On 3/29/23 11:16, Maxime Ripard wrote: >>> On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote: >>>> Hi Rob, >>>> >>>> On 3/16/23 22:57, Rob Herring wrote: >>>>> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote: >>>>>> The sitronix-st7789v driver now considers the panel-timing property. >>>>> >>>>> I read the patch for that and still don't know 'why'. Commit messages >>>>> should answer why. >>>>> >>>>>> Add the property to the documentation. >>>>> >>>>> We generally don't put timings in DT for panels. Why is this one >>>>> special? >>>> >>>> For now, having the timings in the device tree allows for setting the >>>> hsync/vsync/de polarity. >>>> >>>> As a next step, we aim to implement the partial mode feature of this >>>> panel. It is possible to use only a certain region of the panel, which >>>> is helpful e.g., when a part of the panel is occluded and should not be >>>> considered by DRM. We thought that this could be specified as timing in DT. >>>> >>>> (The hactive and vactive properties serve as dimensions of this certain >>>> region, of course. We still need to specify somehow the position of the >>>> region. Maybe with additional properties hactive-start and vactive-start?) >>>> >>>> What do you think about that? >>> >>> I don't see why we would need the device tree to support that. What you >>> described is essentially what overscan is for HDMI/analog output, and we >>> already have everything to deal with overscan properly in KMS. >> >> Thanks for your response, but I am afraid I don't quite follow. >> >> How are we supposed to expose control over the hsync/vsync/data enable >> polarity? I only know that the display controller and the panel need to >> agree on a setting that works for both. What is the canonical way to do >> this? > > So typically, it would come from the panel datasheet and would thus be > exposed by the panel driver. st7789v is not a panel itself but a (pretty > flexible) panel controller so it's not fixed and I don't think we have a > good answer to that (yet). Then it seems to me that creating a panel driver (= st8879v panel controller driver with a new compatible) would make sense. By coincidence Sebastian Reichel has come up with this approach recently, see https://lore.kernel.org/dri-devel/20230317232355.1554980-1-...@kernel.org/ We just need a way to resolve the conflicts between the two series. Cc: Sebastian >> A different question is the partial mode, for which (IIUC) you suggest >> the overscan feature. As I have never heard of this before, it would be >> very nice if you could point me to some examples. Where would the >> effective resolution be set in this case? > > So, back when CRT were a thing the edges of the tube were masked by the > plastic case. HDMI inherited from that and that's why you still have > some UI on some devices (like consoles) to setup the active area of the > display. > > The underlying issue is exactly what you describe: the active area is > larger than what the plastic case allows to see. I don't think anyone > ever had the usecase you have, but it would be the right solution to me > to solve essentially the same issue the same way we do on other output > types. OK, we'll look into the overscan feature. But still the information about the active area should come from the driver, right? Thanks and best regards, Michael
Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v
Hi Maxime, On 3/29/23 11:16, Maxime Ripard wrote: > On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote: >> Hi Rob, >> >> On 3/16/23 22:57, Rob Herring wrote: >>> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote: >>>> The sitronix-st7789v driver now considers the panel-timing property. >>> >>> I read the patch for that and still don't know 'why'. Commit messages >>> should answer why. >>> >>>> Add the property to the documentation. >>> >>> We generally don't put timings in DT for panels. Why is this one >>> special? >> >> For now, having the timings in the device tree allows for setting the >> hsync/vsync/de polarity. >> >> As a next step, we aim to implement the partial mode feature of this >> panel. It is possible to use only a certain region of the panel, which >> is helpful e.g., when a part of the panel is occluded and should not be >> considered by DRM. We thought that this could be specified as timing in DT. >> >> (The hactive and vactive properties serve as dimensions of this certain >> region, of course. We still need to specify somehow the position of the >> region. Maybe with additional properties hactive-start and vactive-start?) >> >> What do you think about that? > > I don't see why we would need the device tree to support that. What you > described is essentially what overscan is for HDMI/analog output, and we > already have everything to deal with overscan properly in KMS. Thanks for your response, but I am afraid I don't quite follow. How are we supposed to expose control over the hsync/vsync/data enable polarity? I only know that the display controller and the panel need to agree on a setting that works for both. What is the canonical way to do this? A different question is the partial mode, for which (IIUC) you suggest the overscan feature. As I have never heard of this before, it would be very nice if you could point me to some examples. Where would the effective resolution be set in this case? We thought that this should enter the device tree as in our case the display is partially occluded due to hardware constraints. For the user there is only one reasonable configuration. Alternatively, we could follow a different approach and handle a separate compatible in the panel driver. Would this be acceptable for mainline inclusion? Best regards, Michael
Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v
Hi Rob,
On 3/16/23 22:57, Rob Herring wrote:
> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
>> The sitronix-st7789v driver now considers the panel-timing property.
>
> I read the patch for that and still don't know 'why'. Commit messages
> should answer why.
>
>> Add the property to the documentation.
>
> We generally don't put timings in DT for panels. Why is this one
> special?
For now, having the timings in the device tree allows for setting the
hsync/vsync/de polarity.
As a next step, we aim to implement the partial mode feature of this
panel. It is possible to use only a certain region of the panel, which
is helpful e.g., when a part of the panel is occluded and should not be
considered by DRM. We thought that this could be specified as timing in DT.
(The hactive and vactive properties serve as dimensions of this certain
region, of course. We still need to specify somehow the position of the
region. Maybe with additional properties hactive-start and vactive-start?)
What do you think about that?
Thanks and best regards,
Michael
>
>>
>> Signed-off-by: Gerald Loacker
>> ---
>> .../display/panel/sitronix,st7789v.yaml | 17 +
>> 1 file changed, 17 insertions(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> index ed942cd3620f..8810f123dedf 100644
>> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> @@ -21,6 +21,7 @@ properties:
>>reset-gpios: true
>>power-supply: true
>>backlight: true
>> + panel-timing: true
>>port: true
>>rotation: true
>>
>> @@ -54,6 +55,22 @@ examples:
>> spi-cpol;
>> spi-cpha;
>>
>> +panel-timing {
>> +clock-frequency = <700>;
>> +hactive = <240>;
>> +vactive = <320>;
>> +hfront-porch = <38>;
>> +hback-porch = <10>;
>> +hsync-len = <10>;
>> +vfront-porch = <8>;
>> +vback-porch = <4>;
>> +vsync-len = <4>;
>> +hsync-active = <1>;
>> +vsync-active = <1>;
>> +de-active = <1>;
>> +pixelclk-active = <1>;
>> +};
>> +
>> port {
>> panel_input: endpoint {
>> remote-endpoint = <_out_panel>;
>> --
>> 2.37.2
>>
Re: [PATCH] drm/rockchip: vop2: fix uninitialized variable possible_crtcs
Hi Tom, Heiko,
On 3/16/23 15:05, Heiko Stuebner wrote:
> Am Donnerstag, 16. März 2023, 14:23:02 CET schrieb Tom Rix:
>> clang reportes this error
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:8: error:
>> variable 'possible_crtcs' is used uninitialized whenever 'if'
>> condition is false [-Werror,-Wsometimes-uninitialized]
>> if (vp) {
>> ^~
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2336:36: note:
>> uninitialized use occurs here
>> ret = vop2_plane_init(vop2, win, possible_crtcs);
>> ^~
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:4:
>> note: remove the 'if' if its condition is always true
>> if (vp) {
>> ^~~~
>>
>> The else-statement changes the win->type to OVERLAY without setting the
>> possible_crtcs variable. Rework the block, initialize possible_crtcs to
>> 0 to remove the else-statement. Split the else-if-statement out to its
>> own if-statement so the OVERLAY check will catch when the win-type has
>> been changed.
>>
>> Fixes: 368419a2d429 ("drm/rockchip: vop2: initialize possible_crtcs
>> properly")
>> Signed-off-by: Tom Rix
>> ---
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 9 -
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 03ca32cd2050..fce992c3506f 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -2301,7 +2301,7 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>> nvp = 0;
>> for (i = 0; i < vop2->registered_num_wins; i++) {
>> struct vop2_win *win = >win[i];
>> -u32 possible_crtcs;
>> +u32 possible_crtcs = 0;
>>
>> if (vop2->data->soc_id == 3566) {
>> /*
>> @@ -2327,12 +2327,11 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>> /* change the unused primary window to overlay
>> window */
>> win->type = DRM_PLANE_TYPE_OVERLAY;
>> }
>> -} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
>> -possible_crtcs = (1 << nvps) - 1;
>> -} else {
>> -possible_crtcs = 0;
>> }
>>
>> +if (win->type == DRM_PLANE_TYPE_OVERLAY)
>> +possible_crtcs = (1 << nvps) - 1;
>> +
>
> After a long hard stare at the code in question, I think doing it this
> way looks like the correct one, as as you mention in the commit message
> the first "if" will change the win->type to OVERLAY in one case, but this
> then will never be added.
>
> Michael, do you agree/disagree?
Shoot, this bit of code is more complicated than I believed it would be.
Yes, Tom's patch makes sense to me. But having overlooked the win->type
change in the if-clause makes me think I shouldn't be the judge of that
:-) Still,
Acked-by: Michael Riesch
Thanks for fixing it and best regards,
Michael
>
>
> Thanks
> Heiko
>
> https://lore.kernel.org/r/20230315090158.2442771-1-michael.rie...@wolfvision.net
>
>> ret = vop2_plane_init(vop2, win, possible_crtcs);
>> if (ret) {
>> drm_err(vop2->drm, "failed to init plane %s: %d\n",
>>
>
>
>
>
[PATCH] drm/rockchip: vop2: fix initialization of possible_crtcs variable
The variable possible_crtcs is not initialized properly since commit
604be85547ce ("drm/rockchip: Add VOP2 driver"). A first attempt to fix this
issue has been made in commit 368419a2d429 ("drm/rockchip: vop2: initialize
possible_crtcs properly") but turned out to be incomplete. Initialize the
variable with zero to cover all possible paths.
Fixes: 368419a2d429 ("drm/rockchip: vop2: initialize possible_crtcs properly")
Reported-by: kernel test robot
Reported-by: Nathan Chancellor
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 1667e5324b29..0569f1211d9b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2301,7 +2301,7 @@ static int vop2_create_crtcs(struct vop2 *vop2)
nvp = 0;
for (i = 0; i < vop2->registered_num_wins; i++) {
struct vop2_win *win = >win[i];
- u32 possible_crtcs;
+ u32 possible_crtcs = 0;
if (vop2->data->soc_id == 3566) {
/*
@@ -2329,8 +2329,6 @@ static int vop2_create_crtcs(struct vop2 *vop2)
}
} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
possible_crtcs = (1 << nvps) - 1;
- } else {
- possible_crtcs = 0;
}
ret = vop2_plane_init(vop2, win, possible_crtcs);
--
2.30.2
Re: [PATCH v3 1/6] drm/rockchip: vop2: initialize possible_crtcs properly
Hi Nathan,
On 3/14/23 17:08, Nathan Chancellor wrote:
> Hi Michael,
>
> On Tue, Jan 24, 2023 at 06:47:01AM +0100, Michael Riesch wrote:
>> The variable possible_crtcs is only initialized for primary and
>> overlay planes. Since the VOP2 driver only supports these plane
>> types at the moment, the current code is safe. However, in order
>> to provide a future-proof solution, fix the initialization of
>> the variable.
>>
>> Reported-by: kernel test robot
>> Reported-by: Dan Carpenter
>> Signed-off-by: Michael Riesch
>> ---
>> v3:
>> - no changes
>> v2:
>> - new patch
>>
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 8cecf81a5ae0..374ef821b453 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -2322,10 +2322,11 @@ static int vop2_create_crtc(struct vop2 *vop2)
>> /* change the unused primary window to overlay
>> window */
>> win->type = DRM_PLANE_TYPE_OVERLAY;
>> }
>> -}
>> -
>> -if (win->type == DRM_PLANE_TYPE_OVERLAY)
>> +} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
>> possible_crtcs = (1 << nvps) - 1;
>> +} else {
>> +possible_crtcs = 0;
>> +}
>>
>> ret = vop2_plane_init(vop2, win, possible_crtcs);
>> if (ret) {
>> --
>> 2.30.2
>>
>
> This patch is now in -next as commit 368419a2d429 ("drm/rockchip: vop2:
> initialize possible_crtcs properly") and it actually appears to
> introduce a path where possible_crtcs could be used uninitialized.
>
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316:8: error: variable
> 'possible_crtcs' is used uninitialized whenever 'if' condition is false
> [-Werror,-Wsometimes-uninitialized]
> if (vp) {
> ^~
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2330:36: note: uninitialized
> use occurs here
> ret = vop2_plane_init(vop2, win, possible_crtcs);
>^~
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316:4: note: remove the 'if'
> if its condition is always true
> if (vp) {
> ^~~~
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2298:21: note: initialize the
> variable 'possible_crtcs' to silence this warning
> u32 possible_crtcs;
> ^
> = 0
> 1 error generated.
>
> Prior to this change, if that else path was hit, clang recognized based on
> the assignment that the next if statement would always be true. Now, if
> the else path is taken, the possible_crtcs assignment will be missed. Is
> that intentional?
As it turns out, the approach in my patch does not cover all paths. I'll
submit a follow-up patch that initializes possible_crtcs = 0 and drops
the else path. This should solve the issue for good.
Regards, Michael
Re: [PATCH v3 5/6] drm/rockchip: vop2: add support for the rgb output block
On 1/24/23 06:47, Michael Riesch wrote:
> The Rockchip VOP2 features an internal RGB output block, which can be
> attached any video port of the VOP2. Add support for this output block.
s/attached any/attached to any/ of course. Can this be fixed when the
patch is applied?
Michael
> Signed-off-by: Michael Riesch
> ---
> v3:
> - fix commit messages (still assumed video port 2)
> - fix condition to make 0 a valid video port
> v2:
> - move away from wrong assumption that the RGB block is always
>connected to video port 2 -> check devicetree to find RGB block
>
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 44
> 1 file changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 06fcdfa7b885..f38ffd0ccd9f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -39,6 +39,7 @@
> #include "rockchip_drm_gem.h"
> #include "rockchip_drm_fb.h"
> #include "rockchip_drm_vop2.h"
> +#include "rockchip_rgb.h"
>
> /*
> * VOP2 architecture
> @@ -212,6 +213,9 @@ struct vop2 {
> struct clk *hclk;
> struct clk *aclk;
>
> + /* optional internal rgb encoder */
> + struct rockchip_rgb *rgb;
> +
> /* must be put at the end of the struct */
> struct vop2_win win[];
> };
> @@ -2393,6 +2397,25 @@ static void vop2_destroy_crtcs(struct vop2 *vop2)
> }
> }
>
> +static int vop2_find_rgb_encoder(struct vop2 *vop2)
> +{
> + struct device_node *node = vop2->dev->of_node;
> + struct device_node *endpoint;
> + int i;
> +
> + for (i = 0; i < vop2->data->nr_vps; i++) {
> + endpoint = of_graph_get_endpoint_by_regs(node, i,
> + ROCKCHIP_VOP2_EP_RGB0);
> + if (!endpoint)
> + continue;
> +
> + of_node_put(endpoint);
> + return i;
> + }
> +
> + return -ENOENT;
> +}
> +
> static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
> [VOP2_WIN_ENABLE] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 0, 0),
> [VOP2_WIN_FORMAT] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 1, 5),
> @@ -2698,11 +2721,29 @@ static int vop2_bind(struct device *dev, struct
> device *master, void *data)
> if (ret)
> return ret;
>
> + ret = vop2_find_rgb_encoder(vop2);
> + if (ret >= 0) {
> + vop2->rgb = rockchip_rgb_init(dev, >vps[ret].crtc,
> + vop2->drm, ret);
> + if (IS_ERR(vop2->rgb)) {
> + if (PTR_ERR(vop2->rgb) == -EPROBE_DEFER) {
> + ret = PTR_ERR(vop2->rgb);
> + goto err_crtcs;
> + }
> + vop2->rgb = NULL;
> + }
> + }
> +
> rockchip_drm_dma_init_device(vop2->drm, vop2->dev);
>
> pm_runtime_enable(>dev);
>
> return 0;
> +
> +err_crtcs:
> + vop2_destroy_crtcs(vop2);
> +
> + return ret;
> }
>
> static void vop2_unbind(struct device *dev, struct device *master, void
> *data)
> @@ -2711,6 +2752,9 @@ static void vop2_unbind(struct device *dev, struct
> device *master, void *data)
>
> pm_runtime_disable(dev);
>
> + if (vop2->rgb)
> + rockchip_rgb_fini(vop2->rgb);
> +
> vop2_destroy_crtcs(vop2);
> }
>
[PATCH v3 5/6] drm/rockchip: vop2: add support for the rgb output block
The Rockchip VOP2 features an internal RGB output block, which can be
attached any video port of the VOP2. Add support for this output block.
Signed-off-by: Michael Riesch
---
v3:
- fix commit messages (still assumed video port 2)
- fix condition to make 0 a valid video port
v2:
- move away from wrong assumption that the RGB block is always
connected to video port 2 -> check devicetree to find RGB block
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 44
1 file changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 06fcdfa7b885..f38ffd0ccd9f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -39,6 +39,7 @@
#include "rockchip_drm_gem.h"
#include "rockchip_drm_fb.h"
#include "rockchip_drm_vop2.h"
+#include "rockchip_rgb.h"
/*
* VOP2 architecture
@@ -212,6 +213,9 @@ struct vop2 {
struct clk *hclk;
struct clk *aclk;
+ /* optional internal rgb encoder */
+ struct rockchip_rgb *rgb;
+
/* must be put at the end of the struct */
struct vop2_win win[];
};
@@ -2393,6 +2397,25 @@ static void vop2_destroy_crtcs(struct vop2 *vop2)
}
}
+static int vop2_find_rgb_encoder(struct vop2 *vop2)
+{
+ struct device_node *node = vop2->dev->of_node;
+ struct device_node *endpoint;
+ int i;
+
+ for (i = 0; i < vop2->data->nr_vps; i++) {
+ endpoint = of_graph_get_endpoint_by_regs(node, i,
+ROCKCHIP_VOP2_EP_RGB0);
+ if (!endpoint)
+ continue;
+
+ of_node_put(endpoint);
+ return i;
+ }
+
+ return -ENOENT;
+}
+
static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
[VOP2_WIN_ENABLE] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 0, 0),
[VOP2_WIN_FORMAT] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 1, 5),
@@ -2698,11 +2721,29 @@ static int vop2_bind(struct device *dev, struct device
*master, void *data)
if (ret)
return ret;
+ ret = vop2_find_rgb_encoder(vop2);
+ if (ret >= 0) {
+ vop2->rgb = rockchip_rgb_init(dev, >vps[ret].crtc,
+ vop2->drm, ret);
+ if (IS_ERR(vop2->rgb)) {
+ if (PTR_ERR(vop2->rgb) == -EPROBE_DEFER) {
+ ret = PTR_ERR(vop2->rgb);
+ goto err_crtcs;
+ }
+ vop2->rgb = NULL;
+ }
+ }
+
rockchip_drm_dma_init_device(vop2->drm, vop2->dev);
pm_runtime_enable(>dev);
return 0;
+
+err_crtcs:
+ vop2_destroy_crtcs(vop2);
+
+ return ret;
}
static void vop2_unbind(struct device *dev, struct device *master, void *data)
@@ -2711,6 +2752,9 @@ static void vop2_unbind(struct device *dev, struct device
*master, void *data)
pm_runtime_disable(dev);
+ if (vop2->rgb)
+ rockchip_rgb_fini(vop2->rgb);
+
vop2_destroy_crtcs(vop2);
}
--
2.30.2
[PATCH v3 4/6] drm/rockchip: vop2: use symmetric function pair vop2_{create, destroy}_crtcs
Let the function name vop2_create_crtcs reflect that the function creates
multiple CRTCS. Also, use a symmetric function pair to create and destroy
the CRTCs and the corresponding planes.
Signed-off-by: Michael Riesch
---
v3:
- no changes
v2:
- no changes
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 31 ++--
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 374ef821b453..06fcdfa7b885 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2246,7 +2246,7 @@ static struct vop2_video_port
*find_vp_without_primary(struct vop2 *vop2)
#define NR_LAYERS 6
-static int vop2_create_crtc(struct vop2 *vop2)
+static int vop2_create_crtcs(struct vop2 *vop2)
{
const struct vop2_data *vop2_data = vop2->data;
struct drm_device *drm = vop2->drm;
@@ -2372,15 +2372,25 @@ static int vop2_create_crtc(struct vop2 *vop2)
return 0;
}
-static void vop2_destroy_crtc(struct drm_crtc *crtc)
+static void vop2_destroy_crtcs(struct vop2 *vop2)
{
- of_node_put(crtc->port);
+ struct drm_device *drm = vop2->drm;
+ struct list_head *crtc_list = >mode_config.crtc_list;
+ struct list_head *plane_list = >mode_config.plane_list;
+ struct drm_crtc *crtc, *tmpc;
+ struct drm_plane *plane, *tmpp;
+
+ list_for_each_entry_safe(plane, tmpp, plane_list, head)
+ drm_plane_cleanup(plane);
/*
* Destroy CRTC after vop2_plane_destroy() since vop2_disable_plane()
* references the CRTC.
*/
- drm_crtc_cleanup(crtc);
+ list_for_each_entry_safe(crtc, tmpc, crtc_list, head) {
+ of_node_put(crtc->port);
+ drm_crtc_cleanup(crtc);
+ }
}
static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
@@ -2684,7 +2694,7 @@ static int vop2_bind(struct device *dev, struct device
*master, void *data)
if (ret)
return ret;
- ret = vop2_create_crtc(vop2);
+ ret = vop2_create_crtcs(vop2);
if (ret)
return ret;
@@ -2698,19 +2708,10 @@ static int vop2_bind(struct device *dev, struct device
*master, void *data)
static void vop2_unbind(struct device *dev, struct device *master, void *data)
{
struct vop2 *vop2 = dev_get_drvdata(dev);
- struct drm_device *drm = vop2->drm;
- struct list_head *plane_list = >mode_config.plane_list;
- struct list_head *crtc_list = >mode_config.crtc_list;
- struct drm_crtc *crtc, *tmpc;
- struct drm_plane *plane, *tmpp;
pm_runtime_disable(dev);
- list_for_each_entry_safe(plane, tmpp, plane_list, head)
- drm_plane_cleanup(plane);
-
- list_for_each_entry_safe(crtc, tmpc, crtc_list, head)
- vop2_destroy_crtc(crtc);
+ vop2_destroy_crtcs(vop2);
}
const struct component_ops vop2_component_ops = {
--
2.30.2
[PATCH v3 6/6] arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to rk356x
The rk3568-pinctrl.dtsi only defines the 24-bit RGB interface. Add separate
nodes for the 16-bit and 18-bit version, respectively. While at it, split
off the clock/sync signals from the data signals.
The exact mapping of the data pins was discussed here:
https://lore.kernel.org/linux-rockchip/f33a0488-528c-99de-3279-3c0346a03...@wolfvision.net/T/
Signed-off-by: Michael Riesch
---
v3:
- no changes
v2:
- no changes
.../boot/dts/rockchip/rk3568-pinctrl.dtsi | 94 +++
1 file changed, 94 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
index 8f90c66dd9e9..0a979bfb63d9 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
@@ -3117,4 +3117,98 @@ tsadc_pin: tsadc-pin {
<0 RK_PA1 0 _pull_none>;
};
};
+
+ lcdc {
+ /omit-if-no-ref/
+ lcdc_clock: lcdc-clock {
+ rockchip,pins =
+ /* lcdc_clk */
+ <3 RK_PA0 1 _pull_none>,
+ /* lcdc_den */
+ <3 RK_PC3 1 _pull_none>,
+ /* lcdc_hsync */
+ <3 RK_PC1 1 _pull_none>,
+ /* lcdc_vsync */
+ <3 RK_PC2 1 _pull_none>;
+ };
+
+ /omit-if-no-ref/
+ lcdc_data16: lcdc-data16 {
+ rockchip,pins =
+ /* lcdc_d3 */
+ <2 RK_PD3 1 _pull_none>,
+ /* lcdc_d4 */
+ <2 RK_PD4 1 _pull_none>,
+ /* lcdc_d5 */
+ <2 RK_PD5 1 _pull_none>,
+ /* lcdc_d6 */
+ <2 RK_PD6 1 _pull_none>,
+ /* lcdc_d7 */
+ <2 RK_PD7 1 _pull_none>,
+ /* lcdc_d10 */
+ <3 RK_PA3 1 _pull_none>,
+ /* lcdc_d11 */
+ <3 RK_PA4 1 _pull_none>,
+ /* lcdc_d12 */
+ <3 RK_PA5 1 _pull_none>,
+ /* lcdc_d13 */
+ <3 RK_PA6 1 _pull_none>,
+ /* lcdc_d14 */
+ <3 RK_PA7 1 _pull_none>,
+ /* lcdc_d15 */
+ <3 RK_PB0 1 _pull_none>,
+ /* lcdc_d19 */
+ <3 RK_PB4 1 _pull_none>,
+ /* lcdc_d20 */
+ <3 RK_PB5 1 _pull_none>,
+ /* lcdc_d21 */
+ <3 RK_PB6 1 _pull_none>,
+ /* lcdc_d22 */
+ <3 RK_PB7 1 _pull_none>,
+ /* lcdc_d23 */
+ <3 RK_PC0 1 _pull_none>;
+ };
+
+ /omit-if-no-ref/
+ lcdc_data18: lcdc-data18 {
+ rockchip,pins =
+ /* lcdc_d2 */
+ <2 RK_PD2 1 _pull_none>,
+ /* lcdc_d3 */
+ <2 RK_PD3 1 _pull_none>,
+ /* lcdc_d4 */
+ <2 RK_PD4 1 _pull_none>,
+ /* lcdc_d5 */
+ <2 RK_PD5 1 _pull_none>,
+ /* lcdc_d6 */
+ <2 RK_PD6 1 _pull_none>,
+ /* lcdc_d7 */
+ <2 RK_PD7 1 _pull_none>,
+ /* lcdc_d10 */
+ <3 RK_PA3 1 _pull_none>,
+ /* lcdc_d11 */
+ <3 RK_PA4 1 _pull_none>,
+ /* lcdc_d12 */
+ <3 RK_PA5 1 _pull_none>,
+ /* lcdc_d13 */
+ <3 RK_PA6 1 _pull_none>,
+ /* lcdc_d14 */
+ <3 RK_PA7 1 _pull_none>,
+ /* lcdc_d15 */
+ <3 RK_PB0 1 _pull_none>,
+ /* lcdc_d18 */
+ <3 RK_PB3 1 _pull_none>,
+
[PATCH v3 3/6] drm/rockchip: rgb: add video_port parameter to init function
The VOP2 driver has more than one video port, hence the hard-coded
port id will not work anymore. Add an extra parameter for the video
port id to the rockchip_rgb_init function.
Signed-off-by: Michael Riesch
---
v3:
- no changes
v2:
- no changes
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_rgb.c | 9 +
drivers/gpu/drm/rockchip/rockchip_rgb.h | 6 --
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fa1f4ee6d195..5d18dea5c8d6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -2221,7 +2221,7 @@ static int vop_bind(struct device *dev, struct device
*master, void *data)
goto err_disable_pm_runtime;
if (vop->data->feature & VOP_FEATURE_INTERNAL_RGB) {
- vop->rgb = rockchip_rgb_init(dev, >crtc, vop->drm_dev);
+ vop->rgb = rockchip_rgb_init(dev, >crtc, vop->drm_dev, 0);
if (IS_ERR(vop->rgb)) {
ret = PTR_ERR(vop->rgb);
goto err_disable_pm_runtime;
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c
b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 5971df4302f2..c677b71ae516 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -72,7 +72,8 @@ struct drm_encoder_helper_funcs
rockchip_rgb_encoder_helper_funcs = {
struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
struct drm_crtc *crtc,
- struct drm_device *drm_dev)
+ struct drm_device *drm_dev,
+ int video_port)
{
struct rockchip_rgb *rgb;
struct drm_encoder *encoder;
@@ -90,7 +91,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
rgb->dev = dev;
rgb->drm_dev = drm_dev;
- port = of_graph_get_port_by_id(dev->of_node, 0);
+ port = of_graph_get_port_by_id(dev->of_node, video_port);
if (!port)
return ERR_PTR(-EINVAL);
@@ -103,8 +104,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
continue;
child_count++;
- ret = drm_of_find_panel_or_bridge(dev->of_node, 0, endpoint_id,
- , );
+ ret = drm_of_find_panel_or_bridge(dev->of_node, video_port,
+ endpoint_id, , );
if (!ret) {
of_node_put(endpoint);
break;
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.h
b/drivers/gpu/drm/rockchip/rockchip_rgb.h
index 27b9635124bc..1bd4e20e91eb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.h
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.h
@@ -8,12 +8,14 @@
#ifdef CONFIG_ROCKCHIP_RGB
struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
struct drm_crtc *crtc,
- struct drm_device *drm_dev);
+ struct drm_device *drm_dev,
+ int video_port);
void rockchip_rgb_fini(struct rockchip_rgb *rgb);
#else
static inline struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
struct drm_crtc *crtc,
-struct drm_device *drm_dev)
+struct drm_device *drm_dev,
+int video_port)
{
return NULL;
}
--
2.30.2
[PATCH v3 2/6] drm/rockchip: rgb: embed drm_encoder into rockchip_encoder
Commit 540b8f271e53 ("drm/rockchip: Embed drm_encoder into
rockchip_decoder") provides the means to pass the endpoint ID to the
VOP2 driver, which sets the interface MUX accordingly. However, this
step has not yet been carried out for the RGB output block. Embed the
drm_encoder structure into the rockchip_encoder structure and set the
endpoint ID correctly.
Signed-off-by: Michael Riesch
---
v3:
- no changes
v2:
- use endpoint id from device tree instead of hardcoded value
drivers/gpu/drm/rockchip/rockchip_rgb.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c
b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 75eb7cca3d82..5971df4302f2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -22,13 +22,11 @@
#include "rockchip_drm_vop.h"
#include "rockchip_rgb.h"
-#define encoder_to_rgb(c) container_of(c, struct rockchip_rgb, encoder)
-
struct rockchip_rgb {
struct device *dev;
struct drm_device *drm_dev;
struct drm_bridge *bridge;
- struct drm_encoder encoder;
+ struct rockchip_encoder encoder;
struct drm_connector connector;
int output_mode;
};
@@ -125,7 +123,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
return ERR_PTR(ret);
}
- encoder = >encoder;
+ encoder = >encoder.encoder;
encoder->possible_crtcs = drm_crtc_mask(crtc);
ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_NONE);
@@ -161,6 +159,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
goto err_free_encoder;
}
+ rgb->encoder.crtc_endpoint_id = endpoint_id;
+
ret = drm_connector_attach_encoder(connector, encoder);
if (ret < 0) {
DRM_DEV_ERROR(drm_dev->dev,
@@ -182,6 +182,6 @@ void rockchip_rgb_fini(struct rockchip_rgb *rgb)
{
drm_panel_bridge_remove(rgb->bridge);
drm_connector_cleanup(>connector);
- drm_encoder_cleanup(>encoder);
+ drm_encoder_cleanup(>encoder.encoder);
}
EXPORT_SYMBOL_GPL(rockchip_rgb_fini);
--
2.30.2
[PATCH v3 1/6] drm/rockchip: vop2: initialize possible_crtcs properly
The variable possible_crtcs is only initialized for primary and
overlay planes. Since the VOP2 driver only supports these plane
types at the moment, the current code is safe. However, in order
to provide a future-proof solution, fix the initialization of
the variable.
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Signed-off-by: Michael Riesch
---
v3:
- no changes
v2:
- new patch
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 8cecf81a5ae0..374ef821b453 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2322,10 +2322,11 @@ static int vop2_create_crtc(struct vop2 *vop2)
/* change the unused primary window to overlay
window */
win->type = DRM_PLANE_TYPE_OVERLAY;
}
- }
-
- if (win->type == DRM_PLANE_TYPE_OVERLAY)
+ } else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
possible_crtcs = (1 << nvps) - 1;
+ } else {
+ possible_crtcs = 0;
+ }
ret = vop2_plane_init(vop2, win, possible_crtcs);
if (ret) {
--
2.30.2
[PATCH v3 0/6] drm/rockchip: vop2: add support for the rgb output block
Hi all,
This series adds support for the RGB output block that can be found in the
Rockchip Video Output Processor (VOP) 2. Version 2 of this series
incorporates the feedback by Dan Carpenter and Sascha Hauer. Version 3
fixes a dumb mistake pointed out by Sascha :-) Thanks for your comments!
Patches 1-4 clean up the code and make it more general.
Patch 5 activates the support for the RGB output block in the VOP2 driver.
Patch 6 adds pinctrls for the 16-bit and 18-bit RGB data lines.
Tested on a custom board featuring the RK3568 SoC with a 18-bit RGB
display.
Looking forward to your comments!
Best regards,
Michael
Michael Riesch (6):
drm/rockchip: vop2: initialize possible_crtcs properly
drm/rockchip: rgb: embed drm_encoder into rockchip_encoder
drm/rockchip: rgb: add video_port parameter to init function
drm/rockchip: vop2: use symmetric function pair
vop2_{create,destroy}_crtcs
drm/rockchip: vop2: add support for the rgb output block
arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to
rk356x
.../boot/dts/rockchip/rk3568-pinctrl.dtsi | 94 +++
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 80
drivers/gpu/drm/rockchip/rockchip_rgb.c | 19 ++--
drivers/gpu/drm/rockchip/rockchip_rgb.h | 6 +-
5 files changed, 172 insertions(+), 29 deletions(-)
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
--
2.30.2
Re: [PATCH v2 5/6] drm/rockchip: vop2: add support for the rgb output block
Hi Sascha,
On 1/19/23 16:17, Sascha Hauer wrote:
> Hi Michael,
>
> On Thu, Jan 19, 2023 at 03:39:10PM +0100, Michael Riesch wrote:
>> The Rockchip VOP2 features an internal RGB output block, which can be
>> attached to the video port 2 of the VOP2. Add support for this output
>> block.
>>
>> Signed-off-by: Michael Riesch
>> ---
>> v2:
>> - move away from wrong assumption that the RGB block is always
>>connected to video port 2 -> check devicetree to find RGB block
>
> Traces of that assumption are still in the commmit message.
Oops, this needs fixing of course...
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 44
>> 1 file changed, 44 insertions(+)
>>
>> +static int vop2_find_rgb_encoder(struct vop2 *vop2)
>> +{
>> +struct device_node *node = vop2->dev->of_node;
>> +struct device_node *endpoint;
>> +int i;
>> +
>> +for (i = 0; i < vop2->data->nr_vps; i++) {
>> +endpoint = of_graph_get_endpoint_by_regs(node, i,
>> + ROCKCHIP_VOP2_EP_RGB0);
>> +if (!endpoint)
>> +continue;
>> +
>> +of_node_put(endpoint);
>> +return i;
>> +}
>> +
>> +return -ENOENT;
>> +}
>> +
>> static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
>> [VOP2_WIN_ENABLE] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 0, 0),
>> [VOP2_WIN_FORMAT] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 1, 5),
>> @@ -2698,11 +2721,29 @@ static int vop2_bind(struct device *dev, struct
>> device *master, void *data)
>> if (ret)
>> return ret;
>>
>> +ret = vop2_find_rgb_encoder(vop2);
>> +if (ret > 0) {
>
> '0' seems to be a valid vp as well. Shouldn't this be ret >= 0?
...and you are right of course. What a stupid mistake.
Will fix these in v3.
Best regards,
Michael
>
>> +vop2->rgb = rockchip_rgb_init(dev, >vps[ret].crtc,
>> + vop2->drm, ret);
>> +if (IS_ERR(vop2->rgb)) {
>> +if (PTR_ERR(vop2->rgb) == -EPROBE_DEFER) {
>> +ret = PTR_ERR(vop2->rgb);
>> +goto err_crtcs;
>> +}
>> +vop2->rgb = NULL;
>> +}
>> +}
>> +
>
> Sascha
>
[PATCH v2 2/6] drm/rockchip: rgb: embed drm_encoder into rockchip_encoder
Commit 540b8f271e53 ("drm/rockchip: Embed drm_encoder into
rockchip_decoder") provides the means to pass the endpoint ID to the
VOP2 driver, which sets the interface MUX accordingly. However, this
step has not yet been carried out for the RGB output block. Embed the
drm_encoder structure into the rockchip_encoder structure and set the
endpoint ID correctly.
Signed-off-by: Michael Riesch
---
v2:
- use endpoint id from device tree instead of hardcoded value
drivers/gpu/drm/rockchip/rockchip_rgb.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c
b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 75eb7cca3d82..5971df4302f2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -22,13 +22,11 @@
#include "rockchip_drm_vop.h"
#include "rockchip_rgb.h"
-#define encoder_to_rgb(c) container_of(c, struct rockchip_rgb, encoder)
-
struct rockchip_rgb {
struct device *dev;
struct drm_device *drm_dev;
struct drm_bridge *bridge;
- struct drm_encoder encoder;
+ struct rockchip_encoder encoder;
struct drm_connector connector;
int output_mode;
};
@@ -125,7 +123,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
return ERR_PTR(ret);
}
- encoder = >encoder;
+ encoder = >encoder.encoder;
encoder->possible_crtcs = drm_crtc_mask(crtc);
ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_NONE);
@@ -161,6 +159,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
goto err_free_encoder;
}
+ rgb->encoder.crtc_endpoint_id = endpoint_id;
+
ret = drm_connector_attach_encoder(connector, encoder);
if (ret < 0) {
DRM_DEV_ERROR(drm_dev->dev,
@@ -182,6 +182,6 @@ void rockchip_rgb_fini(struct rockchip_rgb *rgb)
{
drm_panel_bridge_remove(rgb->bridge);
drm_connector_cleanup(>connector);
- drm_encoder_cleanup(>encoder);
+ drm_encoder_cleanup(>encoder.encoder);
}
EXPORT_SYMBOL_GPL(rockchip_rgb_fini);
--
2.30.2
[PATCH v2 5/6] drm/rockchip: vop2: add support for the rgb output block
The Rockchip VOP2 features an internal RGB output block, which can be
attached to the video port 2 of the VOP2. Add support for this output
block.
Signed-off-by: Michael Riesch
---
v2:
- move away from wrong assumption that the RGB block is always
connected to video port 2 -> check devicetree to find RGB block
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 44
1 file changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 06fcdfa7b885..f30037d161ae 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -39,6 +39,7 @@
#include "rockchip_drm_gem.h"
#include "rockchip_drm_fb.h"
#include "rockchip_drm_vop2.h"
+#include "rockchip_rgb.h"
/*
* VOP2 architecture
@@ -212,6 +213,9 @@ struct vop2 {
struct clk *hclk;
struct clk *aclk;
+ /* optional internal rgb encoder */
+ struct rockchip_rgb *rgb;
+
/* must be put at the end of the struct */
struct vop2_win win[];
};
@@ -2393,6 +2397,25 @@ static void vop2_destroy_crtcs(struct vop2 *vop2)
}
}
+static int vop2_find_rgb_encoder(struct vop2 *vop2)
+{
+ struct device_node *node = vop2->dev->of_node;
+ struct device_node *endpoint;
+ int i;
+
+ for (i = 0; i < vop2->data->nr_vps; i++) {
+ endpoint = of_graph_get_endpoint_by_regs(node, i,
+ROCKCHIP_VOP2_EP_RGB0);
+ if (!endpoint)
+ continue;
+
+ of_node_put(endpoint);
+ return i;
+ }
+
+ return -ENOENT;
+}
+
static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
[VOP2_WIN_ENABLE] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 0, 0),
[VOP2_WIN_FORMAT] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 1, 5),
@@ -2698,11 +2721,29 @@ static int vop2_bind(struct device *dev, struct device
*master, void *data)
if (ret)
return ret;
+ ret = vop2_find_rgb_encoder(vop2);
+ if (ret > 0) {
+ vop2->rgb = rockchip_rgb_init(dev, >vps[ret].crtc,
+ vop2->drm, ret);
+ if (IS_ERR(vop2->rgb)) {
+ if (PTR_ERR(vop2->rgb) == -EPROBE_DEFER) {
+ ret = PTR_ERR(vop2->rgb);
+ goto err_crtcs;
+ }
+ vop2->rgb = NULL;
+ }
+ }
+
rockchip_drm_dma_init_device(vop2->drm, vop2->dev);
pm_runtime_enable(>dev);
return 0;
+
+err_crtcs:
+ vop2_destroy_crtcs(vop2);
+
+ return ret;
}
static void vop2_unbind(struct device *dev, struct device *master, void *data)
@@ -2711,6 +2752,9 @@ static void vop2_unbind(struct device *dev, struct device
*master, void *data)
pm_runtime_disable(dev);
+ if (vop2->rgb)
+ rockchip_rgb_fini(vop2->rgb);
+
vop2_destroy_crtcs(vop2);
}
--
2.30.2
[PATCH v2 4/6] drm/rockchip: vop2: use symmetric function pair vop2_{create, destroy}_crtcs
Let the function name vop2_create_crtcs reflect that the function creates
multiple CRTCS. Also, use a symmetric function pair to create and destroy
the CRTCs and the corresponding planes.
Signed-off-by: Michael Riesch
---
v2:
- no changes
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 31 ++--
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 374ef821b453..06fcdfa7b885 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2246,7 +2246,7 @@ static struct vop2_video_port
*find_vp_without_primary(struct vop2 *vop2)
#define NR_LAYERS 6
-static int vop2_create_crtc(struct vop2 *vop2)
+static int vop2_create_crtcs(struct vop2 *vop2)
{
const struct vop2_data *vop2_data = vop2->data;
struct drm_device *drm = vop2->drm;
@@ -2372,15 +2372,25 @@ static int vop2_create_crtc(struct vop2 *vop2)
return 0;
}
-static void vop2_destroy_crtc(struct drm_crtc *crtc)
+static void vop2_destroy_crtcs(struct vop2 *vop2)
{
- of_node_put(crtc->port);
+ struct drm_device *drm = vop2->drm;
+ struct list_head *crtc_list = >mode_config.crtc_list;
+ struct list_head *plane_list = >mode_config.plane_list;
+ struct drm_crtc *crtc, *tmpc;
+ struct drm_plane *plane, *tmpp;
+
+ list_for_each_entry_safe(plane, tmpp, plane_list, head)
+ drm_plane_cleanup(plane);
/*
* Destroy CRTC after vop2_plane_destroy() since vop2_disable_plane()
* references the CRTC.
*/
- drm_crtc_cleanup(crtc);
+ list_for_each_entry_safe(crtc, tmpc, crtc_list, head) {
+ of_node_put(crtc->port);
+ drm_crtc_cleanup(crtc);
+ }
}
static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
@@ -2684,7 +2694,7 @@ static int vop2_bind(struct device *dev, struct device
*master, void *data)
if (ret)
return ret;
- ret = vop2_create_crtc(vop2);
+ ret = vop2_create_crtcs(vop2);
if (ret)
return ret;
@@ -2698,19 +2708,10 @@ static int vop2_bind(struct device *dev, struct device
*master, void *data)
static void vop2_unbind(struct device *dev, struct device *master, void *data)
{
struct vop2 *vop2 = dev_get_drvdata(dev);
- struct drm_device *drm = vop2->drm;
- struct list_head *plane_list = >mode_config.plane_list;
- struct list_head *crtc_list = >mode_config.crtc_list;
- struct drm_crtc *crtc, *tmpc;
- struct drm_plane *plane, *tmpp;
pm_runtime_disable(dev);
- list_for_each_entry_safe(plane, tmpp, plane_list, head)
- drm_plane_cleanup(plane);
-
- list_for_each_entry_safe(crtc, tmpc, crtc_list, head)
- vop2_destroy_crtc(crtc);
+ vop2_destroy_crtcs(vop2);
}
const struct component_ops vop2_component_ops = {
--
2.30.2
[PATCH v2 1/6] drm/rockchip: vop2: initialize possible_crtcs properly
The variable possible_crtcs is only initialized for primary and
overlay planes. Since the VOP2 driver only supports these plane
types at the moment, the current code is safe. However, in order
to provide a future-proof solution, fix the initialization of
the variable.
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Signed-off-by: Michael Riesch
---
v2:
- new patch
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 8cecf81a5ae0..374ef821b453 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2322,10 +2322,11 @@ static int vop2_create_crtc(struct vop2 *vop2)
/* change the unused primary window to overlay
window */
win->type = DRM_PLANE_TYPE_OVERLAY;
}
- }
-
- if (win->type == DRM_PLANE_TYPE_OVERLAY)
+ } else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
possible_crtcs = (1 << nvps) - 1;
+ } else {
+ possible_crtcs = 0;
+ }
ret = vop2_plane_init(vop2, win, possible_crtcs);
if (ret) {
--
2.30.2
[PATCH v2 6/6] arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to rk356x
The rk3568-pinctrl.dtsi only defines the 24-bit RGB interface. Add separate
nodes for the 16-bit and 18-bit version, respectively. While at it, split
off the clock/sync signals from the data signals.
The exact mapping of the data pins was discussed here:
https://lore.kernel.org/linux-rockchip/f33a0488-528c-99de-3279-3c0346a03...@wolfvision.net/T/
Signed-off-by: Michael Riesch
---
v2:
- no changes
.../boot/dts/rockchip/rk3568-pinctrl.dtsi | 94 +++
1 file changed, 94 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
index 8f90c66dd9e9..0a979bfb63d9 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
@@ -3117,4 +3117,98 @@ tsadc_pin: tsadc-pin {
<0 RK_PA1 0 _pull_none>;
};
};
+
+ lcdc {
+ /omit-if-no-ref/
+ lcdc_clock: lcdc-clock {
+ rockchip,pins =
+ /* lcdc_clk */
+ <3 RK_PA0 1 _pull_none>,
+ /* lcdc_den */
+ <3 RK_PC3 1 _pull_none>,
+ /* lcdc_hsync */
+ <3 RK_PC1 1 _pull_none>,
+ /* lcdc_vsync */
+ <3 RK_PC2 1 _pull_none>;
+ };
+
+ /omit-if-no-ref/
+ lcdc_data16: lcdc-data16 {
+ rockchip,pins =
+ /* lcdc_d3 */
+ <2 RK_PD3 1 _pull_none>,
+ /* lcdc_d4 */
+ <2 RK_PD4 1 _pull_none>,
+ /* lcdc_d5 */
+ <2 RK_PD5 1 _pull_none>,
+ /* lcdc_d6 */
+ <2 RK_PD6 1 _pull_none>,
+ /* lcdc_d7 */
+ <2 RK_PD7 1 _pull_none>,
+ /* lcdc_d10 */
+ <3 RK_PA3 1 _pull_none>,
+ /* lcdc_d11 */
+ <3 RK_PA4 1 _pull_none>,
+ /* lcdc_d12 */
+ <3 RK_PA5 1 _pull_none>,
+ /* lcdc_d13 */
+ <3 RK_PA6 1 _pull_none>,
+ /* lcdc_d14 */
+ <3 RK_PA7 1 _pull_none>,
+ /* lcdc_d15 */
+ <3 RK_PB0 1 _pull_none>,
+ /* lcdc_d19 */
+ <3 RK_PB4 1 _pull_none>,
+ /* lcdc_d20 */
+ <3 RK_PB5 1 _pull_none>,
+ /* lcdc_d21 */
+ <3 RK_PB6 1 _pull_none>,
+ /* lcdc_d22 */
+ <3 RK_PB7 1 _pull_none>,
+ /* lcdc_d23 */
+ <3 RK_PC0 1 _pull_none>;
+ };
+
+ /omit-if-no-ref/
+ lcdc_data18: lcdc-data18 {
+ rockchip,pins =
+ /* lcdc_d2 */
+ <2 RK_PD2 1 _pull_none>,
+ /* lcdc_d3 */
+ <2 RK_PD3 1 _pull_none>,
+ /* lcdc_d4 */
+ <2 RK_PD4 1 _pull_none>,
+ /* lcdc_d5 */
+ <2 RK_PD5 1 _pull_none>,
+ /* lcdc_d6 */
+ <2 RK_PD6 1 _pull_none>,
+ /* lcdc_d7 */
+ <2 RK_PD7 1 _pull_none>,
+ /* lcdc_d10 */
+ <3 RK_PA3 1 _pull_none>,
+ /* lcdc_d11 */
+ <3 RK_PA4 1 _pull_none>,
+ /* lcdc_d12 */
+ <3 RK_PA5 1 _pull_none>,
+ /* lcdc_d13 */
+ <3 RK_PA6 1 _pull_none>,
+ /* lcdc_d14 */
+ <3 RK_PA7 1 _pull_none>,
+ /* lcdc_d15 */
+ <3 RK_PB0 1 _pull_none>,
+ /* lcdc_d18 */
+ <3 RK_PB3 1 _pull_none>,
+ /* lcdc_d19 */
[PATCH v2 3/6] drm/rockchip: rgb: add video_port parameter to init function
The VOP2 driver has more than one video port, hence the hard-coded
port id will not work anymore. Add an extra parameter for the video
port id to the rockchip_rgb_init function.
Signed-off-by: Michael Riesch
---
v2:
- no changes
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_rgb.c | 9 +
drivers/gpu/drm/rockchip/rockchip_rgb.h | 6 --
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fa1f4ee6d195..5d18dea5c8d6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -2221,7 +2221,7 @@ static int vop_bind(struct device *dev, struct device
*master, void *data)
goto err_disable_pm_runtime;
if (vop->data->feature & VOP_FEATURE_INTERNAL_RGB) {
- vop->rgb = rockchip_rgb_init(dev, >crtc, vop->drm_dev);
+ vop->rgb = rockchip_rgb_init(dev, >crtc, vop->drm_dev, 0);
if (IS_ERR(vop->rgb)) {
ret = PTR_ERR(vop->rgb);
goto err_disable_pm_runtime;
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c
b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 5971df4302f2..c677b71ae516 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -72,7 +72,8 @@ struct drm_encoder_helper_funcs
rockchip_rgb_encoder_helper_funcs = {
struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
struct drm_crtc *crtc,
- struct drm_device *drm_dev)
+ struct drm_device *drm_dev,
+ int video_port)
{
struct rockchip_rgb *rgb;
struct drm_encoder *encoder;
@@ -90,7 +91,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
rgb->dev = dev;
rgb->drm_dev = drm_dev;
- port = of_graph_get_port_by_id(dev->of_node, 0);
+ port = of_graph_get_port_by_id(dev->of_node, video_port);
if (!port)
return ERR_PTR(-EINVAL);
@@ -103,8 +104,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
continue;
child_count++;
- ret = drm_of_find_panel_or_bridge(dev->of_node, 0, endpoint_id,
- , );
+ ret = drm_of_find_panel_or_bridge(dev->of_node, video_port,
+ endpoint_id, , );
if (!ret) {
of_node_put(endpoint);
break;
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.h
b/drivers/gpu/drm/rockchip/rockchip_rgb.h
index 27b9635124bc..1bd4e20e91eb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.h
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.h
@@ -8,12 +8,14 @@
#ifdef CONFIG_ROCKCHIP_RGB
struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
struct drm_crtc *crtc,
- struct drm_device *drm_dev);
+ struct drm_device *drm_dev,
+ int video_port);
void rockchip_rgb_fini(struct rockchip_rgb *rgb);
#else
static inline struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
struct drm_crtc *crtc,
-struct drm_device *drm_dev)
+struct drm_device *drm_dev,
+int video_port)
{
return NULL;
}
--
2.30.2
[PATCH v2 0/6] drm/rockchip: vop2: add support for the rgb output block
Hi all,
This series adds support for the RGB output block that can be found in the
Rockchip Video Output Processor (VOP) 2. Version 2 of this series incorporates
the feedback by Dan Carpenter and Sascha Hauer. Thanks for your comments!
Patches 1-4 clean up the code and make it more general.
Patch 5 activates the support for the RGB output block in the VOP2 driver.
Patch 6 adds pinctrls for the 16-bit and 18-bit RGB data lines.
Tested on a custom board featuring the RK3568 SoC with a 18-bit RGB
display.
Looking forward to your comments!
Best regards,
Michael
Michael Riesch (6):
drm/rockchip: vop2: initialize possible_crtcs properly
drm/rockchip: rgb: embed drm_encoder into rockchip_encoder
drm/rockchip: rgb: add video_port parameter to init function
drm/rockchip: vop2: use symmetric function pair
vop2_{create,destroy}_crtcs
drm/rockchip: vop2: add support for the rgb output block
arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to
rk356x
.../boot/dts/rockchip/rk3568-pinctrl.dtsi | 94 +++
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 80
drivers/gpu/drm/rockchip/rockchip_rgb.c | 19 ++--
drivers/gpu/drm/rockchip/rockchip_rgb.h | 6 +-
5 files changed, 172 insertions(+), 29 deletions(-)
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
--
2.30.2
Re: [PATCH 1/5] drm/rockchip: rgb: embed drm_encoder into rockchip_encoder
Hi Sascha,
Thanks for your comments!
On 12/7/22 07:45, Sascha Hauer wrote:
> On Wed, Nov 30, 2022 at 03:02:13PM +0100, Michael Riesch wrote:
>> Commit 540b8f271e53 ("drm/rockchip: Embed drm_encoder into
>> rockchip_decoder") provides the means to pass the endpoint ID to the
>> VOP2 driver, which sets the interface MUX accordingly. However, this
>> step has not yet been carried out for the RGB output block. Embed the
>> drm_encoder structure into the rockchip_encoder structure and set the
>> endpoint ID correctly.
>>
>> Signed-off-by: Michael Riesch
>> ---
>> drivers/gpu/drm/rockchip/rockchip_rgb.c | 12 +++-
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> b/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> index 75eb7cca3d82..16201a5cf1e8 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> @@ -18,17 +18,17 @@
>> #include
>> #include
>>
>> +#include
>> +
>> #include "rockchip_drm_drv.h"
>> #include "rockchip_drm_vop.h"
>> #include "rockchip_rgb.h"
>>
>> -#define encoder_to_rgb(c) container_of(c, struct rockchip_rgb, encoder)
>> -
>> struct rockchip_rgb {
>> struct device *dev;
>> struct drm_device *drm_dev;
>> struct drm_bridge *bridge;
>> -struct drm_encoder encoder;
>> +struct rockchip_encoder encoder;
>> struct drm_connector connector;
>> int output_mode;
>> };
>> @@ -125,7 +125,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device
>> *dev,
>> return ERR_PTR(ret);
>> }
>>
>> -encoder = >encoder;
>> +encoder = >encoder.encoder;
>> encoder->possible_crtcs = drm_crtc_mask(crtc);
>>
>> ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_NONE);
>> @@ -161,6 +161,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device
>> *dev,
>> goto err_free_encoder;
>> }
>>
>> +rgb->encoder.crtc_endpoint_id = ROCKCHIP_VOP2_EP_RGB0;
>
> This is vop2 specific. This code is used with the vop1 as well, so it
> doesn't look like it belongs here, at least not hidden in a patch titled
> "embed drm_encoder into rockchip_encoder".
OK, the very least I can do is to create an extra patch that sets the
crtc_endpoint_id.
> Normally the crtc_endpoint_id is set by the encoder, coming from the
> encoder node, asking the question "To which port on the VOP am I
> connected to?"
>
> Here the situation is different. We are called from the VOP and the
> question instead is: "Is there something connected to VPx endpoint id
> ROCKCHIP_VOP2_EP_RGB0?"
>
> You might need a vop2 specific entry to this code.
I just realized that rockchip_rgb_init parses the endpoint ID. If I am
not mistaken, we can directly store it in the crtc_endpoint_id member.
Seeing that this would be pretty generic, can it be included in one
patch (or should I still split this into a separate patch)?
Best regards,
Michael
Re: [PATCH 5/5] arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to rk356x
Hi Heiko,
On 11/30/22 15:02, Michael Riesch wrote:
> The rk3568-pinctrl.dtsi only defines the 24-bit RGB interface. Add separate
> nodes for the 16-bit and 18-bit version, respectively. While at it, split
> off the clock/sync signals from the data signals.
>
> The exact mapping of the data pins was discussed here:
> https://lore.kernel.org/linux-rockchip/f33a0488-528c-99de-3279-3c0346a03...@wolfvision.net/T/
>
> Signed-off-by: Michael Riesch
This patch is somewhat independent of the other patches of the series.
In particular it is not affected by the comments that this series has
received so far. If there are no objections, you might consider applying it.
Thanks and best regards,
Michael
> ---
> .../boot/dts/rockchip/rk3568-pinctrl.dtsi | 94 +++
> 1 file changed, 94 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
> index 8f90c66dd9e9..0a979bfb63d9 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
> @@ -3117,4 +3117,98 @@ tsadc_pin: tsadc-pin {
> <0 RK_PA1 0 _pull_none>;
> };
> };
> +
> + lcdc {
> + /omit-if-no-ref/
> + lcdc_clock: lcdc-clock {
> + rockchip,pins =
> + /* lcdc_clk */
> + <3 RK_PA0 1 _pull_none>,
> + /* lcdc_den */
> + <3 RK_PC3 1 _pull_none>,
> + /* lcdc_hsync */
> + <3 RK_PC1 1 _pull_none>,
> + /* lcdc_vsync */
> + <3 RK_PC2 1 _pull_none>;
> + };
> +
> + /omit-if-no-ref/
> + lcdc_data16: lcdc-data16 {
> + rockchip,pins =
> + /* lcdc_d3 */
> + <2 RK_PD3 1 _pull_none>,
> + /* lcdc_d4 */
> + <2 RK_PD4 1 _pull_none>,
> + /* lcdc_d5 */
> + <2 RK_PD5 1 _pull_none>,
> + /* lcdc_d6 */
> + <2 RK_PD6 1 _pull_none>,
> + /* lcdc_d7 */
> + <2 RK_PD7 1 _pull_none>,
> + /* lcdc_d10 */
> + <3 RK_PA3 1 _pull_none>,
> + /* lcdc_d11 */
> + <3 RK_PA4 1 _pull_none>,
> + /* lcdc_d12 */
> + <3 RK_PA5 1 _pull_none>,
> + /* lcdc_d13 */
> + <3 RK_PA6 1 _pull_none>,
> + /* lcdc_d14 */
> + <3 RK_PA7 1 _pull_none>,
> + /* lcdc_d15 */
> + <3 RK_PB0 1 _pull_none>,
> + /* lcdc_d19 */
> + <3 RK_PB4 1 _pull_none>,
> + /* lcdc_d20 */
> + <3 RK_PB5 1 _pull_none>,
> + /* lcdc_d21 */
> + <3 RK_PB6 1 _pull_none>,
> + /* lcdc_d22 */
> + <3 RK_PB7 1 _pull_none>,
> + /* lcdc_d23 */
> + <3 RK_PC0 1 _pull_none>;
> + };
> +
> + /omit-if-no-ref/
> + lcdc_data18: lcdc-data18 {
> + rockchip,pins =
> + /* lcdc_d2 */
> + <2 RK_PD2 1 _pull_none>,
> + /* lcdc_d3 */
> + <2 RK_PD3 1 _pull_none>,
> + /* lcdc_d4 */
> + <2 RK_PD4 1 _pull_none>,
> + /* lcdc_d5 */
> + <2 RK_PD5 1 _pull_none>,
> + /* lcdc_d6 */
> + <2 RK_PD6 1 _pull_none>,
> + /* lcdc_d7 */
> + <2 RK_PD7 1 _pull_none>,
> + /* lcdc_d10 */
> + <3 RK_PA3 1 _pull_none>,
> + /* lcdc_d11 */
> +
Re: [PATCH 3/5] drm/rockchip: vop2: use symmetric function pair vop2_{create,destroy}_crtcs
Hi Dan,
On 1/3/23 09:07, Dan Carpenter wrote:
> Hi Michael,
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Michael-Riesch/drm-rockchip-vop2-add-support-for-the-rgb-output-block/20221130-220346
> base: b7b275e60bcd5f89771e865a8239325f86d9927d
> patch link:
> https://lore.kernel.org/r/20221130140217.3196414-4-michael.riesch%40wolfvision.net
> patch subject: [PATCH 3/5] drm/rockchip: vop2: use symmetric function pair
> vop2_{create,destroy}_crtcs
> config: parisc-randconfig-m031-20221225
> compiler: hppa-linux-gcc (GCC) 12.1.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot
> | Reported-by: Dan Carpenter
Thanks for the report.
> New smatch warnings:
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2330 vop2_create_crtcs() error:
> uninitialized symbol 'possible_crtcs'.
>
> vim +/possible_crtcs +2330 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>
> fb83276e59f2d6 Michael Riesch 2022-11-30 2249 static int
> vop2_create_crtcs(struct vop2 *vop2)
> 604be85547ce4d Andy Yan 2022-04-22 2250 {
> 604be85547ce4d Andy Yan 2022-04-22 2251const struct vop2_data
> *vop2_data = vop2->data;
> 604be85547ce4d Andy Yan 2022-04-22 2252struct drm_device *drm
> = vop2->drm;
> 604be85547ce4d Andy Yan 2022-04-22 2253struct device *dev =
> vop2->dev;
> 604be85547ce4d Andy Yan 2022-04-22 2254struct drm_plane *plane;
> 604be85547ce4d Andy Yan 2022-04-22 2255struct device_node
> *port;
> 604be85547ce4d Andy Yan 2022-04-22 2256struct vop2_video_port
> *vp;
> 604be85547ce4d Andy Yan 2022-04-22 2257int i, nvp, nvps = 0;
> 604be85547ce4d Andy Yan 2022-04-22 2258int ret;
> 604be85547ce4d Andy Yan 2022-04-22 2259
> 604be85547ce4d Andy Yan 2022-04-22 2260for (i = 0; i <
> vop2_data->nr_vps; i++) {
> 604be85547ce4d Andy Yan 2022-04-22 2261const struct
> vop2_video_port_data *vp_data;
> 604be85547ce4d Andy Yan 2022-04-22 2262struct
> device_node *np;
> 604be85547ce4d Andy Yan 2022-04-22 2263char
> dclk_name[9];
> 604be85547ce4d Andy Yan 2022-04-22 2264
> 604be85547ce4d Andy Yan 2022-04-22 2265vp_data =
> _data->vp[i];
> 604be85547ce4d Andy Yan 2022-04-22 2266vp =
> >vps[i];
> 604be85547ce4d Andy Yan 2022-04-22 2267vp->vop2 = vop2;
> 604be85547ce4d Andy Yan 2022-04-22 2268vp->id =
> vp_data->id;
> 604be85547ce4d Andy Yan 2022-04-22 2269vp->regs =
> vp_data->regs;
> 604be85547ce4d Andy Yan 2022-04-22 2270vp->data =
> vp_data;
> 604be85547ce4d Andy Yan 2022-04-22 2271
> 604be85547ce4d Andy Yan 2022-04-22 2272
> snprintf(dclk_name, sizeof(dclk_name), "dclk_vp%d", vp->id);
> 604be85547ce4d Andy Yan 2022-04-22 2273vp->dclk =
> devm_clk_get(vop2->dev, dclk_name);
> 604be85547ce4d Andy Yan 2022-04-22 2274if
> (IS_ERR(vp->dclk)) {
> 604be85547ce4d Andy Yan 2022-04-22 2275
> drm_err(vop2->drm, "failed to get %s\n", dclk_name);
> 604be85547ce4d Andy Yan 2022-04-22 2276return
> PTR_ERR(vp->dclk);
> 604be85547ce4d Andy Yan 2022-04-22 2277}
> 604be85547ce4d Andy Yan 2022-04-22 2278
> 604be85547ce4d Andy Yan 2022-04-22 2279np =
> of_graph_get_remote_node(dev->of_node, i, -1);
> 604be85547ce4d Andy Yan 2022-04-22 2280if (!np) {
> 604be85547ce4d Andy Yan 2022-04-22 2281
> drm_dbg(vop2->drm, "%s: No remote for vp%d\n", __func__, i);
> 604be85547ce4d Andy Yan 2022-04-22 2282
> continue;
> 604be85547ce4d Andy Yan 2022-04-22 2283}
> 604be85547ce4d Andy Yan 2022-04-22 2284of_node_put(np);
> 604be85547ce4d Andy Yan 2022-04-22 2285
> 604be85547ce4d Andy Yan 2022-04-22 2286port =
> of_graph_get_port_by_id(dev->of_node, i);
> 604be85547ce4d Andy Yan 2022-04-22 2287if (!port) {
> 604be85547ce4d Andy Yan 2022-04-22 2288
> drm_err(vop2->drm, "no port node found for video_port%d\n", i);
> 604be85547ce4d Andy Yan 2022-04-22 2289return
> -ENOENT;
> 604be85547ce4d Andy Yan 2022-04-22 2290
Re: [PATCH v3 0/3] drm/rockchip: dw_hdmi: Add 4k@30 support
Hi Sascha, Thanks a lot for the v3, works great in my setup! On 1/18/23 14:22, Sascha Hauer wrote: > It's been some time since I last sent this series. This version fixes > a regression Dan Johansen reported. The reason turned out to be simple, > I used the YUV420 register values instead of the RGB ones. > > I realized that we cannot achieve several modes offered by my monitor > as these require pixelclocks that are slightly below the standard > pixelclocks. As these are lower than the standard clock rates the PLL > driver offers the clk driver falls back to a way lower frequency > which results in something the monitor can't display, so this series > now contains a patch to discard these unachievable modes. > > Sascha > > Changes since v2: > - Use correct register values for mpll_cfg > - Add patch to discard modes we cannot achieve > > Changes since v1: > - Allow non standard clock rates only on Synopsys phy as suggested by > Robin Murphy > > Sascha Hauer (3): > drm/rockchip: dw_hdmi: relax mode_valid hook > drm/rockchip: dw_hdmi: Add support for 4k@30 resolution > drm/rockchip: dw_hdmi: discard modes with unachievable pixelclocks For the complete series Tested-by: Michael Riesch Best regards, Michael > > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 40 - > 1 file changed, 32 insertions(+), 8 deletions(-) >
[PATCH 1/5] drm/rockchip: rgb: embed drm_encoder into rockchip_encoder
Commit 540b8f271e53 ("drm/rockchip: Embed drm_encoder into
rockchip_decoder") provides the means to pass the endpoint ID to the
VOP2 driver, which sets the interface MUX accordingly. However, this
step has not yet been carried out for the RGB output block. Embed the
drm_encoder structure into the rockchip_encoder structure and set the
endpoint ID correctly.
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/rockchip/rockchip_rgb.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c
b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 75eb7cca3d82..16201a5cf1e8 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -18,17 +18,17 @@
#include
#include
+#include
+
#include "rockchip_drm_drv.h"
#include "rockchip_drm_vop.h"
#include "rockchip_rgb.h"
-#define encoder_to_rgb(c) container_of(c, struct rockchip_rgb, encoder)
-
struct rockchip_rgb {
struct device *dev;
struct drm_device *drm_dev;
struct drm_bridge *bridge;
- struct drm_encoder encoder;
+ struct rockchip_encoder encoder;
struct drm_connector connector;
int output_mode;
};
@@ -125,7 +125,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
return ERR_PTR(ret);
}
- encoder = >encoder;
+ encoder = >encoder.encoder;
encoder->possible_crtcs = drm_crtc_mask(crtc);
ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_NONE);
@@ -161,6 +161,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
goto err_free_encoder;
}
+ rgb->encoder.crtc_endpoint_id = ROCKCHIP_VOP2_EP_RGB0;
+
ret = drm_connector_attach_encoder(connector, encoder);
if (ret < 0) {
DRM_DEV_ERROR(drm_dev->dev,
@@ -182,6 +184,6 @@ void rockchip_rgb_fini(struct rockchip_rgb *rgb)
{
drm_panel_bridge_remove(rgb->bridge);
drm_connector_cleanup(>connector);
- drm_encoder_cleanup(>encoder);
+ drm_encoder_cleanup(>encoder.encoder);
}
EXPORT_SYMBOL_GPL(rockchip_rgb_fini);
--
2.30.2
[PATCH 5/5] arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to rk356x
The rk3568-pinctrl.dtsi only defines the 24-bit RGB interface. Add separate
nodes for the 16-bit and 18-bit version, respectively. While at it, split
off the clock/sync signals from the data signals.
The exact mapping of the data pins was discussed here:
https://lore.kernel.org/linux-rockchip/f33a0488-528c-99de-3279-3c0346a03...@wolfvision.net/T/
Signed-off-by: Michael Riesch
---
.../boot/dts/rockchip/rk3568-pinctrl.dtsi | 94 +++
1 file changed, 94 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
index 8f90c66dd9e9..0a979bfb63d9 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
@@ -3117,4 +3117,98 @@ tsadc_pin: tsadc-pin {
<0 RK_PA1 0 _pull_none>;
};
};
+
+ lcdc {
+ /omit-if-no-ref/
+ lcdc_clock: lcdc-clock {
+ rockchip,pins =
+ /* lcdc_clk */
+ <3 RK_PA0 1 _pull_none>,
+ /* lcdc_den */
+ <3 RK_PC3 1 _pull_none>,
+ /* lcdc_hsync */
+ <3 RK_PC1 1 _pull_none>,
+ /* lcdc_vsync */
+ <3 RK_PC2 1 _pull_none>;
+ };
+
+ /omit-if-no-ref/
+ lcdc_data16: lcdc-data16 {
+ rockchip,pins =
+ /* lcdc_d3 */
+ <2 RK_PD3 1 _pull_none>,
+ /* lcdc_d4 */
+ <2 RK_PD4 1 _pull_none>,
+ /* lcdc_d5 */
+ <2 RK_PD5 1 _pull_none>,
+ /* lcdc_d6 */
+ <2 RK_PD6 1 _pull_none>,
+ /* lcdc_d7 */
+ <2 RK_PD7 1 _pull_none>,
+ /* lcdc_d10 */
+ <3 RK_PA3 1 _pull_none>,
+ /* lcdc_d11 */
+ <3 RK_PA4 1 _pull_none>,
+ /* lcdc_d12 */
+ <3 RK_PA5 1 _pull_none>,
+ /* lcdc_d13 */
+ <3 RK_PA6 1 _pull_none>,
+ /* lcdc_d14 */
+ <3 RK_PA7 1 _pull_none>,
+ /* lcdc_d15 */
+ <3 RK_PB0 1 _pull_none>,
+ /* lcdc_d19 */
+ <3 RK_PB4 1 _pull_none>,
+ /* lcdc_d20 */
+ <3 RK_PB5 1 _pull_none>,
+ /* lcdc_d21 */
+ <3 RK_PB6 1 _pull_none>,
+ /* lcdc_d22 */
+ <3 RK_PB7 1 _pull_none>,
+ /* lcdc_d23 */
+ <3 RK_PC0 1 _pull_none>;
+ };
+
+ /omit-if-no-ref/
+ lcdc_data18: lcdc-data18 {
+ rockchip,pins =
+ /* lcdc_d2 */
+ <2 RK_PD2 1 _pull_none>,
+ /* lcdc_d3 */
+ <2 RK_PD3 1 _pull_none>,
+ /* lcdc_d4 */
+ <2 RK_PD4 1 _pull_none>,
+ /* lcdc_d5 */
+ <2 RK_PD5 1 _pull_none>,
+ /* lcdc_d6 */
+ <2 RK_PD6 1 _pull_none>,
+ /* lcdc_d7 */
+ <2 RK_PD7 1 _pull_none>,
+ /* lcdc_d10 */
+ <3 RK_PA3 1 _pull_none>,
+ /* lcdc_d11 */
+ <3 RK_PA4 1 _pull_none>,
+ /* lcdc_d12 */
+ <3 RK_PA5 1 _pull_none>,
+ /* lcdc_d13 */
+ <3 RK_PA6 1 _pull_none>,
+ /* lcdc_d14 */
+ <3 RK_PA7 1 _pull_none>,
+ /* lcdc_d15 */
+ <3 RK_PB0 1 _pull_none>,
+ /* lcdc_d18 */
+ <3 RK_PB3 1 _pull_none>,
+ /* lcdc_d19 */
[PATCH 4/5] drm/rockchip: vop2: add support for the rgb output block
The Rockchip VOP2 features an internal RGB output block, which can be
attached to the video port 2 of the VOP2. Add support for this output
block.
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 21
1 file changed, 21 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 94fddbf70ff6..16041c79d228 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -39,6 +39,7 @@
#include "rockchip_drm_gem.h"
#include "rockchip_drm_fb.h"
#include "rockchip_drm_vop2.h"
+#include "rockchip_rgb.h"
/*
* VOP2 architecture
@@ -212,6 +213,9 @@ struct vop2 {
struct clk *hclk;
struct clk *aclk;
+ /* optional internal rgb encoder */
+ struct rockchip_rgb *rgb;
+
/* must be put at the end of the struct */
struct vop2_win win[];
};
@@ -2697,11 +2701,25 @@ static int vop2_bind(struct device *dev, struct device
*master, void *data)
if (ret)
return ret;
+ vop2->rgb = rockchip_rgb_init(dev, >vps[2].crtc, vop2->drm, 2);
+ if (IS_ERR(vop2->rgb)) {
+ if (PTR_ERR(vop2->rgb) == -EPROBE_DEFER) {
+ ret = PTR_ERR(vop2->rgb);
+ goto err_crtcs;
+ }
+ vop2->rgb = NULL;
+ }
+
rockchip_drm_dma_init_device(vop2->drm, vop2->dev);
pm_runtime_enable(>dev);
return 0;
+
+err_crtcs:
+ vop2_destroy_crtcs(vop2);
+
+ return ret;
}
static void vop2_unbind(struct device *dev, struct device *master, void *data)
@@ -2710,6 +2728,9 @@ static void vop2_unbind(struct device *dev, struct device
*master, void *data)
pm_runtime_disable(dev);
+ if (vop2->rgb)
+ rockchip_rgb_fini(vop2->rgb);
+
vop2_destroy_crtcs(vop2);
}
--
2.30.2
[PATCH 3/5] drm/rockchip: vop2: use symmetric function pair vop2_{create, destroy}_crtcs
Let the function name vop2_create_crtcs reflect that the function creates
multiple CRTCS. Also, use a symmetric function pair to create and destroy
the CRTCs and the corresponding planes.
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 31 ++--
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 105a548d0abe..94fddbf70ff6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2246,7 +2246,7 @@ static struct vop2_video_port
*find_vp_without_primary(struct vop2 *vop2)
#define NR_LAYERS 6
-static int vop2_create_crtc(struct vop2 *vop2)
+static int vop2_create_crtcs(struct vop2 *vop2)
{
const struct vop2_data *vop2_data = vop2->data;
struct drm_device *drm = vop2->drm;
@@ -2371,15 +2371,25 @@ static int vop2_create_crtc(struct vop2 *vop2)
return 0;
}
-static void vop2_destroy_crtc(struct drm_crtc *crtc)
+static void vop2_destroy_crtcs(struct vop2 *vop2)
{
- of_node_put(crtc->port);
+ struct drm_device *drm = vop2->drm;
+ struct list_head *crtc_list = >mode_config.crtc_list;
+ struct list_head *plane_list = >mode_config.plane_list;
+ struct drm_crtc *crtc, *tmpc;
+ struct drm_plane *plane, *tmpp;
+
+ list_for_each_entry_safe(plane, tmpp, plane_list, head)
+ drm_plane_cleanup(plane);
/*
* Destroy CRTC after vop2_plane_destroy() since vop2_disable_plane()
* references the CRTC.
*/
- drm_crtc_cleanup(crtc);
+ list_for_each_entry_safe(crtc, tmpc, crtc_list, head) {
+ of_node_put(crtc->port);
+ drm_crtc_cleanup(crtc);
+ }
}
static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
@@ -2683,7 +2693,7 @@ static int vop2_bind(struct device *dev, struct device
*master, void *data)
if (ret)
return ret;
- ret = vop2_create_crtc(vop2);
+ ret = vop2_create_crtcs(vop2);
if (ret)
return ret;
@@ -2697,19 +2707,10 @@ static int vop2_bind(struct device *dev, struct device
*master, void *data)
static void vop2_unbind(struct device *dev, struct device *master, void *data)
{
struct vop2 *vop2 = dev_get_drvdata(dev);
- struct drm_device *drm = vop2->drm;
- struct list_head *plane_list = >mode_config.plane_list;
- struct list_head *crtc_list = >mode_config.crtc_list;
- struct drm_crtc *crtc, *tmpc;
- struct drm_plane *plane, *tmpp;
pm_runtime_disable(dev);
- list_for_each_entry_safe(plane, tmpp, plane_list, head)
- drm_plane_cleanup(plane);
-
- list_for_each_entry_safe(crtc, tmpc, crtc_list, head)
- vop2_destroy_crtc(crtc);
+ vop2_destroy_crtcs(vop2);
}
const struct component_ops vop2_component_ops = {
--
2.30.2
[PATCH 2/5] drm/rockchip: rgb: add video_port parameter to init function
The VOP2 driver has more than one video port, hence the hard-coded
port id will not work anymore. Add an extra parameter for the video
port id to the rockchip_rgb_init function.
Signed-off-by: Michael Riesch
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_rgb.c | 9 +
drivers/gpu/drm/rockchip/rockchip_rgb.h | 6 --
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c356de5dd220..f7335f9cac73 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -2221,7 +2221,7 @@ static int vop_bind(struct device *dev, struct device
*master, void *data)
goto err_disable_pm_runtime;
if (vop->data->feature & VOP_FEATURE_INTERNAL_RGB) {
- vop->rgb = rockchip_rgb_init(dev, >crtc, vop->drm_dev);
+ vop->rgb = rockchip_rgb_init(dev, >crtc, vop->drm_dev, 0);
if (IS_ERR(vop->rgb)) {
ret = PTR_ERR(vop->rgb);
goto err_disable_pm_runtime;
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c
b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 16201a5cf1e8..ed6ccd1db465 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -74,7 +74,8 @@ struct drm_encoder_helper_funcs
rockchip_rgb_encoder_helper_funcs = {
struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
struct drm_crtc *crtc,
- struct drm_device *drm_dev)
+ struct drm_device *drm_dev,
+ int video_port)
{
struct rockchip_rgb *rgb;
struct drm_encoder *encoder;
@@ -92,7 +93,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
rgb->dev = dev;
rgb->drm_dev = drm_dev;
- port = of_graph_get_port_by_id(dev->of_node, 0);
+ port = of_graph_get_port_by_id(dev->of_node, video_port);
if (!port)
return ERR_PTR(-EINVAL);
@@ -105,8 +106,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
continue;
child_count++;
- ret = drm_of_find_panel_or_bridge(dev->of_node, 0, endpoint_id,
- , );
+ ret = drm_of_find_panel_or_bridge(dev->of_node, video_port,
+ endpoint_id, , );
if (!ret) {
of_node_put(endpoint);
break;
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.h
b/drivers/gpu/drm/rockchip/rockchip_rgb.h
index 27b9635124bc..1bd4e20e91eb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.h
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.h
@@ -8,12 +8,14 @@
#ifdef CONFIG_ROCKCHIP_RGB
struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
struct drm_crtc *crtc,
- struct drm_device *drm_dev);
+ struct drm_device *drm_dev,
+ int video_port);
void rockchip_rgb_fini(struct rockchip_rgb *rgb);
#else
static inline struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
struct drm_crtc *crtc,
-struct drm_device *drm_dev)
+struct drm_device *drm_dev,
+int video_port)
{
return NULL;
}
--
2.30.2
[PATCH 0/5] drm/rockchip: vop2: add support for the rgb output block
Hi all,
This series adds support for the RGB output block that can be found in the
Rockchip Video Output Processor (VOP) 2.
Patches 1-2 prepare the RGB part, which has been used only with the VOP(1)
so far.
Patch 3 is a cleanup patch and aims to make the creation and destruction of
the CRTCs and planes more readable.
Patch 4 activates the support for the RGB output block in the VOP2 driver.
Patch 5 adds pinctrls for the 16-bit and 18-bit RGB data lines.
Tested on a custom board featuring the RK3568 SoC with a 18-bit RGB
display.
Looking forward to your comments!
Best regards,
Michael
Michael Riesch (5):
drm/rockchip: rgb: embed drm_encoder into rockchip_encoder
drm/rockchip: rgb: add video_port parameter to init function
drm/rockchip: vop2: use symmetric function pair
vop2_{create,destroy}_crtcs
drm/rockchip: vop2: add support for the rgb output block
arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to
rk356x
.../boot/dts/rockchip/rk3568-pinctrl.dtsi | 94 +++
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 50 +++---
drivers/gpu/drm/rockchip/rockchip_rgb.c | 21 +++--
drivers/gpu/drm/rockchip/rockchip_rgb.h | 6 +-
5 files changed, 147 insertions(+), 26 deletions(-)
base-commit: b7b275e60bcd5f89771e865a8239325f86d9927d
--
2.30.2
Re: [PATCH v2 0/2] drm/rockchip: dw_hdmi: Add 4k@30 support
Hi Sascha, On 9/26/22 10:04, Sascha Hauer wrote: > This series adds support for 4k@30 to the rockchip HDMI controller. This > has been tested on a rk3568 rock3a board. It should be possible to add > 4k@60 support the same way, but it doesn't work for me, so let's add > 4k@30 as a first step. > > Sascha > > Changes since v1: > - Allow non standard clock rates only on Synopsys phy as suggested by > Robin Murphy > > Sascha Hauer (2): > drm/rockchip: dw_hdmi: relax mode_valid hook > drm/rockchip: dw_hdmi: Add support for 4k@30 resolution > > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 34 - > 1 file changed, 27 insertions(+), 7 deletions(-) Thanks for the v2! On a RK3568 EVB1 with a HP 27f 4k monitor Tested-by: Michael Riesch Best regards, Michael
Re: [PATCH] drm/rockchip: vop2: Fix Null Pointer Dereference on Multiple VPs
Hi,
On 9/12/22 20:02, Chris Morgan wrote:
> From: Chris Morgan
Cc: Sascha -> any thoughts on this one?
Best regards,
Michael
> If I use more than one VP to output on an RK3566 based device I
> receive the following error (and then everything freezes):
>
> [0.838375] Unable to handle kernel NULL pointer dereference at virtual
> address 0250
> [0.839191] Mem abort info:
> [0.839442] ESR = 0x9605
> [0.839785] EC = 0x25: DABT (current EL), IL = 32 bits
> [0.840256] SET = 0, FnV = 0
> [0.840530] EA = 0, S1PTW = 0
> [0.840821] FSC = 0x05: level 1 translation fault
> [0.841254] Data abort info:
> [0.841512] ISV = 0, ISS = 0x0005
> [0.841864] CM = 0, WnR = 0
> [0.842130] [0250] user address but active_mm is swapper
> [0.842704] Internal error: Oops: 9605 [#1] SMP
> [0.843139] Modules linked in:
> [0.843420] CPU: 0 PID: 37 Comm: kworker/u8:1 Not tainted 6.0.0-rc5+ #40
> [0.844013] Hardware name: RG503 (DT)
> [0.844343] Workqueue: events_unbound deferred_probe_work_func
> [0.844871] pstate: 8009 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [0.845487] pc : __drm_crtc_init_with_planes+0x48/0x2e4
> [0.845956] lr : drm_crtc_init_with_planes+0x68/0x94
> [0.846399] sp : ffc00a7a3710
> [0.846695] x29: ffc00a7a3710 x28: ff8000fb4080 x27:
> ffc00a7a37a0
> [0.847332] x26: ffc0097d01c7 x25: ff8000fb44d8 x24:
> ffc0097d01c7
> [0.847967] x23: ffc009311870 x22: x21:
> 0008
> [0.848603] x20: ff80010d0800 x19: ff8000fb44e8 x18:
>
> [0.849237] x17: 08d1 x16: 0891 x15:
> 08c1
> [0.849874] x14: x13: 3432564e3631564e x12:
> 3231564e36314742
> [0.850509] x11: 3631475234324742 x10: 002d x9 :
> ffc008682004
> [0.851144] x8 : 006f7475 x7 : fff0 x6 :
> ffc00a7a37a0
> [0.851778] x5 : ffc0097d01c7 x4 : ffc009311870 x3 :
>
> [0.852412] x2 : 0008 x1 : ff8000fb44e8 x0 :
> ff80010d0800
> [0.853048] Call trace:
> [0.853270] __drm_crtc_init_with_planes+0x48/0x2e4
> [0.853706] drm_crtc_init_with_planes+0x68/0x94
> [0.854118] vop2_bind+0x89c/0x920
> [0.854429] component_bind_all+0x18c/0x290
> [0.854805] rockchip_drm_bind+0xe4/0x1f0
> [0.855166] try_to_bring_up_aggregate_device+0x9c/0x290
> [0.855639] __component_add+0x110/0x168
> [0.855991] component_add+0x1c/0x28
> [0.856312] dw_mipi_dsi_rockchip_host_attach+0x98/0x10c
> [0.856785] dw_mipi_dsi_host_attach+0xbc/0xd0
> [0.857184] mipi_dsi_attach+0x30/0x44
> [0.857521] devm_mipi_dsi_attach+0x2c/0x70
> [0.857896] ams495qa01_probe+0x2d4/0x33c
> [0.858257] spi_probe+0x8c/0xb8
> [0.858550] really_probe+0x1e0/0x3b8
> [0.858881] __driver_probe_device+0x16c/0x184
> [0.859278] driver_probe_device+0x4c/0xfc
> [0.859646] __device_attach_driver+0xf0/0x170
> [0.860043] bus_for_each_drv+0xa4/0xcc
> [0.860389] __device_attach+0xfc/0x1a8
> [0.860733] device_initial_probe+0x1c/0x28
> [0.861108] bus_probe_device+0x38/0x9c
> [0.861452] deferred_probe_work_func+0xdc/0xf0
> [0.861855] process_one_work+0x1b0/0x260
> [0.862217] process_scheduled_works+0x4c/0x50
> [0.862614] worker_thread+0x1f0/0x26c
> [0.862949] kthread+0xc4/0xd4
> [0.863227] ret_from_fork+0x10/0x20
> [0.863553] Code: aa0503fa f9002bfb aa0603fb b4a2 (b9424840)
> [0.864095] ---[ end trace ]---
>
> A cursory reading of the datasheet suggests it's possible to have
> simultaneous output to 2 distinct outputs. On page 13 it states:
>
> Support two simultaneous displays(same source) in the following interfaces:
> - MIPI_DSI_TX
> - LVDS
> - HDMI
> - eDP
>
> In order to achieve this though, I need to output to VP0 and VP1 (or
> any 2 distinct VPs really). This is so I can have the ref clock set
> to 24MHz for the HDMI and the pixel clock of the DSI panel (for the
> example above it's 33.5MHz). Currently, only by removing this code
> block is such a thing possible, though I'm not sure if long-term
> there should only be 1 CRTC for the rk3566 (and 2 CRTCs for 3568)
> along with a max of 2 encoders for rk3566 (and 3 encoders for 3568).
>
> Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
>
> Signed-off-by: Chris Morgan
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 14 --
> 1 file changed, 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index e4631f515ba4..f18d7f6f9f86 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -2289,20 +2289,6 @@ static int vop2_create_crtc(struct vop2 *vop2)
>
Re: [PATCH 0/2] drm/rockchip: dw_hdmi: Add 4k@30 support
Hi Sascha, Can you Cc: linux-rockchip list to get more feedback? On 8/22/22 17:20, Sascha Hauer wrote: > This series adds support for 4k@30 to the rockchip HDMI controller. This > has been tested on a rk3568 rock3a board. It should be possible to add > 4k@60 support the same way, but it doesn't work for me, so let's add > 4k@30 as a first step. > > Sascha > > Sascha Hauer (2): > drm/rockchip: dw_hdmi: relax mode_valid hook > drm/rockchip: dw_hdmi: Add support for 4k@30 resolution Great stuff, thanks! On a Radxa ROCK3 Model A with a HP 27f 4k monitor Tested-by: Michael Riesch Best regards, Michael > > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 10 +- > 1 file changed, 9 insertions(+), 1 deletion(-) >
Re: [PATCH] drm/rockchip: vop2: Fix eDP/HDMI sync polarities
Hi Sascha,
On 8/15/22 15:39, Sascha Hauer wrote:
> The hsync/vsync polarities were not honoured for the eDP and HDMI ports.
> Add the register settings to configure the polarities as requested by the
> DRM_MODE_FLAG_PHSYNC/DRM_MODE_FLAG_PVSYNC flags.
Amazingly enough it worked even without this fix in my setup (Radxa
ROCK3 Model A + HP 27f 4k monitor). Hence I can only say that this patch
does not break anything in my setup :-)
Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver") ?
> Signed-off-by: Sascha Hauer
Tested-by: Michael Riesch
Thanks and best regards,
Michael
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index e4631f515ba42..f9aa8b96c6952 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1439,11 +1439,15 @@ static void rk3568_set_intf_mux(struct
> vop2_video_port *vp, int id,
> die &= ~RK3568_SYS_DSP_INFACE_EN_HDMI_MUX;
> die |= RK3568_SYS_DSP_INFACE_EN_HDMI |
> FIELD_PREP(RK3568_SYS_DSP_INFACE_EN_HDMI_MUX,
> vp->id);
> + dip &= ~RK3568_DSP_IF_POL__HDMI_PIN_POL;
> + dip |= FIELD_PREP(RK3568_DSP_IF_POL__HDMI_PIN_POL, polflags);
> break;
> case ROCKCHIP_VOP2_EP_EDP0:
> die &= ~RK3568_SYS_DSP_INFACE_EN_EDP_MUX;
> die |= RK3568_SYS_DSP_INFACE_EN_EDP |
> FIELD_PREP(RK3568_SYS_DSP_INFACE_EN_EDP_MUX, vp->id);
> + dip &= ~RK3568_DSP_IF_POL__EDP_PIN_POL;
> + dip |= FIELD_PREP(RK3568_DSP_IF_POL__EDP_PIN_POL, polflags);
> break;
> case ROCKCHIP_VOP2_EP_MIPI0:
> die &= ~RK3568_SYS_DSP_INFACE_EN_MIPI0_MUX;
Re: [PATCH v11 00/24] drm/rockchip: RK356x VOP2 support
Hello Sascha,
On 4/22/22 09:28, Sascha Hauer wrote:
> It's v11 time. There's only one small change to v10. Discussion seems to
> have settled now. Is there anything left that prevents the series from
> being merged? I'd really like to have it in during the next merge
> window.
Oh yes that'd be awesome :-)
On a RK3568 EVB1 and a Radxa Rock 3 Model A connected to a HP 27f 4K
monitor, using
$ modetest -M rockchip -s 69:{1920x1080,3840x2160}-{30,60}
as well as using weston and glmark2-es2-wayland:
Tested-by: Michael Riesch
Thanks and best regards,
Michael
> This series still depends on:
> drm/rockchip: Refactor IOMMU initialisation
> (https://lists.freedesktop.org/archives/dri-devel/2022-April/349548.html)
> arm64: dts: rockchip: add basic dts for the radxa rock3 model a
>
> Sascha
>
> Changes since v10:
> - relax mode_valid hook rather than dropping it in hdmi driver
>
> Changes since v9:
> - rebase on v5.18-rc1
> - Do not register windows which don't have its own framebuffer on RK3566
> - fix mixed up register writes in vop2_setup_dly_for_windows()
> - move call to rockchip_drm_dma_attach_device() from vop2_bind() to
> vop2_enable()
> - Fix zpos handling
>
> Changes since v8:
> - make hclk_vo a critical clock instead of enabling it in the hdmi driver
> - Fix vop2_setup_layer_mixer(), reported by Andy Yan
> - Limit planes possible_crtcs to actually existing crtcs
> - simplify vop2_create_crtc() a bit
>
> Changes since v7:
> - rename hclk to niu
>
> Changes since v6:
> - Move of_graph parsing out of runtime code to initialization
>
> Changes since v5:
> - Add new patch to fix dw-hdmi of_graph binding
> - Drop "drm/encoder: Add of_graph port to struct drm_encoder" and solve
> issue internally in the driver
> - make checkpatch cleaner
>
> Changes since v4:
> - Reorder patches in a way that binding/dts/driver patches are closer together
> - Drop clk patches already applied by Heiko
>
> Changes since v3:
> - added changelog to each patch
> - Add 4k support to hdmi driver
> - rebase on v5.17-rc1
>
> Changes since v2:
> - Add pin names to HDMI supply pin description
> - Add hclk support to HDMI driver
> - Dual license rockchip-vop2 binding, update binding
> - Add HDMI connector to board dts files
> - drop unnecessary gamma_lut registers from vop2
> - Update dclk_vop[012] clock handling, no longer hacks needed
> - Complete regmap conversion
>
> Changes since v1:
> - drop all unnecessary waiting for frames within atomic modeset and plane
> update
> - Cluster subwin support removed
> - gamma support removed
> - unnecessary irq_lock removed
> - interrupt handling simplified
> - simplified zpos handling
> - drop is_alpha_support(), use fb->format->has_alpha instead
> - use devm_regulator_get() rather than devm_regulator_get_optional() for hdmi
> regulators
> - Use fixed number of planes per video port
> - Drop homegrown regmap code from vop2 driver (not complete yet)
> - Add separate include file for vop2 driver to not pollute the vop include
>
> Andy Yan (1):
> drm: rockchip: Add VOP2 driver
>
> Benjamin Gaignard (1):
> dt-bindings: display: rockchip: dw-hdmi: Add compatible for rk3568
> HDMI
>
> Douglas Anderson (2):
> drm/rockchip: dw_hdmi: Use auto-generated tables
> drm/rockchip: dw_hdmi: Set cur_ctr to 0 always
>
> Michael Riesch (2):
> arm64: dts: rockchip: enable vop2 and hdmi tx on quartz64a
> arm64: dts: rockchip: enable vop2 and hdmi tx on rock-3a
>
> Nickey Yang (1):
> drm/rockchip: dw_hdmi: add default 594Mhz clk for 4K@60hz
>
> Sascha Hauer (17):
> clk: rk3568: Mark hclk_vo as critical
> drm/rockchip: Embed drm_encoder into rockchip_decoder
> drm/rockchip: Add crtc_endpoint_id to rockchip_encoder
> drm/rockchip: dw_hdmi: rename vpll clock to reference clock
> dt-bindings: display: rockchip: dw-hdmi: use "ref" as clock name
> arm64: dts: rockchip: rk3399: rename HDMI ref clock to 'ref'
> drm/rockchip: dw_hdmi: add rk3568 support
> drm/rockchip: dw_hdmi: add regulator support
> dt-bindings: display: rockchip: dw-hdmi: Add regulator support
> drm/rockchip: dw_hdmi: relax mode_valid hook
> dt-bindings: display: rockchip: dw-hdmi: Make unwedge pinctrl optional
> arm64: dts: rockchip: rk356x: Add VOP2 nodes
> arm64: dts: rockchip: rk356x: Add HDMI nodes
> arm64: dts: rockchip: rk3568-evb: Enable VOP2 and hdmi
> drm/rockchip: Make VOP driver optional
> dt-bindings: display: rockchip: Add binding for VOP2
> dt-bindings: display: rockchip: dw-hdmi: fix ports description
>
> .../display/rockchip/rockchip,dw-hdmi.yaml| 46 +-
> .../display/rockchip/rockchip-vop2.yaml
Re: [PATCH v5 22/23] drm: rockchip: Add VOP2 driver
Hello Sascha,
On 2/9/22 10:53, Sascha Hauer wrote:
> From: Andy Yan
>
> The VOP2 unit is found on Rockchip SoCs beginning with rk3566/rk3568.
> It replaces the VOP unit found in the older Rockchip SoCs.
>
> This driver has been derived from the downstream Rockchip Kernel and
> heavily modified:
>
> - All nonstandard DRM properties have been removed
> - dropped struct vop2_plane_state and pass around less data between
> functions
> - Dropped all DRM_FORMAT_* not known on upstream
> - rework register access to get rid of excessively used macros
> - Drop all waiting for framesyncs
>
> The driver is tested with HDMI and MIPI-DSI display on a RK3568-EVB
> board. Overlay support is tested with the modetest utility. AFBC support
> on the cluster windows is tested with weston-simple-dmabuf-egl on
> weston using the (yet to be upstreamed) panfrost driver support.
>
> Signed-off-by: Sascha Hauer
On a RK3568 EVB1 connected to a HP 27f 4K monitor, using
$ modetest -M rockchip -s 69:{1920x1080,3840x2160}-{30,60}
Tested-by: Michael Riesch
Thanks and best regards,
Michael
> ---
>
> Notes:
> Changes since v4:
> - Avoid stack frame overflow by not allocating big array on the stack
>
> Changes since v3:
> - Sort includes
> - fix typos
> - Drop spinlock
> - Use regmap_set_bits()/regmap_clear_bits()
> - simplify vop2_scale_factor()
> - simplify vop2_afbc_transform_offset()
>
> drivers/gpu/drm/rockchip/Kconfig |6 +
> drivers/gpu/drm/rockchip/Makefile|1 +
> drivers/gpu/drm/rockchip/rockchip_drm_drv.c |1 +
> drivers/gpu/drm/rockchip/rockchip_drm_drv.h |7 +-
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c |2 +
> drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 15 +
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2689 ++
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 480
> drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 285 ++
> 9 files changed, 3485 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
> create mode 100644 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
>
> diff --git a/drivers/gpu/drm/rockchip/Kconfig
> b/drivers/gpu/drm/rockchip/Kconfig
> index b9b156308460..4ff0043f0ee7 100644
> --- a/drivers/gpu/drm/rockchip/Kconfig
> +++ b/drivers/gpu/drm/rockchip/Kconfig
> @@ -28,6 +28,12 @@ config ROCKCHIP_VOP
> This selects support for the VOP driver. You should enable it
> on all older SoCs up to RK3399.
>
> +config ROCKCHIP_VOP2
> + bool "Rockchip VOP2 driver"
> + help
> + This selects support for the VOP2 driver. You should enable it
> + on all newer SoCs beginning form RK3568.
> +
> config ROCKCHIP_ANALOGIX_DP
> bool "Rockchip specific extensions for Analogix DP driver"
> depends on ROCKCHIP_VOP
> diff --git a/drivers/gpu/drm/rockchip/Makefile
> b/drivers/gpu/drm/rockchip/Makefile
> index dfc5512fdb9f..3ff7b21c0414 100644
> --- a/drivers/gpu/drm/rockchip/Makefile
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -6,6 +6,7 @@
> rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> rockchip_drm_gem.o
>
> +rockchipdrm-$(CONFIG_ROCKCHIP_VOP2) += rockchip_drm_vop2.o
> rockchip_vop2_reg.o
> rockchipdrm-$(CONFIG_ROCKCHIP_VOP) += rockchip_drm_vop.o rockchip_vop_reg.o
> rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 82c8faf1fb6b..95f6c5985fdd 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -459,6 +459,7 @@ static int __init rockchip_drm_init(void)
>
> num_rockchip_sub_drivers = 0;
> ADD_ROCKCHIP_SUB_DRIVER(vop_platform_driver, CONFIG_ROCKCHIP_VOP);
> + ADD_ROCKCHIP_SUB_DRIVER(vop2_platform_driver, CONFIG_ROCKCHIP_VOP2);
> ADD_ROCKCHIP_SUB_DRIVER(rockchip_lvds_driver,
> CONFIG_ROCKCHIP_LVDS);
> ADD_ROCKCHIP_SUB_DRIVER(rockchip_dp_driver,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index 143a48330f84..6e1f97e1e4a6 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -18,7 +18,7 @@
>
> #define ROCKCHIP_MAX_FB_BUFFER 3
> #define ROCKCHIP_MAX_CONNECTOR 2
> -#define ROCKCHIP_MAX_CRTC2
> +#define ROCKCHIP_MAX_CRT
[PATCH v6 3/5] arm64: dts: rockchip: add cooling map and trip points for gpu to rk356x
From: Alex Bee
RK356x SoCs have a second thermal sensor for the GPU. This adds the
cooling map and trip points for it to make use of its contribution as
a cooling device.
Signed-off-by: Alex Bee
Signed-off-by: Michael Riesch
---
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 27
1 file changed, 27 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 50bbea862a6a..37194d735028 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -1093,6 +1093,33 @@ gpu_thermal: gpu-thermal {
polling-delay = <1000>; /* milliseconds */
thermal-sensors = < 1>;
+
+ trips {
+ gpu_threshold: gpu-threshold {
+ temperature = <7>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+ gpu_target: gpu-target {
+ temperature = <75000>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+ gpu_crit: gpu-crit {
+ temperature = <95000>;
+ hysteresis = <2000>;
+ type = "critical";
+ };
+ };
+
+ cooling-maps {
+ map0 {
+ trip = <_target>;
+ cooling-device =
+ < THERMAL_NO_LIMIT
THERMAL_NO_LIMIT>;
+ };
+ };
+
};
};
--
2.30.2
[PATCH v6 5/5] arm64: dts: rockchip: enable the gpu on rk3568-evb1-v10
Enable the GPU core on the Rockchip RK3568 EVB1.
Signed-off-by: Michael Riesch
---
arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts | 11 +++
1 file changed, 11 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
index d8a4f7a9f562..39c495ff0157 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
@@ -140,6 +140,11 @@ _rgmii_clk
status = "okay";
};
+ {
+ mali-supply = <_gpu>;
+ status = "okay";
+};
+
{
status = "okay";
@@ -462,6 +467,12 @@ {
status = "okay";
};
+ {
+ rockchip,hw-tshut-mode = <1>;
+ rockchip,hw-tshut-polarity = <0>;
+ status = "okay";
+};
+
{
status = "okay";
};
--
2.30.2
[PATCH v6 4/5] arm64: dts: rockchip: enable the gpu on quartz64-a
From: Ezequiel Garcia
Enable the GPU core on the Pine64 Quartz64 Model A.
Signed-off-by: Ezequiel Garcia
Signed-off-by: Alex Bee
Signed-off-by: Michael Riesch
---
arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts | 5 +
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
index 3e65465ac7d5..b048db6cff3a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
@@ -221,6 +221,11 @@ _clkinout
status = "okay";
};
+ {
+ mali-supply = <_gpu>;
+ status = "okay";
+};
+
{
status = "okay";
--
2.30.2
[PATCH v6 0/5] Add GPU for RK356x SoCs
Hi all, This series aims to bring the GPU support for the RK356x mainline. In conjunction with the VOP2/HDMI TX patches v4 [0]) it has been tested successfully on a RK3568 EVB1 with weston and glmark2-es2-wayland. It should be noted that on the RK3568 EVB1 the supply of the GPU power domain needs to be set to "always-on" in the device tree. There is an ongoing discussion to provide a clean solution [1], in the meantime one has to apply a hack. Looking forward to your comments! Best regards, Michael v6: - use existing clock name "gpu" instead of "core" - fix missing space in indentation v5: - address Rob's comments, describe clocks in SoC specific region - move gpu_opp_table so that nodes without a reg are sorted alphabetically - add GPU support to the RK3568 EVB1 v4: see https://lore.kernel.org/linux-rockchip/20211126151729.1026566-1-knaerz...@gmail.com/ v3: see https://lore.kernel.org/linux-rockchip/20210805025948.10900-1-ezequ...@collabora.com/ v2: see https://lore.kernel.org/linux-rockchip/20210730164515.83044-1-ezequ...@collabora.com/ [0] https://lore.kernel.org/linux-rockchip/20220126145549.617165-1-s.ha...@pengutronix.de/ [1] https://lore.kernel.org/linux-rockchip/20211217130919.3035788-1-s.ha...@pengutronix.de/ Alex Bee (2): dt-bindings: gpu: mali-bifrost: describe clocks for the rk356x gpu arm64: dts: rockchip: add cooling map and trip points for gpu to rk356x Ezequiel Garcia (2): arm64: dts: rockchip: add gpu node to rk356x arm64: dts: rockchip: enable the gpu on quartz64-a Michael Riesch (1): arm64: dts: rockchip: enable the gpu on rk3568-evb1-v10 .../bindings/gpu/arm,mali-bifrost.yaml| 15 .../boot/dts/rockchip/rk3566-quartz64-a.dts | 5 ++ .../boot/dts/rockchip/rk3568-evb1-v10.dts | 11 +++ arch/arm64/boot/dts/rockchip/rk356x.dtsi | 76 +++ 4 files changed, 107 insertions(+) -- 2.30.2
[PATCH v6 2/5] arm64: dts: rockchip: add gpu node to rk356x
From: Ezequiel Garcia
Rockchip SoCs RK3566 and RK3568 have a Mali Gondul core
which is based on the Bifrost architecture. It has
one shader core and two execution engines.
Quoting the datasheet:
Mali-G52 1-Core-2EE
* Support 1600Mpix/s fill rate when 800MHz clock frequency
* Support 38.4GLOPs when 800MHz clock frequency
Signed-off-by: Ezequiel Garcia
Signed-off-by: Alex Bee
Signed-off-by: Michael Riesch
---
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 49
1 file changed, 49 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index ff1689283996..50bbea862a6a 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -144,6 +144,40 @@ scmi_clk: protocol@14 {
};
};
+ gpu_opp_table: opp-table-1 {
+ compatible = "operating-points-v2";
+
+ opp-2 {
+ opp-hz = /bits/ 64 <2>;
+ opp-microvolt = <825000>;
+ };
+
+ opp-3 {
+ opp-hz = /bits/ 64 <3>;
+ opp-microvolt = <825000>;
+ };
+
+ opp-4 {
+ opp-hz = /bits/ 64 <4>;
+ opp-microvolt = <825000>;
+ };
+
+ opp-6 {
+ opp-hz = /bits/ 64 <6>;
+ opp-microvolt = <825000>;
+ };
+
+ opp-7 {
+ opp-hz = /bits/ 64 <7>;
+ opp-microvolt = <90>;
+ };
+
+ opp-8 {
+ opp-hz = /bits/ 64 <8>;
+ opp-microvolt = <100>;
+ };
+ };
+
pmu {
compatible = "arm,cortex-a55-pmu";
interrupts = ,
@@ -444,6 +478,21 @@ power-domain@RK3568_PD_RKVENC {
};
};
+ gpu: gpu@fde6 {
+ compatible = "rockchip,rk3568-mali", "arm,mali-bifrost";
+ reg = <0x0 0xfde6 0x0 0x4000>;
+ interrupts = ,
+,
+;
+ interrupt-names = "job", "mmu", "gpu";
+ clocks = <_clk 1>, < CLK_GPU>;
+ clock-names = "gpu", "bus";
+ #cooling-cells = <2>;
+ operating-points-v2 = <_opp_table>;
+ power-domains = < RK3568_PD_GPU>;
+ status = "disabled";
+ };
+
sdmmc2: mmc@fe00 {
compatible = "rockchip,rk3568-dw-mshc",
"rockchip,rk3288-dw-mshc";
reg = <0x0 0xfe00 0x0 0x4000>;
--
2.30.2
[PATCH v6 1/5] dt-bindings: gpu: mali-bifrost: describe clocks for the rk356x gpu
From: Alex Bee The Bifrost GPU in Rockchip RK356x SoCs has a core and a bus clock. Reflect this in the SoC specific part of the binding. Signed-off-by: Alex Bee [move the changes to the SoC section] Signed-off-by: Michael Riesch --- .../devicetree/bindings/gpu/arm,mali-bifrost.yaml | 15 +++ 1 file changed, 15 insertions(+) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 63a08f3f321d..4d6bfae0653c 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -159,6 +159,21 @@ allOf: power-domains: maxItems: 1 sram-supply: false + - if: + properties: +compatible: + contains: +const: rockchip,rk3568-mali +then: + properties: +clocks: + minItems: 2 +clock-names: + items: +- const: gpu +- const: bus + required: +- clock-names examples: - | -- 2.30.2
Re: [PATCH v5 1/5] dt-bindings: gpu: mali-bifrost: describe clocks for the rk356x gpu
Hi Rob, On 2/9/22 16:35, Rob Herring wrote: > On Wed, 09 Feb 2022 09:51:06 +0100, Michael Riesch wrote: >> From: Alex Bee >> >> The Bifrost GPU in Rockchip RK356x SoCs has a core and a bus clock. >> Reflect this in the SoC specific part of the binding. >> >> Signed-off-by: Alex Bee >> [move the changes to the SoC section] >> Signed-off-by: Michael Riesch >> --- >> .../devicetree/bindings/gpu/arm,mali-bifrost.yaml | 15 +++ >> 1 file changed, 15 insertions(+) >> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > ./Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml:173:12: > [warning] wrong indentation: expected 12 but found 11 (indentation) D'oh! Sorry for the stupid mistake, I found that yamllint was indeed missing. Lines 173 and 174 need an extra space. In the case that a v6 is required I'll fix this. But of course I wouldn't say no if this could be fixed when the patch is applied :-) Best regards, Michael > > dtschema/dtc warnings/errors: > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/patch/1590238 > > This check can fail if there are any dependencies. The base for a patch > series is generally the most recent rc1. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit. >
[PATCH v5 4/5] arm64: dts: rockchip: enable the gpu on quartz64-a
From: Ezequiel Garcia
Enable the GPU core on the Pine64 Quartz64 Model A.
Signed-off-by: Ezequiel Garcia
Signed-off-by: Alex Bee
Signed-off-by: Michael Riesch
---
arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts | 5 +
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
index 3e65465ac7d5..b048db6cff3a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
@@ -221,6 +221,11 @@ _clkinout
status = "okay";
};
+ {
+ mali-supply = <_gpu>;
+ status = "okay";
+};
+
{
status = "okay";
--
2.30.2
[PATCH v5 5/5] arm64: dts: rockchip: enable the gpu on rk3568-evb1-v10
Enable the GPU core on the Rockchip RK3568 EVB1.
Signed-off-by: Michael Riesch
---
arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts | 11 +++
1 file changed, 11 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
index d8a4f7a9f562..39c495ff0157 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
@@ -140,6 +140,11 @@ _rgmii_clk
status = "okay";
};
+ {
+ mali-supply = <_gpu>;
+ status = "okay";
+};
+
{
status = "okay";
@@ -462,6 +467,12 @@ {
status = "okay";
};
+ {
+ rockchip,hw-tshut-mode = <1>;
+ rockchip,hw-tshut-polarity = <0>;
+ status = "okay";
+};
+
{
status = "okay";
};
--
2.30.2
[PATCH v5 2/5] arm64: dts: rockchip: add gpu node to rk356x
From: Ezequiel Garcia
Rockchip SoCs RK3566 and RK3568 have a Mali Gondul core
which is based on the Bifrost architecture. It has
one shader core and two execution engines.
Quoting the datasheet:
Mali-G52 1-Core-2EE
* Support 1600Mpix/s fill rate when 800MHz clock frequency
* Support 38.4GLOPs when 800MHz clock frequency
Signed-off-by: Ezequiel Garcia
Signed-off-by: Alex Bee
Signed-off-by: Michael Riesch
---
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 49
1 file changed, 49 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index ff1689283996..47484305b7a4 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -144,6 +144,40 @@ scmi_clk: protocol@14 {
};
};
+ gpu_opp_table: opp-table-1 {
+ compatible = "operating-points-v2";
+
+ opp-2 {
+ opp-hz = /bits/ 64 <2>;
+ opp-microvolt = <825000>;
+ };
+
+ opp-3 {
+ opp-hz = /bits/ 64 <3>;
+ opp-microvolt = <825000>;
+ };
+
+ opp-4 {
+ opp-hz = /bits/ 64 <4>;
+ opp-microvolt = <825000>;
+ };
+
+ opp-6 {
+ opp-hz = /bits/ 64 <6>;
+ opp-microvolt = <825000>;
+ };
+
+ opp-7 {
+ opp-hz = /bits/ 64 <7>;
+ opp-microvolt = <90>;
+ };
+
+ opp-8 {
+ opp-hz = /bits/ 64 <8>;
+ opp-microvolt = <100>;
+ };
+ };
+
pmu {
compatible = "arm,cortex-a55-pmu";
interrupts = ,
@@ -444,6 +478,21 @@ power-domain@RK3568_PD_RKVENC {
};
};
+ gpu: gpu@fde6 {
+ compatible = "rockchip,rk3568-mali", "arm,mali-bifrost";
+ reg = <0x0 0xfde6 0x0 0x4000>;
+ interrupts = ,
+,
+;
+ interrupt-names = "job", "mmu", "gpu";
+ clocks = <_clk 1>, < CLK_GPU>;
+ clock-names = "core", "bus";
+ #cooling-cells = <2>;
+ operating-points-v2 = <_opp_table>;
+ power-domains = < RK3568_PD_GPU>;
+ status = "disabled";
+ };
+
sdmmc2: mmc@fe00 {
compatible = "rockchip,rk3568-dw-mshc",
"rockchip,rk3288-dw-mshc";
reg = <0x0 0xfe00 0x0 0x4000>;
--
2.30.2
[PATCH v5 1/5] dt-bindings: gpu: mali-bifrost: describe clocks for the rk356x gpu
From: Alex Bee The Bifrost GPU in Rockchip RK356x SoCs has a core and a bus clock. Reflect this in the SoC specific part of the binding. Signed-off-by: Alex Bee [move the changes to the SoC section] Signed-off-by: Michael Riesch --- .../devicetree/bindings/gpu/arm,mali-bifrost.yaml | 15 +++ 1 file changed, 15 insertions(+) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 63a08f3f321d..21409c8d3813 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -159,6 +159,21 @@ allOf: power-domains: maxItems: 1 sram-supply: false + - if: + properties: +compatible: + contains: +const: rockchip,rk3568-mali +then: + properties: +clocks: + minItems: 2 +clock-names: + items: + - const: core + - const: bus + required: +- clock-names examples: - | -- 2.30.2
[PATCH v5 3/5] arm64: dts: rockchip: add cooling map and trip points for gpu to rk356x
From: Alex Bee
RK356x SoCs have a second thermal sensor for the GPU. This adds the
cooling map and trip points for it to make use of its contribution as
a cooling device.
Signed-off-by: Alex Bee
Signed-off-by: Michael Riesch
---
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 27
1 file changed, 27 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 47484305b7a4..2334fed4620f 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -1093,6 +1093,33 @@ gpu_thermal: gpu-thermal {
polling-delay = <1000>; /* milliseconds */
thermal-sensors = < 1>;
+
+ trips {
+ gpu_threshold: gpu-threshold {
+ temperature = <7>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+ gpu_target: gpu-target {
+ temperature = <75000>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+ gpu_crit: gpu-crit {
+ temperature = <95000>;
+ hysteresis = <2000>;
+ type = "critical";
+ };
+ };
+
+ cooling-maps {
+ map0 {
+ trip = <_target>;
+ cooling-device =
+ < THERMAL_NO_LIMIT
THERMAL_NO_LIMIT>;
+ };
+ };
+
};
};
--
2.30.2
[PATCH v5 0/5] Add GPU for RK356x SoCs
Hi all, This series seems to be abandoned so I would like to pick it up in order to bring the GPU support for the RK356x mainline. The series (in conjunction with the VOP2/HDMI TX patches v4 [0]) has been tested successfully on a RK3568 EVB1 with weston and glmark2-es2-wayland. It should be noted that on the RK3568 EVB1 the supply of the GPU power domain needs to be set to "always-on" in the device tree. There is an ongoing discussion to provide a clean solution [1], in the meantime one has to apply a hack. Looking forward to your comments! Best regards, Michael v5: - address Rob's comments, describe clocks in SoC specific region - move gpu_opp_table so that nodes without a reg are sorted alphabetically - add GPU support to the RK3568 EVB1 v4: see https://lore.kernel.org/linux-rockchip/20211126151729.1026566-1-knaerz...@gmail.com/ v3: see https://lore.kernel.org/linux-rockchip/20210805025948.10900-1-ezequ...@collabora.com/ v2: see https://lore.kernel.org/linux-rockchip/20210730164515.83044-1-ezequ...@collabora.com/ [0] https://lore.kernel.org/linux-rockchip/20220126145549.617165-1-s.ha...@pengutronix.de/ [1] https://lore.kernel.org/linux-rockchip/20211217130919.3035788-1-s.ha...@pengutronix.de/ Alex Bee (2): dt-bindings: gpu: mali-bifrost: describe clocks for the rk356x gpu arm64: dts: rockchip: add cooling map and trip points for gpu to rk356x Ezequiel Garcia (2): arm64: dts: rockchip: add gpu node to rk356x arm64: dts: rockchip: enable the gpu on quartz64-a Michael Riesch (1): arm64: dts: rockchip: enable the gpu on rk3568-evb1-v10 .../bindings/gpu/arm,mali-bifrost.yaml| 15 .../boot/dts/rockchip/rk3566-quartz64-a.dts | 5 ++ .../boot/dts/rockchip/rk3568-evb1-v10.dts | 11 +++ arch/arm64/boot/dts/rockchip/rk356x.dtsi | 76 +++ 4 files changed, 107 insertions(+) -- 2.30.2
Re: [PATCH v4 00/27] drm/rockchip: RK356x VOP2 support
Hello Sascha,
On 1/26/22 15:55, Sascha Hauer wrote:
> This is v4 of adding RK356x VOP2 support. Due to popular demand I added
> a changelog to each patch, at least starting with changes to v3, I
> didn't care to add the older changes as well. I hopefully integrated all
> feedback I received to v3. Additionally I added some patches to the HDMI
> driver to support resolutions up to 4k@60Hz. The patches are mostly
> taken from the downstream kernel. Some have been send to public lists,
> but were never applied upstream for reasons I do not know. The patches
> are a bit more intrusive than needed for my case, but are present in the
> downstream kernel for a long time, so I decided just to take them
> instead of stripping them down to my needs. With these patches I
> successfully used the driver with 4k@30Hz. 4k@60Hz doesn't work for me,
> I hope this is due to my low quality cable.
The cable might be the issue indeed, at least in my tests 4k@60Hz worked
just fine. On a RK3568 EVB1, using
$ modetest -M rockchip -s 69:{1920x1080,3840x2160}-{30,60}
and a HP 27f 4K monitor:
Tested-by: Michael Riesch
Thanks for your work and best regards,
Michael
>
> Sascha
>
> Changes since v3:
> - added changelog to each patch
> - Add 4k support to hdmi driver
> - rebase on v5.17-rc1
>
> Changes since v2:
> - Add pin names to HDMI supply pin description
> - Add hclk support to HDMI driver
> - Dual license rockchip-vop2 binding, update binding
> - Add HDMI connector to board dts files
> - drop unnecessary gamma_lut registers from vop2
> - Update dclk_vop[012] clock handling, no longer hacks needed
> - Complete regmap conversion
>
> Changes since v1:
> - drop all unnecessary waiting for frames within atomic modeset and plane
> update
> - Cluster subwin support removed
> - gamma support removed
> - unnecessary irq_lock removed
> - interrupt handling simplified
> - simplified zpos handling
> - drop is_alpha_support(), use fb->format->has_alpha instead
> - use devm_regulator_get() rather than devm_regulator_get_optional() for hdmi
> regulators
> - Use fixed number of planes per video port
> - Drop homegrown regmap code from vop2 driver (not complete yet)
> - Add separate include file for vop2 driver to not pollute the vop include
>
> Andy Yan (1):
> drm: rockchip: Add VOP2 driver
>
> Benjamin Gaignard (1):
> dt-bindings: display: rockchip: dw-hdmi: Add compatible for rk3568
> HDMI
>
> Douglas Anderson (2):
> drm/rockchip: dw_hdmi: Use auto-generated tables
> drm/rockchip: dw_hdmi: Set cur_ctr to 0 always
>
> Michael Riesch (1):
> arm64: dts: rockchip: enable vop2 and hdmi tx on quartz64a
>
> Nickey Yang (1):
> drm/rockchip: dw_hdmi: add default 594Mhz clk for 4K@60hz
>
> Sascha Hauer (21):
> drm/encoder: Add of_graph port to struct drm_encoder
> drm/rockchip: dw_hdmi: Do not leave clock enabled in error case
> drm/rockchip: dw_hdmi: rename vpll clock to reference clock
> drm/rockchip: dw_hdmi: add rk3568 support
> drm/rockchip: dw_hdmi: add regulator support
> drm/rockchip: dw_hdmi: Add support for hclk
> drm/rockchip: dw_hdmi: drop mode_valid hook
> clk: rockchip: rk3568: Add more PLL rates
> dt-bindings: display: rockchip: dw-hdmi: Make unwedge pinctrl optional
> dt-bindings: display: rockchip: dw-hdmi: use "ref" as clock name
> dt-bindings: display: rockchip: dw-hdmi: Add regulator support
> dt-bindings: display: rockchip: dw-hdmi: Add additional clock
> dt-bindings: display: rockchip: Add binding for VOP2
> arm64: dts: rockchip: rk3399: reorder hmdi clocks
> arm64: dts: rockchip: rk3399: rename HDMI ref clock to 'ref'
> arm64: dts: rockchip: rk356x: Add VOP2 nodes
> arm64: dts: rockchip: rk356x: Add HDMI nodes
> arm64: dts: rockchip: rk3568-evb: Enable VOP2 and hdmi
> clk: rk3568: drop CLK_SET_RATE_PARENT from dclk_vop*
> clk: rk3568: Add CLK_SET_RATE_PARENT to the HDMI reference clock
> drm/rockchip: Make VOP driver optional
>
> .../display/rockchip/rockchip,dw-hdmi.yaml| 29 +-
> .../display/rockchip/rockchip-vop2.yaml | 146 +
> arch/arm64/boot/dts/rockchip/rk3399.dtsi |6 +-
> .../boot/dts/rockchip/rk3566-quartz64-a.dts | 48 +
> arch/arm64/boot/dts/rockchip/rk3566.dtsi |4 +
> .../boot/dts/rockchip/rk3568-evb1-v10.dts | 48 +
> arch/arm64/boot/dts/rockchip/rk3568.dtsi |4 +
> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 86 +
> drivers/clk/rockchip/clk-rk3568.c | 14 +-
> drivers/gpu/drm/rockchip/Kconfig | 14 +
> drivers/gpu/drm/rockchip/Makefile |4 +-
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 293 +-
> drivers/gpu/drm/rockchip/
Re: [PATCH v1 00/12] drm/rockchip: RK356x VOP2 support
Hello Kever, On 11/18/21 11:50 AM, Kever Yang wrote: On 2021/11/18 下午5:53, Daniel Stone wrote: Hi, On Thu, 18 Nov 2021 at 09:26, Heiko Stübner wrote: Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: I don't agree with this, we do believe you have do some clean up to meet the requirement of upstream, but all the framework and feature implement are from Rockchip engineer, we have made a great effort to make everything work which block us to upstream this driver for now. I don't fully understand what you mean here (language barrier probably), but dropping non-essential functionality in a first round is pretty common to at least get basic functionality working for everyone. With the special features getting added again in later patches over time. This happenened on the old vop as well. And of course, having a kernel that can "just" do normal graphics without the additional features is still preferable over having _NO_ graphics support at all ;-) NAK for this series. As you might've seen from previous graphics related patches, there is a big number of people _and companies_ that seems to want/need to work with the rk3566/rk3568 with a kernel based on mainline. --> Most likely even in real products! Yes, we've been trying to ship a real product based on RK356x. We started by using the vendor VOP2 driver, but it is broken beyond belief. The driver needs a fundamental ground-up rework, and all the additional features get in the way of doing this core rework to make it actually function correctly. So, NAK to the NAK. I would like to see the VOP2 support start simple, with more features being added one by one. While Rockchip did say that they want to upstream VOP2 support, there has been _NO_ movement or even information at all on this over at least the last year(!), so it's pretty understandable that developers will do this themself at some point, because they don't want to wait anymore for something that might never happen. So a simple "NAK" without additional information is not really helpful here. If you don't like Sascha's series, I really want to know _WHEN_ Rockchip plans on upstreaming at least basic graphis support themself. The kernel is often called a do-ocracy - the one who does the work, gets to decide. So if you really don't like Sascha's series at all, I do expect Rockchip to step up and provide a solution themself - and in a usable timeframe. Exactly what Heiko said. If you would like to upstream the driver then that would be fantastic to see, but I'm afraid you do not get to prevent someone else from doing the work themselves. First of all, we never stop any one to doing there work on upstream if the source code is write totally by themselves. Second, there are also many modules are upstream by developers based on Rockchip source code, please note that all of them have basic respect to our work, they do communicate with us first. But this committer do not take any respect to our engineers and their hard working: - He didn't contact with us; I approached Andy Yan and you off-list on October 20, 2021 in this regard, as Andy mentioned on linux-rockchip in July 2021 some plans to bring the driver mainline. Since there was no response, we asked Sascha to make this happen. - There isn't any information about original author in the commit message; As I have known, if I use source code from another developer, I need to at least add a "Signed-off-by" with original author; As has been discussed before, this will be fixed in v2. Simple mistake, no harm intended. - This commit and mail does not even have a 'CC' to original author. I NAK because I think this is not part of the open source spirit, and this kind of behavior should not be encouraged. It is great to hear that you care about the open source spirit. IMHO communication is a big part thereof. If Rockchip would communicate better their plans to bring things mainline including a time schedule, it would be a lot easier for all of us. Best regards, Michael
[PATCH] arm64: dts: rockchip: enable vop2 and hdmi tx on quartz64a
Enable the RK356x Video Output Processor (VOP) 2 on the Pine64
Quartz64 Model A.
Signed-off-by: Michael Riesch
---
.../boot/dts/rockchip/rk3566-quartz64-a.dts | 24 +++
1 file changed, 24 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
index 4d4b2a301b1a..9fba790c6af4 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
@@ -205,6 +205,16 @@ _clkinout
status = "okay";
};
+ {
+ status = "okay";
+ avdd-0v9-supply = <_0v9>;
+ avdd-1v8-supply = <_1v8>;
+};
+
+_in_vp0 {
+ status = "okay";
+};
+
{
status = "okay";
@@ -546,3 +556,17 @@ bluetooth {
{
status = "okay";
};
+
+ {
+ status = "okay";
+ assigned-clocks = < DCLK_VOP0>, < DCLK_VOP1>;
+ assigned-clock-parents = < PLL_HPLL>, < PLL_VPLL>;
+};
+
+_mmu {
+ status = "okay";
+};
+
+_out_hdmi {
+ status = "okay";
+};
--
2.30.2
Re: [PATCH 10/12] arm64: dts: rockchip: rk3568-evb: Enable VOP2 and hdmi
Hi Sascha,
On 11/17/21 3:33 PM, Sascha Hauer wrote:
This enabled the VOP2 display controller along with hdmi and the
required port routes which is enough to get a picture out of the
hdmi port of the board.
Signed-off-by: Sascha Hauer
---
.../boot/dts/rockchip/rk3568-evb1-v10.dts | 24 +++
1 file changed, 24 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
index 184e2aa2416af..156e001492173 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
@@ -106,6 +106,12 @@ _rgmii_clk
status = "okay";
};
+ {
+ status = "okay";
+ avdd-0v9-supply = <_image>;
+ avdd-1v8-supply = <_image>;
+};
+
{
status = "okay";
@@ -390,3 +396,21 @@ {
{
status = "okay";
};
+
+ {
+ status = "okay";
+ assigned-clocks = < DCLK_VOP0>, < DCLK_VOP1>;
+ assigned-clock-parents = < PLL_HPLL>, < PLL_VPLL>;
+};
+
+_mmu {
+ status = "okay";
+};
+
+_in_vp0 {
+ status = "okay";
+};
Minor nitpick: This should probably be sorted alphabetically.
Best regards,
Michael
+
+_out_hdmi {
+ status = "okay";
+};
Re: [PATCH v2 1/2] dt-bindings: display: rockchip: Add compatible for rk3568 HDMI
Hello Heiko, On 7/13/21 10:49 AM, Heiko Stübner wrote: > Hi Michael, > > Am Dienstag, 13. Juli 2021, 10:44:00 CEST schrieb Michael Riesch: >> The HDMI TX block in the RK3568 requires two power supplies, which have >> to be enabled in some cases (at least on the RK3568 EVB1 the voltages >> VDDA0V9_IMAGE and VCCA1V8_IMAGE are disabled by default). It would be >> great if this was considered by the driver and the device tree binding. >> I am not sure, though, whether this is a RK3568 specific or >> rockchip_dw_hdmi specific thing. Maybe it can even enter the Synopsis DW >> HDMI driver. > > I do remember that this discussion happened many years back already. > And yes the supplies are needed for all but back then there was opposition > as these are supposedly phy-related supplies, not for the dw-hdmi itself. > [There are variants with an external phy, like on the rk3328] > > See discussion on [0] > > [0] > https://dri-devel.freedesktop.narkive.com/pen2zWo1/patch-v3-1-2-drm-bridge-dw-hdmi-support-optional-supply-regulators Thanks for the pointer. My summary of this discussion would be the following: - There was no consensus on how to handle the issue. The voltages still have to be enabled from the outside of the driver. - Open question: rockchip-specific or general solution? (one may detect a tendency towards a rockchip-specific solution) - Open question: separation of the phy from the dw_hdmi IP core? First of all, IMHO the driver should enable those voltages, otherwise we will have the same discussion again in 5-6 years :-) Then, the rockchip,dw-hdmi binding features a property "phys", presumably to handle external phys (e.g., for the RK3328). This fact and the referenced discussion suggest a rockchip-specific solution. In the Rockchip documentation (at least for RK3328, RK3399 and RK3568), there are two extra voltages denoted as "HDMI PHY analog power". It would be tempting to add the internal phy to the device tree and glue it to the dw-hdmi using the "phys" property. However, as pointed out in the referenced discussion, the configuration registers of the phy are somewhat interleaved with the dw-hdmi registers and a clear separation may be tricky. As a more pragmatic alternative, we could add optional supplies to the rockchip,dw-hdmi binding and evaluate the "phys" property. If the latter is not specified, the internal phy is used and the supplies must be enabled. Would such an approach be acceptable? Best regards, Michael >> On 7/7/21 2:03 PM, Benjamin Gaignard wrote: >>> Define a new compatible for rk3568 HDMI. >>> This version of HDMI hardware block needs two new clocks hclk_vio and hclk >>> to provide phy reference clocks. >>> >>> Signed-off-by: Benjamin Gaignard >>> --- >>> version 2: >>> - Add the clocks needed for the phy. >>> >>> .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 6 +- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml >>> b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml >>> index 75cd9c686e985..cb8643b3a8b84 100644 >>> --- >>> a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml >>> +++ >>> b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml >>> @@ -23,6 +23,7 @@ properties: >>>- rockchip,rk3288-dw-hdmi >>>- rockchip,rk3328-dw-hdmi >>>- rockchip,rk3399-dw-hdmi >>> + - rockchip,rk3568-dw-hdmi >>> >>>reg-io-width: >>> const: 4 >>> @@ -51,8 +52,11 @@ properties: >>>- vpll >>>- enum: >>>- grf >>> + - hclk_vio >>> + - vpll >>> + - enum: >>> + - hclk >>>- vpll >>> - - const: vpll >> >> The description and documentation of the clocks are somewhat misleading >> IMHO. This is not caused by your patches, of course. But maybe this is a >> chance to clean them up a bit. >> >> It seems that the CEC clock is an optional clock of the dw-hdmi driver. >> Shouldn't it be documented in the synopsys,dw-hdmi.yaml? >> >> Also, it would be nice if the clocks hclk_vio and hclk featured a >> description in the binding. >> >> BTW, I am not too familiar with the syntax here, but shouldn't items in >> clocks and items in clock-names be aligned (currently, there is a plain >> list vs. an enum structure)? >> >> Best regards, >> Michael >> >>> >>>ddc-i2c-bus: >>> $ref: /schemas/types.yaml#/definitions/phandle >>> >> > > > >
Re: [PATCH v2 1/2] dt-bindings: display: rockchip: Add compatible for rk3568 HDMI
Hello Benjamin, The HDMI TX block in the RK3568 requires two power supplies, which have to be enabled in some cases (at least on the RK3568 EVB1 the voltages VDDA0V9_IMAGE and VCCA1V8_IMAGE are disabled by default). It would be great if this was considered by the driver and the device tree binding. I am not sure, though, whether this is a RK3568 specific or rockchip_dw_hdmi specific thing. Maybe it can even enter the Synopsis DW HDMI driver. On 7/7/21 2:03 PM, Benjamin Gaignard wrote: > Define a new compatible for rk3568 HDMI. > This version of HDMI hardware block needs two new clocks hclk_vio and hclk > to provide phy reference clocks. > > Signed-off-by: Benjamin Gaignard > --- > version 2: > - Add the clocks needed for the phy. > > .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git > a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > index 75cd9c686e985..cb8643b3a8b84 100644 > --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > @@ -23,6 +23,7 @@ properties: >- rockchip,rk3288-dw-hdmi >- rockchip,rk3328-dw-hdmi >- rockchip,rk3399-dw-hdmi > + - rockchip,rk3568-dw-hdmi > >reg-io-width: > const: 4 > @@ -51,8 +52,11 @@ properties: >- vpll >- enum: >- grf > + - hclk_vio > + - vpll > + - enum: > + - hclk >- vpll > - - const: vpll The description and documentation of the clocks are somewhat misleading IMHO. This is not caused by your patches, of course. But maybe this is a chance to clean them up a bit. It seems that the CEC clock is an optional clock of the dw-hdmi driver. Shouldn't it be documented in the synopsys,dw-hdmi.yaml? Also, it would be nice if the clocks hclk_vio and hclk featured a description in the binding. BTW, I am not too familiar with the syntax here, but shouldn't items in clocks and items in clock-names be aligned (currently, there is a plain list vs. an enum structure)? Best regards, Michael > >ddc-i2c-bus: > $ref: /schemas/types.yaml#/definitions/phandle >
