From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: dri-devel@lists.freedesktop.org
Cc: Sebastian Reichel <sre@kernel.org>, Tony Lindgren <tony@atomide.com>,
        Aaro Koskinen <aaro.koskinen@iki.fi>,
        Tomi Valkeinen <tomi.valkeinen@ti.com>, linux-omap@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic update
Date: Tue, 13 Dec 2016 19:35:48 +0200
Message-ID: <8209536.SBuD0B7HP3@avalon>
User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; )
In-Reply-To: <1457455195-1938-13-git-send-email-sre@kernel.org>
References: <1457455195-1938-1-git-send-email-sre@kernel.org> <1457455195-1938-13-git-send-email-sre@kernel.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3010
Lines: 92

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