| Message ID | 20181002133526.13685-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | atomic helper cleanups | expand |
On Tue, Oct 02, 2018 at 03:35:12PM +0200, Daniel Vetter wrote: > The core _does_ the call to drm_atomic_commit for you. That's pretty > much the entire point of having the fancy new atomic_set/get_prop > callbacks. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > Cc: Sinclair Yeh <syeh@vmware.com> > Cc: Thomas Hellstrom <thellstrom@vmware.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > index 292e48feba83..049bd50eea87 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -2311,12 +2311,6 @@ vmw_du_connector_atomic_set_property(struct drm_connector *connector, > > if (property == dev_priv->implicit_placement_property) { > vcs->is_implicit = val; > - > - /* > - * We should really be doing a drm_atomic_commit() to > - * commit the new state, but since this doesn't cause > - * an immedate state change, this is probably ok > - */ > du->is_implicit = vcs->is_implicit; Maybe the comment is referring to delaying the du->is_implicit assignment to commit time? Otherwise a TEST_ONLY/failed commit will clobber this. Hmm. There's both .set_property() and .atomic_set_property() in there. I wonder what that's about. > } else { > return -EINVAL; > -- > 2.19.0.rc2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 10/02/2018 05:15 PM, Ville Syrjälä wrote: > On Tue, Oct 02, 2018 at 03:35:12PM +0200, Daniel Vetter wrote: >> The core _does_ the call to drm_atomic_commit for you. That's pretty >> much the entire point of having the fancy new atomic_set/get_prop >> callbacks. >> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> >> Cc: Sinclair Yeh <syeh@vmware.com> >> Cc: Thomas Hellstrom <thellstrom@vmware.com> >> --- >> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c >> index 292e48feba83..049bd50eea87 100644 >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c >> @@ -2311,12 +2311,6 @@ vmw_du_connector_atomic_set_property(struct drm_connector *connector, >> >> if (property == dev_priv->implicit_placement_property) { >> vcs->is_implicit = val; >> - >> - /* >> - * We should really be doing a drm_atomic_commit() to >> - * commit the new state, but since this doesn't cause >> - * an immedate state change, this is probably ok >> - */ >> du->is_implicit = vcs->is_implicit; > Maybe the comment is referring to delaying the du->is_implicit > assignment to commit time? Otherwise a TEST_ONLY/failed commit > will clobber this. The is_implicit property is made read-only in a vmwgfx recent commit. Not sure exactly where it ended up, though. (-fixes, -next or -limbo). Need to take a look. > > Hmm. There's both .set_property() and .atomic_set_property() > in there. I wonder what that's about. Probably a leftover. I take it .set_property() is not needed when we have .atomic_set_property()? /Thomas > >> } else { >> return -EINVAL; >> -- >> 2.19.0.rc2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fintel-gfx&data=02%7C01%7Cthellstrom%40vmware.com%7C8376824afaaa4e7ebd6808d6287a0a88%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636740901969428557&sdata=JDQsTWKhvZAyUnW76dNMFGm0nzJIJjNrSSJYtDuqDlg%3D&reserved=0
On Tue, Oct 02, 2018 at 04:36:31PM +0000, Thomas Hellstrom wrote: > On 10/02/2018 05:15 PM, Ville Syrjälä wrote: > > On Tue, Oct 02, 2018 at 03:35:12PM +0200, Daniel Vetter wrote: > >> The core _does_ the call to drm_atomic_commit for you. That's pretty > >> much the entire point of having the fancy new atomic_set/get_prop > >> callbacks. > >> > >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > >> Cc: Sinclair Yeh <syeh@vmware.com> > >> Cc: Thomas Hellstrom <thellstrom@vmware.com> > >> --- > >> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 ------ > >> 1 file changed, 6 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > >> index 292e48feba83..049bd50eea87 100644 > >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > >> @@ -2311,12 +2311,6 @@ vmw_du_connector_atomic_set_property(struct drm_connector *connector, > >> > >> if (property == dev_priv->implicit_placement_property) { > >> vcs->is_implicit = val; > >> - > >> - /* > >> - * We should really be doing a drm_atomic_commit() to > >> - * commit the new state, but since this doesn't cause > >> - * an immedate state change, this is probably ok > >> - */ > >> du->is_implicit = vcs->is_implicit; > > Maybe the comment is referring to delaying the du->is_implicit > > assignment to commit time? Otherwise a TEST_ONLY/failed commit > > will clobber this. > > The is_implicit property is made read-only in a vmwgfx recent commit. > Not sure exactly where it ended up, though. (-fixes, -next or -limbo). > Need to take a look. > > > > > > Hmm. There's both .set_property() and .atomic_set_property() > > in there. I wonder what that's about. > > Probably a leftover. I take it .set_property() is not needed when we > have .atomic_set_property()? Yeah, the legacy one is dead weight at this point.
On Tue, Oct 02, 2018 at 04:36:31PM +0000, Thomas Hellstrom wrote: > On 10/02/2018 05:15 PM, Ville Syrjälä wrote: > > On Tue, Oct 02, 2018 at 03:35:12PM +0200, Daniel Vetter wrote: > >> The core _does_ the call to drm_atomic_commit for you. That's pretty > >> much the entire point of having the fancy new atomic_set/get_prop > >> callbacks. > >> > >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > >> Cc: Sinclair Yeh <syeh@vmware.com> > >> Cc: Thomas Hellstrom <thellstrom@vmware.com> > >> --- > >> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 ------ > >> 1 file changed, 6 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > >> index 292e48feba83..049bd50eea87 100644 > >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > >> @@ -2311,12 +2311,6 @@ vmw_du_connector_atomic_set_property(struct drm_connector *connector, > >> > >> if (property == dev_priv->implicit_placement_property) { > >> vcs->is_implicit = val; > >> - > >> - /* > >> - * We should really be doing a drm_atomic_commit() to > >> - * commit the new state, but since this doesn't cause > >> - * an immedate state change, this is probably ok > >> - */ > >> du->is_implicit = vcs->is_implicit; > > Maybe the comment is referring to delaying the du->is_implicit > > assignment to commit time? Otherwise a TEST_ONLY/failed commit > > will clobber this. > > The is_implicit property is made read-only in a vmwgfx recent commit. > Not sure exactly where it ended up, though. (-fixes, -next or -limbo). > Need to take a look. I guess -limbo, since my tree contains both drm-fixes and drm-next. Or at least they didn't make it to Dave yet. -Daniel > > > > > > Hmm. There's both .set_property() and .atomic_set_property() > > in there. I wonder what that's about. > > Probably a leftover. I take it .set_property() is not needed when we > have .atomic_set_property()? > > /Thomas > > > > >> } else { > >> return -EINVAL; > >> -- > >> 2.19.0.rc2 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fintel-gfx&data=02%7C01%7Cthellstrom%40vmware.com%7C8376824afaaa4e7ebd6808d6287a0a88%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636740901969428557&sdata=JDQsTWKhvZAyUnW76dNMFGm0nzJIJjNrSSJYtDuqDlg%3D&reserved=0 > >
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 292e48feba83..049bd50eea87 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -2311,12 +2311,6 @@ vmw_du_connector_atomic_set_property(struct drm_connector *connector, if (property == dev_priv->implicit_placement_property) { vcs->is_implicit = val; - - /* - * We should really be doing a drm_atomic_commit() to - * commit the new state, but since this doesn't cause - * an immedate state change, this is probably ok - */ du->is_implicit = vcs->is_implicit; } else { return -EINVAL;
The core _does_ the call to drm_atomic_commit for you. That's pretty much the entire point of having the fancy new atomic_set/get_prop callbacks. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> Cc: Sinclair Yeh <syeh@vmware.com> Cc: Thomas Hellstrom <thellstrom@vmware.com> --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 ------ 1 file changed, 6 deletions(-)