[Intel-gfx] [PATCH] drm/i915: Always program CSR if CSR is uninitialized

[Patrik Jakobsson]

On Thu, Oct 22, 2015 at 6:07 PM, Rodrigo Vivi <rodrigo.vivi at gmail.com> wrote:
> regarding your offline question: yes, I had your patch applied here, so
>
> Tested-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>
> On Wed, Oct 21, 2015 at 7:57 AM, Patrik Jakobsson
> <patrik.jakobsson at linux.intel.com> wrote:
>> The current CSR loading code depends on the CSR program memory to be
>> cleared after boot. This is unfortunately not true on all hardware.
>> Instead make use of the FW_UNINITIALIZED state in init and check for
>> FW_LOADED to prevent init path from skipping the actual programming.
>>
>> Signed-off-by: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_csr.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> index 9e530a7..0f7c49e 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -271,7 +271,7 @@ void intel_csr_load_program(struct drm_device *dev)
>>          * Unfortunately the ACPI subsystem doesn't yet give us a way to
>>          * differentiate this, hence figure it out with this hack.
>>          */
>> -       if (I915_READ(CSR_PROGRAM(0)))
>> +       if (I915_READ(CSR_PROGRAM(0)) && dev_priv->csr.state == FW_LOADED)
>>                 return;
>>
>>         mutex_lock(&dev_priv->csr_lock);
>> @@ -425,6 +425,8 @@ void intel_csr_ucode_init(struct drm_device *dev)
>>         struct intel_csr *csr = &dev_priv->csr;
>>         int ret;
>>
>> +       intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
>> +
>>         if (!HAS_CSR(dev))
>
> My bikesheding here is that I like this kind of HAS_FEATURE()
> protection before anything else so we are always sure that useless
> code won't be executed in case you are running it on a platform that
> doesn't have this feature.

This also crossed my mind. My reasoning was that it's nice to
initialize this value instead of leaving it undefined but on the other
hand FW_UNINITIALIZED = 0 so the untouched state is still good. I
don't feel strongly about this so I'll change it and send out a v2.

Thanks for the review!
-Patrik

> With this change also fell free to use a
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>
>
>>                 return;
>>
>> --
>> 2.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx