[PATCH] drm/i915: Don't call intel_plane_restore() when the prop value didn't change

[PATCH] drm/i915: Don't call intel_plane_restore() when the prop value didn't change

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

No point in calling intel_plane_restore() in .set_property() if the
value didn't change.

More importantly this papers over a bug where the current primary plane
code forgets to update the user coordinates we store under intel_plane
unless the primary plane .update_plane() hook is actually called. This
means we have 0 in the coordinates straight after boot and any call
to intel_restore_plane() (such as from restore_fbdev_mode()) will
actually turn off the primary plane. This mess needs to be fixed properly
but that's a bigger task and the first step there is killing off
intel_pipe_set_base() and just calling the primary plane
.update_plane() hook. For the immediate problem of black screen after
boot this small patch is enough to hide it.

The problem originates from these two commits:
 commit 3a5f87c286515c54ff5c52c3e64d0c522b7570c0
 Author: Thomas Wood <thomas.wood@intel.com>
 Date:   Wed Aug 20 14:45:00 2014 +0100

    drm: fix plane rotation when restoring fbdev configuration

 commit d91a2cb8e5104233c02bbde539bd4ee455ec12ac
 Author: Sonika Jindal <sonika.jindal@intel.com>
 Date:   Fri Aug 22 14:06:04 2014 +0530

    drm/i915: Add 180 degree primary plane rotation support

Cc: Thomas Wood <thomas.wood@intel.com>
Cc: Sonika Jindal <sonika.jindal@intel.com>
Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index fd5f271..cf596cd 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1232,6 +1232,9 @@ int intel_plane_set_property(struct drm_plane *plane,
 		if (hweight32(val & 0xf) != 1)
 			return -EINVAL;
 
+		if (intel_plane->rotation == val)
+			return 0;
+
 		old_val = intel_plane->rotation;
 		intel_plane->rotation = val;
 		ret = intel_plane_restore(plane);
-- 
1.8.5.5

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

Re: [PATCH] drm/i915: Don't call intel_plane_restore() when the prop value didn't change

[Damien Lespiau]

On Mon, Sep 01, 2014 at 06:08:25PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> No point in calling intel_plane_restore() in .set_property() if the
> value didn't change.
> 
> More importantly this papers over a bug where the current primary plane
> code forgets to update the user coordinates we store under intel_plane
> unless the primary plane .update_plane() hook is actually called. This
> means we have 0 in the coordinates straight after boot and any call
> to intel_restore_plane() (such as from restore_fbdev_mode()) will
> actually turn off the primary plane. This mess needs to be fixed properly
> but that's a bigger task and the first step there is killing off
> intel_pipe_set_base() and just calling the primary plane
> .update_plane() hook. For the immediate problem of black screen after
> boot this small patch is enough to hide it.
> 
> The problem originates from these two commits:
>  commit 3a5f87c286515c54ff5c52c3e64d0c522b7570c0
>  Author: Thomas Wood <thomas.wood@intel.com>
>  Date:   Wed Aug 20 14:45:00 2014 +0100
> 
>     drm: fix plane rotation when restoring fbdev configuration
> 
>  commit d91a2cb8e5104233c02bbde539bd4ee455ec12ac
>  Author: Sonika Jindal <sonika.jindal@intel.com>
>  Date:   Fri Aug 22 14:06:04 2014 +0530
> 
>     drm/i915: Add 180 degree primary plane rotation support
> 
> Cc: Thomas Wood <thomas.wood@intel.com>
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index fd5f271..cf596cd 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1232,6 +1232,9 @@ int intel_plane_set_property(struct drm_plane *plane,
>  		if (hweight32(val & 0xf) != 1)
>  			return -EINVAL;
>  
> +		if (intel_plane->rotation == val)
> +			return 0;
> +
>  		old_val = intel_plane->rotation;
>  		intel_plane->rotation = val;
>  		ret = intel_plane_restore(plane);
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't call intel_plane_restore() when the prop value didn't change

[Damien Lespiau]

On Mon, Sep 01, 2014 at 04:16:59PM +0100, Damien Lespiau wrote:
> > Cc: Thomas Wood <thomas.wood@intel.com>
> > Cc: Sonika Jindal <sonika.jindal@intel.com>
> > Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

This is also:

Tested-by: Alan Stern <stern@rowland.harvard.edu>

-- 
Damien