Re: [PATCH v7 3/9] drm/i915: Drop AUX backlight enable check for backlight control

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Fri, 2017-05-12 at 11:10 -0700, Puthikorn Voravootivat wrote:
> 
> 
> On Fri, May 12, 2017 at 6:14 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>         On Fri, 12 May 2017, "Pandiyan, Dhinakaran"
>         <dhinakaran.pandiyan@intel.com> wrote:
>         > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat
>         wrote:
>         >> There are some panel that
>         >> (1) does not support display backlight enable via AUX
>         >> (2) support display backlight adjustment via AUX
>         >> (3) support display backlight enable via eDP BL_ENABLE pin
>         >>
>         >> The current driver required that (1) must be support to
>         enable (2).
>         >> This patch drops that requirement.
>         >>
>         >
>         > You sent this version before I finished my follow-up
>         questions, copying
>         > the conversation here for context.
>         
>         Puthikorn, please don't send new versions before the review is
>         addressed.
> Sorry I thought I was explained it clear enough.  
>         
>         Pushed patches 1, 2, 5, and 7. Thanks for the patches and
>         review.
>         
>         BR,
>         Jani.
>         
>         > DK: Won't DP_EDP_BACKLIGHT_AUX_ENABLE_CAP be 1 always? The
>         code below,
>         > in
>         > intel_dp_aux_display_control_capable(), makes sure
>         > DP_EDP_BACKLIGHT_PIN_ENABLE_CAP=0. The spec says at least
>         one of these
>         > has to be 1.
>         >
>         > Puthikorn: We will drop the
>         DP_EDP_BACKLIGHT_PIN_ENABLE_CAP != 0 check
>         > in next patch set.
>         > This patch adds check here to prepare for that.
>         >
>         >
>         > 1) So, this patch does not really fix what the commit
>         message claims
>         > because it is dependent on the following patch. Does it make
>         sense to
>         > remove this check in this patch? That way, this patch by
>         itself is the
>         > fix that the commit message says.
>         >
>         > -           !((intel_dp->edp_dpcd[1] &
>         DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)
>         >
>         
> Sure. I can remove this here and adds it in next patch instead.
> 
> 
>         >
>         > 2) If a panel supports backlight enable via AUX and
>         BL_ENABLE pin, this
>         > patch (along with the next) enables backlight twice, doesn't
>         it?
>         > _intel_edp_backlight_on(intel_dp) in intel_dp.c is called
>         > unconditionally after intel_dp_aux_enable_backlight(). I
>         don't know how
>         > likely this configuration is or if it's alright to enable
>         via both AUX
>         > and BL_ENABLE pin.
>         >
>         
> The eDP spec did not mention this case explicitly.
> But it should not hurt to enable backlight twice as we want the
> backlight to be enabled anyway.
>  

Hope so, at least a TODO would be a reasonable warning.

>         >
>         >
>         >
>         >> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>         >> ---
>         >>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
>         >>  1 file changed, 4 insertions(+), 1 deletion(-)
>         >>
>         >> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >> index 870c03fc0f3a..c22712762957 100644
>         >> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >> @@ -28,6 +28,10 @@ static void
>         set_aux_backlight_enable(struct intel_dp *intel_dp, bool
>         enable)
>         >>  {
>         >>      uint8_t reg_val = 0;
>         >>
>         >> +       /* Early return when display use other mechanism to
>         enable backlight. */
>         >> +    if (!(intel_dp->edp_dpcd[1] &
>         DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
>         >> +            return;
>         >> +
>         >>      if (drm_dp_dpcd_readb(&intel_dp->aux,
>         DP_EDP_DISPLAY_CONTROL_REGISTER,
>         >>                            &reg_val) < 0) {
>         >>              DRM_DEBUG_KMS("Failed to read DPCD register 0x
>         %x\n",
>         >> @@ -164,7 +168,6 @@
>         intel_dp_aux_display_control_capable(struct intel_connector
>         *connector)
>         >>       * the panel can support backlight control over the
>         aux channel
>         >>       */
>         >>      if (intel_dp->edp_dpcd[1] &
>         DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
>         >> -        (intel_dp->edp_dpcd[1] &
>         DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
>         >>          (intel_dp->edp_dpcd[2] &
>         DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
>         >>          !((intel_dp->edp_dpcd[1] &
>         DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
>         >>            (intel_dp->edp_dpcd[2] &
>         DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
>         >
>         
>         
>         --
>         Jani Nikula, Intel Open Source Technology Center
>         
> 
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx