<div dir="ltr"><div><br></div><div class="gmail_extra"><div class="gmail_quote">On Wed, May 3, 2017 at 2:11 AM, Jani Nikula <span dir="ltr"><<a href="mailto:jani.nikula@linux.intel.com" target="_blank" class="gmail-cremed gmail-cremed cremed">jani.nikula@linux.intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On Tue, 18 Apr 2017, Puthikorn Voravootivat <<a href="mailto:puthik@chromium.org" class="gmail-cremed gmail-cremed cremed">puthik@chromium.org</a>> wrote:<br>
> Currently the intel_dp_aux_backlight driver requires eDP panel<br>
> to not also support backlight adjustment via PWM pin to use<br>
> this driver.<br>
><br>
> This force the eDP panel that support both ways of backlight<br>
> adjustment to do it via PWM pin.<br>
<br>
</span>Currently this is by design. But I think we agreed previously that the<br>
driver also has incorrect capability checks for the current design. The<br>
first priority would be to fix those checks. And the patch order in the<br>
series should reflect that.<br>
<span class="gmail-"><br>
> This patch adds the new prefer DPCD mode in the i915_param<br>
> to make it enable to prefer DPCD over the PWM via kernel param.<br>
><br>
> This patch also add a check to DP_EDP_BACKLIGHT_AUX_ENABLE_<wbr>CAP<br>
> in set_aux_backlight_enable() since the backlight enablement<br>
> can be done via BL_ENABLE eDP connector pin in the case that<br>
> it does not support doing that via AUX.<br>
<br>
</span>"also" in the commit message is a strong clue it should be a separate<br>
patch. What you describe is potentially a fix that should precede this<br>
patch.<br></blockquote><div>I will split this into 3 patches.</div><div>1. Fix cap check</div><div>2. Drop AUX backlight enable requirement</div><div>3. Allow choosing to use PWM pin or AUX if both supported</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="gmail-h5"><br>
><br>
> Signed-off-by: Puthikorn Voravootivat <<a href="mailto:puthik@chromium.org" class="gmail-cremed gmail-cremed cremed">puthik@chromium.org</a>><br>
> ---<br>
> drivers/gpu/drm/i915/i915_<wbr>params.c | 6 ++---<br>
> drivers/gpu/drm/i915/i915_<wbr>params.h | 2 +-<br>
> drivers/gpu/drm/i915/intel_dp_<wbr>aux_backlight.c | 33 +++++++++++++++++++--------<br>
> 3 files changed, 27 insertions(+), 14 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/i915/i915_<wbr>params.c b/drivers/gpu/drm/i915/i915_<wbr>params.c<br>
> index b6a7e363d076..960393dd5edf 100644<br>
> --- a/drivers/gpu/drm/i915/i915_<wbr>params.c<br>
> +++ b/drivers/gpu/drm/i915/i915_<wbr>params.c<br>
> @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = {<br>
> .huc_firmware_path = NULL,<br>
> .enable_dp_mst = true,<br>
> .inject_load_failure = 0,<br>
> - .enable_dpcd_backlight = false,<br>
> + .enable_dpcd_backlight = 0,<br>
> .enable_gvt = false,<br>
> };<br>
><br>
> @@ -246,9 +246,9 @@ MODULE_PARM_DESC(enable_dp_<wbr>mst,<br>
> module_param_named_unsafe(<wbr>inject_load_failure, i915.inject_load_failure, uint, 0400);<br>
> MODULE_PARM_DESC(inject_load_<wbr>failure,<br>
> "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");<br>
> -module_param_named(enable_<wbr>dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);<br>
> +module_param_named(enable_<wbr>dpcd_backlight, i915.enable_dpcd_backlight, int, 0600);<br>
> MODULE_PARM_DESC(enable_dpcd_<wbr>backlight,<br>
> - "Enable support for DPCD backlight control (default:false)");<br>
> + "Enable support for DPCD backlight control (0:disable (default), 1:prefer PWM pin, 2: prefer DPCD)");<br>
<br>
</div></div>See my comments below. I think you need to rethink the options.<br>
<div><div class="gmail-h5"><br>
><br>
> module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);<br>
> MODULE_PARM_DESC(enable_gvt,<br>
> diff --git a/drivers/gpu/drm/i915/i915_<wbr>params.h b/drivers/gpu/drm/i915/i915_<wbr>params.h<br>
> index 34148cc8637c..bf6e2c60f697 100644<br>
> --- a/drivers/gpu/drm/i915/i915_<wbr>params.h<br>
> +++ b/drivers/gpu/drm/i915/i915_<wbr>params.h<br>
> @@ -51,6 +51,7 @@<br>
> func(int, use_mmio_flip); \<br>
> func(int, mmio_debug); \<br>
> func(int, edp_vswing); \<br>
> + func(int, enable_dpcd_backlight); \<br>
> func(unsigned int, inject_load_failure); \<br>
> /* leave bools at the end to not create holes */ \<br>
> func(bool, alpha_support); \<br>
> @@ -66,7 +67,6 @@<br>
> func(bool, verbose_state_checks); \<br>
> func(bool, nuclear_pageflip); \<br>
> func(bool, enable_dp_mst); \<br>
> - func(bool, enable_dpcd_backlight); \<br>
> func(bool, enable_gvt)<br>
><br>
> #define MEMBER(T, member) T member<br>
> diff --git a/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> index 6532e226db29..42f73d9a3ccf 100644<br>
> --- a/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> +++ b/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(<wbr>struct intel_dp *intel_dp, bool enable)<br>
> {<br>
> uint8_t reg_val = 0;<br>
><br>
> + /* Early return when display use other mechanism to enable backlight. */<br>
> + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_<wbr>CAP))<br>
> + return;<br>
> +<br>
> if (drm_dp_dpcd_readb(&intel_dp-><wbr>aux, DP_EDP_DISPLAY_CONTROL_<wbr>REGISTER,<br>
> ®_val) < 0) {<br>
> DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",<br>
> @@ -138,27 +142,36 @@ static bool<br>
> intel_dp_aux_display_control_<wbr>capable(struct intel_connector *connector)<br>
> {<br>
> struct intel_dp *intel_dp = enc_to_intel_dp(&connector-><wbr>encoder->base);<br>
> + bool supported;<br>
><br>
> /* Check the eDP Display control capabilities registers to determine if<br>
> * the panel can support backlight control over the aux channel<br>
> */<br>
> - if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_<wbr>ADJUSTMENT_CAP &&<br>
> - (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_<wbr>CAP) &&<br>
> - !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_<wbr>CAP) ||<br>
> - (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_<wbr>PWM_PIN_CAP))) {<br>
> - DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");<br>
> - return true;<br>
> + switch (i915.enable_dpcd_backlight) {<br>
> + case 1: /* prefer PWM pin */<br>
> + supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_<wbr>ADJUSTMENT_CAP) &&<br>
> + (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_<wbr>CAP) &&<br>
> + !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_<wbr>CAP) &&<br>
> + !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_<wbr>PWM_PIN_CAP);<br>
> + break;<br>
<br>
</div></div>This is not right. If we're going to use the PWM pin for backlight<br>
control, it's what's in intel_panel.c, and<br>
intel_dp_aux_init_backlight_<wbr>funcs() should return -ENODEV. We only<br>
support one or the other.<br></blockquote><div>Fixed in next patch set. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I think you probably need to have a hard look at Table 10-1 "Summary of<br>
Backlight Control Modes Using DPCD Registers" in eDP 1.4b, and tell us<br>
what modes you really want to support and how. For example, the product<br>
mode or any DPCD/PWM mixed modes aren't very easily achieved with the<br>
current design.<br></blockquote><div><br></div><div><br class="gmail-Apple-interchange-newline">What we actually need is that we have panel that</div><div>- does not support display backlight enable via AUX</div><div>- support display backlight adjustment via AUX</div><div>- support display backlight enable via eDP BL_ENABLE pin</div><div>- support display backlight adjustment via eDP PWM pin but that pin is not connected</div><div>- support display backlight frequency adjustment via AUX</div><div><br></div><div>and we want to</div><div>- enable backlight via eDP BL_ENABLE pin</div><div>- adjust backlight brightness via AUX</div><div>- adjust backlight frequency via AUX </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
BR,<br>
Jani.<br>
<span class="gmail-"><br>
<br>
> + case 2: /* prefer DPCD */<br>
> + supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_<wbr>ADJUSTMENT_CAP) &&<br>
> + (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_<wbr>AUX_SET_CAP);<br>
> + break;<br>
> + default:<br>
> + supported = false;<br>
> }<br>
> - return false;<br>
> +<br>
> + if (supported)<br>
> + DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");<br>
> +<br>
> + return supported;<br>
> }<br>
><br>
> int intel_dp_aux_init_backlight_<wbr>funcs(struct intel_connector *intel_connector)<br>
> {<br>
> struct intel_panel *panel = &intel_connector->panel;<br>
><br>
> - if (!i915.enable_dpcd_backlight)<br>
> - return -ENODEV;<br>
> -<br>
> if (!intel_dp_aux_display_<wbr>control_capable(intel_<wbr>connector))<br>
> return -ENODEV;<br>
<br>
--<br>
</span>Jani Nikula, Intel Open Source Technology Center<br>
</blockquote></div><br></div></div>