[05/21] drm/vmwgfx: Remove confused comment from vmw_du_connector_atomic_set_property
diff mbox series

Message ID 20181004202446.22905-6-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • atomic helper cleanup, take 2
Related show

Commit Message

Daniel Vetter Oct. 4, 2018, 8:24 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

Thomas Hellstrom Oct. 4, 2018, 10:40 p.m. UTC | #1
Hi!

I've sent out patches that replace 05/21 and 07/21. Since I'm on travel, 
I'm not sure I'll be able to incorporate them in a pull before the merge 
window though.

/Thomas

On 10/04/2018 10:24 PM, Daniel Vetter wrote:
...
Daniel Vetter Oct. 5, 2018, 6:43 a.m. UTC | #2
On Thu, Oct 04, 2018 at 10:40:16PM +0000, Thomas Hellstrom wrote:
> Hi!
> 
> I've sent out patches that replace 05/21 and 07/21. Since I'm on travel, 
> I'm not sure I'll be able to incorporate them in a pull before the merge 
> window though.

is_implicit is taken care of with those indeed. But patch 07 seems still
needed, at least I'm not seeing where you remove the custom page_flip
checks.
-Daniel
Daniel Vetter Oct. 5, 2018, 7:48 a.m. UTC | #3
On Fri, Oct 05, 2018 at 08:43:51AM +0200, Daniel Vetter wrote:
> On Thu, Oct 04, 2018 at 10:40:16PM +0000, Thomas Hellstrom wrote:
> > Hi!
> > 
> > I've sent out patches that replace 05/21 and 07/21. Since I'm on travel, 
> > I'm not sure I'll be able to incorporate them in a pull before the merge 
> > window though.
> 
> is_implicit is taken care of with those indeed. But patch 07 seems still
> needed, at least I'm not seeing where you remove the custom page_flip
> checks.

Clarification, now that coffee kicks in: I wrote the FIXME comments since
an atomic driver should not have _any_ checks in its page_flip callback
beyond what the helper does. Only thing you can do in there is fix up uapi
inconsistency, because your driver-specific does something funky. Like in
your set_config callback, where you clear a parameter that old versions of
vmwgfx userspace don't clear.

So end result for addressing patch 07 is that you directly put the helper
function into the vtable. You removed a bunch of checks, but there's still
a bit left.

For why this matters: I'd like to, at least long term, move the
legacy2atomic helpers into the core and just enforce them as the default.
That's why I need to know which drivers actually need to patch up things,
and which just have code in there that should be moved to atomic_check.
-Daniel
Thomas Hellstrom Oct. 5, 2018, 6:19 p.m. UTC | #4
On 10/05/2018 09:48 AM, Daniel Vetter wrote:
> On Fri, Oct 05, 2018 at 08:43:51AM +0200, Daniel Vetter wrote:
>> On Thu, Oct 04, 2018 at 10:40:16PM +0000, Thomas Hellstrom wrote:
>>> Hi!
>>>
>>> I've sent out patches that replace 05/21 and 07/21. Since I'm on travel,
>>> I'm not sure I'll be able to incorporate them in a pull before the merge
>>> window though.
>> is_implicit is taken care of with those indeed. But patch 07 seems still
>> needed, at least I'm not seeing where you remove the custom page_flip
>> checks.
> Clarification, now that coffee kicks in: I wrote the FIXME comments since
> an atomic driver should not have _any_ checks in its page_flip callback
> beyond what the helper does. Only thing you can do in there is fix up uapi
> inconsistency, because your driver-specific does something funky. Like in
> your set_config callback, where you clear a parameter that old versions of
> vmwgfx userspace don't clear.
>
> So end result for addressing patch 07 is that you directly put the helper
> function into the vtable. You removed a bunch of checks, but there's still
> a bit left.

Ah, are you referring to the stdu->defined check? I think that's just a 
leftover sanity check that I can remove. Or were you referring to 
something else?

/Thomas
Daniel Vetter Oct. 5, 2018, 6:30 p.m. UTC | #5
On Fri, Oct 5, 2018 at 8:19 PM Thomas Hellstrom <thellstrom@vmware.com> wrote:
>
> On 10/05/2018 09:48 AM, Daniel Vetter wrote:
> > On Fri, Oct 05, 2018 at 08:43:51AM +0200, Daniel Vetter wrote:
> >> On Thu, Oct 04, 2018 at 10:40:16PM +0000, Thomas Hellstrom wrote:
> >>> Hi!
> >>>
> >>> I've sent out patches that replace 05/21 and 07/21. Since I'm on travel,
> >>> I'm not sure I'll be able to incorporate them in a pull before the merge
> >>> window though.
> >> is_implicit is taken care of with those indeed. But patch 07 seems still
> >> needed, at least I'm not seeing where you remove the custom page_flip
> >> checks.
> > Clarification, now that coffee kicks in: I wrote the FIXME comments since
> > an atomic driver should not have _any_ checks in its page_flip callback
> > beyond what the helper does. Only thing you can do in there is fix up uapi
> > inconsistency, because your driver-specific does something funky. Like in
> > your set_config callback, where you clear a parameter that old versions of
> > vmwgfx userspace don't clear.
> >
> > So end result for addressing patch 07 is that you directly put the helper
> > function into the vtable. You removed a bunch of checks, but there's still
> > a bit left.
>
> Ah, are you referring to the stdu->defined check? I think that's just a
> leftover sanity check that I can remove. Or were you referring to
> something else?

Yeah I think that's the last one. I didn't look what that could be doing.
-Daniel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 91ad45b22fee..a50fb0360317 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2303,12 +2303,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;