| Message ID | 20170511230225.142870-4-puthik@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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) 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. > 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, > ®_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))) {
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. 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) > > > 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. > > > > >> 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, >> ®_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))) { >
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. > > > > > > > >> 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, > >> ®_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 >
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, > >> ®_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 > > >
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, ®_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))) {
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. 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(-)