| Message ID | 20240905221124.2587271-1-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [RFC] drm: allow encoder mode_set even when connectors change for crtc | expand |
Hi, On Thu, Sep 05, 2024 at 03:11:24PM GMT, Abhinav Kumar wrote: > In certain use-cases, a CRTC could switch between two encoders > and because the mode being programmed on the CRTC remains > the same during this switch, the CRTC's mode_changed remains false. > In such cases, the encoder's mode_set also gets skipped. > > Skipping mode_set on the encoder for such cases could cause an issue > because even though the same CRTC mode was being used, the encoder > type could have changed like the CRTC could have switched from a > real time encoder to a writeback encoder OR vice-versa. > > Allow encoder's mode_set to happen even when connectors changed on a > CRTC and not just when the mode changed. > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> The patch and rationale looks sane to me, but we should really add kunit tests for that scenarii. Maxime
Hi Maxime On 9/9/2024 6:37 AM, Maxime Ripard wrote: > Hi, > > On Thu, Sep 05, 2024 at 03:11:24PM GMT, Abhinav Kumar wrote: >> In certain use-cases, a CRTC could switch between two encoders >> and because the mode being programmed on the CRTC remains >> the same during this switch, the CRTC's mode_changed remains false. >> In such cases, the encoder's mode_set also gets skipped. >> >> Skipping mode_set on the encoder for such cases could cause an issue >> because even though the same CRTC mode was being used, the encoder >> type could have changed like the CRTC could have switched from a >> real time encoder to a writeback encoder OR vice-versa. >> >> Allow encoder's mode_set to happen even when connectors changed on a >> CRTC and not just when the mode changed. >> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > The patch and rationale looks sane to me, but we should really add kunit > tests for that scenarii. > Thanks for the review. We have a IGT for recreating this scenario and thats how this issue was captured kms_writeback --run-subtest writeback-check-output -c <primary display mode> We had added an option ( 'c' - custom mode) a couple of yrs ago to allow writeback to be tested using any mode the user passes in (https://lore.kernel.org/r/all/YuJhGkkxah9U6FGx@platvala-desk.ger.corp.intel.com/T/) If we pass in the same resolution as the primary RT display, this scenario always happens as the CRTC switches between RT encoder and WB encoder. Hope that addresses some of the concern. Regarding KUnit tests, I have a couple of questions: 1) This is more of a run-time scenario where CRTC switch happens, does this qualify for a KUnit or perhaps I am missing something. 2) Is there any existing KUnit test file under drm/tests for validating drm_atomic_helper_commit_modeset_disables() / drm_atomic_helper_commit_modeset_enables() path because this will fall under that bucket. I didnt find any matching case where we can extend this. Thanks Abhinav > Maxime
Hi, On Mon, Sep 09, 2024 at 12:59:47PM GMT, Abhinav Kumar wrote: > On 9/9/2024 6:37 AM, Maxime Ripard wrote: > > Hi, > > > > On Thu, Sep 05, 2024 at 03:11:24PM GMT, Abhinav Kumar wrote: > > > In certain use-cases, a CRTC could switch between two encoders > > > and because the mode being programmed on the CRTC remains > > > the same during this switch, the CRTC's mode_changed remains false. > > > In such cases, the encoder's mode_set also gets skipped. > > > > > > Skipping mode_set on the encoder for such cases could cause an issue > > > because even though the same CRTC mode was being used, the encoder > > > type could have changed like the CRTC could have switched from a > > > real time encoder to a writeback encoder OR vice-versa. > > > > > > Allow encoder's mode_set to happen even when connectors changed on a > > > CRTC and not just when the mode changed. > > > > > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > > > The patch and rationale looks sane to me, but we should really add kunit > > tests for that scenarii. > > > > Thanks for the review. > > We have a IGT for recreating this scenario and thats how this issue was > captured > > kms_writeback --run-subtest writeback-check-output -c <primary display mode> > > We had added an option ( 'c' - custom mode) a couple of yrs ago to allow > writeback to be tested using any mode the user passes in (https://lore.kernel.org/r/all/YuJhGkkxah9U6FGx@platvala-desk.ger.corp.intel.com/T/) > > If we pass in the same resolution as the primary RT display, this scenario > always happens as the CRTC switches between RT encoder and WB encoder. Hope > that addresses some of the concern. Unless it can easily be run in some sort of CI loop by anyone contributing to that part of the kernel, it doesn't. Don't get me wrong, it's a great feature, but it doesn't help making sure that issue never creeps back in. > Regarding KUnit tests, I have a couple of questions: > > 1) This is more of a run-time scenario where CRTC switch happens, does this > qualify for a KUnit or perhaps I am missing something. We've been using kunit to perform integration tests in the kernel too, so I would say that it definitely qualifies. > 2) Is there any existing KUnit test file under drm/tests for validating > drm_atomic_helper_commit_modeset_disables() / > drm_atomic_helper_commit_modeset_enables() path because this will fall under > that bucket. I didnt find any matching case where we can extend this. We don't have that at the moment, but we shouldn't be too far off. The HDMI framework I contributed some months ago for example has all the mode checking infrastructure in kunit. So you already have some way to create a driver, a new state, modify that state and check it. The only thing missing in your case is being able to commit it and check that it has run, which shouldn't be too hard Maxime
Hi Maxime On 9/10/2024 1:40 AM, Maxime Ripard wrote: > Hi, > > On Mon, Sep 09, 2024 at 12:59:47PM GMT, Abhinav Kumar wrote: >> On 9/9/2024 6:37 AM, Maxime Ripard wrote: >>> Hi, >>> >>> On Thu, Sep 05, 2024 at 03:11:24PM GMT, Abhinav Kumar wrote: >>>> In certain use-cases, a CRTC could switch between two encoders >>>> and because the mode being programmed on the CRTC remains >>>> the same during this switch, the CRTC's mode_changed remains false. >>>> In such cases, the encoder's mode_set also gets skipped. >>>> >>>> Skipping mode_set on the encoder for such cases could cause an issue >>>> because even though the same CRTC mode was being used, the encoder >>>> type could have changed like the CRTC could have switched from a >>>> real time encoder to a writeback encoder OR vice-versa. >>>> >>>> Allow encoder's mode_set to happen even when connectors changed on a >>>> CRTC and not just when the mode changed. >>>> >>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> >>> The patch and rationale looks sane to me, but we should really add kunit >>> tests for that scenarii. >>> >> >> Thanks for the review. >> >> We have a IGT for recreating this scenario and thats how this issue was >> captured >> >> kms_writeback --run-subtest writeback-check-output -c <primary display mode> >> >> We had added an option ( 'c' - custom mode) a couple of yrs ago to allow >> writeback to be tested using any mode the user passes in (https://lore.kernel.org/r/all/YuJhGkkxah9U6FGx@platvala-desk.ger.corp.intel.com/T/) >> >> If we pass in the same resolution as the primary RT display, this scenario >> always happens as the CRTC switches between RT encoder and WB encoder. Hope >> that addresses some of the concern. > > Unless it can easily be run in some sort of CI loop by anyone > contributing to that part of the kernel, it doesn't. > > Don't get me wrong, it's a great feature, but it doesn't help making > sure that issue never creeps back in. > Ack, I understand. >> Regarding KUnit tests, I have a couple of questions: >> >> 1) This is more of a run-time scenario where CRTC switch happens, does this >> qualify for a KUnit or perhaps I am missing something. > > We've been using kunit to perform integration tests in the kernel too, > so I would say that it definitely qualifies. > >> 2) Is there any existing KUnit test file under drm/tests for validating >> drm_atomic_helper_commit_modeset_disables() / >> drm_atomic_helper_commit_modeset_enables() path because this will fall under >> that bucket. I didnt find any matching case where we can extend this. > > We don't have that at the moment, but we shouldn't be too far off. The > HDMI framework I contributed some months ago for example has all the > mode checking infrastructure in kunit. So you already have some way to > create a driver, a new state, modify that state and check it. > > The only thing missing in your case is being able to commit it and check > that it has run, which shouldn't be too hard > > Maxime Alright. Yes I reviewed the hdmi infrastructure tests and you seem to have most of the pieces. I just need to find some cycles to work on this :) so you can have my name down for it and either me one of our team members or perhaps with some help from other msm developers we can get it added. The reason I was hoping to get this reviewed and added as a "fix" was we had already run into this scenario with kms_writeback test case and the same scenario was seen in another msm bug https://gitlab.freedesktop.org/drm/msm/-/issues/59 leading to a null ptr crash but we ended up fixing that within msm because that was a better fix anyway so I was thinking this change would help to resolve these types of issues for us once for all. But if this needs to wait for the KUnit to be added, thats fine, we will resend this one along with the KUnit once we work on it.
Hi, On Wed, Sep 11, 2024 at 05:54:44PM GMT, Abhinav Kumar wrote: > On 9/10/2024 1:40 AM, Maxime Ripard wrote: > > Hi, > > > > On Mon, Sep 09, 2024 at 12:59:47PM GMT, Abhinav Kumar wrote: > > > On 9/9/2024 6:37 AM, Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Thu, Sep 05, 2024 at 03:11:24PM GMT, Abhinav Kumar wrote: > > > > > In certain use-cases, a CRTC could switch between two encoders > > > > > and because the mode being programmed on the CRTC remains > > > > > the same during this switch, the CRTC's mode_changed remains false. > > > > > In such cases, the encoder's mode_set also gets skipped. > > > > > > > > > > Skipping mode_set on the encoder for such cases could cause an issue > > > > > because even though the same CRTC mode was being used, the encoder > > > > > type could have changed like the CRTC could have switched from a > > > > > real time encoder to a writeback encoder OR vice-versa. > > > > > > > > > > Allow encoder's mode_set to happen even when connectors changed on a > > > > > CRTC and not just when the mode changed. > > > > > > > > > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > > > > > > > The patch and rationale looks sane to me, but we should really add kunit > > > > tests for that scenarii. > > > > > > > > > > Thanks for the review. > > > > > > We have a IGT for recreating this scenario and thats how this issue was > > > captured > > > > > > kms_writeback --run-subtest writeback-check-output -c <primary display mode> > > > > > > We had added an option ( 'c' - custom mode) a couple of yrs ago to allow > > > writeback to be tested using any mode the user passes in (https://lore.kernel.org/r/all/YuJhGkkxah9U6FGx@platvala-desk.ger.corp.intel.com/T/) > > > > > > If we pass in the same resolution as the primary RT display, this scenario > > > always happens as the CRTC switches between RT encoder and WB encoder. Hope > > > that addresses some of the concern. > > > > Unless it can easily be run in some sort of CI loop by anyone > > contributing to that part of the kernel, it doesn't. > > > > Don't get me wrong, it's a great feature, but it doesn't help making > > sure that issue never creeps back in. > > > > Ack, I understand. > > > > Regarding KUnit tests, I have a couple of questions: > > > > > > 1) This is more of a run-time scenario where CRTC switch happens, does this > > > qualify for a KUnit or perhaps I am missing something. > > > > We've been using kunit to perform integration tests in the kernel too, > > so I would say that it definitely qualifies. > > > > > 2) Is there any existing KUnit test file under drm/tests for validating > > > drm_atomic_helper_commit_modeset_disables() / > > > drm_atomic_helper_commit_modeset_enables() path because this will fall under > > > that bucket. I didnt find any matching case where we can extend this. > > > > We don't have that at the moment, but we shouldn't be too far off. The > > HDMI framework I contributed some months ago for example has all the > > mode checking infrastructure in kunit. So you already have some way to > > create a driver, a new state, modify that state and check it. > > > > The only thing missing in your case is being able to commit it and check > > that it has run, which shouldn't be too hard > > Alright. Yes I reviewed the hdmi infrastructure tests and you seem to have > most of the pieces. I just need to find some cycles to work on this :) so > you can have my name down for it and either me one of our team members or > perhaps with some help from other msm developers we can get it added. > > The reason I was hoping to get this reviewed and added as a "fix" was we had > already run into this scenario with kms_writeback test case and the same > scenario was seen in another msm bug > https://gitlab.freedesktop.org/drm/msm/-/issues/59 leading to a null ptr > crash but we ended up fixing that within msm because that was a better fix > anyway so I was thinking this change would help to resolve these types of > issues for us once for all. > > But if this needs to wait for the KUnit to be added, thats fine, we will > resend this one along with the KUnit once we work on it. Yeah, if it's not urgent I'd rather have the kunit test at the same time. Maxime
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fb97b51b38f1..8dc50dd2481d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1376,7 +1376,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) mode = &new_crtc_state->mode; adjusted_mode = &new_crtc_state->adjusted_mode; - if (!new_crtc_state->mode_changed) + if (!new_crtc_state->mode_changed && !new_crtc_state->connectors_changed) continue; drm_dbg_atomic(dev, "modeset on [ENCODER:%d:%s]\n",
In certain use-cases, a CRTC could switch between two encoders and because the mode being programmed on the CRTC remains the same during this switch, the CRTC's mode_changed remains false. In such cases, the encoder's mode_set also gets skipped. Skipping mode_set on the encoder for such cases could cause an issue because even though the same CRTC mode was being used, the encoder type could have changed like the CRTC could have switched from a real time encoder to a writeback encoder OR vice-versa. Allow encoder's mode_set to happen even when connectors changed on a CRTC and not just when the mode changed. Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/drm_atomic_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)