drm/i915: Always program CSR if CSR is uninitialized

Submitted by Patrik Jakobsson on Oct. 21, 2015, 2:57 p.m.

Details

Message ID 1445439460-4385-1-git-send-email-patrik.jakobsson@linux.intel.com
State New
Headers show
Series "drm/i915: Always program CSR if CSR is uninitialized" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Patrik Jakobsson Oct. 21, 2015, 2:57 p.m.
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@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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))
 		return;
 

Comments

regarding your offline question: yes, I had your patch applied here, so

Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Wed, Oct 21, 2015 at 7:57 AM, Patrik Jakobsson
<patrik.jakobsson@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@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.

With this change also fell free to use a
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


>                 return;
>
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 22, 2015 at 6:07 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> regarding your offline question: yes, I had your patch applied here, so
>
> Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> On Wed, Oct 21, 2015 at 7:57 AM, Patrik Jakobsson
> <patrik.jakobsson@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@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@intel.com>
>
>
>>                 return;
>>
>> --
>> 2.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx