Re: [Intel-gfx] [PATCH v3] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Tue, Jan 18, 2022 at 06:34:20PM +0200, Ville Syrjälä wrote:
> On Fri, Oct 22, 2021 at 12:51:12PM -0700, Navare, Manasi wrote:
> >
> > Hi Ville,
> >
> > Could you take a look at this, this addresses teh review comments from prev
> > version
>
> I don't think I ever got an answer to my question as to whether this
> was tested with all the interesting scenarios:
> 1) just with the master crtc added by userspace into the commit
> 2) just with the slave crtc added by userspace into the commit
> 3) both crtcs added by userspace into the commit
>
> I guess 1) has been tested since that happens all the time, but the other
> two scanarios would likely need to be done with a synthetic test to make
> sure we're actually hitting them.
>
> I think it *should* work, but I'd like to have real proof of that.
> Reviewed-by: Ville Syrjälä
Thank you for your review here Ville.
I have triggered a separate email thread to understand all the above testing
scenarios and get them tested with bigjoiner IGT.
Manasi
>
> >
> > Manasi
> >
> > On Mon, Oct 04, 2021 at 04:59:13AM -0700, 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.
> > >
> > > 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-de...@lists.freedesktop.org
> > > Signed-off-by: Manasi Navare
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 12
> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index ff1416cd609a..a1e4c7905ebb 100644
> > > --- 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_atomic_state
> > > *state)
> > >
> > > 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_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
> > >
>
> --
> Ville Syrjälä
> Intel
Re: [Intel-gfx] [PATCH v3] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Fri, Oct 22, 2021 at 12:51:12PM -0700, Navare, Manasi wrote:
>
> Hi Ville,
>
> Could you take a look at this, this addresses teh review comments from prev
> version
I don't think I ever got an answer to my question as to whether this
was tested with all the interesting scenarios:
1) just with the master crtc added by userspace into the commit
2) just with the slave crtc added by userspace into the commit
3) both crtcs added by userspace into the commit
I guess 1) has been tested since that happens all the time, but the other
two scanarios would likely need to be done with a synthetic test to make
sure we're actually hitting them.
I think it *should* work, but I'd like to have real proof of that.
Reviewed-by: Ville Syrjälä
>
> Manasi
>
> On Mon, Oct 04, 2021 at 04:59:13AM -0700, 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.
> >
> > 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-de...@lists.freedesktop.org
> > Signed-off-by: Manasi Navare
> > ---
> > drivers/gpu/drm/drm_atomic.c | 12
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index ff1416cd609a..a1e4c7905ebb 100644
> > --- 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_atomic_state
> > *state)
> >
> > 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_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
> >
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH v3] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
Hi Ville,
Could you take a look at this, this addresses teh review comments from prev
version
Manasi
On Mon, Oct 04, 2021 at 04:59:13AM -0700, 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.
>
> 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-de...@lists.freedesktop.org
> Signed-off-by: Manasi Navare
> ---
> drivers/gpu/drm/drm_atomic.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ff1416cd609a..a1e4c7905ebb 100644
> --- 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_atomic_state
> *state)
>
> 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_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
>
[Intel-gfx] [PATCH v3] 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.
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-de...@lists.freedesktop.org
Signed-off-by: Manasi Navare
---
drivers/gpu/drm/drm_atomic.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index ff1416cd609a..a1e4c7905ebb 100644
--- 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_atomic_state *state)
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_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
