Re: [PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic update

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sebastian,

Thank you for the patch.

On Tuesday 08 Mar 2016 17:39:44 Sebastian Reichel wrote:
> This is a workaround for a hardware bug occuring
> on OMAP3 with manually updated panels.

Could you please explain what the bug is and how the workaround operates ? Do 
you have a reference to an errata document ?

> Signed-off-By: Sebastian Reichel <sre@kernel.org>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h   |  1 +
>  drivers/gpu/drm/omapdrm/omap_plane.c | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> b/drivers/gpu/drm/omapdrm/omap_drv.h index 71e2c2284b86..3ab4919aff4b
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -161,6 +161,7 @@ struct drm_plane *omap_plane_init(struct drm_device
> *dev, int id, enum drm_plane_type type);
>  void omap_plane_install_properties(struct drm_plane *plane,
>  		struct drm_mode_object *obj);
> +void omap_plane_update_fifo(struct drm_plane *plane);
> 
>  struct drm_encoder *omap_encoder_init(struct drm_device *dev,
>  		struct omap_dss_device *dssdev);
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c index d75b197eff46..0147e416140c
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -75,6 +75,28 @@ static void omap_plane_cleanup_fb(struct drm_plane
> *plane, omap_framebuffer_unpin(old_state->fb);
>  }
> 
> +void omap_plane_update_fifo(struct drm_plane *plane)
> +{
> +	struct omap_plane *omap_plane = to_omap_plane(plane);
> +	struct drm_plane_state *state = plane->state;
> +	struct drm_device *dev = plane->dev;
> +	bool use_fifo_merge = false;
> +	u32 fifo_low, fifo_high;
> +	bool use_manual_update;
> +
> +	if (!dispc_ovl_enabled(omap_plane->id))
> +		return;

Given that this function is called right after dispc_ovl_enable(omap_plane-
>id, true), can this condition be true ?

> +	use_manual_update = omap_crtc_is_manual_updated(state->crtc);
> +
> +	dispc_ovl_compute_fifo_thresholds(omap_plane->id, &fifo_low, 
&fifo_high,
> +			use_fifo_merge, use_manual_update);

You can remove the use_fifo_merge variable and set the argument to false 
directly.

> +
> +	dev_dbg(dev->dev, "update fifo: %d %d", fifo_low, fifo_high);

The two variables are unsigned, you should use %u.

> +	dispc_ovl_set_fifo_threshold(omap_plane->id, fifo_low, fifo_high);

On a side note, shouldn't the dispc_ovl_compute_fifo_thresholds() and 
dispc_ovl_set_fifo_threshold() functions be merged into a single one as 
they're always called together ?

> +}
> +
>  static void omap_plane_atomic_update(struct drm_plane *plane,
>  				     struct drm_plane_state *old_state)
>  {
> @@ -141,6 +163,7 @@ static void omap_plane_atomic_update(struct drm_plane
> *plane, }
> 
>  	dispc_ovl_enable(omap_plane->id, true);
> +	omap_plane_update_fifo(plane);
>  }
> 
>  static void omap_plane_atomic_disable(struct drm_plane *plane,

-- 
Regards,

Laurent Pinchart