| Message ID | 20200720113117.16131-3-karthik.b.s@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Asynchronous flip implementation for i915 | expand |
Em seg, 2020-07-20 às 17:01 +0530, Karthik B S escreveu: > Set the Async Address Update Enable bit in plane ctl > when async flip is requested. > > v2: -Move the Async flip enablement to individual patch (Paulo) > > v3: -Rebased. > > v4: -Add separate plane hook for async flip case (Ville) > > v5: -Rebased. > > Signed-off-by: Karthik B S <karthik.b.s@intel.com> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 6 +++++ > drivers/gpu/drm/i915/display/intel_sprite.c | 25 ++++++++++++++++++++ > drivers/gpu/drm/i915/i915_reg.h | 1 + > 3 files changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index b8ff032195d9..4773f39e5924 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -4766,6 +4766,12 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state, > const struct drm_intel_sprite_colorkey *key = &plane_state->ckey; > u32 plane_ctl; > > + /* During Async flip, no other updates are allowed */ My understanding is that this function is fully setting the right bits based on the chosen config (instead of doing read-modify-write), and the checks for "other updates" were done before. So the logic implemented here of early returning doesn't make sense. > + if (crtc_state->uapi.async_flip) { > + plane_ctl |= PLANE_CTL_ASYNC_FLIP; I wonder why gcc does not complain we're ORing with an unitialized value. > + return plane_ctl; > + } > + > plane_ctl = PLANE_CTL_ENABLE; It seems to be the return above means we'll never even try to enable the plane, we're only relying on the fact that plane_ctl is not zero initialize so maybe bit 31 is already set. > > if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) { > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > index c26ca029fc0a..3747482e8fa3 100644 > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > @@ -603,6 +603,24 @@ icl_program_input_csc(struct intel_plane *plane, > PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0); > } > > +static void > +skl_program_async_surface_address(struct drm_i915_private *dev_priv, > + const struct intel_plane_state *plane_state, > + enum pipe pipe, enum plane_id plane_id, > + u32 surf_addr) > +{ > + unsigned long irqflags; > + u32 plane_ctl = plane_state->ctl; > + > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > + > + intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl); > + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), > + intel_plane_ggtt_offset(plane_state) + surf_addr); > + > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > +} > + > static void > skl_program_plane(struct intel_plane *plane, > const struct intel_crtc_state *crtc_state, > @@ -631,6 +649,13 @@ skl_program_plane(struct intel_plane *plane, > u32 keymsk, keymax; > u32 plane_ctl = plane_state->ctl; > > + /* During Async flip, no other updates are allowed */ > + if (crtc_state->uapi.async_flip) { > + skl_program_async_surface_address(dev_priv, plane_state, > + pipe, plane_id, surf_addr); > + return; > + } I'd vote for us to keep the "don't rewrite registers that shouldn't change" part on its own commit, since it's just an optimization. It could even go at the end of the series. But perhaps this is simple enough and not needed. > + > plane_ctl |= skl_plane_ctl_crtc(crtc_state); > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 8cee06314d5d..19aad4199874 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6935,6 +6935,7 @@ enum { > #define PLANE_CTL_TILED_X (1 << 10) > #define PLANE_CTL_TILED_Y (4 << 10) > #define PLANE_CTL_TILED_YF (5 << 10) > +#define PLANE_CTL_ASYNC_FLIP (1 << 9) > #define PLANE_CTL_FLIP_HORIZONTAL (1 << 8) > #define PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE (1 << 4) /* TGL+ */ > #define PLANE_CTL_ALPHA_MASK (0x3 << 4) /* Pre-GLK */
On 7/25/2020 4:56 AM, Paulo Zanoni wrote: > Em seg, 2020-07-20 às 17:01 +0530, Karthik B S escreveu: >> Set the Async Address Update Enable bit in plane ctl >> when async flip is requested. >> >> v2: -Move the Async flip enablement to individual patch (Paulo) >> >> v3: -Rebased. >> >> v4: -Add separate plane hook for async flip case (Ville) >> >> v5: -Rebased. >> >> Signed-off-by: Karthik B S <karthik.b.s@intel.com> >> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 6 +++++ >> drivers/gpu/drm/i915/display/intel_sprite.c | 25 ++++++++++++++++++++ >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> 3 files changed, 32 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index b8ff032195d9..4773f39e5924 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -4766,6 +4766,12 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state, >> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey; >> u32 plane_ctl; >> >> + /* During Async flip, no other updates are allowed */ > > My understanding is that this function is fully setting the right bits > based on the chosen config (instead of doing read-modify-write), and > the checks for "other updates" were done before. So the logic > implemented here of early returning doesn't make sense. > Thanks for the review. Yes the check for other updates are done before. So I could either do read-modify-write and return early, or, keep the existing code flow as is, since the are checks already present. I will keep the existing flow and remove the early return in the next revision. > >> + if (crtc_state->uapi.async_flip) { >> + plane_ctl |= PLANE_CTL_ASYNC_FLIP; > > I wonder why gcc does not complain we're ORing with an unitialized > value. Will initialize the plane_ctl variable to zero. > > >> + return plane_ctl; >> + } >> + >> plane_ctl = PLANE_CTL_ENABLE; > > It seems to be the return above means we'll never even try to enable > the plane, we're only relying on the fact that plane_ctl is not zero > initialize so maybe bit 31 is already set. > Since we only allow async flips on planes that are already enabled, I assumed this would not be needed. Also, other than bit 9 (async address update enable), this register is double buffered and cannot be updated asynchronously. > >> >> if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) { >> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c >> index c26ca029fc0a..3747482e8fa3 100644 >> --- a/drivers/gpu/drm/i915/display/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c >> @@ -603,6 +603,24 @@ icl_program_input_csc(struct intel_plane *plane, >> PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0); >> } >> >> +static void >> +skl_program_async_surface_address(struct drm_i915_private *dev_priv, >> + const struct intel_plane_state *plane_state, >> + enum pipe pipe, enum plane_id plane_id, >> + u32 surf_addr) >> +{ >> + unsigned long irqflags; >> + u32 plane_ctl = plane_state->ctl; >> + >> + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); >> + >> + intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl); >> + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), >> + intel_plane_ggtt_offset(plane_state) + surf_addr); >> + >> + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); >> +} >> + >> static void >> skl_program_plane(struct intel_plane *plane, >> const struct intel_crtc_state *crtc_state, >> @@ -631,6 +649,13 @@ skl_program_plane(struct intel_plane *plane, >> u32 keymsk, keymax; >> u32 plane_ctl = plane_state->ctl; >> >> + /* During Async flip, no other updates are allowed */ >> + if (crtc_state->uapi.async_flip) { >> + skl_program_async_surface_address(dev_priv, plane_state, >> + pipe, plane_id, surf_addr); >> + return; >> + } > > > I'd vote for us to keep the "don't rewrite registers that shouldn't > change" part on its own commit, since it's just an optimization. It > could even go at the end of the series. But perhaps this is simple > enough and not needed. > > Will move this change to the end of the series. Thanks, Karthik.B.S >> + >> plane_ctl |= skl_plane_ctl_crtc(crtc_state); >> >> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 8cee06314d5d..19aad4199874 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -6935,6 +6935,7 @@ enum { >> #define PLANE_CTL_TILED_X (1 << 10) >> #define PLANE_CTL_TILED_Y (4 << 10) >> #define PLANE_CTL_TILED_YF (5 << 10) >> +#define PLANE_CTL_ASYNC_FLIP (1 << 9) >> #define PLANE_CTL_FLIP_HORIZONTAL (1 << 8) >> #define PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE (1 << 4) /* TGL+ */ >> #define PLANE_CTL_ALPHA_MASK (0x3 << 4) /* Pre-GLK */ >
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index b8ff032195d9..4773f39e5924 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -4766,6 +4766,12 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state, const struct drm_intel_sprite_colorkey *key = &plane_state->ckey; u32 plane_ctl; + /* During Async flip, no other updates are allowed */ + if (crtc_state->uapi.async_flip) { + plane_ctl |= PLANE_CTL_ASYNC_FLIP; + return plane_ctl; + } + plane_ctl = PLANE_CTL_ENABLE; if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) { diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c index c26ca029fc0a..3747482e8fa3 100644 --- a/drivers/gpu/drm/i915/display/intel_sprite.c +++ b/drivers/gpu/drm/i915/display/intel_sprite.c @@ -603,6 +603,24 @@ icl_program_input_csc(struct intel_plane *plane, PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0); } +static void +skl_program_async_surface_address(struct drm_i915_private *dev_priv, + const struct intel_plane_state *plane_state, + enum pipe pipe, enum plane_id plane_id, + u32 surf_addr) +{ + unsigned long irqflags; + u32 plane_ctl = plane_state->ctl; + + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); + + intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl); + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), + intel_plane_ggtt_offset(plane_state) + surf_addr); + + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); +} + static void skl_program_plane(struct intel_plane *plane, const struct intel_crtc_state *crtc_state, @@ -631,6 +649,13 @@ skl_program_plane(struct intel_plane *plane, u32 keymsk, keymax; u32 plane_ctl = plane_state->ctl; + /* During Async flip, no other updates are allowed */ + if (crtc_state->uapi.async_flip) { + skl_program_async_surface_address(dev_priv, plane_state, + pipe, plane_id, surf_addr); + return; + } + plane_ctl |= skl_plane_ctl_crtc(crtc_state); if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8cee06314d5d..19aad4199874 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6935,6 +6935,7 @@ enum { #define PLANE_CTL_TILED_X (1 << 10) #define PLANE_CTL_TILED_Y (4 << 10) #define PLANE_CTL_TILED_YF (5 << 10) +#define PLANE_CTL_ASYNC_FLIP (1 << 9) #define PLANE_CTL_FLIP_HORIZONTAL (1 << 8) #define PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE (1 << 4) /* TGL+ */ #define PLANE_CTL_ALPHA_MASK (0x3 << 4) /* Pre-GLK */