Re: [PATCH] drm/exynos: fix kernel panic issue at drm releasing
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 wrote:
> >> 2015년 12월 24일 22:32에 Daniel Stone 이(가) 쓴 글:
> >>> On 24 December 2015 at 09:10, Inki Dae 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(&crtc->dev->event_lock, flags);
> >>>> + exynos_crtc->event = NULL;
> >>>> + spin_unlock_irqrestore(&crtc->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
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 > >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 v3 2/7] drm/exynos: make zpos property configurable
truct 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(&exynos_plane->base,
> - config->zpos);
> + exynos_plane_attach_zpos_property(&exynos_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] drm/exynos: only run atomic_check() if crtc is active
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
> >>>
> >>> 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
> >>> ---
> >>> 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
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
> >
> > 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
> > ---
> > 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
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, &ctx->planes[zpos],
> > - 1 << ctx->pipe, type, decon_formats,
> > + 1 << ctx->pipe, decon_formats,
> > ARRAY_SIZE(decon_formats), zpos);
> > if (ret)
> > return ret;
> > }
> >
> > - exynos_plane = &ctx->planes[ctx->default_win];
> > + exynos_plane = &ctx->planes[DEFAULT_WIN];
> > ctx->crtc = exynos_drm_crtc_create(drm_dev, &exynos_plane->base,
> >ctx->pipe, EXYNOS_DISPLAY_TYPE_LCD,
> >&decon_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()
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 :
>
> > From: Gustavo Padovan
> >
> > 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
> > ---
> > 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
On Thu, Aug 13, 2015 at 05:06:39PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan
>
> 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
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
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 : > > > >> Hi Gustavo, > >> > >> On 2015년 08월 06일 22:31, Gustavo Padovan wrote: > >>> From: Gustavo Padovan > >>> > >>> 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: [PATCH v2 11/11] drm/exynos: remove struct exynos_drm_encoder layer
On Fri, Aug 7, 2015 at 1:50 PM, Inki Dae 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: [RFC PATCH 2/8] drm: exynos/dp: convert to drm bridge mode
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 > >>--- > >> 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 > >>+ * Yakir Yang > >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 12/23] drm/exynos: don't track enabled state at exynos_crtc
On Mon, Jul 06, 2015 at 11:20:13AM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan
>
> 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
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
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
> > ---
> > 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 to ");
> > +
> > + if (last)
> > + printf("exit test application\n");
> > + else
> > + printf("skip to next test\n");
> > +
> If interested you can compact this as
>
> printf("press 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
On Thu, Feb 05, 2015 at 12:48:07PM +, Daniel Stone wrote: > Hi, > > On 5 February 2015 at 12:26, Rob Clark wrote: > > On Thu, Feb 5, 2015 at 4:15 AM, Daniel Vetter 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 08/14] drm/exynos: atomic phase 1: add atomic_begin()/atomic_flush()
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 > >>> > >>> 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 04/14] drm/exynos: remove struct *_win_data abstraction on planes
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
> >>>
> >>> 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 02/14] drm/exynos: Remove exynos_plane_dpms() call with no effect
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 > >>> > >>> 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 > >>> --- > >>> 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 00/14] drm/exynos: cleanups + atomic phases 1 and 2
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 > > > > 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 11/14] drm/exynos: atomic phase 2: keep track of framebuffer pointer
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 > > > > 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 > > --- > > 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 > > 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()
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
> >
> > 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
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
> > ---
> > 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 v
Re: [PATCH 04/14] drm/exynos: remove struct *_win_data abstraction on planes
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
> >
> > 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.
When I've done the testconversion with exynos to validate my atomic
helpers I've noticed that exynos makes a mess here, and on a quick look
Padovan seems to fix this up here. Not a detailed review, but this has my
Ack.
-Daniel
>
> Thanks.
>
> > Signed-off-by: Gustavo Padovan
> > ---
> > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 +-
> > drivers/gpu/drm/exynos/exynos_drm_crtc.h | 1 +
> > drivers/gpu/drm/exynos/exynos_drm_drv.c | 14 --
> > drivers/gpu/drm/exynos/exynos_drm_drv.h | 5 +-
> > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 182 ++---
> > drivers/gpu/drm/exynos/exynos_drm_plane.c | 20 +--
> > drivers/gpu/drm/exynos/exynos_drm_plane.h | 6 +-
> > drivers/gpu/drm/exynos/exynos_drm_vidi.c | 123 +
> > drivers/gpu/drm/exynos/exynos_mixer.c | 212
> > +++---
> > 9 files changed, 183 insertions(+), 389 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > index ad675fb..d504f0b 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > @@ -296,13 +296,13 @@ static void
> > exynos_drm_crtc_attach_mode_property(struct drm_crtc *crtc)
> > }
> >
> > struct exynos_drm_crtc *exynos_drm_crtc_create(struct drm_device *drm_dev,
> > + struct drm_plane *plane,
> >int pipe,
> >enum exynos_drm_output_type type,
> >struct exynos_drm_crtc_ops *ops,
> >void *ctx)
> > {
> > struct exynos_drm_crtc *exynos_crtc;
> > - struct drm_plane *plane;
> > struct exynos_drm_private *private = drm_dev->dev_private;
> > struct drm_crtc *crtc;
> > int ret;
> > @@ -318,12 +318,6 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct
> > drm_device *drm_dev,
> > exynos_crtc->type = type;
> > exynos_crtc->ops = ops;
> > exynos_crtc->ctx = ctx;
> > - plane = exynos_plane_init(drm_dev, 1 << pipe,
> > - DRM_PLANE_TYPE_PRIMARY);
> > - if (IS_ERR(plane)) {
> > - ret = PTR_ERR(plane);
> > - goto err_plane;
> > - }
> >
> > crtc = &exynos_crtc->base;
> >
> > @@ -342,7 +336,6 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct
> > drm_device *drm_dev,
> >
> > err_crtc:
> > plane->funcs->destroy(plane);
> > -err_plane:
> > kfree(exynos_crtc);
> > return ERR_PTR(ret);
> > }
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
> > b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
> > index 628b787..0ecd8fc 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
> > @@ -18,6 +18,7 @@
> > #include "exynos_drm_drv.h"
> >
> > struct exynos_drm_crtc *exynos_drm_crtc_create(struct drm_device *drm_dev,
> > + struct drm_plane *plane,
> >int pipe,
> >enum exynos_drm_output_type type,
> >struct exynos_drm_crtc_ops *ops,
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > index 737164d..778c91e 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -55,7 +55,6 @@ static int exynos_drm_load(struct drm_device *dev,
> > unsigned long flags)
> > {
> > struct exynos_drm_private *private;
> > int ret;
> > - int nr;
> >
> > private = kzalloc(sizeof(struct exynos_drm_private), GFP_KERNEL);
> > if (!private)
> > @@ -80,19 +79,6 @@ static int exynos_drm_load(struct drm_device *dev,
> > unsigned long flags)
> >
> > exynos_drm_mode_config_init(dev);
> >
> > - for (nr = 0; nr < MAX_PLANE; nr++) {
> > - struct drm_plane *plane;
> > - unsigned long possible_crtcs = (1 << MAX_CRTC) - 1;
> > -
> > - plane =
Re: [PATCH 02/14] drm/exynos: Remove exynos_plane_dpms() call with no effect
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 > > > > 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 > > --- > > 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 -v2] drm/exynos: track vblank events on a per crtc basis
On Fri, Jan 30, 2015 at 03:57:53PM +, Daniel Stone wrote: > Hi, > > On 30 January 2015 at 14:30, Gustavo Padovan wrote: > > 2015-01-30 Joonyoung Shim : > >> 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
On Tue, Jan 27, 2015 at 12:59:15PM +, Daniel Stone wrote:
> Hi,
>
> On 23 January 2015 at 12:42, Gustavo Padovan 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()
On Thu, Dec 18, 2014 at 11:58:51AM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan
>
> 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
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 11/13] drm/irq: Document return values more consistently
On Tue, Dec 16, 2014 at 05:53:33PM +0100, Thierry Reding wrote:
> From: Thierry Reding
>
> 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
With the comment in patch 9 addressed patches 1-11 are
Reviewed-by: Daniel Vetter
> ---
> 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 12/13] drm/irq: Expel legacy API
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 > > > > 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 > > 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 12/13] drm/irq: Expel legacy API
On Tue, Dec 16, 2014 at 05:53:34PM +0100, Thierry Reding wrote: > From: Thierry Reding > > 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 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 > -#include "drm_trace.h" > -#include "drm_internal.h" > - > +#include > #include /* For task queue support */ > #include > > -#include > -#include > +#include > + > +#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 so we can de
Re: [PATCH 09/13] drm/irq: Make pipe unsigned and name consistent
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 V7 03/12] drm/bridge: Add helper functions for drm_bridge
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
> > > > > > 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 k
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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 > >>> 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 wrote: > >>>>>> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter 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 bridges&panels 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
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
> > > > 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
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
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
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 dt&drm_panel or dt&drm_bridge.
> > Or maybe even acpi&drm_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
On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
> > 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 wrote:
> > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul
> > > > > 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.
>
> Yeah, I certainly agree that adding proper reference counting would be a
> good thing. I think perhaps we could just take a reference on the struct
> device * to prevent it from disappearing.
>
> Although perhaps I misunderstand what you mean by "go away".
I meant the drm_panel/bridge could go away any second, since nothing
prevents a module unload of the panel/bridge driver. So in theory you
could get the unregister call right after you've done the lookup. Which
means the bridge/panel pointer you've just returned is freed memory.
I think we nee try_get_module for the code and kref on the actual data
structures. 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 dt&drm_panel or dt&drm_bridge. Or
maybe even acpi&drm_bridge, who knows ;-)
-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
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 > > 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 wrote: > > >> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter 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
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 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
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 wrote:
> > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul 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
On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar wrote:
> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter wrote:
>> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar wrote:
>>> Hi Daniel and Sean,
>>>
>>> Thanks for the comments!
>>>
>>> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul wrote:
>>>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter 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 quick&grumpy 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
>>>>>
>>>>> [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
Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar wrote:
> Hi Daniel and Sean,
>
> Thanks for the comments!
>
> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul wrote:
>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter 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 quick&grumpy 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
>>>
>>> [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
On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter wrote:
> On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul 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 V7 03/12] drm/bridge: Add helper functions for drm_bridge
On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul 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
So don't ask why but I accidentally ended up in a branch looking at this
patch and didn't like it. So very quick&grumpy 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
[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 3/4] drm/exynos: enable vblank after DPMS on
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 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 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
On Fri, Oct 3, 2014 at 11:42 AM, Andrzej Hajda 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 RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers
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] drm/exynos: init vblank with real number of crtcs
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 > --- > 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
On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake 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 pointer&reference 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
On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake 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 0/2] drm/exynos: use drm generic mmap interface
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 > > 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
On Wed, Sep 17, 2014 at 2:19 PM, Daniel Drake 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] drm/exynos: fix plane-framebuffer linkage
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 > [] (exynos_drm_crtc_plane_commit) from [] > (exynos_drm_crtc_commit+0x44/0x70) > [] (exynos_drm_crtc_commit) from [] > (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4) > [] (exynos_drm_crtc_mode_set_commit.isra.2) from [] > (exynos_drm_crtc_page_flip+0x140/0x1a8) > [] (exynos_drm_crtc_page_flip) from [] > (drm_mode_page_flip_ioctl+0x224/0x2dc) > [] (drm_mode_page_flip_ioctl) from [] > (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 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
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] [] (unwind_backtrace) from [] > >>> (show_stack+0x10/0x14) > >>> [ 95.067766] [] (show_stack) from [] > >>> (dump_stack+0x70/0xbc) > >>> [ 95.074953] [] (dump_stack) from [] > >>> (warn_slowpath_common+0x64/0x88) > >>> [ 95.083005] [] (warn_slowpath_common) from [] > >>> (warn_slowpath_null+0x1c/0x24) > >>> [ 95.091780] [] (warn_slowpath_null) from [] > >>> (drm_mode_config_cleanup+0x258/0x268) > >>> [ 95.100983] [] (drm_mode_config_cleanup) from [] > >>> (exynos_drm_unload+0x38/0x50) > >>> [ 95.109915] [] (exynos_drm_unload) from [] > >>> (drm_dev_unregister+0x24/0x98) > >>> [ 95.118414] [] (drm_dev_unregister) from [] > >>> (drm_put_dev+0x28/0x64) > >>> [ 95.126412] [] (drm_put_dev) from [] > >>> (take_down_master+0x24/0x44) > >>> [ 95.134218] [] (take_down_master) from [] > >>> (component_del+0x8c/0xc8) > >>> [ 95.142201] [] (component_del) from [] > >>> (exynos_dsi_remove+0x18/0x2c) > >>> [ 95.150294] [] (exynos_dsi_remove) from [] > >>> (platform_drv_remove+0x18/0x1c) > >>> [ 95.158872] [] (platform_drv_remove) from [] > >>> (__device_release_driver+0x70/0xc4) > >>> [ 95.167981] [] (__device_release_driver) from [] > >>> (device_release_driver+0x20/0x2c) > >>> [ 95.177268] [] (device_release_driver) from [] > >>> (unbind_store+0x5c/0x94) > >>> [ 95.185597] [] (unbind_store) from [] > >>> (drv_attr_store+0x20/0x2c) > >>> [ 95.193323] [] (drv_attr_store) from [] > >>> (sysfs_kf_write+0x4c/0x50) > >>> [ 95.201224] [] (sysfs_kf_write) from [] > >>> (kernfs_fop_write+0xc4/0x184) > >>> [ 95.209393] [] (kernfs_fop_write) from [] > >>> (vfs_write+0xa0/0x1a8) > >>> [ 95.217111] [] (vfs_write) from [] > >>> (SyS_write+0x40/0x8c) > >>> [ 95.224146] [] (SyS_write) from [] > >>> (ret_fast_syscall+0x0/0x48) > >>> > >>> Signed-off-by: Andrzej Hajda > >>> --- > >>> 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 sh
Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence
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] [] (unwind_backtrace) from [] > > (show_stack+0x10/0x14) > > [ 95.067766] [] (show_stack) from [] > > (dump_stack+0x70/0xbc) > > [ 95.074953] [] (dump_stack) from [] > > (warn_slowpath_common+0x64/0x88) > > [ 95.083005] [] (warn_slowpath_common) from [] > > (warn_slowpath_null+0x1c/0x24) > > [ 95.091780] [] (warn_slowpath_null) from [] > > (drm_mode_config_cleanup+0x258/0x268) > > [ 95.100983] [] (drm_mode_config_cleanup) from [] > > (exynos_drm_unload+0x38/0x50) > > [ 95.109915] [] (exynos_drm_unload) from [] > > (drm_dev_unregister+0x24/0x98) > > [ 95.118414] [] (drm_dev_unregister) from [] > > (drm_put_dev+0x28/0x64) > > [ 95.126412] [] (drm_put_dev) from [] > > (take_down_master+0x24/0x44) > > [ 95.134218] [] (take_down_master) from [] > > (component_del+0x8c/0xc8) > > [ 95.142201] [] (component_del) from [] > > (exynos_dsi_remove+0x18/0x2c) > > [ 95.150294] [] (exynos_dsi_remove) from [] > > (platform_drv_remove+0x18/0x1c) > > [ 95.158872] [] (platform_drv_remove) from [] > > (__device_release_driver+0x70/0xc4) > > [ 95.167981] [] (__device_release_driver) from [] > > (device_release_driver+0x20/0x2c) > > [ 95.177268] [] (device_release_driver) from [] > > (unbind_store+0x5c/0x94) > > [ 95.185597] [] (unbind_store) from [] > > (drv_attr_store+0x20/0x2c) > > [ 95.193323] [] (drv_attr_store) from [] > > (sysfs_kf_write+0x4c/0x50) > > [ 95.201224] [] (sysfs_kf_write) from [] > > (kernfs_fop_write+0xc4/0x184) > > [ 95.209393] [] (kernfs_fop_write) from [] > > (vfs_write+0xa0/0x1a8) > > [ 95.217111] [] (vfs_write) from [] > > (SyS_write+0x40/0x8c) > > [ 95.224146] [] (SyS_write) from [] > > (ret_fast_syscall+0x0/0x48) > > > > Signed-off-by: Andrzej Hajda > > --- > > 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: [RFC V2 0/3] drm/bridge: panel and chaining
idge, 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 - 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
On Fri, Apr 25, 2014 at 8:17 AM, Ajay kumar 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 3/4] drm: Introduce drm_fb_helper_prepare()
On Wed, Apr 23, 2014 at 09:14:41AM +0200, Thierry Reding wrote: > On Tue, Apr 22, 2014 at 05:54:06PM +0200, Daniel Vetter wrote: > > On Tue, Apr 22, 2014 at 04:42:20PM +0200, Thierry Reding wrote: > [...] > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > > > b/drivers/gpu/drm/drm_fb_helper.c > [...] > > > @@ -502,6 +503,33 @@ static void drm_fb_helper_crtc_free(struct > > > drm_fb_helper *helper) > > > } > > > > > > /** > > > + * drm_fb_helper_prepare - setup a drm_fb_helper structure > > > + * @dev: DRM device > > > + * @helper: driver-allocated fbdev helper structure to set up > > > + * @funcs: pointer to structure of functions associate with this helper > > > + * > > > + * Sets up the bare minimum to make the framebuffer helper usable. This > > > is > > > + * useful to implement race-free initialization of the polling helpers. > > > In > > > + * that case a typical sequence would be: > > > + * > > > + * - call drm_fb_helper_prepare() > > > + * - set dev->mode_config.funcs > > > > This step is done in drm_fb_helper_prepare already. > > drm_fb_helper_prepare() sets fb_helper->funcs. dev->mode_config.funcs > needs to be set because it gets called by drm_kms_helper_hotplug_event() > which in turn is called by output_poll_execute(), which can be called at > any point after drm_kms_helper_poll_init(). It could be scheduled > immediately by drm_kms_helper_poll_enable(). > > I wonder if perhaps we should be wrapping that into a function as well. > Currently this is only documented in code by the drivers that call this. > But since it's only a single step it doesn't seem worth it. Perhaps if > we rolled the min/max_width/height into that function as well it would > be more worth it? That could be difficult to do since a couple of > drivers need to set those depending on the chipset generation. Oh I've misread this step for the fb_helper->funcs assignment. Makes sense and I don't think we need to augment your kerneldoc more to explain this. > > > + * - perform driver-specific setup Hm, maybe clarify this as "initialize modeset objects like crtcs, encoders and connectors"? Since that's the important part we need to get done here. > > > + * - call drm_kms_helper_poll_init() > > > > Maybe mention that after this you can start to handle hpd events using the > > probe helpers? > > Isn't that somewhat implied already? The poll helpers call directly the > dev->mode_config.funcs->output_poll_changed() function, which has > already been set up earlier. I've more meant that after this it's save for drivers to enable hpd interrupts and start to process them. I.e. - enable interrupts and start processing hpd events as an additional step to make it really cleear how it all fits together. Otherwise driver authors are left wondering wtf this isn't just one function call to do it all for them ;-) > > > + * - call drm_fb_helper_init() > > > + * - call drm_fb_helper_single_add_all_connectors() > > > + * - call drm_fb_helper_initial_config() > > > > Does this list look sane in the generated DocBook? Afaik kerneldoc > > unfortunately lacks any form of markup, at least afaik :( > > I must admit I didn't check. I'll make sure I do that before sending out > the next version. If it looks ugly I'm ok as-is, just wondered. Kerneldoc is a bit simplistic ime. -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
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 > 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 3/4] drm: Introduce drm_fb_helper_prepare()
On Tue, Apr 22, 2014 at 04:42:20PM +0200, Thierry Reding wrote:
> From: Thierry Reding
>
> 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
Some bikeshed on the kerneldoc below, with that addressed this is
Reviewed-by: Daniel Vetter
> ---
> 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
Re: [PATCH 2/4] drm: Constify struct drm_fb_helper_funcs
On Tue, Apr 22, 2014 at 04:42:19PM +0200, Thierry Reding wrote:
> From: Thierry Reding
>
> 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
Reviewed-by: Daniel Vetter
> ---
> 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/
Re: [PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
ector)
> >
> > power_off = !ptn_bridge->enabled;
> > ptn3460_pre_enable(ptn_bridge->bridge);
> > + ptn3460_enable(ptn_bridge->bridge);
> >
> > edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
> > if (!edid) {
> > @@ -265,7 +277,8 @@ struct drm_connector_funcs ptn3460_connector_funcs = {
> > };
> >
> > 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)
> > {
> > int ret;
> > struct drm_bridge *bridge;
> > @@ -324,6 +337,11 @@ int ptn3460_init(struct drm_device *dev, struct
> > drm_encoder *encoder,
> > goto err;
> > }
> >
> > + if (panel) {
> > + 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: [RFC 3/4] drm: add generic blob properties for image enhancement
On Mon, Mar 10, 2014 at 09:50:14AM +0530, Rahul Sharma wrote:
> On 8 March 2014 06:16, Matt Roper wrote:
> > On Fri, Mar 07, 2014 at 03:50:54PM +0530, Rahul Sharma wrote:
> >> Hi Daniel,
> >>
> >> On 7 March 2014 14:06, Daniel Vetter 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
> > ...
> >> > 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
_ptr) {
> + DRM_ERROR("failed to allocate blob for %s\n", prop_name);
> + drm_property_destroy(dev, prop);
> + return -ENOMEM;
> + }
> +
> + dev->mode_config.edge_enhancement_property = prop;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_edge_enhancement_property);
> +
> static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group
> *group)
> {
> uint32_t total_objects = 0;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 82f2016..df7b178 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -901,6 +901,13 @@ struct drm_mode_config {
> /* 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: Adding set_blob ioctl to DRM
On Thu, Feb 20, 2014 at 11:24:40AM +1000, Dave Airlie wrote: > > I am working on enabling a Color Enhancement block for primary display > > for Exynos SoC. I need to > > set a bunch of parameters like Color Conversion matrix, Contrast > > Improvement parameters etc ~ 30 parameters from User Space. > > > > I am planning to use KDS blob property to receive these parameters. > > Currently drivers are not allowed to create a blob property inside > > drm. Neither, user space can set the blob. There is no ioctl provided > > for same. > > I don't really like the idea of sticking unstructured data into an > ioctl, for the driver to interpret, > > it opens the door to all kinds of ugly driver hacks, so I think we > should have writable blobs, > but with well defined structures inside them, not per-driver ones. > > Per-driver structures will lead to binary userspace drivers that start > sticking a pll timings blob on the end and require it to set a mode, > because I know driver developers will abuse any interface in the worst > way possible. > > Currently the only blob we really have is EDID and its well defined, > so if we are going to add writable blobs, they need to be well defined > and as little as possible driver specific, just to avoid driver > writings doing what driver writers do. tbh I don't see a use for structured blobs at all and would much more prefer a pile of properties. With the atomic modeset ioctl proposals floating around we could then pass in an entire sets of properties, which is essentially what the blob prop would be doing. The upshot of going all-in with explicitly named properties is that we can shovel kms configuration descriptions into different formats. I'm thinking of shovelling an initial config into DT or something similar as an example. For i915 we have some experimental patches which load a DT blob as a firmware file and use the property values to set things up. So imo a blob property needs some _really_ good justification. My default stance is to oppose it ;-) 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 2/2] drm/exynos: Fix build after removal of DRM_WAKUP
On Tue, Jan 07, 2014 at 01:59:12PM +, Mark Brown wrote:
> From: Mark Brown
>
> Commit 57ed0f7b4375 (drm: Kill DRM_WAKUP and DRM_INIT_WAITQUEUE) removed
> the definition of DRM_WAKUP but did not remove the use of it in the Exynos
> DRM driver causing it to fail to build. Fix that.
>
> Signed-off-by: Mark Brown
Oops, looks like something slipped past between me making the patches and
them getting merged. Thanks for taking care of this. Both patches are
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index bdfe31fb731b..05b72b07a134 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -706,7 +706,7 @@ static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
> /* set wait vsync event to zero and wake up queue. */
> if (atomic_read(&ctx->wait_vsync_event)) {
> atomic_set(&ctx->wait_vsync_event, 0);
> - DRM_WAKEUP(&ctx->wait_vsync_queue);
> + wake_up(&ctx->wait_vsync_queue);
> }
> out:
> return IRQ_HANDLED;
> --
> 1.8.5.2
>
--
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?
On Wed, Dec 18, 2013 at 10:28:37AM -0600, Daniel Drake wrote: > On Wed, Dec 18, 2013 at 2:43 AM, Daniel Vetter 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: drm_do_probe_ddc_edid ENXIO check too aggressive?
On Tue, Dec 17, 2013 at 09:12:42AM -0600, Daniel Drake wrote: > On Mon, Dec 16, 2013 at 5:40 PM, Daniel Vetter 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: [PATCH] drm/exynos: fimd: Get signal polarities from device tree
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
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
>
> #include
> +#include
> #include
> +#include
> #include
>
> #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
On Mon, Dec 17, 2012 at 7:59 PM, Stéphane Marchesin 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
