| Message ID | 20190306064728.8234-4-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/9] drm/i915/psr: Remove PSR2 FIXME | expand |
On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote: > If has_psr is set it means that CRTC has a EDP panel attached so it > can be dropped, also has_psr is better than check for EDP output > alone as it will avoid set mode_changed when PSR is not supported in > panel or with current modeset. Since we were already checking has_psr, I don't think it is relevant to say checking just that is "better than check for EDP output alone". Can you please reword the commit message? Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > b/drivers/gpu/drm/i915/intel_psr.c > index 6175b1d2e0c8..2d9f64c362e2 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -981,9 +981,7 @@ static int intel_psr_fastset_force(struct > drm_i915_private *dev_priv) > > intel_crtc_state = to_intel_crtc_state(crtc_state); > > - if (crtc_state->active && > - intel_crtc_has_type(intel_crtc_state, > INTEL_OUTPUT_EDP) && > - intel_crtc_state->has_psr) { > + if (crtc_state->active && intel_crtc_state->has_psr) { > /* Mark mode as changed to trigger a pipe- > >update() */ > crtc_state->mode_changed = true; > break;
On Thu, 2019-03-07 at 12:26 -0800, Dhinakaran Pandiyan wrote: > On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote: > > If has_psr is set it means that CRTC has a EDP panel attached so it > > can be dropped, also has_psr is better than check for EDP output > > alone as it will avoid set mode_changed when PSR is not supported > > in > > panel or with current modeset. > Since we were already checking has_psr, I don't think it is relevant > to > say checking just that is "better than check for EDP output alone". > Can > you please reword the commit message? What about replace everything to: If has_psr is set it means that CRTC has a EDP panel attached so the EDP check is redundant and can be dropped. > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 6175b1d2e0c8..2d9f64c362e2 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -981,9 +981,7 @@ static int intel_psr_fastset_force(struct > > drm_i915_private *dev_priv) > > > > intel_crtc_state = to_intel_crtc_state(crtc_state); > > > > - if (crtc_state->active && > > - intel_crtc_has_type(intel_crtc_state, > > INTEL_OUTPUT_EDP) && > > - intel_crtc_state->has_psr) { > > + if (crtc_state->active && intel_crtc_state->has_psr) { > > /* Mark mode as changed to trigger a pipe- > > > update() */ > > crtc_state->mode_changed = true; > > break;
On Thu, 2019-03-07 at 14:53 -0800, Souza, Jose wrote: > On Thu, 2019-03-07 at 12:26 -0800, Dhinakaran Pandiyan wrote: > > On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote: > > > If has_psr is set it means that CRTC has a EDP panel attached so > > > it > > > can be dropped, also has_psr is better than check for EDP output > > > alone as it will avoid set mode_changed when PSR is not supported > > > in > > > panel or with current modeset. > > > > Since we were already checking has_psr, I don't think it is > > relevant > > to > > say checking just that is "better than check for EDP output alone". > > Can > > you please reword the commit message? > > What about replace everything to: > > If has_psr is set it means that CRTC has a EDP panel attached so the > EDP check is redundant and can be dropped. Yes, reads better. > > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 6175b1d2e0c8..2d9f64c362e2 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -981,9 +981,7 @@ static int intel_psr_fastset_force(struct > > > drm_i915_private *dev_priv) > > > > > > intel_crtc_state = to_intel_crtc_state(crtc_state); > > > > > > - if (crtc_state->active && > > > - intel_crtc_has_type(intel_crtc_state, > > > INTEL_OUTPUT_EDP) && > > > - intel_crtc_state->has_psr) { > > > + if (crtc_state->active && intel_crtc_state->has_psr) { > > > /* Mark mode as changed to trigger a pipe- > > > > update() */ > > > > > > crtc_state->mode_changed = true; > > > break;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 6175b1d2e0c8..2d9f64c362e2 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -981,9 +981,7 @@ static int intel_psr_fastset_force(struct drm_i915_private *dev_priv) intel_crtc_state = to_intel_crtc_state(crtc_state); - if (crtc_state->active && - intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) && - intel_crtc_state->has_psr) { + if (crtc_state->active && intel_crtc_state->has_psr) { /* Mark mode as changed to trigger a pipe->update() */ crtc_state->mode_changed = true; break;