On Mon, Dec 12, 2022 at 04:29:22PM +0200, Jani Nikula wrote:
> Let's not have scanline waits inline in hdmi code.
>
> This kind of waits should really have timeouts; add a FIXME comment.
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 9 ++-------
> drivers/gpu/drm/i915/display/intel_vblank.c | 14 ++++++++++++++
> drivers/gpu/drm/i915/display/intel_vblank.h | 1 +
> 3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index e82f8a07e2b0..af6ef665368e 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -56,6 +56,7 @@
> #include "intel_lspcon.h"
> #include "intel_panel.h"
> #include "intel_snps_phy.h"
> +#include "intel_vblank.h"
>
> static struct drm_i915_private *intel_hdmi_to_i915(struct intel_hdmi *intel_hdmi)
> {
> @@ -1476,15 +1477,9 @@ static int kbl_repositioning_enc_en_signal(struct intel_connector *connector,
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> struct intel_digital_port *dig_port = intel_attached_dig_port(connector);
> struct intel_crtc *crtc = to_intel_crtc(connector->base.state->crtc);
> - u32 scanline;
> int ret;
>
> - for (;;) {
> - scanline = intel_de_read(dev_priv, PIPEDSL(crtc->pipe));
> - if (scanline > 100 && scanline < 200)
> - break;
> - usleep_range(25, 50);
> - }
> + intel_wait_for_pipe_scanline_range(crtc, 100, 200);
>
> ret = intel_ddi_toggle_hdcp_bits(&dig_port->base, cpu_transcoder,
> false, TRANS_DDI_HDCP_SIGNALLING);
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index f25ec643a0a3..aec7758ef917 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -452,3 +452,17 @@ void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc)
> {
> wait_for_pipe_scanline_moving(crtc, true);
> }
> +
> +void intel_wait_for_pipe_scanline_range(struct intel_crtc *crtc, u32 start, u32 end)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + u32 scanline;
> +
> + /* FIXME: This needs to timeout and/or check that scanline is moving. */
> + for (;;) {
> + scanline = intel_de_read(dev_priv, PIPEDSL(crtc->pipe));
If we want to advertise this as any kind of official thing then
it should really use intel_get_crtc_scanline() which eg. accounts
for crtc->scanline_offset.
But I have no solid idea what this thing is even trying to do.
There is one workaround I see that says some hdcp bits must be
toggled during a single frame, and if this is that then it
should probably be rewritten to make proper sense, or at least
it should have a comment stating what the heck it's trying to do.
> + if (scanline > start && scanline < end)
> + break;
> + usleep_range(25, 50);
> + }
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.h b/drivers/gpu/drm/i915/display/intel_vblank.h
> index 54870cabd734..e88addfccea8 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.h
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.h
> @@ -19,5 +19,6 @@ bool intel_crtc_get_vblank_timestamp(struct drm_crtc *crtc, int *max_error,
> int intel_get_crtc_scanline(struct intel_crtc *crtc);
> void intel_wait_for_pipe_scanline_stopped(struct intel_crtc *crtc);
> void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc);
> +void intel_wait_for_pipe_scanline_range(struct intel_crtc *crtc, u32 start, u32 end);
>
> #endif /* __INTEL_VBLANK_H__ */
> --
> 2.34.1
--
Ville Syrjälä
Intel
