[PATCH] drm/fb: Proper support of boundary conditions in bitmasks.

[PATCH] drm/fb: Proper support of boundary conditions in bitmasks.

[Tomasz Lis]

The recently introduced patch changed behavior of masks when
the bit number is negative. Instead of no bits set, the new way
makes all bits set. Problematic patch:
  drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)

This behaviour was not considered when making changes, and boundary
value of count (=0) is now resulting in a mask with all bits on, since
the value is directly decreased and therefore negative. Checking if
all bits are set leads to infinite loop.

This patch introduces an additional check to avoid empty masks. It
reverts the control flow to the exact same way it worked before
the problematic patch.

Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index e6f3eb2d..bc65ecf 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -496,7 +496,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		conn_configured |= BIT(i);
 	}
 
-	if ((conn_configured & mask) != mask) {
+	if (count > 0 && (conn_configured & mask) != mask) {
 		pass++;
 		goto retry;
 	}
-- 
2.7.4

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

[RFC] drm/fb: Avoid infinite loop when no response from connector.

[Tomasz Lis]

This fixes an old patch so it doesn't cause infinite retries:
  drm/fb: add support for tiled monitor configurations.

The max count of iterations, 0xa10070f, was carefully selected based on the fact
that it looks cool.
---
 drivers/gpu/drm/drm_fb_helper.c    | 4 +++-
 drivers/gpu/drm/i915/intel_fbdev.c | 5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0dd5da8..8e6c535 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2011,7 +2011,9 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
 
 	if ((conn_configured & mask) != mask) {
 		tile_pass++;
-		goto retry;
+		if (tile_pass < 0xa10070f)
+			goto retry;
+		DRM_ERROR("Max connector check retry count exceeded\n");
 	}
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index bc65ecf..2fd0f09 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -498,7 +498,10 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 
 	if (count > 0 && (conn_configured & mask) != mask) {
 		pass++;
-		goto retry;
+		if (pass < 0xa10070f)
+			goto retry;
+		DRM_ERROR("Max connector check retry count exceeded\n");
+		goto bail;
 	}
 
 	/*
-- 
2.7.4

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

Re: [RFC] drm/fb: Avoid infinite loop when no response from connector.

[Arkadiusz Hiler]

On Fri, Feb 17, 2017 at 11:17:46AM +0100, Tomasz Lis wrote:
> This fixes an old patch so it doesn't cause infinite retries:
>   drm/fb: add support for tiled monitor configurations.
> 
> The max count of iterations, 0xa10070f, was carefully selected based on the fact
> that it looks cool.
> ---
>  drivers/gpu/drm/drm_fb_helper.c    | 4 +++-
>  drivers/gpu/drm/i915/intel_fbdev.c | 5 ++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c

Change to that file should go through dri-devel.

> index 0dd5da8..8e6c535 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2011,7 +2011,9 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
>  
>  	if ((conn_configured & mask) != mask) {
>  		tile_pass++;
> -		goto retry;
> +		if (tile_pass < 0xa10070f)

Can we have this named? Also it could be more sensible to go with
something representing order of how many retries we want rather than
going full wizard on it?

> +			goto retry;
> +		DRM_ERROR("Max connector check retry count exceeded\n");
>  	}
>  	return true;
>  }

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

Re: [PATCH] drm/fb: Proper support of boundary conditions in bitmasks.

[Arkadiusz Hiler]

On Fri, Feb 17, 2017 at 11:17:45AM +0100, Tomasz Lis wrote:
> The recently introduced patch changed behavior of masks when
> the bit number is negative. Instead of no bits set, the new way
> makes all bits set. Problematic patch:
>   drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)
> 
> This behaviour was not considered when making changes, and boundary
> value of count (=0) is now resulting in a mask with all bits on, since
> the value is directly decreased and therefore negative. Checking if
> all bits are set leads to infinite loop.
> 
> This patch introduces an additional check to avoid empty masks. It
> reverts the control flow to the exact same way it worked before
> the problematic patch.
> 
> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>

Change commit tile to "drm/i915/fbdev: Proper support of boundary".

Other than that:
Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>



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

Re: [RFC] drm/fb: Avoid infinite loop when no response from connector.

[Chris Wilson]

On Fri, Feb 17, 2017 at 01:40:38PM +0100, Arkadiusz Hiler wrote:
> On Fri, Feb 17, 2017 at 11:17:46AM +0100, Tomasz Lis wrote:
> > This fixes an old patch so it doesn't cause infinite retries:
> >   drm/fb: add support for tiled monitor configurations.
> > 
> > The max count of iterations, 0xa10070f, was carefully selected based on the fact
> > that it looks cool.
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c    | 4 +++-
> >  drivers/gpu/drm/i915/intel_fbdev.c | 5 ++++-
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> 
> Change to that file should go through dri-devel.
> 
> > index 0dd5da8..8e6c535 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2011,7 +2011,9 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
> >  
> >  	if ((conn_configured & mask) != mask) {
> >  		tile_pass++;
> > -		goto retry;
> > +		if (tile_pass < 0xa10070f)
> 
> Can we have this named? Also it could be more sensible to go with
> something representing order of how many retries we want rather than
> going full wizard on it?

A more suitable approach might be:

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 041322fef607..32d0af6b07a7 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -348,7 +348,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
                                    bool *enabled, int width, int height)
 {
        struct drm_i915_private *dev_priv = to_i915(fb_helper->dev);
-       unsigned long conn_configured, mask;
+       unsigned long conn_configured, conn_seq, mask;
        unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
        int i, j;
        bool *save_enabled;
@@ -365,6 +365,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
        mask = GENMASK(count - 1, 0);
        conn_configured = 0;
 retry:
+       conn_seq = conn_configured;
        for (i = 0; i < count; i++) {
                struct drm_fb_helper_connector *fb_conn;
                struct drm_connector *connector;
@@ -489,10 +490,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
                conn_configured |= BIT(i);
        }
 
-       if ((conn_configured & mask) != mask) {
-               pass++;
+       if ((conn_configured & mask) != mask && conn_configured != conn_seq)
                goto retry;
-       }
 
        /*
         * If the BIOS didn't enable everything it could, fall back to have the

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/fbdev: Stop repeating tile configuration on stagnation

[Chris Wilson]

If we cease making progress in finding matching outputs for a tiled
configuration, stop looping over the remaining unconfigured outputs.

Fixes: b0ee9e7fa5b4 ("drm/fb: add support for tiled monitor configurations. (v2)")
Cc: Tomasz Lis <tomasz.lis@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: <stable@vger.kernel.org> # v3.19+
---
 drivers/gpu/drm/i915/intel_fbdev.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 041322fef607..32d0af6b07a7 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -348,7 +348,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 				    bool *enabled, int width, int height)
 {
 	struct drm_i915_private *dev_priv = to_i915(fb_helper->dev);
-	unsigned long conn_configured, mask;
+	unsigned long conn_configured, conn_seq, mask;
 	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
 	int i, j;
 	bool *save_enabled;
@@ -365,6 +365,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 	mask = GENMASK(count - 1, 0);
 	conn_configured = 0;
 retry:
+	conn_seq = conn_configured;
 	for (i = 0; i < count; i++) {
 		struct drm_fb_helper_connector *fb_conn;
 		struct drm_connector *connector;
@@ -489,10 +490,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		conn_configured |= BIT(i);
 	}
 
-	if ((conn_configured & mask) != mask) {
-		pass++;
+	if ((conn_configured & mask) != mask && conn_configured != conn_seq)
 		goto retry;
-	}
 
 	/*
 	 * If the BIOS didn't enable everything it could, fall back to have the
-- 
2.11.0

[PATCH] drm/i915/fbdev: Stop repeating tile configuration on stagnation

[Chris Wilson]

If we cease making progress in finding matching outputs for a tiled
configuration, stop looping over the remaining unconfigured outputs.

Fixes: b0ee9e7fa5b4 ("drm/fb: add support for tiled monitor configurations. (v2)")
Cc: Tomasz Lis <tomasz.lis@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: <stable@vger.kernel.org> # v3.19+
---
 drivers/gpu/drm/i915/intel_fbdev.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 041322fef607..32d0af6b07a7 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -348,7 +348,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 				    bool *enabled, int width, int height)
 {
 	struct drm_i915_private *dev_priv = to_i915(fb_helper->dev);
-	unsigned long conn_configured, mask;
+	unsigned long conn_configured, conn_seq, mask;
 	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
 	int i, j;
 	bool *save_enabled;
@@ -365,6 +365,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 	mask = GENMASK(count - 1, 0);
 	conn_configured = 0;
 retry:
+	conn_seq = conn_configured;
 	for (i = 0; i < count; i++) {
 		struct drm_fb_helper_connector *fb_conn;
 		struct drm_connector *connector;
@@ -489,10 +490,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		conn_configured |= BIT(i);
 	}
 
-	if ((conn_configured & mask) != mask) {
-		pass++;
+	if ((conn_configured & mask) != mask && conn_configured != conn_seq)
 		goto retry;
-	}
 
 	/*
 	 * If the BIOS didn't enable everything it could, fall back to have the
-- 
2.11.0

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

✓ Fi.CI.BAT: success for drm/fb: Proper support of boundary conditions in bitmasks. (rev4)

[Patchwork]

== Series Details ==

Series: drm/fb: Proper support of boundary conditions in bitmasks. (rev4)
URL   : https://patchwork.freedesktop.org/series/19826/
State : success

== Summary ==

Series 19826v4 drm/fb: Proper support of boundary conditions in bitmasks.
https://patchwork.freedesktop.org/api/1.0/series/19826/revisions/4/mbox/

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

d13370a042e9d05329bcac34eb43c64e4f6704e0 drm-tip: 2017y-02m-17d-21h-23m-28s UTC integration manifest
671f065 drm/i915/fbdev: Stop repeating tile configuration on stagnation

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3895/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/fb: Proper support of boundary conditions in bitmasks.

[Jani Nikula]

On Fri, 17 Feb 2017, Tomasz Lis <tomasz.lis@intel.com> wrote:
> The recently introduced patch changed behavior of masks when
> the bit number is negative. Instead of no bits set, the new way
> makes all bits set. Problematic patch:
>   drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)

For future reference, please find the commit id of the committed patch,
and reference that with the Fixes: tag. Please Cc the folks from the
commit.

Whoever commits this must add:

Fixes: 3c779a49bd7c ("drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)")
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>

Joonas, Chris, please check the rest of the regressing commit that it
doesn't suffer from the same issue.

Thanks,
Jani.

> This behaviour was not considered when making changes, and boundary
> value of count (=0) is now resulting in a mask with all bits on, since
> the value is directly decreased and therefore negative. Checking if
> all bits are set leads to infinite loop.
>
> This patch introduces an additional check to avoid empty masks. It
> reverts the control flow to the exact same way it worked before
> the problematic patch.
>
> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index e6f3eb2d..bc65ecf 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -496,7 +496,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  		conn_configured |= BIT(i);
>  	}
>  
> -	if ((conn_configured & mask) != mask) {
> +	if (count > 0 && (conn_configured & mask) != mask) {
>  		pass++;
>  		goto retry;
>  	}

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/fbdev: Stop repeating tile configuration on stagnation

[Tomasz Lis]

I tested this variant (reverted the change to "pass" variable before 
testing), and it fixes the issue with count=0 as well as possible 
infinite loop issue.

But something needs to be done with the "pass" var, one way or the other 
- commented below.


W dniu 2017-02-18 o 16:37, Chris Wilson pisze:
> If we cease making progress in finding matching outputs for a tiled
> configuration, stop looping over the remaining unconfigured outputs.
>
> Fixes: b0ee9e7fa5b4 ("drm/fb: add support for tiled monitor configurations. (v2)")
> Cc: Tomasz Lis <tomasz.lis@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v3.19+
> ---
>   drivers/gpu/drm/i915/intel_fbdev.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 041322fef607..32d0af6b07a7 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -348,7 +348,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>   				    bool *enabled, int width, int height)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(fb_helper->dev);
> -	unsigned long conn_configured, mask;
> +	unsigned long conn_configured, conn_seq, mask;
>   	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
>   	int i, j;
>   	bool *save_enabled;
> @@ -365,6 +365,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>   	mask = GENMASK(count - 1, 0);
>   	conn_configured = 0;
>   retry:
> +	conn_seq = conn_configured;
>   	for (i = 0; i < count; i++) {
>   		struct drm_fb_helper_connector *fb_conn;
>   		struct drm_connector *connector;
> @@ -489,10 +490,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>   		conn_configured |= BIT(i);
>   	}
>   
> -	if ((conn_configured & mask) != mask) {
> -		pass++;
This doesn't seem right; increasing the amount of passes should stay, or 
the use of "pass" variable should be completely replaced by conn_seq.
- Tomasz
> +	if ((conn_configured & mask) != mask && conn_configured != conn_seq)
>   		goto retry;
> -	}
>   
>   	/*
>   	 * If the BIOS didn't enable everything it could, fall back to have the

Re: [PATCH] drm/i915/fbdev: Stop repeating tile configuration on stagnation

[Tomasz Lis]

I tested this variant (reverted the change to "pass" variable before 
testing), and it fixes the issue with count=0 as well as possible 
infinite loop issue.

But something needs to be done with the "pass" var, one way or the other 
- commented below.


W dniu 2017-02-18 o 16:37, Chris Wilson pisze:
> If we cease making progress in finding matching outputs for a tiled
> configuration, stop looping over the remaining unconfigured outputs.
>
> Fixes: b0ee9e7fa5b4 ("drm/fb: add support for tiled monitor configurations. (v2)")
> Cc: Tomasz Lis <tomasz.lis@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v3.19+
> ---
>   drivers/gpu/drm/i915/intel_fbdev.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 041322fef607..32d0af6b07a7 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -348,7 +348,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>   				    bool *enabled, int width, int height)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(fb_helper->dev);
> -	unsigned long conn_configured, mask;
> +	unsigned long conn_configured, conn_seq, mask;
>   	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
>   	int i, j;
>   	bool *save_enabled;
> @@ -365,6 +365,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>   	mask = GENMASK(count - 1, 0);
>   	conn_configured = 0;
>   retry:
> +	conn_seq = conn_configured;
>   	for (i = 0; i < count; i++) {
>   		struct drm_fb_helper_connector *fb_conn;
>   		struct drm_connector *connector;
> @@ -489,10 +490,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>   		conn_configured |= BIT(i);
>   	}
>   
> -	if ((conn_configured & mask) != mask) {
> -		pass++;
This doesn't seem right; increasing the amount of passes should stay, or 
the use of "pass" variable should be completely replaced by conn_seq.
- Tomasz
> +	if ((conn_configured & mask) != mask && conn_configured != conn_seq)
>   		goto retry;
> -	}
>   
>   	/*
>   	 * If the BIOS didn't enable everything it could, fall back to have the

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

Re: [PATCH] drm/i915/fbdev: Stop repeating tile configuration on stagnation

[Chris Wilson]

On Mon, Feb 20, 2017 at 04:37:47PM +0100, Tomasz Lis wrote:
> I tested this variant (reverted the change to "pass" variable before
> testing), and it fixes the issue with count=0 as well as possible
> infinite loop issue.
> 
> But something needs to be done with the "pass" var, one way or the
> other - commented below.
> 
> 
> W dniu 2017-02-18 o 16:37, Chris Wilson pisze:
> >If we cease making progress in finding matching outputs for a tiled
> >configuration, stop looping over the remaining unconfigured outputs.
> >
> >Fixes: b0ee9e7fa5b4 ("drm/fb: add support for tiled monitor configurations. (v2)")
> >Cc: Tomasz Lis <tomasz.lis@intel.com>
> >Cc: Dave Airlie <airlied@redhat.com>
> >Cc: Daniel Vetter <daniel.vetter@intel.com>
> >Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >Cc: Sean Paul <seanpaul@chromium.org>
> >Cc: <stable@vger.kernel.org> # v3.19+
> >---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> >index 041322fef607..32d0af6b07a7 100644
> >--- a/drivers/gpu/drm/i915/intel_fbdev.c
> >+++ b/drivers/gpu/drm/i915/intel_fbdev.c
> >@@ -348,7 +348,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >  				    bool *enabled, int width, int height)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(fb_helper->dev);
> >-	unsigned long conn_configured, mask;
> >+	unsigned long conn_configured, conn_seq, mask;
> >  	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
> >  	int i, j;
> >  	bool *save_enabled;
> >@@ -365,6 +365,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >  	mask = GENMASK(count - 1, 0);
> >  	conn_configured = 0;
> >  retry:
> >+	conn_seq = conn_configured;
> >  	for (i = 0; i < count; i++) {
> >  		struct drm_fb_helper_connector *fb_conn;
> >  		struct drm_connector *connector;
> >@@ -489,10 +490,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >  		conn_configured |= BIT(i);
> >  	}
> >-	if ((conn_configured & mask) != mask) {
> >-		pass++;
> This doesn't seem right; increasing the amount of passes should
> stay, or the use of "pass" variable should be completely replaced by
> conn_seq.

Good catch.

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 32d0af6b07a7..f7e9a4e69595 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -355,7 +355,6 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
        bool fallback = true;
        int num_connectors_enabled = 0;
        int num_connectors_detected = 0;
-       int pass = 0;
 
        save_enabled = kcalloc(count, sizeof(bool), GFP_KERNEL);
        if (!save_enabled)
@@ -379,7 +378,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
                if (conn_configured & BIT(i))
                        continue;
 
-               if (pass == 0 && !connector->has_tile)
+               if (conn_seq == 0 && !connector->has_tile)
                        continue;
 
                if (connector->status == connector_status_connected)

Suggestions for a better name than conn_seq much appreciated.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/fb: Proper support of boundary conditions in bitmasks.

[Joonas Lahtinen]

On ma, 2017-02-20 at 10:00 +0200, Jani Nikula wrote:
> On Fri, 17 Feb 2017, Tomasz Lis <tomasz.lis@intel.com> wrote:
> > 
> > The recently introduced patch changed behavior of masks when
> > the bit number is negative. Instead of no bits set, the new way
> > makes all bits set. Problematic patch:
> >   drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)
> 
> For future reference, please find the commit id of the committed patch,
> and reference that with the Fixes: tag. Please Cc the folks from the
> commit.
> 
> Whoever commits this must add:
> 
> Fixes: 3c779a49bd7c ("drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)")
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> 
> Joonas, Chris, please check the rest of the regressing commit that it
> doesn't suffer from the same issue.
> 
> Thanks,
> Jani.

This line was actually rest of the commit. So all good, thanks for
catching this.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/fbdev: Stop repeating tile configuration on stagnation

[Tomasz Lis]

Looks good to me.

Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>


W dniu 2017-02-20 o 16:46, Chris Wilson pisze:
> On Mon, Feb 20, 2017 at 04:37:47PM +0100, Tomasz Lis wrote:
>> I tested this variant (reverted the change to "pass" variable before
>> testing), and it fixes the issue with count=0 as well as possible
>> infinite loop issue.
>>
>> But something needs to be done with the "pass" var, one way or the
>> other - commented below.
>>
>>
>> W dniu 2017-02-18 o 16:37, Chris Wilson pisze:
>>> If we cease making progress in finding matching outputs for a tiled
>>> configuration, stop looping over the remaining unconfigured outputs.
>>>
>>> Fixes: b0ee9e7fa5b4 ("drm/fb: add support for tiled monitor configurations. (v2)")
>>> Cc: Tomasz Lis <tomasz.lis@intel.com>
>>> Cc: Dave Airlie <airlied@redhat.com>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>> Cc: Sean Paul <seanpaul@chromium.org>
>>> Cc: <stable@vger.kernel.org> # v3.19+
>>> ---
>>>   drivers/gpu/drm/i915/intel_fbdev.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>>> index 041322fef607..32d0af6b07a7 100644
>>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>>> @@ -348,7 +348,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>>>   				    bool *enabled, int width, int height)
>>>   {
>>>   	struct drm_i915_private *dev_priv = to_i915(fb_helper->dev);
>>> -	unsigned long conn_configured, mask;
>>> +	unsigned long conn_configured, conn_seq, mask;
>>>   	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
>>>   	int i, j;
>>>   	bool *save_enabled;
>>> @@ -365,6 +365,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>>>   	mask = GENMASK(count - 1, 0);
>>>   	conn_configured = 0;
>>>   retry:
>>> +	conn_seq = conn_configured;
>>>   	for (i = 0; i < count; i++) {
>>>   		struct drm_fb_helper_connector *fb_conn;
>>>   		struct drm_connector *connector;
>>> @@ -489,10 +490,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>>>   		conn_configured |= BIT(i);
>>>   	}
>>> -	if ((conn_configured & mask) != mask) {
>>> -		pass++;
>> This doesn't seem right; increasing the amount of passes should
>> stay, or the use of "pass" variable should be completely replaced by
>> conn_seq.
> Good catch.
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 32d0af6b07a7..f7e9a4e69595 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -355,7 +355,6 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>          bool fallback = true;
>          int num_connectors_enabled = 0;
>          int num_connectors_detected = 0;
> -       int pass = 0;
>   
>          save_enabled = kcalloc(count, sizeof(bool), GFP_KERNEL);
>          if (!save_enabled)
> @@ -379,7 +378,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>                  if (conn_configured & BIT(i))
>                          continue;
>   
> -               if (pass == 0 && !connector->has_tile)
> +               if (conn_seq == 0 && !connector->has_tile)
>                          continue;
>   
>                  if (connector->status == connector_status_connected)
>
> Suggestions for a better name than conn_seq much appreciated.
> -Chris
>