From caea4f384858ee7861367920df36995e7acfe160 Mon Sep 17 00:00:00 2001 From: Hans Verkuil Date: Fri, 15 Dec 2017 16:21:50 +0100 Subject: [PATCH 1/5] drm/sun4i: validate modes for HDMI When I connected my cubieboard running 4.15-rc1 to my 4k display I got no picture. Some digging found that there is no check against the upper pixelclock limit of the HDMI output, so X selects a 4kp60 format at 594 MHz, which obviously won't work. The patch below adds a check for the upper bound of what this hardware can do, and it checks if the requested tmds clock can be obtained. It also allows for the +/- 0.5% pixel clock variation that the HDMI spec permits. That code is based on commit 22d0be2a557e ("drm: arcpgu: Allow some clock deviation in crtc->mode_valid() callback") from Jose Abreu for drm/arc. Signed-off-by: Hans Verkuil Thanks-to: Jose Abreu Signed-off-by: Maxime Ripard Link: https://patchwork.freedesktop.org/patch/msgid/162854cb-c7bd-d9ce-9fa0-9a6cd89c621b@xs4all.nl --- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c index dda904ec0534..c12f9bd12904 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c @@ -208,8 +208,27 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector) return ret; } +static int sun4i_hdmi_mode_valid(struct drm_connector *connector, + struct drm_display_mode *mode) +{ + struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector); + long rate = mode->clock * 1000; + long diff = rate / 200; /* +-0.5% allowed by HDMI spec */ + long rounded_rate; + + /* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */ + if (rate > 165000000) + return MODE_CLOCK_HIGH; + rounded_rate = clk_round_rate(hdmi->tmds_clk, rate); + if (max(rounded_rate, rate) - min(rounded_rate, rate) < diff && + rounded_rate > 0) + return MODE_OK; + return MODE_NOCLOCK; +} + static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = { .get_modes = sun4i_hdmi_get_modes, + .mode_valid = sun4i_hdmi_mode_valid, }; static enum drm_connector_status From 92411f6d7f1afcc95e54295d40e96a75385212ec Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Thu, 7 Dec 2017 16:58:50 +0100 Subject: [PATCH 2/5] drm/sun4i: Fix error path handling The commit 4c7f16d14a33 ("drm/sun4i: Fix TCON clock and regmap initialization sequence") moved a bunch of logic around, but forgot to update the gotos after the introduction of the err_free_dotclock label. It means that if we fail later that the one introduced in that commit, we'll just to the old label which isn't free the clock we created. This will result in a breakage as soon as someone tries to do something with that clock, since its resources will have been long reclaimed. Cc: Fixes: 4c7f16d14a33 ("drm/sun4i: Fix TCON clock and regmap initialization sequence") Reviewed-by: Chen-Yu Tsai Signed-off-by: Maxime Ripard Link: https://patchwork.freedesktop.org/patch/msgid/f83c1cebc731f0b4251f5ddd7b38c718cd79bb0b.1512662253.git-series.maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index e122f5b2a395..f4284b51bdca 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -724,12 +724,12 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, if (IS_ERR(tcon->crtc)) { dev_err(dev, "Couldn't create our CRTC\n"); ret = PTR_ERR(tcon->crtc); - goto err_free_clocks; + goto err_free_dotclock; } ret = sun4i_rgb_init(drm, tcon); if (ret < 0) - goto err_free_clocks; + goto err_free_dotclock; if (tcon->quirks->needs_de_be_mux) { /* From 2c08cd7c20968ddf71feeac2265b4741d2b3fdde Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Wed, 20 Dec 2017 11:52:47 +0100 Subject: [PATCH 3/5] drm/sun4i: hdmi: Move the mode_valid callback to the encoder When attached to the connector, the mode_valid callback will only filter the modes provided by the connector itself as part of its probe. However, it will not be doing it when the mode is provided by the userspace, which still might result in a broken configuration. In order to enforce these constraints, move our mode_valid callback to the encoder which doesn't have this behaviour. Acked-by: Daniel Vetter Signed-off-by: Hans Verkuil [maxime: Wrote the commit log in order to update the patch from the merged v3 to the v4 that was correct.] Signed-off-by: Maxime Ripard Link: https://patchwork.freedesktop.org/patch/msgid/0fa230a8-d01d-561a-f74f-6b4fd421255b@xs4all.nl --- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 39 +++++++++++++------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c index c12f9bd12904..500b6fb3e028 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c @@ -175,11 +175,31 @@ static void sun4i_hdmi_mode_set(struct drm_encoder *encoder, writel(val, hdmi->base + SUN4I_HDMI_VID_TIMING_POL_REG); } +static enum drm_mode_status sun4i_hdmi_mode_valid(struct drm_encoder *encoder, + const struct drm_display_mode *mode) +{ + struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder); + unsigned long rate = mode->clock * 1000; + unsigned long diff = rate / 200; /* +-0.5% allowed by HDMI spec */ + long rounded_rate; + + /* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */ + if (rate > 165000000) + return MODE_CLOCK_HIGH; + rounded_rate = clk_round_rate(hdmi->tmds_clk, rate); + if (rounded_rate > 0 && + max_t(unsigned long, rounded_rate, rate) - + min_t(unsigned long, rounded_rate, rate) < diff) + return MODE_OK; + return MODE_NOCLOCK; +} + static const struct drm_encoder_helper_funcs sun4i_hdmi_helper_funcs = { .atomic_check = sun4i_hdmi_atomic_check, .disable = sun4i_hdmi_disable, .enable = sun4i_hdmi_enable, .mode_set = sun4i_hdmi_mode_set, + .mode_valid = sun4i_hdmi_mode_valid, }; static const struct drm_encoder_funcs sun4i_hdmi_funcs = { @@ -208,27 +228,8 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector) return ret; } -static int sun4i_hdmi_mode_valid(struct drm_connector *connector, - struct drm_display_mode *mode) -{ - struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector); - long rate = mode->clock * 1000; - long diff = rate / 200; /* +-0.5% allowed by HDMI spec */ - long rounded_rate; - - /* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */ - if (rate > 165000000) - return MODE_CLOCK_HIGH; - rounded_rate = clk_round_rate(hdmi->tmds_clk, rate); - if (max(rounded_rate, rate) - min(rounded_rate, rate) < diff && - rounded_rate > 0) - return MODE_OK; - return MODE_NOCLOCK; -} - static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = { .get_modes = sun4i_hdmi_get_modes, - .mode_valid = sun4i_hdmi_mode_valid, }; static enum drm_connector_status From ce0769e0ea4b3e192466243a1a9fd39acf214f1e Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Wed, 20 Dec 2017 10:35:43 +0100 Subject: [PATCH 4/5] drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers lock_all_ctx in setplane_internal may return -EINTR, and __setplane_internal could return -EDEADLK. Making more special cases for fb would make the code even harder to read, so the easiest solution is not taking over the fb refcount, and making callers responsible for dropping the ref. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707 Fixes: 13736ba3b38b ("drm/legacy: Convert setplane ioctl locking to interruptible.") Testcase: kms_atomic_interruptible Signed-off-by: Maarten Lankhorst Link: https://patchwork.freedesktop.org/patch/msgid/20171220093545.613-2-maarten.lankhorst@linux.intel.com Reviewed-by: Daniel Vetter --- drivers/gpu/drm/drm_plane.c | 42 ++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 37a93cdffb4a..2c90519576a3 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -558,11 +558,10 @@ int drm_plane_check_pixel_format(const struct drm_plane *plane, u32 format) } /* - * setplane_internal - setplane handler for internal callers + * __setplane_internal - setplane handler for internal callers * - * Note that we assume an extra reference has already been taken on fb. If the - * update fails, this reference will be dropped before return; if it succeeds, - * the previous framebuffer (if any) will be unreferenced instead. + * This function will take a reference on the new fb for the plane + * on success. * * src_{x,y,w,h} are provided in 16.16 fixed point format */ @@ -630,14 +629,12 @@ static int __setplane_internal(struct drm_plane *plane, if (!ret) { plane->crtc = crtc; plane->fb = fb; - fb = NULL; + drm_framebuffer_get(plane->fb); } else { plane->old_fb = NULL; } out: - if (fb) - drm_framebuffer_put(fb); if (plane->old_fb) drm_framebuffer_put(plane->old_fb); plane->old_fb = NULL; @@ -685,6 +682,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data, struct drm_plane *plane; struct drm_crtc *crtc = NULL; struct drm_framebuffer *fb = NULL; + int ret; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; @@ -717,15 +715,16 @@ int drm_mode_setplane(struct drm_device *dev, void *data, } } - /* - * setplane_internal will take care of deref'ing either the old or new - * framebuffer depending on success. - */ - return setplane_internal(plane, crtc, fb, - plane_req->crtc_x, plane_req->crtc_y, - plane_req->crtc_w, plane_req->crtc_h, - plane_req->src_x, plane_req->src_y, - plane_req->src_w, plane_req->src_h); + ret = setplane_internal(plane, crtc, fb, + plane_req->crtc_x, plane_req->crtc_y, + plane_req->crtc_w, plane_req->crtc_h, + plane_req->src_x, plane_req->src_y, + plane_req->src_w, plane_req->src_h); + + if (fb) + drm_framebuffer_put(fb); + + return ret; } static int drm_mode_cursor_universal(struct drm_crtc *crtc, @@ -788,13 +787,12 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, src_h = fb->height << 16; } - /* - * setplane_internal will take care of deref'ing either the old or new - * framebuffer depending on success. - */ ret = __setplane_internal(crtc->cursor, crtc, fb, - crtc_x, crtc_y, crtc_w, crtc_h, - 0, 0, src_w, src_h, ctx); + crtc_x, crtc_y, crtc_w, crtc_h, + 0, 0, src_w, src_h, ctx); + + if (fb) + drm_framebuffer_put(fb); /* Update successful; save new cursor position, if necessary */ if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) { From d2a48e52541cdf474ef35d51e8d73ded5be33122 Mon Sep 17 00:00:00 2001 From: Keith Packard Date: Wed, 20 Dec 2017 22:54:24 -0800 Subject: [PATCH 5/5] drm: move lease init after validation in drm_lease_create Patch bd36d3bab2e3d08f80766c86487090dbceed4651 fixed a deadlock in the failure path of drm_lease_create. This made the partially initialized lease object visible for a short window of time. To avoid having the lessee state appear transiently, I've rearranged the code so that the lessor fields are not filled in until the parameters are all validated and the function will succeed. Signed-off-by: Keith Packard Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20171221065424.1304-1-keithp@keithp.com --- drivers/gpu/drm/drm_lease.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index 59849f02e2ad..1402c0e71b03 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -220,17 +220,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr mutex_lock(&dev->mode_config.idr_mutex); - /* Insert the new lessee into the tree */ - id = idr_alloc(&(drm_lease_owner(lessor)->lessee_idr), lessee, 1, 0, GFP_KERNEL); - if (id < 0) { - error = id; - goto out_lessee; - } - - lessee->lessee_id = id; - lessee->lessor = drm_master_get(lessor); - list_add_tail(&lessee->lessee_list, &lessor->lessees); - idr_for_each_entry(leases, entry, object) { error = 0; if (!idr_find(&dev->mode_config.crtc_idr, object)) @@ -246,6 +235,17 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr } } + /* Insert the new lessee into the tree */ + id = idr_alloc(&(drm_lease_owner(lessor)->lessee_idr), lessee, 1, 0, GFP_KERNEL); + if (id < 0) { + error = id; + goto out_lessee; + } + + lessee->lessee_id = id; + lessee->lessor = drm_master_get(lessor); + list_add_tail(&lessee->lessee_list, &lessor->lessees); + /* Move the leases over */ lessee->leases = *leases; DRM_DEBUG_LEASE("new lessee %d %p, lessor %d %p\n", lessee->lessee_id, lessee, lessor->lessee_id, lessor);