[04/18] drm/vmwgfx: Remove confused comment from vmw_du_connector_atomic_set_property
diff mbox series

Message ID 20181002133526.13685-5-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • atomic helper cleanups
Related show

Commit Message

Daniel Vetter Oct. 2, 2018, 1:35 p.m. UTC
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(-)

Comments

Ville Syrjälä Oct. 2, 2018, 3:15 p.m. UTC | #1
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
Thomas Hellstrom Oct. 2, 2018, 4:36 p.m. UTC | #2
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&amp;data=02%7C01%7Cthellstrom%40vmware.com%7C8376824afaaa4e7ebd6808d6287a0a88%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636740901969428557&amp;sdata=JDQsTWKhvZAyUnW76dNMFGm0nzJIJjNrSSJYtDuqDlg%3D&amp;reserved=0
Ville Syrjälä Oct. 2, 2018, 5:02 p.m. UTC | #3
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.
Daniel Vetter Oct. 4, 2018, 8:03 p.m. UTC | #4
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&amp;data=02%7C01%7Cthellstrom%40vmware.com%7C8376824afaaa4e7ebd6808d6287a0a88%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636740901969428557&amp;sdata=JDQsTWKhvZAyUnW76dNMFGm0nzJIJjNrSSJYtDuqDlg%3D&amp;reserved=0
> 
>

Patch
diff mbox series

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;