From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C1FF9CD11C2 for ; Fri, 5 Apr 2024 18:20:36 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 1D9DF10E68E; Fri, 5 Apr 2024 18:20:36 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="QvCPtlV3"; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by gabe.freedesktop.org (Postfix) with ESMTPS id EDA0910E68E for ; Fri, 5 Apr 2024 18:20:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1712341231; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=bO5tGPdIThGAHP1dCmNTDrQ7a3/NQfNevy8egofPhNg=; b=QvCPtlV3HpvWSY1AkBqkpaNwUKD6HfWzscgCcP+ehBdzI0usWGmQInkySzl9/2m9drO0wb iRsTPPZF21OPnslndNmYqNU8Is7Qw7oNL+UgK0AX+WsDsJJaYG7rqRmahmsQfbGx+b8UiB EA0q/mzPsfb74v1sJ2gyN6xTNixf9pE= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-18-4eo5Y2SgPn-RdxS4qMatzg-1; Fri, 05 Apr 2024 14:20:30 -0400 X-MC-Unique: 4eo5Y2SgPn-RdxS4qMatzg-1 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-343d6732721so705620f8f.2 for ; Fri, 05 Apr 2024 11:20:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712341228; x=1712946028; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=bO5tGPdIThGAHP1dCmNTDrQ7a3/NQfNevy8egofPhNg=; b=dyEwOVnEv1TXtAU6vK1eKrklUYewLATxr1TsERJTjMURBKIyfZTbDVa1fva4MRAX6r h7qBwt32fSAFoF4Y3cQmXSab7kLfF9HPuOfk1GfkEC4+KtsmDSxVrnr9dnH905GYrKSB jnUGQrngKZ1BCRuts/Ii4atkKsCy79LoJbIo3BRftOWuSs0Cc+zCLHRNB/EgMaU+fJSF vYRKysWSY56EnrNjJgxwISbIRNu8mxCz4Hv1wCgyBTEAq+ym3TWHMlKANdCjMGyyUCB6 O/y5Pz/UDtoViU6UH9jRO1dP33yLcM7jVCggUXJSKCBzweGLshRvEQQJiVIMpjYOqRC/ IEkg== X-Gm-Message-State: AOJu0Yw84A6fFMO8IZ7DvJYW7Q/5HvcGz5/nDQa9lqduUV0cePATcRiD CxH6xg8jtHn8sLPGiwx0c8ipiy1rR53KgUPLFpmgdkpRUflZwE7RsL0phyDaCdiZiuvig/qOBAM 7y7y/e+yBoQoJ5TpYJlEKxQzTgaDHUzAAkPz4AslDDhVfpFsv6lai8wlwtCocgrjCErC9KzP3TA == X-Received: by 2002:a5d:4a90:0:b0:33e:8b93:87c6 with SMTP id o16-20020a5d4a90000000b0033e8b9387c6mr2043521wrq.42.1712341228650; Fri, 05 Apr 2024 11:20:28 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFShQpAtjx2NZuV+DKyDbzT9pMJW52/BaLvReaz0/wtSA+tb47RCpgEvJRy8x2xRDKw6s+jkw== X-Received: by 2002:a5d:4a90:0:b0:33e:8b93:87c6 with SMTP id o16-20020a5d4a90000000b0033e8b9387c6mr2043512wrq.42.1712341228276; Fri, 05 Apr 2024 11:20:28 -0700 (PDT) Received: from toolbox ([2001:9e8:899a:a100:d85f:7bed:a911:4675]) by smtp.gmail.com with ESMTPSA id j11-20020adff54b000000b003433bf6651dsm1756127wrp.75.2024.04.05.11.20.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 05 Apr 2024 11:20:27 -0700 (PDT) Date: Fri, 5 Apr 2024 20:20:26 +0200 From: Sebastian Wick To: Suraj Kandpal Cc: intel-gfx@lists.freedesktop.org, chaitanya.kumar.borah@intel.com, uma.shankar@intel.com, ankit.k.nautiyal@intel.com, arun.r.murthy@intel.com, ville.syrjala@intel.com, naveen1.kumar@intel.com Subject: Re: [5/7] drm/i915/dp: Enable AUX based backlight for HDR Message-ID: <20240405182026.GA935433@toolbox> References: <20240404032931.380887-7-suraj.kandpal@intel.com> MIME-Version: 1.0 In-Reply-To: <20240404032931.380887-7-suraj.kandpal@intel.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Thu, Apr 04, 2024 at 08:59:29AM +0530, Suraj Kandpal wrote: > As of now whenerver HDR is switched on we use the PWM to change the > backlight as opposed to AUX based backlight changes in terms of nits. > This patch writes to the appropriate DPCD registers to enable aux > based backlight using values in nits. > > --v2 > -Fix max_cll and max_fall assignment [Jani] > -Fix the size sent in drm_dpcd_write [Jani] > > --v3 > -Content Luminance needs to be sent only for pre-ICL after that > it is directly picked up from hdr metadata [Ville] > > --v4 > -Add checks for HDR TCON cap bits [Ville] > -Check eotf of hdr_output_data and sets bits base of that value. > > --v5 > -Fix capability check bits. > -Check colorspace before setting BT2020 > > --v6 > -Use intel_dp_has_gamut_dip to check if we have capability > to send sdp [Ville] > -Seprate filling of all hdr tcon related bits into it's > own function. > -Check eotf data to make sure we are in HDR mode [Sebastian] > > Signed-off-by: Suraj Kandpal > --- > .../drm/i915/display/intel_dp_aux_backlight.c | 105 ++++++++++++++++-- > 1 file changed, 94 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > index 2d50a4734823..7af876e2d210 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > @@ -40,11 +40,6 @@ > #include "intel_dp.h" > #include "intel_dp_aux_backlight.h" > > -/* TODO: > - * Implement HDR, right now we just implement the bare minimum to bring us back into SDR mode so we > - * can make people's backlights work in the mean time > - */ > - > /* > * DP AUX registers for Intel's proprietary HDR backlight interface. We define > * them here since we'll likely be the only driver to ever use these. > @@ -127,9 +122,6 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > if (ret != sizeof(tcon_cap)) > return false; > > - if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > - return false; > - > drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Detected %s HDR backlight interface version %d\n", > connector->base.base.id, connector->base.name, > is_intel_tcon_cap(tcon_cap) ? "Intel" : "unsupported", tcon_cap[0]); > @@ -137,6 +129,9 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > if (!is_intel_tcon_cap(tcon_cap)) > return false; > > + if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > + return false; > + > /* > * If we don't have HDR static metadata there is no way to > * runtime detect used range for nits based control. For now > @@ -225,13 +220,27 @@ intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state, > connector->base.base.id, connector->base.name); > } > > +static bool > +intel_dp_aux_in_hdr_mode(const struct drm_connector_state *conn_state) > +{ > + struct hdr_output_metadata *hdr_metadata; > + > + if (!conn_state->hdr_output_metadata) > + return false; > + > + hdr_metadata = conn_state->hdr_output_metadata->data; > + > + return hdr_metadata->hdmi_metadata_type1.eotf != HDMI_EOTF_TRADITIONAL_GAMMA_SDR; This line worries me a bit. The TCON only supports PQ HDR mode so when the metadata request HLG or traditional gamma HDR then only the segmented backlight is enable in intel_dp_aux_fill_hdr_tcon_params but it uses HDR backlight. Did you test that this doesn't result in a black screen? Maybe change this to `eotf == HDMI_EOTF_SMPTE_ST2084` instead? > +} > + > static void > intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 level) > { > struct intel_connector *connector = to_intel_connector(conn_state->connector); > struct intel_panel *panel = &connector->panel; > > - if (panel->backlight.edp.intel.sdr_uses_aux) { > + if (intel_dp_aux_in_hdr_mode(conn_state) || > + panel->backlight.edp.intel.sdr_uses_aux) { > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > } else { > const u32 pwm_level = intel_backlight_level_to_pwm(connector, level); > @@ -240,6 +249,70 @@ intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 > } > } > > +static void > +intel_dp_aux_write_content_luminance(struct intel_connector *connector, > + struct hdr_output_metadata *hdr_metadata) > +{ > + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > + int ret; > + u8 buf[4]; > + > + if (!intel_dp_has_gamut_metadata_dip(connector->encoder)) > + return; I don't entirely understand intel_dp_has_gamut_metadata_dip, but I assume it is true when the SDP HDR stuff can be send? Either way, you only enable the HDR_OUTPUT_METADATA property if intel_dp_has_gamut_metadata_dip returns true, so you will only ever have intel_dp_aux_in_hdr_mode return true if the dip exists. So, this doesn't do anything. I think what you need is * attach the HDR_OUTPUT_METADATA property if you can send the SDP thing and the TCON supports the SDP override (>ICL) OR you can control HDR via AUX (>=ICL). * always set the content luminance via AUX > + > + buf[0] = hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF; > + buf[1] = (hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF00) >> 8; > + buf[2] = hdr_metadata->hdmi_metadata_type1.max_fall & 0xFF; > + buf[3] = (hdr_metadata->hdmi_metadata_type1.max_fall & 0xFF00) >> 8; > + > + ret = drm_dp_dpcd_write(&intel_dp->aux, > + INTEL_EDP_HDR_CONTENT_LUMINANCE, > + buf, sizeof(buf)); > + if (ret < 0) > + drm_dbg_kms(&i915->drm, > + "Content Luminance DPCD reg write failed, err:-%d\n", > + ret); > +} > + > +static void > +intel_dp_aux_fill_hdr_tcon_params(const struct drm_connector_state *conn_state, u8 *ctrl) > +{ > + struct intel_connector *connector = to_intel_connector(conn_state->connector); > + struct intel_panel *panel = &connector->panel; > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > + struct hdr_output_metadata *hdr_metadata = conn_state->hdr_output_metadata->data; > + > + /* According to spec segmented backlight needs to be set whenever panel is in > + * HDR mode. > + */ > + *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > + > + /* 2084 decode needs to set if eotf suggests so or in case of pre-ICL we disable > + * tone mapping and set 2084 decode. > + */ > + if (hdr_metadata->hdmi_metadata_type1.eotf == > + HDMI_EOTF_SMPTE_ST2084 || DISPLAY_VER(i915) < 11) { > + *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > + *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > + } else { > + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] TCON: Cannot decode requested EOTF\n", > + connector->base.base.id, connector->base.name); > + } > + > + if (panel->backlight.edp.intel.supports_2020_gamut && > + (conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB || > + conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC || > + conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_CYCC)) > + *ctrl |= INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE; > + > + if (panel->backlight.edp.intel.supports_sdp_colorimetry && > + intel_dp_has_gamut_metadata_dip(connector->encoder)) > + *ctrl |= INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > + else > + *ctrl &= ~INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > +} > + > static void > intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > const struct drm_connector_state *conn_state, u32 level) > @@ -248,6 +321,7 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > struct intel_panel *panel = &connector->panel; > struct drm_i915_private *i915 = to_i915(connector->base.dev); > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > + struct hdr_output_metadata *hdr_metadata; > int ret; > u8 old_ctrl, ctrl; > > @@ -261,8 +335,10 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > } > > ctrl = old_ctrl; > - if (panel->backlight.edp.intel.sdr_uses_aux) { > + if (intel_dp_aux_in_hdr_mode(conn_state) || > + panel->backlight.edp.intel.sdr_uses_aux) { > ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > + > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > } else { > u32 pwm_level = intel_backlight_level_to_pwm(connector, level); > @@ -272,10 +348,18 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > ctrl &= ~INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > } > > + if (intel_dp_aux_in_hdr_mode(conn_state)) > + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); > + > if (ctrl != old_ctrl && > drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1) > drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to configure DPCD brightness controls\n", > connector->base.base.id, connector->base.name); > + > + if (intel_dp_aux_in_hdr_mode(conn_state)) { > + hdr_metadata = conn_state->hdr_output_metadata->data; > + intel_dp_aux_write_content_luminance(connector, hdr_metadata); > + } > } > > static void > @@ -332,7 +416,6 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi > connector->base.base.id, connector->base.name, > panel->backlight.min, panel->backlight.max); > > - > panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector, pipe); > panel->backlight.enabled = panel->backlight.level != 0; >