[PATCH] drm/atomic: Fix bookkeeping with TEST_ONLY.

[PATCH] drm/atomic: Fix bookkeeping with TEST_ONLY.

[Maarten Lankhorst]

Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
"drm/atomic: Cleanup on error properly in the atomic ioctl."
cleaned up some error paths, but didn't fix the TEST_ONLY path.
In the check only case plane->fb shouldn't be updated, and
the vblank events should be cleared as on failure.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d719ce0b10a0..03672a782e84 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1507,7 +1507,8 @@ retry:
 			copied_props++;
 		}
 
-		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) {
+		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props &&
+		    !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
 			plane = obj_to_plane(obj);
 			plane_mask |= (1 << drm_plane_index(plane));
 			plane->old_fb = plane->fb;
@@ -1532,7 +1533,7 @@ retry:
 		ret = drm_atomic_check_only(state);
 		/* _check_only() does not free state, unlike _commit() */
 		if (!ret)
-			drm_atomic_state_free(state);
+			goto free;
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
 		ret = drm_atomic_async_commit(state);
 	} else {
@@ -1566,6 +1567,7 @@ out:
 	}
 
 	if (ret) {
+free:
 		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
 			for_each_crtc_in_state(state, crtc, crtc_state, i) {
 				if (!crtc_state->event)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Fix bookkeeping with TEST_ONLY.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6996
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -2              302/302              300/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-dpms-interruptible      PASS(1)      DMESG_WARN(1)
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible      PASS(1)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Fix bookkeeping with TEST_ONLY.

[Daniel Stone]

Hi,

On 4 August 2015 at 12:34, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> cleaned up some error paths, but didn't fix the TEST_ONLY path.
> In the check only case plane->fb shouldn't be updated, and
> the vblank events should be cleared as on failure.

Bikeshedding a bit ...

An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
to mention this in the commit message; in this case, the main change
is about plane->{,old_}fb.

> @@ -1532,7 +1533,7 @@ retry:
>                 ret = drm_atomic_check_only(state);
>                 /* _check_only() does not free state, unlike _commit() */
>                 if (!ret)
> -                       drm_atomic_state_free(state);
> +                       goto free;
>         } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>                 ret = drm_atomic_async_commit(state);
>         } else {
> @@ -1566,6 +1567,7 @@ out:
>         }
>
>         if (ret) {
> +free:

This is a bit nasty. Can we please move the label above the
conditional and change the condition to (ret || flags & TEST_ONLY)?
Doing that, you could also move the label above the (ret == -EDEADLK)
check, which would cover ->atomic_check needing to grab other states
(global resources?) and failing.

Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Maarten Lankhorst]

Op 27-08-15 om 14:19 schreef Daniel Stone:
> Hi,
>
> On 4 August 2015 at 12:34, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
>> "drm/atomic: Cleanup on error properly in the atomic ioctl."
>> cleaned up some error paths, but didn't fix the TEST_ONLY path.
>> In the check only case plane->fb shouldn't be updated, and
>> the vblank events should be cleared as on failure.
> Bikeshedding a bit ...
>
> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
> to mention this in the commit message; in this case, the main change
> is about plane->{,old_}fb.
Even testing with PAGE_FLIP_EVENT would be useful because
event && !crtc_state->active should not be allowed. In that case test
could succeed but commit could fail.

Though I guess you're right and it's currently not allowed.
>> @@ -1532,7 +1533,7 @@ retry:
>>                 ret = drm_atomic_check_only(state);
>>                 /* _check_only() does not free state, unlike _commit() */
>>                 if (!ret)
>> -                       drm_atomic_state_free(state);
>> +                       goto free;
>>         } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>>                 ret = drm_atomic_async_commit(state);
>>         } else {
>> @@ -1566,6 +1567,7 @@ out:
>>         }
>>
>>         if (ret) {
>> +free:
> This is a bit nasty. Can we please move the label above the
> conditional and change the condition to (ret || flags & TEST_ONLY)?
> Doing that, you could also move the label above the (ret == -EDEADLK)
> check, which would cover ->atomic_check needing to grab other states
> (global resources?) and failing.
If our bookkeeping is correct then it won't be harmful to fixup old_fb.
Cleaning up more old_fb for more planes than initially had fb updates can always
happen anyway, because a modeset will add all affected planes.

How about the below patch? Apply with git am --scissors

-------->8------
Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
"drm/atomic: Cleanup on error properly in the atomic ioctl."
cleaned up some error paths, but allowed -EDEADLK to
leak vblank events.

Additionally check_only was updating plane->fb, which should
not be done when checking a new configuration only.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c2448f42480f..78ffb4965548 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1542,7 +1542,8 @@ retry:
 			copied_props++;
 		}
 
-		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) {
+		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props &&
+		    !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
 			plane = obj_to_plane(obj);
 			plane_mask |= (1 << drm_plane_index(plane));
 			plane->old_fb = plane->fb;
@@ -1564,10 +1565,11 @@ retry:
 	}
 
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
+		/*
+		 * Unlike commit, check_only does not clean up state.
+		 * Below we call drm_atomic_state_free for it.
+		 */
 		ret = drm_atomic_check_only(state);
-		/* _check_only() does not free state, unlike _commit() */
-		if (!ret)
-			drm_atomic_state_free(state);
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
 		ret = drm_atomic_async_commit(state);
 	} else {
@@ -1594,25 +1596,24 @@ out:
 		plane->old_fb = NULL;
 	}
 
+	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+			if (!crtc_state->event)
+				continue;
+
+			destroy_vblank_event(dev, file_priv,
+					     crtc_state->event);
+		}
+	}
+
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);
 		drm_modeset_backoff(&ctx);
 		goto retry;
 	}
 
-	if (ret) {
-		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
-			for_each_crtc_in_state(state, crtc, crtc_state, i) {
-				if (!crtc_state->event)
-					continue;
-
-				destroy_vblank_event(dev, file_priv,
-						     crtc_state->event);
-			}
-		}
-
+	if (ret || arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
 		drm_atomic_state_free(state);
-	}
 
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Ville Syrjälä]

On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
> Op 27-08-15 om 14:19 schreef Daniel Stone:
> > Hi,
> >
> > On 4 August 2015 at 12:34, Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com> wrote:
> >> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> >> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> >> cleaned up some error paths, but didn't fix the TEST_ONLY path.
> >> In the check only case plane->fb shouldn't be updated, and
> >> the vblank events should be cleared as on failure.
> > Bikeshedding a bit ...
> >
> > An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
> > to mention this in the commit message; in this case, the main change
> > is about plane->{,old_}fb.
> Even testing with PAGE_FLIP_EVENT would be useful because
> event && !crtc_state->active should not be allowed. In that case test
> could succeed but commit could fail.

Why would commit fail when the we're in DPMS off? I would suggest it
should be allowed. The operation would just a be a nop from a HW point
of view, all the calculation/checks would still be performed.

> 
> Though I guess you're right and it's currently not allowed.
> >> @@ -1532,7 +1533,7 @@ retry:
> >>                 ret = drm_atomic_check_only(state);
> >>                 /* _check_only() does not free state, unlike _commit() */
> >>                 if (!ret)
> >> -                       drm_atomic_state_free(state);
> >> +                       goto free;
> >>         } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
> >>                 ret = drm_atomic_async_commit(state);
> >>         } else {
> >> @@ -1566,6 +1567,7 @@ out:
> >>         }
> >>
> >>         if (ret) {
> >> +free:
> > This is a bit nasty. Can we please move the label above the
> > conditional and change the condition to (ret || flags & TEST_ONLY)?
> > Doing that, you could also move the label above the (ret == -EDEADLK)
> > check, which would cover ->atomic_check needing to grab other states
> > (global resources?) and failing.
> If our bookkeeping is correct then it won't be harmful to fixup old_fb.
> Cleaning up more old_fb for more planes than initially had fb updates can always
> happen anyway, because a modeset will add all affected planes.
> 
> How about the below patch? Apply with git am --scissors
> 
> -------->8------
> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> cleaned up some error paths, but allowed -EDEADLK to
> leak vblank events.
> 
> Additionally check_only was updating plane->fb, which should
> not be done when checking a new configuration only.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c2448f42480f..78ffb4965548 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1542,7 +1542,8 @@ retry:
>  			copied_props++;
>  		}
>  
> -		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) {
> +		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props &&
> +		    !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
>  			plane = obj_to_plane(obj);
>  			plane_mask |= (1 << drm_plane_index(plane));
>  			plane->old_fb = plane->fb;
> @@ -1564,10 +1565,11 @@ retry:
>  	}
>  
>  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> +		/*
> +		 * Unlike commit, check_only does not clean up state.
> +		 * Below we call drm_atomic_state_free for it.
> +		 */
>  		ret = drm_atomic_check_only(state);
> -		/* _check_only() does not free state, unlike _commit() */
> -		if (!ret)
> -			drm_atomic_state_free(state);
>  	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>  		ret = drm_atomic_async_commit(state);
>  	} else {
> @@ -1594,25 +1596,24 @@ out:
>  		plane->old_fb = NULL;
>  	}
>  
> +	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +			if (!crtc_state->event)
> +				continue;
> +
> +			destroy_vblank_event(dev, file_priv,
> +					     crtc_state->event);
> +		}
> +	}
> +
>  	if (ret == -EDEADLK) {
>  		drm_atomic_state_clear(state);
>  		drm_modeset_backoff(&ctx);
>  		goto retry;
>  	}
>  
> -	if (ret) {
> -		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> -			for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -				if (!crtc_state->event)
> -					continue;
> -
> -				destroy_vblank_event(dev, file_priv,
> -						     crtc_state->event);
> -			}
> -		}
> -
> +	if (ret || arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
>  		drm_atomic_state_free(state);
> -	}
>  
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Maarten Lankhorst]

Op 27-08-15 om 14:48 schreef Ville Syrjälä:
> On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
>> Op 27-08-15 om 14:19 schreef Daniel Stone:
>>> Hi,
>>>
>>> On 4 August 2015 at 12:34, Maarten Lankhorst
>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
>>>> "drm/atomic: Cleanup on error properly in the atomic ioctl."
>>>> cleaned up some error paths, but didn't fix the TEST_ONLY path.
>>>> In the check only case plane->fb shouldn't be updated, and
>>>> the vblank events should be cleared as on failure.
>>> Bikeshedding a bit ...
>>>
>>> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
>>> to mention this in the commit message; in this case, the main change
>>> is about plane->{,old_}fb.
>> Even testing with PAGE_FLIP_EVENT would be useful because
>> event && !crtc_state->active should not be allowed. In that case test
>> could succeed but commit could fail.
> Why would commit fail when the we're in DPMS off? I would suggest it
> should be allowed. The operation would just a be a nop from a HW point
> of view, all the calculation/checks would still be performed.
>
You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Ville Syrjälä]

On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
> Op 27-08-15 om 14:48 schreef Ville Syrjälä:
> > On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
> >> Op 27-08-15 om 14:19 schreef Daniel Stone:
> >>> Hi,
> >>>
> >>> On 4 August 2015 at 12:34, Maarten Lankhorst
> >>> <maarten.lankhorst@linux.intel.com> wrote:
> >>>> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> >>>> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> >>>> cleaned up some error paths, but didn't fix the TEST_ONLY path.
> >>>> In the check only case plane->fb shouldn't be updated, and
> >>>> the vblank events should be cleared as on failure.
> >>> Bikeshedding a bit ...
> >>>
> >>> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
> >>> to mention this in the commit message; in this case, the main change
> >>> is about plane->{,old_}fb.
> >> Even testing with PAGE_FLIP_EVENT would be useful because
> >> event && !crtc_state->active should not be allowed. In that case test
> >> could succeed but commit could fail.
> > Why would commit fail when the we're in DPMS off? I would suggest it
> > should be allowed. The operation would just a be a nop from a HW point
> > of view, all the calculation/checks would still be performed.
> >
> You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.

What's so special about the event here? Just send it out as soon as the
state has been swapped.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Maarten Lankhorst]

Op 27-08-15 om 14:52 schreef Ville Syrjälä:
> On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
>> Op 27-08-15 om 14:48 schreef Ville Syrjälä:
>>> On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
>>>> Op 27-08-15 om 14:19 schreef Daniel Stone:
>>>>> Hi,
>>>>>
>>>>> On 4 August 2015 at 12:34, Maarten Lankhorst
>>>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>>>> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
>>>>>> "drm/atomic: Cleanup on error properly in the atomic ioctl."
>>>>>> cleaned up some error paths, but didn't fix the TEST_ONLY path.
>>>>>> In the check only case plane->fb shouldn't be updated, and
>>>>>> the vblank events should be cleared as on failure.
>>>>> Bikeshedding a bit ...
>>>>>
>>>>> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
>>>>> to mention this in the commit message; in this case, the main change
>>>>> is about plane->{,old_}fb.
>>>> Even testing with PAGE_FLIP_EVENT would be useful because
>>>> event && !crtc_state->active should not be allowed. In that case test
>>>> could succeed but commit could fail.
>>> Why would commit fail when the we're in DPMS off? I would suggest it
>>> should be allowed. The operation would just a be a nop from a HW point
>>> of view, all the calculation/checks would still be performed.
>>>
>> You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.
> What's so special about the event here? Just send it out as soon as the
> state has been swapped.
Previously this has been disallowed for legacy page flips. I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Ville Syrjälä]

On Thu, Aug 27, 2015 at 03:05:38PM +0200, Maarten Lankhorst wrote:
> Op 27-08-15 om 14:52 schreef Ville Syrjälä:
> > On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
> >> Op 27-08-15 om 14:48 schreef Ville Syrjälä:
> >>> On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
> >>>> Op 27-08-15 om 14:19 schreef Daniel Stone:
> >>>>> Hi,
> >>>>>
> >>>>> On 4 August 2015 at 12:34, Maarten Lankhorst
> >>>>> <maarten.lankhorst@linux.intel.com> wrote:
> >>>>>> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> >>>>>> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> >>>>>> cleaned up some error paths, but didn't fix the TEST_ONLY path.
> >>>>>> In the check only case plane->fb shouldn't be updated, and
> >>>>>> the vblank events should be cleared as on failure.
> >>>>> Bikeshedding a bit ...
> >>>>>
> >>>>> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
> >>>>> to mention this in the commit message; in this case, the main change
> >>>>> is about plane->{,old_}fb.
> >>>> Even testing with PAGE_FLIP_EVENT would be useful because
> >>>> event && !crtc_state->active should not be allowed. In that case test
> >>>> could succeed but commit could fail.
> >>> Why would commit fail when the we're in DPMS off? I would suggest it
> >>> should be allowed. The operation would just a be a nop from a HW point
> >>> of view, all the calculation/checks would still be performed.
> >>>
> >> You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.
> > What's so special about the event here? Just send it out as soon as the
> > state has been swapped.
> Previously this has been disallowed for legacy page flips.

I don't think so. Speaking for i915, I think we've just rejected legacy page
flips entirely with the pipe is off on account of drm_vblank_get() failing.

> I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.

We could stick the last vbl count/timestamp in there.

Not allowing means userspace is forced to consider the dpms state
whenever it wants to call the atomic ioctl.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Maarten Lankhorst]

Op 27-08-15 om 15:50 schreef Ville Syrjälä:
> On Thu, Aug 27, 2015 at 03:05:38PM +0200, Maarten Lankhorst wrote:
>> Op 27-08-15 om 14:52 schreef Ville Syrjälä:
>>> On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
>>>> Op 27-08-15 om 14:48 schreef Ville Syrjälä:
>>>>> On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
>>>>>> Op 27-08-15 om 14:19 schreef Daniel Stone:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 4 August 2015 at 12:34, Maarten Lankhorst
>>>>>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>>>>>> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
>>>>>>>> "drm/atomic: Cleanup on error properly in the atomic ioctl."
>>>>>>>> cleaned up some error paths, but didn't fix the TEST_ONLY path.
>>>>>>>> In the check only case plane->fb shouldn't be updated, and
>>>>>>>> the vblank events should be cleared as on failure.
>>>>>>> Bikeshedding a bit ...
>>>>>>>
>>>>>>> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
>>>>>>> to mention this in the commit message; in this case, the main change
>>>>>>> is about plane->{,old_}fb.
>>>>>> Even testing with PAGE_FLIP_EVENT would be useful because
>>>>>> event && !crtc_state->active should not be allowed. In that case test
>>>>>> could succeed but commit could fail.
>>>>> Why would commit fail when the we're in DPMS off? I would suggest it
>>>>> should be allowed. The operation would just a be a nop from a HW point
>>>>> of view, all the calculation/checks would still be performed.
>>>>>
>>>> You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.
>>> What's so special about the event here? Just send it out as soon as the
>>> state has been swapped.
>> Previously this has been disallowed for legacy page flips.
> I don't think so. Speaking for i915, I think we've just rejected legacy page
> flips entirely with the pipe is off on account of drm_vblank_get() failing.
No atomic driver handles this case correctly. You can't get vblank events with the crtc off.
>> I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.
> We could stick the last vbl count/timestamp in there.
>
> Not allowing means userspace is forced to consider the dpms state
> whenever it wants to call the atomic ioctl.
Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Ville Syrjälä]

On Thu, Aug 27, 2015 at 04:00:05PM +0200, Maarten Lankhorst wrote:
> Op 27-08-15 om 15:50 schreef Ville Syrjälä:
> > On Thu, Aug 27, 2015 at 03:05:38PM +0200, Maarten Lankhorst wrote:
> >> Op 27-08-15 om 14:52 schreef Ville Syrjälä:
> >>> On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
> >>>> Op 27-08-15 om 14:48 schreef Ville Syrjälä:
> >>>>> On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
> >>>>>> Op 27-08-15 om 14:19 schreef Daniel Stone:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On 4 August 2015 at 12:34, Maarten Lankhorst
> >>>>>>> <maarten.lankhorst@linux.intel.com> wrote:
> >>>>>>>> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> >>>>>>>> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> >>>>>>>> cleaned up some error paths, but didn't fix the TEST_ONLY path.
> >>>>>>>> In the check only case plane->fb shouldn't be updated, and
> >>>>>>>> the vblank events should be cleared as on failure.
> >>>>>>> Bikeshedding a bit ...
> >>>>>>>
> >>>>>>> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
> >>>>>>> to mention this in the commit message; in this case, the main change
> >>>>>>> is about plane->{,old_}fb.
> >>>>>> Even testing with PAGE_FLIP_EVENT would be useful because
> >>>>>> event && !crtc_state->active should not be allowed. In that case test
> >>>>>> could succeed but commit could fail.
> >>>>> Why would commit fail when the we're in DPMS off? I would suggest it
> >>>>> should be allowed. The operation would just a be a nop from a HW point
> >>>>> of view, all the calculation/checks would still be performed.
> >>>>>
> >>>> You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.
> >>> What's so special about the event here? Just send it out as soon as the
> >>> state has been swapped.
> >> Previously this has been disallowed for legacy page flips.
> > I don't think so. Speaking for i915, I think we've just rejected legacy page
> > flips entirely with the pipe is off on account of drm_vblank_get() failing.
> No atomic driver handles this case correctly. You can't get vblank events with the crtc off.

I don't understand what you're saying. Should there be a comma after
"No" ? If not, then I'm not sure what they don't handle.

> >> I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.
> > We could stick the last vbl count/timestamp in there.
> >
> > Not allowing means userspace is forced to consider the dpms state
> > whenever it wants to call the atomic ioctl.
> Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)

Meh. Much simpler to write code when you don't have to worry about such
details.

In the kernel it should amount to
if (!pipe_active)
	send_event

We anyway need something like that for the crtc getting disabled case,
don't we?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Daniel Stone]

Hi,

On 27 August 2015 at 15:09, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Aug 27, 2015 at 04:00:05PM +0200, Maarten Lankhorst wrote:
>> Op 27-08-15 om 15:50 schreef Ville Syrjälä:
>> > I don't think so. Speaking for i915, I think we've just rejected legacy page
>> > flips entirely with the pipe is off on account of drm_vblank_get() failing.
>> No atomic driver handles this case correctly. You can't get vblank events with the crtc off.
>
> I don't understand what you're saying. Should there be a comma after
> "No" ? If not, then I'm not sure what they don't handle.

I think you're arguing over the definition of 'correctly' perhaps?

>> >> I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.
>> > We could stick the last vbl count/timestamp in there.
>> >
>> > Not allowing means userspace is forced to consider the dpms state
>> > whenever it wants to call the atomic ioctl.
>> Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)
>
> Meh. Much simpler to write code when you don't have to worry about such
> details.
>
> In the kernel it should amount to
> if (!pipe_active)
>         send_event

No, thankyou. Asking for an event, having the request succeed, and
never getting an event, is a deathtrap. PAGE_FLIP_EVENT should mean
that either an event gets delivered for every CRTC in crtc_state, or
the request getting rejected. Nothing else.

> We anyway need something like that for the crtc getting disabled case,
> don't we?

Yes, which I was getting at previously. We know when the CRTC gets
disabled - we have to, in order to sensibly unpin etc - so that's when
the event gets sent.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Ville Syrjälä]

On Thu, Aug 27, 2015 at 03:28:53PM +0100, Daniel Stone wrote:
> Hi,
> 
> On 27 August 2015 at 15:09, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Aug 27, 2015 at 04:00:05PM +0200, Maarten Lankhorst wrote:
> >> Op 27-08-15 om 15:50 schreef Ville Syrjälä:
> >> > I don't think so. Speaking for i915, I think we've just rejected legacy page
> >> > flips entirely with the pipe is off on account of drm_vblank_get() failing.
> >> No atomic driver handles this case correctly. You can't get vblank events with the crtc off.
> >
> > I don't understand what you're saying. Should there be a comma after
> > "No" ? If not, then I'm not sure what they don't handle.
> 
> I think you're arguing over the definition of 'correctly' perhaps?
> 
> >> >> I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.
> >> > We could stick the last vbl count/timestamp in there.
> >> >
> >> > Not allowing means userspace is forced to consider the dpms state
> >> > whenever it wants to call the atomic ioctl.
> >> Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)
> >
> > Meh. Much simpler to write code when you don't have to worry about such
> > details.
> >
> > In the kernel it should amount to
> > if (!pipe_active)
> >         send_event
> 
> No, thankyou. Asking for an event, having the request succeed, and
> never getting an event, is a deathtrap.

Obviously the event gets sent if the operation succeeds. I never
suggested anything else.

> PAGE_FLIP_EVENT should mean
> that either an event gets delivered for every CRTC in crtc_state, or
> the request getting rejected. Nothing else.
> 
> > We anyway need something like that for the crtc getting disabled case,
> > don't we?
> 
> Yes, which I was getting at previously. We know when the CRTC gets
> disabled - we have to, in order to sensibly unpin etc - so that's when
> the event gets sent.
> 
> Cheers,
> Daniel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Daniel Stone]

On 27 August 2015 at 15:34, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Aug 27, 2015 at 03:28:53PM +0100, Daniel Stone wrote:
>> On 27 August 2015 at 15:09, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > In the kernel it should amount to
>> > if (!pipe_active)
>> >         send_event
>>
>> No, thankyou. Asking for an event, having the request succeed, and
>> never getting an event, is a deathtrap.
>
> Obviously the event gets sent if the operation succeeds. I never
> suggested anything else.

Er, yeah. Totally missed the ! in the condition. As you were.

Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Daniel Stone]

Hi,

On 27 August 2015 at 13:43, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 27-08-15 om 14:19 schreef Daniel Stone:
>> On 4 August 2015 at 12:34, Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com> wrote:
>> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
>> to mention this in the commit message; in this case, the main change
>> is about plane->{,old_}fb.
> Even testing with PAGE_FLIP_EVENT would be useful because
> event && !crtc_state->active should not be allowed. In that case test
> could succeed but commit could fail.

Will reply to that in the (very old) thread.

>> This is a bit nasty. Can we please move the label above the
>> conditional and change the condition to (ret || flags & TEST_ONLY)?
>> Doing that, you could also move the label above the (ret == -EDEADLK)
>> check, which would cover ->atomic_check needing to grab other states
>> (global resources?) and failing.
> If our bookkeeping is correct then it won't be harmful to fixup old_fb.
> Cleaning up more old_fb for more planes than initially had fb updates can always
> happen anyway, because a modeset will add all affected planes.

It won't happen: plane_mask will always be 0, because it's only set in
the branch which is now !TEST_ONLY. So yeah, it's safe to just skip
the label completely.

> How about the below patch? Apply with git am --scissors
>
> -------->8------
> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> cleaned up some error paths, but allowed -EDEADLK to
> leak vblank events.
>
> Additionally check_only was updating plane->fb, which should
> not be done when checking a new configuration only.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c2448f42480f..78ffb4965548 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1542,7 +1542,8 @@ retry:
>                         copied_props++;
>                 }
>
> -               if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) {
> +               if (obj->type == DRM_MODE_OBJECT_PLANE && count_props &&
> +                   !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
>                         plane = obj_to_plane(obj);
>                         plane_mask |= (1 << drm_plane_index(plane));
>                         plane->old_fb = plane->fb;
> @@ -1564,10 +1565,11 @@ retry:
>         }
>
>         if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> +               /*
> +                * Unlike commit, check_only does not clean up state.
> +                * Below we call drm_atomic_state_free for it.
> +                */
>                 ret = drm_atomic_check_only(state);
> -               /* _check_only() does not free state, unlike _commit() */
> -               if (!ret)
> -                       drm_atomic_state_free(state);
>         } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>                 ret = drm_atomic_async_commit(state);
>         } else {
> @@ -1594,25 +1596,24 @@ out:
>                 plane->old_fb = NULL;
>         }
>
> +       if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> +               for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +                       if (!crtc_state->event)
> +                               continue;
> +
> +                       destroy_vblank_event(dev, file_priv,
> +                                            crtc_state->event);
> +               }
> +       }

Yeah, moving this looks good for fixing the -EDEADLK leak. So, great.
Can you please add a comment above though noting that we don't need to
call this branch on (ret == 0 && flags & DRM_MODE_ATOMIC_TEST_ONLY),
as we do for freeing the state, because TEST_ONLY and PAGE_FLIP_EVENT
are mutually exclusive? Otherwise the next person to come along and
have the same idea of allowing them is probably going to break this.
:P

With that though, feel free to send it directly with:
Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7274
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              283/283              282/283
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-dpms-interruptible      PASS(1)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx