drm/sun4i: Fix V3s YUV scanline size
Subsystems (5)
A message that is not in the archive (sent off-list or not yet received): [email protected]
Jernej Skrabec
maintainerPATCH 1/13 drm/sun4i: Fix V3s YUV scanline size
The VI scaler line buffer on V3s can hold 1024 pixels for subsampled formats, not 2048 as currently claimed. Since coarse horizontal scaling is engaged only once the source width exceeds that limit, YUV layers wider than 1024 pixels are passed to the scaler unchanged and the output is corrupted.
Use the value from the vendor driver.
--- drivers/gpu/drm/sun4i/sun8i_mixer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun8i_mixer.c +1 -1
@@ -785,7 +785,7 @@ static const struct sun8i_mixer_cfg sun8i_v3s_mixer_cfg = { .de_type = SUN8I_MIXER_DE2, .vi_scaler_num = 2, .scaler_mask = 0x3, - .scanline_yuv = 2048, + .scanline_yuv = 1024, }, .de_type = SUN8I_MIXER_DE2, .mod_rate = 150000000,
Signature · 1 line
2.43.0
Quoted 11 lines · Jernej Skrabec
The VI scaler line buffer on V3s can hold 1024 pixels for subsampled formats, not 2048 as currently claimed. Since coarse horizontal scaling is engaged only once the source width exceeds that limit, YUV layers wider than 1024 pixels are passed to the scaler unchanged and the output is corrupted.
Use the value from the vendor driver.
Fixes 2586de70c15c ("drm/sun4i: Add VI scaler line size quirk for DE2/DE3")Signed-off-by Jernej Skrabec <[email protected]>
Re: (subset) [PATCH 01/13] drm/sun4i: Fix V3s YUV scanline size
In reply to Jernej SkrabecQuoted 9 lines · Jernej Skrabec
The VI scaler line buffer on V3s can hold 1024 pixels for subsampled formats, not 2048 as currently claimed. Since coarse horizontal scaling is engaged only once the source width exceeds that limit, YUV layers wider than 1024 pixels are passed to the scaler unchanged and the output is corrupted.
Use the value from the vendor driver.
[...]
Applied to drm-misc-next in drm-misc, thanks!
[01/13] drm/sun4i: Fix V3s YUV scanline size
commit: abffce8b1eb5e119efccc51fc6a18ba38e2f6db6
[02/13] drm/sun4i: vi scaler: Fix coefficient selection
commit: 5c31990b21f0b535732deb2b658b78b07464f56c
[03/13] drm/sun4i: vi scaler: Restore opaque alpha in video modes
commit: 583d99165a85c3aaee924078c0f03c72e25644ac
[04/13] drm/sun4i: tcon-top: Keep mixer routes distinct
commit: 62bac3202042a8cfd4d4969a6d057cebd8b7cc1a
[05/13] drm/sun4i: tcon: Set output mux for DSI and LVDS
commit: 9c90199b39637ad94253c4fd8e7b1333ca1d3e0d
[06/13] drm/sun4i: tcon: Drop TCON TOP device reference
commit: 8208832a38ff3d2560eb8a77a9d7a2f17d8ebcdc
[07/13] drm/sun4i: hdmi: Don't leak sync polarity bits into packet control
commit: f5c3b1b0d228624786d22973a20c908c84e1a576
[08/13] drm/sun4i: crtc: Propagate layer initialization error
commit: 7061ff05ed4a3cf16e83f7e3ad09cbd212508a32
[09/13] drm/sun4i: tcon: Drop remote endpoint reference
commit: 3f77e4072630e2301efdbe521e7fca10311043ec
[10/13] drm/sun4i: dw-hdmi: Drop TCON TOP port reference
commit: d2a242e5688a17b79c89cd966cd31820c5096d80
[11/13] drm/sun4i: Drop node references while building component list
commit: 08a91f2a0664b73c2af9295ecbd441e852abe33c
[12/13] drm/sun4i: hdmi-phy: Fix H6 8-bit MPLL config at 594 MHz
commit: 0ba6deddaae74f0539c0303bfb5f860adbe1a68bBest regards,
Signature · 1 line
Chen-Yu Tsai <[email protected]>
Jernej Skrabec
maintainerPATCH 2/13 drm/sun4i: vi scaler: Fix coefficient selection
In reply to a message not in the archiveCurrently, vertical coefficients are selected based on horizontal scaling, which is wrong. Additionally, chroma coefficients should be selected based on format subsampling.
Fix all that.
--- drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/gpu/drm/sun4i/sun8i_vi_scaler.c +12 -6
@@ -894,20 +894,26 @@ static void sun8i_vi_scaler_set_coeff(struct regmap *map, u32 base, lan3coefftab32_left[offset + i]);regmap_write (map,SUN8I_SCALER_VSU_YHCOEFF1 (base, i), lan3coefftab32_right[offset + i]); + } + offset =sun8i_vi_scaler_coef_index (vstep) * +SUN8I_VI_SCALER_COEFF_COUNT ; +for (i =0 ; i <SUN8I_VI_SCALER_COEFF_COUNT ; i++) +regmap_write (map,SUN8I_SCALER_VSU_YVCOEFF (base, i), + lan2coefftab32[offset + i]); + + offset =sun8i_vi_scaler_coef_index (hstep / format->hsub) * +SUN8I_VI_SCALER_COEFF_COUNT ; +for (i =0 ; i <SUN8I_VI_SCALER_COEFF_COUNT ; i++) {regmap_write (map,SUN8I_SCALER_VSU_CHCOEFF0 (base, i), ch_left[offset + i]);regmap_write (map,SUN8I_SCALER_VSU_CHCOEFF1 (base, i), ch_right[offset + i]); } - - offset =sun8i_vi_scaler_coef_index (hstep) * + offset =sun8i_vi_scaler_coef_index (vstep / format->vsub) *SUN8I_VI_SCALER_COEFF_COUNT ; -for (i =0 ; i <SUN8I_VI_SCALER_COEFF_COUNT ; i++) { -regmap_write (map,SUN8I_SCALER_VSU_YVCOEFF (base, i), - lan2coefftab32[offset + i]); +for (i =0 ; i <SUN8I_VI_SCALER_COEFF_COUNT ; i++)regmap_write (map,SUN8I_SCALER_VSU_CVCOEFF (base, i), cy[offset + i]); - } }void sun8i_vi_scaler_enable (struct sun8i_layer *layer,bool enable)
Signature · 1 line
2.43.0
Re: [PATCH 02/13] drm/sun4i: vi scaler: Fix coefficient selection
Quoted 9 lines · Jernej Skrabec
Currently, vertical coefficients are selected based on horizontal scaling, which is wrong. Additionally, chroma coefficients should be selected based on format subsampling.
Fix all that.
Fixes b862a648de3b ("drm/sun4i: Add support for HW scaling to DE2")Signed-off-by Jernej Skrabec <[email protected]>
Somehow having the two fixes together made the diff much harder for me to understand.
Quoted 44 lines
--- drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)drivers/gpu/drm/sun4i/sun8i_vi_scaler.c +12
-7@@ -894,20 +894,26 @@ static void sun8i_vi_scaler_set_coeff(struct regmap *map, u32 base, lan3coefftab32_left[offset + i]);regmap_write (map,SUN8I_SCALER_VSU_YHCOEFF1 (base, i), lan3coefftab32_right[offset + i]); + } + offset =sun8i_vi_scaler_coef_index (vstep) * +SUN8I_VI_SCALER_COEFF_COUNT ; +for (i =0 ; i <SUN8I_VI_SCALER_COEFF_COUNT ; i++) +regmap_write (map,SUN8I_SCALER_VSU_YVCOEFF (base, i), + lan2coefftab32[offset + i]); + + offset =sun8i_vi_scaler_coef_index (hstep / format->hsub) * +SUN8I_VI_SCALER_COEFF_COUNT ; +for (i =0 ; i <SUN8I_VI_SCALER_COEFF_COUNT ; i++) {regmap_write (map,SUN8I_SCALER_VSU_CHCOEFF0 (base, i), ch_left[offset + i]);regmap_write (map,SUN8I_SCALER_VSU_CHCOEFF1 (base, i), ch_right[offset + i]); } - - offset =sun8i_vi_scaler_coef_index (hstep) * + offset =sun8i_vi_scaler_coef_index (vstep / format->vsub) *SUN8I_VI_SCALER_COEFF_COUNT ; -for (i =0 ; i <SUN8I_VI_SCALER_COEFF_COUNT ; i++) { -regmap_write (map,SUN8I_SCALER_VSU_YVCOEFF (base, i), - lan2coefftab32[offset + i]); +for (i =0 ; i <SUN8I_VI_SCALER_COEFF_COUNT ; i++)regmap_write (map,SUN8I_SCALER_VSU_CVCOEFF (base, i), cy[offset + i]); - } }void sun8i_vi_scaler_enable (struct sun8i_layer *layer,bool enable) --2.43.0
Jernej Skrabec
maintainerPATCH 3/13 drm/sun4i: vi scaler: Restore opaque alpha in video modes
In reply to a message not in the archiveThe DE3 mixer initialization clears the entire mixer register space. This also clears VSU_GLOBAL_ALPHA, despite its hardware reset value being 0xff.
The VI scaler uses Video Normal mode for subsampled YUV formats. In this mode, VSU_GLOBAL_ALPHA provides the scaler output alpha. Leaving the register at zero causes the scaler to produce fully transparent output.
Set VSU_GLOBAL_ALPHA to 0xff whenever configuring a DE3 or newer VI scaler. The register is ignored in UI scaling mode.
--- drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 2 ++ drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 1 + 2 files changed, 3 insertions(+)
drivers/gpu/drm/sun4i/sun8i_vi_scaler.c +2 -0
@@ -975,6 +975,8 @@ void sun8i_vi_scaler_setup(struct sun8i_layer *layer,regmap_write (layer->regs,SUN50I_SCALER_VSU_SCALE_MODE (base), val); +regmap_write (layer->regs, +SUN50I_SCALER_VSU_GLB_ALPHA (base),0xff ); } regmap_write(layer->regs,
drivers/gpu/drm/sun4i/sun8i_vi_scaler.h +1 -0
@@ -36,6 +36,7 @@#define SUN50I_SCALER_VSU_EDSCL_CTRL (base) ((base) + 0x28)#define SUN50I_SCALER_VSU_ANGLE_THR (base) ((base) + 0x2c)#define SUN8I_SCALER_VSU_OUTSIZE (base) ((base) + 0x40) +#define SUN50I_SCALER_VSU_GLB_ALPHA (base) ((base) + 0x44)#define SUN8I_SCALER_VSU_YINSIZE (base) ((base) + 0x80)#define SUN8I_SCALER_VSU_YHSTEP (base) ((base) + 0x88)#define SUN8I_SCALER_VSU_YVSTEP (base) ((base) + 0x8c)
Signature · 1 line
2.43.0
Re: [PATCH 03/13] drm/sun4i: vi scaler: Restore opaque alpha in video modes
Quoted 8 lines · Jernej Skrabec
The DE3 mixer initialization clears the entire mixer register space. This also clears
VSU_GLOBAL_ALPHA, despite its hardware reset value being0xff.The VI scaler uses Video Normal mode for subsampled YUV formats. In this mode,
VSU_GLOBAL_ALPHAprovides the scaler output alpha. Leaving the register at zero causes the scaler to produce fully transparent output.
Quoted 5 lines
Set
VSU_GLOBAL_ALPHAto0xffwhenever configuring a DE3 or newer VI scaler. The register is ignored in UI scaling mode.Fixes c50519e6db4d ("drm/sun4i: Add basic support for DE3")Signed-off-by Jernej Skrabec <[email protected]>
Quoted 33 lines
--- drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 2 ++ drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 1 + 2 files changed, 3 insertions(+)drivers/gpu/drm/sun4i/sun8i_vi_scaler.c +2
-0@@ -975,6 +975,8 @@ void sun8i_vi_scaler_setup(struct sun8i_layer *layer,regmap_write (layer->regs,SUN50I_SCALER_VSU_SCALE_MODE (base), val); +regmap_write (layer->regs, +SUN50I_SCALER_VSU_GLB_ALPHA (base),0xff ); } regmap_write(layer->regs,drivers/gpu/drm/sun4i/sun8i_vi_scaler.h +1
-1@@ -36,6 +36,7 @@#define SUN50I_SCALER_VSU_EDSCL_CTRL (base) ((base) + 0x28)#define SUN50I_SCALER_VSU_ANGLE_THR (base) ((base) + 0x2c)#define SUN8I_SCALER_VSU_OUTSIZE (base) ((base) + 0x40) +#define SUN50I_SCALER_VSU_GLB_ALPHA (base) ((base) + 0x44)#define SUN8I_SCALER_VSU_YINSIZE (base) ((base) + 0x80)#define SUN8I_SCALER_VSU_YHSTEP (base) ((base) + 0x88)#define SUN8I_SCALER_VSU_YVSTEP (base) ((base) + 0x8c) --2.43.0
Jernej Skrabec
maintainerPATCH 4/13 drm/sun4i: tcon-top: Keep mixer routes distinct
In reply to a message not in the archiveBoth mixer selectors reset to TCON 0. Selecting the same TCON for both mixers causes black or corrupted output.
When a route would collide, park the other mixer on another described TCON, or an unused selector if none exists.
Since the TCON index is now used as a shift, also reject negative values.
Tested on Orange Pi 3 with TCON_LCD0 and TCON_TV0.
--- drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 71 +++++++++++++++++++++----- drivers/gpu/drm/sun4i/sun8i_tcon_top.h | 6 +++ 2 files changed, 65 insertions(+), 12 deletions(-)
drivers/gpu/drm/sun4i/sun8i_tcon_top.c +59 -12
@@ -25,6 +25,49 @@ static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node)return !!of_match_node (sun8i_tcon_top_of_table, node); } +static unsigned int sun8i_tcon_top_get_tcon_map (struct device_node *node) +{ +static const u32 out_ports[] = { +TCON_TOP_MIXER0_OUT_PORT , +TCON_TOP_MIXER1_OUT_PORT , + }; +unsigned int i, map =0 ; + +for (i =0 ; i <ARRAY_SIZE (out_ports); i++) { +struct device_node *port; + + port =of_graph_get_port_by_id (node, out_ports[i]); +if (!port) +continue ; + +for_each_of_graph_port_endpoint (port, ep) { +struct of_endpoint endpoint; + +if (of_graph_parse_endpoint (ep, &endpoint)) +continue ; + +if (endpoint.id <TCON_TOP_PORT_TCON_NUM ) + map |=BIT (endpoint.id); + } + +of_node_put (port); + } + +return map; +} + +static unsigned int sun8i_tcon_top_park_index (struct sun8i_tcon_top *tcon_top, +int tcon) +{ +unsigned int candidates; + + candidates = tcon_top->tcon_map & ~BIT (tcon); +if (!candidates) + candidates =GENMASK (TCON_TOP_PORT_TCON_NUM -1 ,0 ) & ~BIT (tcon); + +return ffs (candidates) -1 ; +} +int sun8i_tcon_top_set_hdmi_src (struct device *dev,int tcon) {struct sun8i_tcon_top *tcon_top =dev_get_drvdata (dev);
@@ -57,6 +100,7 @@ EXPORT_SYMBOL(sun8i_tcon_top_set_hdmi_src);int sun8i_tcon_top_de_config (struct device *dev,int mixer,int tcon) {struct sun8i_tcon_top *tcon_top =dev_get_drvdata (dev); +u32 mixer_msk, other_msk;unsigned long flags;u32 reg;
@@ -70,21 +114,27 @@ int sun8i_tcon_top_de_config(struct device *dev, int mixer, int tcon)return -EINVAL ; } -if (tcon >3 ) { -dev_err (dev,"TCON index is too high!\n" ); +if (tcon <0 || tcon >=TCON_TOP_PORT_TCON_NUM ) { +dev_err (dev,"TCON index is invalid!\n" );return -EINVAL ; } + mixer_msk = mixer ?TCON_TOP_PORT_DE1_MSK :TCON_TOP_PORT_DE0_MSK ; + other_msk = mixer ?TCON_TOP_PORT_DE0_MSK :TCON_TOP_PORT_DE1_MSK ; +spin_lock_irqsave (&tcon_top->reg_lock, flags); reg =readl (tcon_top->regs +TCON_TOP_PORT_SEL_REG ); -if (mixer ==0 ) { - reg &= ~TCON_TOP_PORT_DE0_MSK ; - reg |=FIELD_PREP (TCON_TOP_PORT_DE0_MSK , tcon); - }else { - reg &= ~TCON_TOP_PORT_DE1_MSK ; - reg |=FIELD_PREP (TCON_TOP_PORT_DE1_MSK , tcon); + + reg &= ~mixer_msk; + reg |=field_prep (mixer_msk, tcon); + +if (field_get (other_msk, reg) == tcon) { + reg &= ~other_msk; + reg |=field_prep (other_msk, +sun8i_tcon_top_park_index (tcon_top, tcon)); } +writel (reg, tcon_top->regs +TCON_TOP_PORT_SEL_REG );spin_unlock_irqrestore (&tcon_top->reg_lock, flags);
@@ -143,6 +193,7 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,return -ENOMEM ; clk_data->num =CLK_NUM ; tcon_top->clk_data = clk_data; + tcon_top->tcon_map =sun8i_tcon_top_get_tcon_map (dev->of_node);spin_lock_init (&tcon_top->reg_lock);
@@ -175,10 +226,6 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, goto err_assert_reset; } -/* -* At least on H6, some registers have some bits set by default -* which may cause issues. Clear them here. -*/ writel (0 , regs +TCON_TOP_PORT_SEL_REG );writel (0 , regs +TCON_TOP_GATE_SRC_REG );
drivers/gpu/drm/sun4i/sun8i_tcon_top.h +6 -0
@@ -14,6 +14,10 @@#define TCON_TOP_PORT_SEL_REG 0x1C#define TCON_TOP_PORT_DE0_MSK GENMASK(1, 0)#define TCON_TOP_PORT_DE1_MSK GENMASK(5, 4) +#define TCON_TOP_PORT_TCON_NUM 4 + +#define TCON_TOP_MIXER0_OUT_PORT 1 +#define TCON_TOP_MIXER1_OUT_PORT 3#define TCON_TOP_GATE_SRC_REG 0x20#define TCON_TOP_HDMI_SRC_MSK GENMASK(29, 28)
@@ -29,6 +33,8 @@ struct sun8i_tcon_top {void __iomem *regs;struct reset_control *rst; +unsigned int tcon_map; +/* * spinlock is used to synchronize access to same * register where multiple clock gates can be set.
Signature · 1 line
2.43.0
Re: [PATCH 04/13] drm/sun4i: tcon-top: Keep mixer routes distinct
Quoted 10 lines · Jernej Skrabec
Both mixer selectors reset to TCON 0. Selecting the same TCON for both mixers causes black or corrupted output.
When a route would collide, park the other mixer on another described TCON, or an unused selector if none exists.
Since the TCON index is now used as a shift, also reject negative values.
Quoted 6 lines
Tested on Orange Pi 3 with
TCON_LCD0andTCON_TV0.Fixes 05db311a792d ("drm/sun4i: tcon-top: Add helpers for mux switching")Signed-off-by Jernej Skrabec <[email protected]>
Though I have a couple nits.
Quoted 12 lines
--- drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 71 +++++++++++++++++++++----- drivers/gpu/drm/sun4i/sun8i_tcon_top.h | 6 +++ 2 files changed, 65 insertions(+), 12 deletions(-)drivers/gpu/drm/sun4i/sun8i_tcon_top.c +0
-0@@ -25,6 +25,49 @@ static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node)return !!of_match_node (sun8i_tcon_top_of_table, node); }
drivers/gpu/drm/sun4i/sun8i_tcon_top.c static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) · line 36
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_tcon_top.c +9
-0+static unsigned int sun8i_tcon_top_get_tcon_map (struct device_node *node) +{ +static const u32 out_ports[] = { +TCON_TOP_MIXER0_OUT_PORT , +TCON_TOP_MIXER1_OUT_PORT , + }; +unsigned int i, map =0 ; + +for (i =0 ; i <ARRAY_SIZE (out_ports); i++) {
You can declare i directly in the for statement.
drivers/gpu/drm/sun4i/sun8i_tcon_top.c static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) · line 39
Quoted 3 lines
drivers/gpu/drm/sun4i/sun8i_tcon_top.c +3
-0+struct device_node *port; + + port =of_graph_get_port_by_id (node, out_ports[i]);
You could probably use the __free() cleanup macro and make port scoped and put the reference automatically.
Quoted 4 lines
drivers/gpu/drm/sun4i/sun8i_tcon_top.c +4
-0+if (!port) +continue ; + +for_each_of_graph_port_endpoint (port, ep) {
drivers/gpu/drm/sun4i/sun8i_tcon_top.c static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) · line 53
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_tcon_top.c +10
-0+struct of_endpoint endpoint; + +if (of_graph_parse_endpoint (ep, &endpoint)) +continue ; + +if (endpoint.id <TCON_TOP_PORT_TCON_NUM ) + map |=BIT (endpoint.id); + } + +of_node_put (port);
And this line wouldn't be needed.
Quoted 5 lines
drivers/gpu/drm/sun4i/sun8i_tcon_top.c +5
-0+ } + +return map; +} +
[...]
Jernej Skrabec
maintainerPATCH 5/13 drm/sun4i: tcon: Set output mux for DSI and LVDS
In reply to a message not in the archiveDSI and LVDS skip output mux setup, so TCON TOP cannot route the selected mixer. Configure them like other channel 0 outputs.
In practice this matters for D1, where channel 0 TCONs are fed through TCON TOP. The remaining set_mux implementations only handle TMDS and return an error for other encoder types, as before.
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/gpu/drm/sun4i/sun4i_tcon.c +2 -0
@@ -717,9 +717,11 @@ void sun4i_tcon_mode_set(struct sun4i_tcon *tcon,case DRM_MODE_ENCODER_DSI :/* DSI is tied to special case of CPU interface */ sun4i_tcon0_mode_set_cpu (tcon, encoder, mode); +sun4i_tcon_set_mux (tcon,0 , encoder);break ;case DRM_MODE_ENCODER_LVDS :sun4i_tcon0_mode_set_lvds (tcon, encoder, mode); +sun4i_tcon_set_mux (tcon,0 , encoder);break ;case DRM_MODE_ENCODER_NONE :sun4i_tcon0_mode_set_rgb (tcon, encoder, mode);
Signature · 1 line
2.43.0
Re: [PATCH 05/13] drm/sun4i: tcon: Set output mux for DSI and LVDS
Quoted 10 lines · Jernej Skrabec
DSI and LVDS skip output mux setup, so TCON TOP cannot route the selected mixer. Configure them like other channel 0 outputs.
In practice this matters for D1, where channel 0 TCONs are fed through TCON TOP. The remaining
set_muximplementations only handle TMDS and return an error for other encoder types, as before.Fixes b9b52d2f4aaf ("drm/sun4i: Add support for D1 TCONs")Signed-off-by Jernej Skrabec <[email protected]>
Quoted 24 lines
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 ++ 1 file changed, 2 insertions(+)drivers/gpu/drm/sun4i/sun4i_tcon.c +2
-1@@ -717,9 +717,11 @@ void sun4i_tcon_mode_set(struct sun4i_tcon *tcon,case DRM_MODE_ENCODER_DSI :/* DSI is tied to special case of CPU interface */ sun4i_tcon0_mode_set_cpu (tcon, encoder, mode); +sun4i_tcon_set_mux (tcon,0 , encoder);break ;case DRM_MODE_ENCODER_LVDS :sun4i_tcon0_mode_set_lvds (tcon, encoder, mode); +sun4i_tcon_set_mux (tcon,0 , encoder);break ;case DRM_MODE_ENCODER_NONE :sun4i_tcon0_mode_set_rgb (tcon, encoder, mode); --2.43.0
Jernej Skrabec
maintainerPATCH 6/13 drm/sun4i: tcon: Drop TCON TOP device reference
In reply to a message not in the archiveof_find_device_by_node() takes a device reference. Drop it after mux configuration succeeds.
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
drivers/gpu/drm/sun4i/sun4i_tcon.c +9 -10
@@ -1409,7 +1409,7 @@ static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon, {struct device_node *port, *remote;struct platform_device *pdev; -int id, ret; +int id, ret =0 ;/* find TCON TOP platform device and TCON id */
@@ -1432,21 +1432,20 @@ static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon,if (IS_ENABLED (CONFIG_DRM_SUN8I_TCON_TOP ) && encoder->encoder_type ==DRM_MODE_ENCODER_TMDS ) { ret =sun8i_tcon_top_set_hdmi_src (&pdev->dev, id); -if (ret) { -put_device (&pdev->dev); -return ret; - } +if (ret) + goto out_put_device; }if (IS_ENABLED (CONFIG_DRM_SUN8I_TCON_TOP )) { ret =sun8i_tcon_top_de_config (&pdev->dev, tcon->id, id); -if (ret) { -put_device (&pdev->dev); -return ret; - } +if (ret) + goto out_put_device; } -return 0 ; +out_put_device: +put_device (&pdev->dev); + +return ret; }static const struct sun4i_tcon_quirks sun4i_a10_quirks = {
Signature · 1 line
2.43.0
Re: [PATCH 06/13] drm/sun4i: tcon: Drop TCON TOP device reference
Quoted 6 lines · Jernej Skrabec
of_find_device_by_node()takes a device reference. Drop it after mux configuration succeeds.Fixes 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")Signed-off-by Jernej Skrabec <[email protected]>
Though I'd probably rewrite it with
struct platform_device *pdev __free(platform_device_put) =
of_find_device_by_node();
and then all paths can just return instead of needing the goto.
Quoted 51 lines
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)drivers/gpu/drm/sun4i/sun4i_tcon.c +9
-11@@ -1409,7 +1409,7 @@ static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon, {struct device_node *port, *remote;struct platform_device *pdev; -int id, ret; +int id, ret =0 ;/* find TCON TOP platform device and TCON id */ @@ -1432,21 +1432,20 @@ static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon,if (IS_ENABLED (CONFIG_DRM_SUN8I_TCON_TOP ) && encoder->encoder_type ==DRM_MODE_ENCODER_TMDS ) { ret =sun8i_tcon_top_set_hdmi_src (&pdev->dev, id); -if (ret) { -put_device (&pdev->dev); -return ret; - } +if (ret) + goto out_put_device; }if (IS_ENABLED (CONFIG_DRM_SUN8I_TCON_TOP )) { ret =sun8i_tcon_top_de_config (&pdev->dev, tcon->id, id); -if (ret) { -put_device (&pdev->dev); -return ret; - } +if (ret) + goto out_put_device; } -return 0 ; +out_put_device: +put_device (&pdev->dev); + +return ret; }static const struct sun4i_tcon_quirks sun4i_a10_quirks = { --2.43.0
Jernej Skrabec
maintainerPATCH 7/13 drm/sun4i: hdmi: Don't leak sync polarity bits into packet control
In reply to a message not in the archivesun4i_hdmi_enable() keeps using the same variable after it programmed the video timing polarity register with it. The leftover TX_CLK, HSYNC and VSYNC bits are then ORed into the packet control register, where each nibble selects the packet type sent in one slot.
As a result, slot 0 selects packet type 3 instead of the AVI infoframe whenever the mode has positive HSYNC polarity, and the TX_CLK bits set nibbles which the driver never programs.
Assign the packet types instead of ORing them into the stale value.
--- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +1 -1
@@ -161,7 +161,7 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder,drm_atomic_helper_connector_hdmi_update_infoframes (connector, state); - val |=SUN4I_HDMI_PKT_CTRL_TYPE (0 ,SUN4I_HDMI_PKT_AVI ); + val =SUN4I_HDMI_PKT_CTRL_TYPE (0 ,SUN4I_HDMI_PKT_AVI ); val |=SUN4I_HDMI_PKT_CTRL_TYPE (1 ,SUN4I_HDMI_PKT_END );writel (val, hdmi->base +SUN4I_HDMI_PKT_CTRL_REG (0 ));
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH 07/13] drm/sun4i: hdmi: Don't leak sync polarity bits into packet control
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Encoder atomic enable callback calls functions that can sleep. --
--- Patch [7]: [PATCH 07/13] drm/sun4i: hdmi: Don't leak sync polarity bits into packet control --- commit 65d502c65ade8ff425b6b395888cc0a59bbbb7c3
Author: Jernej Skrabec <[email protected]>
drm/sun4i: hdmi: Don't leak sync polarity bits into packet control
This commit fixes an issue where stale sync polarity bits were ORed into the packet control register. It replaces the OR assignment with a direct assignment to prevent selecting incorrect packet types.
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c static void sun4i_hdmi_enable(struct drm_encoder *encoder, · line 161
Quoted 7 lines
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +0
-0@@ -160,7 +160,7 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder,drm_atomic_helper_connector_hdmi_update_infoframes (connector, state);
[Severity: High]
This isn't a bug introduced by this patch, but does this code sleep in an atomic context?
The sun4i_hdmi_enable() function is registered as an encoder atomic enable callback, which executes in an atomic context during non-blocking commits. However, it calls drm_atomic_helper_connector_hdmi_update_infoframes() here, which explicitly acquires a mutex. It also calls clk_prepare_enable() just above this snippet, which can sleep.
Could calling these sleeping functions during atomic commit paths cause kernel warnings or potential deadlocks?
Quoted 5 lines
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +1
-1- val |=SUN4I_HDMI_PKT_CTRL_TYPE (0 ,SUN4I_HDMI_PKT_AVI ); + val =SUN4I_HDMI_PKT_CTRL_TYPE (0 ,SUN4I_HDMI_PKT_AVI ); val |=SUN4I_HDMI_PKT_CTRL_TYPE (1 ,SUN4I_HDMI_PKT_END );writel (val, hdmi->base +SUN4I_HDMI_PKT_CTRL_REG (0 ));
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
Re: [PATCH 07/13] drm/sun4i: hdmi: Don't leak sync polarity bits into packet control
In reply to Jernej SkrabecQuoted 6 lines · Jernej Skrabec
sun4i_hdmi_enable()keeps using the same variable after it programmed the video timing polarity register with it. The leftoverTX_CLK, HSYNC and VSYNC bits are then ORed into the packet control register, where each nibble selects the packet type sent in one slot.
Quoted 8 lines
As a result, slot 0 selects packet type 3 instead of the AVI infoframe whenever the mode has positive HSYNC polarity, and the
TX_CLKbits set nibbles which the driver never programs.Assign the packet types instead of ORing them into the stale value.
Fixes 9ca6bc246035 ("drm/sun4i: hdmi: Move mode_set into enable")Signed-off-by Jernej Skrabec <[email protected]>
Quoted 21 lines
--- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +1
-2@@ -161,7 +161,7 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder,drm_atomic_helper_connector_hdmi_update_infoframes (connector, state); - val |=SUN4I_HDMI_PKT_CTRL_TYPE (0 ,SUN4I_HDMI_PKT_AVI ); + val =SUN4I_HDMI_PKT_CTRL_TYPE (0 ,SUN4I_HDMI_PKT_AVI ); val |=SUN4I_HDMI_PKT_CTRL_TYPE (1 ,SUN4I_HDMI_PKT_END );writel (val, hdmi->base +SUN4I_HDMI_PKT_CTRL_REG (0 )); --2.43.0
Jernej Skrabec
maintainerPATCH 8/13 drm/sun4i: crtc: Propagate layer initialization error
In reply to a message not in the archivesun4i_crtc_init() returns plain NULL when layer initialization fails, while all its other error paths return an error pointer. The only caller, sun4i_tcon_bind(), checks the result with IS_ERR() and happily continues with tcon->crtc set to NULL. sun4i_rgb_init() and sun4i_lvds_init() then dereference it in drm_crtc_mask(), which oopses.
Return the error pointer instead.
--- drivers/gpu/drm/sun4i/sun4i_crtc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_crtc.c +1 -1
@@ -208,7 +208,7 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm, planes =sunxi_engine_layers_init (drm, engine);if (IS_ERR (planes)) {dev_err (drm->dev,"Couldn't create the planes\n" ); -return NULL ; +return ERR_CAST (planes); }/* find primary and cursor planes for drm_crtc_init_with_planes */
Signature · 1 line
2.43.0
Re: [PATCH 08/13] drm/sun4i: crtc: Propagate layer initialization error
Quoted 8 lines · Jernej Skrabec
sun4i_crtc_init()returns plain NULL when layer initialization fails, while all its other error paths return an error pointer. The only caller,sun4i_tcon_bind(), checks the result withIS_ERR()and happily continues withtcon->crtcset to NULL.sun4i_rgb_init()andsun4i_lvds_init()then dereference it indrm_crtc_mask(), which oopses.
Quoted 4 lines
Return the error pointer instead.
Fixes dcd215801b02 ("drm/sun4i: Drop primary layer pointer from sun4i_drv")Signed-off-by Jernej Skrabec <[email protected]>
Quoted 21 lines
--- drivers/gpu/drm/sun4i/sun4i_crtc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/sun4i/sun4i_crtc.c +1
-2@@ -208,7 +208,7 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm, planes =sunxi_engine_layers_init (drm, engine);if (IS_ERR (planes)) {dev_err (drm->dev,"Couldn't create the planes\n" ); -return NULL ; +return ERR_CAST (planes); }/* find primary and cursor planes for drm_crtc_init_with_planes */ --2.43.0
Jernej Skrabec
maintainerPATCH 9/13 drm/sun4i: tcon: Drop remote endpoint reference
In reply to a message not in the archivesun4i_tcon_of_get_id_from_port() never drops the reference taken by of_graph_get_remote_endpoint(). The function is not only called during bind, but also on every mode set through sun8i_r40_tcon_tv_set_mux(), so the leak accumulates.
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpu/drm/sun4i/sun4i_tcon.c +1 -0
@@ -972,6 +972,7 @@ static int sun4i_tcon_of_get_id_from_port(struct device_node *port)continue ; ret =of_property_read_u32 (remote,"reg" , ®); +of_node_put (remote);if (ret)continue ;
Signature · 1 line
2.43.0
Re: [PATCH 09/13] drm/sun4i: tcon: Drop remote endpoint reference
Quoted 8 lines · Jernej Skrabec
sun4i_tcon_of_get_id_from_port()never drops the reference taken byof_graph_get_remote_endpoint(). The function is not only called during bind, but also on every mode set throughsun8i_r40_tcon_tv_set_mux(), so the leak accumulates.Fixes e8d5bbf7f4c4 ("drm/sun4i: tcon: get TCON ID and matching engine with remote endpoint ID")Signed-off-by Jernej Skrabec <[email protected]>
Quoted 20 lines
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/sun4i/sun4i_tcon.c +1
-1@@ -972,6 +972,7 @@ static int sun4i_tcon_of_get_id_from_port(struct device_node *port)continue ; ret =of_property_read_u32 (remote,"reg" , ®); +of_node_put (remote);if (ret)continue ; --2.43.0
Jernej Skrabec
maintainerPATCH 10/13 drm/sun4i: dw-hdmi: Drop TCON TOP port reference
In reply to a message not in the archiveWhen the HDMI controller is fed by TCON TOP, the port node used to enumerate the possible CRTCs is never released.
--- drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c +2 -0
@@ -83,6 +83,8 @@ static u32 sun8i_dw_hdmi_find_possible_crtcs(struct drm_device *drm,of_node_put (remote_port); } } + +of_node_put (port); }else { crtcs =drm_of_find_possible_crtcs (drm, node); }
Signature · 1 line
2.43.0
Re: [PATCH 10/13] drm/sun4i: dw-hdmi: Drop TCON TOP port reference
Quoted 6 lines · Jernej Skrabec
When the HDMI controller is fed by TCON TOP, the port node used to enumerate the possible CRTCs is never released.
Fixes 57e23de02f48 ("drm/sun4i: DW HDMI: Expand algorithm for possible crtcs")Signed-off-by Jernej Skrabec <[email protected]>
Jernej Skrabec
maintainerPATCH 11/13 drm/sun4i: Drop node references while building component list
In reply to a message not in the archiveTwo references are leaked every time the display pipeline is walked:
the output port node in sun4i_drv_traverse_endpoints(), which was never released since the driver was introduced, and each node taken out of the endpoint fifo in sun4i_drv_probe(), which stopped being released when the fifo was introduced. The latter is still safe to drop right after processing, since drm_of_component_match_add() takes its own reference.
--- drivers/gpu/drm/sun4i/sun4i_drv.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/gpu/drm/sun4i/sun4i_drv.c +3 -0
@@ -289,6 +289,8 @@ static void sun4i_drv_traverse_endpoints(struct endpoint_list *list,kfifo_put (&list->fifo, remote); } + +of_node_put (port); } static int sun4i_drv_add_endpoints(struct device *dev,
@@ -394,6 +396,7 @@ static int sun4i_drv_probe(struct platform_device *pdev)/* process this endpoint */ ret =sun4i_drv_add_endpoints (&pdev->dev, &list, &match, endpoint); +of_node_put (endpoint);/* sun4i_drv_add_endpoints can fail to allocate memory */ if (ret <0 )
Signature · 1 line
2.43.0
Re: [PATCH 11/13] drm/sun4i: Drop node references while building component list
Quoted 9 lines · Jernej Skrabec
Two references are leaked every time the display pipeline is walked:
the output port node insun4i_drv_traverse_endpoints(), which was never released since the driver was introduced, and each node taken out of the endpoint fifo insun4i_drv_probe(), which stopped being released when the fifo was introduced. The latter is still safe to drop right after processing, sincedrm_of_component_match_add()takes its own reference.
Quoted 3 lines
Fixes 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")Fixes 8b11aaface2b ("drm/sun4i: Implement endpoint parsing using kfifo")Signed-off-by Jernej Skrabec <[email protected]>
Quoted 28 lines
--- drivers/gpu/drm/sun4i/sun4i_drv.c | 3 +++ 1 file changed, 3 insertions(+)drivers/gpu/drm/sun4i/sun4i_drv.c +3
-1@@ -289,6 +289,8 @@ static void sun4i_drv_traverse_endpoints(struct endpoint_list *list,kfifo_put (&list->fifo, remote); } + +of_node_put (port); } static int sun4i_drv_add_endpoints(struct device *dev,@@ -394,6 +396,7 @@ static int sun4i_drv_probe(struct platform_device *pdev)/* process this endpoint */ ret =sun4i_drv_add_endpoints (&pdev->dev, &list, &match, endpoint); +of_node_put (endpoint);/* sun4i_drv_add_endpoints can fail to allocate memory */ if (ret <0 ) --2.43.0
Jernej Skrabec
maintainerPATCH 12/13 drm/sun4i: hdmi-phy: Fix H6 8-bit MPLL config at 594 MHz
In reply to a message not in the archiveThe 8-bit entry of the last MPLL row (594 MHz) doesn't lock reliably on H6. 4K@60 RGB/YUV444, which is the mode that reaches this entry, doesn't come up.
Align the value with the vendor driver. Other entries are left alone, they are used by lower pixel clocks which work fine.
Tested with 4K@60 on a LG TV.
--- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +1 -1
@@ -91,7 +91,7 @@ static const struct dw_hdmi_mpll_config sun50i_h6_mpll_cfg[] = { }, }, { 594000000, { - { 0x1a40, 0x0003 }, + { 0x1a7c, 0x0003 }, { 0x3b4c, 0x0003 }, { 0x5a64, 0x0003 }, },
Signature · 1 line
2.43.0
Re: [PATCH 12/13] drm/sun4i: hdmi-phy: Fix H6 8-bit MPLL config at 594 MHz
Quoted 5 lines · Jernej Skrabec
The 8-bit entry of the last MPLL row (594 MHz) doesn't lock reliably on H6. 4K@60 RGB/YUV444, which is the mode that reaches this entry, doesn't come up.
Quoted 7 lines
Align the value with the vendor driver. Other entries are left alone, they are used by lower pixel clocks which work fine.
Tested with 4K@60 on a LG TV.
Fixes 0fb4b858b102 ("drm/sun4i: Add support for H6 HDMI PHY")Signed-off-by Jernej Skrabec <[email protected]>
Quoted 20 lines
--- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +1
-2@@ -91,7 +91,7 @@ static const struct dw_hdmi_mpll_config sun50i_h6_mpll_cfg[] = { }, }, { 594000000, { - { 0x1a40, 0x0003 }, + { 0x1a7c, 0x0003 }, { 0x3b4c, 0x0003 }, { 0x5a64, 0x0003 }, }, --2.43.0
Jernej Skrabec
maintainerPATCH 13/13 drm/sun4i: Align VI buffer addresses for subsampled formats
In reply to a message not in the archiveThis is a partial revert of commit 79ac1c945ab8 ("drm/sun4i: layers:
Use drm_fb_dma_get_gem_addr() to get display memory").
Chroma must start at the beginning of a subsampling block, for example chroma start address for NV12 must be aligned to 2 pixels. drm_fb_dma_get_gem_addr() offsets luma by the exact source coordinates and chroma by the coordinates divided by the subsampling factor, so for odd offsets both planes no longer describe the same pixel, which the Display Engine scaler can't handle.
Align source coordinates down for all planes instead. Remaining shift of one pixel is already compensated with scaler phase shift in sun8i_vi_layer_update_coord().
--- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +18 -2
@@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,struct drm_plane_state *state = plane->state;struct drm_framebuffer *fb = state->fb;const struct drm_format_info *format = fb->format; +struct drm_gem_dma_object *gem; +u32 dx, dy, src_x, src_y;dma_addr_t dma_addr;u32 ch_base;int i; ch_base =sun8i_channel_base (layer); +/* Adjust x and y to be divisible by subsampling factor */ + src_x = (state->src.x1 >>16 ) & ~(format->hsub -1 ); + src_y = (state->src.y1 >>16 ) & ~(format->vsub -1 ); +for (i =0 ; i < format->num_planes; i++) { -/* Get the start of the displayed memory */ - dma_addr =drm_fb_dma_get_gem_addr (fb, state, i); + gem =drm_fb_dma_get_gem_obj (fb, i); + dma_addr = gem->dma_addr + fb->offsets[i]; + + dx = src_x; + dy = src_y; +if (i >0 ) { + dx /= format->hsub; + dy /= format->vsub; + } + + dma_addr += dx * format->cpp[i]; + dma_addr += dy * fb->pitches[i];/* Set the line width */ DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n",
Signature · 1 line
2.43.0
Chen-Yu Tsai
maintainerRe: [PATCH 13/13] drm/sun4i: Align VI buffer addresses for subsampled formats
Quoted 11 lines · Jernej Skrabec
This is a partial revert of commit
79ac1c945ab8("drm/sun4i: layers:
Usedrm_fb_dma_get_gem_addr()to get display memory").Chroma must start at the beginning of a subsampling block, for example chroma start address for NV12 must be aligned to 2 pixels.
drm_fb_dma_get_gem_addr()offsets luma by the exact source coordinates and chroma by the coordinates divided by the subsampling factor, so for odd offsets both planes no longer describe the same pixel, which the Display Engine scaler can't handle.
Quoted 3 lines
Align source coordinates down for all planes instead. Remaining shift of one pixel is already compensated with scaler phase shift in
sun8i_vi_layer_update_coord().
Well I think this applies to the format in general, and probably should be fixed in drm_fb_dma_get_gem_addr() instead?
Quoted 15 lines
Fixes 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")Signed-off-by Jernej Skrabec <[email protected]>--- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)drivers/gpu/drm/sun4i/sun8i_vi_layer.c +1
-0@@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,struct drm_plane_state *state = plane->state;struct drm_framebuffer *fb = state->fb;const struct drm_format_info *format = fb->format; +struct drm_gem_dma_object *gem;
drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 210
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +4
-0+u32 dx, dy, src_x, src_y;dma_addr_t dma_addr;u32 ch_base;int i; ch_base =sun8i_channel_base (layer); +/* Adjust x and y to be divisible by subsampling factor */ + src_x = (state->src.x1 >>16 ) & ~(format->hsub -1 ); + src_y = (state->src.y1 >>16 ) & ~(format->vsub -1 );
AFAICT the only difference compared to drm_fb_dma_get_gem_addr() is the masking here, i.e. round_down().
Quoted 6 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +3
-2+for (i =0 ; i < format->num_planes; i++) { -/* Get the start of the displayed memory */ - dma_addr =drm_fb_dma_get_gem_addr (fb, state, i); + gem =drm_fb_dma_get_gem_obj (fb, i); + dma_addr = gem->dma_addr + fb->offsets[i];
drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 224
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +10
-0+ + dx = src_x; + dy = src_y; +if (i >0 ) { + dx /= format->hsub; + dy /= format->vsub; + } + + dma_addr += dx * format->cpp[i]; + dma_addr += dy * fb->pitches[i];
Where as the helper has (or used to have before the blocksize stuff):
paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub;
paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;Am I missing something?
ChenYu
Quoted 6 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +0
-1/* Set the line width */ DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n", --2.43.0
Chen-Yu Tsai
maintainerRe: [PATCH 13/13] drm/sun4i: Align VI buffer addresses for subsampled formats
Quoted 69 lines · Chen-Yu Tsai
Quoted 11 lines · Jernej Skrabec
This is a partial revert of commit
79ac1c945ab8("drm/sun4i: layers:
Usedrm_fb_dma_get_gem_addr()to get display memory").Chroma must start at the beginning of a subsampling block, for example chroma start address for NV12 must be aligned to 2 pixels.
drm_fb_dma_get_gem_addr()offsets luma by the exact source coordinates and chroma by the coordinates divided by the subsampling factor, so for odd offsets both planes no longer describe the same pixel, which the Display Engine scaler can't handle.Quoted 3 lines
Align source coordinates down for all planes instead. Remaining shift of one pixel is already compensated with scaler phase shift in
sun8i_vi_layer_update_coord().Well I think this applies to the format in general, and probably should be fixed in
drm_fb_dma_get_gem_addr()instead?Quoted 15 lines
Fixes 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")Signed-off-by Jernej Skrabec <[email protected]>--- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)drivers/gpu/drm/sun4i/sun8i_vi_layer.c +1
-0@@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,struct drm_plane_state *state = plane->state;struct drm_framebuffer *fb = state->fb;const struct drm_format_info *format = fb->format; +struct drm_gem_dma_object *gem;drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 210
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +4
-0+u32 dx, dy, src_x, src_y;dma_addr_t dma_addr;u32 ch_base;int i; ch_base =sun8i_channel_base (layer); +/* Adjust x and y to be divisible by subsampling factor */ + src_x = (state->src.x1 >>16 ) & ~(format->hsub -1 ); + src_y = (state->src.y1 >>16 ) & ~(format->vsub -1 );AFAICT the only difference compared to
drm_fb_dma_get_gem_addr()is the masking here, i.e.round_down().Quoted 6 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +3
-2+for (i =0 ; i < format->num_planes; i++) { -/* Get the start of the displayed memory */ - dma_addr =drm_fb_dma_get_gem_addr (fb, state, i); + gem =drm_fb_dma_get_gem_obj (fb, i); + dma_addr = gem->dma_addr + fb->offsets[i];drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 224
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +10
-0+ + dx = src_x; + dy = src_y; +if (i >0 ) { + dx /= format->hsub; + dy /= format->vsub; + } + + dma_addr += dx * format->cpp[i]; + dma_addr += dy * fb->pitches[i];Where as the helper has (or used to have before the blocksize stuff):
Quoted 4 lines
paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub; paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;Am I missing something?
After some headbanging on my end I see that the offset for the Y plane needs to be rounded down.
But instead of reverting the whole thing and open-coding the helper again, could you adjust the address returned by the helper for odd offsets?
And just a heads up, this also needs a clipped version of drm_fb_dma_get_gem_addr() as sun8i_ui_layer_update_coord() uses the clipped dimensions. I am currently working on this part.
ChenYu
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +0
-1/* Set the line width */ DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n", --2.43.0
Jernej Škrabec
maintainerRe: [PATCH 13/13] drm/sun4i: Align VI buffer addresses for subsampled formats
Dne torek, 4. avgust 2026 ob 13:14:38 Srednjeevropski poletni čas je Chen-Yu Tsai napisal(a):
Quoted 73 lines · Chen-Yu Tsai
Quoted 72 lines · Jernej Skrabec
Quoted 11 lines
This is a partial revert of commit
79ac1c945ab8("drm/sun4i: layers:
Usedrm_fb_dma_get_gem_addr()to get display memory").Chroma must start at the beginning of a subsampling block, for example chroma start address for NV12 must be aligned to 2 pixels.
drm_fb_dma_get_gem_addr()offsets luma by the exact source coordinates and chroma by the coordinates divided by the subsampling factor, so for odd offsets both planes no longer describe the same pixel, which the Display Engine scaler can't handle.Quoted 3 lines
Align source coordinates down for all planes instead. Remaining shift of one pixel is already compensated with scaler phase shift in
sun8i_vi_layer_update_coord().Well I think this applies to the format in general, and probably should be fixed in
drm_fb_dma_get_gem_addr()instead?Quoted 15 lines
Fixes 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")Signed-off-by Jernej Skrabec <[email protected]>--- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)drivers/gpu/drm/sun4i/sun8i_vi_layer.c +1
-0@@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,struct drm_plane_state *state = plane->state;struct drm_framebuffer *fb = state->fb;const struct drm_format_info *format = fb->format; +struct drm_gem_dma_object *gem;drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 210
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +4
-0+u32 dx, dy, src_x, src_y;dma_addr_t dma_addr;u32 ch_base;int i; ch_base =sun8i_channel_base (layer); +/* Adjust x and y to be divisible by subsampling factor */ + src_x = (state->src.x1 >>16 ) & ~(format->hsub -1 ); + src_y = (state->src.y1 >>16 ) & ~(format->vsub -1 );AFAICT the only difference compared to
drm_fb_dma_get_gem_addr()is the masking here, i.e.round_down().Quoted 6 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +3
-2+for (i =0 ; i < format->num_planes; i++) { -/* Get the start of the displayed memory */ - dma_addr =drm_fb_dma_get_gem_addr (fb, state, i); + gem =drm_fb_dma_get_gem_obj (fb, i); + dma_addr = gem->dma_addr + fb->offsets[i];drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 224
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +10
-0+ + dx = src_x; + dy = src_y; +if (i >0 ) { + dx /= format->hsub; + dy /= format->vsub; + } + + dma_addr += dx * format->cpp[i]; + dma_addr += dy * fb->pitches[i];Where as the helper has (or used to have before the blocksize stuff):
paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub; paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;
Quoted 8 lines
Quoted 1 line
Am I missing something?
After some headbanging on my end I see that the offset for the Y plane needs to be rounded down.
But instead of reverting the whole thing and open-coding the helper again, could you adjust the address returned by the helper for odd offsets?
Yes, that's also an option. I'll do it in v2.
Quoted 4 lines
And just a heads up, this also needs a clipped version of
drm_fb_dma_get_gem_addr()assun8i_ui_layer_update_coord()uses the clipped dimensions. I am currently working on this part.
Can you explain a bit more? I don't see why it needs any adjustement.
Best regards,
Jernej
Quoted 11 lines
ChenYu
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +0
-1/* Set the line width */ DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n", --2.43.0
Chen-Yu Tsai
maintainerRe: [PATCH 13/13] drm/sun4i: Align VI buffer addresses for subsampled formats
Quoted 84 lines · Jernej Škrabec
Dne torek, 4. avgust 2026 ob 13:14:38 Srednjeevropski poletni čas je Chen-Yu Tsai napisal(a):
Quoted 73 lines · Chen-Yu Tsai
Quoted 72 lines · Jernej Skrabec
Quoted 11 lines
This is a partial revert of commit
79ac1c945ab8("drm/sun4i: layers:
Usedrm_fb_dma_get_gem_addr()to get display memory").Chroma must start at the beginning of a subsampling block, for example chroma start address for NV12 must be aligned to 2 pixels.
drm_fb_dma_get_gem_addr()offsets luma by the exact source coordinates and chroma by the coordinates divided by the subsampling factor, so for odd offsets both planes no longer describe the same pixel, which the Display Engine scaler can't handle.Quoted 3 lines
Align source coordinates down for all planes instead. Remaining shift of one pixel is already compensated with scaler phase shift in
sun8i_vi_layer_update_coord().Well I think this applies to the format in general, and probably should be fixed in
drm_fb_dma_get_gem_addr()instead?Quoted 15 lines
Fixes 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")Signed-off-by Jernej Skrabec <[email protected]>--- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)drivers/gpu/drm/sun4i/sun8i_vi_layer.c +1
-0@@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,struct drm_plane_state *state = plane->state;struct drm_framebuffer *fb = state->fb;const struct drm_format_info *format = fb->format; +struct drm_gem_dma_object *gem;drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 210
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +4
-0+u32 dx, dy, src_x, src_y;dma_addr_t dma_addr;u32 ch_base;int i; ch_base =sun8i_channel_base (layer); +/* Adjust x and y to be divisible by subsampling factor */ + src_x = (state->src.x1 >>16 ) & ~(format->hsub -1 ); + src_y = (state->src.y1 >>16 ) & ~(format->vsub -1 );AFAICT the only difference compared to
drm_fb_dma_get_gem_addr()is the masking here, i.e.round_down().Quoted 6 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +3
-2+for (i =0 ; i < format->num_planes; i++) { -/* Get the start of the displayed memory */ - dma_addr =drm_fb_dma_get_gem_addr (fb, state, i); + gem =drm_fb_dma_get_gem_obj (fb, i); + dma_addr = gem->dma_addr + fb->offsets[i];drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 224
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +10
-0+ + dx = src_x; + dy = src_y; +if (i >0 ) { + dx /= format->hsub; + dy /= format->vsub; + } + + dma_addr += dx * format->cpp[i]; + dma_addr += dy * fb->pitches[i];Where as the helper has (or used to have before the blocksize stuff):
paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub; paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;Quoted 8 lines
Quoted 1 line
Am I missing something?
After some headbanging on my end I see that the offset for the Y plane needs to be rounded down.
But instead of reverting the whole thing and open-coding the helper again, could you adjust the address returned by the helper for odd offsets?
Quoted 8 lines
Yes, that's also an option. I'll do it in v2.
Quoted 4 lines
And just a heads up, this also needs a clipped version of
drm_fb_dma_get_gem_addr()assun8i_ui_layer_update_coord()uses the clipped dimensions. I am currently working on this part.Can you explain a bit more? I don't see why it needs any adjustement.
My understanding is that drm_atomic_helper_check_plane_state() calculates the "clipped" rectangles for the plane using values from userspace in state->src_[xywh] and state->crtc_[xywh] and puts them in state->src and state->dst, respectively. If the overlay is moved partially outside the screen, the overlay is "clipped".
Say we have a screen of 1920x1080, with an overlay buffer that is 1280x720. Say state->src_x and state->src_y are (-50, 0), given by userspace. drm_atomic_helper_check_plane_state() will calculate the clipped & scaled rectangles and put them in state->src. This latter rectangle is what is used sun8i_ui_layer_update_coord().
So we would have:
(src_x, src_y) = (-50, 0), (src_w, src_h) = (1280, 720)
The clipped numbers are
(src.x1, src.y1) = (0, 0), (src.x2, src.y2) = (1230, 720)
src, not src_[xywh], is what sun8i layers uses to program the coordinates, and prior to the drm_fb_dma_get_gem_addr() conversion, also to calculate the buffer start address. drm_fb_dma_get_gem_addr() however uses src_[xy] to calculate the address.
Essentially, when overlaying a clipped plane, the start address needs to be adjusted if the source (top left) offset is outside the screen. clipping == automatic cropping to fit the screen.
I don't know if userspace applications routinely do this, but I think this needs to be restored to the prior behavior.
ChenYu
Jernej Škrabec
maintainerRe: [PATCH 13/13] drm/sun4i: Align VI buffer addresses for subsampled formats
Dne torek, 4. avgust 2026 ob 19:04:10 Srednjeevropski poletni čas je Chen-Yu Tsai napisal(a):
Quoted 121 lines · Jernej Škrabec
Quoted 84 lines
Dne torek, 4. avgust 2026 ob 13:14:38 Srednjeevropski poletni čas je Chen-Yu Tsai napisal(a):
Quoted 73 lines · Chen-Yu Tsai
Quoted 72 lines · Jernej Skrabec
Quoted 11 lines
This is a partial revert of commit
79ac1c945ab8("drm/sun4i: layers:
Usedrm_fb_dma_get_gem_addr()to get display memory").Chroma must start at the beginning of a subsampling block, for example chroma start address for NV12 must be aligned to 2 pixels.
drm_fb_dma_get_gem_addr()offsets luma by the exact source coordinates and chroma by the coordinates divided by the subsampling factor, so for odd offsets both planes no longer describe the same pixel, which the Display Engine scaler can't handle.Quoted 3 lines
Align source coordinates down for all planes instead. Remaining shift of one pixel is already compensated with scaler phase shift in
sun8i_vi_layer_update_coord().Well I think this applies to the format in general, and probably should be fixed in
drm_fb_dma_get_gem_addr()instead?Quoted 15 lines
Fixes 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")Signed-off-by Jernej Skrabec <[email protected]>--- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)drivers/gpu/drm/sun4i/sun8i_vi_layer.c +1
-0@@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,struct drm_plane_state *state = plane->state;struct drm_framebuffer *fb = state->fb;const struct drm_format_info *format = fb->format; +struct drm_gem_dma_object *gem;drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 210
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +4
-0+u32 dx, dy, src_x, src_y;dma_addr_t dma_addr;u32 ch_base;int i; ch_base =sun8i_channel_base (layer); +/* Adjust x and y to be divisible by subsampling factor */ + src_x = (state->src.x1 >>16 ) & ~(format->hsub -1 ); + src_y = (state->src.y1 >>16 ) & ~(format->vsub -1 );AFAICT the only difference compared to
drm_fb_dma_get_gem_addr()is the masking here, i.e.round_down().Quoted 6 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +3
-2+for (i =0 ; i < format->num_planes; i++) { -/* Get the start of the displayed memory */ - dma_addr =drm_fb_dma_get_gem_addr (fb, state, i); + gem =drm_fb_dma_get_gem_obj (fb, i); + dma_addr = gem->dma_addr + fb->offsets[i];drivers/gpu/drm/sun4i/sun8i_vi_layer.c static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer, · line 224
Quoted 10 lines
drivers/gpu/drm/sun4i/sun8i_vi_layer.c +10
-0+ + dx = src_x; + dy = src_y; +if (i >0 ) { + dx /= format->hsub; + dy /= format->vsub; + } + + dma_addr += dx * format->cpp[i]; + dma_addr += dy * fb->pitches[i];Where as the helper has (or used to have before the blocksize stuff):
paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub; paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;Quoted 8 lines
Quoted 1 line
Am I missing something?
After some headbanging on my end I see that the offset for the Y plane needs to be rounded down.
But instead of reverting the whole thing and open-coding the helper again, could you adjust the address returned by the helper for odd offsets?
Quoted 8 lines
Yes, that's also an option. I'll do it in v2.
Quoted 4 lines
And just a heads up, this also needs a clipped version of
drm_fb_dma_get_gem_addr()assun8i_ui_layer_update_coord()uses the clipped dimensions. I am currently working on this part.Can you explain a bit more? I don't see why it needs any adjustement.
My understanding is that
drm_atomic_helper_check_plane_state()calculates the "clipped" rectangles for the plane using values from userspace in state->src_[xywh] and state->crtc_[xywh] and puts them instate->srcandstate->dst, respectively. If the overlay is moved partially outside the screen, the overlay is "clipped".Say we have a screen of 1920x1080, with an overlay buffer that is 1280x720. Say
state->src_xandstate->src_yare (-50, 0), given by userspace.drm_atomic_helper_check_plane_state()will calculate the clipped & scaled rectangles and put them instate->src. This latter rectangle is what is usedsun8i_ui_layer_update_coord().So we would have:
(src_x, src_y) = (-50, 0), (src_w, src_h) = (1280, 720)The clipped numbers are
(src.x1, src.y1) = (0, 0), (src.x2, src.y2) = (1230, 720)src, not src_[xywh], is what sun8i layers uses to program the coordinates, and prior to the
drm_fb_dma_get_gem_addr()conversion, also to calculate the buffer start address.drm_fb_dma_get_gem_addr()however uses src_[xy] to calculate the address.
Quoted 6 lines
Essentially, when overlaying a clipped plane, the start address needs to be adjusted if the source (top left) offset is outside the screen. clipping == automatic cropping to fit the screen.
I don't know if userspace applications routinely do this, but I think this needs to be restored to the prior behavior.
Uh, it would be nice if this is fixed. But I think it's not too common for app to use negative coordinates.
Best regards,
Jernej