Re: [Intel-gfx] [PATCH 04/18] drm/vmwgfx: Remove confused comment from vmw_du_connector_atomic_set_property
On Tue, Oct 02, 2018 at 04:36:31PM +, 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 > >> Cc: VMware Graphics > >> Cc: Sinclair Yeh > >> Cc: Thomas Hellstrom > >> --- > >> 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-...@lists.freedesktop.org > >> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fintel-gfxdata=02%7C01%7Cthellstrom%40vmware.com%7C8376824afaaa4e7ebd6808d6287a0a88%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636740901969428557sdata=JDQsTWKhvZAyUnW76dNMFGm0nzJIJjNrSSJYtDuqDlg%3Dreserved=0 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 04/18] drm/vmwgfx: Remove confused comment from vmw_du_connector_atomic_set_property
On Tue, Oct 02, 2018 at 04:36:31PM +, 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 > >> Cc: VMware Graphics > >> Cc: Sinclair Yeh > >> Cc: Thomas Hellstrom > >> --- > >> 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. -- Ville Syrjälä Intel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 04/18] drm/vmwgfx: Remove confused comment from vmw_du_connector_atomic_set_property
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 >> Cc: VMware Graphics >> Cc: Sinclair Yeh >> Cc: Thomas Hellstrom >> --- >> 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-...@lists.freedesktop.org >> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fintel-gfxdata=02%7C01%7Cthellstrom%40vmware.com%7C8376824afaaa4e7ebd6808d6287a0a88%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636740901969428557sdata=JDQsTWKhvZAyUnW76dNMFGm0nzJIJjNrSSJYtDuqDlg%3Dreserved=0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 04/18] drm/vmwgfx: Remove confused comment from vmw_du_connector_atomic_set_property
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 > Cc: VMware Graphics > Cc: Sinclair Yeh > Cc: Thomas Hellstrom > --- > 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-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel