The writeback capture code was using the job to get access to various information about the capture. However, intel_writeback_capture() is called immediately after drm_writeback_queue_job(). And the description for the DRM function explicitly says it clears the job pointer. To work around this, it looks like the code was trying to save the job pointer in the Intel specific writeback structure. Unfortunately, this was broken - the pointer was cleared but never actually set in the first place. Thus, the capture code hit a null pointer derefence and died.
The capture function is the only place that needs this saved job pointer and it is the call immediately after the drm job queue call. So, it is much simpler to just save the pointer locally and pass it in to the capture code as a parameter. There was also a check in the ISR that the saved job pointer has been initialised. However, the writeback ISR runs for multiple events, some of which are before the above juggling of the job pointer. The check seems redundant as the ISR code itself does not touch the job pointer at all. So just remove that check completely. As to whether the writeback job is the best place for storing the pixel format and vma of the writeback buffer is another question entirely. Maybe they should be saved elsewhere and the juggling of the job pointer would not be necessary at all? Signed-off-by: John Harrison <[email protected]> --- .../gpu/drm/i915/display/intel_writeback.c | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c index 52c2d8b91aff..2fd49405c0b6 100644 --- a/drivers/gpu/drm/i915/display/intel_writeback.c +++ b/drivers/gpu/drm/i915/display/intel_writeback.c @@ -32,7 +32,6 @@ struct intel_writeback_connector { struct intel_connector connector; struct intel_encoder encoder; - struct intel_writeback_job *job; enum transcoder trans; enum pipe pipe; int frame_num; @@ -260,25 +259,22 @@ get_color_mode_bpp(struct intel_display *display, u32 color_format) } static void intel_writeback_capture(struct intel_atomic_state *state, - struct intel_connector *connector) + struct intel_connector *connector, + struct intel_writeback_job *job) { struct intel_display *display = to_intel_display(connector); struct intel_writeback_connector *wb_conn = conn_to_intel_writeback_connector(connector); - struct drm_connector_state *conn_state = - drm_atomic_get_new_connector_state(&state->base, &connector->base); struct intel_crtc *crtc = intel_crtc_for_pipe(display, wb_conn->pipe); struct intel_crtc_state *crtc_state = intel_atomic_get_new_crtc_state(state, crtc); const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; - struct drm_writeback_job *wb_job = conn_state->writeback_job; - struct intel_writeback_job *job = conn_state->writeback_job->priv; enum transcoder trans = wb_conn->trans; u32 val = 0; int bpp; - bpp = get_color_mode_bpp(display, wb_job->fb->format->format); + bpp = get_color_mode_bpp(display, job->fb->format->format); val = DIV_ROUND_UP((adjusted_mode->hdisplay * bpp), 64); intel_de_write(display, WD_STRIDE(trans), WD_STRIDE_VAL(val)); @@ -304,7 +300,6 @@ static void intel_writeback_capture(struct intel_atomic_state *state, wb_conn->frame_num++; if (wb_conn->frame_num > 7) wb_conn->frame_num = 1; - wb_conn->job = NULL; } } @@ -321,10 +316,12 @@ void intel_writeback_atomic_commit(struct intel_atomic_state *state) return; if (conn_state->writeback_job && conn_state->writeback_job->fb) { + struct intel_writeback_job *wb_job = conn_state->writeback_job->priv; + WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK); drm_writeback_queue_job(connector, conn_state); - intel_writeback_capture(state, intel_connector); + intel_writeback_capture(state, intel_connector, wb_job); } } } @@ -352,7 +349,7 @@ static void intel_writeback_enable_encoder(struct intel_atomic_state *state, struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct intel_writeback_connector *wb_conn = enc_to_intel_writeback_connector(encoder); - struct intel_writeback_job *job = wb_conn->job; + struct intel_writeback_job *job; const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; enum transcoder trans = crtc_state->cpu_transcoder; struct intel_crtc *pipe_crtc; @@ -363,6 +360,7 @@ static void intel_writeback_enable_encoder(struct intel_atomic_state *state, if (!conn_state->writeback_job) return; + job = conn_state->writeback_job->priv; wb_conn->trans = trans; wb_conn->pipe = crtc->pipe; fb = job->fb; @@ -542,11 +540,6 @@ void intel_writeback_isr_handler(struct intel_display *display) continue; wb_conn = enc_to_intel_writeback_connector(encoder); - if (!wb_conn->job) { - drm_err(display->drm, "No writeback job for the connector\n"); - continue; - } - crtc = intel_crtc_for_pipe(display, wb_conn->pipe); iir = intel_de_read(display, WD_IIR(wb_conn->trans)); if (iir & WD_GTT_FAULT_INT) -- 2.43.0
