Received-SPF: pass (google.com: domain of linux-kernel+bounces-48413-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org>
 <20231207-kms-hdmi-connector-state-v5-15-6538e19d634d@kernel.org>
 <CAPY8ntBQ+qY9441-rMzq_JAoYAaY_r+E-ADv7Wry0tJNTzKpwg@mail.gmail.com> <jlkoofv7nszj2uqmo2672yo4wjd3yjqarge2l2hxofixcchu6a@j72pa4iybitd>
In-Reply-To: <jlkoofv7nszj2uqmo2672yo4wjd3yjqarge2l2hxofixcchu6a@j72pa4iybitd>
From: Dave Stevenson <dave.stevenson@raspberrypi.com>
Date: Thu, 1 Feb 2024 15:33:24 +0000
Message-ID: <CAPY8ntAmXyKtebMeM7rLtgRR+QwL1H+UCwSO=bLBQN4TsgwsDg@mail.gmail.com>
Subject: Re: Re: [PATCH v5 15/44] drm/connector: hdmi: Compute bpc and format automatically
To: Maxime Ripard <mripard@kernel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, 
	Emma Anholt <emma@anholt.net>, Jonathan Corbet <corbet@lwn.net>, Sandy Huang <hjc@rock-chips.com>, 
	=?UTF-8?Q?Heiko_St=C3=BCbner?= <heiko@sntech.de>, 
	Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, 
	Samuel Holland <samuel@sholland.org>, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, 
	Hans Verkuil <hverkuil@xs4all.nl>, linux-rockchip@lists.infradead.org, 
	linux-sunxi@lists.linux.dev, linux-arm-kernel@lists.infradead.org, 
	linux-media@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Hi Maxime

On Thu, 1 Feb 2024 at 12:51, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Thu, Dec 14, 2023 at 03:10:43PM +0000, Dave Stevenson wrote:
> > > +static bool
> > > +sink_supports_format_bpc(const struct drm_connector *connector,
> > > +                        const struct drm_display_info *info,
> > > +                        const struct drm_display_mode *mode,
> > > +                        unsigned int format, unsigned int bpc)
> > > +{
> > > +       struct drm_device *dev = connector->dev;
> > > +       u8 vic = drm_match_cea_mode(mode);
> > > +
> > > +       if (vic == 1 && bpc != 8) {
> > > +               drm_dbg(dev, "VIC1 requires a bpc of 8, got %u\n", bpc);
> > > +               return false;
> > > +       }
> > > +
> > > +       if (!info->is_hdmi &&
> > > +           (format != HDMI_COLORSPACE_RGB || bpc != 8)) {
> > > +               drm_dbg(dev, "DVI Monitors require an RGB output at 8 bpc\n");
> > > +               return false;
> > > +       }
> > > +
> > > +       if (!(connector->hdmi.supported_formats & BIT(format))) {
> > > +               drm_dbg(dev, "%s format unsupported by the connector.\n",
> > > +                       drm_hdmi_connector_get_output_format_name(format));
> > > +               return false;
> > > +       }
> > > +
> > > +       switch (format) {
> > > +       case HDMI_COLORSPACE_RGB:
> > > +               drm_dbg(dev, "RGB Format, checking the constraints.\n");
> > > +
> > > +               if (!(info->color_formats & DRM_COLOR_FORMAT_RGB444))
> > > +                       return false;
> >
> > We've dropped this check from vc4 in our downstream kernel as it stops
> > you using the prebaked EDIDs (eg drm.edid_firmware=edid/1024x768.bin),
> > or any other EDID that is defined as an analog monitor.
> > The EDID parsing bombs out at [1], so info->color_formats gets left at 0.
>
> Right, but it only does so if the display isn't defined as a digital display...
>
> > RGB is mandatory for both DVI and HDMI, so rejecting it seems overly fussy.
>
> ... which is required for both DVI and HDMI.
>
> And sure enough, if we decode that EDID:
>
> edid-decode (hex):
>
> 00 ff ff ff ff ff ff 00 31 d8 00 00 00 00 00 00
> 05 16 01 03 6d 23 1a 78 ea 5e c0 a4 59 4a 98 25
> 20 50 54 00 08 00 61 40 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 64 19 00 40 41 00 26 30 08 90
> 36 00 63 0a 11 00 00 18 00 00 00 ff 00 4c 69 6e
> 75 78 20 23 30 0a 20 20 20 20 00 00 00 fd 00 3b
> 3d 2f 31 07 00 0a 20 20 20 20 20 20 00 00 00 fc
> 00 4c 69 6e 75 78 20 58 47 41 0a 20 20 20 00 55
>
> ----------------
>
> Block 0, Base EDID:
>   EDID Structure Version & Revision: 1.3
>   Vendor & Product Identification:
>     Manufacturer: LNX
>     Model: 0
>     Made in: week 5 of 2012
>   Basic Display Parameters & Features:
>     Analog display
>     Signal Level Standard: 0.700 : 0.000 : 0.700 V p-p
>     Blank level equals black level
>     Sync: Separate Composite Serration
>     Maximum image size: 35 cm x 26 cm
>     Gamma: 2.20
>     DPMS levels: Standby Suspend Off
>     RGB color display
>     First detailed timing is the preferred timing
>   Color Characteristics:
>     Red  : 0.6416, 0.3486
>     Green: 0.2919, 0.5957
>     Blue : 0.1474, 0.1250
>     White: 0.3125, 0.3281
>   Established Timings I & II:
>     DMT 0x10:  1024x768    60.003840 Hz   4:3     48.363 kHz     65.000000 MHz
>   Standard Timings:
>     DMT 0x10:  1024x768    60.003840 Hz   4:3     48.363 kHz     65.000000 MHz
>   Detailed Timing Descriptors:
>     DTD 1:  1024x768    60.003840 Hz   4:3     48.363 kHz     65.000000 MHz (355 mm x 266 mm)
>                  Hfront    8 Hsync 144 Hback  168 Hpol N
>                  Vfront    3 Vsync   6 Vback   29 Vpol N
>     Display Product Serial Number: 'Linux #0'
>     Display Range Limits:
>       Monitor ranges (GTF): 59-61 Hz V, 47-49 kHz H, max dotclock 70 MHz
>     Display Product Name: 'Linux XGA'
> Checksum: 0x55
>
> ----------------
>
> Warnings:
>
> Block 0, Base EDID:
>   Detailed Timing Descriptor #1: DTD is similar but not identical to DMT 0x10.
>
> EDID conformity: PASS
>
> So, if anything, it's the EDID that needs to be updated, not the code there.

So are these EDIDs only valid for VGA outputs and another set needs to
be added for HDMI monitors?

Having drm.edid_firmware=edid/1024x768.bin works on an HDMI connector
prior to this patch, so presumably drm_edid_loader needs to
automatically switch between the existing (analog) and new (digital)
EDIDs based on the connector type? Or are you requiring users to
change the strings they use?

Cheers.
  Dave