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@xxxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/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
Attachment:
signature.asc
Description: PGP signature
