| Message ID | 20160829123522.9532-2-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Seg, 2016-08-29 às 18:05 +0530, Kumar, Mahesh escreveu: > Set the intel_crtc->active flag after pipe/crtc is actually active in > haswell_crtc_enable function. Why? Can you please elaborate more on why this change is needed, what are the benefits it brings, what are the problems it solves and why is the current code bad or wrong? Please explain all this in the commit message, not just as an email reply. In other words: if I'm bisecting a theoretical bug and then suddenly conclude that this patch is the problem, how will I know what's going to break once I revert this patch? Thanks, Paulo > > Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index e4e6141..7258883 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5427,8 +5427,6 @@ static void haswell_crtc_enable(struct > intel_crtc_state *pipe_config, > > intel_color_set_csc(&pipe_config->base); > > - intel_crtc->active = true; > - > if (intel_crtc->config->has_pch_encoder) > intel_set_cpu_fifo_underrun_reporting(dev_priv, > pipe, false); > else > @@ -5475,6 +5473,8 @@ static void haswell_crtc_enable(struct > intel_crtc_state *pipe_config, > assert_vblank_disabled(crtc); > drm_crtc_vblank_on(crtc); > > + intel_crtc->active = true; > + > intel_encoders_enable(crtc, pipe_config, old_state); > > if (intel_crtc->config->has_pch_encoder) {
Op 29-08-16 om 14:35 schreef Kumar, Mahesh: > Set the intel_crtc->active flag after pipe/crtc is actually active in > haswell_crtc_enable function. > > Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com> I think more state needs to be pre-calculated, rather than touching intel_crtc->active. Something like using the .initial_watermarks callback, we're so close to removing that non-atomic flag altogether. :-) ~Maarten
earlier implementation of "DDB writing" was using this flag before wait_for_vblank if we need to expand the DDB & it overlap with other pipe's DDB. In that case, I observed on corner case requiring a cd-clock change, intel_update_watermarks was waiting for vblank, when it was called from haswell_crtc_enable function. This was leading to "vblank not available on crtc " warning (since vblank int was not yet enabled on that CRTC). But as I can see we now no longer use wait for vblank during DDB writing if it's called from crtc_enable function after Paul's patch series, I can drop this patch all together. thanks for review & input Regards, -Mahesh On Wednesday 31 August 2016 04:21 PM, Maarten Lankhorst wrote: > more state needs to be pre-calculated, rather than touching intel_crtc->active. > Something like using the .initial_watermarks callback, we're so close to removing > that non-atomic flag altogether.:-) > > ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e4e6141..7258883 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5427,8 +5427,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, intel_color_set_csc(&pipe_config->base); - intel_crtc->active = true; - if (intel_crtc->config->has_pch_encoder) intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); else @@ -5475,6 +5473,8 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, assert_vblank_disabled(crtc); drm_crtc_vblank_on(crtc); + intel_crtc->active = true; + intel_encoders_enable(crtc, pipe_config, old_state); if (intel_crtc->config->has_pch_encoder) {
Set the intel_crtc->active flag after pipe/crtc is actually active in haswell_crtc_enable function. Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)