[PATCH v2 0/3] drm/amd/display: HDMI 1.4 3D output (frame packing, top-and-bottom, side-by-side)

[PATCH v2 0/3] drm/amd/display: HDMI 1.4 3D output (frame packing, top-and-bottom, side-by-side)

[Adrian Betschart]

amdgpu has never exposed the HDMI 1.4 3D modes that drm_edid derives
from a sink's HDMI VSDB: stereo_allowed is false on every connector, so
a 3D-capable TV or projector connected to a Radeon card cannot be driven
in frame-packing, top-and-bottom or side-by-side mode, although i915
has supported exactly this for a decade and the media players that
produce 3D frames (Kodi, mpv with 3D filters) only need the mode to
exist.

These three patches add it for native HDMI connectors, in the simplest form
that works on the hardware: the source packs both views into the frame
and the display core scans that frame out as ordinary 2D; the only
3D-specific output is the HDMI vendor infoframe. The reason it has to
be done this way is in patch 1: any of DC's stereo timing formats,
including the SW_PACKED ones, makes the hardware treat the surface as
two views and paint the whole frame into each half. Patch 2 adds the
3D_Ext_Data byte to the top-and-bottom vendor infoframe, without which
at least JVC D-ILA projectors do not engage 3D (Amlogic sources send
it, which is why those work with the same projector). Patch 3 sizes
the stream and the plane viewport by the doubled frame-packing timing
so each eye receives its own view rather than a stretched copy of the
first.

Tested on a Radeon RX 7600 (Navi 33, DCN 3.2.1) driving a JVC
DLA-RS4100 through an HDFury VRROOM, with Kodi (LibreELEC, GBM) as the
source, on a 7.2.3 kernel carrying these patches; the series here is
rebased onto amd-staging-drm-next and compile-tested there, and the
amdgpu_dm connector (308) and plane (95) KUnit suites pass; the freesync suite
crashes the same way on the unpatched base. All three layouts
engage the projector's 3D mode at 1920x1080p24, RGB 12 bpc, with
correct per-eye geometry (row-coded test frames read back through each
eye of shutter glasses) and correct eye assignment; frame packing is
also the format the projector uses for Blu-ray 3D.

Known limitation, not addressed here: a framebuffer with DCC enabled
does not scan out in a frame-packed mode on this GPU (the first flip
after the modeset never completes and the pipe wedges); the source has
to allocate the 3D framebuffer without DCC. A separate report will
follow once it is better understood.

Two unrelated FRL/DSC issues found on the same setup are tracked as
drm/amd issues 5770 and 5771.

v2 (all three findings came from the Sashiko review bot, and all held up):
- 1/3: a stereo mode keeps its own CRTC timing in
  decide_crtc_timing_for_drm_display_mode() - with scaling enabled, or
  when the sink's native mode is the 2D twin of the stereo mode's base
  timing, the native timing was copied over the doubled frame-packing
  timing; and amdgpu_dm_is_freesync_video_mode() rejects a
  stereo-flagged mode, which otherwise could be replaced by the 2D
  FreeSync base mode and lose its 3D flags. KUnit cases for both.
- 3/3: the plane KUnit mocks now set hdisplay/vdisplay as well as the
  crtc_* size, which drm_mode_get_hv_timing() derives the size from;
  dm_test_helper_check_state_scaling_caps failed otherwise.
- 2/3: unchanged.

Adrian Betschart (3):
  drm/amd/display: support HDMI 1.4 3D modes on HDMI connectors
  drm/amd/display: send the 3D_Ext_Data byte for top-and-bottom too
  drm/amd/display: size frame-packed streams by the doubled timing

 .../display/amdgpu_dm/amdgpu_dm_connector.c   | 65 +++++++++++++--
 .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 13 ++-
 .../tests/amdgpu_dm_connector_test.c          | 80 +++++++++++++++++++
 .../amdgpu_dm/tests/amdgpu_dm_plane_test.c    | 12 +++
 drivers/gpu/drm/amd/display/dc/dc_stream.h    |  7 ++
 .../display/modules/info_packet/info_packet.c |  4 +
 6 files changed, 171 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] drm/amd/display: support HDMI 1.4 3D modes on HDMI connectors

[Adrian Betschart]

Allow the stereo modes drm_edid derives from a sink's HDMI VSDB on
HDMI connectors, and drive them the way a 2D stream is driven: the
source (compositor or media player) lays both views out in the frame
itself - side by side, top and bottom, or the doubled frame-packing
timing with the 45-line active space between the eyes - and the only
3D-specific output is the HDMI vendor infoframe that tells the sink
how the frame is laid out.

Keep the DC timing at TIMING_3D_FORMAT_NONE for all of them. Any DC
stereo timing format, including the SW_PACKED variants, makes the
hardware treat the surface as two views: the pipe is split and both
view addresses point at the same surface, so the whole frame ends up
in each half of the output. The new vsif_3d_format stream field
carries the layout to mod_build_hf_vsif_infopacket() instead, so the
VSIF is right and stays right when the freesync code rebuilds it for
ALLM.

Frame packing needs the doubled CRTC timing; pass CRTC_STEREO_DOUBLE
wherever amdgpu_dm recomputes the CRTC fields itself.

Limit stereo_allowed to native HDMI connectors: on the DP-to-HDMI
converter paths link validation rejects the 3D timings.

Tested on a Radeon RX 7600 (DCN 3.2.1) driving a JVC DLA-RS4100
projector through an HDFury VRROOM, with Kodi rendering the packed
frames: frame packing, top-and-bottom and side-by-side at
1920x1080p24, RGB 12 bpc, all engage the projector's 3D mode with
correct per-eye geometry and eye assignment (checked with per-eye
test patterns through shutter glasses).

Two paths in create_stream_for_sink() would otherwise replace what the
mode carries: decide_crtc_timing_for_drm_display_mode() copies the
native CRTC timing over a mode with matching clock and totals or when
scaling is on, which for frame packing is the doubled timing, and
amdgpu_dm_is_freesync_video_mode() can match a stereo mode whose base
timing equals the FreeSync base mode and swap in the 2D mode, dropping
the 3D flags. A stereo mode keeps its own CRTC timing and is never a
FreeSync video mode; KUnit cases cover both.

Signed-off-by: Adrian Betschart <adrian.betschart@cinemaone.ch>
---
 .../display/amdgpu_dm/amdgpu_dm_connector.c   | 54 ++++++++++++-
 .../tests/amdgpu_dm_connector_test.c          | 80 +++++++++++++++++++
 drivers/gpu/drm/amd/display/dc/dc_stream.h    |  7 ++
 .../display/modules/info_packet/info_packet.c |  3 +
 4 files changed, 141 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
index 0e71ba498..aa3f47632 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
@@ -851,6 +851,25 @@ STATIC_IFN_KUNIT bool adjust_colour_depth_from_display_info(
 }
 EXPORT_IF_KUNIT(adjust_colour_depth_from_display_info);
 
+/*
+ * 3D layout to announce in the HDMI vendor infoframe for a DRM 3D mode. The
+ * stream timing itself stays 2D (see fill_stream_properties_from_drm_display_mode).
+ */
+static enum dc_timing_3d_format amdgpu_dm_vsif_3d_format(unsigned int mode_flags)
+{
+	switch (mode_flags & DRM_MODE_FLAG_3D_MASK) {
+	case DRM_MODE_FLAG_3D_FRAME_PACKING:
+		return TIMING_3D_FORMAT_SW_FRAME_PACKING;
+	case DRM_MODE_FLAG_3D_TOP_AND_BOTTOM:
+		return TIMING_3D_FORMAT_TB_SW_PACKED;
+	case DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF:
+	case DRM_MODE_FLAG_3D_SIDE_BY_SIDE_FULL:
+		return TIMING_3D_FORMAT_SBS_SW_PACKED;
+	default:
+		return TIMING_3D_FORMAT_NONE;
+	}
+}
+
 STATIC_IFN_KUNIT void fill_stream_properties_from_drm_display_mode(
 	struct dc_stream_state *stream,
 	const struct drm_display_mode *mode_in,
@@ -887,7 +906,15 @@ STATIC_IFN_KUNIT void fill_stream_properties_from_drm_display_mode(
 	 */
 	timing_out->pixel_encoding = requested_encoding;
 
+	/*
+	 * The source packs both views into the frame itself (side-by-side,
+	 * top-and-bottom, or the doubled frame-packing timing), so the display
+	 * core scans it out as a plain 2D stream and only the HDMI vendor
+	 * infoframe tells the sink how the frame is laid out. Any DC stereo
+	 * timing format would make the hardware treat the surface as two views.
+	 */
 	timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
+	stream->vsif_3d_format = amdgpu_dm_vsif_3d_format(mode_in->flags);
 	timing_out->display_color_depth = amdgpu_dm_convert_color_depth_from_display_info(
 		connector,
 		(timing_out->pixel_encoding == PIXEL_ENCODING_YCBCR420),
@@ -992,6 +1019,15 @@ decide_crtc_timing_for_drm_display_mode(struct drm_display_mode *drm_mode,
 					const struct drm_display_mode *native_mode,
 					bool scale_enabled)
 {
+	/*
+	 * A stereo mode has to go out at its own timing: frame packing's CRTC
+	 * timing is the doubled one, and a side-by-side or top-and-bottom mode
+	 * scaled to the native timing would announce a layout the sink cannot
+	 * pair with what it receives.
+	 */
+	if (drm_mode->flags & DRM_MODE_FLAG_3D_MASK)
+		return;
+
 	if (scale_enabled || (
 	    native_mode->clock == drm_mode->clock &&
 	    native_mode->htotal == drm_mode->htotal &&
@@ -1120,6 +1156,10 @@ bool amdgpu_dm_is_freesync_video_mode(const struct drm_display_mode *mode,
 	if (!high_mode || !mode)
 		return false;
 
+	/* a stereo mode is never one of the inserted FreeSync video modes */
+	if (mode->flags & DRM_MODE_FLAG_3D_MASK)
+		return false;
+
 	timing_diff = high_mode->vtotal - mode->vtotal;
 
 	if (high_mode->clock == 0 || high_mode->clock != mode->clock ||
@@ -1451,6 +1491,9 @@ create_stream_for_sink(struct drm_connector *connector,
 	struct dc_sink *sink = NULL;
 
 	drm_mode_init(&mode, drm_mode);
+	/* frame packing scans out both views plus the active space in one frame */
+	if (mode.flags & DRM_MODE_FLAG_3D_FRAME_PACKING)
+		drm_mode_set_crtcinfo(&mode, CRTC_STEREO_DOUBLE);
 	memset(&saved_mode, 0, sizeof(saved_mode));
 
 	if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) {
@@ -1530,7 +1573,9 @@ create_stream_for_sink(struct drm_connector *connector,
 	}
 
 	if (recalculate_timing)
-		drm_mode_set_crtcinfo(&saved_mode, 0);
+		drm_mode_set_crtcinfo(&saved_mode,
+				      (saved_mode.flags & DRM_MODE_FLAG_3D_FRAME_PACKING) ?
+				      CRTC_STEREO_DOUBLE : 0);
 
 	/*
 	 * If scaling is enabled and refresh rate didn't change
@@ -2449,7 +2494,9 @@ enum drm_mode_status amdgpu_dm_connector_mode_valid(struct drm_connector *connec
 	if (!test_mode)
 		goto fail;
 
-	drm_mode_set_crtcinfo(test_mode, 0);
+	drm_mode_set_crtcinfo(test_mode,
+			      (test_mode->flags & DRM_MODE_FLAG_3D_FRAME_PACKING) ?
+			      CRTC_STEREO_DOUBLE : 0);
 
 	stream = amdgpu_dm_create_validate_stream_for_sink(connector, test_mode,
 						 to_dm_connector_state(connector->state),
@@ -3143,7 +3190,8 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 	aconnector->dc_link = link;
 	aconnector->base.interlace_allowed = false;
 	aconnector->base.doublescan_allowed = false;
-	aconnector->base.stereo_allowed = false;
+	/* HDMI 1.4 3D only; DP-to-HDMI converters reject the timings in link validation */
+	aconnector->base.stereo_allowed = connector_type == DRM_MODE_CONNECTOR_HDMIA;
 	aconnector->base.dpms = DRM_MODE_DPMS_OFF;
 	aconnector->hpd.hpd = AMDGPU_HPD_NONE; /* not used */
 	aconnector->audio_inst = -1;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
index 76334fb84..396e91013 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
@@ -1430,6 +1430,46 @@ static void dm_test_decide_crtc_timing_no_crtc_clock(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, drm_mode.crtc_hdisplay, 0);
 }
 
+/**
+ * dm_test_decide_crtc_timing_keeps_stereo_mode - A stereo mode keeps its own CRTC timing
+ * @test: The KUnit test context
+ */
+static void dm_test_decide_crtc_timing_keeps_stereo_mode(struct kunit *test)
+{
+	struct drm_display_mode drm_mode = {};
+	struct drm_display_mode native_mode = {};
+
+	/* The native mode is the 2D twin of the frame-packed one: same base timing ... */
+	native_mode.clock = 74250;
+	native_mode.htotal = 2750;
+	native_mode.vtotal = 1125;
+	native_mode.crtc_clock = 74250;
+	native_mode.crtc_hdisplay = 1920;
+	native_mode.crtc_vdisplay = 1080;
+	native_mode.crtc_htotal = 2750;
+	native_mode.crtc_vtotal = 1125;
+
+	/* ... so it matches, but the stereo mode's CRTC timing is the doubled one and must stay */
+	drm_mode.clock = 74250;
+	drm_mode.hdisplay = 1920;
+	drm_mode.vdisplay = 1080;
+	drm_mode.hsync_start = 2558;
+	drm_mode.hsync_end = 2602;
+	drm_mode.htotal = 2750;
+	drm_mode.vsync_start = 1084;
+	drm_mode.vsync_end = 1089;
+	drm_mode.vtotal = 1125;
+	drm_mode.flags = DRM_MODE_FLAG_3D_FRAME_PACKING;
+	drm_mode_set_crtcinfo(&drm_mode, CRTC_STEREO_DOUBLE);
+	KUNIT_ASSERT_EQ(test, drm_mode.crtc_vdisplay, 2205);
+
+	decide_crtc_timing_for_drm_display_mode(&drm_mode, &native_mode, false);
+
+	KUNIT_EXPECT_EQ(test, drm_mode.crtc_vdisplay, 2205);
+	KUNIT_EXPECT_EQ(test, drm_mode.crtc_vtotal, 2250);
+	KUNIT_EXPECT_EQ(test, drm_mode.crtc_clock, 148500);
+}
+
 /* Tests for amdgpu_dm_connector_funcs_reset() */
 
 static const struct drm_connector_funcs dm_test_connector_funcs = {
@@ -2293,6 +2333,44 @@ static void dm_test_is_freesync_video_mode_match(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, amdgpu_dm_is_freesync_video_mode(&candidate, aconnector));
 }
 
+/**
+ * dm_test_is_freesync_video_mode_rejects_stereo - A stereo mode with matching timing returns false
+ * @test: The KUnit test context
+ */
+static void dm_test_is_freesync_video_mode_rejects_stereo(struct kunit *test)
+{
+	struct amdgpu_dm_connector *aconnector;
+	struct drm_display_mode candidate = {};
+
+	aconnector = kunit_kzalloc(test, sizeof(*aconnector), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, aconnector);
+
+	/* Cached high mode acts as reference */
+	aconnector->base.connector_type = DRM_MODE_CONNECTOR_HDMIA;
+	aconnector->freesync_vid_base.clock = 148500;
+	aconnector->freesync_vid_base.hdisplay = 1920;
+	aconnector->freesync_vid_base.vdisplay = 1080;
+	aconnector->freesync_vid_base.hsync_start = 2008;
+	aconnector->freesync_vid_base.hsync_end = 2052;
+	aconnector->freesync_vid_base.htotal = 2200;
+	aconnector->freesync_vid_base.vsync_start = 1084;
+	aconnector->freesync_vid_base.vsync_end = 1089;
+	aconnector->freesync_vid_base.vtotal = 1125;
+
+	candidate.clock = 148500;
+	candidate.hdisplay = 1920;
+	candidate.vdisplay = 1080;
+	candidate.hsync_start = 2008;
+	candidate.hsync_end = 2052;
+	candidate.htotal = 2200;
+	candidate.vsync_start = 1084;
+	candidate.vsync_end = 1089;
+	candidate.vtotal = 1125;
+	candidate.flags = DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
+
+	KUNIT_EXPECT_FALSE(test, amdgpu_dm_is_freesync_video_mode(&candidate, aconnector));
+}
+
 /**
  * dm_test_is_freesync_video_mode_no_match - Test mismatched mode returns false
  * @test: The KUnit test context
@@ -8468,6 +8546,7 @@ static struct kunit_case amdgpu_dm_connector_tests[] = {
 	KUNIT_CASE(dm_test_decide_crtc_timing_matching_mode),
 	KUNIT_CASE(dm_test_decide_crtc_timing_no_copy),
 	KUNIT_CASE(dm_test_decide_crtc_timing_no_crtc_clock),
+	KUNIT_CASE(dm_test_decide_crtc_timing_keeps_stereo_mode),
 	/* amdgpu_dm_connector_funcs_reset */
 	KUNIT_CASE(dm_test_funcs_reset_sets_defaults),
 	KUNIT_CASE(dm_test_funcs_reset_edp_abm_level),
@@ -8521,6 +8600,7 @@ static struct kunit_case amdgpu_dm_connector_tests[] = {
 	KUNIT_CASE(dm_test_is_freesync_video_mode_null_mode),
 	KUNIT_CASE(dm_test_is_freesync_video_mode_match),
 	KUNIT_CASE(dm_test_is_freesync_video_mode_no_match),
+	KUNIT_CASE(dm_test_is_freesync_video_mode_rejects_stereo),
 	/* update_subconnector_property */
 	KUNIT_CASE(dm_test_update_subconnector_dp_with_sink),
 	KUNIT_CASE(dm_test_update_subconnector_dp_no_sink),
diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
index 934ae381e..e6fd75b87 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
@@ -235,6 +235,13 @@ struct dc_stream_state {
 
 	enum view_3d_format view_format;
 
+	/**
+	 * @vsif_3d_format: 3D layout announced in the HDMI vendor infoframe when
+	 * the source packs both views into the frame itself and the timing is
+	 * therefore left at TIMING_3D_FORMAT_NONE.
+	 */
+	enum dc_timing_3d_format vsif_3d_format;
+
 	bool use_vsc_sdp_for_colorimetry;
 	bool ignore_msa_timing_param;
 
diff --git a/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c b/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
index 32b697f46..13478b8bd 100644
--- a/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
+++ b/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
@@ -536,6 +536,9 @@ void mod_build_hf_vsif_infopacket(const struct dc_stream_state *stream,
 		format = stream->timing.timing_3d_format;
 		if (stream->view_format == VIEW_3D_FORMAT_NONE)
 			format = TIMING_3D_FORMAT_NONE;
+		/* a 2D scanout of a frame the source packed itself */
+		if (format == TIMING_3D_FORMAT_NONE)
+			format = stream->vsif_3d_format;
 
 		if (stream->timing.hdmi_vic != 0
 				&& stream->timing.h_total >= 3840
-- 
2.43.0

[PATCH v2 2/3] drm/amd/display: send the 3D_Ext_Data byte for top-and-bottom too

[Adrian Betschart]

HDMI 1.4b only requires the 3D_Ext_Data byte for the side-by-side
(half) structure, and the vendor infoframe for top-and-bottom is
built with a 5-byte payload accordingly. At least one sink family
ignores a top-and-bottom VSIF without the sixth byte: a JVC DLA-RS4100
(D-ILA, "ILAFPJ--X214A") stays in 2D on the 5-byte frame and engages
its 3D mode the moment the byte is present. Amlogic sources send the
6-byte form for top-and-bottom, which is why the same projector works
with them.

Send the extra byte, zero, for top-and-bottom as well. Sinks that do
not need it ignore a zero 3D_Ext_Data field.

Signed-off-by: Adrian Betschart <adrian.betschart@cinemaone.ch>
---
 drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c b/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
index 13478b8bd..3e41b33ea 100644
--- a/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
+++ b/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
@@ -574,6 +574,7 @@ void mod_build_hf_vsif_infopacket(const struct dc_stream_state *stream,
 			case TIMING_3D_FORMAT_TOP_AND_BOTTOM:
 			case TIMING_3D_FORMAT_TB_SW_PACKED:
 				info_packet->sb[5] = (0x6 << 4);
+				length = 6;
 				break;
 
 			default:
-- 
2.43.0

[PATCH v2 3/3] drm/amd/display: size frame-packed streams by the doubled timing

[Adrian Betschart]

A frame-packed 3D mode scans out both views and the active space
between them in one frame, so the CRTC is 2205 lines tall for a
1080p mode. The stream scaling code took the source height from
mode->vdisplay (1080) against the 2205-line addressable destination;
with aspect scaling that keeps 1080 lines and centres them, so the
first view is stretched across both eye windows and the second view
is never shown. The plane viewport check clipped planes to
crtc_vdisplay for the same reason.

Use drm_mode_get_hv_timing(), which returns the doubled height for
stereo modes and the plain display size otherwise, in both places.

Measured on a Radeon RX 7600 with a frame-packed 1920x1080p24 mode:
before, a row-coded 2205-line test frame showed rows of the first
view in both eyes; after, each eye receives its own view.

The plane KUnit tests mocked only the CRTC-adjusted size; set the mode size
they now derive it from as well.

Signed-off-by: Adrian Betschart <adrian.betschart@cinemaone.ch>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c | 11 ++++++++---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c | 13 +++++++++----
 .../display/amdgpu_dm/tests/amdgpu_dm_plane_test.c  | 12 ++++++++++++
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
index aa3f47632..2d35060d7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
@@ -1418,9 +1418,14 @@ void amdgpu_dm_update_stream_scaling_settings(struct drm_device *dev,
 	if (!mode)
 		return;
 
-	/* Full screen scaling by default */
-	src.width = mode->hdisplay;
-	src.height = mode->vdisplay;
+	/*
+	 * Full screen scaling by default. A frame-packed 3D mode scans out the
+	 * doubled timing, so the source size is the CRTC size, not vdisplay:
+	 * with 1080 lines against a 2205-line destination the aspect fit would
+	 * keep 1080 lines and centre them, putting the first view across both
+	 * eye windows and none of the second.
+	 */
+	drm_mode_get_hv_timing(mode, &src.width, &src.height);
 	dst.width = stream->timing.h_addressable;
 	dst.height = stream->timing.v_addressable;
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
index 0a5a73472..8db656ef8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
@@ -1407,16 +1407,21 @@ int amdgpu_dm_plane_helper_check_state(struct drm_plane_state *state,
 		if (state->plane->type != DRM_PLANE_TYPE_CURSOR) {
 			int viewport_width = state->crtc_w;
 			int viewport_height = state->crtc_h;
+			int mode_hdisplay, mode_vdisplay;
+
+			/* frame-packed 3D scans out the doubled timing */
+			drm_mode_get_hv_timing(&new_crtc_state->mode,
+					       &mode_hdisplay, &mode_vdisplay);
 
 			if (state->crtc_x < 0)
 				viewport_width += state->crtc_x;
-			else if (state->crtc_x + state->crtc_w > new_crtc_state->mode.crtc_hdisplay)
-				viewport_width = new_crtc_state->mode.crtc_hdisplay - state->crtc_x;
+			else if (state->crtc_x + state->crtc_w > mode_hdisplay)
+				viewport_width = mode_hdisplay - state->crtc_x;
 
 			if (state->crtc_y < 0)
 				viewport_height += state->crtc_y;
-			else if (state->crtc_y + state->crtc_h > new_crtc_state->mode.crtc_vdisplay)
-				viewport_height = new_crtc_state->mode.crtc_vdisplay - state->crtc_y;
+			else if (state->crtc_y + state->crtc_h > mode_vdisplay)
+				viewport_height = mode_vdisplay - state->crtc_y;
 
 			if (viewport_width < 0 || viewport_height < 0) {
 				DRM_DEBUG_ATOMIC("Plane completely outside of screen\n");
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_plane_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_plane_test.c
index 1b39f41c8..80a952d47 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_plane_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_plane_test.c
@@ -642,6 +642,8 @@ static void dm_test_helper_check_state_viewport_reject(struct kunit *test)
 	state->crtc_y = 0;
 	state->crtc_w = 100;
 	state->crtc_h = 100;
+	new_crtc_state->mode.hdisplay = 100;
+	new_crtc_state->mode.vdisplay = 100;
 	new_crtc_state->mode.crtc_hdisplay = 100;
 	new_crtc_state->mode.crtc_vdisplay = 100;
 
@@ -2286,6 +2288,8 @@ static void dm_test_helper_check_state_small_viewport_width(struct kunit *test)
 	state->crtc_y = 0;
 	state->crtc_w = 10;
 	state->crtc_h = 100;
+	new_crtc_state->mode.hdisplay = 1920;
+	new_crtc_state->mode.vdisplay = 1080;
 	new_crtc_state->mode.crtc_hdisplay = 1920;
 	new_crtc_state->mode.crtc_vdisplay = 1080;
 
@@ -2327,6 +2331,8 @@ static void dm_test_helper_check_state_small_viewport_height(struct kunit *test)
 	state->crtc_y = -95;
 	state->crtc_w = 100;
 	state->crtc_h = 100;
+	new_crtc_state->mode.hdisplay = 1920;
+	new_crtc_state->mode.vdisplay = 1080;
 	new_crtc_state->mode.crtc_hdisplay = 1920;
 	new_crtc_state->mode.crtc_vdisplay = 1080;
 
@@ -2369,6 +2375,8 @@ static void dm_test_helper_check_state_bottom_clipped_height(struct kunit *test)
 	state->crtc_y = 95;
 	state->crtc_w = 100;
 	state->crtc_h = 100;
+	new_crtc_state->mode.hdisplay = 1920;
+	new_crtc_state->mode.vdisplay = 100;
 	new_crtc_state->mode.crtc_hdisplay = 1920;
 	new_crtc_state->mode.crtc_vdisplay = 100;
 
@@ -2429,6 +2437,8 @@ static void dm_test_helper_check_state_scaling_caps(struct kunit *test)
 	state->crtc_w = 200;
 	state->crtc_h = 200;
 	new_crtc_state->crtc = crtc;
+	new_crtc_state->mode.hdisplay = 1920;
+	new_crtc_state->mode.vdisplay = 1080;
 	new_crtc_state->mode.crtc_hdisplay = 1920;
 	new_crtc_state->mode.crtc_vdisplay = 1080;
 
@@ -2738,6 +2748,8 @@ static struct amdgpu_device *dm_test_init_atomic_check_state(struct kunit *test,
 
 	(*new_crtc_state)->crtc = crtc;
 	(*new_crtc_state)->enable = true;
+	(*new_crtc_state)->mode.hdisplay = 1920;
+	(*new_crtc_state)->mode.vdisplay = 1080;
 	(*new_crtc_state)->mode.crtc_hdisplay = 1920;
 	(*new_crtc_state)->mode.crtc_vdisplay = 1080;
 
-- 
2.43.0

Re: [PATCH v2 1/3] drm/amd/display: support HDMI 1.4 3D modes on HDMI connectors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Bitwise AND operator is incorrectly used on an enum value, causing false positives for other 3D formats.
- [Low] Ternary check for 3D frame packing on saved_mode is logically dead code and modifies an unused variable.
--

commit 69c7b6a13ec338ae92b4b6e911a4bfd520258818
Author: Adrian Betschart <adrian.betschart@cinemaone.ch>

drm/amd/display: support HDMI 1.4 3D modes on HDMI connectors

Allow the stereo modes drm_edid derives from a sink's HDMI VSDB on
HDMI connectors, and drive them the way a 2D stream is driven. The
source lays both views out in the frame itself, and the HDMI vendor
infoframe tells the sink how the frame is laid out.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> index 0e71ba498e8aa..aa3f47632de06 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c

[ ... ]

> @@ -1451,6 +1491,9 @@ create_stream_for_sink(struct drm_connector *connector,
>  	struct dc_sink *sink = NULL;
>  
>  	drm_mode_init(&mode, drm_mode);
> +	/* frame packing scans out both views plus the active space in one frame */
> +	if (mode.flags & DRM_MODE_FLAG_3D_FRAME_PACKING)
> +		drm_mode_set_crtcinfo(&mode, CRTC_STEREO_DOUBLE);

[Severity: Low]
Does this bitwise AND incorrectly match other 3D formats? The 3D flags share
bits in the DRM_MODE_FLAG_3D_MASK. For example, DRM_MODE_FLAG_3D_TOP_AND_BOTTOM
is 7<<14 and DRM_MODE_FLAG_3D_FRAME_PACKING is 1<<14, so this evaluates to true
for Top-and-Bottom modes.

Even though drm_mode_set_crtcinfo() filters this internally via a switch
statement, should this use an exact match check like:
(mode.flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING?

>  	memset(&saved_mode, 0, sizeof(saved_mode));
>  
>  	if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) {

[ ... ]

> @@ -1530,7 +1573,9 @@ create_stream_for_sink(struct drm_connector *connector,
>  	}
>  
>  	if (recalculate_timing)
> -		drm_mode_set_crtcinfo(&saved_mode, 0);
> +		drm_mode_set_crtcinfo(&saved_mode,
> +				      (saved_mode.flags & DRM_MODE_FLAG_3D_FRAME_PACKING) ?
> +				      CRTC_STEREO_DOUBLE : 0);

[Severity: Low]
Is this ternary check for 3D frame packing on saved_mode reachable?

The recalculate_timing variable is evaluated to true only when
amdgpu_dm_is_freesync_video_mode() returns true, but that function was
modified in this patch to explicitly reject any mode with 3D flags.

Since saved_mode will never contain a 3D flag when recalculate_timing is
true, and saved_mode is modified here but never consumed again, could this
dead code be simplified?

>  
>  	/*
>  	 * If scaling is enabled and refresh rate didn't change

[ ... ]

> @@ -2449,7 +2494,9 @@ enum drm_mode_status amdgpu_dm_connector_mode_valid(struct drm_connector *connec
>  	if (!test_mode)
>  		goto fail;
>  
> -	drm_mode_set_crtcinfo(test_mode, 0);
> +	drm_mode_set_crtcinfo(test_mode,
> +			      (test_mode->flags & DRM_MODE_FLAG_3D_FRAME_PACKING) ?
> +			      CRTC_STEREO_DOUBLE : 0);

[Severity: Low]
Similar to the check in create_stream_for_sink(), does this bitwise AND
incorrectly pass CRTC_STEREO_DOUBLE for non-frame-packing 3D modes like
Top-and-Bottom due to the overlapping mask values?

>  
>  	stream = amdgpu_dm_create_validate_stream_for_sink(connector, test_mode,
>  						 to_dm_connector_state(connector->state),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907115229.64477-1-adrian.betschart@cinemaone.ch?part=1