Re: [RFC PATCH] drm: allow encoder mode_set even when connectors change for crtc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux