Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1541341 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic update Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-12-13 18:40 +0100
    Re: [PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic  update Tomi Valkeinen <tomi.valkeinen@ti.com> - 2016-12-14 09:50 +0100
      Re: [PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic update Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-12-14 10:10 +0100
        Re: [PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic  update Tomi Valkeinen <tomi.valkeinen@ti.com> - 2016-12-14 10:20 +0100
          Re: [PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic  update Sebastian Reichel <sre@kernel.org> - 2016-12-14 15:10 +0100
            Re: [PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic update Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-12-14 22:40 +0100
              Re: [PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic  update Tomi Valkeinen <tomi.valkeinen@ti.com> - 2016-12-15 09:50 +0100

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

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

[toc] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

On 13/12/16 19:35, Laurent Pinchart wrote:
> 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 ?

I don't think I ever found out exactly why the problem happens. But on
OMAP3 DSI, the fifo thresholds had to be tuned slightly, otherwise DISPC
would stop. dispc_ovl_compute_fifo_thresholds() does that tuning if
"manual_update" parameter is set on OMAP3.

 Tomi

[toc] | [prev] | [next] | [standalone]

#1541770

Hi Tomi,

On Wednesday 14 Dec 2016 10:43:18 Tomi Valkeinen wrote:
> On 13/12/16 19:35, Laurent Pinchart wrote:
> > 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 ?
> 
> I don't think I ever found out exactly why the problem happens. But on
> OMAP3 DSI, the fifo thresholds had to be tuned slightly, otherwise DISPC
> would stop. dispc_ovl_compute_fifo_thresholds() does that tuning if
> "manual_update" parameter is set on OMAP3.

I've had a look at dispc_ovl_compute_fifo_thresholds() and the patch makes 
sense to me. If Sebastian could address the small issues I pointed out, we 
could then merge this. Alternatively I can take care of addressing them.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

On 14/12/16 11:10, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Wednesday 14 Dec 2016 10:43:18 Tomi Valkeinen wrote:
>> On 13/12/16 19:35, Laurent Pinchart wrote:
>>> 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 ?
>>
>> I don't think I ever found out exactly why the problem happens. But on
>> OMAP3 DSI, the fifo thresholds had to be tuned slightly, otherwise DISPC
>> would stop. dispc_ovl_compute_fifo_thresholds() does that tuning if
>> "manual_update" parameter is set on OMAP3.
> 
> I've had a look at dispc_ovl_compute_fifo_thresholds() and the patch makes 
> sense to me. If Sebastian could address the small issues I pointed out, we 
> could then merge this. Alternatively I can take care of addressing them.

It's only needed with the rest of the DSI manual update series, so I'd
rather keep it as part of that series.

 Tomi

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Wed, Dec 14, 2016 at 11:14:32AM +0200, Tomi Valkeinen wrote:
> On 14/12/16 11:10, Laurent Pinchart wrote:
> > Hi Tomi,
> > 
> > On Wednesday 14 Dec 2016 10:43:18 Tomi Valkeinen wrote:
> >> On 13/12/16 19:35, Laurent Pinchart wrote:
> >>> 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 ?

FWIW I don't know anything about this bug. I just hit it while
getting omapdrm working on n950 and ported this over from omapfb.

> >> I don't think I ever found out exactly why the problem happens. But on
> >> OMAP3 DSI, the fifo thresholds had to be tuned slightly, otherwise DISPC
> >> would stop. dispc_ovl_compute_fifo_thresholds() does that tuning if
> >> "manual_update" parameter is set on OMAP3.
> > 
> > I've had a look at dispc_ovl_compute_fifo_thresholds() and the patch makes 
> > sense to me. If Sebastian could address the small issues I pointed out, we 
> > could then merge this. Alternatively I can take care of addressing them.
> 
> It's only needed with the rest of the DSI manual update series, so I'd
> rather keep it as part of that series.

To be honest I haven't worked on this for some time. From some
comments on the patchset I think the biggest issue is, that omapdrm
does not use generic panel drivers and cannot easily use the
mipi_dsi_driver_register. I simply did not have enough time to
implement such a huge change.

I guess a first step is Peter Ujfalusi's series:
https://lkml.org/lkml/2016/9/1/267

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1542220

Hi Sebastian,

(CC'ing Peter Ujfalusi)

On Wednesday 14 Dec 2016 15:03:08 Sebastian Reichel wrote:
> On Wed, Dec 14, 2016 at 11:14:32AM +0200, Tomi Valkeinen wrote:
> > On 14/12/16 11:10, Laurent Pinchart wrote:
> >> On Wednesday 14 Dec 2016 10:43:18 Tomi Valkeinen wrote:
> >>> On 13/12/16 19:35, Laurent Pinchart wrote:
> >>>> 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 ?
> 
> FWIW I don't know anything about this bug. I just hit it while
> getting omapdrm working on n950 and ported this over from omapfb.

I wonder if it's a bug or an expected behaviour. In any case, looking at the 
FIFO thresholds computation function it's quite clear that we need to 
recompute and set the values when the panel mode changes. Maybe you should 
explain this in the commit message.

> >>> I don't think I ever found out exactly why the problem happens. But on
> >>> OMAP3 DSI, the fifo thresholds had to be tuned slightly, otherwise
> >>> DISPC would stop. dispc_ovl_compute_fifo_thresholds() does that tuning
> >>> if "manual_update" parameter is set on OMAP3.
> >> 
> >> I've had a look at dispc_ovl_compute_fifo_thresholds() and the patch
> >> makes sense to me. If Sebastian could address the small issues I pointed
> >> out, we could then merge this. Alternatively I can take care of
> >> addressing them.
> > 
> > It's only needed with the rest of the DSI manual update series, so I'd
> > rather keep it as part of that series.
> 
> To be honest I haven't worked on this for some time. From some
> comments on the patchset I think the biggest issue is, that omapdrm
> does not use generic panel drivers and cannot easily use the
> mipi_dsi_driver_register. I simply did not have enough time to
> implement such a huge change.

I'll probably give it a go at some point, but it will take time.

> I guess a first step is Peter Ujfalusi's series:
> https://lkml.org/lkml/2016/9/1/267

Peter, do you plan to respin that patch series ?

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

On 14/12/16 23:36, Laurent Pinchart wrote:

>> I guess a first step is Peter Ujfalusi's series:
>> https://lkml.org/lkml/2016/9/1/267
> 
> Peter, do you plan to respin that patch series ?

That series has been merged already.

 Tomi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web