Re: [Intel-gfx] [PATCH 4/7] drm/i915/hdmi: abstract scanline range wait into intel_vblank.[ch]

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  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