| Message ID | 1497298572-23848-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 12-06-17 om 22:16 schreef Dhinakaran Pandiyan: > Maarten and Ville noticed that we are enabling backlight via DP aux very > early in the modeset_init path via the intel_dp_aux_setup_backlight() > function. Looks like all we need to do during _setup_backlight() is > read the current brightness state instead of modifying it, so I don't > why need to _enable_backlight() from _setup_backlight(). > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > index 6cc6298..228ca06 100644 > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > @@ -80,10 +80,6 @@ static uint32_t intel_dp_aux_get_backlight(struct intel_connector *connector) > static void > intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 level) > { > - /* > - * conn_state->best_encoder is likely NULL when called from > - * intel_dp_aux_setup_backlight() > - */ > struct intel_connector *connector = to_intel_connector(conn_state->connector); > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > uint8_t vals[2] = { 0x0 }; > @@ -106,10 +102,6 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st > const struct drm_connector_state *conn_state) > { > struct intel_connector *connector = to_intel_connector(conn_state->connector); > - /* > - * conn_state->best_encoder (and crtc_state) are NULL when called from > - * intel_dp_aux_setup_backlight() > - */ > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); Maybe change this one to conn_state->best_encoder now? Same as for set_backlight. Needs some runtime testing though. :) > uint8_t dpcd_buf = 0; > uint8_t edp_backlight_mode = 0; > @@ -156,8 +148,6 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector, > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > struct intel_panel *panel = &connector->panel; > > - intel_dp_aux_enable_backlight(NULL, connector->base.state); > - > if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > panel->backlight.max = 0xFFFF; > else Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cheers, Maarten
On Mon, 12 Jun 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > Maarten and Ville noticed that we are enabling backlight via DP aux very > early in the modeset_init path via the intel_dp_aux_setup_backlight() > function. Looks like all we need to do during _setup_backlight() is > read the current brightness state instead of modifying it, so I don't > why need to _enable_backlight() from _setup_backlight(). Please always use git blame to find the commit, and Cc the relevant folks. Done now. It's e7156c833903 ("drm/i915: Add Backlight Control using DPCD for eDP connectors (v9)"). The changelog says the call was intentionally moved there. ("v5: Moved call to initialize backlight registers to dp_aux_setup_backlight"). I have no recollection of why, and seems wrong, regardless of me signing off on the patch. We might be seeing some of the fallout also because of the recent changes to the enable implementation by Puthikorn, though I agree the whole call does seem wrong to begin with. Cc Puthikorn too. Acked-by: Jani Nikula <jani.nikula@intel.com> BR, Jani. > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > index 6cc6298..228ca06 100644 > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > @@ -80,10 +80,6 @@ static uint32_t intel_dp_aux_get_backlight(struct intel_connector *connector) > static void > intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 level) > { > - /* > - * conn_state->best_encoder is likely NULL when called from > - * intel_dp_aux_setup_backlight() > - */ > struct intel_connector *connector = to_intel_connector(conn_state->connector); > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > uint8_t vals[2] = { 0x0 }; > @@ -106,10 +102,6 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st > const struct drm_connector_state *conn_state) > { > struct intel_connector *connector = to_intel_connector(conn_state->connector); > - /* > - * conn_state->best_encoder (and crtc_state) are NULL when called from > - * intel_dp_aux_setup_backlight() > - */ > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > uint8_t dpcd_buf = 0; > uint8_t edp_backlight_mode = 0; > @@ -156,8 +148,6 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector, > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > struct intel_panel *panel = &connector->panel; > > - intel_dp_aux_enable_backlight(NULL, connector->base.state); > - > if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > panel->backlight.max = 0xFFFF; > else
On Tue, 2017-06-13 at 08:29 +0200, Maarten Lankhorst wrote: > Op 12-06-17 om 22:16 schreef Dhinakaran Pandiyan: > > Maarten and Ville noticed that we are enabling backlight via DP aux very > > early in the modeset_init path via the intel_dp_aux_setup_backlight() > > function. Looks like all we need to do during _setup_backlight() is > > read the current brightness state instead of modifying it, so I don't > > why need to _enable_backlight() from _setup_backlight(). > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 10 ---------- > > 1 file changed, 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > index 6cc6298..228ca06 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > @@ -80,10 +80,6 @@ static uint32_t intel_dp_aux_get_backlight(struct intel_connector *connector) > > static void > > intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 level) > > { > > - /* > > - * conn_state->best_encoder is likely NULL when called from > > - * intel_dp_aux_setup_backlight() > > - */ > > struct intel_connector *connector = to_intel_connector(conn_state->connector); > > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > > uint8_t vals[2] = { 0x0 }; > > @@ -106,10 +102,6 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st > > const struct drm_connector_state *conn_state) > > { > > struct intel_connector *connector = to_intel_connector(conn_state->connector); > > - /* > > - * conn_state->best_encoder (and crtc_state) are NULL when called from > > - * intel_dp_aux_setup_backlight() > > - */ > > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > Maybe change this one to conn_state->best_encoder now? Same as for set_backlight. Needs some runtime testing though. :) I see four instances that need to be converted, I should perhaps do it a in a separate patch. But, I can't seem to get brightness control working on the SDP I have here. So, I'm a bit wary of doing it until I can bench test properly. Thanks for the review. -DK > > uint8_t dpcd_buf = 0; > > uint8_t edp_backlight_mode = 0; > > @@ -156,8 +148,6 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector, > > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > > struct intel_panel *panel = &connector->panel; > > > > - intel_dp_aux_enable_backlight(NULL, connector->base.state); > > - > > if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > > panel->backlight.max = 0xFFFF; > > else > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cheers, > > Maarten > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2017-06-13 at 09:48 +0300, Jani Nikula wrote: > On Mon, 12 Jun 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > > Maarten and Ville noticed that we are enabling backlight via DP aux very > > early in the modeset_init path via the intel_dp_aux_setup_backlight() > > function. Looks like all we need to do during _setup_backlight() is > > read the current brightness state instead of modifying it, so I don't > > why need to _enable_backlight() from _setup_backlight(). > > Please always use git blame to find the commit, and Cc the relevant > folks. Done now. It's e7156c833903 ("drm/i915: Add Backlight Control > using DPCD for eDP connectors (v9)"). I'll amend the commit message to refer to the orginal patch and resend. > > The changelog says the call was intentionally moved there. ("v5: Moved > call to initialize backlight registers to dp_aux_setup_backlight"). I > have no recollection of why, and seems wrong, regardless of me signing > off on the patch. I looked at the conversation history, looks like it was intentionally moved from _display_control_capable() to _setup_backlight() but not clear why it was added to _display_control_capable() in the first place. > > We might be seeing some of the fallout also because of the recent > changes to the enable implementation by Puthikorn, though I agree the > whole call does seem wrong to begin with. Cc Puthikorn too. > > > Acked-by: Jani Nikula <jani.nikula@intel.com> > > > BR, > Jani. > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 10 ---------- > > 1 file changed, 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > index 6cc6298..228ca06 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > @@ -80,10 +80,6 @@ static uint32_t intel_dp_aux_get_backlight(struct intel_connector *connector) > > static void > > intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 level) > > { > > - /* > > - * conn_state->best_encoder is likely NULL when called from > > - * intel_dp_aux_setup_backlight() > > - */ > > struct intel_connector *connector = to_intel_connector(conn_state->connector); > > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > > uint8_t vals[2] = { 0x0 }; > > @@ -106,10 +102,6 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st > > const struct drm_connector_state *conn_state) > > { > > struct intel_connector *connector = to_intel_connector(conn_state->connector); > > - /* > > - * conn_state->best_encoder (and crtc_state) are NULL when called from > > - * intel_dp_aux_setup_backlight() > > - */ > > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > > uint8_t dpcd_buf = 0; > > uint8_t edp_backlight_mode = 0; > > @@ -156,8 +148,6 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector, > > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > > struct intel_panel *panel = &connector->panel; > > > > - intel_dp_aux_enable_backlight(NULL, connector->base.state); > > - > > if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > > panel->backlight.max = 0xFFFF; > > else >
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index 6cc6298..228ca06 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -80,10 +80,6 @@ static uint32_t intel_dp_aux_get_backlight(struct intel_connector *connector) static void intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 level) { - /* - * conn_state->best_encoder is likely NULL when called from - * intel_dp_aux_setup_backlight() - */ struct intel_connector *connector = to_intel_connector(conn_state->connector); struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); uint8_t vals[2] = { 0x0 }; @@ -106,10 +102,6 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st const struct drm_connector_state *conn_state) { struct intel_connector *connector = to_intel_connector(conn_state->connector); - /* - * conn_state->best_encoder (and crtc_state) are NULL when called from - * intel_dp_aux_setup_backlight() - */ struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); uint8_t dpcd_buf = 0; uint8_t edp_backlight_mode = 0; @@ -156,8 +148,6 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector, struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); struct intel_panel *panel = &connector->panel; - intel_dp_aux_enable_backlight(NULL, connector->base.state); - if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) panel->backlight.max = 0xFFFF; else
Maarten and Ville noticed that we are enabling backlight via DP aux very early in the modeset_init path via the intel_dp_aux_setup_backlight() function. Looks like all we need to do during _setup_backlight() is read the current brightness state instead of modifying it, so I don't why need to _enable_backlight() from _setup_backlight(). Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 10 ---------- 1 file changed, 10 deletions(-)