<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 12, 2017 at 6:14 AM, Jani Nikula <span dir="ltr"><<a href="mailto:jani.nikula@linux.intel.com" target="_blank" class="cremed">jani.nikula@linux.intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Fri, 12 May 2017, "Pandiyan, Dhinakaran" <<a href="mailto:dhinakaran.pandiyan@intel.com" class="cremed">dhinakaran.pandiyan@intel.com</a><wbr>> wrote:<br>
> On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote:<br>
>> There are some panel that<br>
>> (1) does not support display backlight enable via AUX<br>
>> (2) support display backlight adjustment via AUX<br>
>> (3) support display backlight enable via eDP BL_ENABLE pin<br>
>><br>
>> The current driver required that (1) must be support to enable (2).<br>
>> This patch drops that requirement.<br>
>><br>
><br>
> You sent this version before I finished my follow-up questions, copying<br>
> the conversation here for context.<br>
<br>
</span>Puthikorn, please don't send new versions before the review is<br>
addressed.<br></blockquote><div>Sorry I thought I was explained it clear enough. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Pushed patches 1, 2, 5, and 7. Thanks for the patches and review.<br>
<br>
BR,<br>
Jani.<br>
<div class="HOEnZb"><div class="h5"><br>
> DK: Won't DP_EDP_BACKLIGHT_AUX_ENABLE_<wbr>CAP be 1 always? The code below,<br>
> in<br>
> intel_dp_aux_display_control_<wbr>capable(), makes sure<br>
> DP_EDP_BACKLIGHT_PIN_ENABLE_<wbr>CAP=0. The spec says at least one of these<br>
> has to be 1.<br>
><br>
> Puthikorn: We will drop the DP_EDP_BACKLIGHT_PIN_ENABLE_<wbr>CAP != 0 check<br>
> in next patch set.<br>
> This patch adds check here to prepare for that.<br>
><br>
><br>
> 1) So, this patch does not really fix what the commit message claims<br>
> because it is dependent on the following patch. Does it make sense to<br>
> remove this check in this patch? That way, this patch by itself is the<br>
> fix that the commit message says.<br>
><br>
> - !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_<wbr>CAP)<br>
><br></div></div></blockquote><div>Sure. I can remove this here and adds it in next patch instead.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
><br>
> 2) If a panel supports backlight enable via AUX and BL_ENABLE pin, this<br>
> patch (along with the next) enables backlight twice, doesn't it?<br>
> _intel_edp_backlight_on(intel_<wbr>dp) in intel_dp.c is called<br>
> unconditionally after intel_dp_aux_enable_backlight(<wbr>). I don't know how<br>
> likely this configuration is or if it's alright to enable via both AUX<br>
> and BL_ENABLE pin.<br>
><br></div></div></blockquote><div>The eDP spec did not mention this case explicitly.</div><div>But it should not hurt to enable backlight twice as we want the backlight to be enabled anyway.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
><br>
><br>
><br>
>> Signed-off-by: Puthikorn Voravootivat <<a href="mailto:puthik@chromium.org" class="cremed">puthik@chromium.org</a>><br>
>> ---<br>
>> drivers/gpu/drm/i915/intel_dp_<wbr>aux_backlight.c | 5 ++++-<br>
>> 1 file changed, 4 insertions(+), 1 deletion(-)<br>
>><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 870c03fc0f3a..c22712762957 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>
>> @@ -164,7 +168,6 @@ intel_dp_aux_display_control_<wbr>capable(struct intel_connector *connector)<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[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_<wbr>AUX_SET_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>
><br>
<br>
</div></div><div class="HOEnZb"><div class="h5">--<br>
Jani Nikula, Intel Open Source Technology Center<br>
</div></div></blockquote></div><br></div></div>