| Message ID | 20170629115954.26029-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 29, 2017 at 01:59:54PM +0200, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_framebuffer.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index fc8ef42203ec..b3ef4f1c2630 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -832,6 +832,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > drm_atomic_clean_old_fb(dev, plane_mask, ret); > > if (ret == -EDEADLK) { > + drm_atomic_state_clear(state); Hmm. We seem to be missing this all over. Do those other places need it as well? Hard to say without a commit message explaining why we need it here. Should we just back it into drm_modeset_backoff() if it's always needed? > drm_modeset_backoff(&ctx); > goto retry; > } > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jun 29, 2017 at 04:57:25PM +0300, Ville Syrjälä wrote: > On Thu, Jun 29, 2017 at 01:59:54PM +0200, Maarten Lankhorst wrote: > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/drm_framebuffer.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > index fc8ef42203ec..b3ef4f1c2630 100644 > > --- a/drivers/gpu/drm/drm_framebuffer.c > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > @@ -832,6 +832,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > > drm_atomic_clean_old_fb(dev, plane_mask, ret); > > > > if (ret == -EDEADLK) { > > + drm_atomic_state_clear(state); > > Hmm. We seem to be missing this all over. Do those other places need it > as well? Hard to say without a commit message explaining why we need it > here. > > Should we just back it into drm_modeset_backoff() if it's always needed? s/back/bake/ > > > drm_modeset_backoff(&ctx); > > goto retry; > > } > > -- > > 2.11.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 29, 2017 at 05:17:39PM +0300, Ville Syrjälä wrote: > On Thu, Jun 29, 2017 at 04:57:25PM +0300, Ville Syrjälä wrote: > > On Thu, Jun 29, 2017 at 01:59:54PM +0200, Maarten Lankhorst wrote: > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > --- > > > drivers/gpu/drm/drm_framebuffer.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > > index fc8ef42203ec..b3ef4f1c2630 100644 > > > --- a/drivers/gpu/drm/drm_framebuffer.c > > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > > @@ -832,6 +832,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > > > drm_atomic_clean_old_fb(dev, plane_mask, ret); > > > > > > if (ret == -EDEADLK) { > > > + drm_atomic_state_clear(state); > > > > Hmm. We seem to be missing this all over. Do those other places need it > > as well? Hard to say without a commit message explaining why we need it > > here. > > > > Should we just back it into drm_modeset_backoff() if it's always needed? > > s/back/bake/ It's not needed everywhere, and that's because you can do the modeset_lock dance without necessarily doing an atomic transaction (e.g. hw state readout on boot or load detect). Or the atomic transaction is happening somewhere else from where the ctx backoff is handled (e.g. for legacy paths the core code handles the w/w dance since my recent rework, whereas compat helpers handle the retry for the atomic side). But yeah if you do an atomic commit, you must have the state_clear in the EDEADLK path somewhere. Maybe we could do a trick with lockdep annotations (make the state another ww mutex that nests within the modeset_lock class, or something like that) to ensure that no one ever forgets to clear this up? But that's a bit tricky, since on successful commit we hand the state over to the driver and must _not_ clear it, this is only for the backoff case (even on any other errors it's not needed, since we just kfree everything). -Daniel > > > > > > drm_modeset_backoff(&ctx); > > > goto retry; > > > } > > > -- > > > 2.11.0 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jun 29, 2017 at 01:59:54PM +0200, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> With a real commit message (just explain why it's needed): Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Also needs Fixes: db8f6403e88a ("drm: Convert drm_framebuffer_remove to atomic, v4.") Cc: stable@vger.kernel.org # v4.12-rc1+ Please push to drm-misc-next-fixes so Sean can send a pull for it for 4.13. Thanks, Daniel > --- > drivers/gpu/drm/drm_framebuffer.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index fc8ef42203ec..b3ef4f1c2630 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -832,6 +832,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > drm_atomic_clean_old_fb(dev, plane_mask, ret); > > if (ret == -EDEADLK) { > + drm_atomic_state_clear(state); > drm_modeset_backoff(&ctx); > goto retry; > } > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 30-06-17 om 14:46 schreef Daniel Vetter: > On Thu, Jun 29, 2017 at 01:59:54PM +0200, Maarten Lankhorst wrote: >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > With a real commit message (just explain why it's needed): > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Also needs > > Fixes: db8f6403e88a ("drm: Convert drm_framebuffer_remove to atomic, v4.") > Cc: stable@vger.kernel.org # v4.12-rc1+ > > Please push to drm-misc-next-fixes so Sean can send a pull for it for > 4.13. Pushed, thanks. :)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index fc8ef42203ec..b3ef4f1c2630 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -832,6 +832,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) drm_atomic_clean_old_fb(dev, plane_mask, ret); if (ret == -EDEADLK) { + drm_atomic_state_clear(state); drm_modeset_backoff(&ctx); goto retry; }
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_framebuffer.c | 1 + 1 file changed, 1 insertion(+)