[PATCH] drm/i915: Check for fused or unused pipes

[PATCH] drm/i915: Check for fused or unused pipes

[Mika Kahola]

In case of fused or unused pipes, return early with a warning when reading information
for encoder.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206
Reported-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index f1502a0..522d54f 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct drm_i915_private *dev_priv,
 {
 	struct intel_encoder *encoder;
 
-	if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes))
+	if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map)))
 		return NULL;
 
 	/* MST */
-- 
2.7.4

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

✓ Fi.CI.BAT: success for drm/i915: Check for fused or unused pipes

[Patchwork]

== Series Details ==

Series: drm/i915: Check for fused or unused pipes
URL   : https://patchwork.freedesktop.org/series/35389/
State : success

== Summary ==

Series 35389v1 drm/i915: Check for fused or unused pipes
https://patchwork.freedesktop.org/api/1.0/series/35389/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989 +1
                incomplete -> PASS       (fi-snb-2520m) fdo#103713
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test gem_sync:
        Subgroup basic-all:
                dmesg-fail -> FAIL       (fi-blb-e6850) fdo#104259
        Subgroup basic-each:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-many-each:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-store-all:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-store-each:
                skip       -> PASS       (fi-blb-e6850)
Test gem_tiled_blits:
        Subgroup basic:
                skip       -> PASS       (fi-blb-e6850)
Test gem_tiled_fence_blits:
        Subgroup basic:
                skip       -> PASS       (fi-blb-e6850)
Test gem_wait:
        Subgroup basic-busy-all:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-wait-all:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-await-all:
                skip       -> PASS       (fi-blb-e6850)
Test kms_busy:
        Subgroup basic-flip-a:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-flip-b:
                skip       -> PASS       (fi-blb-e6850)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                skip       -> PASS       (fi-blb-e6850)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-kbl-r) fdo#104172

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104259 https://bugs.freedesktop.org/show_bug.cgi?id=104259
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:434s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:441s
fi-blb-e6850     total:288  pass:222  dwarn:1   dfail:0   fail:1   skip:64  time:392s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:510s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:278s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:500s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:501s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:483s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:470s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:1   dfail:0   fail:0   skip:108 time:263s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:531s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:409s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:416s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:390s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:471s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:426s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:483s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:521s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:466s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:524s
fi-pnv-d510      total:288  pass:221  dwarn:1   dfail:0   fail:1   skip:65  time:598s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:446s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:537s
fi-skl-6700hq    total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:558s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:489s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:552s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:420s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:601s
fi-cnl-y         total:252  pass:226  dwarn:0   dfail:0   fail:0   skip:25 
fi-glk-dsi       total:67   pass:58   dwarn:0   dfail:0   fail:0   skip:8  

ad43db157c69bf7311ba5a0278f282796242f34a drm-tip: 2017y-12m-14d-20h-09m-53s UTC integration manifest
43ccc80310a7 drm/i915: Check for fused or unused pipes

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7499/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Check for fused or unused pipes

[Patchwork]

== Series Details ==

Series: drm/i915: Check for fused or unused pipes
URL   : https://patchwork.freedesktop.org/series/35389/
State : success

== Summary ==

Test kms_flip:
        Subgroup dpms-vs-vblank-race:
                pass       -> DMESG-FAIL (shard-hsw) fdo#103060
        Subgroup vblank-vs-suspend:
                incomplete -> PASS       (shard-hsw) fdo#103375
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
                fail       -> PASS       (shard-snb) fdo#101623

fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623

shard-hsw        total:2646 pass:1493 dwarn:2   dfail:1   fail:10  skip:1139 time:9206s
shard-snb        total:2712 pass:1309 dwarn:1   dfail:0   fail:11  skip:1391 time:8069s
Blacklisted hosts:
shard-apl        total:2666 pass:1653 dwarn:1   dfail:0   fail:27  skip:983 time:13401s
shard-kbl        total:2712 pass:1797 dwarn:1   dfail:0   fail:33  skip:881 time:11008s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7499/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check for fused or unused pipes

[Dhinakaran Pandiyan]

On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote:
> In case of fused or unused pipes, return early with a warning when reading
> information for encoder.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct
> drm_i915_private *dev_priv, {
>  	struct intel_encoder *encoder;
> 
> -	if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes))
> +	if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map)))
>  		return NULL;

I think we should remove the WARN_ON() and just return NULL. The error return 
and the debug message we print in the caller is good enough IMO. 

> 
>  	/* MST */


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

Re: [PATCH] drm/i915: Check for fused or unused pipes

[Jani Nikula]

On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com> wrote:
> On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote:
>> In case of fused or unused pipes, return early with a warning when reading
>> information for encoder.
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206
>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_audio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f 100644
>> --- a/drivers/gpu/drm/i915/intel_audio.c
>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>> @@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct
>> drm_i915_private *dev_priv, {
>>  	struct intel_encoder *encoder;
>> 
>> -	if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes))
>> +	if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map)))
>>  		return NULL;
>
> I think we should remove the WARN_ON() and just return NULL. The error return 
> and the debug message we print in the caller is good enough IMO. 

On the contrary, I think this one is fine with WARN_ON. The commit
message is inaccurate. This will only become a bounds check with the
change, and I think it's important to catch the totally out of bounds
values loudly.

In case of fused or unused pipes, the code will happily pass that check,
and check the av_enc_map for whether the pipe is available or not.

BR,
Jani.


>
>> 
>>  	/* MST */
>
>

-- 
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: Check for fused or unused pipes

[Mika Kahola]

On Fri, 2017-12-15 at 11:27 +0200, Jani Nikula wrote:
> On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.c
> om> wrote:
> > 
> > On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote:
> > > 
> > > In case of fused or unused pipes, return early with a warning
> > > when reading
> > > information for encoder.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206
> > > Reported-by: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_audio.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f
> > > 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -779,7 +779,7 @@ static struct intel_encoder
> > > *get_saved_enc(struct
> > > drm_i915_private *dev_priv, {
> > >  	struct intel_encoder *encoder;
> > > 
> > > -	if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes))
> > > +	if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map)))
> > >  		return NULL;
> > I think we should remove the WARN_ON() and just return NULL. The
> > error return 
> > and the debug message we print in the caller is good enough IMO. 
> On the contrary, I think this one is fine with WARN_ON. The commit
> message is inaccurate. This will only become a bounds check with the
> change, and I think it's important to catch the totally out of bounds
> values loudly.
Well, I need to rephrase that commit message. 
> 
> In case of fused or unused pipes, the code will happily pass that
> check,
> and check the av_enc_map for whether the pipe is available or not.
> 
> BR,
> Jani.
> 
> 
> > 
> > 
> > > 
> > > 
> > >  	/* MST */
> > 
-- 
Mika Kahola - Intel OTC

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

Re: [PATCH] drm/i915: Check for fused or unused pipes

[Dhinakaran Pandiyan]

On Friday, December 15, 2017 11:27:02 AM PST Jani Nikula wrote:
> On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com> 
wrote:
> > On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote:
> >> In case of fused or unused pipes, return early with a warning when
> >> reading
> >> information for encoder.
> >> 
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206
> >> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/i915/intel_audio.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_audio.c
> >> b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f 100644
> >> --- a/drivers/gpu/drm/i915/intel_audio.c
> >> +++ b/drivers/gpu/drm/i915/intel_audio.c
> >> @@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct
> >> drm_i915_private *dev_priv, {
> >> 
> >>  	struct intel_encoder *encoder;
> >> 
> >> -	if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes))
> >> +	if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map)))
> >> 
> >>  		return NULL;
> > 
> > I think we should remove the WARN_ON() and just return NULL. The error
> > return and the debug message we print in the caller is good enough IMO.
> 
> On the contrary, I think this one is fine with WARN_ON. The commit
> message is inaccurate. This will only become a bounds check with the
> change, and I think it's important to catch the totally out of bounds
> values loudly.
> 

My concern was that we don't expose any interface to tell the audio driver how 
many pipes are present, so a WARN_ON would be unreasonable. But on second 
thoughts, audio should have ways to find that information from the hardware.

> In case of fused or unused pipes, the code will happily pass that check,
> and check the av_enc_map for whether the pipe is available or not.
> 
> BR,
> Jani.
> 
> >>  	/* MST */


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

Re: [PATCH] drm/i915: Check for fused or unused pipes

[Jani Nikula]

On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com> wrote:
> On Friday, December 15, 2017 11:27:02 AM PST Jani Nikula wrote:
>> On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com> 
> wrote:
>> > On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote:
>> >> In case of fused or unused pipes, return early with a warning when
>> >> reading
>> >> information for encoder.
>> >> 
>> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206
>> >> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> >> ---
>> >> 
>> >>  drivers/gpu/drm/i915/intel_audio.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> >> b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f 100644
>> >> --- a/drivers/gpu/drm/i915/intel_audio.c
>> >> +++ b/drivers/gpu/drm/i915/intel_audio.c
>> >> @@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct
>> >> drm_i915_private *dev_priv, {
>> >> 
>> >>  	struct intel_encoder *encoder;
>> >> 
>> >> -	if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes))
>> >> +	if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map)))
>> >> 
>> >>  		return NULL;
>> > 
>> > I think we should remove the WARN_ON() and just return NULL. The error
>> > return and the debug message we print in the caller is good enough IMO.
>> 
>> On the contrary, I think this one is fine with WARN_ON. The commit
>> message is inaccurate. This will only become a bounds check with the
>> change, and I think it's important to catch the totally out of bounds
>> values loudly.
>> 
>
> My concern was that we don't expose any interface to tell the audio driver how 
> many pipes are present, so a WARN_ON would be unreasonable. But on second 
> thoughts, audio should have ways to find that information from the hardware.

Yeah. And if it gets that wrong, we'd like to know. It just doesn't know
about fused stuff, which is why checking against num_pipes fails.

BR,
Jani.


>
>> In case of fused or unused pipes, the code will happily pass that check,
>> and check the av_enc_map for whether the pipe is available or not.
>> 
>> BR,
>> Jani.
>> 
>> >>  	/* MST */
>
>

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