> Subject: Re: [PATCH v3 18/26] drm/i915/writeback: Define writeback frame
> capture function
>
> 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 <[email protected]>
> > ---
> > 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().
I do hook up interrupts in later patches but I have found the writeback job
done interrupt to be a little unreliable.
Could try this out and see if it works out.
Regards,
Suraj Kandpal
>
> > + 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
