From d9ba1b4cbb2989c919198832f4ebd6eb8ba94da0 Mon Sep 17 00:00:00 2001 From: Jani Nikula Date: Mon, 9 May 2022 15:03:23 +0300 Subject: drm/displayid: convert to drm_edid We'll need to propagate drm_edid everywhere. v2: Rebase Signed-off-by: Jani Nikula Reviewed-by: Ankit Nautiyal Link: https://patchwork.freedesktop.org/patch/msgid/a52a6882e87a4bb6b1670918f3aba13f9b52f6de.1652097712.git.jani.nikula@intel.com --- include/drm/drm_displayid.h | 6 +++--- include/drm/drm_edid.h | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) (limited to 'include/drm') diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h index 7ffbd9f7bfc7..49649eb8447e 100644 --- a/include/drm/drm_displayid.h +++ b/include/drm/drm_displayid.h @@ -25,7 +25,7 @@ #include #include -struct edid; +struct drm_edid; #define VESA_IEEE_OUI 0x3a0292 @@ -141,7 +141,7 @@ struct displayid_vesa_vendor_specific_block { /* DisplayID iteration */ struct displayid_iter { - const struct edid *edid; + const struct drm_edid *drm_edid; const u8 *section; int length; @@ -149,7 +149,7 @@ struct displayid_iter { int ext_index; }; -void displayid_iter_edid_begin(const struct edid *edid, +void displayid_iter_edid_begin(const struct drm_edid *drm_edid, struct displayid_iter *iter); const struct displayid_block * __displayid_iter_next(struct displayid_iter *iter); diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index c3204a58fb09..c61e75ab8f63 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -28,6 +28,7 @@ #include struct drm_device; +struct drm_edid; struct i2c_adapter; #define EDID_LENGTH 128 @@ -578,8 +579,9 @@ struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, struct drm_display_mode * drm_display_mode_from_cea_vic(struct drm_device *dev, u8 video_code); -const u8 *drm_find_edid_extension(const struct edid *edid, - int ext_id, int *ext_index); +/* Interface based on struct drm_edid */ +const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid, + int ext_id, int *ext_index); #endif /* __DRM_EDID_H__ */ -- cgit v1.2.3 From 7d64c40a7d96190d9d06e240305389e025295916 Mon Sep 17 00:00:00 2001 From: Dmitry Osipenko Date: Tue, 12 Apr 2022 01:15:36 +0300 Subject: drm/scheduler: Don't kill jobs in interrupt context Interrupt context can't sleep. Drivers like Panfrost and MSM are taking mutex when job is released, and thus, that code can sleep. This results into "BUG: scheduling while atomic" if locks are contented while job is freed. There is no good reason for releasing scheduler's jobs in IRQ context, hence use normal context to fix the trouble. Cc: stable@vger.kernel.org Fixes: 542cff7893a3 ("drm/sched: Avoid lockdep spalt on killing a processes") Signed-off-by: Dmitry Osipenko Signed-off-by: Andrey Grodzovsky Link: https://patchwork.freedesktop.org/patch/msgid/20220411221536.283312-1-dmitry.osipenko@collabora.com --- drivers/gpu/drm/scheduler/sched_entity.c | 6 +++--- include/drm/gpu_scheduler.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) (limited to 'include/drm') diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 191c56064f19..6b25b2f4f5a3 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -190,7 +190,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout) } EXPORT_SYMBOL(drm_sched_entity_flush); -static void drm_sched_entity_kill_jobs_irq_work(struct irq_work *wrk) +static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk) { struct drm_sched_job *job = container_of(wrk, typeof(*job), work); @@ -207,8 +207,8 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f, struct drm_sched_job *job = container_of(cb, struct drm_sched_job, finish_cb); - init_irq_work(&job->work, drm_sched_entity_kill_jobs_irq_work); - irq_work_queue(&job->work); + INIT_WORK(&job->work, drm_sched_entity_kill_jobs_work); + schedule_work(&job->work); } static struct dma_fence * diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 0fca8f38bee4..addb135eeea6 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -28,7 +28,7 @@ #include #include #include -#include +#include #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000) @@ -295,7 +295,7 @@ struct drm_sched_job { */ union { struct dma_fence_cb finish_cb; - struct irq_work work; + struct work_struct work; }; uint64_t id; -- cgit v1.2.3 From 16f1456466c269ecda32b88c3f8fdd76f8ec370c Mon Sep 17 00:00:00 2001 From: Thomas Zimmermann Date: Mon, 16 May 2022 15:43:39 +0200 Subject: drm/mgag200: Implement connector's get_modes with helper Provide drm_connector_helper_get_modes_from_ddc() to implement the connector's get_modes callback. The new helper updates the connector from DDC-provided EDID data. v2: * clear property if EDID is NULL in helper Signed-off-by: Thomas Zimmermann Reviewed-by: Jocelyn Falempe Tested-by: Jocelyn Falempe Link: https://patchwork.freedesktop.org/patch/msgid/20220516134343.6085-4-tzimmermann@suse.de --- drivers/gpu/drm/drm_probe_helper.c | 36 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/mgag200/mgag200_mode.c | 17 ++++------------ include/drm/drm_probe_helper.h | 2 ++ 3 files changed, 42 insertions(+), 13 deletions(-) (limited to 'include/drm') diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 682359512996..d77f17867195 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -964,3 +964,39 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) return changed; } EXPORT_SYMBOL(drm_helper_hpd_irq_event); + +/** + * drm_connector_helper_get_modes_from_ddc - Updates the connector's EDID + * property from the connector's + * DDC channel + * @connector: The connector + * + * Returns: + * The number of detected display modes. + * + * Uses a connector's DDC channel to retrieve EDID data and update the + * connector's EDID property and display modes. Drivers can use this + * function to implement struct &drm_connector_helper_funcs.get_modes + * for connectors with a DDC channel. + */ +int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector) +{ + struct edid *edid; + int count = 0; + + if (!connector->ddc) + return 0; + + edid = drm_get_edid(connector, connector->ddc); + + // clears property if EDID is NULL + drm_connector_update_edid_property(connector, edid); + + if (edid) { + count = drm_add_edid_modes(connector, edid); + kfree(edid); + } + + return count; +} +EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc); diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index b227891d01ec..4c0680dd1a78 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -689,26 +689,17 @@ static void mgag200_disable_display(struct mga_device *mdev) * Connector */ -static int mga_vga_get_modes(struct drm_connector *connector) +static int mgag200_vga_connector_helper_get_modes(struct drm_connector *connector) { struct mga_device *mdev = to_mga_device(connector->dev); - struct mga_connector *mga_connector = to_mga_connector(connector); - struct edid *edid; - int ret = 0; + int ret; /* * Protect access to I/O registers from concurrent modesetting * by acquiring the I/O-register lock. */ mutex_lock(&mdev->rmmio_lock); - - edid = drm_get_edid(connector, &mga_connector->i2c->adapter); - if (edid) { - drm_connector_update_edid_property(connector, edid); - ret = drm_add_edid_modes(connector, edid); - kfree(edid); - } - + ret = drm_connector_helper_get_modes_from_ddc(connector); mutex_unlock(&mdev->rmmio_lock); return ret; @@ -828,7 +819,7 @@ static void mga_connector_destroy(struct drm_connector *connector) } static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = { - .get_modes = mga_vga_get_modes, + .get_modes = mgag200_vga_connector_helper_get_modes, .mode_valid = mga_vga_mode_valid, }; diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h index 48300aa6ca71..c80cab7a53b7 100644 --- a/include/drm/drm_probe_helper.h +++ b/include/drm/drm_probe_helper.h @@ -26,4 +26,6 @@ void drm_kms_helper_poll_disable(struct drm_device *dev); void drm_kms_helper_poll_enable(struct drm_device *dev); bool drm_kms_helper_is_poll_worker(void); +int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector); + #endif -- cgit v1.2.3 From 746b9c62cc8614fa59c23f3332682b5e9e1d801c Mon Sep 17 00:00:00 2001 From: Thomas Zimmermann Date: Tue, 17 May 2022 13:33:24 +0200 Subject: drm/gem: Ignore color planes that are unused by framebuffer format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only handle color planes that exist in a framebuffer's color format. Ignore non-existing planes. So far, several helpers assumed that all 4 planes are available and silently ignored non-existing planes. This lead to subtil bugs with uninitialized data in instances of struct iosys_map. [1] Signed-off-by: Thomas Zimmermann Reviewed-by: Javier Martinez Canillas Tested-by: Noralf Trønnes Acked-by: Christian König Link: https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de/T/#md0172b10bb588d8f20f4f456e304f08d2a4505f7 # 1 Link: https://patchwork.freedesktop.org/patch/msgid/20220517113327.26919-3-tzimmermann@suse.de --- drivers/gpu/drm/drm_gem_atomic_helper.c | 6 +++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 37 +++++++++++++++------------- include/drm/drm_gem_framebuffer_helper.h | 10 +++----- 3 files changed, 27 insertions(+), 26 deletions(-) (limited to 'include/drm') diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index a5026f617739..f16d60217c6c 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -169,8 +169,10 @@ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_gem_object *obj = drm_gem_fb_get_obj(state->fb, i); struct dma_fence *new; - if (WARN_ON_ONCE(!obj)) - continue; + if (!obj) { + ret = -EINVAL; + goto error; + } ret = dma_resv_get_singleton(obj->resv, usage, &new); if (ret) diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 931828784dfe..6d3c26efdeeb 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -92,9 +92,9 @@ drm_gem_fb_init(struct drm_device *dev, */ void drm_gem_fb_destroy(struct drm_framebuffer *fb) { - size_t i; + unsigned int i; - for (i = 0; i < ARRAY_SIZE(fb->obj); i++) + for (i = 0; i < fb->format->num_planes; i++) drm_gem_object_put(fb->obj[i]); drm_framebuffer_cleanup(fb); @@ -329,24 +329,26 @@ EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty); * The argument returns the addresses of the data stored in each BO. This * is different from @map if the framebuffer's offsets field is non-zero. * + * Both, @map and @data, must each refer to arrays with at least + * fb->format->num_planes elements. + * * See drm_gem_fb_vunmap() for unmapping. * * Returns: * 0 on success, or a negative errno code otherwise. */ -int drm_gem_fb_vmap(struct drm_framebuffer *fb, - struct iosys_map map[static DRM_FORMAT_MAX_PLANES], - struct iosys_map data[DRM_FORMAT_MAX_PLANES]) +int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map, + struct iosys_map *data) { struct drm_gem_object *obj; unsigned int i; int ret; - for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) { + for (i = 0; i < fb->format->num_planes; ++i) { obj = drm_gem_fb_get_obj(fb, i); if (!obj) { - iosys_map_clear(&map[i]); - continue; + ret = -EINVAL; + goto err_drm_gem_vunmap; } ret = drm_gem_vmap(obj, &map[i]); if (ret) @@ -354,7 +356,7 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb, } if (data) { - for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) { + for (i = 0; i < fb->format->num_planes; ++i) { memcpy(&data[i], &map[i], sizeof(data[i])); if (iosys_map_is_null(&data[i])) continue; @@ -385,10 +387,9 @@ EXPORT_SYMBOL(drm_gem_fb_vmap); * * See drm_gem_fb_vmap() for more information. */ -void drm_gem_fb_vunmap(struct drm_framebuffer *fb, - struct iosys_map map[static DRM_FORMAT_MAX_PLANES]) +void drm_gem_fb_vunmap(struct drm_framebuffer *fb, struct iosys_map *map) { - unsigned int i = DRM_FORMAT_MAX_PLANES; + unsigned int i = fb->format->num_planes; struct drm_gem_object *obj; while (i) { @@ -443,13 +444,15 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct { struct dma_buf_attachment *import_attach; struct drm_gem_object *obj; - size_t i; + unsigned int i; int ret; - for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) { + for (i = 0; i < fb->format->num_planes; ++i) { obj = drm_gem_fb_get_obj(fb, i); - if (!obj) - continue; + if (!obj) { + ret = -EINVAL; + goto err___drm_gem_fb_end_cpu_access; + } import_attach = obj->import_attach; if (!import_attach) continue; @@ -479,7 +482,7 @@ EXPORT_SYMBOL(drm_gem_fb_begin_cpu_access); */ void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir) { - __drm_gem_fb_end_cpu_access(fb, dir, ARRAY_SIZE(fb->obj)); + __drm_gem_fb_end_cpu_access(fb, dir, fb->format->num_planes); } EXPORT_SYMBOL(drm_gem_fb_end_cpu_access); diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h index 1091e4fa08cb..d302521f3dd4 100644 --- a/include/drm/drm_gem_framebuffer_helper.h +++ b/include/drm/drm_gem_framebuffer_helper.h @@ -4,8 +4,6 @@ #include #include -#include - struct drm_afbc_framebuffer; struct drm_device; struct drm_fb_helper_surface_size; @@ -39,11 +37,9 @@ struct drm_framebuffer * drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd); -int drm_gem_fb_vmap(struct drm_framebuffer *fb, - struct iosys_map map[static DRM_FORMAT_MAX_PLANES], - struct iosys_map data[DRM_FORMAT_MAX_PLANES]); -void drm_gem_fb_vunmap(struct drm_framebuffer *fb, - struct iosys_map map[static DRM_FORMAT_MAX_PLANES]); +int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map, + struct iosys_map *data); +void drm_gem_fb_vunmap(struct drm_framebuffer *fb, struct iosys_map *map); int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir); void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir); -- cgit v1.2.3 From 69ef4a192bba0d76216198ec6d5fe82375337903 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Mon, 9 May 2022 16:18:09 -0700 Subject: drm: Document the power requirements for DP AUX transfers When doing DP AUX transfers there are two actors that need to be powered in order for the DP AUX transfer to work: the DP source and the DP sink. Commit bacbab58f09d ("drm: Mention the power state requirement on side-channel operations") added some documentation saying that the DP source is required to power itself up (if needed) to do AUX transfers. However, that commit doesn't talk anything about the DP sink. For full fledged DP the sink isn't really a problem. It's expected that if an external DP monitor isn't plugged in that attempting to do AUX transfers won't work. It's also expected that if a DP monitor is plugged in (and thus asserting HPD) then AUX transfers will work. When we're looking at eDP, however, things are less obvious. Let's add some documentation about expectations. Here's what we'll say: 1. We don't expect the DP AUX transfer function to power on an eDP panel. If an eDP panel is physically connected but powered off then it makes sense for the transfer to fail. 2. We'll document that the official way to power on a panel is via the bridge chain, specifically by making sure that the panel's prepare function has been called (which is called by panel_bridge_pre_enable()). It's already specified in the kernel doc of drm_panel_prepare() that this is the way to power the panel on and also that after this call "it is possible to communicate with any integrated circuitry via a command bus." 3. We'll also document that for code running in the panel driver itself that it is legal for the panel driver to power itself up however it wants (it doesn't need to officially call drm_panel_pre_enable()) and then it can do AUX bus transfers. This is currently the way that edp-panel works when it's running atop the DP AUX bus. NOTE: there was much discussion of all of this in response to v1 [1] of this patch. A summary of that is: * With the Intel i195 driver, apparently eDP panels do get powered up. We won't forbid this but it is expected that code that wants to run on a variety of platforms should ensure that the drm_panel's prepare() function has been called. * There is at least a reasonable amount of agreement that the transfer() functions itself shouldn't be responsible for powering the panel. It's proposed that if we need the DP AUX dev nodes to be robust for eDP that the code handling the DP AUX dev nodes could handle powering the panel by ensuring that the panel's prepare() call was made. Potentially drm_dp_aux_dev_get_by_minor() could be a good place to do this. This is left as a future exercise. Until that's fixed the DP AUX dev nodes for eDP are probably best just used for debugging. * If a panel could be in PSR and DP AUX via the dev node needs to be reliable then we need to be able to pull the panel out of PSR. On i915 this is also apparently handled as part of the transfer() function. [1] https://lore.kernel.org/r/20220503162033.1.Ia8651894026707e4fa61267da944ff739610d180@changeid Signed-off-by: Douglas Anderson Reviewed-by: Dmitry Baryshkov Reviewed-by: Lyude Paul Link: https://patchwork.freedesktop.org/patch/msgid/20220509161733.v2.1.Ia8651894026707e4fa61267da944ff739610d180@changeid --- include/drm/display/drm_dp_helper.h | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) (limited to 'include/drm') diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h index dca40a045dd6..dc3c02225fcf 100644 --- a/include/drm/display/drm_dp_helper.h +++ b/include/drm/display/drm_dp_helper.h @@ -370,9 +370,19 @@ struct drm_dp_aux { * helpers assume this is the case. * * Also note that this callback can be called no matter the - * state @dev is in. Drivers that need that device to be powered - * to perform this operation will first need to make sure it's - * been properly enabled. + * state @dev is in and also no matter what state the panel is + * in. It's expected: + * - If the @dev providing the AUX bus is currently unpowered then + * it will power itself up for the transfer. + * - If we're on eDP (using a drm_panel) and the panel is not in a + * state where it can respond (it's not powered or it's in a + * low power state) then this function may return an error, but + * not crash. It's up to the caller of this code to make sure that + * the panel is powered on if getting an error back is not OK. If a + * drm_panel driver is initiating a DP AUX transfer it may power + * itself up however it wants. All other code should ensure that + * the pre_enable() bridge chain (which eventually calls the + * drm_panel prepare function) has powered the panel. */ ssize_t (*transfer)(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); -- cgit v1.2.3 From ade1fc91eb99614c7155fec762ad5761bb470e06 Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Tue, 10 May 2022 13:42:41 +0300 Subject: drm/edid: Extract drm_edid_decode_mfg_id() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the PNPID decoding available for other users. Cc: dri-devel@lists.freedesktop.org Reviewed-by: Jani Nikula Signed-off-by: Ville Syrjälä Link: https://patchwork.freedesktop.org/patch/msgid/20220510104242.6099-15-ville.syrjala@linux.intel.com --- include/drm/drm_edid.h | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) (limited to 'include/drm') diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index c61e75ab8f63..1d5950b5b407 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -497,6 +497,22 @@ static inline u8 drm_eld_get_conn_type(const uint8_t *eld) return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK; } +/** + * drm_edid_decode_mfg_id - Decode the manufacturer ID + * @mfg_id: The manufacturer ID + * @vend: A 4-byte buffer to store the 3-letter vendor string plus a '\0' + * termination + */ +static inline const char *drm_edid_decode_mfg_id(u16 mfg_id, char vend[4]) +{ + vend[0] = '@' + ((mfg_id >> 10) & 0x1f); + vend[1] = '@' + ((mfg_id >> 5) & 0x1f); + vend[2] = '@' + ((mfg_id >> 0) & 0x1f); + vend[3] = '\0'; + + return vend; +} + /** * drm_edid_encode_panel_id - Encode an ID for matching against drm_edid_get_panel_id() * @vend_chr_0: First character of the vendor string. @@ -537,10 +553,7 @@ static inline u8 drm_eld_get_conn_type(const uint8_t *eld) static inline void drm_edid_decode_panel_id(u32 panel_id, char vend[4], u16 *product_id) { *product_id = (u16)(panel_id & 0xffff); - vend[0] = '@' + ((panel_id >> 26) & 0x1f); - vend[1] = '@' + ((panel_id >> 21) & 0x1f); - vend[2] = '@' + ((panel_id >> 16) & 0x1f); - vend[3] = '\0'; + drm_edid_decode_mfg_id(panel_id >> 16, vend); } bool drm_probe_ddc(struct i2c_adapter *adapter); -- cgit v1.2.3 From 3800b1710946f7db3cb3a29cb2e218cf5df999d0 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Tue, 10 May 2022 12:29:42 -0700 Subject: drm/dp: Add callbacks to make using DP AUX bus properly easier As talked about in this patch in the kerneldoc of of_dp_aux_populate_ep_device() and also in the past in commit a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), it can be difficult for eDP controller drivers to know when the panel has finished probing when they're using of_dp_aux_populate_ep_devices(). The ti-sn65dsi86 driver managed to solve this because it was already broken up into a bunch of sub-drivers. That means we could solve the problem there by adding a new sub-driver to get the panel. We could use the traditional -EPROBE_DEFER retry mechansim to handle the case where the panel hadn't probed yet. In parade-ps8640 we didn't really solve this. The code just expects the panel to be ready right away. While reviewing the code originally I had managed to convince myself it was fine to just expect the panel right away, but additional testing has shown that not to be the case. We could fix parade-ps8640 like we did ti-sn65dsi86 but it's pretty cumbersome (since we're not already broken into multiple drivers) and requires a bunch of boilerplate code. After discussion [1] it seems like the best solution for most people is: - Accept that there's always at most one device that will probe as a result of the DP AUX bus (it may have sub-devices, but there will be one device _directly_ probed). - When that device finishes probing, we can just have a call back. This patch implements that idea. We'll now take a callback as an argument to the populate function. To make this easier to land in pieces, we'll make wrappers for the old functions. The functions with the new name (which make it clear that we only have one child) will take the callback and the functions with the old name will temporarily wrap. [1] https://lore.kernel.org/r/CAD=FV=Ur3afHhsXe7a3baWEnD=MFKFeKRbhFU+bt3P67G0MVzQ@mail.gmail.com Signed-off-by: Douglas Anderson Reviewed-by: Dmitry Baryshkov Link: https://patchwork.freedesktop.org/patch/msgid/20220510122726.v3.2.I4182ae27e00792842cb86f1433990a0ef9c0a073@changeid --- drivers/gpu/drm/display/drm_dp_aux_bus.c | 209 ++++++++++++++++++++----------- include/drm/display/drm_dp_aux_bus.h | 34 ++++- 2 files changed, 168 insertions(+), 75 deletions(-) (limited to 'include/drm') diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c index 552f949cff59..f5741b45ca07 100644 --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c @@ -3,10 +3,10 @@ * Copyright 2021 Google Inc. * * The DP AUX bus is used for devices that are connected over a DisplayPort - * AUX bus. The devices on the far side of the bus are referred to as - * endpoints in this code. + * AUX bus. The device on the far side of the bus is referred to as an + * endpoint in this code. * - * Commonly there is only one device connected to the DP AUX bus: a panel. + * There is only one device connected to the DP AUX bus: an eDP panel. * Though historically panels (even DP panels) have been modeled as simple * platform devices, putting them under the DP AUX bus allows the panel driver * to perform transactions on that bus. @@ -22,6 +22,11 @@ #include #include +struct dp_aux_ep_device_with_data { + struct dp_aux_ep_device aux_ep; + int (*done_probing)(struct drm_dp_aux *aux); +}; + /** * dp_aux_ep_match() - The match function for the dp_aux_bus. * @dev: The device to match. @@ -48,6 +53,8 @@ static int dp_aux_ep_probe(struct device *dev) { struct dp_aux_ep_driver *aux_ep_drv = to_dp_aux_ep_drv(dev->driver); struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev); + struct dp_aux_ep_device_with_data *aux_ep_with_data = + container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep); int ret; ret = dev_pm_domain_attach(dev, true); @@ -56,7 +63,32 @@ static int dp_aux_ep_probe(struct device *dev) ret = aux_ep_drv->probe(aux_ep); if (ret) - dev_pm_domain_detach(dev, true); + goto err_attached; + + if (aux_ep_with_data->done_probing) { + ret = aux_ep_with_data->done_probing(aux_ep->aux); + if (ret) { + /* + * The done_probing() callback should not return + * -EPROBE_DEFER to us. If it does, we treat it as an + * error. Passing it on as-is would cause the _panel_ + * to defer. + */ + if (ret == -EPROBE_DEFER) { + dev_err(dev, + "DP AUX done_probing() can't defer\n"); + ret = -EINVAL; + } + goto err_probed; + } + } + + return 0; +err_probed: + if (aux_ep_drv->remove) + aux_ep_drv->remove(aux_ep); +err_attached: + dev_pm_domain_detach(dev, true); return ret; } @@ -122,7 +154,11 @@ ATTRIBUTE_GROUPS(dp_aux_ep_dev); */ static void dp_aux_ep_dev_release(struct device *dev) { - kfree(to_dp_aux_ep_dev(dev)); + struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev); + struct dp_aux_ep_device_with_data *aux_ep_with_data = + container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep); + + kfree(aux_ep_with_data); } static struct device_type dp_aux_device_type_type = { @@ -136,12 +172,14 @@ static struct device_type dp_aux_device_type_type = { * @dev: The device to destroy. * @data: Not used * - * This is just used as a callback by of_dp_aux_depopulate_ep_devices() and + * This is just used as a callback by of_dp_aux_depopulate_bus() and * is called for _all_ of the child devices of the device providing the AUX bus. * We'll only act on those that are of type "dp_aux_bus_type". * - * This function is effectively an inverse of what's in the loop - * in of_dp_aux_populate_ep_devices(). + * This function is effectively an inverse of what's in + * of_dp_aux_populate_bus(). NOTE: since we only populate one child + * then it's expected that only one device will match all the "if" tests in + * this function and get to the device_unregister(). * * Return: 0 if no error or negative error code. */ @@ -164,123 +202,150 @@ static int of_dp_aux_ep_destroy(struct device *dev, void *data) } /** - * of_dp_aux_depopulate_ep_devices() - Undo of_dp_aux_populate_ep_devices - * @aux: The AUX channel whose devices we want to depopulate + * of_dp_aux_depopulate_bus() - Undo of_dp_aux_populate_bus + * @aux: The AUX channel whose device we want to depopulate * - * This will destroy all devices that were created - * by of_dp_aux_populate_ep_devices(). + * This will destroy the device that was created + * by of_dp_aux_populate_bus(). */ -void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux) +void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux) { device_for_each_child_reverse(aux->dev, NULL, of_dp_aux_ep_destroy); } -EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_ep_devices); +EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_bus); /** - * of_dp_aux_populate_ep_devices() - Populate the endpoint devices on the DP AUX - * @aux: The AUX channel whose devices we want to populate. It is required that + * of_dp_aux_populate_bus() - Populate the endpoint device on the DP AUX + * @aux: The AUX channel whose device we want to populate. It is required that * drm_dp_aux_init() has already been called for this AUX channel. + * @done_probing: Callback functions to call after EP device finishes probing. + * Will not be called if there are no EP devices and this + * function will return -ENODEV. * - * This will populate all the devices under the "aux-bus" node of the device - * providing the AUX channel (AKA aux->dev). + * This will populate the device (expected to be an eDP panel) under the + * "aux-bus" node of the device providing the AUX channel (AKA aux->dev). * * When this function finishes, it is _possible_ (but not guaranteed) that - * our sub-devices will have finished probing. It should be noted that if our - * sub-devices return -EPROBE_DEFER that we will not return any error codes - * ourselves but our sub-devices will _not_ have actually probed successfully - * yet. There may be other cases (maybe added in the future?) where sub-devices - * won't have been probed yet when this function returns, so it's best not to - * rely on that. + * our sub-device will have finished probing. It should be noted that if our + * sub-device returns -EPROBE_DEFER or is probing asynchronously for some + * reason that we will not return any error codes ourselves but our + * sub-device will _not_ have actually probed successfully yet. + * + * In many cases it's important for the caller of this function to be notified + * when our sub device finishes probing. Our sub device is expected to be an + * eDP panel and the caller is expected to be an eDP controller. The eDP + * controller needs to be able to get a reference to the panel when it finishes + * probing. For this reason the caller can pass in a function pointer that + * will be called when our sub-device finishes probing. * * If this function succeeds you should later make sure you call - * of_dp_aux_depopulate_ep_devices() to undo it, or just use the devm version + * of_dp_aux_depopulate_bus() to undo it, or just use the devm version * of this function. * - * Return: 0 if no error or negative error code. + * Return: 0 if no error or negative error code; returns -ENODEV if there are + * no children. The done_probing() function won't be called in that + * case. */ -int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux) +int of_dp_aux_populate_bus(struct drm_dp_aux *aux, + int (*done_probing)(struct drm_dp_aux *aux)) { - struct device_node *bus, *np; + struct device_node *bus = NULL, *np = NULL; struct dp_aux_ep_device *aux_ep; + struct dp_aux_ep_device_with_data *aux_ep_with_data; int ret; /* drm_dp_aux_init() should have been called already; warn if not */ WARN_ON_ONCE(!aux->ddc.algo); if (!aux->dev->of_node) - return 0; - + return -ENODEV; bus = of_get_child_by_name(aux->dev->of_node, "aux-bus"); if (!bus) - return 0; + return -ENODEV; - for_each_available_child_of_node(bus, np) { - if (of_node_test_and_set_flag(np, OF_POPULATED)) - continue; + np = of_get_next_available_child(bus, NULL); + of_node_put(bus); + if (!np) + return -ENODEV; - aux_ep = kzalloc(sizeof(*aux_ep), GFP_KERNEL); - if (!aux_ep) - continue; - aux_ep->aux = aux; + if (of_node_test_and_set_flag(np, OF_POPULATED)) { + dev_err(aux->dev, "DP AUX EP device already populated\n"); + ret = -EINVAL; + goto err_did_get_np; + } - aux_ep->dev.parent = aux->dev; - aux_ep->dev.bus = &dp_aux_bus_type; - aux_ep->dev.type = &dp_aux_device_type_type; - aux_ep->dev.of_node = of_node_get(np); - dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev)); + aux_ep_with_data = kzalloc(sizeof(*aux_ep_with_data), GFP_KERNEL); + if (!aux_ep_with_data) { + ret = -ENOMEM; + goto err_did_set_populated; + } - ret = device_register(&aux_ep->dev); - if (ret) { - dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret); - of_node_clear_flag(np, OF_POPULATED); - of_node_put(np); + aux_ep_with_data->done_probing = done_probing; - /* - * As per docs of device_register(), call this instead - * of kfree() directly for error cases. - */ - put_device(&aux_ep->dev); + aux_ep = &aux_ep_with_data->aux_ep; + aux_ep->aux = aux; + aux_ep->dev.parent = aux->dev; + aux_ep->dev.bus = &dp_aux_bus_type; + aux_ep->dev.type = &dp_aux_device_type_type; + aux_ep->dev.of_node = of_node_get(np); + dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev)); - /* - * Following in the footsteps of of_i2c_register_devices(), - * we won't fail the whole function here--we'll just - * continue registering any other devices we find. - */ - } - } + ret = device_register(&aux_ep->dev); + if (ret) { + dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret); - of_node_put(bus); + /* + * As per docs of device_register(), call this instead + * of kfree() directly for error cases. + */ + put_device(&aux_ep->dev); + + goto err_did_set_populated; + } return 0; + +err_did_set_populated: + of_node_clear_flag(np, OF_POPULATED); + +err_did_get_np: + of_node_put(np); + + return ret; } -EXPORT_SYMBOL_GPL(of_dp_aux_populate_ep_devices); +EXPORT_SYMBOL_GPL(of_dp_aux_populate_bus); -static void of_dp_aux_depopulate_ep_devices_void(void *data) +static void of_dp_aux_depopulate_bus_void(void *data) { - of_dp_aux_depopulate_ep_devices(data); + of_dp_aux_depopulate_bus(data); } /** - * devm_of_dp_aux_populate_ep_devices() - devm wrapper for of_dp_aux_populate_ep_devices() - * @aux: The AUX channel whose devices we want to populate + * devm_of_dp_aux_populate_bus() - devm wrapper for of_dp_aux_populate_bus() + * @aux: The AUX channel whose device we want to populate + * @done_probing: Callback functions to call after EP device finishes probing. + * Will not be called if there are no EP devices and this + * function will return -ENODEV. * * Handles freeing w/ devm on the device "aux->dev". * - * Return: 0 if no error or negative error code. + * Return: 0 if no error or negative error code; returns -ENODEV if there are + * no children. The done_probing() function won't be called in that + * case. */ -int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux) +int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux, + int (*done_probing)(struct drm_dp_aux *aux)) { int ret; - ret = of_dp_aux_populate_ep_devices(aux); + ret = of_dp_aux_populate_bus(aux, done_probing); if (ret) return ret; return devm_add_action_or_reset(aux->dev, - of_dp_aux_depopulate_ep_devices_void, - aux); + of_dp_aux_depopulate_bus_void, aux); } -EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_ep_devices); +EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_bus); int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *drv, struct module *owner) { diff --git a/include/drm/display/drm_dp_aux_bus.h b/include/drm/display/drm_dp_aux_bus.h index 4f19b20b1dd6..8a0a486383c5 100644 --- a/include/drm/display/drm_dp_aux_bus.h +++ b/include/drm/display/drm_dp_aux_bus.h @@ -44,9 +44,37 @@ static inline struct dp_aux_ep_driver *to_dp_aux_ep_drv(struct device_driver *dr return container_of(drv, struct dp_aux_ep_driver, driver); } -int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux); -void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux); -int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux); +int of_dp_aux_populate_bus(struct drm_dp_aux *aux, + int (*done_probing)(struct drm_dp_aux *aux)); +void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux); +int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux, + int (*done_probing)(struct drm_dp_aux *aux)); + +/* Deprecated versions of the above functions. To be removed when no callers. */ +static inline int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux) +{ + int ret; + + ret = of_dp_aux_populate_bus(aux, NULL); + + /* New API returns -ENODEV for no child case; adapt to old assumption */ + return (ret != -ENODEV) ? ret : 0; +} + +static inline int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux) +{ + int ret; + + ret = devm_of_dp_aux_populate_bus(aux, NULL); + + /* New API returns -ENODEV for no child case; adapt to old assumption */ + return (ret != -ENODEV) ? ret : 0; +} + +static inline void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux) +{ + of_dp_aux_depopulate_bus(aux); +} #define dp_aux_dp_driver_register(aux_ep_drv) \ __dp_aux_dp_driver_register(aux_ep_drv, THIS_MODULE) -- cgit v1.2.3 From 50e156bd8a9d0910ac4bae5fcff00ddb798db967 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Tue, 10 May 2022 12:29:43 -0700 Subject: drm/bridge: Add devm_drm_bridge_add() This adds a devm managed version of drm_bridge_add(). Like other "devm" function listed in drm_bridge.h, this function takes an explicit "dev" to use for the lifetime management. A few notes: * In general we have a "struct device" for bridges that makes a good candidate for where the lifetime matches exactly what we want. * The "bridge->dev->dev" device appears to be the encoder device. That's not the right device to use for lifetime management. Suggested-by: Dmitry Baryshkov Signed-off-by: Douglas Anderson Reviewed-by: Dmitry Baryshkov Link: https://patchwork.freedesktop.org/patch/msgid/20220510122726.v3.3.Iba4b9bf6c7a1ee5ea2835ad7bd5eaf84d7688520@changeid --- drivers/gpu/drm/drm_bridge.c | 23 +++++++++++++++++++++++ include/drm/drm_bridge.h | 1 + 2 files changed, 24 insertions(+) (limited to 'include/drm') diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..e275b4ca344b 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -170,6 +170,29 @@ void drm_bridge_add(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_add); +static void drm_bridge_remove_void(void *bridge) +{ + drm_bridge_remove(bridge); +} + +/** + * devm_drm_bridge_add - devm managed version of drm_bridge_add() + * + * @dev: device to tie the bridge lifetime to + * @bridge: bridge control structure + * + * This is the managed version of drm_bridge_add() which automatically + * calls drm_bridge_remove() when @dev is unbound. + * + * Return: 0 if no error or negative error code. + */ +int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge) +{ + drm_bridge_add(bridge); + return devm_add_action_or_reset(dev, drm_bridge_remove_void, bridge); +} +EXPORT_SYMBOL(devm_drm_bridge_add); + /** * drm_bridge_remove - remove the given bridge from the global bridge list * diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index f27b4060faa2..42aec8612f37 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -796,6 +796,7 @@ drm_priv_to_bridge(struct drm_private_obj *priv) } void drm_bridge_add(struct drm_bridge *bridge); +int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous, -- cgit v1.2.3