On Fri, Mar 19, 2021 at 11:12:41PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 19, 2021 at 01:54:13PM -0700, Navare, Manasi wrote:
> > On Fri, Mar 19, 2021 at 04:56:24PM +0200, Ville Syrjälä wrote:
> > > On Thu, Mar 18, 2021 at 04:01:26PM -0700, Navare, Manasi wrote:
> > > > So basically we see this warning only in case of bigjoiner when
> > > > drm_atomic_check gets called without setting the state->allow_modeset
> > > > flag.
> > >
> > > Considering the code is 'WARN(!state->allow_modeset, ...' that
> > > fact should be rather obvious.
> > >
> > > >
> > > > So do you think that in i915, in intel_atomic_check_bigjoiner() we
> > > > should only
> > > > steal the crtc when allow_modeset flag is set in state?
> > >
> > > No. If you fully read drm_atomic_check_only() you will observe
> > > that it will reject any commit w/ allow_modeset==false which
> > > needs a modeset. And it does that before the WARN.
> > >
> > > So you're barking up the wrong tree here. The problem I think
> > > is that you're just computing requested_crtcs wrong.
> >
> > So here in this case, requested CRTC = 0x1 since it requests modeset on
> > CRTC 0
> > Now in teh atomic check, it steals the slave CRTC 1 and hence affected CRTC
> > comes out
> > as 0x3 and hence the mismatch.
>
> Hmm. How can it be 0x3 if we filtered out the uapi.enable==false case?
>
Yes if I add that condition like in this patch then it correctly calculates
the affected crtc bitmask as only 0x1 since it doesnt include the slave crtc.
So with this patch, requested crtc = 0x 1, affected crtc = 0x1
If this looks good then this fixes our bigjoiner warnings.
Does this patch look good to you as is then?
Manasi
> > Now what is not clear to me is that if a full modeset was not required
> > why did i915 still steal that slave CRTC?
> >
> > Manasi
> >
> > >
> > > > If we add this check there then the affected crtc wont count the slave
> > > > crtc
> > > > and we wont get this warning.
> > > >
> > > > Ville, Danvet?
> > > >
> > > > Manasi
> > > >
> > > >
> > > > On Tue, Mar 16, 2021 at 10:35:09PM +0100, Daniel Vetter wrote:
> > > > > On Tue, Mar 9, 2021 at 10:14 AM Pekka Paalanen <ppaala...@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > On Mon, 8 Mar 2021 16:52:58 -0800
> > > > > > "Navare, Manasi" <manasi.d.nav...@intel.com> wrote:
> > > > > >
> > > > > > > On Thu, Mar 04, 2021 at 10:42:23AM +0200, Pekka Paalanen wrote:
> > > > > > > > On Wed, 3 Mar 2021 12:44:33 -0800
> > > > > > > > "Navare, Manasi" <manasi.d.nav...@intel.com> wrote:
> > > > > > > >
> > > > > > > > > On Wed, Mar 03, 2021 at 10:47:44AM +0200, Pekka Paalanen
> > > > > > > > > wrote:
> > > > > > > > > > On Tue, 2 Mar 2021 12:41:32 -0800
> > > > > > > > > > Manasi Navare <manasi.d.nav...@intel.com> wrote:
> > > > > > > > > >
> > > > > > > > > > > In case of a modeset where a mode gets split across
> > > > > > > > > > > mutiple CRTCs
> > > > > > > > > > > in the driver specific implementation (bigjoiner in i915)
> > > > > > > > > > > we wrongly count
> > > > > > > > > > > the affected CRTCs based on the drm_crtc_mask and
> > > > > > > > > > > indicate the stolen CRTC as
> > > > > > > > > > > an affected CRTC in atomic_check_only().
> > > > > > > > > > > This triggers a warning since affected CRTCs doent match
> > > > > > > > > > > requested CRTC.
> > > > > > > > > > >
> > > > > > > > > > > To fix this in such bigjoiner configurations, we should
> > > > > > > > > > > only
> > > > > > > > > > > increment affected crtcs if that CRTC is enabled in UAPI
> > > > > > > > > > > not
> > > > > > > > > > > if it is just used internally in the driver to split the
> > > > > > > > > > > mode.
> > > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > I think that makes sense to me. Stealing CRTCs that are not
> > > > > > > > > > currently
> > > > > > > > > > used by the userspace (display server) should be ok, as
> > > > > > > > > > long as that
> > > > > > > > > > is completely invisible to userspace: meaning that it does
> > > > > > > > > > not cause
> > > > > > > > > > userspace to unexpectedly e.g. receive or miss per-crtc
> > > > > > > > > > atomic
> > > > > > > > > > completion events.
> > > > > > > > >
> > > > > > > > > Yes since we are only doing atomic_check_only() here, the
> > > > > > > > > stolen
> > > > > > > >
> > > > > > > > But the real not-test-only commit will follow if this test-only
> > > > > > > > commit
> > > > > > > > succeeds, and keeping the guarantees for the real commit are
> > > > > > > > important.
> > > > > > >
> > > > > > > Hmm well after the actual real commit, since the second crtc is
> > > > > > > stolen
> > > > > > > even though it is not being used for the display output, it is
> > > > > > > used for joiner so the uapi.enable will be true after the real
> > > > > > > commit.
> > > > > > >
> > > > > > > so actually the assertion would fail in this case.
> > > > > > >
> > > > > > > @Ville @Danvet any suggestions here in that case?
> > > > >
> > > > > That is very bad. We can't frob uapi state like that. I think that
> > > > > calls for even more checks to make sure kms drivers who try to play
> > > > > clever games don't get it wrong, so we probably need to check uapi
> > > > > enable and active state in another mask before/after ->atomic_check
> > > > > too. Or something like that.
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > that is not what I was talking about, but sounds like you found a
> > > > > > different problem. It seems like the problem you are talking about
> > > > > > would be guaranteed to be hit if bigjoiner was used. Have you not
> > > > > > tested this?
> > > > > >
> > > > > > However, I was talking about the real commit itself, not what
> > > > > > happens
> > > > > > on commits after it, see below.
> > > > > >
> > > > > > > > > crtc is completely invisible to the userspace and hence that
> > > > > > > > > is
> > > > > > > > > indicated by uapi.enable which is not true for this stolen
> > > > > > > > > crtc. However if allow modeset flag set, then it will do a
> > > > > > > > > full
> > > > > > > > > modeset and indicate the uapi.enable for this stolen crtc as
> > > > > > > > > well
> > > > > > > > > since that cannot be used for other modeset requested by
> > > > > > > > > userspace.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Can that also be asserted somehow, or does this already do
> > > > > > > > > > that?
> > > > > > > > >
> > > > > > > > > Not clear what you want the assertion for? Could you elaborate
> > > > > > > >
> > > > > > > > As assertion that when the real atomic commit happens and then
> > > > > > > > completion events are fired, they match exactly the affected
> > > > > > > > crtcs mask.
> > > > > >
> > > > > > This is my concern and a question, although like I say below, only
> > > > > > tangential to this patch.
> > > > > >
> > > > > > However, as this patch aims to allow bigjoiner usage, naturally the
> > > > > > question will arise whether the completion events then match what
> > > > > > userspace expects or not. Userspace does not expect completion
> > > > > > events
> > > > > > referring to the stolen CRTCs.
> > > > >
> > > > > Yeah we also must make sure that we don't send out events for these
> > > > > additional crtc in bigjoiner usage. Sounds like igt testing didn't
> > > > > catch this, I think we need a lot more igts here to make sure all
> > > > > these surprises don't happen.
> > > > >
> > > > > Plus maybe triple-checking that drm_atomic_uapi.c makes sure we can't
> > > > > send out events for stuff that userspace didn't ask for.
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > > > I understand this may be off-topic for this particular patch,
> > > > > > > > but since
> > > > > > > > we are discussing the topic, such checks would be really nice.
> > > > > > > > I'm
> > > > > > > > curious if such checks already exist.
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > pq
> > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > > drivers/gpu/drm/drm_atomic.c | 6 ++++--
> > > > > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > > > > > > > > > > b/drivers/gpu/drm/drm_atomic.c
> > > > > > > > > > > index 5b4547e0f775..d7acd6bbd97e 100644
> > > > > > > > > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > > > > > > > > @@ -1358,8 +1358,10 @@ int drm_atomic_check_only(struct
> > > > > > > > > > > drm_atomic_state *state)
> > > > > > > > > > > }
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > - for_each_new_crtc_in_state(state, crtc,
> > > > > > > > > > > new_crtc_state, i)
> > > > > > > > > > > - affected_crtc |= drm_crtc_mask(crtc);
> > > > > > > > > > > + for_each_new_crtc_in_state(state, crtc,
> > > > > > > > > > > new_crtc_state, i) {
> > > > > > > > > > > + if (new_crtc_state->enable)
> > > > > > > > > > > + affected_crtc |=
> > > > > > > > > > > drm_crtc_mask(crtc);
> > > > > > > > > > > + }
> > > > > > > > > > >
> > > > > > > > > > > /*
> > > > > > > > > > > * For commits that allow modesets drivers can add
> > > > > > > > > > > other CRTCs to the
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-de...@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-de...@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
