[1/7] drm: Don't overwrite UNVERFIED mode status to OK

Commit Message

Ville Syrjälä Dec. 3, 2015, 9:14 p.m. UTC

From: Ville Syrjälä <[e-mail]>

The way the mode probing works is this:
1. All modes currently on the mode list are marked as UNVERIFIED
2. New modes are on the probed_modes list (they start with
   status OK)
3. Modes are moved from the probed_modes list to the actual
   mode list. If a mode already on the mode list is deemed
   to match one of the probed modes, the duplicate is dropped
   and the mode status updated to OK. After this the
   probed_modes list will be empty.
4. All modes on the mode list are verified to not violate any
   constraints. Any that do are marked as such.
5. Any mode left with a non-OK status is pruned from the list,
   with an appropriate debug message.

What all this means is that any mode on the original list that
didn't have a duplicate on the probed_modes list, should be left
with status UNVERFIED (or previously could have been left with
some other status, but never OK).

I broke that in
commit 05acaec334fc ("drm: Reorganize probed mode validation")
by always assigning something to the mode->status during the validation
step. So any mode from the old list that still passed the validation
would be left on the list with status OK in the end.

Fix this by not doing the basic mode validation unless the mode
already has status OK (meaning it came from the probed_modes list,
or at least a duplicate of it was on that list). This way we will
correctly prune away any mode from the old mode list that didn't
appear on the probed_modes list.

Cc: [e-mail]
Cc: Adam Jackson <[e-mail]>
Fixes: 05acaec334fc ("drm: Reorganize probed mode validation")
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/drm_probe_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Dec. 4, 2015, 8:17 a.m. UTC | #1

On Thu, Dec 03, 2015 at 11:14:09PM +0200, [e-mail] wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> The way the mode probing works is this:
> 1. All modes currently on the mode list are marked as UNVERIFIED
> 2. New modes are on the probed_modes list (they start with
>    status OK)
> 3. Modes are moved from the probed_modes list to the actual
>    mode list. If a mode already on the mode list is deemed
>    to match one of the probed modes, the duplicate is dropped
>    and the mode status updated to OK. After this the
>    probed_modes list will be empty.
> 4. All modes on the mode list are verified to not violate any
>    constraints. Any that do are marked as such.
> 5. Any mode left with a non-OK status is pruned from the list,
>    with an appropriate debug message.

This would look really pretty as a kerneldoc addition to
probe_single_connector(). And with asciidoc we can even do pretty ordered
lists like these. Can you please follow-up with a patch for that?

> 
> What all this means is that any mode on the original list that
> didn't have a duplicate on the probed_modes list, should be left
> with status UNVERFIED (or previously could have been left with
> some other status, but never OK).
> 
> I broke that in
> commit 05acaec334fc ("drm: Reorganize probed mode validation")
> by always assigning something to the mode->status during the validation
> step. So any mode from the old list that still passed the validation
> would be left on the list with status OK in the end.
> 
> Fix this by not doing the basic mode validation unless the mode
> already has status OK (meaning it came from the probed_modes list,
> or at least a duplicate of it was on that list). This way we will
> correctly prune away any mode from the old mode list that didn't
> appear on the probed_modes list.
> 
> Cc: [e-mail]
> Cc: Adam Jackson <[e-mail]>
> Fixes: 05acaec334fc ("drm: Reorganize probed mode validation")
> Signed-off-by: Ville Syrjälä <[e-mail]>

Reviewed-by: Daniel Vetter <[e-mail]>

> ---
>  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 94ba39e34299..b9b3bd9349ff 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -229,7 +229,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>  		mode_flags |= DRM_MODE_FLAG_3D_MASK;
>  
>  	list_for_each_entry(mode, &connector->modes, head) {
> -		mode->status = drm_mode_validate_basic(mode);
> +		if (mode->status == MODE_OK)
> +			mode->status = drm_mode_validate_basic(mode);
>  
>  		if (mode->status == MODE_OK)
>  			mode->status = drm_mode_validate_size(mode, maxX, maxY);
> -- 
> 2.4.10
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä Dec. 4, 2015, 12:23 p.m. UTC | #2

On Fri, Dec 04, 2015 at 09:17:28AM +0100, Daniel Vetter wrote:
> On Thu, Dec 03, 2015 at 11:14:09PM +0200, [e-mail] wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > The way the mode probing works is this:
> > 1. All modes currently on the mode list are marked as UNVERIFIED
> > 2. New modes are on the probed_modes list (they start with
> >    status OK)
> > 3. Modes are moved from the probed_modes list to the actual
> >    mode list. If a mode already on the mode list is deemed
> >    to match one of the probed modes, the duplicate is dropped
> >    and the mode status updated to OK. After this the
> >    probed_modes list will be empty.
> > 4. All modes on the mode list are verified to not violate any
> >    constraints. Any that do are marked as such.
> > 5. Any mode left with a non-OK status is pruned from the list,
> >    with an appropriate debug message.
> 
> This would look really pretty as a kerneldoc addition to
> probe_single_connector(). And with asciidoc we can even do pretty ordered
> lists like these. Can you please follow-up with a patch for that?

I can try.

> 
> > 
> > What all this means is that any mode on the original list that
> > didn't have a duplicate on the probed_modes list, should be left
> > with status UNVERFIED (or previously could have been left with
> > some other status, but never OK).
> > 
> > I broke that in
> > commit 05acaec334fc ("drm: Reorganize probed mode validation")
> > by always assigning something to the mode->status during the validation
> > step. So any mode from the old list that still passed the validation
> > would be left on the list with status OK in the end.
> > 
> > Fix this by not doing the basic mode validation unless the mode
> > already has status OK (meaning it came from the probed_modes list,
> > or at least a duplicate of it was on that list). This way we will
> > correctly prune away any mode from the old mode list that didn't
> > appear on the probed_modes list.
> > 
> > Cc: [e-mail]
> > Cc: Adam Jackson <[e-mail]>
> > Fixes: 05acaec334fc ("drm: Reorganize probed mode validation")
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> Reviewed-by: Daniel Vetter <[e-mail]>
> 
> > ---
> >  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 94ba39e34299..b9b3bd9349ff 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -229,7 +229,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
> >  		mode_flags |= DRM_MODE_FLAG_3D_MASK;
> >  
> >  	list_for_each_entry(mode, &connector->modes, head) {
> > -		mode->status = drm_mode_validate_basic(mode);
> > +		if (mode->status == MODE_OK)
> > +			mode->status = drm_mode_validate_basic(mode);
> >  
> >  		if (mode->status == MODE_OK)
> >  			mode->status = drm_mode_validate_size(mode, maxX, maxY);
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Ville Syrjälä Dec. 10, 2015, 9:07 p.m. UTC | #3

On Fri, Dec 04, 2015 at 09:17:28AM +0100, Daniel Vetter wrote:
> On Thu, Dec 03, 2015 at 11:14:09PM +0200, [e-mail] wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > The way the mode probing works is this:
> > 1. All modes currently on the mode list are marked as UNVERIFIED
> > 2. New modes are on the probed_modes list (they start with
> >    status OK)
> > 3. Modes are moved from the probed_modes list to the actual
> >    mode list. If a mode already on the mode list is deemed
> >    to match one of the probed modes, the duplicate is dropped
> >    and the mode status updated to OK. After this the
> >    probed_modes list will be empty.
> > 4. All modes on the mode list are verified to not violate any
> >    constraints. Any that do are marked as such.
> > 5. Any mode left with a non-OK status is pruned from the list,
> >    with an appropriate debug message.
> 
> This would look really pretty as a kerneldoc addition to
> probe_single_connector(). And with asciidoc we can even do pretty ordered
> lists like these. Can you please follow-up with a patch for that?
> 
> > 
> > What all this means is that any mode on the original list that
> > didn't have a duplicate on the probed_modes list, should be left
> > with status UNVERFIED (or previously could have been left with
> > some other status, but never OK).
> > 
> > I broke that in
> > commit 05acaec334fc ("drm: Reorganize probed mode validation")
> > by always assigning something to the mode->status during the validation
> > step. So any mode from the old list that still passed the validation
> > would be left on the list with status OK in the end.
> > 
> > Fix this by not doing the basic mode validation unless the mode
> > already has status OK (meaning it came from the probed_modes list,
> > or at least a duplicate of it was on that list). This way we will
> > correctly prune away any mode from the old mode list that didn't
> > appear on the probed_modes list.
> > 
> > Cc: [e-mail]
> > Cc: Adam Jackson <[e-mail]>
> > Fixes: 05acaec334fc ("drm: Reorganize probed mode validation")
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> Reviewed-by: Daniel Vetter <[e-mail]>

Testcase: igt/kms_force_connector_basic/prune-stale-modes
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93332

> 
> > ---
> >  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 94ba39e34299..b9b3bd9349ff 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -229,7 +229,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
> >  		mode_flags |= DRM_MODE_FLAG_3D_MASK;
> >  
> >  	list_for_each_entry(mode, &connector->modes, head) {
> > -		mode->status = drm_mode_validate_basic(mode);
> > +		if (mode->status == MODE_OK)
> > +			mode->status = drm_mode_validate_basic(mode);
> >  
> >  		if (mode->status == MODE_OK)
> >  			mode->status = drm_mode_validate_size(mode, maxX, maxY);
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Patch

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 94ba39e34299..b9b3bd9349ff 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -229,7 +229,8 @@  static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 		mode_flags |= DRM_MODE_FLAG_3D_MASK;
 
 	list_for_each_entry(mode, &connector->modes, head) {
-		mode->status = drm_mode_validate_basic(mode);
+		if (mode->status == MODE_OK)
+			mode->status = drm_mode_validate_basic(mode);
 
 		if (mode->status == MODE_OK)
 			mode->status = drm_mode_validate_size(mode, maxX, maxY);