Patch "drm/atomic: Add the crtc to affected crtc only if uapi.enable = true" has been added to the 5.15-stable tree
This is a note to let you know that I've just added the patch titled
drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
to the 5.15-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
drm-atomic-add-the-crtc-to-affected-crtc-only-if-uapi.enable-true.patch
and it can be found in the queue-5.15 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let know about it.
>From 5ec1cebd59300ddd26dbaa96c17c508764eef911 Mon Sep 17 00:00:00 2001
From: Manasi Navare
Date: Mon, 4 Oct 2021 04:59:13 -0700
Subject: drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: Manasi Navare
commit 5ec1cebd59300ddd26dbaa96c17c508764eef911 upstream.
In case of a modeset where a mode gets split across multiple 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.
v3: Add the same uapi crtc_state->enable check in requested
crtc calc (Ville)
Cc: Ville Syrjälä
Cc: Simon Ser
Cc: Pekka Paalanen
Cc: Daniel Stone
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
Cc: # v5.11+
Fixes: 919c2299a893 ("drm/i915: Enable bigjoiner")
Signed-off-by: Manasi Navare
Reviewed-by: Ville Syrjälä
Link:
https://patchwork.freedesktop.org/patch/msgid/20211004115913.23889-1-manasi.d.nav...@intel.com
Signed-off-by: Greg Kroah-Hartman
---
drivers/gpu/drm/drm_atomic.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1310,8 +1310,10 @@ int drm_atomic_check_only(struct drm_ato
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
- requested_crtc |= drm_crtc_mask(crtc);
+ for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+ if (new_crtc_state->enable)
+ requested_crtc |= drm_crtc_mask(crtc);
+ }
for_each_oldnew_plane_in_state(state, plane, old_plane_state,
new_plane_state, i) {
ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
@@ -1360,8 +1362,10 @@ int drm_atomic_check_only(struct drm_ato
}
}
- 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
Patches currently in stable-queue which might be from manasi.d.nav...@intel.com
are
queue-5.15/drm-atomic-add-the-crtc-to-affected-crtc-only-if-uapi.enable-true.patch
Patch "drm/atomic: Add the crtc to affected crtc only if uapi.enable = true" has been added to the 5.16-stable tree
This is a note to let you know that I've just added the patch titled
drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
to the 5.16-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
drm-atomic-add-the-crtc-to-affected-crtc-only-if-uapi.enable-true.patch
and it can be found in the queue-5.16 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let know about it.
>From 5ec1cebd59300ddd26dbaa96c17c508764eef911 Mon Sep 17 00:00:00 2001
From: Manasi Navare
Date: Mon, 4 Oct 2021 04:59:13 -0700
Subject: drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: Manasi Navare
commit 5ec1cebd59300ddd26dbaa96c17c508764eef911 upstream.
In case of a modeset where a mode gets split across multiple 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.
v3: Add the same uapi crtc_state->enable check in requested
crtc calc (Ville)
Cc: Ville Syrjälä
Cc: Simon Ser
Cc: Pekka Paalanen
Cc: Daniel Stone
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
Cc: # v5.11+
Fixes: 919c2299a893 ("drm/i915: Enable bigjoiner")
Signed-off-by: Manasi Navare
Reviewed-by: Ville Syrjälä
Link:
https://patchwork.freedesktop.org/patch/msgid/20211004115913.23889-1-manasi.d.nav...@intel.com
Signed-off-by: Greg Kroah-Hartman
---
drivers/gpu/drm/drm_atomic.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1310,8 +1310,10 @@ int drm_atomic_check_only(struct drm_ato
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
- requested_crtc |= drm_crtc_mask(crtc);
+ for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+ if (new_crtc_state->enable)
+ requested_crtc |= drm_crtc_mask(crtc);
+ }
for_each_oldnew_plane_in_state(state, plane, old_plane_state,
new_plane_state, i) {
ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
@@ -1360,8 +1362,10 @@ int drm_atomic_check_only(struct drm_ato
}
}
- 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
Patches currently in stable-queue which might be from manasi.d.nav...@intel.com
are
queue-5.16/drm-atomic-add-the-crtc-to-affected-crtc-only-if-uapi.enable-true.patch
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Thu, Apr 01, 2021 at 02:49:13PM -0700, Navare, Manasi wrote:
> On Fri, Mar 26, 2021 at 06:15:22PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 25, 2021 at 03:01:29PM -0700, Navare, Manasi wrote:
> > > On Fri, Mar 19, 2021 at 11:27:59PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Mar 19, 2021 at 02:26:24PM -0700, Navare, Manasi wrote:
> > > > > 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?
> > > >
> > > > I think you still need to fix the requested_crtcs calculation.
> > >
> > > We calculate requested crtc at the beginning :
> > > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > > requested_crtc |= drm_crtc_mask(crtc);
> > >
> > > Are you suggesting adding this to after:
> > > if (config->funcs->atomic_check) {
> > > ret = config->funcs->atomic_check(state->dev, state);
> > >
> > > if (ret) {
> > > DRM_DEBUG_ATOMIC("atomic driver check for %p
> > > failed: %d\n",
> > > state, ret);
> > > return ret;
> > > }
> > > requested_crtc |= drm_crtc_mask(crtc);// Here it will have
> > > requested crtc = 0x11
> > > }
> > >
> > > in this case here the state should already have master crtc 0 and slave
> > > crtc 1
> > > and that requested crtc should already be 0x11
> > >
> > > Then in that case we dont need any special check for calculating affected
> > > crtc, that also will be 0x11
> >
> > All I'm saying is that you're currently calculating requested_crtcs and
> > affected_crtcs differently. So I'm not at all surprised that they might
> > not match.
> >
>
> I dont get your point yet.
> requested crtc is calculated before the atomic check call and we dont check
> for crtc uapi.enable to be true.
> And hence requested crtc = CRTC 0 = 0x2
> After I added the check in this patch where affected crtc will include only
> the crtcs that have uapi.enable = true
> then it perfectly matches the requested crtc and return 0x2 but without this
> check when the calculation of
> requested and affected crtc is the same is where we see the affected crtc =
> CRTC 0 and 1 = 0x3
>
> So when the calculation is different infcat we dont see the mismatch
>
> What is your point here?
Try doing an atomic commit wiht both crtcs already added in by
userspace. I think that will still WARN.
--
Ville Syrjälä
Intel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Fri, Mar 26, 2021 at 06:15:22PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 25, 2021 at 03:01:29PM -0700, Navare, Manasi wrote:
> > On Fri, Mar 19, 2021 at 11:27:59PM +0200, Ville Syrjälä wrote:
> > > On Fri, Mar 19, 2021 at 02:26:24PM -0700, Navare, Manasi wrote:
> > > > 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?
> > >
> > > I think you still need to fix the requested_crtcs calculation.
> >
> > We calculate requested crtc at the beginning :
> > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > requested_crtc |= drm_crtc_mask(crtc);
> >
> > Are you suggesting adding this to after:
> > if (config->funcs->atomic_check) {
> > ret = config->funcs->atomic_check(state->dev, state);
> >
> > if (ret) {
> > DRM_DEBUG_ATOMIC("atomic driver check for %p
> > failed: %d\n",
> > state, ret);
> > return ret;
> > }
> > requested_crtc |= drm_crtc_mask(crtc);// Here it will have
> > requested crtc = 0x11
> > }
> >
> > in this case here the state should already have master crtc 0 and slave
> > crtc 1
> > and that requested crtc should already be 0x11
> >
> > Then in that case we dont need any special check for calculating affected
> > crtc, that also will be 0x11
>
> All I'm saying is that you're currently calculating requested_crtcs and
> affected_crtcs differently. So I'm not at all surprised that they might
> not match.
>
I dont get your point yet.
requested crtc is calculated before the atomic check call and we dont check for
crtc uapi.enable to be true.
And hence requested crtc = CRTC 0 = 0x2
After I added the check in this patch where affected crtc will include only the
crtcs that have uapi.enable = true
then it perfectly matches the requested crtc and return 0x2 but without this
check when the calculation of
requested and affected crtc is the same is where we see the affected crtc =
CRTC 0 and 1 = 0x3
So when the calculation is different infcat we dont see the mismatch
What is your point here?
Manasi
> --
> Ville Syrjälä
> Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Thu, Mar 25, 2021 at 03:01:29PM -0700, Navare, Manasi wrote:
> On Fri, Mar 19, 2021 at 11:27:59PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 19, 2021 at 02:26:24PM -0700, Navare, Manasi wrote:
> > > 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?
> >
> > I think you still need to fix the requested_crtcs calculation.
>
> We calculate requested crtc at the beginning :
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> requested_crtc |= drm_crtc_mask(crtc);
>
> Are you suggesting adding this to after:
> if (config->funcs->atomic_check) {
> ret = config->funcs->atomic_check(state->dev, state);
>
> if (ret) {
> DRM_DEBUG_ATOMIC("atomic driver check for %p failed:
> %d\n",
> state, ret);
> return ret;
> }
> requested_crtc |= drm_crtc_mask(crtc);// Here it will have
> requested crtc = 0x11
> }
>
> in this case here the state should already have master crtc 0 and slave crtc 1
> and that requested crtc should already be 0x11
>
> Then in that case we dont need any special check for calculating affected
> crtc, that also will be 0x11
All I'm saying is that you're currently calculating requested_crtcs and
affected_crtcs differently. So I'm not at all surprised that they might
not match.
--
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Fri, Mar 19, 2021 at 11:27:59PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 19, 2021 at 02:26:24PM -0700, Navare, Manasi wrote:
> > 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?
>
> I think you still need to fix the requested_crtcs calculation.
We calculate requested crtc at the beginning :
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
requested_crtc |= drm_crtc_mask(crtc);
Are you suggesting adding this to after:
if (config->funcs->atomic_check) {
ret = config->funcs->atomic_check(state->dev, state);
if (ret) {
DRM_DEBUG_ATOMIC("atomic driver check for %p failed:
%d\n",
state, ret);
return ret;
}
requested_crtc |= drm_crtc_mask(crtc);// Here it will have
requested crtc = 0x11
}
in this case here the state should already have master crtc 0 and slave crtc 1
and that requested crtc should already be 0x11
Then in that case we dont need any special check for calculating affected crtc,
that also will be 0x11
Manasi
>
> --
> Ville Syrjälä
> Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Fri, Mar 19, 2021 at 02:26:24PM -0700, Navare, Manasi wrote: > 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? I think you still need to fix the requested_crtcs calculation. -- Ville Syrjälä Intel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
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 > > > > > wrote: > > > > > > > > > > > > On Mon, 8 Mar 2021 16:52:58 -0800 > > > > > > "Navare, Manasi" 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" 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 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 >
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
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? > 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 > > > > wrote: > > > > > > > > > > On Mon, 8 Mar 2021 16:52:58 -0800 > > > > > "Navare, Manasi" 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" 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 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
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
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. 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 > > > wrote: > > > > > > > > On Mon, 8 Mar 2021 16:52:58 -0800 > > > > "Navare, Manasi" 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" 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 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. > > > > > > > > > > > > > > > >
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
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. > 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 wrote: > > > > > > On Mon, 8 Mar 2021 16:52:58 -0800 > > > "Navare, Manasi" 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" 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 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
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
So basically we see this warning only in case of bigjoiner when drm_atomic_check gets called without setting the state->allow_modeset flag. 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? 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 wrote: > > > > On Mon, 8 Mar 2021 16:52:58 -0800 > > "Navare, Manasi" 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" 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 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 > > > > >
Re: [Intel-gfx] [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Tue, Mar 16, 2021 at 11:46:38PM +, Daniel Stone wrote: > On Tue, 16 Mar 2021 at 21:35, Daniel Vetter wrote: > > > On Tue, Mar 9, 2021 at 10:14 AM Pekka Paalanen > > wrote: > > > On Mon, 8 Mar 2021 16:52:58 -0800 > > > "Navare, Manasi" wrote: > > > > 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. > > > > Yeah. We can _never_ generate externally-visible completion events. We can > later fail to enable the stolen CRTC - because trying to enable new things > can fail for any reason whatsoever - but we can't generate spurious > completion events, as doing so falls into the uncanny valley. > > If the kernel is doing clever things behind userspace's back - such as > stealing planes or CRTCs - then userspace can never know about it, apart > from failing to enable those resources later. The kernel can either never > do anything clever (and make userspace bind them both together), or be > extremely clever (by hiding the entire details from userspace), but it > cannot choose the halfway house of doing clever things behind userspace's > back (such as stealing new CRTCs) whilst also exposing all those details to > userspace (such as delivering spurious completion events for resources > userspace never requested to be programmed). > > Cheers, > Daniel Yes I agree, in this case there will not be any completion events associated with the stolen slave CRTC since that does not get used for the output. The completion events will only occur on the bigjoiner master crtc. But I guess like Danvet suggested we need a separate mask for keeping track of active and enabled crtcs before and after atomic check. But need to look at how this will fix the affected crtc not matching warning. Manasi > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Tue, 16 Mar 2021 at 21:35, Daniel Vetter wrote: > On Tue, Mar 9, 2021 at 10:14 AM Pekka Paalanen > wrote: > > On Mon, 8 Mar 2021 16:52:58 -0800 > > "Navare, Manasi" wrote: > > > 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. > Yeah. We can _never_ generate externally-visible completion events. We can later fail to enable the stolen CRTC - because trying to enable new things can fail for any reason whatsoever - but we can't generate spurious completion events, as doing so falls into the uncanny valley. If the kernel is doing clever things behind userspace's back - such as stealing planes or CRTCs - then userspace can never know about it, apart from failing to enable those resources later. The kernel can either never do anything clever (and make userspace bind them both together), or be extremely clever (by hiding the entire details from userspace), but it cannot choose the halfway house of doing clever things behind userspace's back (such as stealing new CRTCs) whilst also exposing all those details to userspace (such as delivering spurious completion events for resources userspace never requested to be programmed). Cheers, Daniel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Tue, Mar 9, 2021 at 10:14 AM Pekka Paalanen wrote: > > On Mon, 8 Mar 2021 16:52:58 -0800 > "Navare, Manasi" 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" 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 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); > >
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Mon, 8 Mar 2021 16:52:58 -0800
"Navare, Manasi" 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" 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 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?
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.
> > 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
pgpIIGcwLTd5j.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Thu, Mar 04, 2021 at 10:42:23AM +0200, Pekka Paalanen wrote:
> On Wed, 3 Mar 2021 12:44:33 -0800
> "Navare, Manasi" 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 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?
Manasi
>
> > 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.
>
> 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
>
> >
> > Manasi
> >
> > >
> > >
> > > Thanks,
> > > pq
> > >
> > > > Cc: Ville Syrjälä
> > > > Cc: Simon Ser
> > > > Cc: Pekka Paalanen
> > > > Cc: Daniel Stone
> > > > Cc: Daniel Vetter
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Signed-off-by: Manasi Navare
> > > > ---
> > > > 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-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Wed, 3 Mar 2021 12:44:33 -0800
"Navare, Manasi" 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 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.
> 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.
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
>
> Manasi
>
> >
> >
> > Thanks,
> > pq
> >
> > > Cc: Ville Syrjälä
> > > Cc: Simon Ser
> > > Cc: Pekka Paalanen
> > > Cc: Daniel Stone
> > > Cc: Daniel Vetter
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Manasi Navare
> > > ---
> > > 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
> >
>
>
pgpo85jwkYEDd.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Wed, Mar 03, 2021 at 10:47:44AM +0200, Pekka Paalanen wrote:
> On Tue, 2 Mar 2021 12:41:32 -0800
> Manasi Navare 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
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
Manasi
>
>
> Thanks,
> pq
>
> > Cc: Ville Syrjälä
> > Cc: Simon Ser
> > Cc: Pekka Paalanen
> > Cc: Daniel Stone
> > Cc: Daniel Vetter
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Manasi Navare
> > ---
> > 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-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Tue, 2 Mar 2021 12:41:32 -0800
Manasi Navare 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.
Can that also be asserted somehow, or does this already do that?
Thanks,
pq
> Cc: Ville Syrjälä
> Cc: Simon Ser
> Cc: Pekka Paalanen
> Cc: Daniel Stone
> Cc: Daniel Vetter
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Manasi Navare
> ---
> 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
pgpIUo4wRhG9D.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
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.
Cc: Ville Syrjälä
Cc: Simon Ser
Cc: Pekka Paalanen
Cc: Daniel Stone
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Manasi Navare
---
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
--
2.19.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
