Re: [PATCH] drm/exynos: fix kernel panic issue at drm releasing

[Daniel Vetter]

On Tue, Jan 05, 2016 at 07:55:52PM +0900, Inki Dae wrote:
> Hi Daniel,
> 
> 2016년 01월 05일 05:24에 Daniel Stone 이(가) 쓴 글:
> > Hi Inki,
> > 
> > On 4 January 2016 at 12:57, Inki Dae <inki@samsung.com> wrote:
> >> 2015년 12월 24일 22:32에 Daniel Stone 이(가) 쓴 글:
> >>> On 24 December 2015 at 09:10, Inki Dae <inki@samsung.com> wrote:
> >>>> +void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc)
> >>>> +{
> >>>> +   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> >>>> +   unsigned long flags;
> >>>> +
> >>>> +   spin_lock_irqsave(>dev->event_lock, flags);
> >>>> +   exynos_crtc->event = NULL;
> >>>> +   spin_unlock_irqrestore(>dev->event_lock, flags);
> >>>> +}
> >>>
> >>> This will leak the event and event space; you should call
> >>> event->base.destroy() here. With that fixed:
> >>
> >> Right. we don't use exynos specific page flip function anymore which 
> >> managed the event as a list so that the event objects can be freed by 
> >> postclose callback.
> >> Anyway, would it be better for event->base.destory() to be called between 
> >> spin lock/unlock?
> > 
> > You must increment event->base.file_priv->event_space (see
> > drm_atomic.c:destroy_vblank_event), as well as calling
> 
> Reasonable to me. Seems other DRM drivers don't increment event_space.
> 
> > event->base.destroy (see drm_fops.c:drm_read) underneath event_lock,
> > yes.
> 
> In addition, only event objects belonging to the request process should be 
> destroyed.

Just random comment out of the far left field, but robclark had a bunch of
patches to clean up all that event alloc/cleanup code a bit and extract it
into core functions. Might be good to ping him on irc to figure out where
that series is and whether you could take it over.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Daniel Vetter]

container_of(state, const struct exynos_drm_plane_state, base);
> + struct exynos_drm_private *dev_priv = plane->dev->dev_private;
> +
> + if (property == dev_priv->plane_zpos_property)
> + *val = exynos_state->zpos;
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
>  static struct drm_plane_funcs exynos_plane_funcs = {
>   .update_plane   = drm_atomic_helper_update_plane,
>   .disable_plane  = drm_atomic_helper_disable_plane,
>   .destroy= drm_plane_cleanup,
> + .set_property   = drm_atomic_helper_plane_set_property,
>   .reset  = exynos_drm_plane_reset,
>   .atomic_duplicate_state = exynos_drm_plane_duplicate_state,
>   .atomic_destroy_state = exynos_drm_plane_destroy_state,
> + .atomic_set_property = exynos_drm_plane_atomic_set_property,
> + .atomic_get_property = exynos_drm_plane_atomic_get_property,
>  };
>  
>  static int
> @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct 
> drm_plane *plane,
>  
>   prop = dev_priv->plane_zpos_property;
>   if (!prop) {
> - prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
> -  "zpos", 0, MAX_PLANE - 1);
> + prop = drm_property_create_range(dev, 0, "zpos",
> +  0, MAX_PLANE - 1);
>   if (!prop)
>   return;
>  
> @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev,
>   exynos_plane->index = index;
>   exynos_plane->config = config;
>  
> - if (config->type == DRM_PLANE_TYPE_OVERLAY)
> - exynos_plane_attach_zpos_property(_plane->base,
> -   config->zpos);
> + exynos_plane_attach_zpos_property(_plane->base, config->zpos);
>  
>   return 0;
>  }
> -- 
> 1.9.2
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Daniel Vetter]

On Wed, Dec 16, 2015 at 02:54:04PM +0100, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-12-16 14:28, Daniel Vetter wrote:
> >On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote:
> >>This patch adds all infrastructure to make zpos plane property
> >>configurable from userspace.
> >>
> >>Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> >Imo zpos should be a generic atomic property with well-defined semantics
> >shared across drivers. So
> >- storead (in decoded form) in drm_plane_state
> >- common functions to register/attach zpos prop to a plane, with
> >   full-blown kerneldoc explaining how it should work
> >- generic kms igt to validate that interface
> >
> >One of the big things that always comes up when we talk about zpos is how
> >equal zpos should be handled, and how exactly they should be sorted. I
> >think for that we should have a drm function which computes a normalized
> >zpos. Or the core check code should reject such nonsense.
> 
> IMHO it will be enough to state that the case of equal zpos is HW specific.
> This might simplify some driver logic. For example, in case of Exynos Mixer,
> equal priority (zpos) means that HW predefined order will be used, so there
> is no need to normalize zpos values.
> 
> If you want I can move zpos to drm core and add a function to normalize
> zpos,
> although for this particular driver normalization is not needed.
> 
> It should be quite easy to convert other drivers to use the generic zpos
> property. The only problem I see is how to handle omap driver, which use
> 'zorder' property.
> 
> What about some other typical properties related to blending:
> - global plane alpha,
> - colorkey,
> - alpha mode (standard or pre-multiplied) for alpha-enabled modes,
> - crtc background color.
> 
> Do you also want to handle them as generic or driver-specific properties?

Should all be generic really. And it's also kinda ABI, so userspace
needed, and preferrably a proper/automated testcase. i915 has
infrastructure to use display pipeline CRCs to really measure it's all
correct even, and that's the standard I'd like to see for all KMS API
extensions like this.

Since if we don't do this we'll end up in a giant mess, and it will be
impossible to write a kms compositor that's generic and uses all this.

And since this stuff is supposed to be generic, fluffy/unspecified
semantics aren't good. Especially if your hw can just somehow deal with it
all.

> I plan to add support for them to Exynos Mixer and I would like to avoid
> doing this twice.

It's a lot more work than just adding a few members to drm_plane_state.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: only run atomic_check() if crtc is active

[Daniel Vetter]

On Tue, Nov 17, 2015 at 03:19:35PM +0100, Andrzej Hajda wrote:
> Hi Daniel,
> 
> On 11/17/2015 11:06 AM, Daniel Vetter wrote:
> > On Thu, Nov 12, 2015 at 02:49:29PM +0100, Thierry Reding wrote:
> >> On Thu, Nov 12, 2015 at 11:11:20AM -0200, Gustavo Padovan wrote:
> >>> From: Gustavo Padovan <gustavo.pado...@collabora.co.uk>
> >>>
> >>> Fixes an regression added by 3ae2436 (drm/exynos/mixer: replace
> >>> direct cross-driver call with drm mode). The whole atomic update
> >>> was failing if the hdmi display is not present/active. Add a test
> >>> to only run atomic_check() if the CRTC is active.
> >>>
> >>> Signed-off-by: Gustavo Padovan <gustavo.pado...@collabora.co.uk>
> >>> ---
> >>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
> >>> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> index b3ba27f..1d3ca0a 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> @@ -55,6 +55,9 @@ static int exynos_crtc_atomic_check(struct drm_crtc 
> >>> *crtc,
> >>>  {
> >>>   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> >>>  
> >>> + if (!state->active)
> >>> + return 0;
> >>> +
> >>>   if (exynos_crtc->ops->atomic_check)
> >>>   return exynos_crtc->ops->atomic_check(exynos_crtc, state);
> >>>  
> >>
> >> This looks like something that the core should be doing.
> > 
> > Nah, this is a bug in exynos atomic_check code. Drivers _must_ check state
> > irrespective of state->active - if they forgoe any checking when active =
> > false then legacy DPMS On might spuriuosly fail, which will upset
> > userspace. There shouldn't be any exceptions to this rule.
> > 
> > Cheers, Daniel
> > 
> 
> What about the situation when we have two display pipelines
> with separate crtcs:
> - one for panel, fimd->dsi->panel,
> - one for hdmi, mixer->hdmi->TV.
> 
> Since TV is not connected mixer state have mode initially filled
> with zeros and active field set to 0. How should we handle situation
> if userspace tries to enable dpms on HDMI connector?
> How should we handle situation userspace tries to start panel pipeline?
> In this case atomic_check for mixer is called also, but since
> it will not be used and its state will not be changed it
> should not return error, am I right?

Check crtc_state->enable, skip if false. That's the "is this pipeline
configured" knob. For plane/connector it's state->crtc, but with the same
role.

I guess we could check that in the helpers, but we need to be careful to
still call ->atomic_check for the disabling transition, in case userspace
is asking for a vblank-synced flip that disables a plane but somehow
that's not possible. I somewhat prefer to handle that all in drivers
though.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: only run atomic_check() if crtc is active

[Daniel Vetter]

On Thu, Nov 12, 2015 at 02:49:29PM +0100, Thierry Reding wrote:
> On Thu, Nov 12, 2015 at 11:11:20AM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.pado...@collabora.co.uk>
> > 
> > Fixes an regression added by 3ae2436 (drm/exynos/mixer: replace
> > direct cross-driver call with drm mode). The whole atomic update
> > was failing if the hdmi display is not present/active. Add a test
> > to only run atomic_check() if the CRTC is active.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.pado...@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
> > b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > index b3ba27f..1d3ca0a 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > @@ -55,6 +55,9 @@ static int exynos_crtc_atomic_check(struct drm_crtc *crtc,
> >  {
> > struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> >  
> > +   if (!state->active)
> > +   return 0;
> > +
> > if (exynos_crtc->ops->atomic_check)
> > return exynos_crtc->ops->atomic_check(exynos_crtc, state);
> >  
> 
> This looks like something that the core should be doing.

Nah, this is a bug in exynos atomic_check code. Drivers _must_ check state
irrespective of state->active - if they forgoe any checking when active =
false then legacy DPMS On might spuriuosly fail, which will upset
userspace. There shouldn't be any exceptions to this rule.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: add cursor plane support

[Daniel Vetter]

s_plane;
> > -   enum drm_plane_type type;
> > unsigned int zpos;
> > int ret;
> >  
> > @@ -702,16 +700,14 @@ static int decon_bind(struct device *dev, struct 
> > device *master, void *data)
> > }
> >  
> > for (zpos = 0; zpos < WINDOWS_NR; zpos++) {
> > -   type = (zpos == ctx->default_win) ? DRM_PLANE_TYPE_PRIMARY :
> > -   DRM_PLANE_TYPE_OVERLAY;
> > ret = exynos_plane_init(drm_dev, >planes[zpos],
> > -   1 << ctx->pipe, type, decon_formats,
> > +   1 << ctx->pipe, decon_formats,
> > ARRAY_SIZE(decon_formats), zpos);
> > if (ret)
> > return ret;
> > }
> >  
> > -   exynos_plane = >planes[ctx->default_win];
> > +   exynos_plane = >planes[DEFAULT_WIN];
> > ctx->crtc = exynos_drm_crtc_create(drm_dev, _plane->base,
> >ctx->pipe, EXYNOS_DISPLAY_TYPE_LCD,
> >_crtc_ops, ctx);
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h 
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > index b7ba21d..cc56c3d 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > @@ -22,6 +22,9 @@
> >  #define MAX_PLANE  5
> >  #define MAX_FB_BUFFER  4
> >  
> > +#define DEFAULT_WIN0
> > +#define CURSOR_WIN 1
> 
> You fixed overlay number for cursor with 1. However, Display controllers
> of Exynos SoC have fixed overlay priority like this,
> 
> win 4 > win 3 > win 2 > win 1 > win 0 so if we fix the overlay number
> for cursor and we use another overlay - which has a higher layer than
> cursor - to display caption or UI then the we couldn't see the cursor on
> screen without additional work such as color key or alpha channel.
> 
> So before that, you need to check how many overlays can be used
> according to Exynos SoC versions, and which way is better - one is for
> top layer to be fixed for cursor, other is for one given layer to be
> fixed by user-space for cursor.

I think cursor should by default be the top layer (if the hw can do it).
Otherwise it will be really confusing to compositors I fear.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] drm/exynos: remove legacy -suspend()/resume()

[Daniel Vetter]

On Wed, Aug 26, 2015 at 12:50:44PM -0300, Gustavo Padovan wrote:
 Hi,
 
 What about this patch? We need it to avoid the WARN_ON added by patch
 2/2 that was already picked up by Daniel.

That patch is only for 4.4, so not too time critical to get the exynos one
in. But might be good to get it into 4.3.
-Daniel

 
   Gustavo
 
 2015-08-13 Gustavo Padovan gust...@padovan.org:
 
  From: Gustavo Padovan gustavo.pado...@collabora.co.uk
  
  These legacy helpers should only be used by shadow-attaching drivers.
  KMS drivers has its own way to handle suspend/resume and don't need to
  use these two helpers.
  
  Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
  ---
   drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 --
   1 file changed, 2 deletions(-)
  
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
  b/drivers/gpu/drm/exynos/exynos_drm_drv.c
  index f1d6966..9bcf679 100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
  @@ -280,8 +280,6 @@ static struct drm_driver exynos_drm_driver = {
  .driver_features= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
  .load   = exynos_drm_load,
  .unload = exynos_drm_unload,
  -   .suspend= exynos_drm_suspend,
  -   .resume = exynos_drm_resume,
  .open   = exynos_drm_open,
  .preclose   = exynos_drm_preclose,
  .lastclose  = exynos_drm_lastclose,
  -- 
  2.1.0
  

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] drm: WARN_ON if a modeset driver uses legacy suspend/resume helpers

[Daniel Vetter]

On Thu, Aug 13, 2015 at 05:06:39PM -0300, Gustavo Padovan wrote:
 From: Gustavo Padovan gustavo.pado...@collabora.co.uk
 
 Legacy s/r hooks are only used for shadow-attaching drivers, warn
 when a KMS driver tries to use them.
 
 Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk

Assuming that Inki picks up the exynos patch I've applied this to
drm-misc.

Thanks, Daniel

 ---
  drivers/gpu/drm/drm_drv.c | 2 ++
  1 file changed, 2 insertions(+)
 
 diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
 index b7bf4ce..4e76193 100644
 --- a/drivers/gpu/drm/drm_drv.c
 +++ b/drivers/gpu/drm/drm_drv.c
 @@ -567,6 +567,8 @@ struct drm_device *drm_dev_alloc(struct drm_driver 
 *driver,
   ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL);
   if (ret)
   goto err_minors;
 +
 + WARN_ON(driver-suspend || driver-resume);
   }
  
   if (drm_core_check_feature(dev, DRIVER_RENDER)) {
 -- 
 2.1.0
 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/11] drm/exynos: remove struct exynos_drm_encoder layer

[Daniel Vetter]

On Tue, Aug 11, 2015 at 09:13:54PM +0900, Inki Dae wrote:
 On 2015년 08월 11일 09:38, Gustavo Padovan wrote:
  Hi Inki,
  
  2015-08-07 Inki Dae inki@samsung.com:
  
  Hi Gustavo,
 
  On 2015년 08월 06일 22:31, Gustavo Padovan wrote:
  From: Gustavo Padovan gustavo.pado...@collabora.co.uk
 
  struct exynos_drm_encoder was justing wrapping struct drm_encoder, it had
  only a drm_encoder member and the internal exynos_drm_encoders ops that
  was directly mapped to the drm_encoder helper funcs.
 
  So now exynos DRM uses struct drm_encoder directly, this removes
  completely the struct exynos_drm_encoder.
 
 
  Trats2 board, which uses Exynos4412 Soc, doesn't work after this patch
  is applied. Below is the booting logs,
  [1.171318] console [ttySAC2] enabled
  [1.175522] 1383.serial: ttySAC3 at MMIO 0x1383 (irq = 60,
  base_baud = 0) is a S3C6400/10
  [1.185545] [drm] Initialized drm 1.1.0 20060810
  [1.194104] exynos-drm exynos-drm: bound 11c0.fimd (ops
  fimd_component_ops)
  [1.200352] exynos-drm exynos-drm: bound 11c8.dsi (ops
  exynos_dsi_component_ops)
  [1.207688] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
  [1.214313] [drm] No driver support for vblank timestamp query.
  [1.220218] [drm] Initialized exynos 1.0.0 20110530 on minor 0
 
  Booting is locked up here. This patch looks good to me so I tried to
  find why locked up and I found the booting is locked up as soon as
  console_lock function is called. Can you and other guys look into this
  issue?
  
  I've realized that I left a fix for patch 01 behind, it could be the
  cause of this issue. I've just resent this patch with the added v2 fix
  up.
 
 With above change, still locked up. So your updated patch doesn't
 resolve this issue.
 
 Anyway, I tested it with fbdev emulation relevant patch series[1] and
 the booting was ok with disabling fbdev emulation as Daniel commented.
 However, I think the booting should also be ok with fbdev emulation so I
 don't want for your last patch to be merged to mainline until the issue
 is resolved.

Without fbdev you need to start a kms client (X, whatever) to force a
modeset. Otherwise you won't reproduce anything. And sometimes it requires
a bit of trickery to create a sequence of modeset calls which exactly
match what fbcon would do.
-Daniel

 
 [1] http://www.spinics.net/lists/dri-devel/msg86617.html
 http://lists.freedesktop.org/archives/dri-devel/2015-March/078975.html
 
 Thanks,
 Inki Dae
 
  
  Gustavo
  --
  To unsubscribe from this list: send the line unsubscribe 
  linux-samsung-soc in
  the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
 
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/8] drm: exynos/dp: convert to drm bridge mode

[Daniel Vetter]

On Thu, Aug 06, 2015 at 10:29:29PM +0800, Yakir Yang wrote:
 Hi Jingoo,
 
 在 2015/8/6 22:19, Jingoo Han 写道:
 On Thursday, August 06, 2015 11:07 PM, Yakir Yang wrote:
 In order to move exynos dp code to bridge directory,
 we need to convert driver drm bridge mode first. As
 dp driver already have a ptn3460 bridge, so we need
 to move ptn bridge to the next bridge of dp bridge.
 
 Signed-off-by: Yakir Yang y...@rock-chips.com
 ---
   drivers/gpu/drm/exynos/exynos_dp_core.c | 196 
  
   drivers/gpu/drm/exynos/exynos_dp_core.h |   2 +
   2 files changed, 126 insertions(+), 72 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c 
 b/drivers/gpu/drm/exynos/exynos_dp_core.c
 index a8097a4..aa99e23 100644
 --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
 +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
 @@ -3,6 +3,7 @@
*
* Copyright (C) 2012 Samsung Electronics Co., Ltd.
* Author: Jingoo Han jg1@samsung.com
 + * Yakir Yang y...@rock-chips.com
 Please don't add this.
 You just fixed some parts of this code. I don't find the reason
 why you have to be added to author for this file.
 If you want the title for an author, please send the patch
 for a new IP, instead of modifying the exiting codes.
 
 Okay, thanks for your remind ;)

tbh the author lines are completely irrelevant, git blame/log is the
authoritative source for that. Copyright is a bit a different matter
entirely, but in general we seem to be not too good at updating.

Really if that's all you have to comment and no substantial technical
concerns or questions then just can such a bikeshed mail.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/11] drm/exynos: remove struct exynos_drm_encoder layer

[Daniel Vetter]

On Fri, Aug 7, 2015 at 1:50 PM, Inki Dae inki@samsung.com wrote:

 Booting is locked up here. This patch looks good to me so I tried to
 find why locked up and I found the booting is locked up as soon as
 console_lock function is called. Can you and other guys look into this
 issue?

It's not locked up, you simply won't see any dmesg output after that.
Use Archit's Kconfig support in drm-misc to disable fbdev emulation
and try again.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 12/23] drm/exynos: don't track enabled state at exynos_crtc

[Daniel Vetter]

On Mon, Jul 06, 2015 at 11:20:13AM -0300, Gustavo Padovan wrote:
 From: Gustavo Padovan gustavo.pado...@collabora.co.uk
 
 struct drm_crtc already stores the enabled state of the crtc
 thus we don't need to replicate enabled in exynos_drm_crtc.
 
 Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk

Note that exynos_crtc-enabled doesn't match drm_crtc-enabled perfectly
since the exynos one reflect hw state (including dpms). You want to look
at crtc-state-active instead on functions enabling stuff, and
old_crtc_state-active on functions disabling stuff (we don't wire that
through everywhere yet).
-Daniel

 ---
  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 16 
  drivers/gpu/drm/exynos/exynos_drm_drv.h  |  1 -
  2 files changed, 17 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
 b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
 index 9bc2353..5ab8972 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
 @@ -26,14 +26,9 @@ static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
  {
   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
  
 - if (exynos_crtc-enabled)
 - return;
 -
   if (exynos_crtc-ops-enable)
   exynos_crtc-ops-enable(exynos_crtc);
  
 - exynos_crtc-enabled = true;
 -
   drm_crtc_vblank_on(crtc);
  }
  
 @@ -41,9 +36,6 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
  {
   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
  
 - if (!exynos_crtc-enabled)
 - return;
 -
   /* wait for the completion of page flip. */
   if (!wait_event_timeout(exynos_crtc-pending_flip_queue,
   (exynos_crtc-event == NULL), HZ/20))
 @@ -53,8 +45,6 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
  
   if (exynos_crtc-ops-disable)
   exynos_crtc-ops-disable(exynos_crtc);
 -
 - exynos_crtc-enabled = false;
  }
  
  static bool
 @@ -171,9 +161,6 @@ int exynos_drm_crtc_enable_vblank(struct drm_device *dev, 
 int pipe)
   struct exynos_drm_crtc *exynos_crtc =
   to_exynos_crtc(private-crtc[pipe]);
  
 - if (!exynos_crtc-enabled)
 - return -EPERM;
 -
   if (exynos_crtc-ops-enable_vblank)
   return exynos_crtc-ops-enable_vblank(exynos_crtc);
  
 @@ -186,9 +173,6 @@ void exynos_drm_crtc_disable_vblank(struct drm_device 
 *dev, int pipe)
   struct exynos_drm_crtc *exynos_crtc =
   to_exynos_crtc(private-crtc[pipe]);
  
 - if (!exynos_crtc-enabled)
 - return;
 -
   if (exynos_crtc-ops-disable_vblank)
   exynos_crtc-ops-disable_vblank(exynos_crtc);
  }
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h 
 b/drivers/gpu/drm/exynos/exynos_drm_drv.h
 index 5bd1d3c..d3a8f0a 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
 +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
 @@ -185,7 +185,6 @@ struct exynos_drm_crtc {
   struct drm_crtc base;
   enum exynos_drm_output_type type;
   unsigned intpipe;
 - boolenabled;
   wait_queue_head_t   pending_flip_queue;
   struct drm_pending_vblank_event *event;
   const struct exynos_drm_crtc_ops*ops;
 -- 
 2.1.0
 
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] tests/exynos: introduce wait_for_user_input

[Daniel Vetter]

On Mon, Feb 23, 2015 at 11:22:09AM +, Emil Velikov wrote:
 On 16/02/15 13:46, Tobias Jakobi wrote:
  Currently getchar() is used to pause execution after each test.
  The user isn't informed if one is supposed to do anything for
  the tests to continue, so print a simple message to make this
  more clear.
  
  Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
  ---
   tests/exynos/exynos_fimg2d_test.c | 20 
   1 file changed, 16 insertions(+), 4 deletions(-)
  
  diff --git a/tests/exynos/exynos_fimg2d_test.c 
  b/tests/exynos/exynos_fimg2d_test.c
  index 55d2970..446a6c6 100644
  --- a/tests/exynos/exynos_fimg2d_test.c
  +++ b/tests/exynos/exynos_fimg2d_test.c
  @@ -237,6 +237,18 @@ void *create_checkerboard_pattern(unsigned int 
  num_tiles_x,
  return buf;
   }
   
  +static void wait_for_user_input(int last)
  +{
  +   printf(press ENTER to );
  +
  +   if (last)
  +   printf(exit test application\n);
  +   else
  +   printf(skip to next test\n);
  +
 If interested you can compact this as
 
   printf(press ENTER to %s\n, last ? exit test application :
  skip to next test);

We have this and a ton of other neat helpers in igt. As I've probably said
countless times on irc and at conferences if someone bothers to make the
core of igt i915-agnostic (just needs changes in the function to open the
drm dev mostly) I'd love to see igt converted into a generic drm
testsuite.

And similar to piglit I think it makes more sense to have that outside of
any userspace components we ship to users, i.e. not in libdrm.

But I really can't justify doing this work to my dear employer ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/14] drm/exynos: remove struct *_win_data abstraction on planes

[Daniel Vetter]

On Thu, Feb 05, 2015 at 12:48:07PM +, Daniel Stone wrote:
 Hi,
 
 On 5 February 2015 at 12:26, Rob Clark robdcl...@gmail.com wrote:
  On Thu, Feb 5, 2015 at 4:15 AM, Daniel Vetter dan...@ffwll.ch wrote:
  Yeah I noticed the zpos fun when hacking around too. Exynos should
  probably switch defaults so that overlays are visible by default. And we
  need to standardize the zpos property so that other drivers can use it
  too.
 
  jfyi, I have a bit of logic in mdp5_crtc_atomic_check() (and really
  mdp4 probably needs the same) to sort attached planes and derive the
  actual hw zpos (with each layer having a unique zpos)..
 
  I suspect most hw will end up needing the same (ie. dislike having two
  overlays at same zpos), so might not be a horrible idea to make the
  atomic helpers do this sorting for us..

Oh yeah such a helper would be nice. Especially since on intel hw flipping
planes around means fishing the right value out of some enum table ;-)
So some sort of helper to compute the absolute ordering in a stable way
would be nice.

 Same with Exynos, except it's a bit funnier still. In terms of its
 hardware, each CRTC has a number of planes which have a fixed zpos.
 The reason exynos_drm exposes zpos is because it sets up a fixed
 number of planes for the entire device and declares they can run
 across every single CRTC, and then works out from the combination of
 CRTC assignment and zpos property, which hardware plane to assign it
 to. This includes the primary, so if you accidentally get zpos==0 in
 there, then you smash the primary plane. Or set a duplicate zpos and
 then the last one in wins.
 
 It also means if you're using multiple CRTCs (e.g. FIMD for internal
 panel plus mixer for external HDMI), then you can only use 5 planes in
 total, rather than 5 planes per head. (And let's not forget that each
 backend has disjoint format support, so again the driver just lies to
 you and claims to support them all, with a silent fallback via a
 default case statement when the format isn't actually supported.
 Whoops.)
 
 I think a more ideal setup would be a much more direct 1:1 mapping
 with the hardware, where each actual plane on each actual CRTC gets
 exposed directly to userspace, perhaps with a fixed/read-only zpos
 property to tell the userspace which plane it's actually configuring.
 I suspect this would be a pretty good map to other hardware as well.

Hm that sounds indeed a bit funny. I agree that exynos should split planes
into per-crtc and separate the code and capability tables out so that
there's less lying. And zpos should probably be converted to a read-only
property to tell userspace about the facts, instead of doing something
funny behind the scenes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/14] drm/exynos: Remove exynos_plane_dpms() call with no effect

[Daniel Vetter]

On Thu, Feb 05, 2015 at 10:05:43AM +0900, Joonyoung Shim wrote:
 Hi Daniel,
 
 On 02/04/2015 11:16 PM, Daniel Vetter wrote:
  On Wed, Feb 04, 2015 at 04:42:57PM +0900, Joonyoung Shim wrote:
  Hi,
 
  On 02/04/2015 04:14 AM, Gustavo Padovan wrote:
  From: Gustavo Padovan gustavo.pado...@collabora.co.uk
 
  exynos_plane_dpms(DRM_MODE_DPMS_ON) calls the win_enable()'s callback
  from the underlying layer. However neither one of these layers implement
  win_enable() - FIMD, Mixer and VIDI. Thus the call to exynos_plane_dpms()
  is pointless.
 
  Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
  ---
   drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 --
   1 file changed, 2 deletions(-)
 
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
  b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  index b2a4b84..ad675fb 100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  @@ -65,8 +65,6 @@ static void exynos_drm_crtc_commit(struct drm_crtc 
  *crtc)
   
if (exynos_crtc-ops-commit)
exynos_crtc-ops-commit(exynos_crtc);
  -
  - exynos_plane_dpms(crtc-primary, DRM_MODE_DPMS_ON);
 
  As i said, this needs to keep pair enabled flag of struct
  exynos_drm_plane.
  
  The reason exynos needs that exynos_plane-enable is because it has its
  own per-plane dpms state. There's two problems with that:
  - It's highyl non-standard, the drm kms way is to just disable the plane
and not have some additional knob on top.
  - The atomic helpers will not be able to handle this. They assume that
there's only one dpms knob for the entire display pipeline, and
per-plane enable/disable is handled by setting plane-state-crtc, which
must be set iff plane-state-fb is set right now.
  
  I recommend we rip this all out if we can adjust existing userspace to
  stop using the mode property on planes and crtcs.
  
  And with that non-standard exynos plane mode thing gone we can indeed rip
  out exynos_plane_dpms and exynos_plane-enabled and just directly call
  manager-ops-win_enable/disble. And then rip out the win_enable since
  it's unused.
 
 But this cleanup on current codes doesn't care now current operation
 normally. First let's cleanup non-standard exynos plane dpms state as
 you said.

Yeah my reply wasn't too clear, so let me clarify: I agree with you,
Padovan's patch as-is can't be merged. First we need to get rid of the
non-standard plane dpms stuff, then we can remove the -win_enable hook
and then can we remove this call here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/14] drm/exynos: remove struct *_win_data abstraction on planes

[Daniel Vetter]

On Thu, Feb 05, 2015 at 11:37:13AM +0900, Joonyoung Shim wrote:
 Hi Daniel,
 
 On 02/04/2015 11:28 PM, Daniel Vetter wrote:
  On Wed, Feb 04, 2015 at 04:44:12PM +0900, Joonyoung Shim wrote:
  Hi,
 
  On 02/04/2015 04:14 AM, Gustavo Padovan wrote:
  From: Gustavo Padovan gustavo.pado...@collabora.co.uk
 
  struct {fimd,mixer,vidi}_win_data was just keeping the same data
  as struct exynos_drm_plane thus get ride of it and use exynos_drm_plane
  directly.
 
  It changes how planes are created and remove .win_mode_set() callback
  that was only filling all *_win_data structs.
 
 
  I commented already on prior patch.
  
  I think you don't quite understand how this primary/overlay plane stuff
  works in drm core. The entire point of the drm core primary plane is to
  work _exactly_ like an overlay plane and allow userspace to mangle the
  primary plane configuration through the overlay plane. The only reason we
  have primary planes is so that old userspace keeps working.
  
 
 Right, i misunderstood a bit because exynos hw drivers have dependency
 of zpos(hw overlay position).
 
 Current exynos drm driver has each primary plane of hw drivers and five
 overlay planes. The primary plane is fixed on default hw overlay and all
 overlay plane can map to all hw overlays using specific zpos property of
 exynos drm plane.
 
 Gustavo approach will include specific hw overlay data in overlay plane
 and hw driver keeps overlay planes to array by zpos order. But current
 zpos of overlay plane is 0 always if user doesn't modify it, so hw
 driver will use only hw overlay data of primary plane always even if
 user want to use overlay plane.
 
 If user is modified zpos of overlay plane, hw driver can get wrong hw
 overlay data from different overlay plane because hw driver keeps
 overlay planes by zpos order.

Yeah I noticed the zpos fun when hacking around too. Exynos should
probably switch defaults so that overlays are visible by default. And we
need to standardize the zpos property so that other drivers can use it
too.

But that doesn't change anything with the primary plane just being a
special plane from the sw side (backwards compat), for exynos hw they all
look the same.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/14] drm/exynos: atomic phase 1: add atomic_begin()/atomic_flush()

[Daniel Vetter]

On Thu, Feb 05, 2015 at 11:48:18AM +0900, Joonyoung Shim wrote:
 Hi Daniel,
 
 On 02/04/2015 11:30 PM, Daniel Vetter wrote:
  On Wed, Feb 04, 2015 at 04:49:25PM +0900, Joonyoung Shim wrote:
  Hi,
 
  On 02/04/2015 04:14 AM, Gustavo Padovan wrote:
  From: Gustavo Padovan gustavo.pado...@collabora.co.uk
 
  Add CRTC callbacks .atomic_begin() .atomic_flush(). On exynos they
  unprotect the windows before the commit and protects it after based on
  a plane mask tha store which plane will be updated.
 
 
  I don't think they need now.
  
  This does exactly what I wanted to do in my atomic poc but couldn't
  because of the massive layer hell that was still around in atomic. Haven't
  looked into the patch in details, so no full r-b but good enough for an
  
 
 I agree about its operation but i think it is unnecessary now. Because
 it's exactly same operation with current codes.

Well that's to be expected since if you don't want to have some duplicated
code while transitioning to atomic you must do it all in one giant patch.
With a bit of duplication you can move things over slowly, in differnt
phases and piece by piece like Padovan's patch series here. The
duplication should go away in the end again.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/14] drm/exynos: Remove exynos_plane_dpms() call with no effect

[Daniel Vetter]

On Wed, Feb 04, 2015 at 04:42:57PM +0900, Joonyoung Shim wrote:
 Hi,
 
 On 02/04/2015 04:14 AM, Gustavo Padovan wrote:
  From: Gustavo Padovan gustavo.pado...@collabora.co.uk
  
  exynos_plane_dpms(DRM_MODE_DPMS_ON) calls the win_enable()'s callback
  from the underlying layer. However neither one of these layers implement
  win_enable() - FIMD, Mixer and VIDI. Thus the call to exynos_plane_dpms()
  is pointless.
  
  Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
  ---
   drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 --
   1 file changed, 2 deletions(-)
  
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
  b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  index b2a4b84..ad675fb 100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  @@ -65,8 +65,6 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
   
  if (exynos_crtc-ops-commit)
  exynos_crtc-ops-commit(exynos_crtc);
  -
  -   exynos_plane_dpms(crtc-primary, DRM_MODE_DPMS_ON);
 
 As i said, this needs to keep pair enabled flag of struct
 exynos_drm_plane.

The reason exynos needs that exynos_plane-enable is because it has its
own per-plane dpms state. There's two problems with that:
- It's highyl non-standard, the drm kms way is to just disable the plane
  and not have some additional knob on top.
- The atomic helpers will not be able to handle this. They assume that
  there's only one dpms knob for the entire display pipeline, and
  per-plane enable/disable is handled by setting plane-state-crtc, which
  must be set iff plane-state-fb is set right now.

I recommend we rip this all out if we can adjust existing userspace to
stop using the mode property on planes and crtcs.

And with that non-standard exynos plane mode thing gone we can indeed rip
out exynos_plane_dpms and exynos_plane-enabled and just directly call
manager-ops-win_enable/disble. And then rip out the win_enable since
it's unused.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/14] drm/exynos: remove struct *_win_data abstraction on planes

[Daniel Vetter]

 void mixer_win_disable(struct exynos_drm_crtc 
  *crtc, int zpos)
  mutex_lock(mixer_ctx-mixer_mutex);
  if (!mixer_ctx-powered) {
  mutex_unlock(mixer_ctx-mixer_mutex);
  -   mixer_ctx-win_data[win].resume = false;
  +   mixer_ctx-planes[win].resume = false;
  return;
  }
  mutex_unlock(mixer_ctx-mixer_mutex);
  @@ -1012,7 +939,7 @@ static void mixer_win_disable(struct exynos_drm_crtc 
  *crtc, int zpos)
  mixer_vsync_set_update(mixer_ctx, true);
  spin_unlock_irqrestore(res-reg_slock, flags);
   
  -   mixer_ctx-win_data[win].enabled = false;
  +   mixer_ctx-planes[win].enabled = false;
   }
   
   static void mixer_wait_for_vblank(struct exynos_drm_crtc *crtc)
  @@ -1049,12 +976,12 @@ static void mixer_wait_for_vblank(struct 
  exynos_drm_crtc *crtc)
   
   static void mixer_window_suspend(struct mixer_context *ctx)
   {
  -   struct hdmi_win_data *win_data;
  +   struct exynos_drm_plane *plane;
  int i;
   
  for (i = 0; i  MIXER_WIN_NR; i++) {
  -   win_data = ctx-win_data[i];
  -   win_data-resume = win_data-enabled;
  +   plane = ctx-planes[i];
  +   plane-resume = plane-enabled;
  mixer_win_disable(ctx-crtc, i);
  }
  mixer_wait_for_vblank(ctx-crtc);
  @@ -1062,14 +989,14 @@ static void mixer_window_suspend(struct 
  mixer_context *ctx)
   
   static void mixer_window_resume(struct mixer_context *ctx)
   {
  -   struct hdmi_win_data *win_data;
  +   struct exynos_drm_plane *plane;
  int i;
   
  for (i = 0; i  MIXER_WIN_NR; i++) {
  -   win_data = ctx-win_data[i];
  -   win_data-enabled = win_data-resume;
  -   win_data-resume = false;
  -   if (win_data-enabled)
  +   plane = ctx-planes[i];
  +   plane-enabled = plane-resume;
  +   plane-resume = false;
  +   if (plane-enabled)
  mixer_win_commit(ctx-crtc, i);
  }
   }
  @@ -1179,7 +1106,6 @@ static struct exynos_drm_crtc_ops mixer_crtc_ops = {
  .enable_vblank  = mixer_enable_vblank,
  .disable_vblank = mixer_disable_vblank,
  .wait_for_vblank= mixer_wait_for_vblank,
  -   .win_mode_set   = mixer_win_mode_set,
  .win_commit = mixer_win_commit,
  .win_disable= mixer_win_disable,
   };
  @@ -1243,15 +1169,25 @@ static int mixer_bind(struct device *dev, struct 
  device *manager, void *data)
   {
  struct mixer_context *ctx = dev_get_drvdata(dev);
  struct drm_device *drm_dev = data;
  -   int ret;
  +   struct exynos_drm_plane *exynos_plane;
  +   enum drm_plane_type type;
  +   int i, ret;
  +
  +   for (i = 0; i  MIXER_WIN_NR; i++) {
  +   type = (i == MIXER_DEFAULT_WIN) ? DRM_PLANE_TYPE_PRIMARY :
  +   DRM_PLANE_TYPE_OVERLAY;
  +   exynos_plane_init(drm_dev, ctx-planes[i], 1  ctx-pipe,
  + type);
  +   }
   
  ret = mixer_initialize(ctx, drm_dev);
  if (ret)
  return ret;
   
  -   ctx-crtc = exynos_drm_crtc_create(drm_dev, ctx-pipe,
  -EXYNOS_DISPLAY_TYPE_HDMI,
  -mixer_crtc_ops, ctx);
  +   exynos_plane = ctx-planes[MIXER_DEFAULT_WIN];
  +   ctx-crtc = exynos_drm_crtc_create(drm_dev, exynos_plane-base,
  +  ctx-pipe, EXYNOS_DISPLAY_TYPE_HDMI,
  +  mixer_crtc_ops, ctx);
  if (IS_ERR(ctx-crtc)) {
  mixer_ctx_remove(ctx);
  ret = PTR_ERR(ctx-crtc);
  
 
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/14] drm/exynos: atomic phase 2: keep track of framebuffer pointer

[Daniel Vetter]

On Wed, Feb 04, 2015 at 04:53:12PM +0900, Joonyoung Shim wrote:
 Hi,
 
 On 02/04/2015 04:14 AM, Gustavo Padovan wrote:
  From: Gustavo Padovan gustavo.pado...@collabora.co.uk
  
  Use drm_atomic_set_fb_for_plane() in the legacy page_flip path to keep
  track of the framebuffer pointer and reference.
  
  Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
  ---
   drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++
   1 file changed, 3 insertions(+)
  
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
  b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  index 660ad64..2edc73c 100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  @@ -211,6 +211,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc 
  *crtc,
  crtc_w, crtc_h, crtc-x, crtc-y,
  crtc_w, crtc_h);
   
  +   if (crtc-primary-state)
  +   drm_atomic_set_fb_for_plane(crtc-primary-state, fb);
  +
 
 I'm not sure whether this needs, how about go to
 drm_atomic_helper_page_flip?

You can only do that once you have full async atomic commit support, which
is done in phase 3. Until that's the case you need to keep this little bit
of temporary fixup code around.

I've had the same in my exynos branch, we have the same now in i915 (well
it looks a bit different since we dont use all the helpers but roll some
things ourselves).

Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch

 
 Thanks.
 
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/14] drm/exynos: atomic phase 1: add atomic_begin()/atomic_flush()

[Daniel Vetter]

On Wed, Feb 04, 2015 at 04:49:25PM +0900, Joonyoung Shim wrote:
 Hi,
 
 On 02/04/2015 04:14 AM, Gustavo Padovan wrote:
  From: Gustavo Padovan gustavo.pado...@collabora.co.uk
  
  Add CRTC callbacks .atomic_begin() .atomic_flush(). On exynos they
  unprotect the windows before the commit and protects it after based on
  a plane mask tha store which plane will be updated.
  
 
 I don't think they need now.

This does exactly what I wanted to do in my atomic poc but couldn't
because of the massive layer hell that was still around in atomic. Haven't
looked into the patch in details, so no full r-b but good enough for an

Acked-by: Daniel Vetter daniel.vet...@ffwll.ch

Aside: If you think something doesn't need to be done please explain what
you mean or at least ask about what you don't understand in a patch. As-is
your review is pretty much unactionable.

Cheers, Daniel

 
 Thanks.
 
  For that we create two new exynos_crtc callbacks: .win_protect() and
  .win_unprotect(). The only driver that implement those now is FIMD.
  
  Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
  ---
   drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 34 ++
   drivers/gpu/drm/exynos/exynos_drm_drv.h   |  4 +++
   drivers/gpu/drm/exynos/exynos_drm_fimd.c  | 57 
  ++-
   drivers/gpu/drm/exynos/exynos_drm_plane.c |  4 +++
   4 files changed, 82 insertions(+), 17 deletions(-)
  
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
  b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  index 09d4780..be36cca 100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  @@ -147,6 +147,38 @@ static void exynos_drm_crtc_disable(struct drm_crtc 
  *crtc)
  }
   }
   
  +static void exynos_crtc_atomic_begin(struct drm_crtc *crtc)
  +{
  +   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
  +   struct drm_plane *plane;
  +   int index = 0;
  +
  +   list_for_each_entry(plane, crtc-dev-mode_config.plane_list, head) {
  +   if (exynos_crtc-ops-win_protect 
  +   exynos_crtc-plane_mask  (0x01  index))
  +   exynos_crtc-ops-win_protect(exynos_crtc, index);
  +
  +   index++;
  +   }
  +}
  +
  +static void exynos_crtc_atomic_flush(struct drm_crtc *crtc)
  +{
  +   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
  +   struct drm_plane *plane;
  +   int index = 0;
  +
  +   list_for_each_entry(plane, crtc-dev-mode_config.plane_list, head) {
  +   if (exynos_crtc-ops-win_unprotect 
  +   exynos_crtc-plane_mask  (0x01  index))
  +   exynos_crtc-ops-win_unprotect(exynos_crtc, index);
  +
  +   index++;
  +   }
  +
  +   exynos_crtc-plane_mask = 0;
  +}
  +
   static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
  .dpms   = exynos_drm_crtc_dpms,
  .prepare= exynos_drm_crtc_prepare,
  @@ -155,6 +187,8 @@ static struct drm_crtc_helper_funcs 
  exynos_crtc_helper_funcs = {
  .mode_set   = exynos_drm_crtc_mode_set,
  .mode_set_base  = exynos_drm_crtc_mode_set_base,
  .disable= exynos_drm_crtc_disable,
  +   .atomic_begin   = exynos_crtc_atomic_begin,
  +   .atomic_flush   = exynos_crtc_atomic_flush,
   };
   
   static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h 
  b/drivers/gpu/drm/exynos/exynos_drm_drv.h
  index cad54e7..43efd9f 100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
  +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
  @@ -194,6 +194,8 @@ struct exynos_drm_crtc_ops {
  void (*win_enable)(struct exynos_drm_crtc *crtc, int zpos);
  void (*win_disable)(struct exynos_drm_crtc *crtc, int zpos);
  void (*te_handler)(struct exynos_drm_crtc *crtc);
  +   void (*win_protect)(struct exynos_drm_crtc *crtc, int zpos);
  +   void (*win_unprotect)(struct exynos_drm_crtc *crtc, int zpos);
   };
   
   enum exynos_crtc_mode {
  @@ -214,6 +216,7 @@ enum exynos_crtc_mode {
* we can refer to the crtc to current hardware interrupt occurred through
* this pipe value.
* @dpms: store the crtc dpms value
  + * @plane_mask: store planes to be updated on atomic modesetting
* @mode: store the crtc mode value
* @event: vblank event that is currently queued for flip
* @ops: pointer to callbacks for exynos drm specific functionality
  @@ -224,6 +227,7 @@ struct exynos_drm_crtc {
  enum exynos_drm_output_type type;
  unsigned intpipe;
  unsigned intdpms;
  +   unsigned intplane_mask;
  enum exynos_crtc_mode   mode;
  wait_queue_head_t   pending_flip_queue;
  struct drm_pending_vblank_event *event;
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
  b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  index ebb4cdc..f498d86 100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  +++ b/drivers/gpu

Re: [PATCH 00/14] drm/exynos: cleanups + atomic phases 1 and 2

[Daniel Vetter]

Hi all,

I've gone through some of the contentions point in this patch review. With
my community guy hat on I really want to make drm atomic a success, exynos
atomic is important for that.

On Wed, Feb 04, 2015 at 04:37:04PM +0900, Joonyoung Shim wrote:
 Hi,
 
 On 02/04/2015 04:14 AM, Gustavo Padovan wrote:
  From: Gustavo Padovan gustavo.pado...@collabora.co.uk
  
  Hi,
  
  This series clean ups a few more paths from exynos-drm with the most 
  important
  being the removal of the global page flip queue and the removal in driver
  internal data (struct *_win_data) that was replicating plane data.
  
  Following these patches comes the first step torwards atomic modesetting
  support on exynos.
  
 
 It's better to split cleanup and atomic support, not one patchset.

Imo the cleanups make perfect sense as prep work for atomic. They're
definitely needed afaict from what I've seen reading exynos code and these
patches here before we can implement atomic.

Personally I'd have delayered even more aggressively before going into the
atomic stuff. But in the end I guess Padovan wants to be able to ship
exynos atomic preferrably sooner than later.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -v2] drm/exynos: track vblank events on a per crtc basis

[Daniel Vetter]

On Fri, Jan 30, 2015 at 03:57:53PM +, Daniel Stone wrote:
 Hi,
 
 On 30 January 2015 at 14:30, Gustavo Padovan gust...@padovan.org wrote:
  2015-01-30 Joonyoung Shim jy0922.s...@samsung.com:
  We will lose unfinished prior events by this change. That's why we use
  linked list.
 
  I think you are right, but I was using exynos_crtc-event to do exactly the
  same as exynos_crtc-pending_flip. So we were losing a event in
  exynos_drm_crtc_dpms() before too. I change this patch to have a page_flip
  list on the crtc.
 
 The usual approach in other drivers is to return -EBUSY when there is
 already an async pageflip pending. This definitely makes sense to me,
 as I don't see the point of submitting pageflips faster than the
 hardware can actually render, and pretending to the application that
 they were actually shown.

Yes, right now drm doesn't really support anything like a pageflip queue.
Same for atomic really. Even the async pageflip mode works like it, it
just ends up flipping faster.

Long-term we want a flip queue where subsequent flips can be folded
together on the next vblank. That makes benchmark-mode games happy,
without resulting in tearing like async flips and still resulting in the
lowest possible latency (since the kernel we just commit the flips for
which all the buffers are ready and not stall).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] drm/exynos: track vblank events on a per crtc basis

[Daniel Vetter]

On Tue, Jan 27, 2015 at 12:59:15PM +, Daniel Stone wrote:
 Hi,
 
 On 23 January 2015 at 12:42, Gustavo Padovan gust...@padovan.org wrote:
   void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int pipe)
   {
  struct exynos_drm_private *dev_priv = dev-dev_private;
  -   struct drm_pending_vblank_event *e, *t;
  struct drm_crtc *drm_crtc = dev_priv-crtc[pipe];
  struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(drm_crtc);
  -   unsigned long flags;
 
  -   spin_lock_irqsave(dev-event_lock, flags);
  +   if (exynos_crtc-event) {
 
  -   list_for_each_entry_safe(e, t, dev_priv-pageflip_event_list,
  -   base.link) {
  -   /* if event's pipe isn't same as crtc then ignore it. */
  -   if (pipe != e-pipe)
  -   continue;
  -
  -   list_del(e-base.link);
  -   drm_send_vblank_event(dev, -1, e);
  +   spin_lock_irq(dev-event_lock);
  +   drm_send_vblank_event(dev, -1, exynos_crtc-event);
  drm_vblank_put(dev, pipe);
  -   atomic_set(exynos_crtc-pending_flip, 0);
  wake_up(exynos_crtc-pending_flip_queue);
  -   }
  +   spin_unlock_irq(dev-event_lock);
 
  -   spin_unlock_irqrestore(dev-event_lock, flags);
  +   exynos_crtc-event = NULL;
  +   }
   }
 
 Doesn't this need to hold the CRTC lock to not race with, e.g, the
 page_flip caller? This gets called from IRQ context though, so might
 need conversion to soft-IRQ to do so, or another per-CRTC spinlock
 just to protect the event.

dev-even_lock should be enough, but I suspect that lock also protects
exynos_crtc-event (at least that's the case in i915.ko). So would need to
be moved out of the if block again.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 25/29] drm/exynos: atomic phase 1: use drm_plane_helper_disable()

[Daniel Vetter]

On Thu, Dec 18, 2014 at 11:58:51AM -0200, Gustavo Padovan wrote:
 From: Gustavo Padovan gustavo.pado...@collabora.co.uk
 
 The atomic helper to disable planes also uses exynos_update_plane() to
 disable plane so we had to adapt it to both commit and disable planes.
 
 A check for NULL CRTC was added to exynos_plane_mode_set() since planes
 to be disabled have plane_state-crtc set to NULL.
 
 Also win_disable() callback uses plane-crtc as arg for the same reason.
 
 exynos_drm_fb_get_buf_cnt() needs a fb check too to avoid a null pointer.
 
 Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk

Thierry has patches to add an optional atomic_disable vfunc to plane
helpers, which seems useful for you too. Until that patch has landed maybe
just carry Thierry's patch locally. Or undo the merge done here again
later on.
-Daniel

 ---
  drivers/gpu/drm/exynos/exynos_drm_fb.c|  2 +-
  drivers/gpu/drm/exynos/exynos_drm_plane.c | 24 ++--
  2 files changed, 19 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c 
 b/drivers/gpu/drm/exynos/exynos_drm_fb.c
 index d346d1e..470456d 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
 @@ -136,7 +136,7 @@ unsigned int exynos_drm_fb_get_buf_cnt(struct 
 drm_framebuffer *fb)
  
   exynos_fb = to_exynos_fb(fb);
  
 - return exynos_fb-buf_cnt;
 + return exynos_fb ? exynos_fb-buf_cnt : 0;
  }
  
  struct drm_framebuffer *
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c 
 b/drivers/gpu/drm/exynos/exynos_drm_plane.c
 index 1c67fbc..dfca218 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
 @@ -98,6 +98,9 @@ static void exynos_plane_mode_set(struct drm_plane *plane, 
 struct drm_crtc *crtc
   unsigned int actual_w;
   unsigned int actual_h;
  
 + if (!crtc)
 + return;
 +
   actual_w = exynos_plane_get_size(crtc_x, crtc_w, crtc-mode.hdisplay);
   actual_h = exynos_plane_get_size(crtc_y, crtc_h, crtc-mode.vdisplay);
  
 @@ -140,8 +143,6 @@ static void exynos_plane_mode_set(struct drm_plane 
 *plane, struct drm_crtc *crtc
   exynos_plane-crtc_x, exynos_plane-crtc_y,
   exynos_plane-crtc_width, exynos_plane-crtc_height);
  
 - plane-crtc = crtc;
 -
   if (exynos_crtc-ops-win_mode_set)
   exynos_crtc-ops-win_mode_set(exynos_crtc, exynos_plane);
  }
 @@ -179,15 +180,26 @@ exynos_update_plane(struct drm_plane *plane, struct 
 drm_crtc *crtc,
uint32_t src_x, uint32_t src_y,
uint32_t src_w, uint32_t src_h)
  {
 - struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
   struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
 + struct exynos_drm_crtc *exynos_crtc;
  
   exynos_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y,
 crtc_w, crtc_h, src_x  16, src_y  16,
 src_w  16, src_h  16);
  
 - if (exynos_crtc-ops-win_commit)
 - exynos_crtc-ops-win_commit(exynos_crtc, exynos_plane-zpos);
 + if (fb) {
 + exynos_crtc = to_exynos_crtc(crtc);
 + if (exynos_crtc-ops-win_commit)
 + exynos_crtc-ops-win_commit(exynos_crtc,
 +  exynos_plane-zpos);
 + } else {
 + exynos_crtc = to_exynos_crtc(plane-crtc);
 + if (exynos_crtc-ops-win_disable)
 + exynos_crtc-ops-win_disable(exynos_crtc,
 +   exynos_plane-zpos);
 + }
 +
 + plane-crtc = crtc;
  }
  
  static int exynos_disable_plane(struct drm_plane *plane)
 @@ -224,7 +236,7 @@ static int exynos_plane_set_property(struct drm_plane 
 *plane,
  
  static struct drm_plane_funcs exynos_plane_funcs = {
   .update_plane   = drm_plane_helper_update,
 - .disable_plane  = exynos_disable_plane,
 + .disable_plane  = drm_plane_helper_disable,
   .destroy= exynos_plane_destroy,
   .set_property   = exynos_plane_set_property,
  };
 -- 
 1.9.3
 
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/13] drm/irq: Make pipe unsigned and name consistent

[Daniel Vetter]

On Tue, Dec 16, 2014 at 05:53:31PM +0100, Thierry Reding wrote:
 -void drm_wait_one_vblank(struct drm_device *dev, int crtc)
 +void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
  {
 + struct drm_vblank_crtc *vblank = dev-vblank[pipe];
   int ret;
   u32 last;
  
 - ret = drm_vblank_get(dev, crtc);
 - if (WARN(ret, vblank not available on crtc %i, ret=%i\n, crtc, ret))
 + if (WARN_ON(pipe = dev-num_crtcs))

This addition should be in the previous patch. I didn't spot anything else
but it's a bit a tedious patch. But makes sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/13] drm/irq: Expel legacy API

[Daniel Vetter]

On Tue, Dec 16, 2014 at 05:53:34PM +0100, Thierry Reding wrote:
 From: Thierry Reding tred...@nvidia.com
 
 These legacy functions all operate on the struct drm_device * and an
 index to the CRTC that they should access. This is bad because it
 requires keeping track of a global data structures to resolve the index
 to CRTC object lookup. In order to get rid of this global data new APIs
 have been introduced that operate directly on these objects. Currently
 the new functions still operate on the old data, but the goal is to
 eventually move that data into struct drm_crtc. In order to start
 conversion of drivers to the new API, move the old API away.
 
 Signed-off-by: Thierry Reding tred...@nvidia.com

Imo we should try to share code between the legacy vblank code and what
we're now building up with the drm_crtc_ prefixed functions for native kms
drivers. Instead I think we should do full copies with the following
recipe:
- Look at a given function and make sure all kms drivers (and any
  callchains used by kms drivers) uses the drm_crtc_ variant and that any
  ums drivers uses the drm_vblank_ version. So big audit.
- Then for each such function copy it to drm_irq_legacy.c and mark the old
  copy in drm_irq.c as static and drop the EXPORT_SYMBOL for it.
- Then inline the logic for native kms drivers.

Just moving the functions around doesn't really help us at all since we
still have the problem that both ums and kms code uses them. Which means
we can't use drm_crtc and store vblank data in there.
-Daniel

 ---
  drivers/gpu/drm/Makefile |2 +-
  drivers/gpu/drm/drm_internal.h   |2 +
  drivers/gpu/drm/drm_irq.c| 1121 +
  drivers/gpu/drm/drm_irq_legacy.c | 1144 
 ++
  4 files changed, 1165 insertions(+), 1104 deletions(-)
  create mode 100644 drivers/gpu/drm/drm_irq_legacy.c
 
 diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
 index 66e40398b3d3..c0d93beda612 100644
 --- a/drivers/gpu/drm/Makefile
 +++ b/drivers/gpu/drm/Makefile
 @@ -5,7 +5,7 @@
  ccflags-y := -Iinclude/drm
  
  drm-y   :=   drm_auth.o drm_bufs.o drm_cache.o \
 - drm_context.o drm_dma.o \
 + drm_context.o drm_dma.o drm_irq_legacy.o \
   drm_fops.o drm_gem.o drm_ioctl.o drm_irq.o \
   drm_lock.o drm_memory.o drm_drv.o drm_vm.o \
   drm_agpsupport.o drm_scatter.o drm_pci.o \
 diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
 index 12a61d706827..61d83b581c8b 100644
 --- a/drivers/gpu/drm/drm_internal.h
 +++ b/drivers/gpu/drm/drm_internal.h
 @@ -22,7 +22,9 @@
   */
  
  /* drm_irq.c */
 +extern unsigned int drm_timestamp_precision;
  extern unsigned int drm_timestamp_monotonic;
 +extern int drm_vblank_offdelay;
  
  /* drm_fops.c */
  extern struct mutex drm_global_mutex;
 diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
 index ef5d993f06ee..37b536c57cd2 100644
 --- a/drivers/gpu/drm/drm_irq.c
 +++ b/drivers/gpu/drm/drm_irq.c
 @@ -32,15 +32,13 @@
   * OTHER DEALINGS IN THE SOFTWARE.
   */
  
 -#include drm/drmP.h
 -#include drm_trace.h
 -#include drm_internal.h
 -
 +#include linux/export.h
  #include linux/interrupt.h /* For task queue support */
  #include linux/slab.h
  
 -#include linux/vgaarb.h
 -#include linux/export.h
 +#include drm/drmP.h
 +
 +#include drm_trace.h
  
  /* Access macro for slots in vblank timestamp ringbuffer. */
  #define vblanktimestamp(dev, pipe, count) \
 @@ -51,16 +49,7 @@
   */
  #define DRM_TIMESTAMP_MAXRETRIES 3
  
 -/* Threshold in nanoseconds for detection of redundant
 - * vblank irq in drm_handle_vblank(). 1 msec should be ok.
 - */
 -#define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100
 -
 -static bool
 -drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
 -   struct timeval *tvblank, unsigned flags);
 -
 -static unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
 +unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
  
  /*
   * Default to use monotonic timestamps for wait-for-vblank and page-flip
 @@ -68,492 +57,13 @@ static unsigned int drm_timestamp_precision = 20;  /* 
 Default to 20 usecs. */
   */
  unsigned int drm_timestamp_monotonic = 1;
  
 -static int drm_vblank_offdelay = 5000;/* Default to 5000 msecs. */
 +int drm_vblank_offdelay = 5000;/* Default to 5000 msecs. */
  
  module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600);
  module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 
 0600);
  module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
  
  /**
 - * drm_update_vblank_count - update the master vblank counter
 - * @dev: DRM device
 - * @pipe: counter to update
 - *
 - * Call back into the driver to update the appropriate vblank counter
 - * (specified by @crtc).  Deal with wraparound, if it occurred, and
 - * update the last read value 

Re: [PATCH 12/13] drm/irq: Expel legacy API

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:59:28PM +0100, Daniel Vetter wrote:
 On Tue, Dec 16, 2014 at 05:53:34PM +0100, Thierry Reding wrote:
  From: Thierry Reding tred...@nvidia.com
  
  These legacy functions all operate on the struct drm_device * and an
  index to the CRTC that they should access. This is bad because it
  requires keeping track of a global data structures to resolve the index
  to CRTC object lookup. In order to get rid of this global data new APIs
  have been introduced that operate directly on these objects. Currently
  the new functions still operate on the old data, but the goal is to
  eventually move that data into struct drm_crtc. In order to start
  conversion of drivers to the new API, move the old API away.
  
  Signed-off-by: Thierry Reding tred...@nvidia.com
 
 Imo we should try to share code between the legacy vblank code and what
 we're now building up with the drm_crtc_ prefixed functions for native kms
 drivers. Instead I think we should do full copies with the following
 recipe:
 - Look at a given function and make sure all kms drivers (and any
   callchains used by kms drivers) uses the drm_crtc_ variant and that any
   ums drivers uses the drm_vblank_ version. So big audit.
 - Then for each such function copy it to drm_irq_legacy.c and mark the old
   copy in drm_irq.c as static and drop the EXPORT_SYMBOL for it.
 - Then inline the logic for native kms drivers.
 
 Just moving the functions around doesn't really help us at all since we
 still have the problem that both ums and kms code uses them. Which means
 we can't use drm_crtc and store vblank data in there.

There's the added problem that moving breaks git blame (at least in
default mode) a bit, so better to keep the version that we want to
transform into the kms-native ones where they currently are.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/13] drm/irq: Document return values more consistently

[Daniel Vetter]

On Tue, Dec 16, 2014 at 05:53:33PM +0100, Thierry Reding wrote:
 From: Thierry Reding tred...@nvidia.com
 
 Some of the functions are documented inconsistently. Add Returns:
 sections where missing and use consistent style to describe the return
 value.
 
 Signed-off-by: Thierry Reding tred...@nvidia.com

With the comment in patch 9 addressed patches 1-11 are

Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch

 ---
  drivers/gpu/drm/drm_irq.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
 index b34900a8e172..ef5d993f06ee 100644
 --- a/drivers/gpu/drm/drm_irq.c
 +++ b/drivers/gpu/drm/drm_irq.c
 @@ -991,6 +991,9 @@ EXPORT_SYMBOL(drm_crtc_send_vblank_event);
   * drm_vblank_enable - enable the vblank interrupt on a CRTC
   * @dev: DRM device
   * @pipe: CRTC index
 + *
 + * Returns:
 + * Zero on success or a negative error code on failure.
   */
  static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
  {
 @@ -1035,7 +1038,7 @@ static int drm_vblank_enable(struct drm_device *dev, 
 unsigned int pipe)
   * This is the legacy version of drm_crtc_vblank_get().
   *
   * Returns:
 - * Zero on success, nonzero on failure.
 + * Zero on success or a negative error code on failure.
   */
  int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
  {
 @@ -1072,7 +1075,7 @@ EXPORT_SYMBOL(drm_vblank_get);
   * This is the native kms version of drm_vblank_off().
   *
   * Returns:
 - * Zero on success, nonzero on failure.
 + * Zero on success or a negative error code on failure.
   */
  int drm_crtc_vblank_get(struct drm_crtc *crtc)
  {
 -- 
 2.1.3
 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Mon, Nov 03, 2014 at 09:06:04AM +0100, Thierry Reding wrote:
 On Fri, Oct 31, 2014 at 04:59:40PM +0100, Daniel Vetter wrote:
  On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
   On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
 On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
  On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org 
  wrote:
   @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
 * @driver_private: pointer to the bridge driver's internal 
   context
 */
struct drm_bridge {
   - struct drm_device *dev;
   + struct device *dev;
  
   Please don't rename the -dev pointer into drm. Because _all_ 
   the other
   drm structures still call it -dev. Also, can't we use struct 
   device_node
   here like we do in the of helpers Russell added? See 
   7e435aad38083
  
  
   I think this is modeled after the naming in drm_panel, FWIW. 
   However,
   seems reasonable to keep the device_node instead.
  
  Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like 
  with
  drm_crtc drop the struct device and go directly to a struct
  device_node. Since we don't really need the sturct device, the only
  thing we care about is the of_node. For added bonus wrap an #ifdef
  CONFIG_OF around all the various struct device_node in drm_foo.h.
  Should be all fairly simple to pull off with cocci.
  
  Thierry?
 
 The struct device * is in DRM panel because there's nothing device 
 tree
 specific about the concept. Having a struct device_node * instead 
 would
 indicate that it can only be used with a device tree, whereas the
 framework doesn't care the tiniest bit what type of device we have.
 
 While the trend clearly is to use more device tree, I don't think we
 should make it impossible for anybody else to use these frameworks.
 
 There are other advantages to keeping a struct device *, like having
 access to the proper device and its name. Also you get access to the
 device_node * via dev-of_node anyway. I don't see any advantage in
 switching to just a struct device_node *, only disadvantages.

Well the idea is to make the lookup key specific, and conditional on
#CONFIG_OF. If there's going to be another neat way to enumerate 
platform
devices then I think we should add that, too. Or maybe we should have a
void *platform_data or so.

The reason I really don't want a struct device * in core drm structures 
is
that two releases down the road people will have found tons of really
great ways to abuse them and re-create a midlayer. DRM core really 
should
only care about the sw objects and not be hw specific at all. Heck 
there's
not even an requirement to have any piece of actual hw, you could write 
a
completely fake drm driver (for e.g. testing like the new v4l driver).

Tbh I wonder a bit why we even have this registery embedded into the 
core
drm objects. Essentially the only thing you're doing is a list that maps
some platform specific key onto some subsystem specific driver structure
or fails the lookup. So instead of putting all these low-level details
into drm core structures can't we just have a generic hashtable/list for
this, plus some static inline helpers that cast the void * you get into
the one you want?

I also get the feeling that this really should be in the driver core 
(like
the component helpers), and that we should think a lot harder about
lifetimes and refcounting (see my other reply on that).
   
   Yes, that sounds very useful indeed. Also see my reply to yours. =)
  
  Just replying here with some of the irc discussions we've had. Since
  drm_bridge/panel isn't a core drm interface object exposed to userspace
  it's less critical. I still think that wasting a few brain cycles to have
  a clear separation between the abstract interface object and how to bind
  and unbind the pieces together is worthwhile, even though the cost when
  getting it wrong is much less severe than in the case of a mandatory piece
  of core infrastructure.
  
  I think in general the recent armsoc motivated drm infrastructure lacks a
  bit in attention to these details. E.g. the cma helpers are built into the
  drm.ko module, but clearly are auxilliary library code. So they should be
  pulled out and the headers clean up, so that we have a clear separation
  between core and helpers. Otherwise someone will sooner or later screw up
  and insert a helper depency into the core, and then we've started with the
  midlayer mess. Same goes with drm_bridge/panel, which didn't even bother
  to have separate headers from the core modeset header drm_crtc.h.
 
 DRM panel

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote:
 On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
  That makes the entire thing a bit non-trivial, which is why I think it
  would be better as some generic helper. Which then gets embedded or
  instantiated for specific cases, like dtdrm_panel or dtdrm_bridge.
  Or maybe even acpidrm_bridge, who knows ;-)
 
 I worry a little about type safety. How will this generic helper know
 what registry to look in for? Or conversely, if all these resources are
 added to a single registry how do you know that they're of the correct
 type? Failing to ensure this could cause situations where you're asking
 for a panel and get a bridge in return because you've wrongly wired it
 up in device tree for example.
 
 But perhaps if both the registry and the device parts are turned into
 helpers we could still have a single core implementation and then
 instantiate that for each type, something roughly like this:
 
   struct registry {
   struct list_head list;
   struct mutex lock;
   };
 
   struct registry_record {
   struct list_head list;
   struct module *owner;
   struct kref *ref;
 
   struct device *dev;
   };
 
   int registry_add(struct registry *registry, struct registry_record 
 *record)
   {
   ...
   try_module_get(record-owner);
   ...
   }
 
   struct registry_record *registry_find_by_of_node(struct registry 
 *registry,
struct device_node *np)
   {
   ...
   kref_get(...);
   ...
   }
 
 That way it should be possible to embed these into other structures,
 like so:
 
   struct drm_panel {
   struct registry_record record;
   ...
   };
 
   static struct registry drm_panels;
 
   int drm_panel_add(struct drm_panel *panel)
   {
   return registry_add(drm_panels, panel-record);
   }
 
   struct drm_panel *of_drm_panel_find(struct device_node *np)
   {
   struct registry_record *record;
 
   record = registry_find_by_of_node(drm_panels, np);
 
   return container_of(record, struct drm_panel, record);
   }
 
 Is that what you had in mind?

Yeah I've thought that we should instantiate using macros even, so that we
have per-type registries. So you'd smash the usual set of
DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take
a (name, key, value) tripled. For the example here(of_drm_panel, struct
device_node *, struct drm_panel *) or similar. I might be hand-waving over
a few too many details though ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Thu, Oct 30, 2014 at 10:09:28AM +, Russell King - ARM Linux wrote:
 On Thu, Oct 30, 2014 at 11:01:02AM +0100, Andrzej Hajda wrote:
  On 10/29/2014 10:14 AM, Thierry Reding wrote:
   On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
   I think we nee try_get_module for the code and kref on the actual data
   structures.
   
   Agreed, that should do the trick. We'd probably need some sort of logic
   to also make operations return something like -ENODEV when the
   underlying device has disappeared. I think David had introduced
   something similar for DRM device not so long ago?
  
  If the underlying device disappears it would be good to receive
  notification anyway to trigger DRM HPD event. And if we have the
  notification, we can release references to the device smoothly. We do
  not need to play tricky games with krefs, zombie data and module
  refcounting.
 
 Any solution which thinks it needs to lock modules into the core is
 fundamentally broken.  It totally misses the point.
 
 While you can lock a module into the core using try_get_module(), that
 doesn't stop the device itself being unbound from a driver.  Soo many
 people have fallen into that trap.  They write their device driver,
 along with some kind of framework which they make use try_get_module().
 They think its safe.  When you then echo the device name into the
 driver's unbind sysfs file, all hell breaks loose and the kernel oopses.
 
 try_get_module is /totally/ useless for ensuring that things stick around.
 
 The reality is that you can't make devices stick around.  Once that
 remove callback from the driver layer is called, that's it, the device
 _is_ going away whether you like it or not.  You can't stop it.  It's
 no good returning -EBUSY, because the remove return code is ignored.
 
 What's more scarey is when you consider that in a real hotplug
 situation, when the remove callback is called, the device has
 /already/ gone.
 
 So please, stop thinking that try_get_module() has some magic solution.
 Any solution to device lifetimes using try_get_module() totally misses
 the problem, and is just mere obfuscation and actually a bug in itself.

We need proper refcounting ofc, but we also need to make sure that as long
as the thing is around the text section for the final unref (at least
that) doesn't go poof. I'd prefer if the framework takes care of that
detail and drivers could just supply a THIS_MODULE at the right place.

But fully agree on your overall point that try_get_module alone is pure
snake oil.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
 On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
  On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
   On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org 
wrote:
 @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
   * @driver_private: pointer to the bridge driver's internal context
   */
  struct drm_bridge {
 - struct drm_device *dev;
 + struct device *dev;

 Please don't rename the -dev pointer into drm. Because _all_ the 
 other
 drm structures still call it -dev. Also, can't we use struct 
 device_node
 here like we do in the of helpers Russell added? See 7e435aad38083


 I think this is modeled after the naming in drm_panel, FWIW. However,
 seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
drm_crtc drop the struct device and go directly to a struct
device_node. Since we don't really need the sturct device, the only
thing we care about is the of_node. For added bonus wrap an #ifdef
CONFIG_OF around all the various struct device_node in drm_foo.h.
Should be all fairly simple to pull off with cocci.

Thierry?
   
   The struct device * is in DRM panel because there's nothing device tree
   specific about the concept. Having a struct device_node * instead would
   indicate that it can only be used with a device tree, whereas the
   framework doesn't care the tiniest bit what type of device we have.
   
   While the trend clearly is to use more device tree, I don't think we
   should make it impossible for anybody else to use these frameworks.
   
   There are other advantages to keeping a struct device *, like having
   access to the proper device and its name. Also you get access to the
   device_node * via dev-of_node anyway. I don't see any advantage in
   switching to just a struct device_node *, only disadvantages.
  
  Well the idea is to make the lookup key specific, and conditional on
  #CONFIG_OF. If there's going to be another neat way to enumerate platform
  devices then I think we should add that, too. Or maybe we should have a
  void *platform_data or so.
  
  The reason I really don't want a struct device * in core drm structures is
  that two releases down the road people will have found tons of really
  great ways to abuse them and re-create a midlayer. DRM core really should
  only care about the sw objects and not be hw specific at all. Heck there's
  not even an requirement to have any piece of actual hw, you could write a
  completely fake drm driver (for e.g. testing like the new v4l driver).
  
  Tbh I wonder a bit why we even have this registery embedded into the core
  drm objects. Essentially the only thing you're doing is a list that maps
  some platform specific key onto some subsystem specific driver structure
  or fails the lookup. So instead of putting all these low-level details
  into drm core structures can't we just have a generic hashtable/list for
  this, plus some static inline helpers that cast the void * you get into
  the one you want?
  
  I also get the feeling that this really should be in the driver core (like
  the component helpers), and that we should think a lot harder about
  lifetimes and refcounting (see my other reply on that).
 
 Yes, that sounds very useful indeed. Also see my reply to yours. =)

Just replying here with some of the irc discussions we've had. Since
drm_bridge/panel isn't a core drm interface object exposed to userspace
it's less critical. I still think that wasting a few brain cycles to have
a clear separation between the abstract interface object and how to bind
and unbind the pieces together is worthwhile, even though the cost when
getting it wrong is much less severe than in the case of a mandatory piece
of core infrastructure.

I think in general the recent armsoc motivated drm infrastructure lacks a
bit in attention to these details. E.g. the cma helpers are built into the
drm.ko module, but clearly are auxilliary library code. So they should be
pulled out and the headers clean up, so that we have a clear separation
between core and helpers. Otherwise someone will sooner or later screw up
and insert a helper depency into the core, and then we've started with the
midlayer mess. Same goes with drm_bridge/panel, which didn't even bother
to have separate headers from the core modeset header drm_crtc.h.

So would be great if someone could invest a bit of time into cleaning this
up. Writing proper api docs also helps a lot with achieving a clean and
sensible split ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Wed, Oct 29, 2014 at 10:09:04AM +0100, Andrzej Hajda wrote:
 On 10/29/2014 08:58 AM, Daniel Vetter wrote:
  On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote:
  On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
  On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
  thierry.red...@gmail.com wrote:
  On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
  On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar ajayn...@gmail.com wrote:
  On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@ffwll.ch wrote:
  [...]
  Hm, if you do this can you pls also update drm_panel accordingly? It
  shouldn't be a lot of fuzz and would make things around drm+dt more
  consistent.
  Are you talking about using struct device_node instead of struct 
  device?
  I guess you have misplaced the comment under the wrong section!
 
  Yeah, that should have been one up ;-)
 
  Like I said earlier, I don't think dropping struct device * in favour of
  struct device_node * is a good idea.
  I am not sure about drm_panel.
  But, I am not really doing anything with the struct device pointer in
  case of bridge.
  So, just wondering if it is really needed?
 
  I think it's useful to have it just to send the right message. DRM panel
  and DRM bridge aren't specific to device tree. They are completely
  generic and can work with any type of device, whether it was
  instantiated from the device tree or some other infrastructure. Dropping
  struct device * will make it effectively useless on anything but DT. I
  don't think we should strive for that, even if only DT-enabled platforms
  currently use them.
  
  See my other reply, but I now think we should put neither into drm
  structures. This find me the driver structures for this device problem
  looks really generic, and it has nothing to do with the drm structures and
  concepts like bridges/panels at all. It shouldn't be in there at all.
  
  Adding it looks very much like reintroducing the drm midlayer that we just
  finally made obsolete, just not at the top-level (struct drm_device) but
  at a bunch of leaf nodes. I expect all the same issues though. And I'm
  definitely not looking to de-midlayer more stuff that we're just adding.
  
  Imo this should be solved as a separate helper thing, maybe in the driver
  core akin to the component helpers from Russell.
  -Daniel
  
 
 As I understand you want something generic to look for panels, bridges,
 whatever and, like components, it should allow to safe hot plug/unplug.
 I have proposed such thing few months ago [1]. Have you looked at it?
 
 [1]: https://lkml.org/lkml/2014/4/30/345

Yeah I think I've looked but wasn't convinced. I do agree though that we
should definitely aim for something in the driver core. Since if Greg
doesn't like it it's not suddenly better if we just hide it in the drm
subsystem. This really smells like a generic issue - after all we already
have a two implementations with bridgespanels already.

So maybe we need to augment the component framework with the possibility
to add additional devices later on at runtime, or something similar. Not
really sure since I don't have actual practice with these issues since
i915 doesn't (yet) have these problems.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
 On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
  On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter dan...@ffwll.ch wrote:
   On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org wrote:
   @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
 * @driver_private: pointer to the bridge driver's internal context
 */
struct drm_bridge {
   - struct drm_device *dev;
   + struct device *dev;
  
   Please don't rename the -dev pointer into drm. Because _all_ the other
   drm structures still call it -dev. Also, can't we use struct 
   device_node
   here like we do in the of helpers Russell added? See 7e435aad38083
  
  
   I think this is modeled after the naming in drm_panel, FWIW. However,
   seems reasonable to keep the device_node instead.
  
   Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
   drm_crtc drop the struct device and go directly to a struct
   device_node. Since we don't really need the sturct device, the only
   thing we care about is the of_node. For added bonus wrap an #ifdef
   CONFIG_OF around all the various struct device_node in drm_foo.h.
   Should be all fairly simple to pull off with cocci.
  
   Thierry?
  
  Looking at the of_drm_find_panel function I actually wonder how that
  works - the drm_panel doesn't really need to stick around afaics.
  After all panel_list is global so some other driver can unload.
  Russell's of support for possible_crtcs code works differently since
  it only looks at per-drm_device lists.
 
 I don't understand. Panels are global resources that get registered by
 some driver that isn't tied to the DRM device until attached. It can't
 be in a per-DRM device list, because it's external to the device.
 
 And yes, they can go away when a driver is unloaded, though it's not the
 typical use-case.
 
  This bridge code here though suffers from the same. So to me this
  looks rather fishy.
 
 Well, this version of the DRM bridge support was written to be close to
 DRM panel, so it isn't really surprising that it's similar =), but like
 I said, I don't really understand what you think is wrong with it.

You have a mutex to protect the global list of bridges/panels. But if you
do a lookup you don't grab a reference count on the panel, so the moment
you drop the list mutex the panel/bridge can go away.

Yes usually you don't unload a driver on a soc but soc isn't everything
and designing new stuff to not be hotunplug save isn't great either.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
 On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
  On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org wrote:
   @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
 * @driver_private: pointer to the bridge driver's internal context
 */
struct drm_bridge {
   - struct drm_device *dev;
   + struct device *dev;
  
   Please don't rename the -dev pointer into drm. Because _all_ the other
   drm structures still call it -dev. Also, can't we use struct device_node
   here like we do in the of helpers Russell added? See 7e435aad38083
  
  
   I think this is modeled after the naming in drm_panel, FWIW. However,
   seems reasonable to keep the device_node instead.
  
  Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
  drm_crtc drop the struct device and go directly to a struct
  device_node. Since we don't really need the sturct device, the only
  thing we care about is the of_node. For added bonus wrap an #ifdef
  CONFIG_OF around all the various struct device_node in drm_foo.h.
  Should be all fairly simple to pull off with cocci.
  
  Thierry?
 
 The struct device * is in DRM panel because there's nothing device tree
 specific about the concept. Having a struct device_node * instead would
 indicate that it can only be used with a device tree, whereas the
 framework doesn't care the tiniest bit what type of device we have.
 
 While the trend clearly is to use more device tree, I don't think we
 should make it impossible for anybody else to use these frameworks.
 
 There are other advantages to keeping a struct device *, like having
 access to the proper device and its name. Also you get access to the
 device_node * via dev-of_node anyway. I don't see any advantage in
 switching to just a struct device_node *, only disadvantages.

Well the idea is to make the lookup key specific, and conditional on
#CONFIG_OF. If there's going to be another neat way to enumerate platform
devices then I think we should add that, too. Or maybe we should have a
void *platform_data or so.

The reason I really don't want a struct device * in core drm structures is
that two releases down the road people will have found tons of really
great ways to abuse them and re-create a midlayer. DRM core really should
only care about the sw objects and not be hw specific at all. Heck there's
not even an requirement to have any piece of actual hw, you could write a
completely fake drm driver (for e.g. testing like the new v4l driver).

Tbh I wonder a bit why we even have this registery embedded into the core
drm objects. Essentially the only thing you're doing is a list that maps
some platform specific key onto some subsystem specific driver structure
or fails the lookup. So instead of putting all these low-level details
into drm core structures can't we just have a generic hashtable/list for
this, plus some static inline helpers that cast the void * you get into
the one you want?

I also get the feeling that this really should be in the driver core (like
the component helpers), and that we should think a lot harder about
lifetimes and refcounting (see my other reply on that).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote:
 On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
  On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
  thierry.red...@gmail.com wrote:
   On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
   On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar ajayn...@gmail.com wrote:
On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@ffwll.ch wrote:
   [...]
Hm, if you do this can you pls also update drm_panel accordingly? It
shouldn't be a lot of fuzz and would make things around drm+dt more
consistent.
Are you talking about using struct device_node instead of struct 
device?
I guess you have misplaced the comment under the wrong section!
  
   Yeah, that should have been one up ;-)
  
   Like I said earlier, I don't think dropping struct device * in favour of
   struct device_node * is a good idea.
  I am not sure about drm_panel.
  But, I am not really doing anything with the struct device pointer in
  case of bridge.
  So, just wondering if it is really needed?
 
 I think it's useful to have it just to send the right message. DRM panel
 and DRM bridge aren't specific to device tree. They are completely
 generic and can work with any type of device, whether it was
 instantiated from the device tree or some other infrastructure. Dropping
 struct device * will make it effectively useless on anything but DT. I
 don't think we should strive for that, even if only DT-enabled platforms
 currently use them.

See my other reply, but I now think we should put neither into drm
structures. This find me the driver structures for this device problem
looks really generic, and it has nothing to do with the drm structures and
concepts like bridges/panels at all. It shouldn't be in there at all.

Adding it looks very much like reintroducing the drm midlayer that we just
finally made obsolete, just not at the top-level (struct drm_device) but
at a bunch of leaf nodes. I expect all the same issues though. And I'm
definitely not looking to de-midlayer more stuff that we're just adding.

Imo this should be solved as a separate helper thing, maybe in the driver
core akin to the component helpers from Russell.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar ajayn...@gmail.com wrote:
 Hi Daniel and Sean,

 Thanks for the comments!

 On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanp...@chromium.org wrote:
 On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter dan...@ffwll.ch wrote:
 So don't ask why but I accidentally ended up in a branch looking at this
 patch and didn't like it. So very quickgrumpy review.

 First, please make the patch subject more descriptive: I'd expect a helper
 function scaffolding like the various crtc/probe/dp ... helpers we already
 have. You instead add code to untangle the probe ordering. Please say so.
 Sure. I will reword it properly.

 More comments below.

 On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
 A set of helper functions are defined in this patch to make
 bridge driver probe independent of the drm flow.

 The bridge devices register themselves on a lookup table
 when they get probed by calling drm_bridge_add.

 The parent encoder driver waits till the bridge is available
 in the lookup table(by calling of_drm_find_bridge) and then
 continues with its initialization.

 The encoder driver should also call drm_bridge_attach to pass
 on the drm_device, encoder pointers to the bridge object.

 drm_bridge_attach inturn calls drm_bridge_init to register itself
 with the drm core. Later, it calls bridge-funcs-attach so that
 bridge can continue with other initializations.

 Signed-off-by: Ajay Kumar ajaykumar...@samsung.com

 [snip]

 @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
   * @driver_private: pointer to the bridge driver's internal context
   */
  struct drm_bridge {
 - struct drm_device *dev;
 + struct device *dev;

 Please don't rename the -dev pointer into drm. Because _all_ the other
 drm structures still call it -dev. Also, can't we use struct device_node
 here like we do in the of helpers Russell added? See 7e435aad38083


 I think this is modeled after the naming in drm_panel,
 Right, The entire rework is based on how drm_panel framework is structured.

 FWIW. However,
 seems reasonable to keep the device_node instead.
 Yes, its visible that just device_node would be sufficient.
 This will save us from renaming drm_device as well.

 + struct drm_device *drm;
 + struct drm_encoder *encoder;

 This breaks bridge-bridge chaining (if we ever get there). It seems
 pretty much unused anyway except for an EBUSY check. Can't you use
 bridge-dev for that?


 Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
 and leave it up to the caller to establish the proper chain.
 Ok. I will use drm_device pointer directly instead of passing encoder pointer.

Hm, if you do this can you pls also update drm_panel accordingly? It
shouldn't be a lot of fuzz and would make things around drm+dt more
consistent.


   struct list_head head;
 + struct list_head list;

 These lists need better names. I know that the head is really awful,
 especially since it's actually not the head of the list but just an
 element.

 I think we can just rip bridge_list out of mode_config if we're going
 to keep track of bridges elsewhere. So we can nuke head and keep
 list. This also means that bridge-destroy() goes away, but that's
 probably Ok if everything converts to the standalone driver model
 where we have driver-remove()

 Sean
 Great! Thierry actually mentioned about this once, and we have the
 confirmation now.


   struct drm_mode_object base;


 Aside: I've noticed all this trying to update the kerneldoc for struct
 drm_bridge, which just showed that this patch makes inconsistent changes.
 Trying to write kerneldoc is a really great way to come up with better
 interfaces imo.

 Cheers, Daniel
 I din't get this actually. You want me to create Doc../drm_bridge.txt
 or something similar?

If you want to document drm_bridge then I recomment to sprinkle proper
kerneldoc over drm_bridge.c and pull it all into the drm DocBook
template. That way all the drm documentation is in one place. I've
done that for drm_crtc.h in an unrelated patch series (but based upon
a branch with your patch here included) and there's struct drm_bridge*
in there. Hence why I've noticed.

If you do kerneldoc/DocBook integration for drm_bridge it's probably
best to also do it for drm_panel and use the opportunity to
review/rework the interfaces a bit for consistency. E.g. move dt
related stuff into drm_of.c and have that in a separate section in the
docs.
-Daniel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar ajayn...@gmail.com wrote:
 On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter dan...@ffwll.ch wrote:
 On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar ajayn...@gmail.com wrote:
 Hi Daniel and Sean,

 Thanks for the comments!

 On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanp...@chromium.org wrote:
 On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter dan...@ffwll.ch wrote:
 So don't ask why but I accidentally ended up in a branch looking at this
 patch and didn't like it. So very quickgrumpy review.

 First, please make the patch subject more descriptive: I'd expect a helper
 function scaffolding like the various crtc/probe/dp ... helpers we already
 have. You instead add code to untangle the probe ordering. Please say so.
 Sure. I will reword it properly.

 More comments below.

 On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
 A set of helper functions are defined in this patch to make
 bridge driver probe independent of the drm flow.

 The bridge devices register themselves on a lookup table
 when they get probed by calling drm_bridge_add.

 The parent encoder driver waits till the bridge is available
 in the lookup table(by calling of_drm_find_bridge) and then
 continues with its initialization.

 The encoder driver should also call drm_bridge_attach to pass
 on the drm_device, encoder pointers to the bridge object.

 drm_bridge_attach inturn calls drm_bridge_init to register itself
 with the drm core. Later, it calls bridge-funcs-attach so that
 bridge can continue with other initializations.

 Signed-off-by: Ajay Kumar ajaykumar...@samsung.com

 [snip]

 @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
   * @driver_private: pointer to the bridge driver's internal context
   */
  struct drm_bridge {
 - struct drm_device *dev;
 + struct device *dev;

 Please don't rename the -dev pointer into drm. Because _all_ the other
 drm structures still call it -dev. Also, can't we use struct device_node
 here like we do in the of helpers Russell added? See 7e435aad38083


 I think this is modeled after the naming in drm_panel,
 Right, The entire rework is based on how drm_panel framework is structured.

 FWIW. However,
 seems reasonable to keep the device_node instead.
 Yes, its visible that just device_node would be sufficient.
 This will save us from renaming drm_device as well.

 + struct drm_device *drm;
 + struct drm_encoder *encoder;

 This breaks bridge-bridge chaining (if we ever get there). It seems
 pretty much unused anyway except for an EBUSY check. Can't you use
 bridge-dev for that?


 Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
 and leave it up to the caller to establish the proper chain.
 Ok. I will use drm_device pointer directly instead of passing encoder 
 pointer.

 Hm, if you do this can you pls also update drm_panel accordingly? It
 shouldn't be a lot of fuzz and would make things around drm+dt more
 consistent.
 Are you talking about using struct device_node instead of struct device?
 I guess you have misplaced the comment under the wrong section!

Yeah, that should have been one up ;-)

   struct list_head head;
 + struct list_head list;

 These lists need better names. I know that the head is really awful,
 especially since it's actually not the head of the list but just an
 element.

 I think we can just rip bridge_list out of mode_config if we're going
 to keep track of bridges elsewhere. So we can nuke head and keep
 list. This also means that bridge-destroy() goes away, but that's
 probably Ok if everything converts to the standalone driver model
 where we have driver-remove()

 Sean
 Great! Thierry actually mentioned about this once, and we have the
 confirmation now.


   struct drm_mode_object base;


 Aside: I've noticed all this trying to update the kerneldoc for struct
 drm_bridge, which just showed that this patch makes inconsistent changes.
 Trying to write kerneldoc is a really great way to come up with better
 interfaces imo.

 Cheers, Daniel
 I din't get this actually. You want me to create Doc../drm_bridge.txt
 or something similar?

 If you want to document drm_bridge then I recomment to sprinkle proper
 kerneldoc over drm_bridge.c and pull it all into the drm DocBook
 template. That way all the drm documentation is in one place. I've
 done that for drm_crtc.h in an unrelated patch series (but based upon
 a branch with your patch here included) and there's struct drm_bridge*
 in there. Hence why I've noticed.
 Can you send a link for that?
 And, is there any problem if the doc comes later?

Since quite a while we've asked for the kerneldoc polish as part of
each drm core patch series. It's just that drm_bridge/panel kinda have
flown under the radar of the people usually asking for docs ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

So don't ask why but I accidentally ended up in a branch looking at this
patch and didn't like it. So very quickgrumpy review.

First, please make the patch subject more descriptive: I'd expect a helper
function scaffolding like the various crtc/probe/dp ... helpers we already
have. You instead add code to untangle the probe ordering. Please say so.

More comments below.

On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
 A set of helper functions are defined in this patch to make
 bridge driver probe independent of the drm flow.
 
 The bridge devices register themselves on a lookup table
 when they get probed by calling drm_bridge_add.
 
 The parent encoder driver waits till the bridge is available
 in the lookup table(by calling of_drm_find_bridge) and then
 continues with its initialization.
 
 The encoder driver should also call drm_bridge_attach to pass
 on the drm_device, encoder pointers to the bridge object.
 
 drm_bridge_attach inturn calls drm_bridge_init to register itself
 with the drm core. Later, it calls bridge-funcs-attach so that
 bridge can continue with other initializations.
 
 Signed-off-by: Ajay Kumar ajaykumar...@samsung.com

[snip]

 @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
   * @driver_private: pointer to the bridge driver's internal context
   */
  struct drm_bridge {
 - struct drm_device *dev;
 + struct device *dev;

Please don't rename the -dev pointer into drm. Because _all_ the other
drm structures still call it -dev. Also, can't we use struct device_node
here like we do in the of helpers Russell added? See 7e435aad38083

 + struct drm_device *drm;
 + struct drm_encoder *encoder;

This breaks bridge-bridge chaining (if we ever get there). It seems
pretty much unused anyway except for an EBUSY check. Can't you use
bridge-dev for that?

   struct list_head head;
 + struct list_head list;

These lists need better names. I know that the head is really awful,
especially since it's actually not the head of the list but just an
element.
  
   struct drm_mode_object base;


Aside: I've noticed all this trying to update the kerneldoc for struct
drm_bridge, which just showed that this patch makes inconsistent changes.
Trying to write kerneldoc is a really great way to come up with better
interfaces imo.

Cheers, Daniel

  
 @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector 
 *connector);
  /* helper to unplug all connectors from sysfs for device */
  extern void drm_connector_unplug_all(struct drm_device *dev);
  
 +extern int drm_bridge_add(struct drm_bridge *bridge);
 +extern void drm_bridge_remove(struct drm_bridge *bridge);
 +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
 +extern int drm_bridge_attach(struct drm_bridge *bridge,
 + struct drm_encoder *encoder);
  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge 
 *bridge);
  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
  
 -- 
 1.7.9.5
 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org wrote:
 @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
   * @driver_private: pointer to the bridge driver's internal context
   */
  struct drm_bridge {
 - struct drm_device *dev;
 + struct device *dev;

 Please don't rename the -dev pointer into drm. Because _all_ the other
 drm structures still call it -dev. Also, can't we use struct device_node
 here like we do in the of helpers Russell added? See 7e435aad38083


 I think this is modeled after the naming in drm_panel, FWIW. However,
 seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
drm_crtc drop the struct device and go directly to a struct
device_node. Since we don't really need the sturct device, the only
thing we care about is the of_node. For added bonus wrap an #ifdef
CONFIG_OF around all the various struct device_node in drm_foo.h.
Should be all fairly simple to pull off with cocci.

Thierry?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter dan...@ffwll.ch wrote:
 On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanp...@chromium.org wrote:
 @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
   * @driver_private: pointer to the bridge driver's internal context
   */
  struct drm_bridge {
 - struct drm_device *dev;
 + struct device *dev;

 Please don't rename the -dev pointer into drm. Because _all_ the other
 drm structures still call it -dev. Also, can't we use struct device_node
 here like we do in the of helpers Russell added? See 7e435aad38083


 I think this is modeled after the naming in drm_panel, FWIW. However,
 seems reasonable to keep the device_node instead.

 Hm, indeed. Tbh I vote to rename drm_panel-drm to -dev and like with
 drm_crtc drop the struct device and go directly to a struct
 device_node. Since we don't really need the sturct device, the only
 thing we care about is the of_node. For added bonus wrap an #ifdef
 CONFIG_OF around all the various struct device_node in drm_foo.h.
 Should be all fairly simple to pull off with cocci.

 Thierry?

Looking at the of_drm_find_panel function I actually wonder how that
works - the drm_panel doesn't really need to stick around afaics.
After all panel_list is global so some other driver can unload.
Russell's of support for possible_crtcs code works differently since
it only looks at per-drm_device lists.

This bridge code here though suffers from the same. So to me this
looks rather fishy.

It doesn't help that we have drm_of.[hc] around but not all the of
code is in there. Adding Russell too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drm/exynos: enable vblank after DPMS on

[Daniel Vetter]

On Fri, Oct 10, 2014 at 02:31:55PM +0200, Andrzej Hajda wrote:
 Before DPMS off driver disables vblank.
 It should be balanced by vblank enable after DPMS on.
 The patch fixes issue with page_flip ioctl not being able
 to acquire vblank counter introduced by patch:
 drm: Always reject drm_vblank_get() after drm_vblank_off()
 
 Signed-off-by: Andrzej Hajda a.ha...@samsung.com

Yeah, you should always call vblank_on again when you (re)enable a crtc,
whether this is through a set_config call or through dpms. This is
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch

Sorry that we didn't catch the impact of this additional check on existing
drivers, I've thought I've reviewed them and checked that they all call
vblank_on. But I didn't take into account that the codepaths might differ
for dpms and set_config paths. Otoh most drivers really should implement
one in terms of the other.

Cheers, Daniel

 ---
  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
 b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
 index 8e38e9f..45026e6 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
 @@ -71,13 +71,16 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, 
 int mode)
   !atomic_read(exynos_crtc-pending_flip),
   HZ/20))
   atomic_set(exynos_crtc-pending_flip, 0);
 - drm_vblank_off(crtc-dev, exynos_crtc-pipe);
 + drm_crtc_vblank_off(crtc);
   }
  
   if (manager-ops-dpms)
   manager-ops-dpms(manager, mode);
  
   exynos_crtc-dpms = mode;
 +
 + if (mode == DRM_MODE_DPMS_ON)
 + drm_crtc_vblank_on(crtc);
  }
  
  static void exynos_drm_crtc_prepare(struct drm_crtc *crtc)
 -- 
 1.9.1
 
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Daniel Vetter]

On Fri, Oct 03, 2014 at 10:24:09AM +0200, Andrzej Hajda wrote:
 The main intent of this patchset is to allow use of suspend/resume drm driver
 callbacks in KMS drivers, as these callbacks seems to me the best place
 to implement suspend/resume functionality in drm driver.
 Implementing this functionality in master component driver PM ops is 
 problematic
 as those callbacks can be called asynchronously regardless of state/existence 
 of
 drm device, thus it would require additional synchronization mechanism.
 
 Callbacks re-enabling requires small changes in i915 and exynos driver.
 The patchset contains also fix of exynos resume callback.

Nack.

Like completely and totally. The drm core has really no business doing
hardware stuff, which includes runtime pm, system suspend and all that
nonsense. It' an interface between userspace and drivers, with a big
library to back it all up. Everything else just repeats the old midlayer
mistake.

If you driver needs this, do it there. Also, the component framework is
probably the solution you're looking for. And if there are synchronization
issues with that then we need to fix those instead of reinventing yet
another half-assed broken wheel.

Aside: With David Herrmann's latest patches to de-midlayer the drm
init/teardown sequence the driver is in full control of when the drm data
structures get allocate, initialized and registered. If you convert to
that plus the component framework I'm pretty sure your problem is solved.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Daniel Vetter]

On Fri, Oct 3, 2014 at 11:42 AM, Andrzej Hajda a.ha...@samsung.com wrote:
 On 10/03/2014 10:31 AM, Daniel Vetter wrote:
 On Fri, Oct 03, 2014 at 10:24:09AM +0200, Andrzej Hajda wrote:
 The main intent of this patchset is to allow use of suspend/resume drm 
 driver
 callbacks in KMS drivers, as these callbacks seems to me the best place
 to implement suspend/resume functionality in drm driver.
 Implementing this functionality in master component driver PM ops is 
 problematic
 as those callbacks can be called asynchronously regardless of 
 state/existence of
 drm device, thus it would require additional synchronization mechanism.

 Callbacks re-enabling requires small changes in i915 and exynos driver.
 The patchset contains also fix of exynos resume callback.
 Nack.

 Like completely and totally. The drm core has really no business doing
 hardware stuff, which includes runtime pm, system suspend and all that
 nonsense. It' an interface between userspace and drivers, with a big
 library to back it all up. Everything else just repeats the old midlayer
 mistake.

 Hmm, I have just tried to reuse the existing infrastructure, I did not see
 any sign do not touch, this is a mistake. Now I see it, thanks :)

As a rule of thumb, if you see a !DRIVER_MODESET check anywhere, then
that's a clear sign that you're wandering off the light and into the
dangerous parts of what drm was like age dark ages ;-)

 If you driver needs this, do it there. Also, the component framework is
 probably the solution you're looking for. And if there are synchronization
 issues with that then we need to fix those instead of reinventing yet
 another half-assed broken wheel.

 But this is an issue closely connected with component framework.
 Component framework separates master component probe and drm device
 initialization. As a result PM ops which are synchronized with probe
 (via device_lock)
 are no more synchronized with drm initialization which is usually called
 from
 .bind callback.

Now I'm confused. component-bind and drm initialization is about
driver load, but all this code here is about system suspend/resume
really. So I'm rather confused what problem you actually want to fix
..

 Aside: With David Herrmann's latest patches to de-midlayer the drm
 init/teardown sequence the driver is in full control of when the drm data
 structures get allocate, initialized and registered. If you convert to
 that plus the component framework I'm pretty sure your problem is solved.

 I will look closer at it but as I described above it is rather matter of
 separation
 of master component and drm device initialization.

 My idea was to avoid creation of new synchronization mechanism and to
 reuse the
 existing ones which seems to fit perfectly to the scenario, but if there
 is big NO for it
 another solution should be found.

 Anyway I guess the problem exists for all drivers having component
 framework and suspend:
 exynos, msm and incoming rockchip.

It sounds like the component framework needs to be teached to work
with system suspend/resume. I guess either we need to have clever
abuse of deferred probing to make sure that the master device is
probed first and so in the correct place in the system/resume
sequence. Or the master framework needs to grow pm_ops itself so that
the state associated with the logical componentized framework can be
saved/restored.

So master pm_ops would save/restore kms state, and all the components
would just save/restore any additional (register, clock, whatever)
state that each componnent driver would need.

But I'm not really an expert here, so adding more people.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: init vblank with real number of crtcs

[Daniel Vetter]

On Fri, Sep 19, 2014 at 02:57:20PM +0200, Andrzej Hajda wrote:
 Initialization of vblank with MAX_CRTC caused attempts
 to disabling vblanks for non-existing crtcs in case
 drm used fewer crtcs. The patch fixes it.
 
 Signed-off-by: Andrzej Hajda a.ha...@samsung.com
 ---
  drivers/gpu/drm/exynos/exynos_drm_drv.c | 18 +-
  1 file changed, 9 insertions(+), 9 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
 b/drivers/gpu/drm/exynos/exynos_drm_drv.c
 index 9b00e4e..dc4affd 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
 @@ -94,10 +94,6 @@ static int exynos_drm_load(struct drm_device *dev, 
 unsigned long flags)
   /* init kms poll for handling hpd */
   drm_kms_helper_poll_init(dev);
  
 - ret = drm_vblank_init(dev, MAX_CRTC);
 - if (ret)
 - goto err_mode_config_cleanup;
 -
   /* setup possible_clones. */
   exynos_drm_encoder_setup(dev);
  
 @@ -106,22 +102,26 @@ static int exynos_drm_load(struct drm_device *dev, 
 unsigned long flags)
   /* Try to bind all sub drivers. */
   ret = component_bind_all(dev-dev, dev);
   if (ret)
 - goto err_cleanup_vblank;
 + goto err_mode_config_cleanup;
 +
 + ret = drm_vblank_init(dev, dev-mode_config.num_crtc);

Hm, I wonder whether we should have a drm_mode_vblank_init which dtrt here
for kms drivers? Suggestions for a better name welcome ;-)
-Daniel

 + if (ret)
 + goto err_unbind_all;
  
   /* Probe non kms sub drivers and virtual display driver. */
   ret = exynos_drm_device_subdrv_probe(dev);
   if (ret)
 - goto err_unbind_all;
 + goto err_cleanup_vblank;
  
   /* force connectors detection */
   drm_helper_hpd_irq_event(dev);
  
   return 0;
  
 -err_unbind_all:
 - component_unbind_all(dev-dev, dev);
  err_cleanup_vblank:
   drm_vblank_cleanup(dev);
 +err_unbind_all:
 + component_unbind_all(dev-dev, dev);
  err_mode_config_cleanup:
   drm_mode_config_cleanup(dev);
   drm_release_iommu_mapping(dev);
 @@ -138,8 +138,8 @@ static int exynos_drm_unload(struct drm_device *dev)
   exynos_drm_fbdev_fini(dev);
   drm_kms_helper_poll_fini(dev);
  
 - component_unbind_all(dev-dev, dev);
   drm_vblank_cleanup(dev);
 + component_unbind_all(dev-dev, dev);
   drm_mode_config_cleanup(dev);
   drm_release_iommu_mapping(dev);
  
 -- 
 1.9.1
 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Vetter]

On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake dr...@endlessm.com wrote:
 However there is *another* fb reference taken in
 omap_plane_mode_set(). And my patch is modelled to do the same in
 exynos-drm.

This is because omapdrm does _everything_ asynchrously, even plain
modesets. Unfortunately that async modeset support is broken, so the
latest omapdrm patches insert a synchronization point.

So picking omap's mode_set logic as a reference because it also does
fb refcounting is not a good idea - that code does something crazy and
gets it wrong. And really, if you do modeset synchronously the drm
core will take care of your refcounting needs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Vetter]

On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake dr...@endlessm.com wrote:
 2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls
 drm_mode_set_config_internal() in order to turn off the CRTC, dropping
 another reference in the process.
 if (tmp-old_fb)
 drm_framebuffer_unreference(tmp-old_fb);

 3. drm_framebuffer_remove calls drm_plane_force_disable() which drops
 another reference:
 /* disconnect the plane from the fb and crtc: */
 __drm_framebuffer_unreference(old_fb);

If 3. here is about the primary plane then this won't happen, since
the primary plane pointerreference has already been cleared in step
2.

And even if their would be a bug in here, you _certainly_ should not
try to paper over this in your driver, but instead fix up the
refcounting done in the drm core.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Vetter]

On Wed, Sep 17, 2014 at 2:19 PM, Daniel Drake dr...@endlessm.com wrote:
 Chip specific drm driver internally doesn't have to care fb reference count 
 if
 there is no special case. We should have switched to universal plane at that
 time.

 To me it seems like the chip-specific DRM drivers do need to add a
 reference in the crtc_mode_set and crtc page flip paths otherwise
 framebuffer removal crashes (expecting to remove 3 references), as
 noted by my testing and also in commit 25c8b5c304.

I think fb refcounting in exynos is just plain busted. If you look at
other drivers the only place the refcount framebuffers or backing
storage objects is for pageflips to make sure the memory doesn't go
away while the hw is still scanning out the old framebuffer. If you
refcount anywhere else you either do something really crazy or your
driver is broken.

 However, I'll be happy if universal planes means the driver does not
 have to care about this any more. Andrej, please go ahead if you are
 interested, I'll be happy to test your results.

universal planes will fix up the mess with 2 drm plane objects
(primary plane + exonys internal primary). So should help to untangle
this not, but it will not magically fix the refcounting bugs itself.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] drm/exynos: use drm generic mmap interface

[Daniel Vetter]

On Wed, Sep 17, 2014 at 10:48:44PM +0900, Inki Dae wrote:
 This patch set removes unnecessary DRM_EXYNOS_GEM_MAP_OFFSET interface
 which isn't used anymore and also uses drm generic mmap interface
 instead of a mmap interface specific to Exynos drm. So this patch set
 removes a existing mmap interface and relevant codes specific to Exynos
 drm.
 
 Inki Dae (2):
   drm/exynos: remove DRM_EXYNOS_GEM_MAP_OFFSET ioctl
   drm/exynos: use drm generic mmap interface

Thanks for doing this. Acked-by: Daniel Vetter daniel.vet...@ffwll.ch
-Daniel

 
  drivers/gpu/drm/exynos/exynos_drm_drv.c |   28 -
  drivers/gpu/drm/exynos/exynos_drm_drv.h |1 -
  drivers/gpu/drm/exynos/exynos_drm_gem.c |  101 
 +--
  drivers/gpu/drm/exynos/exynos_drm_gem.h |   14 -
  include/uapi/drm/exynos_drm.h   |   40 
  5 files changed, 14 insertions(+), 170 deletions(-)
 
 -- 
 1.7.9.5
 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: fix plane-framebuffer linkage

[Daniel Vetter]

On Mon, Sep 15, 2014 at 12:52:17PM -0600, Daniel Drake wrote:
 Pageflipping currently causes some inconsistencies that lead to
 crashes. Just run an app that causes a CRTC pageflip in a raw X session
 and check that it exits cleanly and can be restarted - you'll see
 crashes like:
  Unable to handle kernel NULL pointer dereference at virtual address 0334
  PC is at exynos_drm_crtc_plane_commit+0x20/0x40
  LR is at exynos_drm_crtc_plane_commit+0x20/0x40
  [c03749b4] (exynos_drm_crtc_plane_commit) from [c03741bc] 
 (exynos_drm_crtc_commit+0x44/0x70)
  [c03741bc] (exynos_drm_crtc_commit) from [c03743a0] 
 (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4)
  [c03743a0] (exynos_drm_crtc_mode_set_commit.isra.2) from [c03744f4] 
 (exynos_drm_crtc_page_flip+0x140/0x1a8)
  [c03744f4] (exynos_drm_crtc_page_flip) from [c036b20c] 
 (drm_mode_page_flip_ioctl+0x224/0x2dc)
  [c036b20c] (drm_mode_page_flip_ioctl) from [c035c324] 
 (drm_ioctl+0x338/0x4fc)
 
 These crashes happen because drm_plane_force_disable has previously set
 plane-crtc to NULL.
 
 When drm_mode_page_flip_ioctl() is used to flip another framebuffer
 onto the primary plane, crtc-primary-fb is correctly updated (this is
 a virtual plane created by plane_helper), but plane-fb is not (this
 plane is the real one, created by exynos_drm_crtc_create).
 
 We then come to handle rmfb of the backbuffer, which the real primary
 plane is incorrectly pointing at. So drm_framebuffer_remove() decides that
 the buffer is actually active on a plane and force-disables the plane.
 
 Ensuring that plane-fb is kept up-to-date solves that issue, but
 exposes a reference counting problem. Now we see crashes when rmfb is
 called on the front-buffer, because the rmfb code expects to drop 3
 references here, and there are only 2.
 
 That can be fixed by adopting the reference management found in omapdrm:
 Framebuffer references are not taken directly in crtc mode_set context,
 but rather in the context of updating the plane, which also covers
 flips. Like omapdrm we also unreference the old framebuffer here.
 
 Signed-off-by: Daniel Drake dr...@endlessm.com

This sounds very much like exynos should switch to universal planes so
that the fake primary plane created by the helpers doesn't get in the way.
And for chips which already use planes for everything internally this
shouldn't be a lot more than a few lines.
-Daniel

 ---
  drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 12 ++--
  drivers/gpu/drm/exynos/exynos_drm_plane.c |  8 
  2 files changed, 10 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
 b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
 index b68e58f..7aa9dee 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
 @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct 
 drm_display_mode *mode,
   if (manager-ops-mode_set)
   manager-ops-mode_set(manager, crtc-mode);
  
 - ret = exynos_plane_mode_set(plane, crtc, crtc-primary-fb, 0, 0, 
 crtc_w, crtc_h,
 - x, y, crtc_w, crtc_h);
 - if (ret)
 - return ret;
 -
 - plane-crtc = crtc;
 - plane-fb = crtc-primary-fb;
 - drm_framebuffer_reference(plane-fb);
 -
 - return 0;
 + return exynos_plane_mode_set(plane, crtc, crtc-primary-fb, 0, 0,
 +  crtc_w, crtc_h, x, y, crtc_w, crtc_h);
  }
  
  static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int 
 y,
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c 
 b/drivers/gpu/drm/exynos/exynos_drm_plane.c
 index 8371cbd..df27e35 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
 @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, 
 struct drm_crtc *crtc,
   overlay-crtc_x, overlay-crtc_y,
   overlay-crtc_width, overlay-crtc_height);
  
 + if (plane-fb)
 + drm_framebuffer_unreference(plane-fb);
 +
 + drm_framebuffer_reference(fb);
 +
 + plane-fb = fb;
 + plane-crtc = crtc;
 +
   exynos_drm_crtc_plane_mode_set(crtc, overlay);
  
   return 0;
 -- 
 1.9.1
 
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Daniel Vetter]

On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
 Hi Andrzej,
 
 On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
  Adding reference to framebuffer should be accompanied with removing
  reference to old framebuffer assigned to the plane.
  This patch removes following warning:
  
  [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 
  drm_mode_config_cleanup+0x258/0x268()
  [   95.048086] Modules linked in:
  [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: GW  
  3.16.0-11355-g7a6eca5-dirty #3015
  [   95.060058] [c0013e90] (unwind_backtrace) from [c0011128] 
  (show_stack+0x10/0x14)
  [   95.067766] [c0011128] (show_stack) from [c04a5dc4] 
  (dump_stack+0x70/0xbc)
  [   95.074953] [c04a5dc4] (dump_stack) from [c0021784] 
  (warn_slowpath_common+0x64/0x88)
  [   95.083005] [c0021784] (warn_slowpath_common) from [c00217c4] 
  (warn_slowpath_null+0x1c/0x24)
  [   95.091780] [c00217c4] (warn_slowpath_null) from [c0275fa0] 
  (drm_mode_config_cleanup+0x258/0x268)
  [   95.100983] [c0275fa0] (drm_mode_config_cleanup) from [c0281724] 
  (exynos_drm_unload+0x38/0x50)
  [   95.109915] [c0281724] (exynos_drm_unload) from [c026e248] 
  (drm_dev_unregister+0x24/0x98)
  [   95.118414] [c026e248] (drm_dev_unregister) from [c026ecb0] 
  (drm_put_dev+0x28/0x64)
  [   95.126412] [c026ecb0] (drm_put_dev) from [c02947c4] 
  (take_down_master+0x24/0x44)
  [   95.134218] [c02947c4] (take_down_master) from [c0294870] 
  (component_del+0x8c/0xc8)
  [   95.142201] [c0294870] (component_del) from [c0286c10] 
  (exynos_dsi_remove+0x18/0x2c)
  [   95.150294] [c0286c10] (exynos_dsi_remove) from [c0299e04] 
  (platform_drv_remove+0x18/0x1c)
  [   95.158872] [c0299e04] (platform_drv_remove) from [c02987a4] 
  (__device_release_driver+0x70/0xc4)
  [   95.167981] [c02987a4] (__device_release_driver) from [c0298818] 
  (device_release_driver+0x20/0x2c)
  [   95.177268] [c0298818] (device_release_driver) from [c02979d0] 
  (unbind_store+0x5c/0x94)
  [   95.185597] [c02979d0] (unbind_store) from [c0297278] 
  (drv_attr_store+0x20/0x2c)
  [   95.193323] [c0297278] (drv_attr_store) from [c012aa90] 
  (sysfs_kf_write+0x4c/0x50)
  [   95.201224] [c012aa90] (sysfs_kf_write) from [c0129e94] 
  (kernfs_fop_write+0xc4/0x184)
  [   95.209393] [c0129e94] (kernfs_fop_write) from [c00d03ec] 
  (vfs_write+0xa0/0x1a8)
  [   95.217111] [c00d03ec] (vfs_write) from [c00d07f8] 
  (SyS_write+0x40/0x8c)
  [   95.224146] [c00d07f8] (SyS_write) from [c000e660] 
  (ret_fast_syscall+0x0/0x48)
  
  Signed-off-by: Andrzej Hajda a.ha...@samsung.com
  ---
   drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++
   1 file changed, 6 insertions(+)
  
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
  b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  index b68e58f..bde19f4 100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, 
  struct drm_display_mode *mode,
  if (ret)
  return ret;
   
  +   /* we need to unreference current fb after replacing it with new one */
  +   old_fb = plane-fb;
  +
  plane-crtc = crtc;
  plane-fb = crtc-primary-fb;
  drm_framebuffer_reference(plane-fb);
   
  +   if (old_fb)
  +   drm_framebuffer_unreference(old_fb);
 
 This time would be a good chance that we can consider drm flip queue to
 make sure that whole memory region to old_fb is scanned out completely
 before dropping a reference of old_fb. the reference of old_fb should be
 dropped at irq handler of each crtc devices, fimd and mixer.

Generally it's not a good idea to drop fb references from irq context,
since if you actually drop the last reference it'll blow up: fb cleanup
needs a bunch of mutexes.

Also the drm core really should be taking care of this for you, you only
need to grab references yourself for async flips if you want the buffer to
survive a bit. crtc_mode_set has not need for this. I expect that the
refcounting bug is somewhere else, at least from my experience chasing
such issues in i915 ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Daniel Vetter]

On Fri, Sep 12, 2014 at 11:27:41AM +0200, Andrzej Hajda wrote:
 On 09/12/2014 10:57 AM, Daniel Vetter wrote:
  On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
  Hi Andrzej,
 
  On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
  Adding reference to framebuffer should be accompanied with removing
  reference to old framebuffer assigned to the plane.
  This patch removes following warning:
 
  [   95.038017] WARNING: CPU: 1 PID: 3067 at 
  drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
  [   95.048086] Modules linked in:
  [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: GW  
  3.16.0-11355-g7a6eca5-dirty #3015
  [   95.060058] [c0013e90] (unwind_backtrace) from [c0011128] 
  (show_stack+0x10/0x14)
  [   95.067766] [c0011128] (show_stack) from [c04a5dc4] 
  (dump_stack+0x70/0xbc)
  [   95.074953] [c04a5dc4] (dump_stack) from [c0021784] 
  (warn_slowpath_common+0x64/0x88)
  [   95.083005] [c0021784] (warn_slowpath_common) from [c00217c4] 
  (warn_slowpath_null+0x1c/0x24)
  [   95.091780] [c00217c4] (warn_slowpath_null) from [c0275fa0] 
  (drm_mode_config_cleanup+0x258/0x268)
  [   95.100983] [c0275fa0] (drm_mode_config_cleanup) from [c0281724] 
  (exynos_drm_unload+0x38/0x50)
  [   95.109915] [c0281724] (exynos_drm_unload) from [c026e248] 
  (drm_dev_unregister+0x24/0x98)
  [   95.118414] [c026e248] (drm_dev_unregister) from [c026ecb0] 
  (drm_put_dev+0x28/0x64)
  [   95.126412] [c026ecb0] (drm_put_dev) from [c02947c4] 
  (take_down_master+0x24/0x44)
  [   95.134218] [c02947c4] (take_down_master) from [c0294870] 
  (component_del+0x8c/0xc8)
  [   95.142201] [c0294870] (component_del) from [c0286c10] 
  (exynos_dsi_remove+0x18/0x2c)
  [   95.150294] [c0286c10] (exynos_dsi_remove) from [c0299e04] 
  (platform_drv_remove+0x18/0x1c)
  [   95.158872] [c0299e04] (platform_drv_remove) from [c02987a4] 
  (__device_release_driver+0x70/0xc4)
  [   95.167981] [c02987a4] (__device_release_driver) from [c0298818] 
  (device_release_driver+0x20/0x2c)
  [   95.177268] [c0298818] (device_release_driver) from [c02979d0] 
  (unbind_store+0x5c/0x94)
  [   95.185597] [c02979d0] (unbind_store) from [c0297278] 
  (drv_attr_store+0x20/0x2c)
  [   95.193323] [c0297278] (drv_attr_store) from [c012aa90] 
  (sysfs_kf_write+0x4c/0x50)
  [   95.201224] [c012aa90] (sysfs_kf_write) from [c0129e94] 
  (kernfs_fop_write+0xc4/0x184)
  [   95.209393] [c0129e94] (kernfs_fop_write) from [c00d03ec] 
  (vfs_write+0xa0/0x1a8)
  [   95.217111] [c00d03ec] (vfs_write) from [c00d07f8] 
  (SyS_write+0x40/0x8c)
  [   95.224146] [c00d07f8] (SyS_write) from [c000e660] 
  (ret_fast_syscall+0x0/0x48)
 
  Signed-off-by: Andrzej Hajda a.ha...@samsung.com
  ---
   drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++
   1 file changed, 6 insertions(+)
 
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
  b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  index b68e58f..bde19f4 100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
  @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, 
  struct drm_display_mode *mode,
if (ret)
return ret;
   
  + /* we need to unreference current fb after replacing it with new one */
  + old_fb = plane-fb;
  +
plane-crtc = crtc;
plane-fb = crtc-primary-fb;
drm_framebuffer_reference(plane-fb);
   
  + if (old_fb)
  + drm_framebuffer_unreference(old_fb);
  This time would be a good chance that we can consider drm flip queue to
  make sure that whole memory region to old_fb is scanned out completely
  before dropping a reference of old_fb. the reference of old_fb should be
  dropped at irq handler of each crtc devices, fimd and mixer.
  Generally it's not a good idea to drop fb references from irq context,
  since if you actually drop the last reference it'll blow up: fb cleanup
  needs a bunch of mutexes.
 
 I agree with that.
 
 
  Also the drm core really should be taking care of this for you, you only
  need to grab references yourself for async flips if you want the buffer to
  survive a bit. crtc_mode_set has not need for this. I expect that the
  refcounting bug is somewhere else, at least from my experience chasing
  such issues in i915 ;-)
 
 Hmm, maybe I miss something but I do not see the core grabbing fb reference
 on plane-fb update. On the other side drm_framebuffer_remove calls
 drm_plane_force_disable which drops plane-fb reference.
 I am not yet familiar with this code so maybe there is better solution.

It does, see e.g. drm_mode_set_config_internal. All the other places also
do it.

 If not I guess it would be better to move this code to
 exynos_plane_mode_set.
 At least it is done this way in omap and msm, in fact it seems better place
 for such things. What do you think?

Drivers _only_ need to do their own refcounting if they do asynchronous
updates (pageflips or asynchronous modesets). Which omap does (and iirc
msm for pageflips). But if that need is common we

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Daniel Vetter]

On Thu, May 15, 2014 at 12:32:58PM +0200, Thierry Reding wrote:
 On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
  On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.ha...@samsung.com wrote:
 [...]
   4. And the last thing, it is more about the concept/design. drm_bridge,
   drm_hw_block suggests that those interfaces describes the whole device:
   bridge, panel, whatever.
  
  hmm, I don't think this is the case.  I can easily see things like:
  
struct foo_panel {
  struct drm_panel base;
  struct drm_bridge bridge;
  ...
}
  
  where a panel implementation implements both panel and bridge.  In
  fact that is kinda what I was encouraging.
 
 For lack of a better entry point into the discussion, let me pick this
 example. It seems like in general we all agree that the basic structure
 would have to be something like this:
 
   CRTC - encoder - bridge [ - bridge ... ] - panel
 
 Where panel could optionally be a bridge/panel hybrid as Rob pointed
 out. I'm not sure if there's panels like this, but I suspect the use
 case would be where a panel had built-in controls to modify the image in
 some fashion (brightness, saturation, ...). I think those things exist
 in DCS for example.
 
 What's missing here, and if I understand correctly what Sean said about
 the connector type, what we need to solve is how to wire up the
 connector so that it reflects the correct type. So the above would have
 to become something like this:
 
   CRTC - encoder - bridge [ - bridge ... ] - panel
   connector -^

This is kinda always how I've thought it should play out: The last item in
the chain actually implements the drm connector, everyone else just
ignores it.

 One of the problems with that approach is that panel is more than just a
 video sink. It also controls the panel (and optionally backlight) power.
 The standard DRM connector helpers don't work well in that case, because
 drm_helper_connector_dpms() forwards changes to the CRTC and encoder,
 though that could possibly be solved by wrapping it in a small custom
 implementation that also controls the panel. Anyway, that's really just
 an implementation detail.

I don't see an issue with the dpms really. At worst the overall kms driver
(which provides the crtc and encoder implementation) needs to pass a
suitable dpms implementation for the drm_bridge to use in the connector.
Or we could just move this vfunc into the core kms funtion table since
everyone uses the same (well except i915, but that's a different story).

dpms changes would then still be driven through the crtc - encoder -
bridge ... chain.

 So once a complete chain from encoder to panel has been created, we need
 a way to find the appropriate connector type. Perhaps to achieve that it
 would be useful to instantiate the connector by the bridge that's
 connected to the panel. So essentially something like this:
 
   struct drm_bridge {
   /* to allow chaining */
   struct drm_bridge *next;
   /* for bridges directly connected to a panel or monitor */
   struct drm_connector *connector;
   /* for bridges directly connected to a panel */
   struct drm_panel *panel;

Imo these two shouldn't be here, but instead should be embedded into the
overall structure the drm_bridge driver is using.
 
   ...
   };
 
 To make this work in a generic way, I think we're missing one bridge
 instance. Currently the bridge in an encoder is completely optional, so
 if there is no bridge in the system this needs to be special cased. One
 way to unify this would be to make drm_encoder implement drm_bridge, or
 an alternative would be to make drm_panel implement a bridge. Both can
 possibly be dummies stubbed out with simple helpers.

Yeah, imo if the panel isn't just a dumb thing but needs to actively take
part in the modeset dance, it simply needs to implement itself as a
drm_bridge+panel combo and do the magic in the various hooks provided.

 I wonder if this would have any consequences on Dave's DP MST work,
 since presumably an MST hub could be considered a bridge. In that case I
 guess there would need to be support for multiple next bridges,
 perhaps a simple doubly-linked list instead of a next pointer. The first
 bridge would then be used to model the hub's input and child bridges
 would be used to model each
 of the outputs.

DP is standardize. No need to wrestle the complexity through a drm_bridge,
since code reuse anywhere else (non-DP) will be know to be zero. And for
all the guys implementing a DP output can simply use the helpers. So I
don't see an upside of this idea.

Imo we should restrict drm_bridge to all the crazy transcoder chips/blocks
commonly found on SoCs which all do non-standard stuff and where we
actually have an established or anticipated need for sharing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48

Re: [PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls

[Daniel Vetter]

On Fri, Apr 25, 2014 at 8:17 AM, Ajay kumar ajayn...@gmail.com wrote:
 We can call panel_enable/disable at the right point. Even the bridge chips
 expect the same. So, I am not ok with combining the bridge and panel and
 calling the fxn pointers from crtc_helpers.
 So, either we leave it the way it is in this patch (call drm_panel
 functions at right points) or
 we don't use drm_panel to control the panel sequence and instead use few DT
 properties and regulators, inside the bridge chip driver.

That wasn't really suggested, but instead the idea was to provide a
default drm_bridge which wraps the drm_panel so that you could more
easily chain them up.

Also I'm not really happy that the bridge callbacks have been added to
the drm helpers (I'd prefer if driver callbacks would bear that
responsibility). But you can always create your own drm_bridge
integration. In any case your concern that drivers need to control
when certain callbacks are called is shared by everyone here afaics.
And I also don't see any issues with Rob's proposal in this regard.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/9] drm/panel: add pre_enable and post_disable routines

[Daniel Vetter]

On Tue, Apr 22, 2014 at 08:06:19PM +0530, Ajay kumar wrote:
 Hi Thierry,
 
 
 On Tue, Apr 22, 2014 at 1:49 PM, Thierry Reding thierry.red...@gmail.com
 wrote:
  On Tue, Apr 22, 2014 at 04:09:11AM +0530, Ajay Kumar wrote:
  Most of the panels need an init sequence as mentioned below:
-- poweron LCD unit/LCD_EN
-- start video data
-- poweron LED unit/BL_EN
  And, a de-init sequence as mentioned below:
-- poweroff LED unit/BL_EN
-- stop video data
-- poweroff LCD unit/LCD_EN
  With existing callbacks for drm panel, we cannot accomodate such panels,
  since only two callbacks, i.e panel_enable and panel_disable are
 supported.
 
  This patch adds:
-- pre_enable callback which can be called before
the actual video data is on, and then call the enable
callback after the video data is available.
 
-- post_disable callback which can be called after
the video data is off, and use disable callback
to do something before switching off the video data.
 
  Now, we can easily map the above scenario as shown below:
poweron LCD unit/LCD_EN = pre_enable callback
poweron LED unit/BL_EN = enable callback
poweroff LED unit/BL_EN = disable callback
poweroff LCD unit/LCD_EN = post_disable callback
 
  I don't like this. What happens when the next panel comes around that
  has a yet slightly different requirement? Will we introduce a new
  pre_pre_enable() and post_post_disable() function then?
 
 As I have already explained, these 2 callbacks are sufficient enough to
 take care
 the power up/down sequence for LVDS and eDP panels. And, definitely having
 just 2 callbacks enable and disable is not at all sufficient.
 
 I initially thought of using panel_simple_enable from panel-simple driver.
 But it doesn't go well with case wherein there are 2 regulators(one for LCD
 and one for LED)
 a BL_EN signal etc. And, often(Believe me, I have referred to both eDP
 panel datasheets
 and LVDS panel datasheets) proper powerup sequence for such panels is as
 mentioned below:
 
 powerup:
 -- power up the supply (LCD_VCC).
 -- start video data.
 -- enable backlight.
 
 powerdown
 -- disable backlight.
 -- stop video data.
 -- power off the supply (LCD VCC)
 
 For the above cases, if I have to somehow fit in all the required settings,
 it breaks the sequence and I end up getting glitches during bootup/DPMS.
 
 Also, when the drm_bridge can support pre_enable and post_disable, why not
 drm_panel provide that both should work in tandem?

Imo this makes sense, especially if we go through with the idea talked
about yesterday of creating a drm_bridge to wrap-up a drm_panel with
sufficient isolation between all components.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls

[Daniel Vetter]

) {
  +   ptn_bridge-panel = panel;
  +   drm_panel_attach(ptn_bridge-panel, ptn_bridge-connector);
  +   }
  +
  bridge-driver_private = ptn_bridge;
  encoder-bridge = bridge;
  ptn_bridge-connector.polled = DRM_CONNECTOR_POLL_HPD;
  diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c 
  b/drivers/gpu/drm/exynos/exynos_dp_core.c
  index dbc5ccc..4853f31 100644
  --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
  +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
  @@ -989,13 +989,14 @@ static bool find_bridge(const char *compat, struct 
  bridge_init *bridge)
 
   /* returns the number of bridges attached */
   static int exynos_drm_attach_lcd_bridge(struct drm_device *dev,
  -   struct drm_encoder *encoder)
  +   struct drm_encoder *encoder, struct drm_panel *panel)
   {
  struct bridge_init bridge;
  int ret;
 
  if (find_bridge(nxp,ptn3460, bridge)) {
  -   ret = ptn3460_init(dev, encoder, bridge.client, 
  bridge.node);
  +   ret = ptn3460_init(dev, encoder, bridge.client, bridge.node,
  +   panel);
  if (!ret)
  return 1;
  }
  @@ -1012,9 +1013,16 @@ static int exynos_dp_create_connector(struct 
  exynos_drm_display *display,
  dp-encoder = encoder;
 
  /* Pre-empt DP connector creation if there's a bridge */
  -   ret = exynos_drm_attach_lcd_bridge(dp-drm_dev, encoder);
  -   if (ret)
  +   ret = exynos_drm_attach_lcd_bridge(dp-drm_dev, encoder, 
  dp-drm_panel);
  +   if (ret) {
  +   /*
  +* Also set dp-drm_panel = NULL so that we don't end up
  +* controlling panel power both in exynos_dp and bridge
  +* DPMS routines.
  +*/
  +   dp-drm_panel = NULL;
  return 0;
  +   }
 
  connector-polled = DRM_CONNECTOR_POLL_HPD;
 
  diff --git a/include/drm/bridge/ptn3460.h b/include/drm/bridge/ptn3460.h
  index ff62344..570cebb 100644
  --- a/include/drm/bridge/ptn3460.h
  +++ b/include/drm/bridge/ptn3460.h
  @@ -18,16 +18,18 @@ struct drm_device;
   struct drm_encoder;
   struct i2c_client;
   struct device_node;
  +struct drm_panel;
 
   #if defined(CONFIG_DRM_PTN3460) || defined(CONFIG_DRM_PTN3460_MODULE)
 
   int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
  -   struct i2c_client *client, struct device_node *node);
  +   struct i2c_client *client, struct device_node *node,
  +   struct drm_panel *panel);
   #else
 
   static inline int ptn3460_init(struct drm_device *dev,
  struct drm_encoder *encoder, struct i2c_client *client,
  -   struct device_node *node)
  +   struct device_node *node, struct drm_panel *panel)
   {
  return 0;
   }
  --
  1.7.9.5
 
  ___
  dri-devel mailing list
  dri-de...@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/dri-devel
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] drm: Constify struct drm_fb_helper_funcs

[Daniel Vetter]

On Tue, Apr 22, 2014 at 04:42:19PM +0200, Thierry Reding wrote:
 From: Thierry Reding tred...@nvidia.com
 
 There's no need for this to be modifiable. Make it const so that it can
 be put into the .rodata section.
 
 Signed-off-by: Thierry Reding tred...@nvidia.com

Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch


 ---
  drivers/gpu/drm/armada/armada_fbdev.c | 2 +-
  drivers/gpu/drm/ast/ast_fb.c  | 2 +-
  drivers/gpu/drm/bochs/bochs_fbdev.c   | 2 +-
  drivers/gpu/drm/cirrus/cirrus_fbdev.c | 2 +-
  drivers/gpu/drm/drm_fb_cma_helper.c   | 2 +-
  drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 2 +-
  drivers/gpu/drm/gma500/framebuffer.c  | 2 +-
  drivers/gpu/drm/i915/intel_fbdev.c| 2 +-
  drivers/gpu/drm/mgag200/mgag200_fb.c  | 2 +-
  drivers/gpu/drm/msm/msm_fbdev.c   | 2 +-
  drivers/gpu/drm/nouveau/nouveau_fbcon.c   | 2 +-
  drivers/gpu/drm/omapdrm/omap_fbdev.c  | 2 +-
  drivers/gpu/drm/qxl/qxl_fb.c  | 2 +-
  drivers/gpu/drm/radeon/radeon_fb.c| 2 +-
  drivers/gpu/drm/tegra/fb.c| 2 +-
  drivers/gpu/drm/udl/udl_fb.c  | 2 +-
  include/drm/drm_fb_helper.h   | 2 +-
  17 files changed, 17 insertions(+), 17 deletions(-)
 
 diff --git a/drivers/gpu/drm/armada/armada_fbdev.c 
 b/drivers/gpu/drm/armada/armada_fbdev.c
 index 948cb14c561e..21aa1261dba2 100644
 --- a/drivers/gpu/drm/armada/armada_fbdev.c
 +++ b/drivers/gpu/drm/armada/armada_fbdev.c
 @@ -131,7 +131,7 @@ static int armada_fb_probe(struct drm_fb_helper *fbh,
   return ret;
  }
  
 -static struct drm_fb_helper_funcs armada_fb_helper_funcs = {
 +static const struct drm_fb_helper_funcs armada_fb_helper_funcs = {
   .gamma_set  = armada_drm_crtc_gamma_set,
   .gamma_get  = armada_drm_crtc_gamma_get,
   .fb_probe   = armada_fb_probe,
 diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
 index a28640f47c27..2113894e4ff8 100644
 --- a/drivers/gpu/drm/ast/ast_fb.c
 +++ b/drivers/gpu/drm/ast/ast_fb.c
 @@ -287,7 +287,7 @@ static void ast_fb_gamma_get(struct drm_crtc *crtc, u16 
 *red, u16 *green,
   *blue = ast_crtc-lut_b[regno]  8;
  }
  
 -static struct drm_fb_helper_funcs ast_fb_helper_funcs = {
 +static const struct drm_fb_helper_funcs ast_fb_helper_funcs = {
   .gamma_set = ast_fb_gamma_set,
   .gamma_get = ast_fb_gamma_get,
   .fb_probe = astfb_create,
 diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c 
 b/drivers/gpu/drm/bochs/bochs_fbdev.c
 index 561b84474122..17e5c17f2730 100644
 --- a/drivers/gpu/drm/bochs/bochs_fbdev.c
 +++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
 @@ -179,7 +179,7 @@ void bochs_fb_gamma_get(struct drm_crtc *crtc, u16 *red, 
 u16 *green,
   *blue  = regno;
  }
  
 -static struct drm_fb_helper_funcs bochs_fb_helper_funcs = {
 +static const struct drm_fb_helper_funcs bochs_fb_helper_funcs = {
   .gamma_set = bochs_fb_gamma_set,
   .gamma_get = bochs_fb_gamma_get,
   .fb_probe = bochsfb_create,
 diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c 
 b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
 index 32bbba0a787b..2bd0291168e4 100644
 --- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
 +++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
 @@ -288,7 +288,7 @@ static int cirrus_fbdev_destroy(struct drm_device *dev,
   return 0;
  }
  
 -static struct drm_fb_helper_funcs cirrus_fb_helper_funcs = {
 +static const struct drm_fb_helper_funcs cirrus_fb_helper_funcs = {
   .gamma_set = cirrus_crtc_fb_gamma_set,
   .gamma_get = cirrus_crtc_fb_gamma_get,
   .fb_probe = cirrusfb_create,
 diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c 
 b/drivers/gpu/drm/drm_fb_cma_helper.c
 index 61b5a47ad239..b74f9e58b69d 100644
 --- a/drivers/gpu/drm/drm_fb_cma_helper.c
 +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
 @@ -327,7 +327,7 @@ err_drm_gem_cma_free_object:
   return ret;
  }
  
 -static struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
 +static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
   .fb_probe = drm_fbdev_cma_create,
  };
  
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c 
 b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
 index addbf7536da4..7ccf04917f47 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
 @@ -233,7 +233,7 @@ out:
   return ret;
  }
  
 -static struct drm_fb_helper_funcs exynos_drm_fb_helper_funcs = {
 +static const struct drm_fb_helper_funcs exynos_drm_fb_helper_funcs = {
   .fb_probe = exynos_drm_fbdev_create,
  };
  
 diff --git a/drivers/gpu/drm/gma500/framebuffer.c 
 b/drivers/gpu/drm/gma500/framebuffer.c
 index e7fcc148f333..76e4d777d01d 100644
 --- a/drivers/gpu/drm/gma500/framebuffer.c
 +++ b/drivers/gpu/drm/gma500/framebuffer.c
 @@ -561,7 +561,7 @@ static int psbfb_probe(struct drm_fb_helper *helper,
   return psbfb_create(psb_fbdev, sizes);
  }
  
 -static struct drm_fb_helper_funcs psb_fb_helper_funcs = {
 +static const struct

Re: [PATCH 3/4] drm: Introduce drm_fb_helper_prepare()

[Daniel Vetter]

On Tue, Apr 22, 2014 at 04:42:20PM +0200, Thierry Reding wrote:
 From: Thierry Reding tred...@nvidia.com
 
 To implement hotplug detection in a race-free manner, drivers must call
 drm_kms_helper_poll_init() before hotplug events can be triggered. Such
 events can be triggered right after any of the encoders or connectors
 are initialized. At the same time, if the drm_fb_helper_hotplug_event()
 helper is used by a driver, then the poll helper requires some parts of
 the FB helper to be initialized to prevent a crash.
 
 At the same time, drm_fb_helper_init() requires information that is not
 necessarily available at such an early stage (number of CRTCs and
 connectors), so it cannot be used yet.
 
 Add a new helper, drm_fb_helper_prepare(), that initializes the bare
 minimum needed to allow drm_kms_helper_poll_init() to execute and any
 subsequent hotplug events to be processed properly.
 
 Signed-off-by: Thierry Reding tred...@nvidia.com

Some bikeshed on the kerneldoc below, with that addressed this is

Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch

 ---
  drivers/gpu/drm/armada/armada_fbdev.c |  2 +-
  drivers/gpu/drm/ast/ast_fb.c  |  4 ++-
  drivers/gpu/drm/bochs/bochs_fbdev.c   |  3 ++-
  drivers/gpu/drm/cirrus/cirrus_fbdev.c |  4 ++-
  drivers/gpu/drm/drm_fb_cma_helper.c   |  3 ++-
  drivers/gpu/drm/drm_fb_helper.c   | 43 
 ---
  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |  3 ++-
  drivers/gpu/drm/gma500/framebuffer.c  |  3 ++-
  drivers/gpu/drm/i915/intel_fbdev.c|  3 ++-
  drivers/gpu/drm/mgag200/mgag200_fb.c  |  3 ++-
  drivers/gpu/drm/msm/msm_fbdev.c   |  2 +-
  drivers/gpu/drm/nouveau/nouveau_fbcon.c   |  3 ++-
  drivers/gpu/drm/omapdrm/omap_fbdev.c  |  2 +-
  drivers/gpu/drm/qxl/qxl_fb.c  |  5 +++-
  drivers/gpu/drm/radeon/radeon_fb.c|  4 ++-
  drivers/gpu/drm/tegra/fb.c|  4 +--
  drivers/gpu/drm/udl/udl_fb.c  |  3 ++-
  include/drm/drm_fb_helper.h   |  2 ++
  18 files changed, 68 insertions(+), 28 deletions(-)
 
 diff --git a/drivers/gpu/drm/armada/armada_fbdev.c 
 b/drivers/gpu/drm/armada/armada_fbdev.c
 index 21aa1261dba2..9437e11d5df1 100644
 --- a/drivers/gpu/drm/armada/armada_fbdev.c
 +++ b/drivers/gpu/drm/armada/armada_fbdev.c
 @@ -149,7 +149,7 @@ int armada_fbdev_init(struct drm_device *dev)
  
   priv-fbdev = fbh;
  
 - fbh-funcs = armada_fb_helper_funcs;
 + drm_fb_helper_prepare(dev, fbh, armada_fb_helper_funcs);
  
   ret = drm_fb_helper_init(dev, fbh, 1, 1);
   if (ret) {
 diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
 index 2113894e4ff8..cba45c774552 100644
 --- a/drivers/gpu/drm/ast/ast_fb.c
 +++ b/drivers/gpu/drm/ast/ast_fb.c
 @@ -328,8 +328,10 @@ int ast_fbdev_init(struct drm_device *dev)
   return -ENOMEM;
  
   ast-fbdev = afbdev;
 - afbdev-helper.funcs = ast_fb_helper_funcs;
   spin_lock_init(afbdev-dirty_lock);
 +
 + drm_fb_helper_prepare(dev, afbdev-helper, ast_fb_helper_funcs);
 +
   ret = drm_fb_helper_init(dev, afbdev-helper,
1, 1);
   if (ret) {
 diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c 
 b/drivers/gpu/drm/bochs/bochs_fbdev.c
 index 17e5c17f2730..19cf3e9413b6 100644
 --- a/drivers/gpu/drm/bochs/bochs_fbdev.c
 +++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
 @@ -189,7 +189,8 @@ int bochs_fbdev_init(struct bochs_device *bochs)
  {
   int ret;
  
 - bochs-fb.helper.funcs = bochs_fb_helper_funcs;
 + drm_fb_helper_prepare(bochs-dev, bochs-fb.helper,
 +   bochs_fb_helper_funcs);
  
   ret = drm_fb_helper_init(bochs-dev, bochs-fb.helper,
1, 1);
 diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c 
 b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
 index 2bd0291168e4..2a135f253e29 100644
 --- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
 +++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
 @@ -306,9 +306,11 @@ int cirrus_fbdev_init(struct cirrus_device *cdev)
   return -ENOMEM;
  
   cdev-mode_info.gfbdev = gfbdev;
 - gfbdev-helper.funcs = cirrus_fb_helper_funcs;
   spin_lock_init(gfbdev-dirty_lock);
  
 + drm_fb_helper_prepare(cdev-dev, gfbdev-helper,
 +   cirrus_fb_helper_funcs);
 +
   ret = drm_fb_helper_init(cdev-dev, gfbdev-helper,
cdev-num_crtc, CIRRUSFB_CONN_LIMIT);
   if (ret) {
 diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c 
 b/drivers/gpu/drm/drm_fb_cma_helper.c
 index b74f9e58b69d..acbbd230e081 100644
 --- a/drivers/gpu/drm/drm_fb_cma_helper.c
 +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
 @@ -354,9 +354,10 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct 
 drm_device *dev,
   return ERR_PTR(-ENOMEM);
   }
  
 - fbdev_cma-fb_helper.funcs = drm_fb_cma_helper_funcs;
   helper = fbdev_cma-fb_helper;
  
 + drm_fb_helper_prepare(dev

Re: [RFC 3/4] drm: add generic blob properties for image enhancement

[Daniel Vetter]

On Mon, Mar 10, 2014 at 09:50:14AM +0530, Rahul Sharma wrote:
 On 8 March 2014 06:16, Matt Roper matthew.d.ro...@intel.com wrote:
  On Fri, Mar 07, 2014 at 03:50:54PM +0530, Rahul Sharma wrote:
  Hi Daniel,
 
  On 7 March 2014 14:06, Daniel Vetter dan...@ffwll.ch wrote:
   On Thu, Mar 06, 2014 at 11:42:13AM +0530, Rahul Sharma wrote:
   Add generic KMS blob properties to core drm framework. These
   are writable blob properties which can be used to set Image
   Enhancement parameters. The properties which are added here
   are meant for color reproduction, color saturation and edge
   enhancement.
  
   Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
  ...
   Tbh I don't really understand why we can't use normal properties for 
   this.
   We might want to add a new RBG type of property (or YUV fwiw, both with
   and without alpha) to make this standardized (maybe with some fixed point
   value for non-normalizes).
 
  I dropped Normal properties (the ones which accepts 64 bit data)
  because there will be too many of them. As you can see the
  count is going upto 24 parameters, means 24 such properties, as
  each can carry one parameter. This is too much of overhead.
  Please correct me if you mean something different.
 
  I am not sure what are RGB type properties? I know only about 64-bit
  normal properties which are too compact to carry above structures. Are
  you talking about set_gamma type of ioctls. Each set_gamma type
  ioctl needs a addition in callbacks till down the layer which looks bit
  unnecessary, while writable blob properties are using same set_property
  and get_property infrastructure.
 
  
   If the only reason you group them is to atomically commit them, then the
   only thing we need is the atomic ioctl, which can shovel entire groups of
   properties over the wire at once.
 
  Atomic commit is not an explicit requirement. But splitting them to
  many small pieces (in case of normal properties) are resulting into
  many user-kernel switching overhead, which seems avoidable.
 
  The idea with atomic modeset / nuclear pageflip is that you collect a
  whole bunch of individual property updates into a property set
  container in userspace (libdrm).  When you're done setting all the
  values you want and commit the property set, all of the values that
  you collected get passed to the kernel in one go via a new ioctl (and
  are then updated in an atomic manner by the DRM driver).  So performing
  your 24 different property updates via the upcoming atomic API's
  shouldn't be a problem and shouldn't add any unnecessary overhead.
 
  The kernel-side of atomic modeset / nuclear pageflip is still a WIP, so
  it isn't upstream yet, but you can see the userspace-facing API in Rob
  Clark's repo here:
 
  https://github.com/robclark/libdrm/blob/atomic/xf86drmMode.h
 
  Note the drmModePropertySet{Alloc,Add,Commit,Free}() functions near the
  bottom.
 
 
 Thanks Matt,
 
 I went through the share link. Got the idea of atomic
 modeset. It is a good solution for setting 24 properties
 in one go. But another issue is still unaddressed. I
 mean still need to declare 24 properties which are not
 Really 24 different properties.
 
 These are sub-parameters to 3 properties (color
 reproduction, color saturation and edge enhancement).
 Splitting them to 24 pieces, just because we don't have a
 a provision in KMS, is a work around to get things
 working. And, properties like saturation_hue_gain_red,
 saturation_hue_gain_green ... will be undoubtedly ugly.
 
 In Rob Clark's tree, I noticed
 
 extern int drmModePropertySetAddBlob(drmModePropertySetPtr set,
 uint32_t object_id,
 uint32_t property_id,
 uint64_t length,
 void *blob);
 
 Seems, already some implementation of writable blobs is
 there. I am not able to see the kernel though.
 
 I request experts, to review this solution again. I found it
 quite useful and hold good with the idea of Atomic modeset.

Like I've said I think creating a standard property for RBG and RBGA
values makes sense, to group this stuff a bit. But I don't think we need
to group things further.

Some clear definitions of these properties is needed though - Sagar from
our display group is working on that a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 3/4] drm: add generic blob properties for image enhancement

[Daniel Vetter]

 {
   /* Optional properties */
   struct drm_property *scaling_mode_property;
   struct drm_property *dirty_info_property;
 + struct drm_property *color_saturation_property;
 + struct drm_property *color_reproduction_property;
 + struct drm_property *edge_enhancement_property;
 +
 + struct drm_property_blob *color_saturation_blob_ptr;
 + struct drm_property_blob *color_reproduction_blob_ptr;
 + struct drm_property_blob *edge_enhancement_blob_ptr;
  
   /* dumb ioctl parameters */
   uint32_t preferred_depth, prefer_shadow;
 @@ -1079,6 +1086,12 @@ extern int drm_mode_create_tv_properties(struct 
 drm_device *dev, int num_formats
  extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
  extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
  extern const char *drm_get_encoder_name(const struct drm_encoder *encoder);
 +extern int drm_mode_create_color_saturation_property(
 + struct drm_device *dev);
 +extern int drm_mode_create_color_reproduction_property(
 + struct drm_device *dev);
 +extern int drm_mode_create_edge_enhancement_property(
 + struct drm_device *dev);
  
  extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
struct drm_encoder *encoder);
 diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
 index 1d8216d..d28f82d 100644
 --- a/include/uapi/drm/drm_mode.h
 +++ b/include/uapi/drm/drm_mode.h
 @@ -504,4 +504,45 @@ struct drm_mode_destroy_dumb {
   uint32_t handle;
  };
  
 +/* set up parameters for finer color saturation */
 +struct drm_mode_color_saturation {
 + /* hue gain for individual colors */
 + uint16_t hue_gain_red;
 + uint16_t hue_gain_green;
 + uint16_t hue_gain_blue;
 + uint16_t hue_gain_cyan;
 + uint16_t hue_gain_magenta;
 + uint16_t hue_gain_yellow;
 + /* hue gain for overall display */
 + uint16_t hue_gain_overall;
 +};
 +
 +/* set up parameters for standard color reproduction */
 +struct drm_mode_color_reproduction {
 + /* 16 bit rgb value for primary colors */
 + uint16_t red_rgb[3];
 + uint16_t green_rgb[3];
 + uint16_t blue_rgb[3];
 + uint16_t cyan_rgb[3];
 + uint16_t magenta_rgb[3];
 + uint16_t yellow_rgb[3];
 + uint16_t white_rgb[3];
 + uint16_t black_rgb[3];
 +};
 +
 +/* set up parameters for edge enhancement */
 +struct drm_mode_edge_enhancement {
 + /* threshold values for edge and background*/
 + uint16_t edge_th;
 + uint16_t background_th;
 + /* postive gain */
 + uint16_t pg_edge;
 + uint16_t pg_flat;
 + uint16_t pg_background;
 + /* negative gain */
 + uint16_t ng_edge;
 + uint16_t ng_flat;
 + uint16_t ng_background;
 +};

Tbh I don't really understand why we can't use normal properties for this.
We might want to add a new RBG type of property (or YUV fwiw, both with
and without alpha) to make this standardized (maybe with some fixed point
value for non-normalizes).

If the only reason you group them is to atomically commit them, then the
only thing we need is the atomic ioctl, which can shovel entire groups of
properties over the wire at once.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: drm_do_probe_ddc_edid ENXIO check too aggressive?

[Daniel Vetter]

On Tue, Dec 17, 2013 at 09:12:42AM -0600, Daniel Drake wrote:
 On Mon, Dec 16, 2013 at 5:40 PM, Daniel Vetter dan...@ffwll.ch wrote:
  Have a bit of logic in the exynos -detect function to re-try a 2nd
  round of edid probing after each hdp interrupt if the first one
  returns an -ENXIO. Only tricky part is to be careful with edge
  detection so that userspace gets all the hotplug events still.
  Presuming you don't have any funkiness with reprobing causing yet
  another hpd interrupt and stuff like that (seen it all), as long as
  you're using the helpers in drm_crtc_helper.c it should all be working
  correctly. So you want a work item which just grabs all modeset locks
  and then calls drm_helper_probe_single_connector_modes or something on
  the right connector.
 
 Thanks for the tips. Having trouble sticking to those details though.
 exynos_drm_connector_detect() is actually a long way away from EDID
 probing so it is hard to detect the ENXIO case there.

Yeah, was a bit hand-wavy ;-)

 What happens here is:
 exynos hdmi_irq_thread() calls drm_helper_hpd_irq_event()
 That then calls exynos_drm_connector_detect(), which returns a simple
 yes, hpd detection says we are connected
 Then it calls drm_kms_helper_hotplug_event() for which the call chain is:
 
 drm_kms_helper_hotplug_event
 exynos_drm_output_poll_changed
 drm_fb_helper_hotplug_event
 drm_fb_helper_probe_connector_mode
 exynos_drm_connector_fill_modes
 drm_helper_probe_single_connector_modes
 exynos_drm_connector_get_modes
 drm_get_edid
 drm_do_get_edid
 drm_do_probe_ddc_edid -- ENXIO in here
 
 drm_do_probe_ddc_edid actually swallows the ENXIO code and just
 returns -1, so that particular error is indistinguishable from others.
 
 Trying to follow your suggestions, the closest would seem to be something 
 like:
 
 1. Modify exynos_drm_connector_detect to read and cache the EDID right
 away. If EDID read fails for any reason, report no connection and
 schedule a work item to retry the EDID read. If the later EDID read
 succeeds, call drm_kms_helper_hotplug_event()
 
 2. Modify exynos_drm_connector_get_modes to return cached EDID

I think we can do it simpler. When you get a hpd interrupt you eventually
call drm_helper_hpd_irq_event which will update all the state and poke
connectors. I'd just create a delay_work which you launch from
hdmi_irq_thread with a 1 second delay which again calls
drm_helper_hpd_irq_event. That way you won't miss the EDID (presuming the
user isn't _really_ careful with pluggin in the cable slowly).

It's a bit inefficient but also doesn't really matter since hpd don't
happen often. And when they do userspace usually wants to do a modeset
anyway, so an added 30ms to read the edid twice won't really hurt.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: drm_do_probe_ddc_edid ENXIO check too aggressive?

[Daniel Vetter]

On Wed, Dec 18, 2013 at 10:28:37AM -0600, Daniel Drake wrote:
 On Wed, Dec 18, 2013 at 2:43 AM, Daniel Vetter dan...@ffwll.ch wrote:
  I think we can do it simpler. When you get a hpd interrupt you eventually
  call drm_helper_hpd_irq_event which will update all the state and poke
  connectors. I'd just create a delay_work which you launch from
  hdmi_irq_thread with a 1 second delay which again calls
  drm_helper_hpd_irq_event. That way you won't miss the EDID (presuming the
  user isn't _really_ careful with pluggin in the cable slowly).
 
 OK, I like this more simplistic option.
 
 However, one more pesky detail, if I am understanding your suggestion
 correctly. You are hoping for:
 
 1. I connect the display, causing HPD interrupt
 2. hpd irq handler calls drm_helper_hpd_irq_event() directly. Fails to
 read EDID, no big deal.
 3. hpd irq handles schedules a work item to call
 drm_helper_hpd_irq_event() a second time, 1 second later. Reads EDID
 sucessfully, image gets output to display.
 
 That doesn't quite work. drm_helper_hpd_irq_event() in step 2 notices
 and records the state change (disconnected -- connected). And after
 drm_helper_probe_single_connector_modes() fails to read the EDID, it
 starts outputting an image at 1024x768. When we come to step 3, we
 call drm_helper_hpd_irq_event again, but that just returns without
 doing anything as it does not detect any new state change.
 
 If we skip step 2, i.e. always delay the call to
 drm_helper_hpd_irq_event by 1 second, the problem is solved. As a
 user, the 1 second delay is not noticable. What do you think about
 just doing this?

Oh right, if you use the hpd pin for detection then this will happen. So I
guess always delaying is the right option. Or you could make the connector
state only conditional on an edid being present for HDMI, which would also
work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: fimd: Get signal polarities from device tree

[Daniel Vetter]

On Wed, May 01, 2013 at 09:06:09PM +0200, Tomasz Figa wrote:
 This patch modifies the driver to perform two stage parsing of video
 timings from device tree, to get timing information as struct videomode,
 which contains more data than struct fb_videomode.
 
 Thanks to this change, information about polarity of control signals
 (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard
 video timings.
 
 Signed-off-by: Tomasz Figa tomasz.f...@gmail.com

Since the drm mode struct also contains flags for sync polarity ... why is
there no direct of - drm_mode function? Going through an fb videomode in
a kms drm driver looks _really_ backwards to me.

Cc'in Dave for the fun of it ;-)

Cheers, Daniel

 ---
  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 18 --
  1 file changed, 16 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
 b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 index a1669d4..9023efa 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 @@ -21,7 +21,9 @@
  #include linux/pm_runtime.h
  
  #include video/of_display_timing.h
 +#include video/of_videomode.h
  #include video/samsung_fimd.h
 +#include video/videomode.h
  #include drm/exynos_drm.h
  
  #include exynos_drm_drv.h
 @@ -928,18 +930,30 @@ static int fimd_probe(struct platform_device *pdev)
   DRM_DEBUG_KMS(%s\n, __FILE__);
  
   if (pdev-dev.of_node) {
 + struct videomode vm;
 +
   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
   if (!pdata) {
   DRM_ERROR(memory allocation for pdata failed\n);
   return -ENOMEM;
   }
  
 - ret = of_get_fb_videomode(dev-of_node, pdata-panel.timing,
 - OF_USE_NATIVE_MODE);
 + ret = of_get_videomode(dev-of_node, vm, OF_USE_NATIVE_MODE);
   if (ret) {
   DRM_ERROR(failed: of_get_fb_videomode() : %d\n, ret);
   return ret;
   }
 +
 + fb_videomode_from_videomode(vm, pdata-panel.timing);
 +
 + if (vm.flags  DISPLAY_FLAGS_VSYNC_LOW)
 + pdata-vidcon1 |= VIDCON1_INV_VSYNC;
 + if (vm.flags  DISPLAY_FLAGS_HSYNC_LOW)
 + pdata-vidcon1 |= VIDCON1_INV_HSYNC;
 + if (vm.flags  DISPLAY_FLAGS_DE_LOW)
 + pdata-vidcon1 |= VIDCON1_INV_VDEN;
 + if (vm.flags  DISPLAY_FLAGS_PIXDATA_NEGEDGE)
 + pdata-vidcon1 |= VIDCON1_INV_VCLK;
   } else {
   pdata = pdev-dev.platform_data;
   if (!pdata) {
 -- 
 1.8.2.1
 
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] add mie driver for exynos

[Daniel Vetter]

On Mon, Dec 17, 2012 at 7:59 PM, Stéphane Marchesin
stephane.marche...@gmail.com wrote:

 And
 fimd, as display controller, could be controlled by linux framebuffer or drm
 framework.


 I don't think it's a valid point. If the framebuffer is properly done
 on top of the DRM, you don't need all of that at all.

Imo for hw with full-fledge drm kms drivers the sanest option for
traditional fbdev support is to pimp the fb helpers a bit. Pretty much
the only thing left that a real fbdev driver can do afaics is fb
reallocation - it shouldn't be too hard to (optionally) implement this
in the helpers ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html