[PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation
On 08/25/2015 07:15 PM, Daniel Vetter wrote:
> Faster than recompiling.
>
> Note that restore_fbdev_mode_unlocked is a bit special and the only
> one which returns an error code when fbdev isn't there - i915 needs
> that one to not fall over with some additional fbcon related restore
> code. Everyone else just ignores the return value or only prints a
> DRM_DEBUG level message.
Reviewed-by:Archit Taneja
>
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_fb_helper.c | 29 +
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 83aacb0cc9df..8259dec1de1f 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -39,6 +39,11 @@
> #include
> #include
>
> +static bool drm_fbdev_emulation = true;
> +module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> +MODULE_PARM_DESC(fbdev_emulation,
> + "Enable legacy fbdev emulation [default=true]");
> +
> static LIST_HEAD(kernel_fb_helper_list);
>
> /**
> @@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct
> drm_fb_helper *fb_helper)
> struct drm_connector *connector;
> int i;
>
> + if (!drm_fbdev_emulation)
> + return 0;
> +
> mutex_lock(&dev->mode_config.mutex);
> drm_for_each_connector(connector, dev) {
> struct drm_fb_helper_connector *fb_helper_connector;
> @@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper
> *fb_helper, struct drm_
> struct drm_fb_helper_connector **temp;
> struct drm_fb_helper_connector *fb_helper_connector;
>
> + if (!drm_fbdev_emulation)
> + return 0;
> +
> WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
> if (fb_helper->connector_count + 1 >
> fb_helper->connector_info_alloc_count) {
> temp = krealloc(fb_helper->connector_info, sizeof(struct
> drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
> @@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct
> drm_fb_helper *fb_helper,
> struct drm_fb_helper_connector *fb_helper_connector;
> int i, j;
>
> + if (!drm_fbdev_emulation)
> + return 0;
> +
> WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>
> for (i = 0; i < fb_helper->connector_count; i++) {
> @@ -375,6 +389,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
> drm_fb_helper *fb_helper)
> bool do_delayed
> int ret;
>
> + if (!drm_fbdev_emulation)
> + return -ENODEV;
> +
> drm_modeset_lock_all(dev);
> ret = restore_fbdev_mode(fb_helper);
>
> @@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
> struct drm_crtc *crtc;
> int i;
>
> + if (!drm_fbdev_emulation)
> + return 0;
> +
> if (!max_conn_count)
> return -EINVAL;
>
> @@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
>
> void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> {
> + if (!drm_fbdev_emulation)
> + return;
> +
> if (!list_empty(&fb_helper->kernel_fb_list)) {
> list_del(&fb_helper->kernel_fb_list);
> if (list_empty(&kernel_fb_helper_list)) {
> @@ -1933,6 +1956,9 @@ int drm_fb_helper_initial_config(struct drm_fb_helper
> *fb_helper, int bpp_sel)
> struct drm_device *dev = fb_helper->dev;
> int count = 0;
>
> + if (!drm_fbdev_emulation)
> + return 0;
> +
> mutex_lock(&dev->mode_config.mutex);
> count = drm_fb_helper_probe_connector_modes(fb_helper,
> dev->mode_config.max_width,
> @@ -1976,6 +2002,9 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper
> *fb_helper)
> struct drm_device *dev = fb_helper->dev;
> u32 max_width, max_height;
>
> + if (!drm_fbdev_emulation)
> + return 0;
> +
> mutex_lock(&fb_helper->dev->mode_config.mutex);
> if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> fb_helper->delayed_hotplug = true;
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask
mdp5_hw_init and mdp5_set_irqmask configure registers but may not have
clocks enabled.
Add mdp5_enable/disable calls in these funcs to ensure clocks are
enabled. We need this until we get proper runtime pm support.
Signed-off-by: Archit Taneja
---
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 10 --
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 2 ++
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
index b1f73be..9fabfca 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
@@ -24,9 +24,15 @@
void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
uint32_t old_irqmask)
{
- mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_CLEAR(0),
+ struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
+
+ mdp5_enable(mdp5_kms);
+
+ mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_CLEAR(0),
irqmask ^ (irqmask & old_irqmask));
- mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_EN(0), irqmask);
+ mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_EN(0), irqmask);
+
+ mdp5_disable(mdp5_kms);
}
static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t irqstatus)
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
index 047cb04..2b760f5 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
@@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
unsigned long flags;
pm_runtime_get_sync(dev->dev);
+ mdp5_enable(mdp5_kms);
/* Magic unknown register writes:
*
@@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
mdp5_ctlm_hw_reset(mdp5_kms->ctlm);
+ mdp5_disable(mdp5_kms);
pm_runtime_put_sync(dev->dev);
return 0;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
[PATCH 3/3] modetest: add atomic page flip support
This patch adds support for atomic page flip. User can specify -V option
with the plane id for testing atomic page flipping.
---
tests/modetest/modetest.c | 153 --
1 file changed, 149 insertions(+), 4 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 753a559..9bffa98 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -719,6 +719,10 @@ struct pipe_arg {
struct timeval start;
int swap_count;
+
+ /* for atomic modeset */
+ uint32_t plane_id;
+ uint32_t fb_obj_id;
};
struct plane_arg {
@@ -1444,7 +1448,7 @@ static int parse_property(struct property_arg *p, const
char *arg)
static void usage(char *name)
{
- fprintf(stderr, "usage: %s [-acDdefMPpsCvw]\n", name);
+ fprintf(stderr, "usage: %s [-acDdefMPpsCvVw]\n", name);
fprintf(stderr, "\tA: supported in atomic modeset\n");
fprintf(stderr, "\tL: supported in legacy modeset\n");
@@ -1459,6 +1463,7 @@ static void usage(char *name)
fprintf(stderr, "\n Atomic Test options: [A]\n\n");
fprintf(stderr, "\t-a\tuse atomic modeset\n");
+ fprintf(stderr, "\t-V \ttest vsynced page
flipping\n");
fprintf(stderr, "\n Legacy test options: [L]\n\n");
fprintf(stderr, "\t-P
:x[++][*][@]\tset a plane\n");
@@ -1627,7 +1632,138 @@ static void atomic_modeset(struct device *dev,
drmModeAtomicReqPtr req,
drmModeAtomicCommit(dev->fd, req, flags, NULL);
}
-static char optstr[] = "acdD:efM:P:ps:Cvw:";
+static void
+atomic_page_flip_handler(int fd, unsigned int frame,
+ unsigned int sec, unsigned int usec, void *data)
+{
+ static drmModeAtomicReqPtr req = NULL;
+ unsigned int new_fb_id;
+ struct timeval end;
+ struct pipe_arg *pipe;
+ double t;
+ uint32_t flags = 0;
+
+ pipe = (struct pipe_arg *)(unsigned long)data;
+
+ if (pipe->current_fb_id == pipe->fb_id[0])
+ new_fb_id = pipe->fb_id[1];
+ else
+ new_fb_id = pipe->fb_id[0];
+
+ pipe->current_fb_id = new_fb_id;
+ pipe->swap_count++;
+
+ req = drmModeAtomicAlloc();
+
+ drmModeAtomicAddProperty(req, pipe->plane_id, pipe->fb_obj_id,
new_fb_id);
+
+ flags = DRM_MODE_PAGE_FLIP_EVENT;
+ drmModeAtomicCommit(fd, req, flags, pipe);
+
+ if (pipe->swap_count == 60) {
+ gettimeofday(&end, NULL);
+ t = end.tv_sec + end.tv_usec * 1e-6 -
+ (pipe->start.tv_sec + pipe->start.tv_usec * 1e-6);
+ fprintf(stderr, "freq: %.02fHz\n", pipe->swap_count / t);
+ pipe->swap_count = 0;
+ pipe->start = end;
+ }
+
+ drmModeAtomicFree(req);
+}
+
+static int atomic_test_page_flip(struct device *dev, drmModeAtomicReqPtr req,
+ struct resources *res, struct property_arg *prop_args,
+ unsigned int prop_count,unsigned int plane_id)
+{
+ struct bo *other_bo;
+ unsigned int other_fb_id;
+ struct pipe_arg *pipe = NULL;
+ drmEventContext evctx;
+ uint32_t flags = 0, fb_obj_id = 0, pixel_format;
+ uint64_t width, height;
+ int ret;
+
+ if (!is_obj_id_in_prop_args(prop_args, prop_count, plane_id))
+ return -1;
+
+ fb_obj_id = get_atomic_plane_prop_id(res, plane_id, "FB_ID");
+ if (!fb_obj_id)
+ return -1;
+
+ width = get_value_in_prop_args(prop_args, prop_count, plane_id,
+ "SRC_W");
+ height = get_value_in_prop_args(prop_args, prop_count, plane_id,
+ "SRC_H");
+ pixel_format = DRM_FORMAT_XRGB;
+
+ ret = allocate_fb(dev, req, res, width, height, pixel_format,
+ PATTERN_TILES, &other_bo, &other_fb_id);
+ if (ret < 0)
+ return ret;
+
+ ret = drmModeAtomicAddProperty(req, plane_id, fb_obj_id,
+ other_fb_id);
+ if (ret < 0)
+ goto err;
+
+ if (!fb_obj_id) {
+ fprintf(stderr, "page flipping requires at least one plane
setting.\n");
+ return -1;
+ }
+
+ pipe = drmMalloc(sizeof *pipe);
+
+ gettimeofday(&pipe->start, NULL);
+ pipe->swap_count = 0;
+ pipe->plane_id = plane_id;
+ pipe->fb_obj_id = fb_obj_id;
+ pipe->fb_id[0] = dev->mode.fb_id;
+ pipe->fb_id[1] = other_fb_id;
+ pipe->current_fb_id = other_fb_id;
+
+ flags = DRM_MODE_PAGE_FLIP_EVENT;
+
+ ret = drmModeAtomicCommit(dev->fd, req, flags, pipe);
+ if (ret < 0)
+ goto err_rmfb;
+
+ memset(&evctx, 0, sizeof evctx);
+ evctx.version = DRM_EVENT_CONTEXT_VERSION;
+ evctx.vblank_handler = NULL;
+ evctx.page_flip_handler = atomic_page_flip_handler;
+
+ while (1) {
+ struct timeval timeout = { .tv_sec = 3, .tv_usec = 0 };
+
[PATCH 1/3] modetest: introduce get_prop_info() for getting property id and type
Modetest gets the property name from user to set it. So the name must be
converted to its id. Until now, this is done in the set_property(). But to
support atomic modeset in modetest, this logic should be separated from the
fuction, because atomic modeset and legacy modeset use different IOCTLs.
Signed-off-by: Hyungwon Hwang
---
tests/modetest/modetest.c | 31 +--
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 43bd06f..b7f6d32 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -866,12 +866,11 @@ struct property_arg {
uint64_t value;
};
-static void set_property(struct device *dev, struct property_arg *p)
+static int get_prop_info(struct resources *resources, struct property_arg *p,
+ char *obj_type)
{
drmModeObjectProperties *props = NULL;
drmModePropertyRes **props_info = NULL;
- const char *obj_type;
- int ret;
int i;
p->obj_type = 0;
@@ -884,27 +883,27 @@ static void set_property(struct device *dev, struct
property_arg *p)
if (obj->type->type##_id != p->obj_id)
\
continue;
\
p->obj_type = DRM_MODE_OBJECT_##Type;
\
- obj_type = #Type;
\
+ obj_type = (char *)#Type;
\
props = obj->props;
\
props_info = obj->props_info;
\
}
\
} while(0)
\
- find_object(dev->resources, res, crtc, CRTC);
+ find_object(resources, res, crtc, CRTC);
if (p->obj_type == 0)
- find_object(dev->resources, res, connector, CONNECTOR);
+ find_object(resources, res, connector, CONNECTOR);
if (p->obj_type == 0)
- find_object(dev->resources, plane_res, plane, PLANE);
+ find_object(resources, plane_res, plane, PLANE);
if (p->obj_type == 0) {
fprintf(stderr, "Object %i not found, can't set property\n",
p->obj_id);
- return;
+ return -1;
}
if (!props) {
fprintf(stderr, "%s %i has no properties\n",
obj_type, p->obj_id);
- return;
+ return -1;
}
for (i = 0; i < (int)props->count_props; ++i) {
@@ -917,11 +916,23 @@ static void set_property(struct device *dev, struct
property_arg *p)
if (i == (int)props->count_props) {
fprintf(stderr, "%s %i has no %s property\n",
obj_type, p->obj_id, p->name);
- return;
+ return -1;
}
p->prop_id = props->props[i];
+ return 0;
+}
+
+static void set_property(struct device *dev, struct property_arg *p)
+{
+ int ret;
+ char *obj_type = NULL;
+
+ ret = get_prop_info(dev->resources, p, obj_type);
+ if (ret < 0)
+ return;
+
ret = drmModeObjectSetProperty(dev->fd, p->obj_id, p->obj_type,
p->prop_id, p->value);
if (ret < 0)
--
2.4.3
[PATCH 2/3] modetest: add atomic modeset support
This patch adds support for atomic modeset. Using -a option, user can
make modeset to use DRM_IOCTL_MODE_ATOMIC instead of legacy IOCTLs.
Also, by using -w option, user can set the property as before.
Signed-off-by: Hyungwon Hwang
---
tests/modetest/modetest.c | 221 +++---
1 file changed, 187 insertions(+), 34 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index b7f6d32..753a559 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -1444,25 +1444,32 @@ static int parse_property(struct property_arg *p, const
char *arg)
static void usage(char *name)
{
- fprintf(stderr, "usage: %s [-cDdefMPpsCvw]\n", name);
+ fprintf(stderr, "usage: %s [-acDdefMPpsCvw]\n", name);
+ fprintf(stderr, "\tA: supported in atomic modeset\n");
+ fprintf(stderr, "\tL: supported in legacy modeset\n");
- fprintf(stderr, "\n Query options:\n\n");
+ fprintf(stderr, "\n Query options: [AL]\n\n");
fprintf(stderr, "\t-c\tlist connectors\n");
fprintf(stderr, "\t-e\tlist encoders\n");
fprintf(stderr, "\t-f\tlist framebuffers\n");
fprintf(stderr, "\t-p\tlist CRTCs and planes (pipes)\n");
- fprintf(stderr, "\n Test options:\n\n");
+ fprintf(stderr, "\n Common Test options: [AL]\n\n");
+ fprintf(stderr, "\t-w ::\tset property\n");
+
+ fprintf(stderr, "\n Atomic Test options: [A]\n\n");
+ fprintf(stderr, "\t-a\tuse atomic modeset\n");
+
+ fprintf(stderr, "\n Legacy test options: [L]\n\n");
fprintf(stderr, "\t-P
:x[++][*][@]\tset a plane\n");
fprintf(stderr, "\t-s
[,][@]:[-][@]\tset
a mode\n");
fprintf(stderr, "\t-C\ttest hw cursor\n");
fprintf(stderr, "\t-v\ttest vsynced page flipping\n");
- fprintf(stderr, "\t-w ::\tset property\n");
fprintf(stderr, "\n Generic options:\n\n");
- fprintf(stderr, "\t-d\tdrop master after mode set\n");
- fprintf(stderr, "\t-M module\tuse the given driver\n");
- fprintf(stderr, "\t-D device\tuse the given device\n");
+ fprintf(stderr, "\t-d\tdrop master after mode set [L]\n");
+ fprintf(stderr, "\t-M module\tuse the given driver [AL]\n");
+ fprintf(stderr, "\t-D device\tuse the given device [AL]\n");
fprintf(stderr, "\n\tDefault is to dump all info.\n");
exit(0);
@@ -1495,7 +1502,132 @@ static int cursor_supported(void)
return 1;
}
-static char optstr[] = "cdD:efM:P:ps:Cvw:";
+static uint32_t is_obj_id_in_prop_args(struct property_arg *prop_args,
+ unsigned int prop_count, uint32_t obj_id)
+{
+ unsigned int i;
+
+ for (i = 0; i < prop_count; i++)
+ if (obj_id == prop_args[i].obj_id)
+ return true;
+
+ return false;
+}
+
+static int get_value_in_prop_args(struct property_arg *prop_args,
+ unsigned int prop_count, uint32_t obj_id,
+ const char *name)
+{
+ unsigned int i;
+
+ for (i = 0; i < prop_count; i++)
+ if (prop_args[i].obj_id == obj_id &&
+ !strcmp(prop_args[i].name, name))
+ return (int)prop_args[i].value;
+
+ return -1;
+}
+
+static uint32_t get_atomic_plane_prop_id(struct resources *res, uint32_t
obj_id,
+ const char *name)
+{
+ drmModePropertyRes *props_info;
+ struct plane *plane;
+ unsigned int i, j;
+
+ for (i = 0; i < res->plane_res->count_planes; i++) {
+ plane = &res->planes[i];
+ if (plane->plane->plane_id != obj_id)
+ continue;
+
+ for (j = 0; j < plane->props->count_props; j++) {
+ props_info = plane->props_info[j];
+ if (!strcmp(props_info->name, name))
+ return props_info->prop_id;
+ }
+ }
+
+ return 0;
+}
+
+static int allocate_fb(struct device *dev, drmModeAtomicReqPtr req, struct
resources *res,
+ uint32_t width, uint32_t height, uint32_t pixel_format,
+ int pattern, struct bo **bo, uint32_t *fb_id)
+{
+ uint32_t handles[4] = {0}, pitches[4] = {0}, offsets[4] = {0};
+ int ret;
+
+ *bo = bo_create(dev->fd, pixel_format, width, height,
+ handles, pitches, offsets, pattern);
+ if (*bo == NULL) {
+ fprintf(stderr, "failed to create bo (%ux%u): %s\n",
+ width, height, strerror(errno));
+ return -1;
+ }
+
+ ret = drmModeAddFB2(dev->fd, width, height, pixel_format,
+ handles, pitches, offsets, fb_id, 0);
+ if (ret) {
+ fprintf(stderr, "failed to add fb (%ux%u): %s\n",
+ width, height, strerror(errno));
+
[GIT PULL] drm/rockchip: fixes and new features
Hi Dave Here are some fixes and some new features for rockchip drm, tested on popmetal rk3288 board, can you land them? The following changes since commit db56176025cee5e242dfeed5f4e304d095d29fa3: Revert "drm/atomic: Call ww_acquire_done after check phase is complete" (2015-08-25 17:23:36 +1000) are available in the git repository at: https://github.com/markyzq/kernel-drm-rockchip.git drm-rockchip-2015-08-26 for you to fetch changes up to 4c156c21c7948a0be854cbe5914af3181303e529: drm/rockchip: vop: support plane scale (2015-08-26 14:16:26 +0800) Mark Yao (6): drm/rockchip: vop: Fix virtual stride calculation drm/rockchip: vop: Fix window dest start point drm/rockchip: vop: Add yuv plane support drm/rockchip: vop: Default enable win2/3 area0 bit drm/rockchip: vop: restore vop registers when resume drm/rockchip: vop: support plane scale drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 269 ++- drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 88 + 2 files changed, 348 insertions(+), 9 deletions(-) -Mark
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
Hi, Tiago.
On 08/26/2015 02:02 AM, Tiago Vignatti wrote:
> From: Daniel Vetter
>
> The userspace might need some sort of cache coherency management e.g. when CPU
> and GPU domains are being accessed through dma-buf at the same time. To
> circumvent this problem there are begin/end coherency markers, that forward
> directly to existing dma-buf device drivers vfunc hooks. Userspace can make
> use
> of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be
> used like following:
>
> - mmap dma-buf fd
> - for each drawing/upload cycle in CPU
> 1. SYNC_START ioctl
> 2. read/write to mmap area or a 2d sub-region of it
> 3. SYNC_END ioctl.
> - munamp once you don't need the buffer any more
>
> v2 (Tiago): Fix header file type names (u64 -> __u64)
> v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end
> dma-buf functions. Check for overflows in start/length.
> v4 (Tiago): use 2d regions for sync.
Daniel V had issues with the sync argument proposed by Daniel S. I'm
fine with that argument or an argument containing only a single sync
rect. I'm not sure whether Daniel V will find it easier to accept only a
single sync rect...
Also please see below.
>
> Cc: Sumit Semwal
> Signed-off-by: Daniel Vetter
> Signed-off-by: Tiago Vignatti
> ---
>
> I'm unable to test the 2d sync properly, because begin/end access in i915
> don't track mapped range for nothing.
>
> Documentation/dma-buf-sharing.txt | 13 ++
> drivers/dma-buf/dma-buf.c | 77
> --
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 ++-
> include/linux/dma-buf.h| 20 +
> include/uapi/linux/dma-buf.h | 57 +
> 5 files changed, 150 insertions(+), 23 deletions(-)
> create mode 100644 include/uapi/linux/dma-buf.h
>
> diff --git a/Documentation/dma-buf-sharing.txt
> b/Documentation/dma-buf-sharing.txt
> index 480c8de..8061ac0 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -355,6 +355,19 @@ Being able to mmap an export dma-buf buffer object has 2
> main use-cases:
>
> No special interfaces, userspace simply calls mmap on the dma-buf fd.
>
> + Also, the userspace might need some sort of cache coherency management
> e.g.
> + when CPU and GPU domains are being accessed through dma-buf at the same
> + time. To circumvent this problem there are begin/end coherency markers,
> that
> + forward directly to existing dma-buf device drivers vfunc hooks. Userspace
> + can make use of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The
> + sequence would be used like following:
> + - mmap dma-buf fd
> + - for each drawing/upload cycle in CPU
> + 1. SYNC_START ioctl
> + 2. read/write to mmap area or a 2d sub-region of it
> + 3. SYNC_END ioctl.
> + - munamp once you don't need the buffer any more
> +
> 2. Supporting existing mmap interfaces in importers
>
> Similar to the motivation for kernel cpu access it is again important that
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 155c146..b6a4a06 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -251,11 +251,55 @@ out:
> return events;
> }
>
> +static long dma_buf_ioctl(struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct dma_buf *dmabuf;
> + struct dma_buf_sync sync;
> + enum dma_data_direction direction;
> +
> + dmabuf = file->private_data;
> +
> + if (!is_dma_buf_file(file))
> + return -EINVAL;
> +
> + if (cmd != DMA_BUF_IOCTL_SYNC)
> + return -ENOTTY;
> +
> + if (copy_from_user(&sync, (void __user *) arg, sizeof(sync)))
> + return -EFAULT;
Do we actually copy all sync regions here?
> +
> + if (sync.flags & DMA_BUF_SYNC_RW)
> + direction = DMA_BIDIRECTIONAL;
> + else if (sync.flags & DMA_BUF_SYNC_READ)
> + direction = DMA_FROM_DEVICE;
> + else if (sync.flags & DMA_BUF_SYNC_WRITE)
> + direction = DMA_TO_DEVICE;
> + else
> + return -EINVAL;
> +
> + if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK)
> + return -EINVAL;
> +
> + /* TODO: check for overflowing the buffer's size - how so, checking
> region by
> + * region here? Maybe need to check for the other parameters as well. */
> +
> + if (sync.flags & DMA_BUF_SYNC_END)
> + dma_buf_end_cpu_access(dmabuf, sync.stride_bytes,
> sync.bytes_per_pixel,
> + sync.num_regions, sync.regions, direction);
> + else
> + dma_buf_begin_cpu_access(dmabuf, sync.stride_bytes,
> sync.bytes_per_pixel,
> + sync.num_regions, sync.regions, direction);
> +
> + return 0;
> +}
> +
> static const struct file_operations dma_buf_fops = {
> .release= dma_bu
[Intel-gfx] [PATCH] drm/i915: Fix build warning on 32-bit
On Mon, Aug 17, 2015 at 05:19:09PM +, Zanoni, Paulo R wrote:
> Em Sex, 2015-08-14 Ã s 12:35 +0200, Thierry Reding escreveu:
> > From: Thierry Reding
> >
> > The gtt.stolen_size field is of type size_t, and so should be printed
> > using %zu to avoid build warnings on either 32-bit and 64-bit builds.
>
> While the suggestion from Chris sounds good, this patch alone is
> already a fix, so:
> Reviewed-by: Paulo Zanoni
Queued for -next, thanks for the patch.
-Daniel
>
> >
> > Signed-off-by: Thierry Reding
> > ---
> > drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index a36cb95ec798..f361c4a56995 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -348,7 +348,7 @@ int i915_gem_init_stolen(struct drm_device *dev)
> > * memory, so just consider the start. */
> > reserved_total = stolen_top - reserved_base;
> >
> > - DRM_DEBUG_KMS("Memory reserved for graphics device: %luK,
> > usable: %luK\n",
> > + DRM_DEBUG_KMS("Memory reserved for graphics device: %zuK,
> > usable: %luK\n",
> > dev_priv->gtt.stolen_size >> 10,
> > (dev_priv->gtt.stolen_size - reserved_total)
> > >> 10);
> >
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[Intel-gfx] [PATCH] drm/i915: Fix build warning on 32-bit
On Wed, 26 Aug 2015, Daniel Vetter wrote:
> On Mon, Aug 17, 2015 at 05:19:09PM +, Zanoni, Paulo R wrote:
>> Em Sex, 2015-08-14 Ã s 12:35 +0200, Thierry Reding escreveu:
>> > From: Thierry Reding
>> >
>> > The gtt.stolen_size field is of type size_t, and so should be printed
>> > using %zu to avoid build warnings on either 32-bit and 64-bit builds.
>>
>> While the suggestion from Chris sounds good, this patch alone is
>> already a fix, so:
>> Reviewed-by: Paulo Zanoni
>
> Queued for -next, thanks for the patch.
Great. Also pushed to drm-intel-next-fixes.
Jani.
> -Daniel
>
>>
>> >
>> > Signed-off-by: Thierry Reding
>> > ---
>> > drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> > b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> > index a36cb95ec798..f361c4a56995 100644
>> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> > @@ -348,7 +348,7 @@ int i915_gem_init_stolen(struct drm_device *dev)
>> > * memory, so just consider the start. */
>> >reserved_total = stolen_top - reserved_base;
>> >
>> > - DRM_DEBUG_KMS("Memory reserved for graphics device: %luK,
>> > usable: %luK\n",
>> > + DRM_DEBUG_KMS("Memory reserved for graphics device: %zuK,
>> > usable: %luK\n",
>> > dev_priv->gtt.stolen_size >> 10,
>> > (dev_priv->gtt.stolen_size - reserved_total)
>> > >> 10);
>> >
>> ___
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
[Intel-gfx] [PATCH] drm/i915: Fix build warning on 32-bit
On Wed, Aug 26, 2015 at 10:26:35AM +0300, Jani Nikula wrote: > On Wed, 26 Aug 2015, Daniel Vetter wrote: > > On Mon, Aug 17, 2015 at 05:19:09PM +, Zanoni, Paulo R wrote: > >> Em Sex, 2015-08-14 Ã s 12:35 +0200, Thierry Reding escreveu: > >> > From: Thierry Reding > >> > > >> > The gtt.stolen_size field is of type size_t, and so should be printed > >> > using %zu to avoid build warnings on either 32-bit and 64-bit builds. > >> > >> While the suggestion from Chris sounds good, this patch alone is > >> already a fix, so: > >> Reviewed-by: Paulo Zanoni > > > > Queued for -next, thanks for the patch. > > Great. Also pushed to drm-intel-next-fixes. Oops, dropped again from dinq ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH libdrm] omap: Initialize DMA BUF file descriptor to -1
From: Thierry Reding
Commit c86dabfc9f04 ("omap: zero is a valid fd number, treat it as
such") corrected checks for valid file descriptors, but the OMAP buffer
object code initializes the DMA-BUF file descriptor to 0 (as a result of
calloc()'ing the structure). Obviously this isn't going to work because
subsequent code will try to use file descriptor 0 (most likely stdin at
that point) as a DMA-BUF. It may also try and close stdin when a buffer
object is destroyed.
Fix this by initializing the DMA-BUF file descriptor to -1, properly
marking it as an invalid file descriptor.
Fixes: c86dabfc9f04 ("omap: zero is a valid fd number, treat it as such")
Reported-by: Robert Nelson
Tested-by: Robert Nelson
Signed-off-by: Thierry Reding
---
Emil, I think this may warrant cutting a new release to unbreak OMAP. I
can push the patch myself, but I've never done a libdrm release before,
so maybe you want to do the honors?
omap/omap_drm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/omap/omap_drm.c b/omap/omap_drm.c
index 4a0248d5e0d8..08ba64eb694d 100644
--- a/omap/omap_drm.c
+++ b/omap/omap_drm.c
@@ -186,6 +186,7 @@ static struct omap_bo * bo_from_handle(struct omap_device
*dev,
}
bo->dev = omap_device_ref(dev);
bo->handle = handle;
+ bo->fd = -1;
atomic_set(&bo->refcnt, 1);
/* add ourselves to the handle table: */
drmHashInsert(dev->handle_table, handle, bo);
--
2.4.5
[PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation
On 08/26/2015 10:42 AM, Archit Taneja wrote:
>
>
> On 08/25/2015 07:15 PM, Daniel Vetter wrote:
>> Faster than recompiling.
>>
>> Note that restore_fbdev_mode_unlocked is a bit special and the only
>> one which returns an error code when fbdev isn't there - i915 needs
>> that one to not fall over with some additional fbcon related restore
>> code. Everyone else just ignores the return value or only prints a
>> DRM_DEBUG level message.
>
> Reviewed-by:Archit Taneja
With the module param, and the drivers should see the following state(
based on the truth table below):
module param | config option
true |true -> real helper funcs called, driver
allocated drm_fb_helper is correctly
populated.
false |true -> real helper funcs called, but bailed
out early, driver allocated
drm_fb_helper is non-NULL, but we won't
end up using it.
X|false -> stub functions called, drm_fb_helper is
NULL
I wonder if the 2nd combination could leave space for errors in other
drivers. Drivers tend to check whether the fb_helper struct is NULL
or not, and make decisions based on that. If the decisions are to
just call a drm_fb_helper function, then we're okay. If they do
something more than that, then we could have an issue.
I'll prepare the remainder of 'Step 2' part of the series over this,
and we can test to check if we don't hit any corner cases.
Archit
>
>>
>> Signed-off-by: Daniel Vetter
>> ---
>> drivers/gpu/drm/drm_fb_helper.c | 29 +
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index 83aacb0cc9df..8259dec1de1f 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -39,6 +39,11 @@
>> #include
>> #include
>>
>> +static bool drm_fbdev_emulation = true;
>> +module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
>> +MODULE_PARM_DESC(fbdev_emulation,
>> + "Enable legacy fbdev emulation [default=true]");
>> +
>> static LIST_HEAD(kernel_fb_helper_list);
>>
>> /**
>> @@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct
>> drm_fb_helper *fb_helper)
>> struct drm_connector *connector;
>> int i;
>>
>> +if (!drm_fbdev_emulation)
>> +return 0;
>> +
>> mutex_lock(&dev->mode_config.mutex);
>> drm_for_each_connector(connector, dev) {
>> struct drm_fb_helper_connector *fb_helper_connector;
>> @@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct
>> drm_fb_helper *fb_helper, struct drm_
>> struct drm_fb_helper_connector **temp;
>> struct drm_fb_helper_connector *fb_helper_connector;
>>
>> +if (!drm_fbdev_emulation)
>> +return 0;
>> +
>> WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>> if (fb_helper->connector_count + 1 >
>> fb_helper->connector_info_alloc_count) {
>> temp = krealloc(fb_helper->connector_info, sizeof(struct
>> drm_fb_helper_connector *) * (fb_helper->connector_count + 1),
>> GFP_KERNEL);
>> @@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct
>> drm_fb_helper *fb_helper,
>> struct drm_fb_helper_connector *fb_helper_connector;
>> int i, j;
>>
>> +if (!drm_fbdev_emulation)
>> +return 0;
>> +
>> WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>>
>> for (i = 0; i < fb_helper->connector_count; i++) {
>> @@ -375,6 +389,9 @@ int
>> drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
>> *fb_helper)
>> bool do_delayed
>> int ret;
>>
>> +if (!drm_fbdev_emulation)
>> +return -ENODEV;
>> +
>> drm_modeset_lock_all(dev);
>> ret = restore_fbdev_mode(fb_helper);
>>
>> @@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
>> struct drm_crtc *crtc;
>> int i;
>>
>> +if (!drm_fbdev_emulation)
>> +return 0;
>> +
>> if (!max_conn_count)
>> return -EINVAL;
>>
>> @@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
>>
>> void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>> {
>> +if (!drm_fbdev_emulation)
>> +return;
>> +
>> if (!list_empty(&fb_helper->kernel_fb_list)) {
>> list_del(&fb_helper->kernel_fb_list);
>> if (list_empty(&kernel_fb_helper_list)) {
>> @@ -1933,6 +1956,9 @@ int drm_fb_helper_initial_config(struct
>> drm_fb_helper *fb_helper, int bpp_sel)
>> struct drm_device *dev = fb_helper->dev;
>> int count = 0;
>>
>> +if (!drm_fbdev_emulation)
>> +return 0;
>> +
>> mutex_lock(&dev->mode_config.mutex);
>> count = drm_fb_helper_probe_connector_modes(fb_helper,
>> dev->mode_config.max_w
[Intel-gfx] [PATCH 4/5] Documentation: drm: Convert KMS Properties HTML table to CALS
On Tue, Aug 25, 2015 at 05:10:54PM +0100, Graham Whaley wrote:
> On Tue, 2015-08-25 at 16:29 +0200, Daniel Vetter wrote:
> > On Tue, Aug 25, 2015 at 10:26:44AM +0100, Graham Whaley wrote:
> > > The KMS Properties table is in HTML format, which is not supported
> > > for building pdfdocs, resulting in the following types of errors:
> > >
> > > jade:/Documentation/DocBook/drm.xml:34413:15:E: there is no
> > > attribute
> > > "border"
> > > jade:/Documentation/DocBook/drm.xml:34413:31:E: there is no
> > > attribute
> > > "cellpadding"
> > > jade:/Documentation/DocBook/drm.xml:34413:47:E: there is no
> > > attribute
> > > "cellspacing"
> > > jade:/Documentation/DocBook/drm.xml:34414:7:E: document type does
> > > not
> > > allow element "tbody" here
> > >
> > > Convert the table over to a CALS format table
> >
> > Hm, long-term plan was to move this table into DOC: comments in the
> > source-code using markdown, which we now have (at least in
> > drm-intel-nightly and also planned to be merged into 4.4). Since this
> > is
> > both a lot of churn I'd like to get there in just 1 step ...
> > -Daniel
> First - I've just noted an erroneous debug comment (or two) left in
> this patch as well, so looks like I will have to re-issue the series
> anyway.
>
> OK. I guess this comes down to a matter of timing...
> From Danilos patch of: f6d6913 (drm/doc: Convert to markdown)
> we can see markdown does not natively support tables, and we'd have to
> make this a fixed width layout like the one in that patch I suspect.
> Danilo - any advice on how you did that other table conversion? I just
> did a pandoc docbook->markdown_github and it looks some way there - but
> of course seems to have not honored the multi-column items, of which
> there are a few. It's probably not too bad to fix up by hand - I'll see
> if I can get that to work...
> Any clue to where in the source file DOC: sections it would want to
> live if I get it formatted - I don't see an obvious !include near the
> table in the .tmpl file to piggyback.
Yeah it's a bit more involved and probably a bigger series. We might even
want to split up this table into per-feature stuff since the current one
doesn't really scale. Otoh that can be done later on.
What I'd do is split it into sections (generic, i915, ...) so that we can
avoid the row-spanning as much as possible, and then just place it
somewhere into drm_crtc.c - that's the grab-bag for all things modeset
(which properties are a part of). Maybe longer-term we want to split out a
drm_properties.c or something like that with the pile of support code we
have already, plus this kerneldoc.
-Daniel
>
> Graham
> >
> > >
> > > Signed-off-by: Graham Whaley
> > > ---
> > > Documentation/DocBook/drm.tmpl | 1866
> > >
> > > 1 file changed, 937 insertions(+), 929 deletions(-)
> > >
> > > diff --git a/Documentation/DocBook/drm.tmpl
> > > b/Documentation/DocBook/drm.tmpl
> > > index 2e05a79..e5bfdd8 100644
> > > --- a/Documentation/DocBook/drm.tmpl
> > > +++ b/Documentation/DocBook/drm.tmpl
> > > @@ -2580,935 +2580,943 @@ void intel_crt_init(struct drm_device
> > > *dev)
> > >and an initial instance value.
> > >
> > >
> > > - Existing KMS Properties
> > > -
> > > - The following table gives description of drm properties
> > > exposed by various
> > > - modules/drivers.
> > > -
> > > -
> > > -
> > > -
> > > - Owner Module/Drivers
> > > - Group
> > > - Property Name
> > > - Type
> > > - Property Values
> > > - Object attached
> > > - Description/Restrictions
> > > -
> > > -
> > > - DRM
> > > - Generic
> > > - ârotationâ
> > > - BITMASK
> > > - { 0, "rotate-0" },
> > > - { 1, "rotate-90" },
> > > - { 2, "rotate-180" },
> > > - { 3, "rotate-270" },
> > > - { 4, "reflect-x" },
> > > - { 5, "reflect-y" }
> > > - CRTC, Plane
> > > - rotate-(degrees) rotates the image by
> > > the specified amount in degrees
> > > - in counter clockwise direction. reflect-x and reflect-y
> > > reflects the
> > > - image along the specified axis prior to rotation
> > > -
> > > -
> > > - Connector
> > > - âEDIDâ
> > > - BLOB | IMMUTABLE
> > > - 0
> > > - Connector
> > > - Contains id of edid blob ptr
> > > object.
> > > -
> > > -
> > > - âDPMSâ
> > > - ENUM
> > > - { âOnâ, âStandbyâ, âSuspendâ, âOffâ
> > > }
> > > - Connector
> > > - Contains DPMS operation mode value.
> > > -
> > > -
> > > - âPATHâ
> > > - BLOB | IMMUTABLE
> > > - 0
> > > - Connector
> > > - Contains topology path to a
> > > connector.
> > > -
> > > -
> > > - âTILEâ
> > > - BLOB | IMMUTABLE
> > > - 0
> > > - Connector
> > > - Contains tiling information for a
> > > connector.
> > > -
> > > -
> > > - âCRTC_IDâ
> > > - OBJECT
> > > - DRM_MODE_OBJECT_CRTC
> > > - Connector
> > > - CRTC that connector is attached to
> > > (atomic)
> > > -
> > > -
> > > - Plane
> > > - âtypeâ
> > > - ENUM | IMMUTABLE
> > > - { "Overlay", "Primary", "Cursor" }
> >
[PATCH v2 1/7] drm/vc4: Add devicetree bindings for VC4.
On Tue, Aug 18, 2015 at 02:54:02PM -0700, Eric Anholt wrote: > VC4 is the GPU (display and 3D) subsystem present on the 2835 and some > other Broadcom SoCs. > ... > +Broadcom VC4 GPU > + > +The VC4 device present on the Raspberry Pi includes a display system > +with HDMI output and the HVS scaler for compositing display planes. Is this a direct hw driver implementation, banging registers, or is this talking to the os/firmware/binary blob running on the videocore dsp? Luc Verhaegen.
[RFC 04/13] drm/dp: Enhanced framing capability is DP 1.1+
On Wed, 12 Aug 2015, Thierry Reding wrote: > From: Thierry Reding > > The enhanced framing capability was added in DisplayPort 1.1, so any > code dealing with it needs to be protected accordingly. > > Signed-off-by: Thierry Reding > --- > drivers/gpu/drm/drm_dp_helper.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 45701c650a5d..dcfd6898aebe 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -308,8 +308,9 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct > drm_dp_link *link) > link->rate = drm_dp_bw_code_to_link_rate(values[1]); > link->num_lanes = values[2] & DP_MAX_LANE_COUNT_MASK; > > - if (values[2] & DP_ENHANCED_FRAME_CAP) > - link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING; > + if (link->revision >= 0x11) > + if (values[2] & DP_ENHANCED_FRAME_CAP) > + link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING; Oh, and perhaps the conditions should be encoded into helpers (of helpers!) in drm_dp_helper.h. There's already drm_dp_enhanced_frame_cap that checks DPCD REV. BR, Jani. > > if (link->revision >= 0x12) > if (values[2] & DP_TPS3_SUPPORTED) > -- > 2.4.5 > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Jani Nikula, Intel Open Source Technology Center
[PATCH] drm/crtc: Add a helper func to get a registered crtc from its index
On Tue, Aug 25, 2015 at 11:36:18AM +0200, Daniel Vetter wrote:
> On Tue, Aug 25, 2015 at 11:13:51AM +0800, Xinliang Liu wrote:
> > This patch add a helper func to get a registered crtc from its index.
> > In some case, where we know the crtc's index and we want to know the
> > crtc too.
> >
> > For example, the enable_vblank func of struct drm_driver:
> > In the implementation of this func, we know the index of the crtc but
> > we want to know the crtc. This helper func can get the crtc easily.
> > A sample impelmentation of enable_vblank is as shown as bellow:
> >
> > int hisi_drm_crtc_enable_vblank(struct drm_device *dev, int c)
> > {
> > struct drm_crtc *crtc = drm_get_crtc_from_index(dev, c);
> > struct hisi_crtc *hcrtc = to_hisi_crtc(crtc);
> > struct hisi_crtc_ops *ops = hcrtc->ops;
> > int ret = 0;
> >
> > if (ops->enable_vblank)
> > ret = ops->enable_vblank(hcrtc);
> >
> > return ret;
> > }
> >
> > Signed-off-by: Xinliang Liu
>
> Yeah unfortunately drm_irq.c is still stick in the old pre-KMS days. I
> think we should go a bit further here though to allow new drivers to be
> completely free of int pipe:
Of course you meant to say /unsigned/ int pipe =)
> - add a new array pointer dev->mode_conifg.crtc_arr, which is
> (re-)allocated in drm_crtc_init_with_planes. Then a pipe->crtc lookup
> will be just
>
> crtc = dev->mode_config.crtc_arr[pipe];
>
> - add new hooks for vblank handling int drm_crtc_helper_funcs for
> enable_vblanke, disable_vblank, get_vblank_timestamp and get_scanoutpos.
> Ofc also anotate the docs for the existing hooks and make it clear new
> drivers should use the new ones. Ofc these new hooks should directly
> take a struct drm_crtc * instead of inte pipe.
I have a couple patches to address this partially, which came about as a
result of the int crtc/crtc_id/pipe/whatever -> unsigned int pipe
conversion work that I've been doing.
> - change the code in drm_irq.c to wrap all callbacks and first check
> whether the new ones are there and only if that's not the case call the
> old ones.
>
> With these changes drivers can be completely free of int pipe and use
> struct drm_crtc exclusivly I think, and the mess would be fully restricted
> to drm_irq.c.
I like the idea of moving the callbacks to drm_crtc_helper_funcs. That
allows us to introduce this step by step, without a flag date when every
driver needs to switch the drm_driver functions over.
Thierry
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/dri-devel/attachments/20150826/b4ba366e/attachment.sig>
[PATCH 1/3] drm/dp: add drm_dp_tps3_supported helper
Cc: Thierry Reding
Signed-off-by: Jani Nikula
---
include/drm/drm_dp_helper.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 499e9f625aef..8c52d0ef1fc9 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -634,6 +634,13 @@ drm_dp_enhanced_frame_cap(const u8
dpcd[DP_RECEIVER_CAP_SIZE])
(dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP);
}
+static inline bool
+drm_dp_tps3_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+ return dpcd[DP_DPCD_REV] >= 0x12 &&
+ dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED;
+}
+
/*
* DisplayPort AUX channel
*/
--
2.1.4
[PATCH 2/3] drm/i915/dp: use the drm dp helper for determining sink tps3 support
No functional changes.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_dp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f10a4724b841..60dca34d2f0f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3940,8 +3940,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
* SKL < B0: due it's WaDisableHBR2 is the only exception where TP3 is
* supported but still not enabled.
*/
- if (intel_dp->dpcd[DP_DPCD_REV] >= 0x12 &&
- intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED &&
+ if (drm_dp_tps3_supported(intel_dp->dpcd) &&
intel_dp_source_supports_hbr2(dev)) {
intel_dp->use_tps3 = true;
DRM_DEBUG_KMS("Displayport TPS3 supported\n");
--
2.1.4
[PATCH 3/3] drm/i915/dp: move TPS3 logic to where it's used
There is no need to have a separate flag for tps3 as the information is
only used at one location. Move the logic there to make it easier to
follow.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_dp.c | 31 +--
drivers/gpu/drm/i915/intel_drv.h | 1 -
2 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 60dca34d2f0f..cc97d5d7fe5e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3746,13 +3746,25 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
void
intel_dp_complete_link_train(struct intel_dp *intel_dp)
{
+ struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+ struct drm_device *dev = dig_port->base.base.dev;
bool channel_eq = false;
int tries, cr_tries;
uint32_t DP = intel_dp->DP;
uint32_t training_pattern = DP_TRAINING_PATTERN_2;
- /* Training Pattern 3 for HBR2 or 1.2 devices that support it*/
- if (intel_dp->link_rate == 54 || intel_dp->use_tps3)
+ /*
+* Training Pattern 3 for HBR2 or 1.2 devices that support it.
+*
+* Intel platforms that support HBR2 also support TPS3. TPS3 support is
+* also mandatory for downstream devices that support HBR2.
+*
+* Due to WaDisableHBR2 SKL < B0 is the only exception where TPS3 is
+* supported but still not enabled.
+*/
+ if (intel_dp->link_rate == 54 ||
+ (intel_dp_source_supports_hbr2(dev) &&
+drm_dp_tps3_supported(intel_dp->dpcd)))
training_pattern = DP_TRAINING_PATTERN_3;
/* channel equalization */
@@ -3934,18 +3946,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
}
}
- /* Training Pattern 3 support, Intel platforms that support HBR2 alone
-* have support for TP3 hence that check is used along with dpcd check
-* to ensure TP3 can be enabled.
-* SKL < B0: due it's WaDisableHBR2 is the only exception where TP3 is
-* supported but still not enabled.
-*/
- if (drm_dp_tps3_supported(intel_dp->dpcd) &&
- intel_dp_source_supports_hbr2(dev)) {
- intel_dp->use_tps3 = true;
- DRM_DEBUG_KMS("Displayport TPS3 supported\n");
- } else
- intel_dp->use_tps3 = false;
+ DRM_DEBUG_KMS("Display Port TPS3 support: source %s, sink %s\n",
+ intel_dp_source_supports_hbr2(dev) ? "yes" : "no",
+ drm_dp_tps3_supported(intel_dp->dpcd) ? "yes" : "no");
/* Intermediate frequency support */
if (is_edp(intel_dp) &&
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 71a2e18d9f50..1de68b2cdae6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -743,7 +743,6 @@ struct intel_dp {
enum pipe pps_pipe;
struct edp_power_seq pps_delays;
- bool use_tps3;
bool can_mst; /* this port supports mst */
bool is_mst;
int active_mst_links;
--
2.1.4
[PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation
On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote:
>
>
> On 08/26/2015 10:42 AM, Archit Taneja wrote:
> >
> >
> >On 08/25/2015 07:15 PM, Daniel Vetter wrote:
> >>Faster than recompiling.
> >>
> >>Note that restore_fbdev_mode_unlocked is a bit special and the only
> >>one which returns an error code when fbdev isn't there - i915 needs
> >>that one to not fall over with some additional fbcon related restore
> >>code. Everyone else just ignores the return value or only prints a
> >>DRM_DEBUG level message.
> >
> >Reviewed-by:Archit Taneja
>
>
> With the module param, and the drivers should see the following state(
> based on the truth table below):
>
> module param | config option
>true |true -> real helper funcs called, driver
> allocated drm_fb_helper is correctly
> populated.
>
>false |true -> real helper funcs called, but bailed
> out early, driver allocated
> drm_fb_helper is non-NULL, but we won't
> end up using it.
Hm I tried to give drivers the same semantics here as with the stub
functions. Where did I screw up? The goal really was to match the end
result for drivers ...
-Daniel
> X|false -> stub functions called, drm_fb_helper is
> NULL
>
> I wonder if the 2nd combination could leave space for errors in other
> drivers. Drivers tend to check whether the fb_helper struct is NULL
> or not, and make decisions based on that. If the decisions are to
> just call a drm_fb_helper function, then we're okay. If they do something
> more than that, then we could have an issue.
>
> I'll prepare the remainder of 'Step 2' part of the series over this,
> and we can test to check if we don't hit any corner cases.
>
> Archit
>
> >
> >>
> >>Signed-off-by: Daniel Vetter
> >>---
> >> drivers/gpu/drm/drm_fb_helper.c | 29 +
> >> 1 file changed, 29 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/drm_fb_helper.c
> >>b/drivers/gpu/drm/drm_fb_helper.c
> >>index 83aacb0cc9df..8259dec1de1f 100644
> >>--- a/drivers/gpu/drm/drm_fb_helper.c
> >>+++ b/drivers/gpu/drm/drm_fb_helper.c
> >>@@ -39,6 +39,11 @@
> >> #include
> >> #include
> >>
> >>+static bool drm_fbdev_emulation = true;
> >>+module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> >>+MODULE_PARM_DESC(fbdev_emulation,
> >>+ "Enable legacy fbdev emulation [default=true]");
> >>+
> >> static LIST_HEAD(kernel_fb_helper_list);
> >>
> >> /**
> >>@@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct
> >>drm_fb_helper *fb_helper)
> >> struct drm_connector *connector;
> >> int i;
> >>
> >>+if (!drm_fbdev_emulation)
> >>+return 0;
> >>+
> >> mutex_lock(&dev->mode_config.mutex);
> >> drm_for_each_connector(connector, dev) {
> >> struct drm_fb_helper_connector *fb_helper_connector;
> >>@@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct
> >>drm_fb_helper *fb_helper, struct drm_
> >> struct drm_fb_helper_connector **temp;
> >> struct drm_fb_helper_connector *fb_helper_connector;
> >>
> >>+if (!drm_fbdev_emulation)
> >>+return 0;
> >>+
> >> WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
> >> if (fb_helper->connector_count + 1 >
> >>fb_helper->connector_info_alloc_count) {
> >> temp = krealloc(fb_helper->connector_info, sizeof(struct
> >>drm_fb_helper_connector *) * (fb_helper->connector_count + 1),
> >>GFP_KERNEL);
> >>@@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct
> >>drm_fb_helper *fb_helper,
> >> struct drm_fb_helper_connector *fb_helper_connector;
> >> int i, j;
> >>
> >>+if (!drm_fbdev_emulation)
> >>+return 0;
> >>+
> >> WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
> >>
> >> for (i = 0; i < fb_helper->connector_count; i++) {
> >>@@ -375,6 +389,9 @@ int
> >>drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> >>*fb_helper)
> >> bool do_delayed
> >> int ret;
> >>
> >>+if (!drm_fbdev_emulation)
> >>+return -ENODEV;
> >>+
> >> drm_modeset_lock_all(dev);
> >> ret = restore_fbdev_mode(fb_helper);
> >>
> >>@@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
> >> struct drm_crtc *crtc;
> >> int i;
> >>
> >>+if (!drm_fbdev_emulation)
> >>+return 0;
> >>+
> >> if (!max_conn_count)
> >> return -EINVAL;
> >>
> >>@@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
> >>
> >> void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> >> {
> >>+if (!drm_fbdev_emulation)
> >>+return;
> >>+
> >> if (!list_empty(&fb_helper->kernel_fb_list)) {
> >> list_del(&fb_helper->kernel_fb_list);
> >> if (list_empty(&kernel_fb_helper_list)) {
>
[PATCH] drm/fb-helper: Use -errno return in restore_mode_unlocked
On Tue, Aug 25, 2015 at 03:20:02PM -0400, Rob Clark wrote:
> On Tue, Aug 25, 2015 at 11:20 AM, Daniel Vetter
> wrote:
> > Using bool and returning true upon error is very uncommon. Also an int
> > return value is actually what all the callers which did check it seem
> > to have expected.
> >
> > v2: Restore hunk misplaced in a rebase, spotted by Rob.
> >
> > Cc: Rob Clark
> > Signed-off-by: Daniel Vetter
>
> Reviewed-by: Rob Clark
Merged the first 2 patches from this series to drm-misc, thanks for your
review.
-Daniel
>
>
> > ---
> > drivers/gpu/drm/drm_fb_helper.c | 19 +++
> > include/drm/drm_fb_helper.h | 6 +++---
> > 2 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c
> > index 418d299f3b12..859134e0d72d 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
> > }
> > EXPORT_SYMBOL(drm_fb_helper_debug_leave);
> >
> > -static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> > +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> > {
> > struct drm_device *dev = fb_helper->dev;
> > struct drm_plane *plane;
> > - bool error = false;
> > int i;
> >
> > drm_warn_on_modeset_not_all_locked(dev);
> > @@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper
> > *fb_helper)
> > if (crtc->funcs->cursor_set) {
> > ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
> > if (ret)
> > - error = true;
> > + return ret;
> > }
> >
> > ret = drm_mode_set_config_internal(mode_set);
> > if (ret)
> > - error = true;
> > + return ret;
> > }
> > - return error;
> > +
> > + return 0;
> > }
> >
> > /**
> > @@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper
> > *fb_helper)
> > * This should be called from driver's drm ->lastclose callback
> > * when implementing an fbcon on top of kms using this helper. This
> > ensures that
> > * the user isn't greeted with a black screen when e.g. X dies.
> > + *
> > + * RETURNS:
> > + * Zero if everything went ok, negative error code otherwise.
> > */
> > -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> > *fb_helper)
> > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> > *fb_helper)
> > {
> > struct drm_device *dev = fb_helper->dev;
> > - bool ret;
> > - bool do_delayed = false;
> > + bool do_delayed;
> > + int ret;
> >
> > drm_modeset_lock_all(dev);
> > ret = restore_fbdev_mode(fb_helper);
> > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> > index dbab4622b58f..67de1f10008e 100644
> > --- a/include/drm/drm_fb_helper.h
> > +++ b/include/drm/drm_fb_helper.h
> > @@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info);
> > int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > struct fb_info *info);
> >
> > -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> > *fb_helper);
> > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> > *fb_helper);
> >
> > struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper);
> > void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper);
> > @@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct
> > fb_var_screeninfo *var,
> > return 0;
> > }
> >
> > -static inline bool
> > +static inline int
> > drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> > {
> > - return true;
> > + return 0;
> > }
> >
> > static inline struct fb_info *
> > --
> > 1.8.3.1
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation
On Wed, Aug 26, 2015 at 01:34:58PM +0200, Daniel Vetter wrote: > On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote: > > > > > > On 08/26/2015 10:42 AM, Archit Taneja wrote: > > > > > > > > >On 08/25/2015 07:15 PM, Daniel Vetter wrote: > > >>Faster than recompiling. > > >> > > >>Note that restore_fbdev_mode_unlocked is a bit special and the only > > >>one which returns an error code when fbdev isn't there - i915 needs > > >>that one to not fall over with some additional fbcon related restore > > >>code. Everyone else just ignores the return value or only prints a > > >>DRM_DEBUG level message. > > > > > >Reviewed-by:Archit Taneja > > > > > > With the module param, and the drivers should see the following state( > > based on the truth table below): > > > > module param | config option > >true |true -> real helper funcs called, driver > > allocated drm_fb_helper is correctly > > populated. > > > >false |true -> real helper funcs called, but bailed > > out early, driver allocated > > drm_fb_helper is non-NULL, but we won't > > end up using it. > > Hm I tried to give drivers the same semantics here as with the stub > functions. Where did I screw up? The goal really was to match the end > result for drivers ... Note that at least for i915 we can't make it perfectly equal since i915 compiles out a few more things with FBDEV_EMULATION=n than just the stubs. Long-term we might want to push some of that into helpers too perhaps. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed
On Wed, Aug 26, 2015 at 12:11:56PM +0100, simon.farnsworth at onelan.com wrote:
> On Tuesday 25 August 2015 18:10:14 ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä
> >
> > Calculate the number of retries we should do for each i2c-over-aux
> > message based on the time it takes to perform the i2c transfer vs. the
> > aux transfer.
> >
> > This mostly matches what the DP spec recommends. The numbers didn't come
> > out exactly the same as the tables in the spec, but I think there's
> > something fishy about the AUX trasnfer size calculations in the spec.
> > Also the way the i2c transfer length is calculated is somewhat open to
> > interpretation.
> >
> > Note that currently we assume 10 kHz speed for the i2c bus. Some real
> > world devices (eg. some Apple DP->VGA dongle) fails with less than 16
> > retries, and that would correspond to something close to 20 kHz, so we
> > assume 10 kHz to be on the safe side. Ideally we should query/set the
> > i2c bus speed via DPCD but for now this should at leaast remove the
> > regression from the 1->16 byte trasnfer size change. And of course if
> > the sink completes the transfer quicker this shouldn't slow things down
> > since we don't change the interval between retries.
> >
> > Fixes a regression with some DP dongles from:
> > commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
> > Author: Simon Farnsworth
> > Date: Tue Feb 10 18:38:08 2015 +
> >
> > drm/dp: Use large transactions for I2C over AUX
> >
> > Cc: Simon Farnsworth
> > Cc: moosotc at gmail.com
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/drm_dp_helper.c | 64
> > +++--
> > 1 file changed, 62 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index 80a02a4..2b6b7f9 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -422,6 +422,61 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter
> > *adapter)
> >I2C_FUNC_10BIT_ADDR;
> > }
> >
> > +#define AUX_PRECHARGE_LEN 16 /* 10 to 16 */
>
> I'd change this to 10 - see below for reasoning.
>
> > +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
> > +#define AUX_STOP_LEN 4
> > +#define AUX_CMD_LEN 4
> > +#define AUX_ADDRESS_LEN 20
> > +#define AUX_REPLY_PAD_LEN 4
> > +#define AUX_LENGTH_LEN 8
> > +
> > +#define AUX_RESPONSE_TIMEOUT 300
> > +
> > +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
> > +{
> > + int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > + AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
> > +
> > + if ((msg->request & DP_AUX_I2C_READ) == 0)
> > + len += msg->size * 8;
> > +
> > + return len;
> > +}
> > +
> > +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
> > +{
> > + int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > + AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
> > +
> > + if (msg->request & DP_AUX_I2C_READ)
> > + len += msg->size * 8;
> > + else
> > + len += 8; /* 0 or 1 data bytes for write reply */
> > +
> > + return len;
> > +}
> > +
> > +#define I2C_START_LEN 1
> > +#define I2C_STOP_LEN 1
> > +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
> > +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
> > +
> > +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
> > + int i2c_speed_khz)
> > +{
> > + return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
> > + I2C_STOP_LEN) * 1000 / i2c_speed_khz;
> > +}
> > +
>
> This function caught me out at first - because you're not tracking state
> between two AUX transactions, you assume that every DP AUX transaction is
> mapped to a single I2C transaction. This then gets you the worst case timings
> for the I2C transaction.
Yeah I did consider making it more accurate but then thought it's easier
to just assume worst case behaviour.
>
> > +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
> > + int i2c_speed_khz)
> > +{
> > + int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg) +
> > AUX_RESPONSE_TIMEOUT;
> > + int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz) -
> > AUX_RESPONSE_TIMEOUT;
> > +
> > + return DIV_ROUND_UP(i2c_len, aux_len);
> > +}
> > +
>
> The logic here looks wrong to me - aux_len is the worst case time for a
> single AUX transaction, while I'd expect it to be the best case time (as it's
> the divisor). Similarly, I'd expect i2c_len to be the worst case time for the
> I2C transaction, but then you subtract a response timeout from it.
>
> I'd change AUX_PRECHARGE_LEN to 10, so that drm_dp_aux_req_len and
> drm_dp_aux_reply_len return best case times for the AUX transaction. Then
> this function becomes:
>
> static int drm_dp_i2c_retry_count(c
[PATCH 2/4] drm/fb-helper: atomic restore_fbdev_mode()..
On Tue, Aug 25, 2015 at 03:35:58PM -0400, Rob Clark wrote:
> Add support for using atomic code-paths for restore_fbdev_mode().
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 131
> +---
> drivers/gpu/drm/drm_fb_helper.c | 74
> include/drm/drm_atomic_helper.h | 6 ++
> include/drm/drm_fb_helper.h | 8 +++
> 4 files changed, 166 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index d432348..dbce582 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1502,21 +1502,9 @@ retry:
> goto fail;
> }
>
> - ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> + ret = __drm_atomic_helper_disable_plane(plane, plane_state);
> if (ret != 0)
> goto fail;
> - drm_atomic_set_fb_for_plane(plane_state, NULL);
> - plane_state->crtc_x = 0;
> - plane_state->crtc_y = 0;
> - plane_state->crtc_h = 0;
> - plane_state->crtc_w = 0;
> - plane_state->src_x = 0;
> - plane_state->src_y = 0;
> - plane_state->src_h = 0;
> - plane_state->src_w = 0;
> -
> - if (plane == plane->crtc->cursor)
> - state->legacy_cursor_update = true;
>
> ret = drm_atomic_commit(state);
> if (ret != 0)
> @@ -1546,6 +1534,32 @@ backoff:
> }
> EXPORT_SYMBOL(drm_atomic_helper_disable_plane);
>
> +/* just used from fb-helper and atomic-helper: */
> +int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
> + struct drm_plane_state *plane_state)
> +{
> + int ret;
> +
> + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> + if (ret != 0)
> + return ret;
> +
> + drm_atomic_set_fb_for_plane(plane_state, NULL);
> + plane_state->crtc_x = 0;
> + plane_state->crtc_y = 0;
> + plane_state->crtc_h = 0;
> + plane_state->crtc_w = 0;
> + plane_state->src_x = 0;
> + plane_state->src_y = 0;
> + plane_state->src_h = 0;
> + plane_state->src_w = 0;
> +
> + if (plane->crtc && (plane == plane->crtc->cursor))
> + plane_state->state->legacy_cursor_update = true;
> +
> + return 0;
> +}
> +
> static int update_output_state(struct drm_atomic_state *state,
> struct drm_mode_set *set)
> {
> @@ -1629,8 +1643,6 @@ int drm_atomic_helper_set_config(struct drm_mode_set
> *set)
> {
> struct drm_atomic_state *state;
> struct drm_crtc *crtc = set->crtc;
> - struct drm_crtc_state *crtc_state;
> - struct drm_plane_state *primary_state;
> int ret = 0;
>
> state = drm_atomic_state_alloc(crtc->dev);
> @@ -1639,17 +1651,54 @@ int drm_atomic_helper_set_config(struct drm_mode_set
> *set)
>
> state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> retry:
> - crtc_state = drm_atomic_get_crtc_state(state, crtc);
> - if (IS_ERR(crtc_state)) {
> - ret = PTR_ERR(crtc_state);
> + ret = __drm_atomic_helper_set_config(set, state);
> + if (ret != 0)
> goto fail;
> - }
>
> - primary_state = drm_atomic_get_plane_state(state, crtc->primary);
> - if (IS_ERR(primary_state)) {
> - ret = PTR_ERR(primary_state);
> + ret = drm_atomic_commit(state);
> + if (ret != 0)
> goto fail;
> - }
> +
> + /* Driver takes ownership of state on successful commit. */
> + return 0;
> +fail:
> + if (ret == -EDEADLK)
> + goto backoff;
> +
> + drm_atomic_state_free(state);
> +
> + return ret;
> +backoff:
> + drm_atomic_state_clear(state);
> + drm_atomic_legacy_backoff(state);
> +
> + /*
> + * Someone might have exchanged the framebuffer while we dropped locks
> + * in the backoff code. We need to fix up the fb refcount tracking the
> + * core does for us.
> + */
> + crtc->primary->old_fb = crtc->primary->fb;
> +
> + goto retry;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_set_config);
> +
> +/* just used from fb-helper and atomic-helper: */
> +int __drm_atomic_helper_set_config(struct drm_mode_set *set,
> + struct drm_atomic_state *state)
> +{
> + struct drm_crtc_state *crtc_state;
> + struct drm_plane_state *primary_state;
> + struct drm_crtc *crtc = set->crtc;
> + int ret;
> +
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> +
> + primary_state = drm_atomic_get_plane_state(state, crtc->primary);
> + if (IS_ERR(primary_state))
> + return PTR_ERR(primary_state);
>
> if (!set->mode) {
> WARN_ON(set->fb);
> @@ -1657,13 +1706,13 @@ retry:
>
> ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> if (ret != 0)
> - goto fail;
> + r
[PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.
On Tue, Aug 25, 2015 at 04:42:18PM -0400, Rob Clark wrote: > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring wrote: > > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt wrote: > >> Stephen Warren writes: > >> > >>> On 08/12/2015 06:56 PM, Eric Anholt wrote: > Signed-off-by: Eric Anholt > >>> > >>> This one definitely needs a patch description, since someone might not > >>> know what a VC4 is, and "git log" won't show the text from the binding > >>> doc itself. I'd suggest adding the initial paragraph of the binding doc > >>> as the patch description, or more. > >>> > diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > >> > +- hvss: List of references to HVS video scalers > +- encoders: List of references to output encoders (HDMI, SDTV) > >>> > >>> Would it make sense to make all those nodes child node of the vc4 > >>> object. That way, there's no need to have these lists of objects; they > >>> can be automatically built up as the DT is enumerated. I know that e.g. > >>> the NVIDIA Tegra host1x binding works this way, and I think it may have > >>> been inspired by other similar cases. > >> > >> I've looked at tegra, and the component system used by msm appears to be > >> nicer than it. To follow tegra's model, it looks like I need to build > >> this extra bus thing corresponding to host1x that is effectively the > >> drivers/base/component.c code, so that I can get at vc4's structure from > >> the component drivers. > >> > >>> Of course, this is only appropriate if the HW modules really are > >>> logically children of the VC4 HW module. Perhaps they aren't. If they > >>> aren't though, I wonder what this "vc4" module actually represents in HW? > >> > >> It's the subsystem, same as we use a subsystem node for msm, sti, > >> rockchip, imx, and exynos. This appears to be the common model of how > >> the collection of graphics-related components is represented in the DT. > > > > I think most of these bindings are wrong. They are grouped together > > because that is what DRM wants not because that reflects the h/w. So > > convince me this is one block, not that it is what other people do. > > I think, when it comes to more complex driver subsystems (like drm in > particular) we have a bit of mismatch between how things look from the > "pure hw ignoring sw" perspective, and the "how sw and in particular > userspace expects things" perspective. Maybe it is less a problem in > other subsystems, where bindings map to things that are only visible > in the kernel, or well defined devices like uart or sata controller. > But when given the choice, I'm going to err on the side of not > confusing userspace and the large software stack that sits above > drm/kms, over dt purity. > > Maybe it would be nice to have a sort of dt overlay that adds the bits > needed to tie together hw blocks that should be assembled into a > single logical device for linux and userspace (but maybe not some > other hypothetical operating system). But so far that doesn't exist. > All we have is a hammer (devicetree), everything looks like a nail. > End result is we end up adding some things in the bindings which > aren't purely about the hw. Until someone invents a screwdriver, I'm > not sure what else to do. In the end, other hypothetical OS is free > to ignore those extra fields in the bindings if it doesn't need them. > So meh? I thought we agreed a while back that these kind of "pull everything for the logical device together" dt nodes which just have piles of phandles are totally accepted? At least that's the point behind the component helpers, and Eric even suggested to create dt-specific component helpers to cut down a bit on the usual boilerplate. dt maintainers are also fine with this approach afaik. From my understanding tegra with the host1x bus really is the odd one out and not the norm. Given that and with the hope that we'll eventually see a dt-enabled component functions to standardize this even more the overall concept is Acked-by: Daniel Vetter Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.
On Fri, Aug 14, 2015 at 10:38:54PM -0600, Stephen Warren wrote: > On 08/12/2015 06:56 PM, Eric Anholt wrote: > > Signed-off-by: Eric Anholt > > This one definitely needs a patch description, since someone might not > know what a VC4 is, and "git log" won't show the text from the binding > doc itself. I'd suggest adding the initial paragraph of the binding doc > as the patch description, or more. > > > diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > > b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > > > +Required properties for VC4: > > +- compatible: Should be "brcm,vc4" > > +- crtcs: List of references to pixelvalve scanout engines > > s/references to/phandles of/ would be more typical DT language. > > > +- hvss:List of references to HVS video scalers > > +- encoders:List of references to output encoders (HDMI, SDTV) > > Would it make sense to make all those nodes child node of the vc4 > object. That way, there's no need to have these lists of objects; they > can be automatically built up as the DT is enumerated. I know that e.g. > the NVIDIA Tegra host1x binding works this way, and I think it may have > been inspired by other similar cases. Actually the host1x binding was the first of its kind. Unfortunately for the purposes of this discussion (but fortunately otherwise) Tegra is the odd-ball it seems. host1x is indeed a physical parent of all the devices pertaining to the DRM driver, so the DT description is accurate from a hardware point of view while at the same time giving us a top-level device that we can bind against. Now for most other cases it seems like the central piece that they are missing is this top-level device, hence why the "virtual DRM subsystem device" is instantiated. I tried to argue in the past that it wasn't a proper description and proposed alternatives, but I was always pretty much the only one with this viewpoint, so my comments ended up being ignored. Technically there is nothing that would prevent other drivers from doing without the lists of phandles. On Tegra, again this might be special for this particular hardware, we've never had a need to describe these kinds of relationships. Each display controller can essentially drive each of the outputs, which we deal with elegantly by setting the .possible_crtcs mask of the encoders. Also, to pull together all devices that are needed to make up the DRM device, we use a list of compatible strings in the driver to find these devices. Then as each of them registers with the host1x bus we wait for the subdevice list to become empty and ->probe() the component host1x device. Note that while this predates component/master, this is all very similar in principle (Russell and I did have some discussions about this back at the time, but I'm not sure how much, if anything, he took as inspiration from the host1x infrastructure). After component/master was merged I did try to convert Tegra DRM to use it. Things looked pretty good, but ended up not working because each componentized device must have a unique master device. This poses a problem because on Tegra we needed the top- level (i.e. master) device to be shared among multiple drivers. I posted patches at some point to try and fix remedy the situation but wasn't able to elicit any reactions, and since I had something that was working did not pursue this any further. Thierry -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150826/6ac61672/attachment.sig>
[PATCH 2/3] drm:msm: Initial Add Writeback Support (V2)
On Tue, Aug 25, 2015 at 9:11 PM, Rob Clark wrote: > On Tue, Aug 25, 2015 at 3:05 AM, Daniel Vetter wrote: >> On Wed, Aug 19, 2015 at 03:00:04PM -0400, Rob Clark wrote: >>> On Tue, Apr 7, 2015 at 2:09 PM, Jilai Wang wrote: >>> So one thing that I wanted sorting out before we let userspace see >>> streaming writeback (where I do think v4l is the right interface), is >>> a way to deal w/ permissions/security.. Ie. only the kms master >>> should control access to writeback. Ie. an process that the >>> compositor isn't aware of / doesn't trust, should not be able to open >>> the v4l device and start snooping on the screen contents. And I don't >>> think just file permissions in /dev is sufficient. You likely don't >>> want to run your helper process doing video encode and streaming as a >>> privilaged user. >>> >>> One way to handle this would be some sort of dri2 style >>> getmagic/authmagic sort of interface between the drm/kms master, and >>> v4l device, to unlock streaming. But that is kind of passe. Fd >>> passing is the fashionable thing now. So instead we could use a dummy >>> v4l2_file_opererations::open() which always returns an error. So v4l >>> device shows up in /dev.. but no userspace can open it. And instead, >>> the way to get a fd for the v4l dev would be via a drm/kms ioctl (with >>> DRM_MASTER flag set). Once compositor gets the fd, it can use fd >>> passing, if needed, to hand it off to a helper process, etc. >>> >>> (probably use something like alloc_file() to get the 'struct file *', >>> then call directly into v4l2_fh_open(), and then get_unused_fd_flags() >>> + fd_install() to get fd to return to userspace) >> >> Just following up here, but consensus from the lpc track is that we don't >> need this as long as writeback is something which needs a specific action >> to initiate. I.e. if we have a separate writeback connector which won't >> grab any frames at all as long as it's disconnected we should be fine. Wrt >> session handling that's something which would need to be fixed between >> v4l and logind (or whatever). > > Was that consensus? I agree that something should initiate writeback > from the kms side of things. But if we don't have *something* to > ensure whatever is on the other end of writeback is who we think it > is, it seems at least racy.. > >> In general I don't like hand-rolling our own proprietary v4l-open process, >> it means that all the existing v4l test&dev tooling must be changed, even >> when you're root. > > well, I know that, for example, gst v4l2src allows you to pass in an > already opened v4l dev fd, which fits in pretty well with what I > propose. The alternative, I think, is a dri2 style auth handshake > between drm/kms and v4l, which I am less thrilled about. > > I would have to *assume* that userspace is at least prepared to deal > with -EPERM when it tries to open a device.. at least more so than > special auth ioctl sequence. Imo the right approach for that is to defer to logind. We already have these issues, e.g. you probably don't want the wrong seat to be able to access camera devices. Maybe no one bothered to support this yet, but the problem is definitely there already. And with that broder use-case of just making sure the right process can access the v4l device nodes drm writeback is just another special case. At least that's what I wanted to say at lpc, might have done a bad job at phrasing it ;-) DRM auth dance otoh is something entirely different and only needed because we want multiple processes to use the same device. v4l doesn't support that model yet at all, it really only needs some form of revoke + logind adjusting permissions to match whatever the current user is on a given seat. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
On Wed, Aug 26, 2015 at 08:49:00AM +0200, Thomas Hellstrom wrote: > Hi, Tiago. > > On 08/26/2015 02:02 AM, Tiago Vignatti wrote: > > From: Daniel Vetter > > > > The userspace might need some sort of cache coherency management e.g. when > > CPU > > and GPU domains are being accessed through dma-buf at the same time. To > > circumvent this problem there are begin/end coherency markers, that forward > > directly to existing dma-buf device drivers vfunc hooks. Userspace can make > > use > > of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be > > used like following: > > > > - mmap dma-buf fd > > - for each drawing/upload cycle in CPU > > 1. SYNC_START ioctl > > 2. read/write to mmap area or a 2d sub-region of it > > 3. SYNC_END ioctl. > > - munamp once you don't need the buffer any more > > > > v2 (Tiago): Fix header file type names (u64 -> __u64) > > v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end > > dma-buf functions. Check for overflows in start/length. > > v4 (Tiago): use 2d regions for sync. > > Daniel V had issues with the sync argument proposed by Daniel S. I'm > fine with that argument or an argument containing only a single sync > rect. I'm not sure whether Daniel V will find it easier to accept only a > single sync rect... I'm kinda against all the 2d rect sync proposals ;-) At least for the current stuff it's all about linear subranges afaik, and even there we don't bother with flushing them precisely right now. My expectation would be that if you _really_ want to etch out that last bit of performance with a list of 2d sync ranges then probably you want to do the cpu cache flushing in userspace anyway, with 100% machine-specific trickery. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.
and not the norm. I agree that in many aspects Tegra is somewhat special. But the same principles that the host1x infrastructure uses could be implemented in a similar way for other DRM drivers. You can easily collect information about subdevices by walking the device tree and matching on known compatible strings. And you can also instantiate the top-level device from driver code rather than have it in DT. It should still be possible to make this work without an artificial device node in DT. The component and master infrastructure is largely orthogonal to that, and as far as I remember the only blocker is the need for a top-level device. I wonder if perhaps this could be made to work by binding the master to the top- level SoC device. Obviously adding the node in DT is easier, but to my knowledge easy has never been a good excuse for mangling DT. Perhaps that's different these days... Thierry -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150826/23936e24/attachment-0001.sig>
[PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation
On 08/26/2015 05:04 PM, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote:
>>
>>
>> On 08/26/2015 10:42 AM, Archit Taneja wrote:
>>>
>>>
>>> On 08/25/2015 07:15 PM, Daniel Vetter wrote:
Faster than recompiling.
Note that restore_fbdev_mode_unlocked is a bit special and the only
one which returns an error code when fbdev isn't there - i915 needs
that one to not fall over with some additional fbcon related restore
code. Everyone else just ignores the return value or only prints a
DRM_DEBUG level message.
>>>
>>> Reviewed-by:Archit Taneja
>>
>>
>> With the module param, and the drivers should see the following state(
>> based on the truth table below):
>>
>> module param | config option
>> true |true -> real helper funcs called, driver
>> allocated drm_fb_helper is correctly
>> populated.
>>
>> false |true -> real helper funcs called, but bailed
>> out early, driver allocated
>> drm_fb_helper is non-NULL, but we won't
>> end up using it.
>
> Hm I tried to give drivers the same semantics here as with the stub
> functions. Where did I screw up? The goal really was to match the end
> result for drivers ...
Right, they would behave like the stub functions. But driver level
fbdev functions would still be called.
For example, with DRM_FBDEV_EMULATION not set, a stub
intel_fbdev_init() would be called. With DRM_FBDEV_EMULATION set but
the module param not set, the non-stub intel_fbdev_init() would still
be called, allocating an empty ifbdev. The ifbdev->helper would be
passed to the fb_helper funcs, but the module param check will bail
us out immediately.
So, the code flow isn't exactly the same as compared to when
DRM_FBDEV_EMULATION is disabled. The end result should be the same.
My only concern was whether other drivers might get confused with
a non-NULL fb_helper pointer. It most likely shouldn't be, though.
Archit
> -Daniel
>
>> X|false -> stub functions called, drm_fb_helper is
>> NULL
>>
>> I wonder if the 2nd combination could leave space for errors in other
>> drivers. Drivers tend to check whether the fb_helper struct is NULL
>> or not, and make decisions based on that. If the decisions are to
>> just call a drm_fb_helper function, then we're okay. If they do something
>> more than that, then we could have an issue.
>>
>> I'll prepare the remainder of 'Step 2' part of the series over this,
>> and we can test to check if we don't hit any corner cases.
>
>>
>> Archit
>>
>>>
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_fb_helper.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c
b/drivers/gpu/drm/drm_fb_helper.c
index 83aacb0cc9df..8259dec1de1f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -39,6 +39,11 @@
#include
#include
+static bool drm_fbdev_emulation = true;
+module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
+MODULE_PARM_DESC(fbdev_emulation,
+ "Enable legacy fbdev emulation [default=true]");
+
static LIST_HEAD(kernel_fb_helper_list);
/**
@@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct
drm_fb_helper *fb_helper)
struct drm_connector *connector;
int i;
+if (!drm_fbdev_emulation)
+return 0;
+
mutex_lock(&dev->mode_config.mutex);
drm_for_each_connector(connector, dev) {
struct drm_fb_helper_connector *fb_helper_connector;
@@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct
drm_fb_helper *fb_helper, struct drm_
struct drm_fb_helper_connector **temp;
struct drm_fb_helper_connector *fb_helper_connector;
+if (!drm_fbdev_emulation)
+return 0;
+
WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
if (fb_helper->connector_count + 1 >
fb_helper->connector_info_alloc_count) {
temp = krealloc(fb_helper->connector_info, sizeof(struct
drm_fb_helper_connector *) * (fb_helper->connector_count + 1),
GFP_KERNEL);
@@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct
drm_fb_helper *fb_helper,
struct drm_fb_helper_connector *fb_helper_connector;
int i, j;
+if (!drm_fbdev_emulation)
+return 0;
+
WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
for (i = 0; i < fb_helper->connector_count; i++) {
@@ -375,6 +389,9 @@ int
drm_fb_helper_restore_fbdev_mode_un
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
On 08/26/2015 02:10 PM, Daniel Vetter wrote: > On Wed, Aug 26, 2015 at 08:49:00AM +0200, Thomas Hellstrom wrote: >> Hi, Tiago. >> >> On 08/26/2015 02:02 AM, Tiago Vignatti wrote: >>> From: Daniel Vetter >>> >>> The userspace might need some sort of cache coherency management e.g. when >>> CPU >>> and GPU domains are being accessed through dma-buf at the same time. To >>> circumvent this problem there are begin/end coherency markers, that forward >>> directly to existing dma-buf device drivers vfunc hooks. Userspace can make >>> use >>> of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be >>> used like following: >>> >>> - mmap dma-buf fd >>> - for each drawing/upload cycle in CPU >>> 1. SYNC_START ioctl >>> 2. read/write to mmap area or a 2d sub-region of it >>> 3. SYNC_END ioctl. >>> - munamp once you don't need the buffer any more >>> >>> v2 (Tiago): Fix header file type names (u64 -> __u64) >>> v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end >>> dma-buf functions. Check for overflows in start/length. >>> v4 (Tiago): use 2d regions for sync. >> Daniel V had issues with the sync argument proposed by Daniel S. I'm >> fine with that argument or an argument containing only a single sync >> rect. I'm not sure whether Daniel V will find it easier to accept only a >> single sync rect... > I'm kinda against all the 2d rect sync proposals ;-) At least for the > current stuff it's all about linear subranges afaik, and even there we > don't bother with flushing them precisely right now. > > My expectation would be that if you _really_ want to etch out that last > bit of performance with a list of 2d sync ranges then probably you want to > do the cpu cache flushing in userspace anyway, with 100% machine-specific > trickery. Daniel, I might be misunderstanding things, but isn't this about finally accepting a dma-buf mmap() generic interface for people who want to use it for zero-copy applications (like people have been trying to do for years but never bothered to specify an interface that actually performed on incoherent hardware)? If it's only about exposing the kernel 1D sync interface to user-space for correctness, then why isn't that done transparently to the user? Thanks, Thomas > -Daniel
[PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation
On 08/26/2015 05:07 PM, Daniel Vetter wrote: > On Wed, Aug 26, 2015 at 01:34:58PM +0200, Daniel Vetter wrote: >> On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote: >>> >>> >>> On 08/26/2015 10:42 AM, Archit Taneja wrote: On 08/25/2015 07:15 PM, Daniel Vetter wrote: > Faster than recompiling. > > Note that restore_fbdev_mode_unlocked is a bit special and the only > one which returns an error code when fbdev isn't there - i915 needs > that one to not fall over with some additional fbcon related restore > code. Everyone else just ignores the return value or only prints a > DRM_DEBUG level message. Reviewed-by:Archit Taneja >>> >>> >>> With the module param, and the drivers should see the following state( >>> based on the truth table below): >>> >>> module param | config option >>> true |true -> real helper funcs called, driver >>> allocated drm_fb_helper is correctly >>> populated. >>> >>> false |true -> real helper funcs called, but bailed >>> out early, driver allocated >>> drm_fb_helper is non-NULL, but we won't >>> end up using it. >> >> Hm I tried to give drivers the same semantics here as with the stub >> functions. Where did I screw up? The goal really was to match the end >> result for drivers ... > > Note that at least for i915 we can't make it perfectly equal since i915 > compiles out a few more things with FBDEV_EMULATION=n than just the stubs. > Long-term we might want to push some of that into helpers too perhaps. Ah, I missed looking at this mail. Yeah, that's what I wanted to mainly point out. It looks okay otherwise. Since the param is 'true' by default. Things should be okay for all drivers. If someone reports an issue with a driver with the above combination, we could fix it individually. I guess the next step now is to remove the custom config fbdev emulation options and module params from drivers that have those. After that, we could start with scary process of removing the CONFIG_FB and CONFIG_DRM_KMS_FB_HELPER from each driver. Archit -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation
On Wed, Aug 26, 2015 at 05:59:14PM +0530, Archit Taneja wrote: > > > On 08/26/2015 05:07 PM, Daniel Vetter wrote: > >On Wed, Aug 26, 2015 at 01:34:58PM +0200, Daniel Vetter wrote: > >>On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote: > >>> > >>> > >>>On 08/26/2015 10:42 AM, Archit Taneja wrote: > > > On 08/25/2015 07:15 PM, Daniel Vetter wrote: > >Faster than recompiling. > > > >Note that restore_fbdev_mode_unlocked is a bit special and the only > >one which returns an error code when fbdev isn't there - i915 needs > >that one to not fall over with some additional fbcon related restore > >code. Everyone else just ignores the return value or only prints a > >DRM_DEBUG level message. > > Reviewed-by:Archit Taneja > >>> > >>> > >>>With the module param, and the drivers should see the following state( > >>>based on the truth table below): > >>> > >>>module param | config option > >>>true |true -> real helper funcs called, driver > >>> allocated drm_fb_helper is correctly > >>> populated. > >>> > >>>false |true -> real helper funcs called, but bailed > >>> out early, driver allocated > >>> drm_fb_helper is non-NULL, but we won't > >>> end up using it. > >> > >>Hm I tried to give drivers the same semantics here as with the stub > >>functions. Where did I screw up? The goal really was to match the end > >>result for drivers ... > > > >Note that at least for i915 we can't make it perfectly equal since i915 > >compiles out a few more things with FBDEV_EMULATION=n than just the stubs. > >Long-term we might want to push some of that into helpers too perhaps. > > Ah, I missed looking at this mail. Yeah we had to go ahead with removing I915_FBDEV since it was causing trouble if you disable one but not the other. I think i915 is the only driver where this can happen though, the others with their own fbdev Kconfig option disable a lot less. > Yeah, that's what I wanted to mainly point out. It looks okay > otherwise. > > Since the param is 'true' by default. Things should be okay for all > drivers. If someone reports an issue with a driver with the above > combination, we could fix it individually. > > I guess the next step now is to remove the custom config fbdev > emulation options and module params from drivers that have > those. > > After that, we could start with scary process of removing the > CONFIG_FB and CONFIG_DRM_KMS_FB_HELPER from each driver. Yeah, definitely should do that for 4.4. I'll pull in this one here with your r-b too, thanks for the feedback. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
On Wed, Aug 26, 2015 at 02:28:30PM +0200, Thomas Hellstrom wrote: > On 08/26/2015 02:10 PM, Daniel Vetter wrote: > > On Wed, Aug 26, 2015 at 08:49:00AM +0200, Thomas Hellstrom wrote: > >> Hi, Tiago. > >> > >> On 08/26/2015 02:02 AM, Tiago Vignatti wrote: > >>> From: Daniel Vetter > >>> > >>> The userspace might need some sort of cache coherency management e.g. > >>> when CPU > >>> and GPU domains are being accessed through dma-buf at the same time. To > >>> circumvent this problem there are begin/end coherency markers, that > >>> forward > >>> directly to existing dma-buf device drivers vfunc hooks. Userspace can > >>> make use > >>> of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would > >>> be > >>> used like following: > >>> > >>> - mmap dma-buf fd > >>> - for each drawing/upload cycle in CPU > >>> 1. SYNC_START ioctl > >>> 2. read/write to mmap area or a 2d sub-region of it > >>> 3. SYNC_END ioctl. > >>> - munamp once you don't need the buffer any more > >>> > >>> v2 (Tiago): Fix header file type names (u64 -> __u64) > >>> v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the > >>> begin/end > >>> dma-buf functions. Check for overflows in start/length. > >>> v4 (Tiago): use 2d regions for sync. > >> Daniel V had issues with the sync argument proposed by Daniel S. I'm > >> fine with that argument or an argument containing only a single sync > >> rect. I'm not sure whether Daniel V will find it easier to accept only a > >> single sync rect... > > I'm kinda against all the 2d rect sync proposals ;-) At least for the > > current stuff it's all about linear subranges afaik, and even there we > > don't bother with flushing them precisely right now. > > > > My expectation would be that if you _really_ want to etch out that last > > bit of performance with a list of 2d sync ranges then probably you want to > > do the cpu cache flushing in userspace anyway, with 100% machine-specific > > trickery. > > Daniel, > > I might be misunderstanding things, but isn't this about finally > accepting a dma-buf mmap() generic interface for people who want to use > it for zero-copy applications (like people have been trying to do for > years but never bothered to specify an interface that actually performed > on incoherent hardware)? > > If it's only about exposing the kernel 1D sync interface to user-space > for correctness, then why isn't that done transparently to the user? Mostly pragmatic reasons - we could do the page-fault trickery, but that means i915 needs another mmap implementation. At least I didn't figure out how to do faulting in a completely generic way. And we already have 3 other mmap implementations so I prefer not to do that. The other is that right now there's no user nor implementation in sight which actually does range-based flush optimizations, so I'm pretty much expecting we'll get it wrong. Maybe instead we should go one step further and remove the range from the internal dma-buf interface and also drop it from the ioctl? With the flags we can always add something later on once we have a real user with a clear need for it. But afaik cros only wants to shuffle around entire tiles and has a buffer-per-tile approach. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH libdrm] omap: Initialize DMA BUF file descriptor to -1
On 26 August 2015 at 09:16, Thierry Reding wrote:
> From: Thierry Reding
>
> Commit c86dabfc9f04 ("omap: zero is a valid fd number, treat it as
> such") corrected checks for valid file descriptors, but the OMAP buffer
> object code initializes the DMA-BUF file descriptor to 0 (as a result of
> calloc()'ing the structure). Obviously this isn't going to work because
> subsequent code will try to use file descriptor 0 (most likely stdin at
> that point) as a DMA-BUF. It may also try and close stdin when a buffer
> object is destroyed.
>
> Fix this by initializing the DMA-BUF file descriptor to -1, properly
> marking it as an invalid file descriptor.
>
Sorry about this guys. I should have gone to specsavers ...
> Fixes: c86dabfc9f04 ("omap: zero is a valid fd number, treat it as such")
> Reported-by: Robert Nelson
> Tested-by: Robert Nelson
> Signed-off-by: Thierry Reding
Reviewed-by: Emil Velikov
> ---
> Emil, I think this may warrant cutting a new release to unbreak OMAP. I
> can push the patch myself, but I've never done a libdrm release before,
> so maybe you want to do the honors?
>
I would have done it in a second if it wasn't for the lack of feedback
wrt the drmGetDevices API. I've been nudging people to take a look,
perhaps you can do so too :-)
Alternatively I'll cut a new release in a couple of days.
Cheers,
Emil
[PATCH 1/3] drm/dp: add drm_dp_tps3_supported helper
On Wed, Aug 26, 2015 at 7:33 AM, Jani Nikula wrote:
> Cc: Thierry Reding
> Signed-off-by: Jani Nikula
Reviewed-by: Alex Deucher
> ---
> include/drm/drm_dp_helper.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 499e9f625aef..8c52d0ef1fc9 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -634,6 +634,13 @@ drm_dp_enhanced_frame_cap(const u8
> dpcd[DP_RECEIVER_CAP_SIZE])
> (dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP);
> }
>
> +static inline bool
> +drm_dp_tps3_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> + return dpcd[DP_DPCD_REV] >= 0x12 &&
> + dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED;
> +}
> +
> /*
> * DisplayPort AUX channel
> */
> --
> 2.1.4
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask
Hi Archit,
> mdp5_hw_init and mdp5_set_irqmask configure registers but may not have
> clocks enabled.
>
> Add mdp5_enable/disable calls in these funcs to ensure clocks are
> enabled. We need this until we get proper runtime pm support.
>
> Signed-off-by: Archit Taneja
> ---
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 10 --
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 2 ++
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> index b1f73be..9fabfca 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> @@ -24,9 +24,15 @@
> void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
> uint32_t old_irqmask)
> {
> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_CLEAR(0),
> + struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
> +
> + mdp5_enable(mdp5_kms);
> +
> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_CLEAR(0),
> irqmask ^ (irqmask & old_irqmask));
> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_EN(0), irqmask);
> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_EN(0), irqmask);
> +
> + mdp5_disable(mdp5_kms);
> }
mdp5_set_irqmask() can be invoked in atomic context, clk_prepare() is not
allowed in this function because it may cause process to sleep. We can
enable the clocks in the caller at initialization.
>
> static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t
> irqstatus)
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index 047cb04..2b760f5 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
> unsigned long flags;
>
> pm_runtime_get_sync(dev->dev);
> + mdp5_enable(mdp5_kms);
>
> /* Magic unknown register writes:
>*
> @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
>
> mdp5_ctlm_hw_reset(mdp5_kms->ctlm);
>
> + mdp5_disable(mdp5_kms);
> pm_runtime_put_sync(dev->dev);
>
> return 0;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>
>
[PATCH v2 00/22] Enable gpu switching on the MacBook Pro
Hi Daniel, On Tue, Aug 25, 2015 at 10:21:20AM +0200, Daniel Vetter wrote: > On Tue, Aug 25, 2015 at 09:36:47AM +0200, Lukas Wunner wrote: > > I've overhauled locking once more because previously all EDID reads > > were serialized even on machines which don't use vga_switcheroo at all. > > Seems it's impossible to fix this with mutexes and still be race-free > > and deadlock-free, so I've changed vgasr_mutex to an rwlock: > > https://github.com/l1k/linux/commit/ffa4546d85244a53541eed6799f93e8eea7f29e8 > > > > There are a number of vga_switcheroo functions added by Takashi Iwai > > and yourself which don't lock anything at all, I believe this was in > > part to avoid deadlocks (vga_switcheroo_runtime_resume_hdmi_audio locks > > vgasr_mutex, wakes up the GPU with vga_switcheroo_runtime_resume > > which locks vgasr_mutex once again). After changing vgasr_mutex to an > > rwlock we can safely use locking in those functions as well: > > https://github.com/l1k/linux/commit/cdea8f5c92039329dde47cadf20b67c7d9af0d62 > > > > With this change, on machines which don't use vga_switcheroo, the > > overhead in drm_get_edid() is exactly 1 read_lock + 1 read_unlock > > and EDID reads can happen in parallel. Thierry Reding and Alex Deucher > > are in favor of adding a separate drm_get_edid_switcheroo() and changing > > the relevant drivers (i915, nouveau, radeon, amdgpu) to use that in lieu > > of drm_get_edid() so that drivers which don't use vga_switcheroo avoid > > the lock_ddc/unlock_ddc calls. Performance-wise these additional calls > > should be negligible, so I think the motivation can only be better > > readability/clarity. One should also keep in mind that drivers which > > don't use vga_switcheroo currently might do so in the future, e.g. > > if mobile GPUs use a big.LITTLE concept like ARM CPUs already do. > > Just a quick aside: Switching to rwlocks to make lockdep happy is a > fallacy - lockdep unfortunately doesn't accurately track read lock > depencies. Which means very likely you didn't fix the locking inversions > but just made lockdep shut up about them. [...] > I'd highly suggest you try fixing the vga-switcheroo locking without > resorting to rw lock primitives - that just means we need to manually > prove the locking for many cases which is tons of work. mutex is not the right tool for the job: To make DDC switching bullet proof you need to block the handler from unregistering between lock_ddc and unlock_ddc. Solution: ddc_lock grabs a mutex, ddc_unlock releases it, unregister_handler grabs that same lock. Downside: All EDID reads are serialized, you can't probe EDID in parallel if you have multiple displays. Which is ugly. One could be tempted to "solve" this for non-muxed machines by immediately releasing the lock in lock_ddc if there's no handler, and by checking in unlock_ddc if mutex_is_locked before releasing it. But on muxed machines this would be racy: GPU driver A calls lock_ddc, acquires the mutex, sees there's no handler registered, releases the mutex. Between this and the call to unlock_ddc, a handler registers and GPU driver B calls lock_ddc. This time the mutex is actually locked and when driver A calls unlock_ddc, it will release it, even though the other driver had acquired it. Of course one could come up with all sorts of ugly hacks like remembering for which client the mutex was acquired, but this wouldn't be atomic and 100% bullet proof, unlike rwlocks. So it seems there's no way with mutexes to achieve parallel EDID reads and still be race-free and deadlock-free. rwlocks have the right semantics for this use case: Registering and unregistering requires write access to the lock, thus happens exclusively. In lock_ddc we acquire read access to the rwlock and in unlock_ddc we release it. So the handler can't disappear while we've switched DDC. We use a mutex ddc_lock to lock the DDC lines but we only acquire that lock if there's actually a handler. So on non-muxed machines, EDID reads *can* happen in parallel, there's just the (negligible) overhead of 1 read_lock + 1 read_unlock: https://github.com/l1k/linux/commit/d0b6f659ae8f4b8b94f4b3ded9fc80e93950d0b3 Best regards, Lukas
[PATCH] drm/atomic-helper: Add option to update planes only on active crtc
With drivers supporting runtime pm it's generally not a good idea to
touch the hardware when it's off. Add an option to the commit_planes
helper to support this case.
Note that the helpers already add all planes on a crtc when a modeset
happens, hence plane updates will not be lost if drivers set this to
true.
v2: Check for NULL state->crtc before chasing the pointer. Also check
both old and new crtc if there's a switch. Finally just outright
disallow switching crtcs for a plane if the plane is in active use, on
most hardware that doesn't make sense.
v3: Since commit_planes(active_only = true) is for enabling things
only after all the crtc are on we should only look at the new crtc to
decide whether to call the plane hooks - if the current CRTC isn't on
then skip. If the old crtc (when moving a plane) went down then the
plane should have been disabled as part of the pipe shutdown work
already. For which there's currently no helper really unfortunately.
Also move the check for wether a plane gets a new CRTC assigned while
still in active use out of this patch.
Cc: Maarten Lankhorst
Cc: Thierry Reding
Cc: Laurent Pinchart
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_atomic_helper.c| 20 ++--
drivers/gpu/drm/exynos/exynos_drm_fb.c | 2 +-
drivers/gpu/drm/msm/msm_atomic.c | 2 +-
drivers/gpu/drm/omapdrm/omap_drv.c | 2 +-
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +-
drivers/gpu/drm/sti/sti_drv.c | 2 +-
drivers/gpu/drm/tegra/drm.c| 2 +-
include/drm/drm_atomic_helper.h| 3 ++-
8 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index e0b99302c667..8bfd64f71bc4 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1037,7 +1037,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
drm_atomic_helper_commit_modeset_disables(dev, state);
- drm_atomic_helper_commit_planes(dev, state);
+ drm_atomic_helper_commit_planes(dev, state, false);
drm_atomic_helper_commit_modeset_enables(dev, state);
@@ -1152,10 +1152,16 @@ fail:
}
EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
+bool plane_crtc_active(struct drm_plane_state *state)
+{
+ return state->crtc && state->crtc->state->active;
+}
+
/**
* drm_atomic_helper_commit_planes - commit plane state
* @dev: DRM device
* @old_state: atomic state object with old state structures
+ * @active_only: Only commit on active CRTC if set
*
* This function commits the new plane state using the plane and atomic helper
* functions for planes and crtcs. It assumes that the atomic state has already
@@ -1170,7 +1176,8 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
* drm_atomic_helper_commit_planes_on_crtc() instead.
*/
void drm_atomic_helper_commit_planes(struct drm_device *dev,
-struct drm_atomic_state *old_state)
+struct drm_atomic_state *old_state,
+bool active_only)
{
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state;
@@ -1186,6 +1193,9 @@ void drm_atomic_helper_commit_planes(struct drm_device
*dev,
if (!funcs || !funcs->atomic_begin)
continue;
+ if (active_only && !crtc->state->active)
+ continue;
+
funcs->atomic_begin(crtc, old_crtc_state);
}
@@ -1197,6 +1207,9 @@ void drm_atomic_helper_commit_planes(struct drm_device
*dev,
if (!funcs)
continue;
+ if (active_only && !plane_crtc_active(plane->state))
+ continue;
+
/*
* Special-case disabling the plane if drivers support it.
*/
@@ -1216,6 +1229,9 @@ void drm_atomic_helper_commit_planes(struct drm_device
*dev,
if (!funcs || !funcs->atomic_flush)
continue;
+ if (active_only && !crtc->state->active)
+ continue;
+
funcs->atomic_flush(crtc, old_crtc_state);
}
}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 9738f4e0c6eb..2e1f7143174c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -293,7 +293,7 @@ static int exynos_atomic_commit(struct drm_device *dev,
* have the relevant clocks enabled to perform the update.
*/
- drm_atomic_helper_commit_planes(dev, state);
+ drm_atomic_helper_commit_planes(dev, state, false);
drm_atomic_helper_cleanup_planes(dev, state);
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 1ceb4f22dd89..7eb253bc24df 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -125,7
[Bug 84614] radeon gpu crash /kernel crash when using dynamic light in borderlands 2
https://bugs.freedesktop.org/show_bug.cgi?id=84614 --- Comment #20 from Eduardo --- For me, the issue is solved. But I'm not the original reporter. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150826/846a7a0d/attachment-0001.html>
[PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask
2015-08-26 9:55 GMT-04:00 :
> Hi Archit,
>
>> mdp5_hw_init and mdp5_set_irqmask configure registers but may not have
>> clocks enabled.
>>
>> Add mdp5_enable/disable calls in these funcs to ensure clocks are
>> enabled. We need this until we get proper runtime pm support.
>>
>> Signed-off-by: Archit Taneja
>> ---
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 10 --
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 2 ++
>> 2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>> index b1f73be..9fabfca 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>> @@ -24,9 +24,15 @@
>> void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
>> uint32_t old_irqmask)
>> {
>> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_CLEAR(0),
>> + struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
>> +
>> + mdp5_enable(mdp5_kms);
>> +
>> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_CLEAR(0),
>> irqmask ^ (irqmask & old_irqmask));
>> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_EN(0), irqmask);
>> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_EN(0), irqmask);
>> +
>> + mdp5_disable(mdp5_kms);
>> }
>
> mdp5_set_irqmask() can be invoked in atomic context, clk_prepare() is not
> allowed in this function because it may cause process to sleep. We can
> enable the clocks in the caller at initialization.
iirc, it will be called with at least one spinlock held..
We do already move the enable/disable_vblank() paths off to a worker
so that we can ensure things are enabled before we get into
update_irq().. the only other path to update_irq() should be when
driver code does mdp_irq_register/unregister().. so maybe we should
just require that the mdp4/mdp5 kms code only calls those when clk's
are already enabled (which should be mostly true already, I think)
BR,
-R
>>
>> static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t
>> irqstatus)
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> index 047cb04..2b760f5 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
>> unsigned long flags;
>>
>> pm_runtime_get_sync(dev->dev);
>> + mdp5_enable(mdp5_kms);
>>
>> /* Magic unknown register writes:
>>*
>> @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
>>
>> mdp5_ctlm_hw_reset(mdp5_kms->ctlm);
>>
>> + mdp5_disable(mdp5_kms);
>> pm_runtime_put_sync(dev->dev);
>>
>> return 0;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> hosted by The Linux Foundation
>>
>>
>
>
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
On 08/26/2015 09:58 AM, Daniel Vetter wrote: > The other is that right now there's no user nor implementation in sight > which actually does range-based flush optimizations, so I'm pretty much > expecting we'll get it wrong. Maybe instead we should go one step further > and remove the range from the internal dma-buf interface and also drop it > from the ioctl? With the flags we can always add something later on once > we have a real user with a clear need for it. But afaik cros only wants to > shuffle around entire tiles and has a buffer-per-tile approach. Thomas, I think Daniel has a point here and also, I wouldn't mind removing all range control from the dma-buf ioctl either. In this last patch we can see that it's not complicated to add the 2d-sync if we eventually decides to want it. But right now there's no way we can test it. Therefore in that case I'm all in favour of doing this work gradually, adding the basics first. Tiago
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
On 08/26/2015 04:32 PM, Tiago Vignatti wrote: > On 08/26/2015 09:58 AM, Daniel Vetter wrote: >> The other is that right now there's no user nor implementation in sight >> which actually does range-based flush optimizations, so I'm pretty much >> expecting we'll get it wrong. Maybe instead we should go one step >> further >> and remove the range from the internal dma-buf interface and also >> drop it >> from the ioctl? With the flags we can always add something later on once >> we have a real user with a clear need for it. But afaik cros only >> wants to >> shuffle around entire tiles and has a buffer-per-tile approach. > > Thomas, I think Daniel has a point here and also, I wouldn't mind > removing all range control from the dma-buf ioctl either. > > In this last patch we can see that it's not complicated to add the > 2d-sync if we eventually decides to want it. But right now there's no > way we can test it. Therefore in that case I'm all in favour of doing > this work gradually, adding the basics first. > Sure. Unfortunately I wasn't completely clear about the use-case here. IMO adding a user-space sync functionality should be purely for performance. /Thomas
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
On Wed, Aug 26, 2015 at 11:32:30AM -0300, Tiago Vignatti wrote: > On 08/26/2015 09:58 AM, Daniel Vetter wrote: > >The other is that right now there's no user nor implementation in sight > >which actually does range-based flush optimizations, so I'm pretty much > >expecting we'll get it wrong. Maybe instead we should go one step further > >and remove the range from the internal dma-buf interface and also drop it > >from the ioctl? With the flags we can always add something later on once > >we have a real user with a clear need for it. But afaik cros only wants to > >shuffle around entire tiles and has a buffer-per-tile approach. > > Thomas, I think Daniel has a point here and also, I wouldn't mind removing > all range control from the dma-buf ioctl either. if we go with nuking it from the ioctl I'd suggest to also nuke it from the dma-buf internal inferface first too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
On 08/26/2015 11:51 AM, Daniel Vetter wrote: > On Wed, Aug 26, 2015 at 11:32:30AM -0300, Tiago Vignatti wrote: >> On 08/26/2015 09:58 AM, Daniel Vetter wrote: >>> The other is that right now there's no user nor implementation in sight >>> which actually does range-based flush optimizations, so I'm pretty much >>> expecting we'll get it wrong. Maybe instead we should go one step further >>> and remove the range from the internal dma-buf interface and also drop it >> >from the ioctl? With the flags we can always add something later on once >>> we have a real user with a clear need for it. But afaik cros only wants to >>> shuffle around entire tiles and has a buffer-per-tile approach. >> >> Thomas, I think Daniel has a point here and also, I wouldn't mind removing >> all range control from the dma-buf ioctl either. > > if we go with nuking it from the ioctl I'd suggest to also nuke it from > the dma-buf internal inferface first too. yep, I can do it. Thomas, so we leave 2d sync out now? Tiago
[PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask
> 2015-08-26 9:55 GMT-04:00 :
>> Hi Archit,
>>
>>> mdp5_hw_init and mdp5_set_irqmask configure registers but may not have
>>> clocks enabled.
>>>
>>> Add mdp5_enable/disable calls in these funcs to ensure clocks are
>>> enabled. We need this until we get proper runtime pm support.
>>>
>>> Signed-off-by: Archit Taneja
>>> ---
>>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 10 --
>>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 2 ++
>>> 2 files changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>> index b1f73be..9fabfca 100644
>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>> @@ -24,9 +24,15 @@
>>> void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
>>> uint32_t old_irqmask)
>>> {
>>> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_CLEAR(0),
>>> + struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
>>> +
>>> + mdp5_enable(mdp5_kms);
>>> +
>>> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_CLEAR(0),
>>> irqmask ^ (irqmask & old_irqmask));
>>> - mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_EN(0),
>>> irqmask);
>>> + mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_EN(0), irqmask);
>>> +
>>> + mdp5_disable(mdp5_kms);
>>> }
>>
>> mdp5_set_irqmask() can be invoked in atomic context, clk_prepare() is
>> not
>> allowed in this function because it may cause process to sleep. We can
>> enable the clocks in the caller at initialization.
>
> iirc, it will be called with at least one spinlock held..
>
> We do already move the enable/disable_vblank() paths off to a worker
> so that we can ensure things are enabled before we get into
> update_irq().. the only other path to update_irq() should be when
> driver code does mdp_irq_register/unregister().. so maybe we should
> just require that the mdp4/mdp5 kms code only calls those when clk's
> are already enabled (which should be mostly true already, I think)
>
> BR,
> -R
Yes, the only case that not been covered is mdp5_irq_postinstall(). We can
enable clocks in this function. Actually, this is what we are doing in
downstream test.
>
>>>
>>> static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t
>>> irqstatus)
>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> index 047cb04..2b760f5 100644
>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>> @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
>>> unsigned long flags;
>>>
>>> pm_runtime_get_sync(dev->dev);
>>> + mdp5_enable(mdp5_kms);
>>>
>>> /* Magic unknown register writes:
>>>*
>>> @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms)
>>>
>>> mdp5_ctlm_hw_reset(mdp5_kms->ctlm);
>>>
>>> + mdp5_disable(mdp5_kms);
>>> pm_runtime_put_sync(dev->dev);
>>>
>>> return 0;
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum,
>>> hosted by The Linux Foundation
>>>
>>>
>>
>>
>
drm/msm/dsi: hs_zero timing
Hi Werner,
Thanks for sharing this. The DPHY timings in downstream dtsi are exactly the
same as the excel calculation, but slightly different from the output of drm
code as you posted. (e.g hs_zero is 116 vs 118)
I think it is caused by some precision loss during driver calculation, but I
need to double check.
Could you help to try configuring the same DPHY timings as downstream, but
leave the values in DSIPHY_LNx_CFG4 as is, to see if it works?
This could help us to narrow down the issue.
Thanks,
Hai
-Original Message-
From: Werner Johansson [mailto:werner.johans...@gmail.com]
Sent: Monday, August 24, 2015 9:24 PM
To: Hai Li
Cc: Rob Clark; Johansson, Werner; dri-devel at lists.freedesktop.org
Subject: Re: drm/msm/dsi: hs_zero timing
On Mon, Aug 24, 2015 at 7:32 AM, Hai Li wrote:
> Hi Werner,
>
> Yes, the register is to adjust hs_zero.
> Could you share the panel's video timing and dphy timings (or the panel DT),
> used by downstream driver?
>
> The dphy timing calculations in the phy driver are from the excel sheet as
> well, I can check if there is any issue inside the calculation code making
> the difference.
>
>
> Thanks,
> Hai
Hi Hai,
Yes definitely, our downstream DT definitions for the Panasonic panel looks
like this: (taken from our kernel available here:
https://github.com/sonyxperiadev/kernel-copyleft/blob/23.1.A.1.xxx/arch/arm/boot/dts/dsi-panel-castor.dtsi#L78-L145)
dsi_novatek_panasonic_wuxga_vid: somc,novatek_panasonic_wuxga_panel {
qcom,mdss-dsi-panel-name = "panasonic novatek wuxga video";
qcom,mdss-dsi-panel-controller = <&mdss_dsi0>; qcom,mdss-dsi-panel-type =
"dsi_video_mode"; qcom,mdss-dsi-panel-destination = "display_1";
qcom,mdss-dsi-panel-framerate = <60>; qcom,mdss-dsi-virtual-channel-id = <0>;
qcom,mdss-dsi-stream = <0>; qcom,mdss-dsi-panel-width = <1920>;
qcom,mdss-dsi-panel-height = <1200>; qcom,mdss-dsi-h-front-porch = <152>;
qcom,mdss-dsi-h-back-porch = <20>; qcom,mdss-dsi-h-pulse-width = <52>;
qcom,mdss-dsi-h-sync-skew = <0>; qcom,mdss-dsi-v-back-porch = <48>;
qcom,mdss-dsi-v-front-porch = <24>; qcom,mdss-dsi-v-pulse-width = <6>;
qcom,mdss-dsi-h-left-border = <0>; qcom,mdss-dsi-h-right-border = <0>;
qcom,mdss-dsi-v-top-border = <0>; qcom,mdss-dsi-v-bottom-border = <0>;
qcom,mdss-dsi-bpp = <24>; qcom,mdss-dsi-underflow-color = <0x0>;
qcom,mdss-dsi-border-color = <0>; qcom,mdss-dsi-on-command = [32 01 00 00 00 00
02 00 00]; qcom,mdss-dsi-on-command-state = "dsi_lp_mode";
qcom,mdss-dsi-h-sync-pulse = <1>; qcom,mdss-dsi-traffic-mode =
"non_burst_sync_event"; qcom,mdss-dsi-bllp-eof-power-mode;
qcom,mdss-dsi-bllp-power-mode;
qcom,mdss-dsi-lane-0-state;
qcom,mdss-dsi-lane-1-state;
qcom,mdss-dsi-lane-2-state;
qcom,mdss-dsi-lane-3-state;
qcom,mdss-dsi-panel-timings = [FB 3E 2A 00 70 74 2E 42 33 03 04 00];
qcom,mdss-dsi-t-clk-post = <0x02>; qcom,mdss-dsi-t-clk-pre = <0x2E>;
qcom,mdss-dsi-bl-min-level = <1>; qcom,mdss-dsi-bl-max-level = <255>;
qcom,mdss-brightness-max-level = <255>; qcom,mdss-dsi-dma-trigger =
"trigger_sw"; qcom,mdss-dsi-mdp-trigger = "none";
qcom,mdss-dsi-bl-pmic-control-type = "bl_ctrl_wled";
qcom,mdss-dsi-pan-enable-dynamic-fps;
qcom,mdss-dsi-pan-fps-update = "dfps_suspend_resume_mode";
qcom,cont-splash-enabled; qcom,mdss-dsi-tx-eot-append; somc,driver-ic = <3>;
somc,dric-gpio = <&msmgpio 26 0>; somc,mul-channel-scaling = <3>;
somc,mdss-phy-size-mm = <217 135>; somc,mdss-dsi-lane-config = [00 c2 ef 00 00
00 00 01 ff
00 c2 ef 00 00 00 00 01 ff
00 c2 ef 00 00 00 00 01 ff
00 c2 ef 00 00 00 00 01 ff
00 02 00 00 00 00 00 01 97];
somc,lcd-id = <1>;
somc,lcd-id-adc = <801000 917000>;
somc,disp-en-on-post = <251>;
somc,disp-en-off-pre = <86>;
somc,pw-down-period = <500>;
somc,mdss-dsi-uv-param-type = <0>;
somc,mdss-dsi-pcc-table-size = <1>;
somc,mdss-dsi-pcc-table = <
0x00 0x01 0x1C 0x1F 0x1C 0x1F 0x8000 0x8000 0x7D80>; };
/wj
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
> 26 aug 2015 kl. 16:58 skrev Tiago Vignatti : > >> On 08/26/2015 11:51 AM, Daniel Vetter wrote: >>> On Wed, Aug 26, 2015 at 11:32:30AM -0300, Tiago Vignatti wrote: On 08/26/2015 09:58 AM, Daniel Vetter wrote: The other is that right now there's no user nor implementation in sight which actually does range-based flush optimizations, so I'm pretty much expecting we'll get it wrong. Maybe instead we should go one step further and remove the range from the internal dma-buf interface and also drop it >>> >from the ioctl? With the flags we can always add something later on once we have a real user with a clear need for it. But afaik cros only wants to shuffle around entire tiles and has a buffer-per-tile approach. >>> >>> Thomas, I think Daniel has a point here and also, I wouldn't mind removing >>> all range control from the dma-buf ioctl either. >> >> if we go with nuking it from the ioctl I'd suggest to also nuke it from >> the dma-buf internal inferface first too. > > yep, I can do it. > > Thomas, so we leave 2d sync out now? > > Tiago > Sure! Thomas
[PATCH] drm/atomic: refuse changing CRTC for planes directly
Very strictly speaking this is possible if you have special hw and
genlocked CRTCs. In general switching a plane between two active CRTC
just won't work so well and is probably not tested at all. Just forbid
it.
I've put this into the core since I really couldn't come up with a
case where we don't want to enforce that. But if that ever happens it
would be easy to move this check into helpers.
v2: don't bother with complexity and just outright disallow plane
switching without the intermediate OFF state. Simplifies drivers, we
don't have any hw that could do it anyway and current atomic userspace
(weston) works like this already anyway.
Cc: Thierry Reding
Cc: Maarten Lankhorst
Cc: Daniel Stone
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_atomic.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 434915448ea0..f27aae3fa765 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
return 0;
}
+static bool
+plane_switching(struct drm_atomic_state *state,
+ struct drm_plane *plane,
+ struct drm_plane_state *plane_state)
+{
+ struct drm_crtc_state *crtc_state, *curr_crtc_state;
+
+ if (!plane->state->crtc || !plane_state->crtc)
+ return false;
+
+ if (plane->state->crtc == plane_state->crtc)
+ return false;
+
+ return true;
+}
+
/**
* drm_atomic_plane_check - check plane state
* @plane: plane to check
@@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
return -ENOSPC;
}
+ if (plane_switching(state->state, plane, state)) {
+ DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
+plane->base.id);
+ return -EINVAL;
+ }
+
return 0;
}
--
2.5.0
[PATCH 03/11] drm/exynos: add prepare and cleanup phases for planes
Hi Inki,
2015-08-24 Inki Dae :
> On 2015ë
08ì 16ì¼ 01:26, Gustavo Padovan wrote:
> > From: Gustavo Padovan
> >
> > .prepare_plane() and .cleanup_plane() allows to perform extra operations
> > before and after the update of planes. For FIMD for example this will
> > be used to enable disable the shadow protection bit.
> >
> > Signed-off-by: Gustavo Padovan
> > ---
> > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 19 +++
> > drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++
> > 2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > index 5a19e16..3a89fc9 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > @@ -72,15 +72,34 @@ exynos_drm_crtc_mode_set_nofb(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;
> >
> > if (crtc->state->event) {
> > WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> > exynos_crtc->event = crtc->state->event;
> > }
> > +
> > + drm_atomic_crtc_for_each_plane(plane, crtc) {
> > + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
> > +
> > + if (exynos_crtc->ops->prepare_plane)
> > + exynos_crtc->ops->prepare_plane(exynos_crtc,
> > + exynos_plane);
>
> There is no any reason to use prepare_plane/cleanup_plane callback
> names. How about using atomic_begin/atomic_flush callback names instead
> for consistency between framework and device drivers?
Either names are fine for me. So let's go with atomic_begin/flush. I'll
send an updated patchset.
Gustavo
[PATCH 1/2] drm/exynos: remove legacy ->suspend()/resume()
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.
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
>
[Intel-gfx] [PATCH] drm/atomic: refuse changing CRTC for planes directly
On Wed, Aug 26, 2015 at 05:41:23PM +0200, Daniel Vetter wrote:
> Very strictly speaking this is possible if you have special hw and
> genlocked CRTCs. In general switching a plane between two active CRTC
> just won't work so well and is probably not tested at all. Just forbid
> it.
>
> I've put this into the core since I really couldn't come up with a
> case where we don't want to enforce that. But if that ever happens it
> would be easy to move this check into helpers.
>
> v2: don't bother with complexity and just outright disallow plane
> switching without the intermediate OFF state. Simplifies drivers, we
> don't have any hw that could do it anyway and current atomic userspace
> (weston) works like this already anyway.
>
> Cc: Thierry Reding
> Cc: Maarten Lankhorst
> Cc: Daniel Stone
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_atomic.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 434915448ea0..f27aae3fa765 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> return 0;
> }
>
> +static bool
> +plane_switching(struct drm_atomic_state *state,
> + struct drm_plane *plane,
> + struct drm_plane_state *plane_state)
plane_switching_crtc()?
> +{
> + struct drm_crtc_state *crtc_state, *curr_crtc_state;
> +
> + if (!plane->state->crtc || !plane_state->crtc)
> + return false;
> +
> + if (plane->state->crtc == plane_state->crtc)
> + return false;
> +
> + return true;
> +}
> +
> /**
> * drm_atomic_plane_check - check plane state
> * @plane: plane to check
> @@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane
> *plane,
> return -ENOSPC;
> }
>
> + if (plane_switching(state->state, plane, state)) {
> + DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
> + plane->base.id);
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> --
> 2.5.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
[PATCH] drm/atomic: refuse changing CRTC for planes directly
On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter
wrote:
> Very strictly speaking this is possible if you have special hw and
> genlocked CRTCs. In general switching a plane between two active CRTC
> just won't work so well and is probably not tested at all. Just forbid
> it.
So, I expect msm should actually be able to do this w/ dual-dsi (where
we are using two CRTC's, w/ synchronized flushes)..
Probably someone who has a dual-dsi panel should actually test that to
confirm. But it seems like it should work. Maybe we need something
in 'struct drm_crtc' so core can realize that two CRTC's are locked
together..
BR,
-R
> I've put this into the core since I really couldn't come up with a
> case where we don't want to enforce that. But if that ever happens it
> would be easy to move this check into helpers.
>
> v2: don't bother with complexity and just outright disallow plane
> switching without the intermediate OFF state. Simplifies drivers, we
> don't have any hw that could do it anyway and current atomic userspace
> (weston) works like this already anyway.
>
> Cc: Thierry Reding
> Cc: Maarten Lankhorst
> Cc: Daniel Stone
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_atomic.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 434915448ea0..f27aae3fa765 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> return 0;
> }
>
> +static bool
> +plane_switching(struct drm_atomic_state *state,
> + struct drm_plane *plane,
> + struct drm_plane_state *plane_state)
> +{
> + struct drm_crtc_state *crtc_state, *curr_crtc_state;
> +
> + if (!plane->state->crtc || !plane_state->crtc)
> + return false;
> +
> + if (plane->state->crtc == plane_state->crtc)
> + return false;
> +
> + return true;
> +}
> +
> /**
> * drm_atomic_plane_check - check plane state
> * @plane: plane to check
> @@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane
> *plane,
> return -ENOSPC;
> }
>
> + if (plane_switching(state->state, plane, state)) {
> + DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
> +plane->base.id);
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> --
> 2.5.0
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/atomic: refuse changing CRTC for planes directly
On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark wrote:
> On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter
> wrote:
>> Very strictly speaking this is possible if you have special hw and
>> genlocked CRTCs. In general switching a plane between two active CRTC
>> just won't work so well and is probably not tested at all. Just forbid
>> it.
>
> So, I expect msm should actually be able to do this w/ dual-dsi (where
> we are using two CRTC's, w/ synchronized flushes)..
>
> Probably someone who has a dual-dsi panel should actually test that to
> confirm. But it seems like it should work. Maybe we need something
> in 'struct drm_crtc' so core can realize that two CRTC's are locked
> together..
oh, and for most drivers, switching plane between CRTCs without an
off-cycle would probably also work for DSI cmd mode..
BR,
-R
>
>> I've put this into the core since I really couldn't come up with a
>> case where we don't want to enforce that. But if that ever happens it
>> would be easy to move this check into helpers.
>>
>> v2: don't bother with complexity and just outright disallow plane
>> switching without the intermediate OFF state. Simplifies drivers, we
>> don't have any hw that could do it anyway and current atomic userspace
>> (weston) works like this already anyway.
>>
>> Cc: Thierry Reding
>> Cc: Maarten Lankhorst
>> Cc: Daniel Stone
>> Signed-off-by: Daniel Vetter
>> ---
>> drivers/gpu/drm/drm_atomic.c | 22 ++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 434915448ea0..f27aae3fa765 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>> return 0;
>> }
>>
>> +static bool
>> +plane_switching(struct drm_atomic_state *state,
>> + struct drm_plane *plane,
>> + struct drm_plane_state *plane_state)
>> +{
>> + struct drm_crtc_state *crtc_state, *curr_crtc_state;
>> +
>> + if (!plane->state->crtc || !plane_state->crtc)
>> + return false;
>> +
>> + if (plane->state->crtc == plane_state->crtc)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> /**
>> * drm_atomic_plane_check - check plane state
>> * @plane: plane to check
>> @@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane
>> *plane,
>> return -ENOSPC;
>> }
>>
>> + if (plane_switching(state->state, plane, state)) {
>> + DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
>> +plane->base.id);
>> + return -EINVAL;
>> + }
>> +
>> return 0;
>> }
>>
>> --
>> 2.5.0
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/edid: Allow comma separated edid binaries. (v2)
Allow comma separated filenames in the edid_firmware parameter.
For example:
edid_firmware=eDP-1:edid/1280x480.bin,DP-2:edid/1920x1080.bin
v2: Use strsep() to simplify parsing of comma seperated string. (Matt)
Move initial bail before strdup. (Matt)
Reviewed-by: Matt Roper
Signed-off-by: Bob Paauwe
---
drivers/gpu/drm/drm_edid_load.c | 44 -
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index c5605fe..93b9275 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -264,27 +264,53 @@ out:
int drm_load_edid_firmware(struct drm_connector *connector)
{
const char *connector_name = connector->name;
- char *edidname = edid_firmware, *last, *colon;
+ char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
int ret;
struct edid *edid;
- if (*edidname == '\0')
+ if (edid_firmware[0] == '\0')
return 0;
- colon = strchr(edidname, ':');
- if (colon != NULL) {
- if (strncmp(connector_name, edidname, colon - edidname))
- return 0;
- edidname = colon + 1;
- if (*edidname == '\0')
- return 0;
+ /*
+* If there are multiple edid files specified and separated
+* by commas, search through the list looking for one that
+* matches the connector.
+*
+* If there's one or more that don't't specify a connector, keep
+* the last one found one as a fallback.
+*/
+ fwstr = kstrdup(edid_firmware, GFP_KERNEL);
+ edidstr = fwstr;
+
+ while ((edidname = strsep(&edidstr, ","))) {
+ colon = strchr(edidname, ':');
+ if (colon != NULL) {
+ if (strncmp(connector_name, edidname, colon-edidname))
+ continue;
+ edidname = colon + 1;
+ break;
+ } else {
+ if (*edidname != '\0') /* corner case: multiple ',' */
+ fallback = edidname;
+ }
+
}
+ if (fallback == NULL && edidname == NULL) {
+ kfree(fwstr);
+ return 0;
+ }
+
+ if (edidname == NULL && fallback)
+ edidname = fallback;
+
last = edidname + strlen(edidname) - 1;
if (*last == '\n')
*last = '\0';
edid = edid_load(connector, edidname, connector_name);
+ kfree(fwstr);
+
if (IS_ERR_OR_NULL(edid))
return 0;
--
2.1.0
[PATCH] drm/atomic: refuse changing CRTC for planes directly
On Wed, Aug 26, 2015 at 12:07:30PM -0400, Rob Clark wrote:
> On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark wrote:
> > On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter
> > wrote:
> >> Very strictly speaking this is possible if you have special hw and
> >> genlocked CRTCs. In general switching a plane between two active CRTC
> >> just won't work so well and is probably not tested at all. Just forbid
> >> it.
> >
> > So, I expect msm should actually be able to do this w/ dual-dsi (where
> > we are using two CRTC's, w/ synchronized flushes)..
> >
> > Probably someone who has a dual-dsi panel should actually test that to
> > confirm. But it seems like it should work. Maybe we need something
> > in 'struct drm_crtc' so core can realize that two CRTC's are locked
> > together..
>
> oh, and for most drivers, switching plane between CRTCs without an
> off-cycle would probably also work for DSI cmd mode..
You'd need to wait for any ongoing transfer on the old crtc to finish
before moving the plane. So that's not really any different than the
driver doing the dance with a vblank wait on a video mode display.
>
> BR,
> -R
>
> >
> >> I've put this into the core since I really couldn't come up with a
> >> case where we don't want to enforce that. But if that ever happens it
> >> would be easy to move this check into helpers.
> >>
> >> v2: don't bother with complexity and just outright disallow plane
> >> switching without the intermediate OFF state. Simplifies drivers, we
> >> don't have any hw that could do it anyway and current atomic userspace
> >> (weston) works like this already anyway.
> >>
> >> Cc: Thierry Reding
> >> Cc: Maarten Lankhorst
> >> Cc: Daniel Stone
> >> Signed-off-by: Daniel Vetter
> >> ---
> >> drivers/gpu/drm/drm_atomic.c | 22 ++
> >> 1 file changed, 22 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 434915448ea0..f27aae3fa765 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >> return 0;
> >> }
> >>
> >> +static bool
> >> +plane_switching(struct drm_atomic_state *state,
> >> + struct drm_plane *plane,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> + struct drm_crtc_state *crtc_state, *curr_crtc_state;
> >> +
> >> + if (!plane->state->crtc || !plane_state->crtc)
> >> + return false;
> >> +
> >> + if (plane->state->crtc == plane_state->crtc)
> >> + return false;
> >> +
> >> + return true;
> >> +}
> >> +
> >> /**
> >> * drm_atomic_plane_check - check plane state
> >> * @plane: plane to check
> >> @@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane
> >> *plane,
> >> return -ENOSPC;
> >> }
> >>
> >> + if (plane_switching(state->state, plane, state)) {
> >> + DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
> >> +plane->base.id);
> >> + return -EINVAL;
> >> + }
> >> +
> >> return 0;
> >> }
> >>
> >> --
> >> 2.5.0
> >>
> >> ___
> >> dri-devel mailing list
> >> dri-devel at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
[Intel-gfx] [PATCH] drm/atomic: refuse changing CRTC for planes directly
On Wed, 2015-08-26 at 18:53 +0300, Ville Syrjälä wrote: > On Wed, Aug 26, 2015 at 05:41:23PM +0200, Daniel Vetter wrote: > > Very strictly speaking this is possible if you have special hw and > > genlocked CRTCs. In general switching a plane between two active > > CRTC > > just won't work so well and is probably not tested at all. Just > > forbid > > it. > > > > I've put this into the core since I really couldn't come up with a > > case where we don't want to enforce that. But if that ever happens > > it > > would be easy to move this check into helpers. > > > > v2: don't bother with complexity and just outright disallow plane > > switching without the intermediate OFF state. Simplifies drivers, > > we > > don't have any hw that could do it anyway and current atomic > > userspace > > (weston) works like this already anyway. > > > > Cc: Thierry Reding > > Cc: Maarten Lankhorst > > Cc: Daniel Stone > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/drm_atomic.c | 22 ++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c > > b/drivers/gpu/drm/drm_atomic.c > > index 434915448ea0..f27aae3fa765 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct > > drm_plane *plane, > > return 0; > > } > > > > +static bool > > +plane_switching(struct drm_atomic_state *state, > > + struct drm_plane *plane, > > + struct drm_plane_state *plane_state) > > plane_switching_crtc()? With Ville's shed colour: Acked-by: Daniel Stone As this won't work in the general case, I'd prefer to see a hint to userspace that two CRTCs are genlocked before we enable it and people start to rely on it. Cheers, Daniel
[RFC][PATCH 1/2] dt-bindings: drm/mediatek: Add Mediatek DRM dts binding
Hi,
overall it looks to me like this binding is modeled after the Linux DRM
abstractions. Instead, the device nodes should be modeled after the
hardware.
Describing each function block as a separate device node is probably not
the best choice, as would be combining all devices in the mmsys range
into a single device node. So a somewhat arbitrary decision has to be
made what to group together. See my comments below:
Am Mittwoch, den 13.05.2015, 23:23 +0800 schrieb CK Hu:
> This patch includes
> 1. Mediatek DRM Device binding
> 2. Mediatek DSI Device binding
> 3. Mediatek CRTC Main Device binding
> 4. Mediatek DDP Device binding
>
> Signed-off-by: CK Hu
> ---
> .../bindings/drm/mediatek/mediatek,crtc-main.txt | 38
> ++
> .../bindings/drm/mediatek/mediatek,ddp.txt | 22 +
> .../bindings/drm/mediatek/mediatek,drm.txt | 27 +++
> .../bindings/drm/mediatek/mediatek,dsi.txt | 20
> 4 files changed, 107 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/drm/mediatek/mediatek,crtc-main.txt
> create mode 100644
> Documentation/devicetree/bindings/drm/mediatek/mediatek,ddp.txt
> create mode 100644
> Documentation/devicetree/bindings/drm/mediatek/mediatek,drm.txt
> create mode 100644
> Documentation/devicetree/bindings/drm/mediatek/mediatek,dsi.txt
>
> diff --git
> a/Documentation/devicetree/bindings/drm/mediatek/mediatek,crtc-main.txt
> b/Documentation/devicetree/bindings/drm/mediatek/mediatek,crtc-main.txt
> new file mode 100644
> index 000..5c6c420
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/mediatek/mediatek,crtc-main.txt
> @@ -0,0 +1,38 @@
> +Mediatek CRTC Main Device
> +
> +
> +The Mediatek CRTC Main device is a crtc device of DRM system.
"crtc" does not belong in the device tree. There is no crtc hardware.
What this node describes is a useful, but fixed configuration of a part
of the DISP subsystem function blocks.
In my opinion, it would be better to not describe the separate display
pipes in the device tree at all if they are dynamically configurable.
What for example if you model two separate fixed pipelines in the device
tree and then in the future you want to support the MERGE or SPLIT
blocks (I'm not sure what MERGE does, SPLIT seems to be needed for
8-lane DSI)?
As far as I currently understand, there are five source function blocks
that can read from memory (OVL0, OVL1, RDMA0, RDMA1, RDMA2) and three
sink function blocks (DSI0, DSI1, DPI0) that can be connected to panels,
or encoder bridges. How these map to the crtcs doesn't necessarily have
to be described in the device tree.
How about a single node that contains all of the DISP functional blocks
that don't need their own node (like DSI, which has to be connectable to
bridges or panels):
disp: disp at 0x1400c000 {
compatible = "mediatek,mt8173-disp";
interrupts = , /* OVL0 */
; /* OVL1 */
interrupt-names = "ovl0", "ovl1";
reg = <0 0x1400c000 0 0x1000>, /* OVL0 */
<0 0x1400d000 0 0x1000>, /* OVL1 */
<0 0x1400e000 0 0x1000>, /* RDMA0 */
<0 0x1400f000 0 0x1000>, /* RDMA1 */
<0 0x1401 0 0x1000>, /* RDMA2 */
<0 0x14013000 0 0x1000>, /* COLOR0 */
<0 0x14014000 0 0x1000>, /* COLOR1 */
<0 0x14015000 0 0x1000>, /* AAL */
<0 0x14016000 0 0x1000>, /* GAMMA */
<0 0x14017000 0 0x1000>, /* MERGE */
<0 0x14018000 0 0x1000>, /* SPLIT0 */
<0 0x14019000 0 0x1000>, /* SPLIT1 */
<0 0x1401a000 0 0x1000>, /* UFOE */
<0 0x1402 0 0x1000>; /* MUTEX */
<0 0x14023000 0 0x1000>; /* OD */
reg-names = "ovl0", "ovl1", "rdma0", "rdma1", "rdma2",
"color0", "color1", "aal", "gamma", "merge",
"split0", "split1", "ufoe", "mutex", "od";
clocks = <&mmsys CLK_MM_DISP_OVL0>,
<&mmsys CLK_MM_DISP_OVL1>,
<&mmsys CLK_MM_DISP_RDMA0>,
<&mmsys CLK_MM_DISP_RDMA1>,
<&mmsys CLK_MM_DISP_RDMA2>,
<&mmsys CLK_MM_DISP_COLOR0>,
<&mmsys CLK_MM_DISP_COLOR1>,
<&mmsys CLK_MM_DISP_AAL>,
<&mmsys CLK_MM_DISP_GAMMA>,
<&mmsys CLK_MM_DISP_MERGE>,
<&mmsys CLK_MM_DISP_SPLIT0>,
<&mmsys CLK_MM_DISP_SPLIT1>,
<&mmsys CLK_MM_DISP_UFOE>,
<&mmsys CLK_MM_MUTEX_32K>,
<&mmsys CLK_MM_DISP_OD>;
clock-names = "ovl0", "ovl1", "rdma0", "rdma1", "rdma2",
"color0", "color1", "aal", "gamma", "merge",
"split0", "split1", "ufoe", "mutex", "od";
power-domains = <&scpsys MT8173_POWER_DOMAIN_DIS>;
config = <&mmsys>; /* syscon */
};
How the muxes in the config
[PATCH] drm/atomic: refuse changing CRTC for planes directly
On Wed, Aug 26, 2015 at 12:30 PM, Ville Syrjälä
wrote:
> On Wed, Aug 26, 2015 at 12:07:30PM -0400, Rob Clark wrote:
>> On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark wrote:
>> > On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter > > ffwll.ch> wrote:
>> >> Very strictly speaking this is possible if you have special hw and
>> >> genlocked CRTCs. In general switching a plane between two active CRTC
>> >> just won't work so well and is probably not tested at all. Just forbid
>> >> it.
>> >
>> > So, I expect msm should actually be able to do this w/ dual-dsi (where
>> > we are using two CRTC's, w/ synchronized flushes)..
>> >
>> > Probably someone who has a dual-dsi panel should actually test that to
>> > confirm. But it seems like it should work. Maybe we need something
>> > in 'struct drm_crtc' so core can realize that two CRTC's are locked
>> > together..
>>
>> oh, and for most drivers, switching plane between CRTCs without an
>> off-cycle would probably also work for DSI cmd mode..
>
> You'd need to wait for any ongoing transfer on the old crtc to finish
> before moving the plane. So that's not really any different than the
> driver doing the dance with a vblank wait on a video mode display.
except that update would need to block from previous xfer anyways, so
there isn't really a race w/ frame n+1 like there would be with video
mode..
I do think that *somehow* we need to expose whether or not the display
is cmd-mode to userspace, since that factors into decisions about
whether it can immediately re-use a plane on another CRTC, and I think
it factors into discussion about some differences between ADF fencing
and upstream fencing as we add explicit fencing support to atomic..
but that is probably a different topic..
BR,
-R
>>
>> BR,
>> -R
>>
>> >
>> >> I've put this into the core since I really couldn't come up with a
>> >> case where we don't want to enforce that. But if that ever happens it
>> >> would be easy to move this check into helpers.
>> >>
>> >> v2: don't bother with complexity and just outright disallow plane
>> >> switching without the intermediate OFF state. Simplifies drivers, we
>> >> don't have any hw that could do it anyway and current atomic userspace
>> >> (weston) works like this already anyway.
>> >>
>> >> Cc: Thierry Reding
>> >> Cc: Maarten Lankhorst
>> >> Cc: Daniel Stone
>> >> Signed-off-by: Daniel Vetter
>> >> ---
>> >> drivers/gpu/drm/drm_atomic.c | 22 ++
>> >> 1 file changed, 22 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >> index 434915448ea0..f27aae3fa765 100644
>> >> --- a/drivers/gpu/drm/drm_atomic.c
>> >> +++ b/drivers/gpu/drm/drm_atomic.c
>> >> @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane
>> >> *plane,
>> >> return 0;
>> >> }
>> >>
>> >> +static bool
>> >> +plane_switching(struct drm_atomic_state *state,
>> >> + struct drm_plane *plane,
>> >> + struct drm_plane_state *plane_state)
>> >> +{
>> >> + struct drm_crtc_state *crtc_state, *curr_crtc_state;
>> >> +
>> >> + if (!plane->state->crtc || !plane_state->crtc)
>> >> + return false;
>> >> +
>> >> + if (plane->state->crtc == plane_state->crtc)
>> >> + return false;
>> >> +
>> >> + return true;
>> >> +}
>> >> +
>> >> /**
>> >> * drm_atomic_plane_check - check plane state
>> >> * @plane: plane to check
>> >> @@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane
>> >> *plane,
>> >> return -ENOSPC;
>> >> }
>> >>
>> >> + if (plane_switching(state->state, plane, state)) {
>> >> + DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
>> >> +plane->base.id);
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> return 0;
>> >> }
>> >>
>> >> --
>> >> 2.5.0
>> >>
>> >> ___
>> >> dri-devel mailing list
>> >> dri-devel at lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
[PATCH v2 00/11] drm/exynos: improve atomic modesetting
From: Gustavo Padovan Hi, This patchset adds a couple of changes to improve atomic modesetting: * add check for the START shadow register for FIMD to only finish the update when the screen was actually updated. * add asynchronous atomic commit, so now page flips can be run asynchronously. It also add infrastructure to serialize commits for the same CRTC and wait all plane updates to finish. * enable the DRIVER_ATOMIC feature to enable userspace to use atomic IOCTL with exynos. v2: rename prepare_plane/cleanup_plane to atomic_begin/atomic_flush Please review. Gustavo Gustavo Padovan (11): drm/exynos: don't track enabled state at exynos_crtc drm/exynos: fimd: unify call to exynos_drm_crtc_finish_pageflip() drm/exynos: add begin and flush phases for planes drm/exynos: fimd: move window protect code to atomic_begin/flush drm/exynos: check for pending fb before finish update drm/exynos: add macro to get the address of START_S reg drm/exynos: fimd: only finish update if START == START_S drm/exynos: add atomic asynchronous commit drm/exynos: wait all planes updates to finish drm/exynos: remove wait queue for pending page flip drm/exynos: Enable atomic modesetting feature drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 +- drivers/gpu/drm/exynos/exynos7_drm_decon.c| 10 +- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 69 +-- drivers/gpu/drm/exynos/exynos_drm_crtc.h | 4 +- drivers/gpu/drm/exynos/exynos_drm_drv.c | 158 +- drivers/gpu/drm/exynos/exynos_drm_drv.h | 24 +++- drivers/gpu/drm/exynos/exynos_drm_fb.c| 35 -- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 81 - drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 + drivers/gpu/drm/exynos/exynos_drm_vidi.c | 10 +- drivers/gpu/drm/exynos/exynos_mixer.c | 10 +- include/video/samsung_fimd.h | 1 + 12 files changed, 309 insertions(+), 105 deletions(-) -- 2.1.0
[PATCH v2 01/11] drm/exynos: don't track enabled state at exynos_crtc
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
---
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 c478997..94eb831 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -25,14 +25,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);
}
@@ -40,9 +35,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))
@@ -52,8 +44,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
@@ -172,9 +162,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);
@@ -187,9 +174,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 6b8a30f..a993aac 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -136,7 +136,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
[PATCH] drm/exynos: fimd: move window protect code to prepare/atomic_flush
From: Gustavo Padovan
Only set/clear the update bit in the CRTC's .atomic_begin()/flush()
so all planes are really committed at the same time.
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 57 +++-
1 file changed, 34 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 30c1409..48d4fbe 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -591,6 +591,16 @@ static void fimd_shadow_protect_win(struct fimd_context
*ctx,
{
u32 reg, bits, val;
+ /*
+* SHADOWCON/PRTCON register is used for enabling timing.
+*
+* for example, once only width value of a register is set,
+* if the dma is started then fimd hardware could malfunction so
+* with protect window setting, the register fields with prefix '_F'
+* wouldn't be updated at vsync also but updated once unprotect window
+* is set.
+*/
+
if (ctx->driver_data->has_shadowcon) {
reg = SHADOWCON;
bits = SHADOWCON_WINx_PROTECT(win);
@@ -607,6 +617,28 @@ static void fimd_shadow_protect_win(struct fimd_context
*ctx,
writel(val, ctx->regs + reg);
}
+static void fimd_atomic_begin(struct exynos_drm_crtc *crtc,
+ struct exynos_drm_plane *plane)
+{
+ struct fimd_context *ctx = crtc->ctx;
+
+ if (ctx->suspended)
+ return;
+
+ fimd_shadow_protect_win(ctx, plane->zpos, true);
+}
+
+static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
+ struct exynos_drm_plane *plane)
+{
+ struct fimd_context *ctx = crtc->ctx;
+
+ if (ctx->suspended)
+ return;
+
+ fimd_shadow_protect_win(ctx, plane->zpos, false);
+}
+
static void fimd_update_plane(struct exynos_drm_crtc *crtc,
struct exynos_drm_plane *plane)
{
@@ -622,20 +654,6 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
if (ctx->suspended)
return;
- /*
-* SHADOWCON/PRTCON register is used for enabling timing.
-*
-* for example, once only width value of a register is set,
-* if the dma is started then fimd hardware could malfunction so
-* with protect window setting, the register fields with prefix '_F'
-* wouldn't be updated at vsync also but updated once unprotect window
-* is set.
-*/
-
- /* protect windows */
- fimd_shadow_protect_win(ctx, win, true);
-
-
offset = plane->src_x * bpp;
offset += plane->src_y * pitch;
@@ -707,9 +725,6 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
if (ctx->driver_data->has_shadowcon)
fimd_enable_shadow_channel_path(ctx, win, true);
- /* Enable DMA channel and unprotect windows */
- fimd_shadow_protect_win(ctx, win, false);
-
if (ctx->i80_if)
atomic_set(&ctx->win_updated, 1);
}
@@ -723,16 +738,10 @@ static void fimd_disable_plane(struct exynos_drm_crtc
*crtc,
if (ctx->suspended)
return;
- /* protect windows */
- fimd_shadow_protect_win(ctx, win, true);
-
fimd_enable_video_output(ctx, win, false);
if (ctx->driver_data->has_shadowcon)
fimd_enable_shadow_channel_path(ctx, win, false);
-
- /* unprotect windows */
- fimd_shadow_protect_win(ctx, win, false);
}
static void fimd_enable(struct exynos_drm_crtc *crtc)
@@ -875,8 +884,10 @@ static const struct exynos_drm_crtc_ops fimd_crtc_ops = {
.enable_vblank = fimd_enable_vblank,
.disable_vblank = fimd_disable_vblank,
.wait_for_vblank = fimd_wait_for_vblank,
+ .atomic_begin = fimd_atomic_begin,
.update_plane = fimd_update_plane,
.disable_plane = fimd_disable_plane,
+ .atomic_flush = fimd_atomic_flush,
.te_handler = fimd_te_handler,
.clock_enable = fimd_dp_clock_enable,
};
--
2.1.0
[PATCH v2 02/11] drm/exynos: fimd: unify call to exynos_drm_crtc_finish_pageflip()
From: Gustavo Padovan
Unify handling of finished plane update to prepare for a following patch
that will check for the START and START_S regs to really make sure that
the plane was updated.
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 5def6bc..30c1409 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -896,15 +896,15 @@ static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
if (ctx->pipe < 0 || !ctx->drm_dev)
goto out;
- if (ctx->i80_if) {
- exynos_drm_crtc_finish_pageflip(ctx->crtc);
+ if (!ctx->i80_if)
+ drm_crtc_handle_vblank(&ctx->crtc->base);
+
+ exynos_drm_crtc_finish_pageflip(ctx->crtc);
+ if (ctx->i80_if) {
/* Exits triggering mode */
atomic_set(&ctx->triggering, 0);
} else {
- drm_crtc_handle_vblank(&ctx->crtc->base);
- exynos_drm_crtc_finish_pageflip(ctx->crtc);
-
/* set wait vsync event to zero and wake up queue. */
if (atomic_read(&ctx->wait_vsync_event)) {
atomic_set(&ctx->wait_vsync_event, 0);
--
2.1.0
[PATCH v2 03/11] drm/exynos: add begin and flush phases for planes
From: Gustavo Padovan
.atomic_begin() and .atomic_flush() allows to perform extra operations
before and after the update of planes. For FIMD for example this will
be used to enable disable the shadow protection bit.
Signed-off-by: Gustavo Padovan
---
v2: rename prepare_plane/cleanup_plane to atomic_begin/atomic_flush
---
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 19 +++
drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++
2 files changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 94eb831..54485b7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -73,16 +73,35 @@ static void exynos_crtc_atomic_begin(struct drm_crtc *crtc,
struct drm_crtc_state *old_crtc_state)
{
struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
+ struct drm_plane *plane;
if (crtc->state->event) {
WARN_ON(drm_crtc_vblank_get(crtc) != 0);
exynos_crtc->event = crtc->state->event;
}
+
+ drm_atomic_crtc_for_each_plane(plane, crtc) {
+ struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
+
+ if (exynos_crtc->ops->atomic_begin)
+ exynos_crtc->ops->atomic_begin(exynos_crtc,
+ exynos_plane);
+ }
}
static void exynos_crtc_atomic_flush(struct drm_crtc *crtc,
struct drm_crtc_state *old_crtc_state)
{
+ struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
+ struct drm_plane *plane;
+
+ drm_atomic_crtc_for_each_plane(plane, crtc) {
+ struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
+
+ if (exynos_crtc->ops->atomic_flush)
+ exynos_crtc->ops->atomic_flush(exynos_crtc,
+ exynos_plane);
+ }
}
static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h
b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index a993aac..28afecc 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -87,6 +87,8 @@ struct exynos_drm_plane {
* @disable_vblank: specific driver callback for disabling vblank interrupt.
* @wait_for_vblank: wait for vblank interrupt to make sure that
* hardware overlay is updated.
+ * @atomic_begin: prepare a window to receive a update
+ * @atomic_flush: mark the end of a window update
* @update_plane: apply hardware specific overlay data to registers.
* @disable_plane: disable hardware specific overlay.
* @te_handler: trigger to transfer video image at the tearing effect
@@ -107,10 +109,14 @@ struct exynos_drm_crtc_ops {
int (*enable_vblank)(struct exynos_drm_crtc *crtc);
void (*disable_vblank)(struct exynos_drm_crtc *crtc);
void (*wait_for_vblank)(struct exynos_drm_crtc *crtc);
+ void (*atomic_begin)(struct exynos_drm_crtc *crtc,
+ struct exynos_drm_plane *plane);
void (*update_plane)(struct exynos_drm_crtc *crtc,
struct exynos_drm_plane *plane);
void (*disable_plane)(struct exynos_drm_crtc *crtc,
struct exynos_drm_plane *plane);
+ void (*atomic_flush)(struct exynos_drm_crtc *crtc,
+ struct exynos_drm_plane *plane);
void (*te_handler)(struct exynos_drm_crtc *crtc);
void (*clock_enable)(struct exynos_drm_crtc *crtc, bool enable);
};
--
2.1.0
[PATCH v2 04/11] drm/exynos: fimd: move window protect code to atomic_begin/flush
From: Gustavo Padovan
Only set/clear the update bit in the CRTC's .atomic_begin()/flush()
so all planes are really committed at the same time.
Signed-off-by: Gustavo Padovan
---
v2: rename prepare_plane/cleanup_plane to atomic_begin/atomic_flush
---
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 57 +++-
1 file changed, 34 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 30c1409..005a996 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -591,6 +591,16 @@ static void fimd_shadow_protect_win(struct fimd_context
*ctx,
{
u32 reg, bits, val;
+ /*
+* SHADOWCON/PRTCON register is used for enabling timing.
+*
+* for example, once only width value of a register is set,
+* if the dma is started then fimd hardware could malfunction so
+* with protect window setting, the register fields with prefix '_F'
+* wouldn't be updated at vsync also but updated once unprotect window
+* is set.
+*/
+
if (ctx->driver_data->has_shadowcon) {
reg = SHADOWCON;
bits = SHADOWCON_WINx_PROTECT(win);
@@ -607,6 +617,28 @@ static void fimd_shadow_protect_win(struct fimd_context
*ctx,
writel(val, ctx->regs + reg);
}
+static void fimd_atomic_begin(struct exynos_drm_crtc *crtc,
+ struct exynos_drm_plane *plane)
+{
+ struct fimd_context *ctx = crtc->ctx;
+
+ if (ctx->suspended)
+ return;
+
+ fimd_shadow_protect_win(ctx, plane->zpos, true);
+}
+
+static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
+ struct exynos_drm_plane *plane)
+{
+ struct fimd_context *ctx = crtc->ctx;
+
+ if (ctx->suspended)
+ return;
+
+ fimd_shadow_protect_win(ctx, plane->zpos, false);
+}
+
static void fimd_update_plane(struct exynos_drm_crtc *crtc,
struct exynos_drm_plane *plane)
{
@@ -622,20 +654,6 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
if (ctx->suspended)
return;
- /*
-* SHADOWCON/PRTCON register is used for enabling timing.
-*
-* for example, once only width value of a register is set,
-* if the dma is started then fimd hardware could malfunction so
-* with protect window setting, the register fields with prefix '_F'
-* wouldn't be updated at vsync also but updated once unprotect window
-* is set.
-*/
-
- /* protect windows */
- fimd_shadow_protect_win(ctx, win, true);
-
-
offset = plane->src_x * bpp;
offset += plane->src_y * pitch;
@@ -707,9 +725,6 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
if (ctx->driver_data->has_shadowcon)
fimd_enable_shadow_channel_path(ctx, win, true);
- /* Enable DMA channel and unprotect windows */
- fimd_shadow_protect_win(ctx, win, false);
-
if (ctx->i80_if)
atomic_set(&ctx->win_updated, 1);
}
@@ -723,16 +738,10 @@ static void fimd_disable_plane(struct exynos_drm_crtc
*crtc,
if (ctx->suspended)
return;
- /* protect windows */
- fimd_shadow_protect_win(ctx, win, true);
-
fimd_enable_video_output(ctx, win, false);
if (ctx->driver_data->has_shadowcon)
fimd_enable_shadow_channel_path(ctx, win, false);
-
- /* unprotect windows */
- fimd_shadow_protect_win(ctx, win, false);
}
static void fimd_enable(struct exynos_drm_crtc *crtc)
@@ -875,8 +884,10 @@ static const struct exynos_drm_crtc_ops fimd_crtc_ops = {
.enable_vblank = fimd_enable_vblank,
.disable_vblank = fimd_disable_vblank,
.wait_for_vblank = fimd_wait_for_vblank,
+ .atomic_begin = fimd_atomic_begin,
.update_plane = fimd_update_plane,
.disable_plane = fimd_disable_plane,
+ .atomic_flush = fimd_atomic_flush,
.te_handler = fimd_te_handler,
.clock_enable = fimd_dp_clock_enable,
};
--
2.1.0
[PATCH v2 05/11] drm/exynos: check for pending fb before finish update
From: Gustavo Padovan
The current code was ignoring the end of update for all overlay planes,
caring only for the primary plane update in case of pageflip.
This change adds a change to start to check for pending updates for all
planes through exynos_plane->pending_fb. At the start of plane update the
pending_fb is set with the fb to be shown on the screen. Then only when to
fb is already presented in the screen we set pending_fb to NULL to
signal that the update was finished.
Signed-off-by: Gustavo Padovan
fixup! drm/exynos: check for pending fb before finish update
---
drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 +-
drivers/gpu/drm/exynos/exynos7_drm_decon.c| 10 +-
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 7 ---
drivers/gpu/drm/exynos/exynos_drm_crtc.h | 3 ++-
drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 +
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 10 +-
drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 ++
drivers/gpu/drm/exynos/exynos_drm_vidi.c | 10 +-
drivers/gpu/drm/exynos/exynos_mixer.c | 10 +-
9 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index 484e312..8d65e45 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -542,13 +542,21 @@ static irqreturn_t decon_lcd_sys_irq_handler(int irq,
void *dev_id)
{
struct decon_context *ctx = dev_id;
u32 val;
+ int win;
if (!test_bit(BIT_CLKS_ENABLED, &ctx->enabled))
goto out;
val = readl(ctx->addr + DECON_VIDINTCON1);
if (val & VIDINTCON1_INTFRMDONEPEND) {
- exynos_drm_crtc_finish_pageflip(ctx->crtc);
+ for (win = 0 ; win < WINDOWS_NR ; win++) {
+ struct exynos_drm_plane *plane = &ctx->planes[win];
+
+ if (!plane->pending_fb)
+ continue;
+
+ exynos_drm_crtc_finish_update(ctx->crtc, plane);
+ }
/* clear */
writel(VIDINTCON1_INTFRMDONEPEND,
diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index 0792654..7651499 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -623,6 +623,7 @@ static irqreturn_t decon_irq_handler(int irq, void *dev_id)
{
struct decon_context *ctx = (struct decon_context *)dev_id;
u32 val, clear_bit;
+ int win;
val = readl(ctx->regs + VIDINTCON1);
@@ -636,7 +637,14 @@ static irqreturn_t decon_irq_handler(int irq, void *dev_id)
if (!ctx->i80_if) {
drm_crtc_handle_vblank(&ctx->crtc->base);
- exynos_drm_crtc_finish_pageflip(ctx->crtc);
+ for (win = 0 ; win < WINDOWS_NR ; win++) {
+ struct exynos_drm_plane *plane = &ctx->planes[win];
+
+ if (!plane->pending_fb)
+ continue;
+
+ exynos_drm_crtc_finish_update(ctx->crtc, plane);
+ }
/* set wait vsync event to zero and wake up queue. */
if (atomic_read(&ctx->wait_vsync_event)) {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 54485b7..582e041 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -197,18 +197,19 @@ void exynos_drm_crtc_disable_vblank(struct drm_device
*dev, int pipe)
exynos_crtc->ops->disable_vblank(exynos_crtc);
}
-void exynos_drm_crtc_finish_pageflip(struct exynos_drm_crtc *exynos_crtc)
+void exynos_drm_crtc_finish_update(struct exynos_drm_crtc *exynos_crtc,
+ struct exynos_drm_plane *exynos_plane)
{
struct drm_crtc *crtc = &exynos_crtc->base;
unsigned long flags;
+ exynos_plane->pending_fb = NULL;
+
spin_lock_irqsave(&crtc->dev->event_lock, flags);
if (exynos_crtc->event) {
-
drm_crtc_send_vblank_event(crtc, exynos_crtc->event);
drm_crtc_vblank_put(crtc);
wake_up(&exynos_crtc->pending_flip_queue);
-
}
exynos_crtc->event = NULL;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
index 9e7027d..8bedfde 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
@@ -25,7 +25,8 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct
drm_device *drm_dev,
void *context);
int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe);
void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int pipe);
-void exynos_drm_crtc_finish_pageflip(struct exynos_drm_crtc
[PATCH v2 06/11] drm/exynos: add macro to get the address of START_S reg
From: Gustavo Padovan This macro is need to get the value of the START shadow register, that will tell if an framebuffer is currently displayed on the screen or not. Signed-off-by: Gustavo Padovan --- include/video/samsung_fimd.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h index 0530e5a..d8fc96e 100644 --- a/include/video/samsung_fimd.h +++ b/include/video/samsung_fimd.h @@ -296,6 +296,7 @@ /* Video buffer addresses */ #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) +#define VIDW_BUF_START_S(_buff)(0x40A0 + ((_buff) * 8)) #define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) #define VIDW_BUF_END(_buff)(0xD0 + ((_buff) * 8)) #define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8)) -- 2.1.0
[PATCH v2 07/11] drm/exynos: fimd: only finish update if START == START_S
From: Gustavo Padovan
fimd_update_plane() programs BUF_START[win] and during the update
BUF_START[win] is copied to BUF_START_S[win] (its shadow register)
and starts scanning out, then it raises a irq.
The fimd_irq_handler, in the case we have a pending_fb, will check
the fb value was copied to START_S register and finish the update
in case of success.
Based on patch from Daniel Kurtz
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index fc26c3e..d96044f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -59,6 +59,7 @@
#define VIDWnALPHA1(win) (VIDW_ALPHA + 0x04 + (win) * 8)
#define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8)
+#define VIDWx_BUF_START_S(win, buf)(VIDW_BUF_START_S(buf) + (win) * 8)
#define VIDWx_BUF_END(win, buf)(VIDW_BUF_END(buf) + (win) * 8)
#define VIDWx_BUF_SIZE(win, buf) (VIDW_BUF_SIZE(buf) + (win) * 4)
@@ -895,7 +896,7 @@ static const struct exynos_drm_crtc_ops fimd_crtc_ops = {
static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
{
struct fimd_context *ctx = (struct fimd_context *)dev_id;
- u32 val, clear_bit;
+ u32 val, clear_bit, start, start_s;
int win;
val = readl(ctx->regs + VIDINTCON1);
@@ -917,7 +918,10 @@ static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
if (!plane->pending_fb)
continue;
- exynos_drm_crtc_finish_update(ctx->crtc, plane);
+ start = readl(ctx->regs + VIDWx_BUF_START(win, 0));
+ start_s = readl(ctx->regs + VIDWx_BUF_START_S(win, 0));
+ if (start == start_s)
+ exynos_drm_crtc_finish_update(ctx->crtc, plane);
}
if (ctx->i80_if) {
--
2.1.0
[PATCH v2 08/11] drm/exynos: add atomic asynchronous commit
From: Gustavo Padovan
The atomic modesetting interfaces supports async commits that should be
implemented by the drivers. If drm core requests an async commit
exynos_atomic_commit() will now schedule a work task to run the update later.
It also serializes commits that needs to run on the same crtc, putting the
following commit to wait until the current one is finished.
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/exynos/exynos_drm_drv.c | 113
drivers/gpu/drm/exynos/exynos_drm_drv.h | 11
drivers/gpu/drm/exynos/exynos_drm_fb.c | 35 --
3 files changed, 124 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 72b88c7..fc207f8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -13,6 +13,8 @@
#include
#include
+#include
+#include
#include
#include
@@ -36,6 +38,56 @@
#define DRIVER_MAJOR 1
#define DRIVER_MINOR 0
+struct exynos_atomic_commit {
+ struct work_struct work;
+ struct drm_device *dev;
+ struct drm_atomic_state *state;
+ u32 crtcs;
+};
+
+static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit)
+{
+ struct drm_device *dev = commit->dev;
+ struct exynos_drm_private *priv = dev->dev_private;
+ struct drm_atomic_state *state = commit->state;
+
+ drm_atomic_helper_commit_modeset_disables(dev, state);
+
+ drm_atomic_helper_commit_modeset_enables(dev, state);
+
+ /*
+* Exynos can't update planes with CRTCs and encoders disabled,
+* its updates routines, specially for FIMD, requires the clocks
+* to be enabled. So it is necessary to handle the modeset operations
+* *before* the commit_planes() step, this way it will always
+* have the relevant clocks enabled to perform the update.
+*/
+
+ drm_atomic_helper_commit_planes(dev, state);
+
+ drm_atomic_helper_wait_for_vblanks(dev, state);
+
+ drm_atomic_helper_cleanup_planes(dev, state);
+
+ drm_atomic_state_free(state);
+
+ spin_lock(&priv->lock);
+ priv->pending &= ~commit->crtcs;
+ spin_unlock(&priv->lock);
+
+ wake_up_all(&priv->wait);
+
+ kfree(commit);
+}
+
+static void exynos_drm_atomic_work(struct work_struct *work)
+{
+ struct exynos_atomic_commit *commit = container_of(work,
+ struct exynos_atomic_commit, work);
+
+ exynos_atomic_commit_complete(commit);
+}
+
static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
{
struct exynos_drm_private *private;
@@ -47,6 +99,9 @@ static int exynos_drm_load(struct drm_device *dev, unsigned
long flags)
if (!private)
return -ENOMEM;
+ init_waitqueue_head(&private->wait);
+ spin_lock_init(&private->lock);
+
dev_set_drvdata(dev->dev, dev);
dev->dev_private = (void *)private;
@@ -149,6 +204,64 @@ static int exynos_drm_unload(struct drm_device *dev)
return 0;
}
+static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs)
+{
+ bool pending;
+
+ spin_lock(&priv->lock);
+ pending = priv->pending & crtcs;
+ spin_unlock(&priv->lock);
+
+ return pending;
+}
+
+int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state
*state,
+bool async)
+{
+ struct exynos_drm_private *priv = dev->dev_private;
+ struct exynos_atomic_commit *commit;
+ int i, ret;
+
+ commit = kzalloc(sizeof(*commit), GFP_KERNEL);
+ if (!commit)
+ return -ENOMEM;
+
+ ret = drm_atomic_helper_prepare_planes(dev, state);
+ if (ret) {
+ kfree(commit);
+ return ret;
+ }
+
+ /* This is the point of no return */
+
+ INIT_WORK(&commit->work, exynos_drm_atomic_work);
+ commit->dev = dev;
+ commit->state = state;
+
+ /* Wait until all affected CRTCs have completed previous commits and
+* mark them as pending.
+*/
+ for (i = 0; i < dev->mode_config.num_crtc; ++i) {
+ if (state->crtcs[i])
+ commit->crtcs |= 1 << drm_crtc_index(state->crtcs[i]);
+ }
+
+ wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs));
+
+ spin_lock(&priv->lock);
+ priv->pending |= commit->crtcs;
+ spin_unlock(&priv->lock);
+
+ drm_atomic_helper_swap_state(dev, state);
+
+ if (async)
+ schedule_work(&commit->work);
+ else
+ exynos_atomic_commit_complete(commit);
+
+ return 0;
+}
+
static int exynos_drm_suspend(struct drm_device *dev, pm_message_t state)
{
struct drm_connector *connector;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h
b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 8116803..b
[PATCH v2 09/11] drm/exynos: wait all planes updates to finish
From: Gustavo Padovan
Add infrastructure to wait for all planes updates to finish by using
an atomic_t variable to track how many pending updates we are waiting
plus a wait_queue for the wait part.
It also changes vblank behaviour and keeps it enabled for all types
of updates
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 18 +
drivers/gpu/drm/exynos/exynos_drm_crtc.h | 1 +
drivers/gpu/drm/exynos/exynos_drm_drv.c | 44 +++-
drivers/gpu/drm/exynos/exynos_drm_drv.h | 4 +++
4 files changed, 61 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 582e041..d6c2c3f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -75,10 +75,7 @@ static void exynos_crtc_atomic_begin(struct drm_crtc *crtc,
struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
struct drm_plane *plane;
- if (crtc->state->event) {
- WARN_ON(drm_crtc_vblank_get(crtc) != 0);
- exynos_crtc->event = crtc->state->event;
- }
+ exynos_crtc->event = crtc->state->event;
drm_atomic_crtc_for_each_plane(plane, crtc) {
struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
@@ -156,6 +153,8 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct
drm_device *drm_dev,
exynos_crtc->ops = ops;
exynos_crtc->ctx = ctx;
+ init_waitqueue_head(&exynos_crtc->wait_update);
+
crtc = &exynos_crtc->base;
private->crtc[pipe] = crtc;
@@ -197,6 +196,13 @@ void exynos_drm_crtc_disable_vblank(struct drm_device
*dev, int pipe)
exynos_crtc->ops->disable_vblank(exynos_crtc);
}
+void exynos_drm_crtc_wait_pending_update(struct exynos_drm_crtc *exynos_crtc)
+{
+ wait_event_timeout(exynos_crtc->wait_update,
+ (atomic_read(&exynos_crtc->pending_update) == 0),
+ msecs_to_jiffies(50));
+}
+
void exynos_drm_crtc_finish_update(struct exynos_drm_crtc *exynos_crtc,
struct exynos_drm_plane *exynos_plane)
{
@@ -205,10 +211,12 @@ void exynos_drm_crtc_finish_update(struct exynos_drm_crtc
*exynos_crtc,
exynos_plane->pending_fb = NULL;
+ if (atomic_dec_and_test(&exynos_crtc->pending_update))
+ wake_up(&exynos_crtc->wait_update);
+
spin_lock_irqsave(&crtc->dev->event_lock, flags);
if (exynos_crtc->event) {
drm_crtc_send_vblank_event(crtc, exynos_crtc->event);
- drm_crtc_vblank_put(crtc);
wake_up(&exynos_crtc->pending_flip_queue);
}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
index 8bedfde..f87d4ab 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
@@ -25,6 +25,7 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct
drm_device *drm_dev,
void *context);
int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe);
void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int pipe);
+void exynos_drm_crtc_wait_pending_update(struct exynos_drm_crtc *exynos_crtc);
void exynos_drm_crtc_finish_update(struct exynos_drm_crtc *exynos_crtc,
struct exynos_drm_plane *exynos_plane);
void exynos_drm_crtc_complete_scanout(struct drm_framebuffer *fb);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index fc207f8..881f178 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -45,11 +45,37 @@ struct exynos_atomic_commit {
u32 crtcs;
};
+static void exynos_atomic_wait_for_commit(struct drm_atomic_state *state)
+{
+ struct drm_crtc_state *crtc_state;
+ struct drm_crtc *crtc;
+ int i, ret;
+
+ for_each_crtc_in_state(state, crtc, crtc_state, i) {
+ struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
+
+ if (!crtc->state->enable)
+ continue;
+
+ ret = drm_crtc_vblank_get(crtc);
+ if (ret)
+ continue;
+
+ exynos_drm_crtc_wait_pending_update(exynos_crtc);
+ drm_crtc_vblank_put(crtc);
+ }
+}
+
static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit)
{
struct drm_device *dev = commit->dev;
struct exynos_drm_private *priv = dev->dev_private;
struct drm_atomic_state *state = commit->state;
+ struct drm_plane *plane;
+ struct drm_crtc *crtc;
+ struct drm_plane_state *plane_state;
+ struct drm_crtc_state *crtc_state;
+ int i;
drm_atomic_helper_commit_modeset_disables(dev, state);
@@ -63,9 +89,25 @@ static
[PATCH v2 10/11] drm/exynos: remove wait queue for pending page flip
From: Gustavo Padovan
Exynos atomic commit procedures already does this job of waiting for
pending updates to finish, that means using pending_flip_queue is
pointless now because the disable CRTC procedure will never happen
during a page_flip.
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 11 +--
drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 -
2 files changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index d6c2c3f..0872aa2f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -35,11 +35,6 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
{
struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
- /* wait for the completion of page flip. */
- if (!wait_event_timeout(exynos_crtc->pending_flip_queue,
- (exynos_crtc->event == NULL), HZ/20))
- exynos_crtc->event = NULL;
-
drm_crtc_vblank_off(crtc);
if (exynos_crtc->ops->disable)
@@ -146,8 +141,6 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct
drm_device *drm_dev,
if (!exynos_crtc)
return ERR_PTR(-ENOMEM);
- init_waitqueue_head(&exynos_crtc->pending_flip_queue);
-
exynos_crtc->pipe = pipe;
exynos_crtc->type = type;
exynos_crtc->ops = ops;
@@ -215,10 +208,8 @@ void exynos_drm_crtc_finish_update(struct exynos_drm_crtc
*exynos_crtc,
wake_up(&exynos_crtc->wait_update);
spin_lock_irqsave(&crtc->dev->event_lock, flags);
- if (exynos_crtc->event) {
+ if (exynos_crtc->event)
drm_crtc_send_vblank_event(crtc, exynos_crtc->event);
- wake_up(&exynos_crtc->pending_flip_queue);
- }
exynos_crtc->event = NULL;
spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h
b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 7193d94..b7ba21d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -145,7 +145,6 @@ struct exynos_drm_crtc {
struct drm_crtc base;
enum exynos_drm_output_type type;
unsigned intpipe;
- wait_queue_head_t pending_flip_queue;
struct drm_pending_vblank_event *event;
wait_queue_head_t wait_update;
atomic_tpending_update;
--
2.1.0
[PATCH v2 11/11] drm/exynos: Enable atomic modesetting feature
From: Gustavo Padovan
Now that atomic modesetting is implemented for exynos enable the
DRIVER_ATOMIC flag on the driver's features.
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/exynos/exynos_drm_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 881f178..c882fd3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -438,7 +438,8 @@ static const struct file_operations exynos_drm_driver_fops
= {
};
static struct drm_driver exynos_drm_driver = {
- .driver_features= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
+ .driver_features= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
+ | DRIVER_ATOMIC,
.load = exynos_drm_load,
.unload = exynos_drm_unload,
.open = exynos_drm_open,
--
2.1.0
drm/msm/dsi: hs_zero timing
On Wed, Aug 26, 2015 at 1:39 PM, Werner Johansson wrote: > > On Aug 26, 2015 08:34, "Hai Li" wrote: >> >> Hi Werner, >> >> Thanks for sharing this. The DPHY timings in downstream dtsi are exactly >> the same as the excel calculation, but slightly different from the output of >> drm code as you posted. (e.g hs_zero is 116 vs 118) >> I think it is caused by some precision loss during driver calculation, but >> I need to double check. >> >> Could you help to try configuring the same DPHY timings as downstream, but >> leave the values in DSIPHY_LNx_CFG4 as is, to see if it works? > > 116 to hs_zero works fine without any other changes (only 118 and every > eighth number up and down from there fails with this panel timing), so if we > can do something to make sure we hit a working value for hs_zero without > touching anything else that would be great! > > Thanks for checking the timings and let me know if you want me to test > anything else. Is the excel formula considered public so we can discuss it > here? btw, w/ some of these clk rounding issues, I suspect we need 'struct drm_display_mode' to be able to represent mode clock with greater accuracy than Khz.. The good news is that drm_display_mode is not part of userspace ABI (although drm_mode_modeinfo is.. and that is one path to populate drm_display_mode).. The bad news is that either way, drm_display_mode::clock is referenced in *many* places. Maybe we could add a new field to drm_display_mode to hold remainder in Hz, and driver just take that field from panel (and ignore whatever userspace may pass in)?? BR, -R > /wj
drm/msm/dsi: hs_zero timing
Thanks Werner to test it out. I will focus on the dphy timing calculation then. Itâs better to avoid discussing the excel formula publicly. :) Thanks, Hai From: Werner Johansson [mailto:werner.johans...@gmail.com] Sent: Wednesday, August 26, 2015 1:39 PM To: Hai Li Cc: Rob Clark; werner.johansson at sonymobile.com; dri-devel at lists.freedesktop.org Subject: RE: drm/msm/dsi: hs_zero timing On Aug 26, 2015 08:34, "Hai Li" mailto:hali at codeaurora.org> > wrote: > > Hi Werner, > > Thanks for sharing this. The DPHY timings in downstream dtsi are exactly the > same as the excel calculation, but slightly different from the output of drm > code as you posted. (e.g hs_zero is 116 vs 118) > I think it is caused by some precision loss during driver calculation, but I > need to double check. > > Could you help to try configuring the same DPHY timings as downstream, but > leave the values in DSIPHY_LNx_CFG4 as is, to see if it works? 116 to hs_zero works fine without any other changes (only 118 and every eighth number up and down from there fails with this panel timing), so if we can do something to make sure we hit a working value for hs_zero without touching anything else that would be great! Thanks for checking the timings and let me know if you want me to test anything else. Is the excel formula considered public so we can discuss it here? /wj -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150826/91186538/attachment.html>
[PATCH] drm/atomic: refuse changing CRTC for planes directly
On Wed, Aug 26, 2015 at 01:38:36PM -0400, Rob Clark wrote: > On Wed, Aug 26, 2015 at 12:30 PM, Ville Syrjälä > wrote: > > On Wed, Aug 26, 2015 at 12:07:30PM -0400, Rob Clark wrote: > >> On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark wrote: > >> > On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter >> > ffwll.ch> wrote: > >> >> Very strictly speaking this is possible if you have special hw and > >> >> genlocked CRTCs. In general switching a plane between two active CRTC > >> >> just won't work so well and is probably not tested at all. Just forbid > >> >> it. > >> > > >> > So, I expect msm should actually be able to do this w/ dual-dsi (where > >> > we are using two CRTC's, w/ synchronized flushes).. > >> > > >> > Probably someone who has a dual-dsi panel should actually test that to > >> > confirm. But it seems like it should work. Maybe we need something > >> > in 'struct drm_crtc' so core can realize that two CRTC's are locked > >> > together.. > >> > >> oh, and for most drivers, switching plane between CRTCs without an > >> off-cycle would probably also work for DSI cmd mode.. > > > > You'd need to wait for any ongoing transfer on the old crtc to finish > > before moving the plane. So that's not really any different than the > > driver doing the dance with a vblank wait on a video mode display. > > except that update would need to block from previous xfer anyways, so > there isn't really a race w/ frame n+1 like there would be with video > mode.. Why would it block if it's on a separate crtc? -- Ville Syrjälä Intel OTC
[PATCH] drm/atomic: refuse changing CRTC for planes directly
On Wed, Aug 26, 2015 at 1:53 PM, Ville Syrjälä wrote: > On Wed, Aug 26, 2015 at 01:38:36PM -0400, Rob Clark wrote: >> On Wed, Aug 26, 2015 at 12:30 PM, Ville Syrjälä >> wrote: >> > On Wed, Aug 26, 2015 at 12:07:30PM -0400, Rob Clark wrote: >> >> On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark >> >> wrote: >> >> > On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter > >> > ffwll.ch> wrote: >> >> >> Very strictly speaking this is possible if you have special hw and >> >> >> genlocked CRTCs. In general switching a plane between two active CRTC >> >> >> just won't work so well and is probably not tested at all. Just forbid >> >> >> it. >> >> > >> >> > So, I expect msm should actually be able to do this w/ dual-dsi (where >> >> > we are using two CRTC's, w/ synchronized flushes).. >> >> > >> >> > Probably someone who has a dual-dsi panel should actually test that to >> >> > confirm. But it seems like it should work. Maybe we need something >> >> > in 'struct drm_crtc' so core can realize that two CRTC's are locked >> >> > together.. >> >> >> >> oh, and for most drivers, switching plane between CRTCs without an >> >> off-cycle would probably also work for DSI cmd mode.. >> > >> > You'd need to wait for any ongoing transfer on the old crtc to finish >> > before moving the plane. So that's not really any different than the >> > driver doing the dance with a vblank wait on a video mode display. >> >> except that update would need to block from previous xfer anyways, so >> there isn't really a race w/ frame n+1 like there would be with video >> mode.. > > Why would it block if it's on a separate crtc? or, well, could block.. but anyways, the more realistic scenario for 2x dsi is dual-dsi to single panel w/ the two CRTC's locked in sync.. BR, -R > -- > Ville Syrjälä > Intel OTC
drm/msm/dsi: hs_zero timing
On Wed, Aug 26, 2015 at 2:22 PM, Werner Johansson wrote: > > On Aug 26, 2015 10:46, "Rob Clark" wrote: >> >> btw, w/ some of these clk rounding issues, I suspect we need 'struct >> drm_display_mode' to be able to represent mode clock with greater >> accuracy than Khz.. > > Interesting point! However, in this case I had to adjust the clock hundreds > of kHz to make it tick over one step of hs_zero, so it might not be > absolutely necessary here. Do we need better than 10ppm accuracy for display > timing (assuming 100MHz pixel clock, 1kHz step size and that I did the math > correctly)? We don't even have kHz accuracy with the PLLs in the QC > platforms we currently use.. > > I think the rounding error happens with the smaller numbers / intermediate > results but maybe clock should be represented in Hz internally anyway? Not > sure if it's worth changing the external-facing representation though? I'm not completely sure.. I did observe that we calculated slightly different settings w/ the auo novatek panel on z3, compared to what downstream had hard-coded in dts (which presumably came from magic-spreadsheet), because (I think) of rounding mode->clock to integer KHz. Although in the case of the z3 panel, it didn't seem to matter. What I am unsure about is whether other panels might be more sensitive to different settings. BR, -R
[PATCH 0/6] Properly detect swiotlb.
So this is only build tested as i am away from hardware right now. Idea is to provide reliable way to check if swiotlb is in use for a device or not. It seems swiotlb_nr_tbl() is no longer reliable for that. Please test. Cheers, Jérôme Glisse
[PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.
From: Jérôme Glisse
Some device like GPU do things differently if swiotlb is in use. We
use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
but this is unreliable. Patch add a simple helpers to check if any of
the dma_ops associated with a device points to the swiotlb functions,
making swiotlb check reliable for a device.
Signed-off-by: Jérôme Glisse
Cc: Konrad Rzeszutek Wilk
Cc: Alex Deucher
Cc: Ben Skeggs
Cc: Dave Airlie
Cc: lkml at vger.kernel.org
Cc: Daniel Vetter
---
include/linux/dma-mapping.h | 18 ++
1 file changed, 18 insertions(+)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index ac07ff0..eac911e 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev,
#define dma_unmap_len_set(PTR, LEN_NAME, VAL)do { } while (0)
#endif
+
+#ifdef CONFIG_SWIOTLB
+static inline bool swiotlb_in_use(struct device *dev)
+{
+ struct dma_map_ops *ops = get_dma_ops(dev);
+
+ return (ops->map_sg == swiotlb_map_sg_attrs ||
+ ops->unmap_sg == swiotlb_unmap_sg_attrs ||
+ ops->map_page == swiotlb_map_page);
+}
+#else
+static inline bool swiotlb_in_use(struct device *dev)
+{
+ return false;
+}
+#endif
+
+
#endif
--
2.1.0
[PATCH 2/6] drm/radeon: Use swiotlb_in_use() to know if swiotlb is enabled or not.
From: Jérôme Glisse
We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or
not for our device. Use the new helper to determine that.
Signed-off-by: Jérôme Glisse
Cc: Konrad Rzeszutek Wilk
Cc: Alex Deucher
Cc: Ben Skeggs
Cc: Dave Airlie
Cc: lkml at vger.kernel.org
Cc: Daniel Vetter
---
drivers/gpu/drm/radeon/radeon_ttm.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
b/drivers/gpu/drm/radeon/radeon_ttm.c
index 06ac59fe..5c9814a 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -38,7 +38,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -742,11 +741,9 @@ static int radeon_ttm_tt_populate(struct ttm_tt *ttm)
}
#endif
-#ifdef CONFIG_SWIOTLB
- if (swiotlb_nr_tbl()) {
+ if (swiotlb_in_use(rdev->dev)) {
return ttm_dma_populate(>t->ttm, rdev->dev);
}
-#endif
r = ttm_pool_populate(ttm);
if (r) {
@@ -794,12 +791,10 @@ static void radeon_ttm_tt_unpopulate(struct ttm_tt *ttm)
}
#endif
-#ifdef CONFIG_SWIOTLB
- if (swiotlb_nr_tbl()) {
+ if (swiotlb_in_use(rdev->dev)) {
ttm_dma_unpopulate(>t->ttm, rdev->dev);
return;
}
-#endif
for (i = 0; i < ttm->num_pages; i++) {
if (gtt->ttm.dma_address[i]) {
@@ -1169,10 +1164,8 @@ static int radeon_ttm_debugfs_init(struct radeon_device
*rdev)
count = ARRAY_SIZE(radeon_ttm_debugfs_list);
-#ifdef CONFIG_SWIOTLB
- if (!swiotlb_nr_tbl())
+ if (!swiotlb_in_use(rdev->dev))
--count;
-#endif
return radeon_debugfs_add_files(rdev, radeon_ttm_debugfs_list, count);
#else
--
2.1.0
[PATCH 3/6] drm/nouveau: Use swiotlb_in_use() to know if swiotlb is enabled or not.
From: Jérôme Glisse
We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or
not for our device. Use the new helper to determine that.
Signed-off-by: Jérôme Glisse
Cc: Konrad Rzeszutek Wilk
Cc: Alex Deucher
Cc: Ben Skeggs
Cc: Dave Airlie
Cc: lkml at vger.kernel.org
Cc: Daniel Vetter
---
drivers/gpu/drm/nouveau/nouveau_bo.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 6edcce1..f601049 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -28,7 +28,6 @@
*/
#include
-#include
#include "nouveau_drm.h"
#include "nouveau_dma.h"
@@ -1504,11 +1503,9 @@ nouveau_ttm_tt_populate(struct ttm_tt *ttm)
}
#endif
-#ifdef CONFIG_SWIOTLB
- if (swiotlb_nr_tbl()) {
+ if (swiotlb_in_use(pdev)) {
return ttm_dma_populate((void *)ttm, dev->dev);
}
-#endif
r = ttm_pool_populate(ttm);
if (r) {
@@ -1572,12 +1569,10 @@ nouveau_ttm_tt_unpopulate(struct ttm_tt *ttm)
}
#endif
-#ifdef CONFIG_SWIOTLB
- if (swiotlb_nr_tbl()) {
+ if (swiotlb_in_use(pdev)) {
ttm_dma_unpopulate((void *)ttm, dev->dev);
return;
}
-#endif
for (i = 0; i < ttm->num_pages; i++) {
if (ttm_dma->dma_address[i]) {
--
2.1.0
[PATCH 5/6] drm/i915: Use swiotlb_in_use() to know if swiotlb is enabled.
From: Jérôme Glisse
We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or
not for our device. Use the new helper to determine that.
Signed-off-by: Jérôme Glisse
Cc: Konrad Rzeszutek Wilk
Cc: Alex Deucher
Cc: Ben Skeggs
Cc: Dave Airlie
Cc: lkml at vger.kernel.org
Cc: Daniel Vetter
---
drivers/gpu/drm/i915/i915_gem.c | 9 +++--
drivers/gpu/drm/i915/i915_gem_userptr.c | 14 +-
2 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 52b446b..a67b649 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2251,14 +2251,13 @@ i915_gem_object_get_pages_gtt(struct
drm_i915_gem_object *obj)
goto err_pages;
}
}
-#ifdef CONFIG_SWIOTLB
- if (swiotlb_nr_tbl()) {
+ if (swiotlb_in_use(obj->base.dev->dev)) {
st->nents++;
sg_set_page(sg, page, PAGE_SIZE, 0);
sg = sg_next(sg);
continue;
}
-#endif
+
if (!i || page_to_pfn(page) != last_pfn + 1) {
if (i)
sg = sg_next(sg);
@@ -2272,9 +2271,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object
*obj)
/* Check that the i965g/gm workaround works. */
WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x0010UL));
}
-#ifdef CONFIG_SWIOTLB
- if (!swiotlb_nr_tbl())
-#endif
+ if (!swiotlb_in_use(obj->base.dev->dev))
sg_mark_end(sg);
obj->pages = st;
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 8fd431b..ecf03b7 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -506,14 +506,9 @@ struct get_pages_work {
struct task_struct *task;
};
-#if IS_ENABLED(CONFIG_SWIOTLB)
-#define swiotlb_active() swiotlb_nr_tbl()
-#else
-#define swiotlb_active() 0
-#endif
-
static int
-st_set_pages(struct sg_table **st, struct page **pvec, int num_pages)
+st_set_pages(struct device *dev, struct sg_table **st,
+struct page **pvec, int num_pages)
{
struct scatterlist *sg;
int ret, n;
@@ -522,7 +517,7 @@ st_set_pages(struct sg_table **st, struct page **pvec, int
num_pages)
if (*st == NULL)
return -ENOMEM;
- if (swiotlb_active()) {
+ if (swiotlb_in_use(dev)) {
ret = sg_alloc_table(*st, num_pages, GFP_KERNEL);
if (ret)
goto err;
@@ -549,9 +544,10 @@ static int
__i915_gem_userptr_set_pages(struct drm_i915_gem_object *obj,
struct page **pvec, int num_pages)
{
+ struct device *dev = obj->base.dev->dev;
int ret;
- ret = st_set_pages(&obj->pages, pvec, num_pages);
+ ret = st_set_pages(dev, &obj->pages, pvec, num_pages);
if (ret)
return ret;
--
2.1.0
[PATCH 6/6] drm/amdgpu: Use swiotlb_in_use() to know if swiotlb is enabled.
From: Jérôme Glisse
We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or
not for our device. Use the new helper to determine that.
Signed-off-by: Jérôme Glisse
Cc: Konrad Rzeszutek Wilk
Cc: Alex Deucher
Cc: Ben Skeggs
Cc: Dave Airlie
Cc: lkml at vger.kernel.org
Cc: Daniel Vetter
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index dd3415d..bb48fc5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -38,7 +38,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -692,11 +691,9 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm)
adev = amdgpu_get_adev(ttm->bdev);
-#ifdef CONFIG_SWIOTLB
- if (swiotlb_nr_tbl()) {
+ if (swiotlb_in_use(adev->dev)) {
return ttm_dma_populate(>t->ttm, adev->dev);
}
-#endif
r = ttm_pool_populate(ttm);
if (r) {
@@ -738,12 +735,10 @@ static void amdgpu_ttm_tt_unpopulate(struct ttm_tt *ttm)
adev = amdgpu_get_adev(ttm->bdev);
-#ifdef CONFIG_SWIOTLB
- if (swiotlb_nr_tbl()) {
+ if (swiotlb_in_use(adev->dev)) {
ttm_dma_unpopulate(>t->ttm, adev->dev);
return;
}
-#endif
for (i = 0; i < ttm->num_pages; i++) {
if (gtt->ttm.dma_address[i]) {
@@ -1190,10 +1185,8 @@ static int amdgpu_ttm_debugfs_init(struct amdgpu_device
*adev)
count = ARRAY_SIZE(amdgpu_ttm_debugfs_list);
-#ifdef CONFIG_SWIOTLB
- if (!swiotlb_nr_tbl())
+ if (!swiotlb_in_use(adev->dev))
--count;
-#endif
return amdgpu_debugfs_add_files(adev, amdgpu_ttm_debugfs_list, count);
#else
--
2.1.0
[PATCH 4/6] drm/vmwgfx: Use swiotlb_in_use() to know if swiotlb is enabled.
From: Jérôme Glisse We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or not for our device. Use the new helper to determine that. Signed-off-by: Jérôme Glisse Cc: Konrad Rzeszutek Wilk Cc: Alex Deucher Cc: Ben Skeggs Cc: Dave Airlie Cc: lkml at vger.kernel.org Cc: Daniel Vetter --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 620bb5c..a2b0ec0 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -517,10 +517,8 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv) if (dma_ops->sync_single_for_cpu) dev_priv->map_mode = vmw_dma_alloc_coherent; -#ifdef CONFIG_SWIOTLB - if (swiotlb_nr_tbl() == 0) + if (!swiotlb_in_use(dev_priv->dev->dev)) dev_priv->map_mode = vmw_dma_map_populate; -#endif #ifdef CONFIG_INTEL_IOMMU out_fixup: -- 2.1.0
[PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.
On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse at redhat.com wrote:
> From: Jérôme Glisse
>
> Some device like GPU do things differently if swiotlb is in use. We
> use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> but this is unreliable. Patch add a simple helpers to check if any of
Why is it unreliable?
> the dma_ops associated with a device points to the swiotlb functions,
> making swiotlb check reliable for a device.
>
> Signed-off-by: Jérôme Glisse
> Cc: Konrad Rzeszutek Wilk
> Cc: Alex Deucher
> Cc: Ben Skeggs
> Cc: Dave Airlie
> Cc: lkml at vger.kernel.org
> Cc: Daniel Vetter
> ---
> include/linux/dma-mapping.h | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index ac07ff0..eac911e 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device
> *dev,
> #define dma_unmap_len_set(PTR, LEN_NAME, VAL)do { } while (0)
> #endif
>
> +
> +#ifdef CONFIG_SWIOTLB
> +static inline bool swiotlb_in_use(struct device *dev)
> +{
> + struct dma_map_ops *ops = get_dma_ops(dev);
> +
> + return (ops->map_sg == swiotlb_map_sg_attrs ||
> + ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> + ops->map_page == swiotlb_map_page);
That won't work. What if we use xen-swiotlb which has different function
names?
> +}
> +#else
> +static inline bool swiotlb_in_use(struct device *dev)
> +{
> + return false;
> +}
> +#endif
> +
> +
> #endif
> --
> 2.1.0
>
[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
[Bug 86351] HDMI audio garbled output on Radeon R9 280X
https://bugzilla.kernel.org/show_bug.cgi?id=86351 --- Comment #16 from Lorenzo Bona --- Any hint on this? -- You are receiving this mail because: You are watching the assignee of the bug.
[PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.
On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse at redhat.com wrote:
> > From: Jérôme Glisse
> >
> > Some device like GPU do things differently if swiotlb is in use. We
> > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> > but this is unreliable. Patch add a simple helpers to check if any of
>
> Why is it unreliable?
Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb
is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f
which cleanup swiotlb init and always allocate default size. Which i believe
is a waste of memory. So we need to add a real helper to know if swiotlb is
in use or not and we should not rely on expectation of some swiotlb value.
>
> > the dma_ops associated with a device points to the swiotlb functions,
> > making swiotlb check reliable for a device.
> >
> > Signed-off-by: Jérôme Glisse
> > Cc: Konrad Rzeszutek Wilk
> > Cc: Alex Deucher
> > Cc: Ben Skeggs
> > Cc: Dave Airlie
> > Cc: lkml at vger.kernel.org
> > Cc: Daniel Vetter
> > ---
> > include/linux/dma-mapping.h | 18 ++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index ac07ff0..eac911e 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device
> > *dev,
> > #define dma_unmap_len_set(PTR, LEN_NAME, VAL)do { } while (0)
> > #endif
> >
> > +
> > +#ifdef CONFIG_SWIOTLB
> > +static inline bool swiotlb_in_use(struct device *dev)
> > +{
> > + struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > + return (ops->map_sg == swiotlb_map_sg_attrs ||
> > + ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > + ops->map_page == swiotlb_map_page);
>
> That won't work. What if we use xen-swiotlb which has different function
> names?
I didn't thought about xen, always doing things differently, i think xen is
just a matter of also testing for the xen function. I just wanted to have
the helper in common code and only rely on common things, instead of having
to add a per arch helper.
Cheers,
Jérôme
[PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.
On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
> On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse at redhat.com wrote:
> > > From: Jérôme Glisse
> > >
> > > Some device like GPU do things differently if swiotlb is in use. We
> > > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> > > but this is unreliable. Patch add a simple helpers to check if any of
> >
> > Why is it unreliable?
>
> Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb
> is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f
> which cleanup swiotlb init and always allocate default size. Which i believe
> is a waste of memory. So we need to add a real helper to know if swiotlb is
> in use or not and we should not rely on expectation of some swiotlb value.
Ah right, that patch. That should have been part of the description
I believe.
>
> >
> > > the dma_ops associated with a device points to the swiotlb functions,
> > > making swiotlb check reliable for a device.
> > >
> > > Signed-off-by: Jérôme Glisse
> > > Cc: Konrad Rzeszutek Wilk
> > > Cc: Alex Deucher
> > > Cc: Ben Skeggs
> > > Cc: Dave Airlie
> > > Cc: lkml at vger.kernel.org
> > > Cc: Daniel Vetter
> > > ---
> > > include/linux/dma-mapping.h | 18 ++
> > > 1 file changed, 18 insertions(+)
> > >
> > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > index ac07ff0..eac911e 100644
> > > --- a/include/linux/dma-mapping.h
> > > +++ b/include/linux/dma-mapping.h
> > > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct
> > > device *dev,
> > > #define dma_unmap_len_set(PTR, LEN_NAME, VAL)do { } while (0)
> > > #endif
> > >
> > > +
> > > +#ifdef CONFIG_SWIOTLB
> > > +static inline bool swiotlb_in_use(struct device *dev)
> > > +{
> > > + struct dma_map_ops *ops = get_dma_ops(dev);
> > > +
> > > + return (ops->map_sg == swiotlb_map_sg_attrs ||
> > > + ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > > + ops->map_page == swiotlb_map_page);
> >
> > That won't work. What if we use xen-swiotlb which has different function
> > names?
>
> I didn't thought about xen, always doing things differently, i think xen is
> just a matter of also testing for the xen function. I just wanted to have
> the helper in common code and only rely on common things, instead of having
> to add a per arch helper.
There has to be a better way. Perhaps you can expand SWIOTLB to actually
check if it is in use?
>
> Cheers,
> Jérôme
[PATCH] drm/atomic: refuse changing CRTC for planes directly
Very strictly speaking this is possible if you have special hw and
genlocked CRTCs. In general switching a plane between two active CRTC
just won't work so well and is probably not tested at all. Just forbid
it.
I've put this into the core since right now no helper or driver copes
with it, no userspace has code for it and no one asks for it. Yes
there's piles of corner-cases where this would be possible to do this
like:
- switch from inactive crtc to active crtc
- swithc from active crtc to inactive crtc
- genlocked display
- invisible plane (to do whatever)
- idle plane hw due to dsi cmd mode/psr
- whatever
but looking at details it's not that easy to implement this correctly.
Hence just put it into the core and add a comment, since the only
userspace we have right now for atomic (weston) doesn't want to use
direct plane switching either.
v2: don't bother with complexity and just outright disallow plane
switching without the intermediate OFF state. Simplifies drivers, we
don't have any hw that could do it anyway and current atomic userspace
(weston) works like this already anyway.
v3: Bikeshed function name (Ville) and add comment (Rob).
v4: Also bikeshed commit message (Rob).
Cc: Thierry Reding
Cc: Maarten Lankhorst
Cc: Daniel Stone
Acked-by: Daniel Stone
Signed-off-by: Daniel Vetter
---
Imo this is bikeshedded enough, so either someone takes this or
someone else can mangle this patch more.
-Daniel
---
drivers/gpu/drm/drm_atomic.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 1066e4b658cf..40ddd6aa100f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -663,6 +663,27 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
return 0;
}
+static bool
+plane_switching_crtc(struct drm_atomic_state *state,
+struct drm_plane *plane,
+struct drm_plane_state *plane_state)
+{
+ struct drm_crtc_state *crtc_state, *curr_crtc_state;
+
+ if (!plane->state->crtc || !plane_state->crtc)
+ return false;
+
+ if (plane->state->crtc == plane_state->crtc)
+ return false;
+
+ /* This could be refined, but currently there's no helper or driver code
+* to implement direct switching of active planes nor userspace to take
+* advantage of more direct plane switching without the intermediate
+* full OFF state.
+*/
+ return true;
+}
+
/**
* drm_atomic_plane_check - check plane state
* @plane: plane to check
@@ -734,6 +755,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
return -ENOSPC;
}
+ if (plane_switching_crtc(state->state, plane, state)) {
+ DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
+plane->base.id);
+ return -EINVAL;
+ }
+
return 0;
}
--
2.5.0
[PATCH 1/3] drm/dp: Define AUX_RETRY_INTERVAL as 500 us
From: Ville Syrjälä Currently we react to native and i2c defers by waiting either 400-500 us or 500-600 us, depending on which code path we take. Consolidate them all to one define AUX_RETRY_INTERVAL which defines the minimum interval. Since we've been using two different intervals pick the longer of them and define AUX_RETRY_INTERVAL as 500 us. For the maximum just use AUX_RETRY_INTERVAL+100 us. I want to have a define for this so that I can use it when calculating the estimated duration of i2c-over-aux transfers. Without a define it would be very easy to change the sleep duration and neglect to update the i2c-over-aux estimates. Cc: Simon Farnsworth Cc: moosotc at gmail.com Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/drm_dp_helper.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 80a02a4..7069e54 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -159,6 +159,8 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw) } EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate); +#define AUX_RETRY_INTERVAL 500 /* us */ + /** * DOC: dp helpers * @@ -213,7 +215,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request, return -EIO; case DP_AUX_NATIVE_REPLY_DEFER: - usleep_range(400, 500); + usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100); break; } } @@ -476,7 +478,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) * For now just defer for long enough to hopefully be * safe for all use-cases. */ - usleep_range(500, 600); + usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100); continue; default: @@ -506,7 +508,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) aux->i2c_defer_count++; if (defer_i2c < 7) defer_i2c++; - usleep_range(400, 500); + usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100); continue; default: -- 2.4.6
[PATCH v2 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed
From: Ville Syrjälä
Calculate the number of retries we should do for each i2c-over-aux
message based on the time it takes to perform the i2c transfer vs. the
aux transfer. We assume the shortest possible length for the aux
transfer, and the longest possible (exluding clock stretching) for the
i2c transfer.
The DP spec has some examples on how to calculate this, but we don't
calculate things quite the same way. The spec doesn't account for the
retry interval (assumes immediate retry on defer), and doesn't assume
the best/worst case behaviour as we do.
Note that currently we assume 10 kHz speed for the i2c bus. Some real
world devices (eg. some Apple DP->VGA dongle) fails with less than 16
retries. and that would correspond to something close to 15 kHz (with
our method of calculating things) But let's just go for 10 kHz to be
on the safe side. Ideally we should query/set the i2c bus speed via
DPCD but for now this should at leaast remove the regression from the
1->16 byte trasnfer size change. And of course if the sink completes
the transfer quicker this shouldn't slow things down since we don't
change the interval between retries.
I did a few experiments with a DP->DVI dongle I have that allows you
to change the i2c bus speed. Here are the results of me changing the
actual bus speed and the assumed bus speed and seeing when we start
to fail the operation:
actual i2c khz assumed i2c khz max retries
1 1 ok -> 2 fail 211 ok -> 106 fail
5 8 ok -> 9 fail 27 ok -> 24 fail
10 17 ok -> 18 fail13 ok -> 12 fail
100 210 ok -> 211 fail 2 ok -> 1 fail
So based on that we have a fairly decent safety margin baked into
the formula to calculate the max number of retries.
Fixes a regression with some DP dongles from:
commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
Author: Simon Farnsworth
Date: Tue Feb 10 18:38:08 2015 +
drm/dp: Use large transactions for I2C over AUX
v2: Use best case for AUX and worst case for i2c (Simon Farnsworth)
Add a define our AUX retry interval and account for it
Cc: Simon Farnsworth
Cc: moosotc at gmail.com
Tested-by: moosotc at gmail.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/drm_dp_helper.c | 81 -
1 file changed, 79 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 7069e54..23b9fcc 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -424,6 +424,78 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter
*adapter)
I2C_FUNC_10BIT_ADDR;
}
+#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
+#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
+#define AUX_STOP_LEN 4
+#define AUX_CMD_LEN 4
+#define AUX_ADDRESS_LEN 20
+#define AUX_REPLY_PAD_LEN 4
+#define AUX_LENGTH_LEN 8
+
+/*
+ * Calculate the length of the AUX request/reply. Gives the "best"
+ * case estimate, ie. successful while as short as possible.
+ */
+static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
+{
+ int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+ AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
+
+ if ((msg->request & DP_AUX_I2C_READ) == 0)
+ len += msg->size * 8;
+
+ return len;
+}
+
+static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
+{
+ int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+ AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
+
+ /*
+* For read we expect what was asked. For writes there will
+* be 0 or 1 data bytes. Assume 0 for the "best" case.
+*/
+ if (msg->request & DP_AUX_I2C_READ)
+ len += msg->size * 8;
+
+ return len;
+}
+
+#define I2C_START_LEN 1
+#define I2C_STOP_LEN 1
+#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
+#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
+
+/*
+ * Calculate the length of the i2c transfer (in AUX clocks)
+ * assuming the i2c bus speed is as specified. Gives the the
+ * "worst" case estimate, ie. successful while as long as possible.
+ * Doesn't account the the "MOT" bit, and instead assumes each
+ * message includes a START, ADDRESS and STOP. Neither does it
+ * account for additional random variables such as clock stretching.
+ */
+static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
+ int i2c_speed_khz)
+{
+ return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
+ I2C_STOP_LEN) * 1000 / i2c_speed_khz;
+}
+
+/*
+ * Deterine how many retries should be attempted to successfully transfer
+ * the specified message, based on the estimated durations of the
+ * i2c and AUX transfers.
+ */
+static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
+
[PATCH 3/3] drm/dp: Add dp_aux_i2c_speed_khz module param to set the assume i2c bus speed
From: Ville Syrjälä
To help with debugging i2c-over-aux issues, add a module parameter than
can be used to tweak the assumed i2c bus speed, and thus the maximum
number of retries we will do for each aux message.
Cc: Simon Farnsworth
Cc: moosotc at gmail.com
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/drm_dp_helper.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 23b9fcc..ee5cd86 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -497,6 +497,15 @@ static int drm_dp_i2c_retry_count(const struct
drm_dp_aux_msg *msg,
}
/*
+ * FIXME currently assumes 10 kHz as some real world devices seem
+ * to require it. We should query/set the speed via DPCD if supported.
+ */
+static int dp_aux_i2c_speed_khz __read_mostly = 10;
+module_param_unsafe(dp_aux_i2c_speed_khz, int, 0644);
+MODULE_PARM_DESC(dp_aux_i2c_speed_khz,
+"Assumed speed of the i2c bus in kHz, (1-400, default 10)");
+
+/*
* Transfer a single I2C-over-AUX message and handle various error conditions,
* retrying the transaction as appropriate. It is assumed that the
* aux->transfer function does not modify anything in the msg other than the
@@ -514,10 +523,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
* before giving up the AUX transaction.
*
* We also try to account for the i2c bus speed.
-* FIXME currently assumes 10 kHz as some real world devices seem
-* to require it. We should query/set the speed via DPCD if supported.
*/
- int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
+ int max_retries = max(7, drm_dp_i2c_retry_count(msg,
dp_aux_i2c_speed_khz));
for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c);
retry++) {
mutex_lock(&aux->hw_mutex);
--
2.4.6
[Bug 87856] Driver load fails with no error on ppc64 host
https://bugs.freedesktop.org/show_bug.cgi?id=87856 --- Comment #12 from Alex Perez --- I can confirm that the driver fails with no error here as well, on an e5500-based ppc64 system. Everything works (with acceleration) when depth is set to 16 bits. I know a handful of other people who have had the same problem on 32-bit ppc hardware (modern, non-macppc), so we can be reasonably confident that this is a broad ppc-specific issue (possibly be/byte-swapping related?) -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150826/cb5c74d7/attachment.html>
[PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.
On Wed, Aug 26, 2015 at 03:44:52PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
> > On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse at redhat.com wrote:
> > > > From: Jérôme Glisse
> > > >
> > > > Some device like GPU do things differently if swiotlb is in use. We
> > > > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> > > > but this is unreliable. Patch add a simple helpers to check if any of
> > >
> > > Why is it unreliable?
> >
> > Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb
> > is disabled. This seems to be due to
> > ac2cbab21f318e19bc176a7f38a120cec835220f
> > which cleanup swiotlb init and always allocate default size. Which i believe
> > is a waste of memory. So we need to add a real helper to know if swiotlb is
> > in use or not and we should not rely on expectation of some swiotlb value.
>
> Ah right, that patch. That should have been part of the description
> I believe.
>
> >
> > >
> > > > the dma_ops associated with a device points to the swiotlb functions,
> > > > making swiotlb check reliable for a device.
> > > >
> > > > Signed-off-by: Jérôme Glisse
> > > > Cc: Konrad Rzeszutek Wilk
> > > > Cc: Alex Deucher
> > > > Cc: Ben Skeggs
> > > > Cc: Dave Airlie
> > > > Cc: lkml at vger.kernel.org
> > > > Cc: Daniel Vetter
> > > > ---
> > > > include/linux/dma-mapping.h | 18 ++
> > > > 1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > index ac07ff0..eac911e 100644
> > > > --- a/include/linux/dma-mapping.h
> > > > +++ b/include/linux/dma-mapping.h
> > > > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct
> > > > device *dev,
> > > > #define dma_unmap_len_set(PTR, LEN_NAME, VAL)do { } while (0)
> > > > #endif
> > > >
> > > > +
> > > > +#ifdef CONFIG_SWIOTLB
> > > > +static inline bool swiotlb_in_use(struct device *dev)
> > > > +{
> > > > + struct dma_map_ops *ops = get_dma_ops(dev);
> > > > +
> > > > + return (ops->map_sg == swiotlb_map_sg_attrs ||
> > > > + ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > > > + ops->map_page == swiotlb_map_page);
> > >
> > > That won't work. What if we use xen-swiotlb which has different function
> > > names?
> >
> > I didn't thought about xen, always doing things differently, i think xen is
> > just a matter of also testing for the xen function. I just wanted to have
> > the helper in common code and only rely on common things, instead of having
> > to add a per arch helper.
>
> There has to be a better way. Perhaps you can expand SWIOTLB to actually
> check if it is in use?
This would require per arch modifications which is what i was trying to avoid.
Cheers,
Jérôme
[PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.
On Wed, Aug 26, 2015 at 04:31:50PM -0400, Jerome Glisse wrote:
> On Wed, Aug 26, 2015 at 03:44:52PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
> > > On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse at redhat.com wrote:
> > > > > From: Jérôme Glisse
> > > > >
> > > > > Some device like GPU do things differently if swiotlb is in use. We
> > > > > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> > > > > but this is unreliable. Patch add a simple helpers to check if any of
> > > >
> > > > Why is it unreliable?
> > >
> > > Alex reported on irc that swiotlb_nr_tbl() returns non zero even if
> > > swiotlb
> > > is disabled. This seems to be due to
> > > ac2cbab21f318e19bc176a7f38a120cec835220f
> > > which cleanup swiotlb init and always allocate default size. Which i
> > > believe
> > > is a waste of memory. So we need to add a real helper to know if swiotlb
> > > is
> > > in use or not and we should not rely on expectation of some swiotlb value.
> >
> > Ah right, that patch. That should have been part of the description
> > I believe.
> >
> > >
> > > >
> > > > > the dma_ops associated with a device points to the swiotlb functions,
> > > > > making swiotlb check reliable for a device.
> > > > >
> > > > > Signed-off-by: Jérôme Glisse
> > > > > Cc: Konrad Rzeszutek Wilk
> > > > > Cc: Alex Deucher
> > > > > Cc: Ben Skeggs
> > > > > Cc: Dave Airlie
> > > > > Cc: lkml at vger.kernel.org
> > > > > Cc: Daniel Vetter
> > > > > ---
> > > > > include/linux/dma-mapping.h | 18 ++
> > > > > 1 file changed, 18 insertions(+)
> > > > >
> > > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > > index ac07ff0..eac911e 100644
> > > > > --- a/include/linux/dma-mapping.h
> > > > > +++ b/include/linux/dma-mapping.h
> > > > > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct
> > > > > device *dev,
> > > > > #define dma_unmap_len_set(PTR, LEN_NAME, VAL)do { } while (0)
> > > > > #endif
> > > > >
> > > > > +
> > > > > +#ifdef CONFIG_SWIOTLB
> > > > > +static inline bool swiotlb_in_use(struct device *dev)
> > > > > +{
> > > > > + struct dma_map_ops *ops = get_dma_ops(dev);
> > > > > +
> > > > > + return (ops->map_sg == swiotlb_map_sg_attrs ||
> > > > > + ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > > > > + ops->map_page == swiotlb_map_page);
> > > >
> > > > That won't work. What if we use xen-swiotlb which has different function
> > > > names?
> > >
> > > I didn't thought about xen, always doing things differently, i think xen
> > > is
> > > just a matter of also testing for the xen function. I just wanted to have
> > > the helper in common code and only rely on common things, instead of
> > > having
> > > to add a per arch helper.
> >
> > There has to be a better way. Perhaps you can expand SWIOTLB to actually
> > check if it is in use?
>
> This would require per arch modifications which is what i was trying to avoid.
How? If you modify 'swiotlb_nr_tbl' to return true only if it has been used
then the modifications are only in the lib/swiotlb.c ?
>
> Cheers,
> Jérôme
[Bug 91676] Random TONGA lockups during normal usage
https://bugs.freedesktop.org/show_bug.cgi?id=91676 Mathias Tillman changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #1 from Mathias Tillman --- Haven't been able to reproduce this using latest mesa and drm from git, and drm-next-4.3-wip (commit 9066b0c318589f47b754a3def4fe8ec4688dc21a) kernel, so I'm going to mark this as resolved. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150826/067af358/attachment-0001.html>
[PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.
On Wed, Aug 26, 2015 at 04:38:14PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 26, 2015 at 04:31:50PM -0400, Jerome Glisse wrote:
> > On Wed, Aug 26, 2015 at 03:44:52PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
> > > > On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > > On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse at redhat.com wrote:
> > > > > > From: Jérôme Glisse
> > > > > >
> > > > > > Some device like GPU do things differently if swiotlb is in use. We
> > > > > > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or
> > > > > > not
> > > > > > but this is unreliable. Patch add a simple helpers to check if any
> > > > > > of
> > > > >
> > > > > Why is it unreliable?
> > > >
> > > > Alex reported on irc that swiotlb_nr_tbl() returns non zero even if
> > > > swiotlb
> > > > is disabled. This seems to be due to
> > > > ac2cbab21f318e19bc176a7f38a120cec835220f
> > > > which cleanup swiotlb init and always allocate default size. Which i
> > > > believe
> > > > is a waste of memory. So we need to add a real helper to know if
> > > > swiotlb is
> > > > in use or not and we should not rely on expectation of some swiotlb
> > > > value.
> > >
> > > Ah right, that patch. That should have been part of the description
> > > I believe.
> > >
> > > >
> > > > >
> > > > > > the dma_ops associated with a device points to the swiotlb
> > > > > > functions,
> > > > > > making swiotlb check reliable for a device.
> > > > > >
> > > > > > Signed-off-by: Jérôme Glisse
> > > > > > Cc: Konrad Rzeszutek Wilk
> > > > > > Cc: Alex Deucher
> > > > > > Cc: Ben Skeggs
> > > > > > Cc: Dave Airlie
> > > > > > Cc: lkml at vger.kernel.org
> > > > > > Cc: Daniel Vetter
> > > > > > ---
> > > > > > include/linux/dma-mapping.h | 18 ++
> > > > > > 1 file changed, 18 insertions(+)
> > > > > >
> > > > > > diff --git a/include/linux/dma-mapping.h
> > > > > > b/include/linux/dma-mapping.h
> > > > > > index ac07ff0..eac911e 100644
> > > > > > --- a/include/linux/dma-mapping.h
> > > > > > +++ b/include/linux/dma-mapping.h
> > > > > > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct
> > > > > > device *dev,
> > > > > > #define dma_unmap_len_set(PTR, LEN_NAME, VAL)do { } while (0)
> > > > > > #endif
> > > > > >
> > > > > > +
> > > > > > +#ifdef CONFIG_SWIOTLB
> > > > > > +static inline bool swiotlb_in_use(struct device *dev)
> > > > > > +{
> > > > > > + struct dma_map_ops *ops = get_dma_ops(dev);
> > > > > > +
> > > > > > + return (ops->map_sg == swiotlb_map_sg_attrs ||
> > > > > > + ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > > > > > + ops->map_page == swiotlb_map_page);
> > > > >
> > > > > That won't work. What if we use xen-swiotlb which has different
> > > > > function
> > > > > names?
> > > >
> > > > I didn't thought about xen, always doing things differently, i think
> > > > xen is
> > > > just a matter of also testing for the xen function. I just wanted to
> > > > have
> > > > the helper in common code and only rely on common things, instead of
> > > > having
> > > > to add a per arch helper.
> > >
> > > There has to be a better way. Perhaps you can expand SWIOTLB to actually
> > > check if it is in use?
> >
> > This would require per arch modifications which is what i was trying to
> > avoid.
>
> How? If you modify 'swiotlb_nr_tbl' to return true only if it has been used
> then the modifications are only in the lib/swiotlb.c ?
I am not sure i follow swiotlb_nr_tbl is an internal value that so far have only
be use by driver through calling swiotlb_nr_tbl() so calls to that functions
does
not reflect if swiotlb is enabled or not.
In fact i am pretty sure only arch specific code stores information on wether or
not swiotlb is enabled. Beside i think some device overide dev->archdata.dma_ops
which effectively disable swiotlb for the device.
But if you really dislike just testing dma_ops against swiotlb & xen_swiotlb i
will respin with arch mod but i will only handle ppc/arm/x86.
Cheers,
Jérôme
[Intel-gfx] [PATCH] drm/atomic: refuse changing CRTC for planes directly
On Wed, Aug 26, 2015 at 3:49 PM, Daniel Vetter
wrote:
> Very strictly speaking this is possible if you have special hw and
> genlocked CRTCs. In general switching a plane between two active CRTC
> just won't work so well and is probably not tested at all. Just forbid
> it.
>
> I've put this into the core since right now no helper or driver copes
> with it, no userspace has code for it and no one asks for it. Yes
> there's piles of corner-cases where this would be possible to do this
> like:
> - switch from inactive crtc to active crtc
> - swithc from active crtc to inactive crtc
> - genlocked display
> - invisible plane (to do whatever)
> - idle plane hw due to dsi cmd mode/psr
> - whatever
> but looking at details it's not that easy to implement this correctly.
> Hence just put it into the core and add a comment, since the only
> userspace we have right now for atomic (weston) doesn't want to use
> direct plane switching either.
I suspect that eventually we'll want to make this a helper exposed to
drivers and push this check down into the drivers.. perhaps that is
not until weston (and/or atomic based hwc) grows driver specific
userspace plugin type API to make smarter hw specific decisions..
until then, this is probably the most reasonable thing to do.. so
(w/ s/swihc/switch/ fix smashed in above)
Reviewed-by: Rob Clark
> v2: don't bother with complexity and just outright disallow plane
> switching without the intermediate OFF state. Simplifies drivers, we
> don't have any hw that could do it anyway and current atomic userspace
> (weston) works like this already anyway.
>
> v3: Bikeshed function name (Ville) and add comment (Rob).
>
> v4: Also bikeshed commit message (Rob).
>
> Cc: Thierry Reding
> Cc: Maarten Lankhorst
> Cc: Daniel Stone
> Acked-by: Daniel Stone
> Signed-off-by: Daniel Vetter
>
> ---
>
> Imo this is bikeshedded enough, so either someone takes this or
> someone else can mangle this patch more.
> -Daniel
> ---
> drivers/gpu/drm/drm_atomic.c | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 1066e4b658cf..40ddd6aa100f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -663,6 +663,27 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> return 0;
> }
>
> +static bool
> +plane_switching_crtc(struct drm_atomic_state *state,
> +struct drm_plane *plane,
> +struct drm_plane_state *plane_state)
> +{
> + struct drm_crtc_state *crtc_state, *curr_crtc_state;
> +
> + if (!plane->state->crtc || !plane_state->crtc)
> + return false;
> +
> + if (plane->state->crtc == plane_state->crtc)
> + return false;
> +
> + /* This could be refined, but currently there's no helper or driver
> code
> +* to implement direct switching of active planes nor userspace to
> take
> +* advantage of more direct plane switching without the intermediate
> +* full OFF state.
> +*/
> + return true;
> +}
> +
> /**
> * drm_atomic_plane_check - check plane state
> * @plane: plane to check
> @@ -734,6 +755,12 @@ static int drm_atomic_plane_check(struct drm_plane
> *plane,
> return -ENOSPC;
> }
>
> + if (plane_switching_crtc(state->state, plane, state)) {
> + DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
> +plane->base.id);
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> --
> 2.5.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Bug 86351] HDMI audio garbled output on Radeon R9 280X
https://bugzilla.kernel.org/show_bug.cgi?id=86351 --- Comment #17 from Alex Deucher --- Maybe something with the audio driver? This doesn't seem to be a GPU driver issue. -- You are receiving this mail because: You are watching the assignee of the bug.
