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 X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2F088C433E0 for ; Fri, 8 Jan 2021 15:05:28 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id ABACC2388B for ; Fri, 8 Jan 2021 15:05:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ABACC2388B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id EBC216E07B; Fri, 8 Jan 2021 15:05:26 +0000 (UTC) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2CF616E07B; Fri, 8 Jan 2021 15:05:26 +0000 (UTC) IronPort-SDR: ONffG2UsGrVgzYtp3Zq12WrrKJKvxTwKJlo1jFFhx0D/L22XD0n3xmhR75JXR6GaAHuovPNqeB WgIRlKAdGblw== X-IronPort-AV: E=McAfee;i="6000,8403,9857"; a="196175259" X-IronPort-AV: E=Sophos;i="5.79,331,1602572400"; d="scan'208";a="196175259" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jan 2021 07:05:25 -0800 IronPort-SDR: FBGvmLPT5HSVhtytI3cq7+A5YKhYUjPW1QrEXXZd6v7ffAr9zUF8C6P3SKZ9mV8aAMq6BIp56C U8VUwjYChQOg== X-IronPort-AV: E=Sophos;i="5.79,331,1602572400"; d="scan'208";a="380147241" Received: from rgwhiteh-mobl.ger.corp.intel.com (HELO localhost) ([10.213.205.160]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jan 2021 07:05:18 -0800 From: Jani Nikula To: Lyude Paul , intel-gfx@lists.freedesktop.org In-Reply-To: <20210107225207.28091-2-lyude@redhat.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20210107225207.28091-1-lyude@redhat.com> <20210107225207.28091-2-lyude@redhat.com> Date: Fri, 08 Jan 2021 17:05:16 +0200 Message-ID: <87r1mvxydv.fsf@intel.com> MIME-Version: 1.0 Subject: Re: [Intel-gfx] [PATCH v5 1/4] drm/i915: Keep track of pwm-related backlight hooks separately 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: , Cc: Dave Airlie , Arnd Bergmann , David Airlie , Lucas De Marchi , open list , "open list:DRM DRIVERS" , Chris Wilson , Vasily Khoruzhick , Sean Paul Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Thu, 07 Jan 2021, Lyude Paul wrote: > @@ -1628,37 +1633,32 @@ static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unus > panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY; > > pch_ctl2 = intel_de_read(dev_priv, BLC_PWM_PCH_CTL2); > - panel->backlight.max = pch_ctl2 >> 16; > + panel->backlight.pwm_level_max = pch_ctl2 >> 16; > > cpu_ctl2 = intel_de_read(dev_priv, BLC_PWM_CPU_CTL2); > > - if (!panel->backlight.max) > - panel->backlight.max = get_backlight_max_vbt(connector); > + if (!panel->backlight.pwm_level_max) > + panel->backlight.pwm_level_max = get_backlight_max_vbt(connector); > > - if (!panel->backlight.max) > + if (!panel->backlight.pwm_level_max) > return -ENODEV; > > - panel->backlight.min = get_backlight_min_vbt(connector); > + panel->backlight.pwm_level_min = get_backlight_min_vbt(connector); > > - panel->backlight.enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE; > + panel->backlight.pwm_enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE; > > - cpu_mode = panel->backlight.enabled && HAS_PCH_LPT(dev_priv) && > + cpu_mode = panel->backlight.pwm_enabled && HAS_PCH_LPT(dev_priv) && > !(pch_ctl1 & BLM_PCH_OVERRIDE_ENABLE) && > (cpu_ctl2 & BLM_PWM_ENABLE); > - if (cpu_mode) > - val = pch_get_backlight(connector); > - else > - val = lpt_get_backlight(connector); > - val = intel_panel_compute_brightness(connector, val); > - panel->backlight.level = clamp(val, panel->backlight.min, > - panel->backlight.max); > > if (cpu_mode) { > + val = intel_panel_sanitize_pwm_level(connector, pch_get_backlight(connector)); > + (This really is a PITA to review, not because of how you do it but because of the hardware and the code itself. I'm just pointing out one thing here, but I'm not finished yet.) I think this sanitize call is wrong here. It should be called only when converting to and from the hw register. Here, we read directly from one hw register and write back to another hw register. Now, looking at the history, I think it's been wrong all the way since commit 5b1ec9ac7ab5 ("drm/i915/backlight: Fix backlight takeover on LPT, v3."). Probably nobody noticed, because AFAIK inverted brightness control has only ever been an issue on some gen4 platforms... *facepalm* BR, Jani. > drm_dbg_kms(&dev_priv->drm, > "CPU backlight register was enabled, switching to PCH override\n"); > > /* Write converted CPU PWM value to PCH override register */ > - lpt_set_backlight(connector->base.state, panel->backlight.level); > + lpt_set_backlight(connector->base.state, val); > intel_de_write(dev_priv, BLC_PWM_PCH_CTL1, > pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE); > -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx