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

[Jani Nikula <jani.nikula@intel.com>]

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