public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 0/2] drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation
@ 2026-06-02 17:00 Alexey Charkov
  2026-06-02 17:00 ` [PATCH 1/2] drm/rockchip: vop2: honor TV margins from CRTC state for " Alexey Charkov
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Alexey Charkov @ 2026-06-02 17:00 UTC (permalink / raw)
  To: Sandy Huang, Heiko Stübner, Andy Yan, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
	Cristian Ciocaltea
  Cc: dri-devel, linux-arm-kernel, linux-rockchip, linux-kernel,
	Alexey Charkov

When using a TV as a display, output image may be cropped due to overscan.
Rockchip VOP2 supports post-composition scaling in hardware, which can be
used to compensate for the overscan.

This adds the necessary pixel margin fields to the CRTC state, and sets
them from the TV overscan property of the connector which is understood
by the userspace (tested with KWin's display configuration module).

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
Alexey Charkov (2):
      drm/rockchip: vop2: honor TV margins from CRTC state for overscan compensation
      drm/rockchip: dw_hdmi_qp: expose "overscan" property

 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h    |  2 ++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c   | 17 +++++++++++------
 3 files changed, 29 insertions(+), 6 deletions(-)
---
base-commit: 08484c504b55a98bd100527fbe10a3caf55ff3ff
change-id: 20260602-hdmi-overscan-4f0d89bfd981

Best regards,
-- 
Alexey Charkov <alchark@flipper.net>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] drm/rockchip: vop2: honor TV margins from CRTC state for overscan compensation
  2026-06-02 17:00 [PATCH 0/2] drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation Alexey Charkov
@ 2026-06-02 17:00 ` Alexey Charkov
  2026-06-04  2:35   ` Claude review: " Claude Code Review Bot
  2026-06-02 17:00 ` [PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property Alexey Charkov
  2026-06-04  2:35 ` Claude review: drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation Claude Code Review Bot
  2 siblings, 1 reply; 9+ messages in thread
From: Alexey Charkov @ 2026-06-02 17:00 UTC (permalink / raw)
  To: Sandy Huang, Heiko Stübner, Andy Yan, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
	Cristian Ciocaltea
  Cc: dri-devel, linux-arm-kernel, linux-rockchip, linux-kernel,
	Alexey Charkov

Replace the hard-coded percent values with pixel margins carried in struct
rockchip_crtc_state, sourced from the standard DRM "left/right/top/bottom
margin" connector properties (struct drm_connector_tv_margins) to pave way
for HDMI overscan compensation support.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h  |  2 ++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 17 +++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 2e86ad00979c..83f8ec4f3319 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -10,6 +10,7 @@
 #define _ROCKCHIP_DRM_DRV_H
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
 #include <drm/drm_gem.h>
 
 #include <linux/bits.h>
@@ -53,6 +54,7 @@ struct rockchip_crtc_state {
 	u32 bus_format;
 	u32 bus_flags;
 	int color_space;
+	struct drm_connector_tv_margins tv_margins;
 };
 #define to_rockchip_crtc_state(s) \
 		container_of(s, struct rockchip_crtc_state, base)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index a160077a507f..55307b8493ae 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1558,30 +1558,35 @@ static void vop2_post_config(struct drm_crtc *crtc)
 {
 	struct vop2_video_port *vp = to_vop2_video_port(crtc);
 	struct vop2 *vop2 = vp->vop2;
+	struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(crtc->state);
 	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
+	const struct drm_connector_tv_margins *m = &vcstate->tv_margins;
 	u64 bgcolor = crtc->state->background_color;
 	u16 vtotal = mode->crtc_vtotal;
 	u16 hdisplay = mode->crtc_hdisplay;
 	u16 hact_st = mode->crtc_htotal - mode->crtc_hsync_start;
 	u16 vdisplay = mode->crtc_vdisplay;
 	u16 vact_st = mode->crtc_vtotal - mode->crtc_vsync_start;
-	u32 left_margin = 100, right_margin = 100;
-	u32 top_margin = 100, bottom_margin = 100;
-	u16 hsize = hdisplay * (left_margin + right_margin) / 200;
-	u16 vsize = vdisplay * (top_margin + bottom_margin) / 200;
+	u16 hsize = hdisplay;
+	u16 vsize = vdisplay;
 	u16 hact_end, vact_end;
 	u32 val;
 
 	vop2->ops->setup_bg_dly(vp);
 
+	if (m->left + m->right < hdisplay)
+		hsize = hdisplay - m->left - m->right;
+	if (m->top + m->bottom < vdisplay)
+		vsize = vdisplay - m->top - m->bottom;
+
 	vsize = rounddown(vsize, 2);
 	hsize = rounddown(hsize, 2);
-	hact_st += hdisplay * (100 - left_margin) / 200;
+	hact_st += m->left;
 	hact_end = hact_st + hsize;
 	val = hact_st << 16;
 	val |= hact_end;
 	vop2_vp_write(vp, RK3568_VP_POST_DSP_HACT_INFO, val);
-	vact_st += vdisplay * (100 - top_margin) / 200;
+	vact_st += m->top;
 	vact_end = vact_st + vsize;
 	val = vact_st << 16;
 	val |= vact_end;

-- 
2.52.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property
  2026-06-02 17:00 [PATCH 0/2] drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation Alexey Charkov
  2026-06-02 17:00 ` [PATCH 1/2] drm/rockchip: vop2: honor TV margins from CRTC state for " Alexey Charkov
@ 2026-06-02 17:00 ` Alexey Charkov
  2026-06-03  7:55   ` Maxime Ripard
                     ` (2 more replies)
  2026-06-04  2:35 ` Claude review: drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation Claude Code Review Bot
  2 siblings, 3 replies; 9+ messages in thread
From: Alexey Charkov @ 2026-06-02 17:00 UTC (permalink / raw)
  To: Sandy Huang, Heiko Stübner, Andy Yan, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
	Cristian Ciocaltea
  Cc: dri-devel, linux-arm-kernel, linux-rockchip, linux-kernel,
	Alexey Charkov

Expose the "overscan" connector property as recognized by KWin and the
likes to compensate for TV overscan cropping.

The CRTC will use the margin values derived from this overscan percentage
in its post-composition scaler to add appropriate blank margins on all
sides of the output image so that the TV doesn't eat up visible content.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index f35484715c2d..fae44d11dbef 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -137,10 +137,18 @@ dw_hdmi_qp_rockchip_encoder_atomic_check(struct drm_encoder *encoder,
 					 struct drm_connector_state *conn_state)
 {
 	struct rockchip_hdmi_qp *hdmi = to_rockchip_hdmi_qp(encoder);
+	const struct drm_display_mode *adj_mode = &crtc_state->adjusted_mode;
 	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
 	union phy_configure_opts phy_cfg = {};
+	unsigned int overscan;
 	int ret;
 
+	overscan = min(conn_state->tv.overscan, 100u);
+	s->tv_margins.left   = adj_mode->hdisplay * overscan / 200;
+	s->tv_margins.right  = s->tv_margins.left;
+	s->tv_margins.top    = adj_mode->vdisplay * overscan / 200;
+	s->tv_margins.bottom = s->tv_margins.top;
+
 	if (hdmi->tmds_char_rate == conn_state->hdmi.tmds_char_rate &&
 	    s->output_bpc == conn_state->hdmi.output_bpc)
 		return 0;
@@ -603,6 +611,14 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
 				     "Failed to init bridge connector\n");
 
+	ret = drm_mode_create_tv_properties_legacy(drm, 0, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to create TV connector properties\n");
+
+	drm_object_attach_property(&connector->base,
+				   drm->mode_config.tv_overscan_property, 0);
+
 	return 0;
 }
 

-- 
2.52.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property
  2026-06-02 17:00 ` [PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property Alexey Charkov
@ 2026-06-03  7:55   ` Maxime Ripard
  2026-06-03  8:15     ` Alexey Charkov
  2026-06-03 13:11   ` Andy Yan
  2026-06-04  2:35   ` Claude review: " Claude Code Review Bot
  2 siblings, 1 reply; 9+ messages in thread
From: Maxime Ripard @ 2026-06-03  7:55 UTC (permalink / raw)
  To: Alexey Charkov
  Cc: Sandy Huang, Heiko Stübner, Andy Yan, Maarten Lankhorst,
	Thomas Zimmermann, David Airlie, Simona Vetter,
	Cristian Ciocaltea, dri-devel, linux-arm-kernel, linux-rockchip,
	linux-kernel

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

On Tue, Jun 02, 2026 at 09:00:40PM +0400, Alexey Charkov wrote:
> Expose the "overscan" connector property as recognized by KWin and the
> likes to compensate for TV overscan cropping.
> 
> The CRTC will use the margin values derived from this overscan percentage
> in its post-composition scaler to add appropriate blank margins on all
> sides of the output image so that the TV doesn't eat up visible content.
> 
> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> ---
>  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index f35484715c2d..fae44d11dbef 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -137,10 +137,18 @@ dw_hdmi_qp_rockchip_encoder_atomic_check(struct drm_encoder *encoder,
>  					 struct drm_connector_state *conn_state)
>  {
>  	struct rockchip_hdmi_qp *hdmi = to_rockchip_hdmi_qp(encoder);
> +	const struct drm_display_mode *adj_mode = &crtc_state->adjusted_mode;
>  	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
>  	union phy_configure_opts phy_cfg = {};
> +	unsigned int overscan;
>  	int ret;
>  
> +	overscan = min(conn_state->tv.overscan, 100u);
> +	s->tv_margins.left   = adj_mode->hdisplay * overscan / 200;
> +	s->tv_margins.right  = s->tv_margins.left;
> +	s->tv_margins.top    = adj_mode->vdisplay * overscan / 200;
> +	s->tv_margins.bottom = s->tv_margins.top;
> +
>  	if (hdmi->tmds_char_rate == conn_state->hdmi.tmds_char_rate &&
>  	    s->output_bpc == conn_state->hdmi.output_bpc)
>  		return 0;
> @@ -603,6 +611,14 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
>  				     "Failed to init bridge connector\n");
>  
> +	ret = drm_mode_create_tv_properties_legacy(drm, 0, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to create TV connector properties\n");
> +
> +	drm_object_attach_property(&connector->base,
> +				   drm->mode_config.tv_overscan_property, 0);
> +

As the name suggests, it's a legacy property only ever used for TV. You
should be using drm_mode_create_tv_margin_properties()

Maxime

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property
  2026-06-03  7:55   ` Maxime Ripard
@ 2026-06-03  8:15     ` Alexey Charkov
  0 siblings, 0 replies; 9+ messages in thread
From: Alexey Charkov @ 2026-06-03  8:15 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Sandy Huang, Heiko Stübner, Andy Yan, Maarten Lankhorst,
	Thomas Zimmermann, David Airlie, Simona Vetter,
	Cristian Ciocaltea, dri-devel, linux-arm-kernel, linux-rockchip,
	linux-kernel

On Wed, Jun 3, 2026 at 11:55 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Tue, Jun 02, 2026 at 09:00:40PM +0400, Alexey Charkov wrote:
> > Expose the "overscan" connector property as recognized by KWin and the
> > likes to compensate for TV overscan cropping.
> >
> > The CRTC will use the margin values derived from this overscan percentage
> > in its post-composition scaler to add appropriate blank margins on all
> > sides of the output image so that the TV doesn't eat up visible content.
> >
> > Signed-off-by: Alexey Charkov <alchark@flipper.net>
> > ---
> >  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> > index f35484715c2d..fae44d11dbef 100644
> > --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> > @@ -137,10 +137,18 @@ dw_hdmi_qp_rockchip_encoder_atomic_check(struct drm_encoder *encoder,
> >                                        struct drm_connector_state *conn_state)
> >  {
> >       struct rockchip_hdmi_qp *hdmi = to_rockchip_hdmi_qp(encoder);
> > +     const struct drm_display_mode *adj_mode = &crtc_state->adjusted_mode;
> >       struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
> >       union phy_configure_opts phy_cfg = {};
> > +     unsigned int overscan;
> >       int ret;
> >
> > +     overscan = min(conn_state->tv.overscan, 100u);
> > +     s->tv_margins.left   = adj_mode->hdisplay * overscan / 200;
> > +     s->tv_margins.right  = s->tv_margins.left;
> > +     s->tv_margins.top    = adj_mode->vdisplay * overscan / 200;
> > +     s->tv_margins.bottom = s->tv_margins.top;
> > +
> >       if (hdmi->tmds_char_rate == conn_state->hdmi.tmds_char_rate &&
> >           s->output_bpc == conn_state->hdmi.output_bpc)
> >               return 0;
> > @@ -603,6 +611,14 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
> >               return dev_err_probe(hdmi->dev, PTR_ERR(connector),
> >                                    "Failed to init bridge connector\n");
> >
> > +     ret = drm_mode_create_tv_properties_legacy(drm, 0, NULL);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret,
> > +                                  "Failed to create TV connector properties\n");
> > +
> > +     drm_object_attach_property(&connector->base,
> > +                                drm->mode_config.tv_overscan_property, 0);
> > +
>
> As the name suggests, it's a legacy property only ever used for TV. You
> should be using drm_mode_create_tv_margin_properties()

Hi Maxime, I tried that one before going for the current solution but
realized that the userspace tooling I care about (KWin in particular)
doesn't recognize those properties, but it does recognize "overscan".
Maybe there's a compat helper somewhere that I missed, which would
translate between the two?

It is for TVs. Turns out that having a proper right-sized digital
input interface doesn't prevent them from doing weird stuff with the
image data, i.e. overscanning and cropping it right where one would
expect to see the system tray and panel.

Best regards,
Alexey

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re:[PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property
  2026-06-02 17:00 ` [PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property Alexey Charkov
  2026-06-03  7:55   ` Maxime Ripard
@ 2026-06-03 13:11   ` Andy Yan
  2026-06-04  2:35   ` Claude review: " Claude Code Review Bot
  2 siblings, 0 replies; 9+ messages in thread
From: Andy Yan @ 2026-06-03 13:11 UTC (permalink / raw)
  To: Alexey Charkov
  Cc: Sandy Huang, Heiko Stübner, Andy Yan, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
	Cristian Ciocaltea, dri-devel, linux-arm-kernel, linux-rockchip,
	linux-kernel


Hello Alexy,

At 2026-06-03 01:00:40, "Alexey Charkov" <alchark@flipper.net> wrote:
>Expose the "overscan" connector property as recognized by KWin and the
>likes to compensate for TV overscan cropping.
>
>The CRTC will use the margin values derived from this overscan percentage
>in its post-composition scaler to add appropriate blank margins on all
>sides of the output image so that the TV doesn't eat up visible content.
>
>Signed-off-by: Alexey Charkov <alchark@flipper.net>
>---
> drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
>diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>index f35484715c2d..fae44d11dbef 100644
>--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>@@ -137,10 +137,18 @@ dw_hdmi_qp_rockchip_encoder_atomic_check(struct drm_encoder *encoder,
> 					 struct drm_connector_state *conn_state)
> {
> 	struct rockchip_hdmi_qp *hdmi = to_rockchip_hdmi_qp(encoder);
>+	const struct drm_display_mode *adj_mode = &crtc_state->adjusted_mode;
> 	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
> 	union phy_configure_opts phy_cfg = {};
>+	unsigned int overscan;
> 	int ret;
> 
>+	overscan = min(conn_state->tv.overscan, 100u);
>+	s->tv_margins.left   = adj_mode->hdisplay * overscan / 200;
>+	s->tv_margins.right  = s->tv_margins.left;
>+	s->tv_margins.top    = adj_mode->vdisplay * overscan / 200;
>+	s->tv_margins.bottom = s->tv_margins.top;
>+
> 	if (hdmi->tmds_char_rate == conn_state->hdmi.tmds_char_rate &&
> 	    s->output_bpc == conn_state->hdmi.output_bpc)
> 		return 0;
>@@ -603,6 +611,14 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
> 		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
> 				     "Failed to init bridge connector\n");
> 
>+	ret = drm_mode_create_tv_properties_legacy(drm, 0, NULL);
>+	if (ret)
>+		return dev_err_probe(dev, ret,
>+				     "Failed to create TV connector properties\n");

      I suggest calling a similar API within rockchip_drm_bind, and then attaching this property in
the connector drivers that need to expose it. This approach avoids invoking the API multiple times in multiple drivers。

>+
>+	drm_object_attach_property(&connector->base,
>+				   drm->mode_config.tv_overscan_property, 0);
>+
> 	return 0;
> }
> 
>
>-- 
>2.52.0
>
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation
  2026-06-02 17:00 [PATCH 0/2] drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation Alexey Charkov
  2026-06-02 17:00 ` [PATCH 1/2] drm/rockchip: vop2: honor TV margins from CRTC state for " Alexey Charkov
  2026-06-02 17:00 ` [PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property Alexey Charkov
@ 2026-06-04  2:35 ` Claude Code Review Bot
  2 siblings, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-06-04  2:35 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation
Author: Alexey Charkov <alchark@flipper.net>
Patches: 6
Reviewed: 2026-06-04T12:35:00.723614

---

This is a 2-patch series adding HDMI overscan compensation to the Rockchip dw_hdmi_qp driver. The approach is: (1) plumb pixel margins through `rockchip_crtc_state` so the VOP2 post-compositor scaler can use them instead of hard-coded 100% values, and (2) expose the standard DRM "overscan" TV property on the HDMI connector and convert the single percentage into per-side pixel margins in `atomic_check`.

The overall approach is reasonable and the code is small. There are several issues worth addressing:

**Major concern:** The use of `drm_mode_create_tv_properties_legacy()` is explicitly deprecated. The docstring in the kernel says *"New drivers must use drm_mode_create_tv_properties() instead."* This driver doesn't need analog TV mode selection, so calling the legacy function just to get the overscan property creates a bunch of unnecessary and misleading properties (subconnector, brightness, contrast, flicker reduction, saturation, hue, and the legacy mode selector). Only the overscan property is wanted.

**Moderate concern:** The early-return logic in `atomic_check` has a subtle interaction with margin changes. If the user changes the overscan percentage but the TMDS rate and BPC stay the same, the margins are computed and written to `s->tv_margins`, but `atomic_check` returns 0 without setting `s->output_mode`, `s->output_type`, or `s->output_bpc`. This works because those fields are preserved across state duplicates, but it means the CRTC state is modified without the function explicitly signaling that a modeset-level change occurred. The margins should probably also trigger a mode change to ensure the CRTC re-programs the post-scaler, though in practice `vop2_post_config` may be called on every commit regardless.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: drm/rockchip: vop2: honor TV margins from CRTC state for overscan compensation
  2026-06-02 17:00 ` [PATCH 1/2] drm/rockchip: vop2: honor TV margins from CRTC state for " Alexey Charkov
@ 2026-06-04  2:35   ` Claude Code Review Bot
  0 siblings, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-06-04  2:35 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Good:**
- Cleanly replaces the hard-coded 100% margin values with the new `tv_margins` from `rockchip_crtc_state`.
- The safety check `if (m->left + m->right < hdisplay)` prevents underflow if nonsensical margins are provided.
- Keeps `rounddown(vsize/hsize, 2)` in place after the computation.

**Issues:**

1. **`drm_connector.h` include may be unnecessary.** The `struct drm_connector_tv_margins` is already defined in `<drm/drm_connector.h>`, but `rockchip_drm_drv.h` already includes `<drm/drm_atomic_helper.h>` which pulls in `<drm/drm_crtc.h>` which includes `<drm/drm_connector.h>`. Check whether the explicit include is actually needed or just defensive.

2. **No validation that margins leave a minimum visible area.** The guard checks `m->left + m->right < hdisplay`, but this allows margins that leave only 1-2 visible pixels. A more robust check would enforce a reasonable minimum (though in practice the overscan property is capped at 100 and the `/200` division limits this).

3. **Asymmetry with the old code.** The old code used percentage-based margin calculations that affected both the active start position and the scaled size. The new code computes `hsize = hdisplay - m->left - m->right` which is equivalent when margins are symmetric (as patch 2 ensures), but the direct pixel subtraction means the scaler behavior is slightly different from what the old percentage formula would give for the same effective margin. This is fine since the old code always used 100% (no margin), just worth noting.

4. **Minor: the `tv_margins` field is zero-initialized by default** (since `rockchip_crtc_state` is allocated via `kzalloc` through `drm_atomic_helper_crtc_duplicate_state`), so the default behavior (no margins) is preserved. This is correct.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: drm/rockchip: dw_hdmi_qp: expose "overscan" property
  2026-06-02 17:00 ` [PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property Alexey Charkov
  2026-06-03  7:55   ` Maxime Ripard
  2026-06-03 13:11   ` Andy Yan
@ 2026-06-04  2:35   ` Claude Code Review Bot
  2 siblings, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-06-04  2:35 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Issues:**

1. **`drm_mode_create_tv_properties_legacy()` is deprecated** (critical). The function docstring says:

   > NOTE: This functions registers the deprecated "mode" connector property to select the analog TV mode (ie, NTSC, PAL, etc.). New drivers must use `drm_mode_create_tv_properties()` instead.

   Using the legacy function creates ~10 unnecessary TV properties (select subconnector, subconnector, brightness, contrast, flicker reduction, saturation, hue, mode, plus the margin properties). For an HDMI connector, most of these make no sense. Either use `drm_mode_create_tv_properties()` instead, or better yet, just create the overscan property directly if that's the only one needed. Check whether `drm_mode_create_tv_properties(dev, 0)` with no supported TV modes would be cleaner.

2. **Margin symmetry assumption.** The conversion in `atomic_check`:
   ```c
   s->tv_margins.left   = adj_mode->hdisplay * overscan / 200;
   s->tv_margins.right  = s->tv_margins.left;
   s->tv_margins.top    = adj_mode->vdisplay * overscan / 200;
   s->tv_margins.bottom = s->tv_margins.top;
   ```
   Applies equal margins on both sides, which is the expected behavior for the "overscan" property (a single percentage). This is correct, but the commit message could be clearer that this is intentionally symmetric. The `/200` divisor means `overscan=100` yields margins of 50% on each side, leaving zero visible area -- while `min(conn_state->tv.overscan, 100u)` allows this. In practice the property range is 0-100 and no user would set 100%, but the edge case exists.

3. **Integer rounding.** For small display sizes or small overscan percentages, `hdisplay * overscan / 200` may round to 0 even when the user requested some overscan. For example, with `hdisplay=640` and `overscan=1`, the result is `640/200 = 3` pixels per side. That actually works fine. But for `overscan=0` the result is correctly 0. This is acceptable.

4. **Missing `drm_connector_attach_tv_margin_properties()` or equivalent.** The patch attaches `tv_overscan_property` but does not attach the individual margin properties (`left margin`, `right margin`, etc.). This is intentional since the driver uses the single "overscan" knob rather than per-side margins. However, userspace compositors like KWin may expect the individual margin properties to also exist. Verify that KWin actually uses the "overscan" property as claimed (it does appear to, based on the KWin source).

5. **Property attached after connector init.** The property is attached in `dw_hdmi_qp_rockchip_bind()` after `drm_bridge_connector_init()` returns. This should work since property attachment is just adding a property reference to the connector object, but it's worth checking that no path reads connector properties before `bind()` returns.

6. **No `atomic_check` for overscan-only changes.** If the user changes overscan but nothing else changes on the connector/CRTC, the margin values are updated in the CRTC state during `atomic_check`, but there's no explicit check that forces a CRTC update. This likely works because changing any connector property triggers a full atomic commit, but it should be verified that `vop2_post_config()` is actually called in this scenario.

**Summary:** The series is a reasonable approach but should switch from `drm_mode_create_tv_properties_legacy()` to either `drm_mode_create_tv_properties()` or a more targeted property creation to avoid registering a pile of irrelevant analog TV properties on an HDMI connector.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-06-04  2:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-02 17:00 [PATCH 0/2] drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation Alexey Charkov
2026-06-02 17:00 ` [PATCH 1/2] drm/rockchip: vop2: honor TV margins from CRTC state for " Alexey Charkov
2026-06-04  2:35   ` Claude review: " Claude Code Review Bot
2026-06-02 17:00 ` [PATCH 2/2] drm/rockchip: dw_hdmi_qp: expose "overscan" property Alexey Charkov
2026-06-03  7:55   ` Maxime Ripard
2026-06-03  8:15     ` Alexey Charkov
2026-06-03 13:11   ` Andy Yan
2026-06-04  2:35   ` Claude review: " Claude Code Review Bot
2026-06-04  2:35 ` Claude review: drm/rockchip: dw_hdmi_qp: Add support for HDMI overscan compensation Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox