| Message ID | 20230509111441.4293-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state | expand |
On Tue, May 09, 2023 at 02:14:41PM +0300, Stanislav Lisovskiy wrote: > There is a suspicion that we might not have all bigjoiner pipes > set in correspodent mask, which leads to that not all crtc are added to the state, > however because we are copying for instance crtc reference from master crtc > to slave crtc, we might be trying to get it via intel_atomic_get_new_crtc_state, > which might the return NULL. > This is surely not a fix, but at least the WARN should give us some clue and > "red light" when this happens. > In future we might need to evaluate the logic of adding crtc to the state, > to make sure that we always have all affected crtcs in the state, > even though such functions already exist, there seem to be still some > glitches in this logic. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 13 +++++++++++++ > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > drivers/gpu/drm/i915/display/intel_display.h | 1 + > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index 4125ee07a271..03cbd755261b 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -695,6 +695,19 @@ int intel_plane_atomic_check(struct intel_atomic_state *state, > > new_master_plane_state = > intel_atomic_get_new_plane_state(state, master_plane); > + > + /* > + * We would be copying plane state from master crtc > + * however if crtc_state->bigjoiner_pipes doesn't contain both > + * master and slave, that means that quite likely we didn't call > + * intel_atomic_get_crtc_state for both, which can cause issues, > + * like intel_atomic_get_new_crtc_state returning NULL suddently, > + * when we for example try to use hw.crtc from that plane state. > + * This WARN might sched some light on out existing issues, also > + * prevent others from happening in future. > + */ > + drm_WARN_ON(state->base.dev, intel_bigjoiner_num_pipes(new_crtc_state) < 2); What you are doing here is basically just if (bigjoiner_pipes) assert(bigjoiner_pipes != 1); which is not going to catch anything. We can trivially see that it will never happen given how bigjoiner_pipes is initialized.
On Wed, May 10, 2023 at 04:47:57PM +0300, Ville Syrjälä wrote: > On Tue, May 09, 2023 at 02:14:41PM +0300, Stanislav Lisovskiy wrote: > > There is a suspicion that we might not have all bigjoiner pipes > > set in correspodent mask, which leads to that not all crtc are added to the state, > > however because we are copying for instance crtc reference from master crtc > > to slave crtc, we might be trying to get it via intel_atomic_get_new_crtc_state, > > which might the return NULL. > > This is surely not a fix, but at least the WARN should give us some clue and > > "red light" when this happens. > > In future we might need to evaluate the logic of adding crtc to the state, > > to make sure that we always have all affected crtcs in the state, > > even though such functions already exist, there seem to be still some > > glitches in this logic. > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 13 +++++++++++++ > > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > > drivers/gpu/drm/i915/display/intel_display.h | 1 + > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > index 4125ee07a271..03cbd755261b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -695,6 +695,19 @@ int intel_plane_atomic_check(struct intel_atomic_state *state, > > > > new_master_plane_state = > > intel_atomic_get_new_plane_state(state, master_plane); > > + > > + /* > > + * We would be copying plane state from master crtc > > + * however if crtc_state->bigjoiner_pipes doesn't contain both > > + * master and slave, that means that quite likely we didn't call > > + * intel_atomic_get_crtc_state for both, which can cause issues, > > + * like intel_atomic_get_new_crtc_state returning NULL suddently, > > + * when we for example try to use hw.crtc from that plane state. > > + * This WARN might sched some light on out existing issues, also > > + * prevent others from happening in future. > > + */ > > + drm_WARN_ON(state->base.dev, intel_bigjoiner_num_pipes(new_crtc_state) < 2); > > What you are doing here is basically just > if (bigjoiner_pipes) > assert(bigjoiner_pipes != 1); > which is not going to catch anything. > > We can trivially see that it will never happen given > how bigjoiner_pipes is initialized. intel_bigjoiner_num_pipes counts how many pipe are used, as I understand in case of bigjoiner we should always have it returning 2, right? For example intel_bigjoiner_adjust_timings uses it currently same way in our code, also I think there other places: static void intel_bigjoiner_adjust_timings(const struct intel_crtc_state *crtc_state, struct drm_display_mode *mode) { int num_pipes = intel_bigjoiner_num_pipes(crtc_state); if (num_pipes < 2) return; ... I was just willing to check if, we might have this mask messed up somehow, because we add crtcs to the state based on this mask. However I already checked, problem seems to be somewhere else. Stan > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index 4125ee07a271..03cbd755261b 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -695,6 +695,19 @@ int intel_plane_atomic_check(struct intel_atomic_state *state, new_master_plane_state = intel_atomic_get_new_plane_state(state, master_plane); + + /* + * We would be copying plane state from master crtc + * however if crtc_state->bigjoiner_pipes doesn't contain both + * master and slave, that means that quite likely we didn't call + * intel_atomic_get_crtc_state for both, which can cause issues, + * like intel_atomic_get_new_crtc_state returning NULL suddently, + * when we for example try to use hw.crtc from that plane state. + * This WARN might sched some light on out existing issues, also + * prevent others from happening in future. + */ + drm_WARN_ON(state->base.dev, intel_bigjoiner_num_pipes(new_crtc_state) < 2); + } else { new_master_plane_state = new_plane_state; } diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 1d5d42a40803..0e0908c1ee8d 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -267,7 +267,7 @@ bool intel_crtc_is_bigjoiner_master(const struct intel_crtc_state *crtc_state) crtc->pipe == bigjoiner_master_pipe(crtc_state); } -static int intel_bigjoiner_num_pipes(const struct intel_crtc_state *crtc_state) +int intel_bigjoiner_num_pipes(const struct intel_crtc_state *crtc_state) { return hweight8(crtc_state->bigjoiner_pipes); } diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h index ac95961f68ba..94761be69bd0 100644 --- a/drivers/gpu/drm/i915/display/intel_display.h +++ b/drivers/gpu/drm/i915/display/intel_display.h @@ -410,6 +410,7 @@ bool is_trans_port_sync_mode(const struct intel_crtc_state *state); bool intel_crtc_is_bigjoiner_slave(const struct intel_crtc_state *crtc_state); bool intel_crtc_is_bigjoiner_master(const struct intel_crtc_state *crtc_state); u8 intel_crtc_bigjoiner_slave_pipes(const struct intel_crtc_state *crtc_state); +int intel_bigjoiner_num_pipes(const struct intel_crtc_state *crtc_state); struct intel_crtc *intel_master_crtc(const struct intel_crtc_state *crtc_state); bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state); bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
There is a suspicion that we might not have all bigjoiner pipes set in correspodent mask, which leads to that not all crtc are added to the state, however because we are copying for instance crtc reference from master crtc to slave crtc, we might be trying to get it via intel_atomic_get_new_crtc_state, which might the return NULL. This is surely not a fix, but at least the WARN should give us some clue and "red light" when this happens. In future we might need to evaluate the logic of adding crtc to the state, to make sure that we always have all affected crtcs in the state, even though such functions already exist, there seem to be still some glitches in this logic. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_atomic_plane.c | 13 +++++++++++++ drivers/gpu/drm/i915/display/intel_display.c | 2 +- drivers/gpu/drm/i915/display/intel_display.h | 1 + 3 files changed, 15 insertions(+), 1 deletion(-)