| Message ID | 20220310200518.247909-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/display: Fix HPD short pulse handling for eDP | expand |
On Thu, Mar 10, 2022 at 12:05:17PM -0800, José Roberto de Souza wrote: > Commit 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel > powered off") completely broke short pulse handling for eDP as it is > usually generated by sink when it is displaying image and there is > some error or status that source needs to handle. > > When power panel is enabled, this state is enough to power aux > transactions and VDD override is disabled, so intel_pps_have_power() > is always returning false causing short pulses to be ignored. I think the times that we use the vdd override should be limited to: - aux transfers while the display off - potentially short periods of time during the modeset sequence So I guess what you're saying here is that during those times some panel is triggering an IRQ_HPD which, if ignored, causes some problem for us? > > So here better naming this function that intends to check if aux > lines are powered to avoid the endless cycle mentioned in the commit > being fixed and fixing the check for what it is intended. > > Fixes: 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel powered off") > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > drivers/gpu/drm/i915/display/intel_pps.c | 4 ++-- > drivers/gpu/drm/i915/display/intel_pps.h | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 619546441eae5..b029b064000d6 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -4867,7 +4867,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *dig_port, bool long_hpd) > struct intel_dp *intel_dp = &dig_port->dp; > > if (dig_port->base.type == INTEL_OUTPUT_EDP && > - (long_hpd || !intel_pps_have_power(intel_dp))) { > + (long_hpd || !intel_pps_have_vdd_power(intel_dp))) { > /* > * vdd off can generate a long/short pulse on eDP which > * would require vdd on to handle it, and thus we > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c > index 9c986e8932f87..d3e6083ad5b79 100644 > --- a/drivers/gpu/drm/i915/display/intel_pps.c > +++ b/drivers/gpu/drm/i915/display/intel_pps.c > @@ -1075,13 +1075,13 @@ static void intel_pps_vdd_sanitize(struct intel_dp *intel_dp) > edp_panel_vdd_schedule_off(intel_dp); > } > > -bool intel_pps_have_power(struct intel_dp *intel_dp) > +bool intel_pps_have_vdd_power(struct intel_dp *intel_dp) > { > intel_wakeref_t wakeref; > bool have_power = false; > > with_intel_pps_lock(intel_dp, wakeref) { > - have_power = edp_have_panel_power(intel_dp) && > + have_power = edp_have_panel_power(intel_dp) || > edp_have_panel_vdd(intel_dp); > } > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h > index fbb47f6f453e4..948523ce32417 100644 > --- a/drivers/gpu/drm/i915/display/intel_pps.h > +++ b/drivers/gpu/drm/i915/display/intel_pps.h > @@ -37,7 +37,7 @@ void intel_pps_vdd_on(struct intel_dp *intel_dp); > void intel_pps_on(struct intel_dp *intel_dp); > void intel_pps_off(struct intel_dp *intel_dp); > void intel_pps_vdd_off_sync(struct intel_dp *intel_dp); > -bool intel_pps_have_power(struct intel_dp *intel_dp); > +bool intel_pps_have_vdd_power(struct intel_dp *intel_dp); > void intel_pps_wait_power_cycle(struct intel_dp *intel_dp); > > void intel_pps_init(struct intel_dp *intel_dp); > -- > 2.35.1
On Thu, 2022-03-10 at 23:52 +0200, Ville Syrjälä wrote: > On Thu, Mar 10, 2022 at 12:05:17PM -0800, José Roberto de Souza wrote: > > Commit 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel > > powered off") completely broke short pulse handling for eDP as it is > > usually generated by sink when it is displaying image and there is > > some error or status that source needs to handle. > > > > When power panel is enabled, this state is enough to power aux > > transactions and VDD override is disabled, so intel_pps_have_power() > > is always returning false causing short pulses to be ignored. > > I think the times that we use the vdd override should be > limited to: > - aux transfers while the display off > - potentially short periods of time during the modeset sequence > > So I guess what you're saying here is that during those times > some panel is triggering an IRQ_HPD which, if ignored, causes > some problem for us? No, not in those times. When panel is on scanning out frames, PPS is enabled and VDD is disabled. So any short pulse HPD was being ignored because (edp_have_panel_power() && edp_have_panel_vdd()) is always false. > > > > > So here better naming this function that intends to check if aux > > lines are powered to avoid the endless cycle mentioned in the commit > > being fixed and fixing the check for what it is intended. > > > > Fixes: 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel powered off") > > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Uma Shankar <uma.shankar@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > > drivers/gpu/drm/i915/display/intel_pps.c | 4 ++-- > > drivers/gpu/drm/i915/display/intel_pps.h | 2 +- > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 619546441eae5..b029b064000d6 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -4867,7 +4867,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *dig_port, bool long_hpd) > > struct intel_dp *intel_dp = &dig_port->dp; > > > > if (dig_port->base.type == INTEL_OUTPUT_EDP && > > - (long_hpd || !intel_pps_have_power(intel_dp))) { > > + (long_hpd || !intel_pps_have_vdd_power(intel_dp))) { > > /* > > * vdd off can generate a long/short pulse on eDP which > > * would require vdd on to handle it, and thus we > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c > > index 9c986e8932f87..d3e6083ad5b79 100644 > > --- a/drivers/gpu/drm/i915/display/intel_pps.c > > +++ b/drivers/gpu/drm/i915/display/intel_pps.c > > @@ -1075,13 +1075,13 @@ static void intel_pps_vdd_sanitize(struct intel_dp *intel_dp) > > edp_panel_vdd_schedule_off(intel_dp); > > } > > > > -bool intel_pps_have_power(struct intel_dp *intel_dp) > > +bool intel_pps_have_vdd_power(struct intel_dp *intel_dp) > > { > > intel_wakeref_t wakeref; > > bool have_power = false; > > > > with_intel_pps_lock(intel_dp, wakeref) { > > - have_power = edp_have_panel_power(intel_dp) && > > + have_power = edp_have_panel_power(intel_dp) || > > edp_have_panel_vdd(intel_dp); > > } > > > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h > > index fbb47f6f453e4..948523ce32417 100644 > > --- a/drivers/gpu/drm/i915/display/intel_pps.h > > +++ b/drivers/gpu/drm/i915/display/intel_pps.h > > @@ -37,7 +37,7 @@ void intel_pps_vdd_on(struct intel_dp *intel_dp); > > void intel_pps_on(struct intel_dp *intel_dp); > > void intel_pps_off(struct intel_dp *intel_dp); > > void intel_pps_vdd_off_sync(struct intel_dp *intel_dp); > > -bool intel_pps_have_power(struct intel_dp *intel_dp); > > +bool intel_pps_have_vdd_power(struct intel_dp *intel_dp); > > void intel_pps_wait_power_cycle(struct intel_dp *intel_dp); > > > > void intel_pps_init(struct intel_dp *intel_dp); > > -- > > 2.35.1 >
On Fri, Mar 11, 2022 at 01:05:49PM +0000, Souza, Jose wrote: > On Thu, 2022-03-10 at 23:52 +0200, Ville Syrjälä wrote: > > On Thu, Mar 10, 2022 at 12:05:17PM -0800, José Roberto de Souza wrote: > > > Commit 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel > > > powered off") completely broke short pulse handling for eDP as it is > > > usually generated by sink when it is displaying image and there is > > > some error or status that source needs to handle. > > > > > > When power panel is enabled, this state is enough to power aux > > > transactions and VDD override is disabled, so intel_pps_have_power() > > > is always returning false causing short pulses to be ignored. > > > > I think the times that we use the vdd override should be > > limited to: > > - aux transfers while the display off > > - potentially short periods of time during the modeset sequence > > > > So I guess what you're saying here is that during those times > > some panel is triggering an IRQ_HPD which, if ignored, causes > > some problem for us? > > No, not in those times. > When panel is on scanning out frames, PPS is enabled and VDD is disabled. > So any short pulse HPD was being ignored because (edp_have_panel_power() && edp_have_panel_vdd()) is always false. > > > > > > > > > So here better naming this function that intends to check if aux > > > lines are powered to avoid the endless cycle mentioned in the commit > > > being fixed and fixing the check for what it is intended. > > > > > > Fixes: 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel powered off") > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Uma Shankar <uma.shankar@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > > > drivers/gpu/drm/i915/display/intel_pps.c | 4 ++-- > > > drivers/gpu/drm/i915/display/intel_pps.h | 2 +- > > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > > index 619546441eae5..b029b064000d6 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -4867,7 +4867,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *dig_port, bool long_hpd) > > > struct intel_dp *intel_dp = &dig_port->dp; > > > > > > if (dig_port->base.type == INTEL_OUTPUT_EDP && > > > - (long_hpd || !intel_pps_have_power(intel_dp))) { > > > + (long_hpd || !intel_pps_have_vdd_power(intel_dp))) { > > > /* > > > * vdd off can generate a long/short pulse on eDP which > > > * would require vdd on to handle it, and thus we > > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c > > > index 9c986e8932f87..d3e6083ad5b79 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_pps.c > > > +++ b/drivers/gpu/drm/i915/display/intel_pps.c > > > @@ -1075,13 +1075,13 @@ static void intel_pps_vdd_sanitize(struct intel_dp *intel_dp) > > > edp_panel_vdd_schedule_off(intel_dp); > > > } > > > > > > -bool intel_pps_have_power(struct intel_dp *intel_dp) > > > +bool intel_pps_have_vdd_power(struct intel_dp *intel_dp) > > > { > > > intel_wakeref_t wakeref; > > > bool have_power = false; > > > > > > with_intel_pps_lock(intel_dp, wakeref) { > > > - have_power = edp_have_panel_power(intel_dp) && Ah, that s/&&/||/ is the bug you're fixing. I didn't even notice, probably because of the broken indentation in the code here. I just read it and thought it only checks for panel power and not vdd. Can you also fix up the indentation a bit so the code doesn't look so weird? I think I'd call the function intel_pps_have_panel_power_or_vdd() (or something along those lines) to make it clear what it does. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > + have_power = edp_have_panel_power(intel_dp) || > > > edp_have_panel_vdd(intel_dp); > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h > > > index fbb47f6f453e4..948523ce32417 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_pps.h > > > +++ b/drivers/gpu/drm/i915/display/intel_pps.h > > > @@ -37,7 +37,7 @@ void intel_pps_vdd_on(struct intel_dp *intel_dp); > > > void intel_pps_on(struct intel_dp *intel_dp); > > > void intel_pps_off(struct intel_dp *intel_dp); > > > void intel_pps_vdd_off_sync(struct intel_dp *intel_dp); > > > -bool intel_pps_have_power(struct intel_dp *intel_dp); > > > +bool intel_pps_have_vdd_power(struct intel_dp *intel_dp); > > > void intel_pps_wait_power_cycle(struct intel_dp *intel_dp); > > > > > > void intel_pps_init(struct intel_dp *intel_dp); > > > -- > > > 2.35.1 > > >
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 619546441eae5..b029b064000d6 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4867,7 +4867,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *dig_port, bool long_hpd) struct intel_dp *intel_dp = &dig_port->dp; if (dig_port->base.type == INTEL_OUTPUT_EDP && - (long_hpd || !intel_pps_have_power(intel_dp))) { + (long_hpd || !intel_pps_have_vdd_power(intel_dp))) { /* * vdd off can generate a long/short pulse on eDP which * would require vdd on to handle it, and thus we diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c index 9c986e8932f87..d3e6083ad5b79 100644 --- a/drivers/gpu/drm/i915/display/intel_pps.c +++ b/drivers/gpu/drm/i915/display/intel_pps.c @@ -1075,13 +1075,13 @@ static void intel_pps_vdd_sanitize(struct intel_dp *intel_dp) edp_panel_vdd_schedule_off(intel_dp); } -bool intel_pps_have_power(struct intel_dp *intel_dp) +bool intel_pps_have_vdd_power(struct intel_dp *intel_dp) { intel_wakeref_t wakeref; bool have_power = false; with_intel_pps_lock(intel_dp, wakeref) { - have_power = edp_have_panel_power(intel_dp) && + have_power = edp_have_panel_power(intel_dp) || edp_have_panel_vdd(intel_dp); } diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h index fbb47f6f453e4..948523ce32417 100644 --- a/drivers/gpu/drm/i915/display/intel_pps.h +++ b/drivers/gpu/drm/i915/display/intel_pps.h @@ -37,7 +37,7 @@ void intel_pps_vdd_on(struct intel_dp *intel_dp); void intel_pps_on(struct intel_dp *intel_dp); void intel_pps_off(struct intel_dp *intel_dp); void intel_pps_vdd_off_sync(struct intel_dp *intel_dp); -bool intel_pps_have_power(struct intel_dp *intel_dp); +bool intel_pps_have_vdd_power(struct intel_dp *intel_dp); void intel_pps_wait_power_cycle(struct intel_dp *intel_dp); void intel_pps_init(struct intel_dp *intel_dp);
Commit 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel powered off") completely broke short pulse handling for eDP as it is usually generated by sink when it is displaying image and there is some error or status that source needs to handle. When power panel is enabled, this state is enough to power aux transactions and VDD override is disabled, so intel_pps_have_power() is always returning false causing short pulses to be ignored. So here better naming this function that intends to check if aux lines are powered to avoid the endless cycle mentioned in the commit being fixed and fixing the check for what it is intended. Fixes: 13ea6db2cf24 ("drm/i915/edp: Ignore short pulse when panel powered off") Cc: Anshuman Gupta <anshuman.gupta@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Uma Shankar <uma.shankar@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- drivers/gpu/drm/i915/display/intel_pps.c | 4 ++-- drivers/gpu/drm/i915/display/intel_pps.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)