From patchwork Wed Jul 4 09:05:41 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Russell King (Oracle)" X-Patchwork-Id: 10509169 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E7A406028F for ; Thu, 5 Jul 2018 13:33:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D309029032 for ; Thu, 5 Jul 2018 13:33:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D11D629019; Thu, 5 Jul 2018 13:33:41 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 5153C29031 for ; Thu, 5 Jul 2018 13:33:41 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 27AF56ED7B; Thu, 5 Jul 2018 13:33:29 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [IPv6:2001:4d48:ad52:3201:214:fdff:fe10:1be6]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1E40C6EAFA for ; Wed, 4 Jul 2018 09:05:53 +0000 (UTC) Received: from n2100.armlinux.org.uk ([2001:4d48:ad52:3201:214:fdff:fe10:4f86]:34447) by pandora.armlinux.org.uk with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.90_1) (envelope-from ) id 1fadjP-0001sR-0J; Wed, 04 Jul 2018 10:05:47 +0100 Received: from linux by n2100.armlinux.org.uk with local (Exim 4.90_1) (envelope-from ) id 1fadjM-0005o4-1w; Wed, 04 Jul 2018 10:05:44 +0100 Date: Wed, 4 Jul 2018 10:05:41 +0100 From: Russell King - ARM Linux To: Daniel Vetter Subject: Re: DRM COLOR_RANGE property Message-ID: <20180704090541.GR17271@n2100.armlinux.org.uk> References: <20180703161857.GQ17271@n2100.armlinux.org.uk> <20180704082604.GK3891@phenom.ffwll.local> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180704082604.GK3891@phenom.ffwll.local> User-Agent: Mutt/1.5.23 (2014-03-12) X-Mailman-Approved-At: Thu, 05 Jul 2018 13:33:28 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Hans Verkuil , Jyri Sarha , dri-devel@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Jul 04, 2018 at 10:26:04AM +0200, Daniel Vetter wrote: > On Tue, Jul 03, 2018 at 05:18:57PM +0100, Russell King - ARM Linux wrote: > > Can someone provide a deeper explanation about exactly what this > > property represents please? > > > > Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB > > conversion (in other words, the range of values in the framebuffer), > > or the expected output range from the conversion? > > > > This matters, because a "limited", (iow, eg, BT601 compliant YCbCr) > > framebuffer where the Y signal is between 16..235 being displayed > > on a full-range RGB output would need different conversion from > > needing a limited-range RGB output. > > > > If it is indeed the output, then why is this a property of the plane? > > Is that not a property of: > > > > (a) whether the plane is being blended or overlaid onto a graphics > > plane which uses full-range RGB > > (b) the properties of the output(s) to which the plane is being > > displayed. > > > > IOW, it seems that the output of the CSC is more to do with what's > > downstream of the plane than with the plane itself. > > > > For example, take this situation: > > > > plane 0 - graphics, full range RGB > > >-- CRTC --> HDMI sink only supporting > > plane 1 - video, limited range YUV limited range RGB > > > > In order to display the graphics correctly in that scenario, the HDMI > > output needs to compress the RGB 0-255 range down to 16..236 to be > > compliant. If the video is limited range, and the CSC produces a > > limited range RGB output, then plane 1 gets its range further > > compressed at the HDMI output, which surely is undesirable. > > > > It would surely be better, if it's not possible to map the range of > > plane 0 to limited range, to instead expand the YUV range and then > > recompress it at the HDMI output to match the capabilities of the > > attached source. > > > > It also seems logical that describing the range of the RGB plane would > > also be sane - if the application is limiting graphics RGB to 16..235, > > then you'd want the CSC output to do the same and there'd be no need > > for any range expansion or compression. > > > > I'd personally like drm_plane_create_color_properties() to allow > > creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges > > being zero) until COLOR_RANGE is better defined than it is at present. > > > > Thoughts? > > > > I'm bringing this up, because the hardware I have has a CSC that > > accepts BT601 and BT709 formats, controlled by a single bit. Another > > bit controls whether the CSC produces 0..255 output or 16..235 output. > > That is then blended/overlaid with the graphics plane (0..255) and > > sent to the output. Having a "limited range" YUV plane produce > > 16..235 range output makes it look low-contrast compared to the > > graphics, which is what would be expected - "16" is not black > > compared to the black of the graphics in the same way that "235" is > > not white compared to the graphics. > > Drivers are supposed to automatically figure this out by looking at the > edid. In i915 we also allow userspace to override this with the "Broadcast > RGB" property on the connector. Unfortunately we haven't polished that > property yet (not sure what other drivers are doing tbh), so it's only > listed in the Documentation/gpu/kms-properties.csv graveyard :-/ In which case, I'd like to implement the COLOR_ENCODING property but not the COLOR_RANGE property until COLOR_RANGE is better defined. Unfortunately, at the moment the choices are to have either both properties or no properties - drm_plane_create_color_properties() doesn't support only creating the encoding property. Implementing it without it being well defined is a recipe for having a broken UAPI. So, I propose: 8<===== From: Russell King Subject: drm: allow COLOR_RANGE property to be optional The COLOR_RANGE plane property is not well defined - it doesn't define where in the colour conversion this control is applied. If it's an attribute of the data in the plane, then it has the reverse effect from if it's an attribute of the range of RGB output from the plane colour converter. Rather than being forced to implement this control when wanting the COLOR_ENCODING property, make it optional instead. --- drivers/gpu/drm/drm_color_mgmt.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index b97e2de2c029..fc397fcf80ab 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -426,9 +426,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane, (supported_encodings & BIT(default_encoding)) == 0)) return -EINVAL; - if (WARN_ON(supported_ranges == 0 || - (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 || - (supported_ranges & BIT(default_range)) == 0)) + if (WARN_ON(supported_ranges != 0 && + ((supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 || + (supported_ranges & BIT(default_range)) == 0))) return -EINVAL; len = 0; @@ -450,6 +450,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane, if (plane->state) plane->state->color_encoding = default_encoding; + if (supported_ranges == 0) + return 0; + len = 0; for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) { if ((supported_ranges & BIT(i)) == 0)