Re: [PATCH v3 18/26] drm/i915/writeback: Define writeback frame capture function

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

On Wed, Mar 25, 2026 at 04:37:36PM +0530, Suraj Kandpal wrote:
> Define the commit function to be called at atomic_commit_tail
> if drm_writeback_job is available. This function calls the
> capture function and queues the job to be called later via
> interrupt handler when the job is complete.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  |  3 +
>  .../gpu/drm/i915/display/intel_writeback.c    | 58 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_writeback.h    |  4 ++
>  3 files changed, 65 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index d433ffaadd65..4cc3e0779e8a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -126,6 +126,7 @@
>  #include "intel_vga.h"
>  #include "intel_vrr.h"
>  #include "intel_wm.h"
> +#include "intel_writeback.h"
>  #include "skl_scaler.h"
>  #include "skl_universal_plane.h"
>  #include "skl_watermark.h"
> @@ -7564,6 +7565,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	/* FIXME probably need to sequence this properly */
>  	intel_program_dpkgc_latency(state);
>  
> +	intel_writeback_atomic_commit(state);
> +
>  	intel_wait_for_vblank_workers(state);
>  
>  	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
> index d45d5faaf7cc..c79e7330b81c 100644
> --- a/drivers/gpu/drm/i915/display/intel_writeback.c
> +++ b/drivers/gpu/drm/i915/display/intel_writeback.c
> @@ -51,6 +51,12 @@ static const u32 writeback_formats[] = {
>  	DRM_FORMAT_XBGR2101010,
>  };
>  
> +static struct intel_writeback_connector
> +*conn_to_intel_writeback_connector(struct intel_connector *connector)
> +{
> +	return container_of(connector, struct intel_writeback_connector, connector);
> +}
> +
>  static struct intel_writeback_connector
>  *enc_to_intel_writeback_connector(struct intel_encoder *encoder)
>  {
> @@ -224,6 +230,58 @@ static int intel_writeback_atomic_check(struct drm_connector *connector,
>  	return 0;
>  }
>  
> +static void intel_writeback_capture(struct intel_atomic_state *state,
> +				    struct intel_connector *connector)
> +{
> +	struct intel_display *display = to_intel_display(connector);
> +	struct intel_writeback_connector *wb_conn =
> +		conn_to_intel_writeback_connector(connector);
> +	enum transcoder trans = wb_conn->trans;
> +	u32 val = 0;
> +
> +	val |= START_TRIGGER_FRAME | WD_FRAME_NUMBER(wb_conn->frame_num);
> +	intel_de_rmw(display, WD_TRANS_FUNC_CTL(trans),
> +		     START_TRIGGER_FRAME | WD_FRAME_NUMBER_MASK,
> +		     val);
> +
> +	if (intel_de_wait_for_set_ms(display, WD_FRAME_STATUS(trans),
> +				     WD_FRAME_COMPLETE, 50)) {

I think we need to hook up the interrupts to avoid this kind of thing.

The trigger we should probably just do from intel_pipe_update_end().

> +		drm_dbg_kms(display->drm,
> +			    "Frame was not captured after triggering a capture\n");
> +		intel_de_rmw(display, WD_TRANS_FUNC_CTL(trans),
> +			     STOP_TRIGGER_FRAME,
> +			     STOP_TRIGGER_FRAME);
> +	} else {
> +		drm_writeback_signal_completion(&connector->base, 0);
> +		intel_de_write(display, WD_FRAME_STATUS(trans), WD_FRAME_COMPLETE);
> +		wb_conn->frame_num++;
> +		if (wb_conn->frame_num > 7)
> +			wb_conn->frame_num = 1;
> +		wb_conn->job = NULL;
> +	}
> +}
> +
> +void intel_writeback_atomic_commit(struct intel_atomic_state *state)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(&state->base, connector, conn_state, i) {
> +		struct intel_connector *intel_connector = to_intel_connector(connector);
> +
> +		if (!conn_state)
> +			return;
> +
> +		if (conn_state->writeback_job && conn_state->writeback_job->fb) {
> +			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> +
> +			drm_writeback_queue_job(connector, conn_state);
> +			intel_writeback_capture(state, intel_connector);
> +		}
> +	}
> +}
> +
>  static void intel_writeback_enable_encoder(struct intel_atomic_state *state,
>  					   struct intel_encoder *encoder,
>  					   const struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback.h b/drivers/gpu/drm/i915/display/intel_writeback.h
> index 5911684cb81a..3c145cf73e20 100644
> --- a/drivers/gpu/drm/i915/display/intel_writeback.h
> +++ b/drivers/gpu/drm/i915/display/intel_writeback.h
> @@ -8,10 +8,14 @@
>  
>  #include <linux/types.h>
>  
> +#include "intel_display_types.h"
> +
> +struct intel_atomic_state;
>  struct intel_display;
>  struct intel_writeback_connector;
>  
>  int intel_writeback_init(struct intel_display *display);
> +void intel_writeback_atomic_commit(struct intel_atomic_state *state);
>  
>  #endif /* __INTEL_WRITEBACK_H__ */
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel