Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state
On Sat, May 11, 2019 at 10:12:02PM +0300, Laurent Pinchart wrote:
> Hi Sean,
>
> Thank you for the patch.
>
> On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote:
> > From: Sean Paul
> >
> > Everyone who implements connector_helper_funcs->atomic_check reaches
> > into the connector state to get the atomic state. Instead of continuing
> > this pattern, change the callback signature to just give atomic state
> > and let the driver determine what it does and does not need from it.
> >
> > Eventually all atomic functions should do this, but that's just too much
> > busy work for me.
>
> Given that drivers also access the connector state, isn't this slightly
> more inefficient ?
It's atomic code, we're trying to optimize for clean code at the expense
of a bit of runtime overhead due to more pointer chasing. And I agree with
the general push, the pile of old/new_state pointers of various objects
we're passing around is confusing. Passing the overall drm_atomic_state
seems much more reasonable, and with that we can get everything else. Plus
it's much more obvious whether you have the old/new state (since that's
explicit when you look it up from the drm_atomic_state).
If we ever see this show up in profile, and it starts mattering, first
thing we need is a hashtable I think (atm it's list walking, which is just
terrible). But thus far no one cares.
-Daniel
>
> > Changes in v3:
> > - Added to the set
> >
> > Cc: Daniel Vetter
> > Cc: Ville Syrjälä
> > Cc: Jani Nikula
> > Cc: Joonas Lahtinen
> > Cc: Rodrigo Vivi
> > Cc: Ben Skeggs
> > Cc: Laurent Pinchart
> > Cc: Kieran Bingham
> > Cc: Eric Anholt
> > Signed-off-by: Sean Paul
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 4 ++--
> > drivers/gpu/drm/i915/intel_atomic.c | 8 +---
> > drivers/gpu/drm/i915/intel_dp_mst.c | 7 ---
> > drivers/gpu/drm/i915/intel_drv.h | 2 +-
> > drivers/gpu/drm/i915/intel_sdvo.c| 9 +
> > drivers/gpu/drm/i915/intel_tv.c | 8 +---
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++--
> > drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++-
> > drivers/gpu/drm/vc4/vc4_txp.c| 7 ---
> > include/drm/drm_modeset_helper_vtables.h | 2 +-
> > 10 files changed, 37 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 9d9e47276839..fa5a367507c1 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > }
> >
> > if (funcs->atomic_check)
> > - ret = funcs->atomic_check(connector,
> > new_connector_state);
> > + ret = funcs->atomic_check(connector, state);
> > if (ret)
> > return ret;
> >
> > @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > continue;
> >
> > if (funcs->atomic_check)
> > - ret = funcs->atomic_check(connector,
> > new_connector_state);
> > + ret = funcs->atomic_check(connector, state);
> > if (ret)
> > return ret;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > b/drivers/gpu/drm/i915/intel_atomic.c
> > index b844e8840c6f..e8a5b82e9242 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -103,12 +103,14 @@ int
> > intel_digital_connector_atomic_set_property(struct drm_connector *connector,
> > }
> >
> > int intel_digital_connector_atomic_check(struct drm_connector *conn,
> > -struct drm_connector_state *new_state)
> > +struct drm_atomic_state *state)
> > {
> > + struct drm_connector_state *new_state =
> > + drm_atomic_get_new_connector_state(state, conn);
> > struct intel_digital_connector_state *new_conn_state =
> > to_intel_digital_connector_state(new_state);
> > struct drm_connector_state *old_state =
> > - drm_atomic_get_old_connector_state(new_state->state, conn);
> > + drm_atomic_get_old_connector_state(state, conn);
> > struct intel_digital_connector_state *old_conn_state =
> > to_intel_digital_connector_state(old_state);
> > struct drm_crtc_state *crtc_state;
> > @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct
> > drm_connector *conn,
> > if (!new_state->crtc)
> > return 0;
> >
> > - crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> > new_state->crtc);
> > + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> >
> > /*
> > * These properties are handled by fastset, and might not end
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> >
Re: [Nouveau] [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state
On Sat, May 11, 2019 at 3:12 PM Laurent Pinchart
wrote:
>
> Hi Sean,
>
> Thank you for the patch.
>
Hey Laurent,
Thanks for looking!
> On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote:
> > From: Sean Paul
> >
> > Everyone who implements connector_helper_funcs->atomic_check reaches
> > into the connector state to get the atomic state. Instead of continuing
> > this pattern, change the callback signature to just give atomic state
> > and let the driver determine what it does and does not need from it.
> >
> > Eventually all atomic functions should do this, but that's just too much
> > busy work for me.
>
> Given that drivers also access the connector state, isn't this slightly
> more inefficient ?
>
Inefficient in terms of what?
Agree that in isolation this patch might seem unnecessary, but it ties
in with the encoder and bridge CLs which accept drm_atomic_state in
their hooks. In general the idea is to convert all atomic functions to
take overall atomic state instead of just their object state. Reality
has proven to be more complicated and we need more access than what
the current implementation provides.
Sean
> > Changes in v3:
> > - Added to the set
> >
> > Cc: Daniel Vetter
> > Cc: Ville Syrjälä
> > Cc: Jani Nikula
> > Cc: Joonas Lahtinen
> > Cc: Rodrigo Vivi
> > Cc: Ben Skeggs
> > Cc: Laurent Pinchart
> > Cc: Kieran Bingham
> > Cc: Eric Anholt
> > Signed-off-by: Sean Paul
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 4 ++--
> > drivers/gpu/drm/i915/intel_atomic.c | 8 +---
> > drivers/gpu/drm/i915/intel_dp_mst.c | 7 ---
> > drivers/gpu/drm/i915/intel_drv.h | 2 +-
> > drivers/gpu/drm/i915/intel_sdvo.c| 9 +
> > drivers/gpu/drm/i915/intel_tv.c | 8 +---
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +++--
> > drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++-
> > drivers/gpu/drm/vc4/vc4_txp.c| 7 ---
> > include/drm/drm_modeset_helper_vtables.h | 2 +-
> > 10 files changed, 37 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 9d9e47276839..fa5a367507c1 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > }
> >
> > if (funcs->atomic_check)
> > - ret = funcs->atomic_check(connector,
> > new_connector_state);
> > + ret = funcs->atomic_check(connector, state);
> > if (ret)
> > return ret;
> >
> > @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > continue;
> >
> > if (funcs->atomic_check)
> > - ret = funcs->atomic_check(connector,
> > new_connector_state);
> > + ret = funcs->atomic_check(connector, state);
> > if (ret)
> > return ret;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > b/drivers/gpu/drm/i915/intel_atomic.c
> > index b844e8840c6f..e8a5b82e9242 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -103,12 +103,14 @@ int
> > intel_digital_connector_atomic_set_property(struct drm_connector *connector,
> > }
> >
> > int intel_digital_connector_atomic_check(struct drm_connector *conn,
> > - struct drm_connector_state
> > *new_state)
> > + struct drm_atomic_state *state)
> > {
> > + struct drm_connector_state *new_state =
> > + drm_atomic_get_new_connector_state(state, conn);
> > struct intel_digital_connector_state *new_conn_state =
> > to_intel_digital_connector_state(new_state);
> > struct drm_connector_state *old_state =
> > - drm_atomic_get_old_connector_state(new_state->state, conn);
> > + drm_atomic_get_old_connector_state(state, conn);
> > struct intel_digital_connector_state *old_conn_state =
> > to_intel_digital_connector_state(old_state);
> > struct drm_crtc_state *crtc_state;
> > @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct
> > drm_connector *conn,
> > if (!new_state->crtc)
> > return 0;
> >
> > - crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> > new_state->crtc);
> > + crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> >
> > /*
> >* These properties are handled by fastset, and might not end
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 19d81cef2ab6..89cfec128ba0 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -143,9 +143,10 @@ static int
[Nouveau] [Bug 110669] New: iMac with GK107M unstable - hangs with xorg / crashes with xwayland
https://bugs.freedesktop.org/show_bug.cgi?id=110669 Bug ID: 110669 Summary: iMac with GK107M unstable - hangs with xorg / crashes with xwayland Product: Mesa Version: 19.0 Hardware: x86-64 (AMD64) OS: Linux (All) Status: NEW Severity: normal Priority: medium Component: Drivers/DRI/nouveau Assignee: nouveau@lists.freedesktop.org Reporter: s...@hortensius.net QA Contact: nouveau@lists.freedesktop.org I'd like to start with noting this issue has been occurring for years on most of our iMacs and I strongly suspect this isn't necessarily "caused" by nouveau but rather by an implementation detail NVidia forgot to document (please remind me why I keep buying their hardware?) Hardware: Apple Inc. iMac13,2/Mac-FC02E91DDD3FA6A4, BIOS IM131.88Z.010A.B05.1211151146 11/15/2012 Software: Archlinux / any nouveau release, currenty running xf86-video-nouveau 1.0.16-1 Using Xorg this iMac hangs maybe 2 or 3 times a day, using Xwayland (using sway, a native wayland implementation) there actually is a backtrace and I have a chance to reboot instead of kill the power. I'd love to help debug this since I assume it only happens when using this specific hardware. To start with - here is a backtrace: Stack trace of thread 765: #0 0x7f9374fe282f raise (libc.so.6) #1 0x7f9374fcd672 abort (libc.so.6) #2 0x55ef04386f6a n/a (Xwayland) #3 0x55ef0437f645 n/a (Xwayland) #4 0x55ef0438a7a6 n/a (Xwayland) #5 0x7f9374fe28b0 __restore_rt (libc.so.6) #6 0x7f9374fe282f raise (libc.so.6) #7 0x7f9374fcd672 abort (libc.so.6) #8 0x7f9375024e78 __libc_message (libc.so.6) #9 0x7f937502b78a malloc_printerr (libc.so.6) #10 0x7f937502d007 _int_free (libc.so.6) #11 0x7f936e02b7da nouveau_bo_ref (libdrm_nouveau.so.2) #12 0x7f9372866894 n/a (nouveau_dri.so) #13 0x7f9372a14c87 n/a (nouveau_dri.so) #14 0x7f93729d6604 n/a (nouveau_dri.so) #15 0x7f93729d9031 n/a (nouveau_dri.so) #16 0x7f93729dc5c8 n/a (nouveau_dri.so) #17 0x55ef0448d8d4 n/a (Xwayland) #18 0x55ef0449963d n/a (Xwayland) #19 0x55ef04404ec1 n/a (Xwayland) #20 0x55ef043e53c8 n/a (Xwayland) #21 0x55ef043e5933 n/a (Xwayland) #22 0x55ef04451270 n/a (Xwayland) #23 0x55ef0434b11d n/a (Xwayland) #24 0x7f9374fcece3 __libc_start_main (libc.so.6) #25 0x55ef0434c14e n/a (Xwayland) Stack trace of thread 769: #0 0x7f93746f2bac pthread_cond_wait@@GLIBC_2.3.2 (libpthread.so.0) #1 0x7f9372687474 n/a (nouveau_dri.so) #2 0x7f93726872c8 n/a (nouveau_dri.so) #3 0x7f93746eca92 start_thread (libpthread.so.0) #4 0x7f93750a5cd3 __clone (libc.so.6) Stack trace of thread 771: #0 0x7f93746f2bac pthread_cond_wait@@GLIBC_2.3.2 (libpthread.so.0) #1 0x7f9372687474 n/a (nouveau_dri.so) #2 0x7f93726872c8 n/a (nouveau_dri.so) #3 0x7f93746eca92 start_thread (libpthread.so.0) #4 0x7f93750a5cd3 __clone (libc.so.6) Stack trace of thread 772: #0 0x7f93746f2bac pthread_cond_wait@@GLIBC_2.3.2 (libpthread.so.0) #1 0x7f9372687474 n/a (nouveau_dri.so) #2 0x7f93726872c8 n/a (nouveau_dri.so) #3 0x7f93746eca92 start_thread (libpthread.so.0) #4 0x7f93750a5cd3 __clone (libc.so.6) Stack trace of thread 768: #0 0x7f93746f2bac pthread_cond_wait@@GLIBC_2.3.2 (libpthread.so.0) #1 0x7f9372cc81e4 n/a (nouveau_dri.so) #2 0x7f9372cc7f08 n/a (nouveau_dri.so) #3 0x7f93746eca92 start_thread (libpthread.so.0) #4 0x7f93750a5cd3 __clone (libc.so.6) Stack trace of thread 770: #0 0x7f93746f2bac pthread_cond_wait@@GLIBC_2.3.2 (libpthread.so.0) #1 0x7f9372687474 n/a (nouveau_dri.so) #2 0x7f93726872c8 n/a (nouveau_dri.so) #3 0x7f93746eca92 start_thread (libpthread.so.0) #4 0x7f93750a5cd3 __clone (libc.so.6) -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug.___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
