Re: [PATCH] drm/msm: Add missing check and destroy for alloc_ordered_workqueue
On Fri, 6 Jan 2023 at 03:38, Jiasheng Jiang wrote: > > Add check for the return value of alloc_ordered_workqueue as it may return > NULL pointer. > Moreover, use the destroy_workqueue in the later fails in order to avoid > memory leak. > > Fixes: c8afe684c95c ("drm/msm: basic KMS driver for snapdragon") > Signed-off-by: Jiasheng Jiang Please resend the patch, including freedreno@ to the cc list. > --- > drivers/gpu/drm/msm/msm_drv.c | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) -- With best wishes Dmitry
Re: [PATCH] drm/vkms: introduce prepare_fb and cleanup_fb functions
Hi Am 05.01.23 um 19:43 schrieb Melissa Wen: On 01/05, Maíra Canal wrote: With commit 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap"), the behavior of the shadow-plane helpers changed and the vunmap is now performed at the end of the current pageflip, instead of the end of the following pageflip. By performing the vunmap at the end of the current pageflip, invalid memory is accessed by the vkms during the plane composition, as the data is being unmapped before being used. Hi Maíra, Thanks for investigating this issue. Can you add in the commit message the kernel Oops caused by this change? Besides that, I wonder if the right thing would be to restore the previous behavior of vunmap in shadow-plane helpers, instead of reintroduce driver-specific hooks for vmap/vunmap correctly to vkms. Maybe Thomas has some inputs on this shadow-plane changing to help us on figuring out the proper fix (?) The fix looks good. I left some minor-important comments below. I would suggest to rethink the overall driver design. Instead of keeping these buffer pinned for long. It might be better to have a per-plane intermediate buffer that you update on each call to atomic_update. That's how real drivers interact with their hardware. Best Regards, Melissa Therefore, introduce again prepare_fb and cleanup_fb functions to the vkms, which were previously removed on commit b43e2ec03b0d ("drm/vkms: Let shadow-plane helpers prepare the plane's FB"). Fixes: 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap") Signed-off-by: Maíra Canal Acked-by: Thomas Zimmermann --- drivers/gpu/drm/vkms/vkms_plane.c | 36 ++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index c3a845220e10..b3f8a115cc23 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -160,10 +160,44 @@ static int vkms_plane_atomic_check(struct drm_plane *plane, return 0; } +static int vkms_prepare_fb(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_shadow_plane_state *shadow_plane_state; + struct drm_framebuffer *fb = state->fb; + int ret; + + if (!fb) + return 0; IIRC this cannot be NULL. Only active planes will be 'prepared'. + + shadow_plane_state = to_drm_shadow_plane_state(state); + + ret = drm_gem_plane_helper_prepare_fb(plane, state); + if (ret) + return ret; + + return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data); +} + +static void vkms_cleanup_fb(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_shadow_plane_state *shadow_plane_state; + struct drm_framebuffer *fb = state->fb; + + if (!fb) + return; Same here. This function won't be called if there has not been a framebuffer. + + shadow_plane_state = to_drm_shadow_plane_state(state); + + drm_gem_fb_vunmap(fb, shadow_plane_state->map); +} + static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { .atomic_update = vkms_plane_atomic_update, .atomic_check = vkms_plane_atomic_check, - DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, You're still installing {being/end}_fb_access, which should not be necessary now. Open-coding DRM_GEM_SHADOW_PLANE_HELPER_FUNCS without those helpers would fix that. Best regards Thomas + .prepare_fb = vkms_prepare_fb, + .cleanup_fb = vkms_cleanup_fb, }; struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, -- 2.39.0 -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v3 0/7] drm/bridge_connector: perform HPD enablement automatically
On 05/01/2023 14:31, Dmitry Baryshkov wrote: On 04/01/2023 11:05, Neil Armstrong wrote: On 04/01/2023 08:29, Tomi Valkeinen wrote: On 28/12/2022 23:58, Dmitry Baryshkov wrote: On 02/11/2022 20:06, Dmitry Baryshkov wrote: From all the drivers using drm_bridge_connector only iMX/dcss and OMAP DRM driver do a proper work of calling drm_bridge_connector_en/disable_hpd() in right places. Rather than teaching each and every driver how to properly handle drm_bridge_connector's HPD, make that automatic. Add two additional drm_connector helper funcs: enable_hpd() and disable_hpd(). Make drm_kms_helper_poll_* functions call them (as this is the time where the drm_bridge_connector's functions are called by the drivers too). Since we are at the beginning of the development window, gracious ping for this patchset. It would be nice to finally handle the bridge_connector's hpd properly. Calling drm_bridge_connector_enable_hpd() from drm_bridge_connector_init() is not a proper way to do this. It results in calling bridge->funcs->hpd_enable() before the rest of the pipeline was set up properly. For the series: Reviewed-by: Tomi Valkeinen I've been using this series in my local branch for quite a while to fix the HPD issues. Works for me. Thanks! I still think the "fix" aspect should be highlighted more here, as the current upstream triggers a WARN for "Hot plug detection already enabled" (at least) on OMAP. LGTM then ! Tomi, Dmitry, I can push the whole serie via drm-misc-next or -fixes then, as you wish. I'm fine either way. We have been living with the warning for some time, so I don't think there is any urgency to get rid of it immediately. Yes, drm-misc-next is fine for me too. Tomi
Re: [PATCH] fbmem: prevent potential use-after-free issues with console_lock()
On Thu, Jan 5, 2023 at 5:25 AM Daniel Vetter wrote: > > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter wrote: > > > > Hi Helge > > > > On Mon, 2 Jan 2023 at 16:28, Helge Deller wrote: > > > > > > On 12/30/22 07:35, Hang Zhang wrote: > > > > In do_fb_ioctl(), user specified "fb_info" can be freed in the callee > > > > fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() -> > > > > con2fb_release_oldinfo(), this free operation is protected by > > > > console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in > > > > the change of certain states such as "minfo->dead" in matroxfb_remove(), > > > > so that it can be checked to avoid use-after-free before the use sites > > > > (e.g., the check at the beginning of matroxfb_ioctl()). However, > > > > the problem is that the use site is not protected by the same locks > > > > as for the free operation, e.g., "default" case in do_fb_ioctl() > > > > can lead to "matroxfb_ioctl()" but it's not protected by console_lock(), > > > > which can invalidate the aforementioned state set and check in a > > > > concurrent setting. > > > > > > > > Prevent the potential use-after-free issues by protecting the "default" > > > > case in do_fb_ioctl() with console_lock(), similarly as for many other > > > > cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY". > > > > > > > > Signed-off-by: Hang Zhang > > > > > > applied to fbdev git tree. > > > > The patch above makes no sense at all to me: > > > > - fb_info is protected by lock_fb_info and > > - the lifetime of fb_info is protected by the get/put functions > > - yes there's the interaction with con2fb, which is protected by > > console_lock, but the lifetime guarantees are ensured by the device > > removal > > - which means any stuff happening in matroxfb_remove is also not a > > concern here (unless matroxfb completely gets all the device lifetime > > stuff wrong, but it doesn't look like it's any worse than any of the > > other fbdev drivers that we haven't recently fixed up due to the > > takeover issues with firmware drivers > > I have also a really hard timing finding the con2fb map use in the > matroxfb ioctl code, but that just might be that I didn't look > carefully enough. Maybe that would shed some light on this. > -Daniel > > > > > > On the very clear downside this now means we take console_lock for the > > vblank ioctl (which is a device driver extension for reasons, despite > > that it's a standard fbdev ioctl), which is no good at all given how > > console_lock() is a really expensive lock. > > > > Unless I'm massively missing something, can you pls push the revert > > before this lands in Linus' tree? > > > > Thanks, Daniel Hi, Daniel. Thank you for your feedback. We're not developers of the video subsystem and thus may be short in domain knowledge (e.g., the performance of console_lock() and the complex lifetime management). This patch initially intended to bring up the potential use-after-free issues here to the community - we have performed a best-effort code review and cannot exclude the possibility based on our understanding. What we have observed is that the call chain leading to the free site (do_fb_ioctl()->fbcon_set_con2fb_map_ioctl()->set_con2fb_map()-> con2fb_release_oldinfo()-> ... ->matroxfb_remove()) is only protected by console_lock() but not lock_fb_info(), while the potential use site (call chain starts from the default case in do_fb_ioctl()) is only protected by lock_fb_info() but not console_lock(). We thus propose to add this extra console_lock() to the default case, which is inspired by the lock protection of many other existing switch-case terms in the same function. Since we do not have deep domain knowledge of this subsystem, we will rely on the developers to make a decision regarding the patch. Thank you again for your review and help! Best, Hang > > > > > Thanks, > > > Helge > > > > > > > --- > > > > drivers/video/fbdev/core/fbmem.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/video/fbdev/core/fbmem.c > > > > b/drivers/video/fbdev/core/fbmem.c > > > > index 1e70d8c67653..8b1a1527d18a 100644 > > > > --- a/drivers/video/fbdev/core/fbmem.c > > > > +++ b/drivers/video/fbdev/core/fbmem.c > > > > @@ -1182,6 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, > > > > unsigned int cmd, > > > > console_unlock(); > > > > break; > > > > default: > > > > + console_lock(); > > > > lock_fb_info(info); > > > > fb = info->fbops; > > > > if (fb->fb_ioctl) > > > > @@ -1189,6 +1190,7 @@ static long do_fb_ioctl(struct fb_info *info, > > > > unsigned int cmd, > > > > else > > > > ret = -ENOTTY; > > > > unlock_fb_info(info); > > > > + console_unlock(); > > > > } > > > > return ret; > > > > } > > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwl
Re: [PATCH] fbcon: Use kzalloc() in fbcon_prepare_logo()
On 2023/01/05 22:22, Daniel Vetter wrote: > Oh I was more asking about the fbdev patch. This here sounds a lot more > something that needs to be discussed with kmsan people, that's definitely > not my area. > -Daniel Commit a6a00d7e8ffd ("fbcon: Use kzalloc() in fbcon_prepare_logo()") was redundant but not reverting that commit is harmless. You don't need to worry about this problem. This is a problem for KMSAN people.
Re: [PATCH] drm/msm: Set preferred depth.
Hi Am 06.01.23 um 08:16 schrieb Steev Klimaszewski: As of commit 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format selection"), if no supported color formats are found, it tries to use the driver provided default, which msm didn't have set and leads to the following output: msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not supported msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not supported msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not supported msm_dpu ae01000.display-controller: [drm] No compatible format found [ cut here ] WARNING: CPU: 0 PID: 73 at drivers/gpu/drm/drm_atomic.c:1604 __drm_atomic_helper_set_config+0x240/0x33c Modules linked in: ext4 mbcache jbd2 msm mdt_loader ocmem gpu_sched llcc_qcom gpio_keys qrtr CPU: 0 PID: 73 Comm: kworker/u16:2 Not tainted 6.2.0-rc2-next-20230106 #53 Hardware name: LENOVO 21BX0015US/21BX0015US, BIOS N3HET74W (1.46 ) 10/12/2022 Workqueue: events_unbound deferred_probe_work_func pstate: 8045 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : __drm_atomic_helper_set_config+0x240/0x33c lr : __drm_atomic_helper_set_config+0x68/0x33c sp : 88a7b790 x29: 88a7b790 x28: 73ee3e130a00 x27: x26: 73ee3d256e00 x25: 0038 x24: 73e6c0d65e00 x23: 73e6c17a7800 x22: 73e6c0d64e00 x21: 73e79c025e00 x20: c0d64e00 x19: 73ee3e130a00 x18: x17: 662074616d726f66 x16: 20656c6269746170 x15: x14: x13: x12: x11: x10: x9 : a829144ff8bc x8 : x7 : x6 : x5 : 73e6c0d65f50 x4 : 73ee3d254950 x3 : 73e6c0d65ec0 x2 : 73ee3c953a00 x1 : 73e79c025580 x0 : Call trace: __drm_atomic_helper_set_config+0x240/0x33c drm_client_modeset_commit_atomic+0x160/0x280 drm_client_modeset_commit_locked+0x64/0x194 drm_client_modeset_commit+0x38/0x60 __drm_fb_helper_initial_config_and_unlock+0x528/0x63c drm_fb_helper_initial_config+0x54/0x64 msm_fbdev_init+0x94/0xfc [msm] msm_drm_bind+0x548/0x614 [msm] try_to_bring_up_aggregate_device+0x1e4/0x2d0 __component_add+0xc4/0x1c0 component_add+0x1c/0x2c dp_display_probe+0x2a4/0x460 [msm] platform_probe+0x70/0xcc really_probe+0xc8/0x3e0 __driver_probe_device+0x84/0x190 driver_probe_device+0x44/0x120 __device_attach_driver+0xc4/0x160 bus_for_each_drv+0x84/0xe0 __device_attach+0xa4/0x1cc device_initial_probe+0x1c/0x2c bus_probe_device+0xa4/0xb0 deferred_probe_work_func+0xc0/0x114 process_one_work+0x1ec/0x470 worker_thread+0x74/0x410 kthread+0xfc/0x110 ret_from_fork+0x10/0x20 ---[ end trace ]--- Signed-off-by: Steev Klimaszewski --- drivers/gpu/drm/msm/msm_drv.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 8b0b0ac74a6f..65c4c93c311e 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -479,6 +479,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) drm_helper_move_panel_connectors_to_head(ddev); + ddev->mode_config.preferred_depth = 24; Reviewed-by: Thomas Zimmermann Best regards Thomas ddev->mode_config.funcs = &mode_config_funcs; ddev->mode_config.helper_private = &mode_config_helper_funcs; -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
[bug report] habanalabs: Timestamps buffers registration
Hello farah kassabri, The patch 9158bf69e74f: "habanalabs: Timestamps buffers registration" from Dec 23, 2021, leads to the following Smatch static checker warning: drivers/accel/habanalabs/common/memory.c:2178 hl_ts_alloc_buf() warn: use 'gfp' here instead of GFP_XXX? drivers/accel/habanalabs/common/memory.c 2170 static int hl_ts_alloc_buf(struct hl_mmap_mem_buf *buf, gfp_t gfp, void *args) ^^^ "gfp" is never used. 2171 { 2172 struct hl_ts_buff *ts_buff = NULL; 2173 u32 size, num_elements; 2174 void *p; 2175 2176 num_elements = *(u32 *)args; This business of passing void pointers and pretending that hl_cb_mmap_mem_alloc() and hl_ts_alloc_buf() are the same function is a nightmare. Create two ->alloc functions. Split hl_mmap_mem_buf_alloc() into one function that allocates idr stuff. Create a function to free/remove the idr stuff. Create two new helper function that call the idr function and then the appropriate alloc() function. It will be much cleaner than using a void pointer. 2177 --> 2178 ts_buff = kzalloc(sizeof(*ts_buff), GFP_KERNEL); ^^ Smatch is correct that it should be used here. 2179 if (!ts_buff) 2180 return -ENOMEM; 2181 2182 /* Allocate the user buffer */ 2183 size = num_elements * sizeof(u64); Can this have an integer overflow on 32bit systems? 2184 p = vmalloc_user(size); 2185 if (!p) 2186 goto free_mem; 2187 2188 ts_buff->user_buff_address = p; 2189 buf->mappable_size = size; 2190 2191 /* Allocate the internal kernel buffer */ 2192 size = num_elements * sizeof(struct hl_user_pending_interrupt); 2193 p = vzalloc(size); 2194 if (!p) 2195 goto free_user_buff; 2196 2197 ts_buff->kernel_buff_address = p; 2198 ts_buff->kernel_buff_size = size; 2199 2200 buf->private = ts_buff; 2201 2202 return 0; 2203 2204 free_user_buff: 2205 vfree(ts_buff->user_buff_address); 2206 free_mem: 2207 kfree(ts_buff); 2208 return -ENOMEM; 2209 } regards, dan carpenter
[PATCH RESEND] drm/msm: Add missing check and destroy for alloc_ordered_workqueue
Add check for the return value of alloc_ordered_workqueue as it may return NULL pointer. Moreover, use the destroy_workqueue in the later fails in order to avoid memory leak. Fixes: c8afe684c95c ("drm/msm: basic KMS driver for snapdragon") Signed-off-by: Jiasheng Jiang --- drivers/gpu/drm/msm/msm_drv.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 8b0b0ac74a6f..b82d938226ad 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -418,6 +418,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) priv->dev = ddev; priv->wq = alloc_ordered_workqueue("msm", 0); + if (!priv->wq) + return -ENOMEM; INIT_LIST_HEAD(&priv->objects); mutex_init(&priv->obj_lock); @@ -440,12 +442,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) ret = msm_init_vram(ddev); if (ret) - return ret; + goto err_destroy_workqueue; /* Bind all our sub-components: */ ret = component_bind_all(dev, ddev); if (ret) - return ret; + goto err_destroy_workqueue; dma_set_max_seg_size(dev, UINT_MAX); @@ -540,6 +542,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) err_msm_uninit: msm_drm_uninit(dev); +err_destroy_workqueue: + destroy_workqueue(priv->wq); return ret; } -- 2.25.1
Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > Setting this property will allow the userspace to look for new modes or > position info when a hotplug event occurs. This works just fine for modes today. I assume this is this need to have userspace also check for position info updates added by patch #1)? take care, Gerd
Re: [PATCH] drm/msm: Set preferred depth.
On Fri, Jan 06, 2023 at 09:31:31AM +0100, Thomas Zimmermann wrote: > Am 06.01.23 um 08:16 schrieb Steev Klimaszewski: > > As of commit 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format > > selection"), if no supported color formats are found, it tries to use the > > driver provided default, which msm didn't have set and leads to the > > following output: > > > > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not > > supported > > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not > > supported > > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not > > supported > > msm_dpu ae01000.display-controller: [drm] No compatible format found > > [ cut here ] > > WARNING: CPU: 0 PID: 73 at drivers/gpu/drm/drm_atomic.c:1604 > > __drm_atomic_helper_set_config+0x240/0x33c > > Modules linked in: ext4 mbcache jbd2 msm mdt_loader ocmem gpu_sched > > llcc_qcom gpio_keys qrtr > > CPU: 0 PID: 73 Comm: kworker/u16:2 Not tainted 6.2.0-rc2-next-20230106 #53 > > Hardware name: LENOVO 21BX0015US/21BX0015US, BIOS N3HET74W (1.46 ) > > 10/12/2022 > > Workqueue: events_unbound deferred_probe_work_func > > pstate: 8045 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > pc : __drm_atomic_helper_set_config+0x240/0x33c > > lr : __drm_atomic_helper_set_config+0x68/0x33c > > sp : 88a7b790 > > x29: 88a7b790 x28: 73ee3e130a00 x27: > > x26: 73ee3d256e00 x25: 0038 x24: 73e6c0d65e00 > > x23: 73e6c17a7800 x22: 73e6c0d64e00 x21: 73e79c025e00 > > x20: c0d64e00 x19: 73ee3e130a00 x18: > > x17: 662074616d726f66 x16: 20656c6269746170 x15: > > x14: x13: x12: > > x11: x10: x9 : a829144ff8bc > > x8 : x7 : x6 : > > x5 : 73e6c0d65f50 x4 : 73ee3d254950 x3 : 73e6c0d65ec0 > > x2 : 73ee3c953a00 x1 : 73e79c025580 x0 : > > Call trace: > > __drm_atomic_helper_set_config+0x240/0x33c > > drm_client_modeset_commit_atomic+0x160/0x280 > > drm_client_modeset_commit_locked+0x64/0x194 > > drm_client_modeset_commit+0x38/0x60 > > __drm_fb_helper_initial_config_and_unlock+0x528/0x63c > > drm_fb_helper_initial_config+0x54/0x64 > > msm_fbdev_init+0x94/0xfc [msm] > > msm_drm_bind+0x548/0x614 [msm] > > try_to_bring_up_aggregate_device+0x1e4/0x2d0 > > __component_add+0xc4/0x1c0 > > component_add+0x1c/0x2c > > dp_display_probe+0x2a4/0x460 [msm] > > platform_probe+0x70/0xcc > > really_probe+0xc8/0x3e0 > > __driver_probe_device+0x84/0x190 > > driver_probe_device+0x44/0x120 > > __device_attach_driver+0xc4/0x160 > > bus_for_each_drv+0x84/0xe0 > > __device_attach+0xa4/0x1cc > > device_initial_probe+0x1c/0x2c > > bus_probe_device+0xa4/0xb0 > > deferred_probe_work_func+0xc0/0x114 > > process_one_work+0x1ec/0x470 > > worker_thread+0x74/0x410 > > kthread+0xfc/0x110 > > ret_from_fork+0x10/0x20 > > ---[ end trace ]--- > > > > Signed-off-by: Steev Klimaszewski > > --- > > drivers/gpu/drm/msm/msm_drv.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index 8b0b0ac74a6f..65c4c93c311e 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -479,6 +479,7 @@ static int msm_drm_init(struct device *dev, const > > struct drm_driver *drv) > > drm_helper_move_panel_connectors_to_head(ddev); > > + ddev->mode_config.preferred_depth = 24; > > Reviewed-by: Thomas Zimmermann preferred_depth is not a mandatory thing, we need to fix the fbdev patch, not work around that in all the drivers. xrgb is the assumed default. -Daniel > > Best regards > Thomas > > > ddev->mode_config.funcs = &mode_config_funcs; > > ddev->mode_config.helper_private = &mode_config_helper_funcs; > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] drm/msm: Set preferred depth.
On Fri, Jan 06, 2023 at 09:18:21AM +0200, Dmitry Baryshkov wrote: > On 06/01/2023 09:16, Steev Klimaszewski wrote: > > As of commit 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format > > selection"), if no supported color formats are found, it tries to use the > > driver provided default, which msm didn't have set and leads to the > > following output: > > > > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not > > supported > > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not > > supported > > msm_dpu ae01000.display-controller: [drm] bpp/depth value of 32/0 not > > supported > > msm_dpu ae01000.display-controller: [drm] No compatible format found > > [ cut here ] > > WARNING: CPU: 0 PID: 73 at drivers/gpu/drm/drm_atomic.c:1604 > > __drm_atomic_helper_set_config+0x240/0x33c > > Modules linked in: ext4 mbcache jbd2 msm mdt_loader ocmem gpu_sched > > llcc_qcom gpio_keys qrtr > > CPU: 0 PID: 73 Comm: kworker/u16:2 Not tainted 6.2.0-rc2-next-20230106 #53 > > Hardware name: LENOVO 21BX0015US/21BX0015US, BIOS N3HET74W (1.46 ) > > 10/12/2022 > > Workqueue: events_unbound deferred_probe_work_func > > pstate: 8045 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > pc : __drm_atomic_helper_set_config+0x240/0x33c > > lr : __drm_atomic_helper_set_config+0x68/0x33c > > sp : 88a7b790 > > x29: 88a7b790 x28: 73ee3e130a00 x27: > > x26: 73ee3d256e00 x25: 0038 x24: 73e6c0d65e00 > > x23: 73e6c17a7800 x22: 73e6c0d64e00 x21: 73e79c025e00 > > x20: c0d64e00 x19: 73ee3e130a00 x18: > > x17: 662074616d726f66 x16: 20656c6269746170 x15: > > x14: x13: x12: > > x11: x10: x9 : a829144ff8bc > > x8 : x7 : x6 : > > x5 : 73e6c0d65f50 x4 : 73ee3d254950 x3 : 73e6c0d65ec0 > > x2 : 73ee3c953a00 x1 : 73e79c025580 x0 : > > Call trace: > > __drm_atomic_helper_set_config+0x240/0x33c > > drm_client_modeset_commit_atomic+0x160/0x280 > > drm_client_modeset_commit_locked+0x64/0x194 > > drm_client_modeset_commit+0x38/0x60 > > __drm_fb_helper_initial_config_and_unlock+0x528/0x63c > > drm_fb_helper_initial_config+0x54/0x64 > > msm_fbdev_init+0x94/0xfc [msm] > > msm_drm_bind+0x548/0x614 [msm] > > try_to_bring_up_aggregate_device+0x1e4/0x2d0 > > __component_add+0xc4/0x1c0 > > component_add+0x1c/0x2c > > dp_display_probe+0x2a4/0x460 [msm] > > platform_probe+0x70/0xcc > > really_probe+0xc8/0x3e0 > > __driver_probe_device+0x84/0x190 > > driver_probe_device+0x44/0x120 > > __device_attach_driver+0xc4/0x160 > > bus_for_each_drv+0x84/0xe0 > > __device_attach+0xa4/0x1cc > > device_initial_probe+0x1c/0x2c > > bus_probe_device+0xa4/0xb0 > > deferred_probe_work_func+0xc0/0x114 > > process_one_work+0x1ec/0x470 > > worker_thread+0x74/0x410 > > kthread+0xfc/0x110 > > ret_from_fork+0x10/0x20 > > ---[ end trace ]--- > > > > Signed-off-by: Steev Klimaszewski > > --- > > drivers/gpu/drm/msm/msm_drv.c | 1 + > > 1 file changed, 1 insertion(+) > > Suggested-by: Dmitry Baryshkov > Reviewed-by: Dmitry Baryshkov I think a documentation patch that preferred_depth = 0 actually means xrgb would be good, since we seem to have a serious confusion going on here? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm/i915: Add missing check and destroy for alloc_workqueue
Add checks for the return value of alloc_workqueue and alloc_ordered_workqueue as they may return NULL pointer and cause NULL pointer dereference. Moreover, destroy them when fails later in order to avoid memory leak. Because in `drivers/gpu/drm/i915/i915_driver.c`, if intel_modeset_init_noirq fails, its workqueues will not be destroyed. Fixes: c26a058680dc ("drm/i915: Use a high priority wq for nonblocking plane updates") Fixes: 757fffcfdffb ("drm/i915: Put all non-blocking modesets onto an ordered wq") Signed-off-by: Jiasheng Jiang --- drivers/gpu/drm/i915/display/intel_display.c | 21 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 6c2686ecb62a..58f6840d6bd8 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -8655,26 +8655,35 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915) intel_dmc_ucode_init(i915); i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0); + if (!i915->display.wq.modeset) { + ret = -ENOMEM; + goto cleanup_vga_client_pw_domain_dmc; + } + i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI | WQ_UNBOUND, WQ_UNBOUND_MAX_ACTIVE); + if (!i915->display.wq.flip) { + ret = -ENOMEM; + goto cleanup_modeset; + } intel_mode_config_init(i915); ret = intel_cdclk_init(i915); if (ret) - goto cleanup_vga_client_pw_domain_dmc; + goto cleanup_flip; ret = intel_color_init(i915); if (ret) - goto cleanup_vga_client_pw_domain_dmc; + goto cleanup_flip; ret = intel_dbuf_init(i915); if (ret) - goto cleanup_vga_client_pw_domain_dmc; + goto cleanup_flip; ret = intel_bw_init(i915); if (ret) - goto cleanup_vga_client_pw_domain_dmc; + goto cleanup_flip; init_llist_head(&i915->display.atomic_helper.free_list); INIT_WORK(&i915->display.atomic_helper.free_work, @@ -8686,6 +8695,10 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915) return 0; +cleanup_flip: + destroy_workqueue(i915->display.wq.flip); +cleanup_modeset: + destroy_workqueue(i915->display.wq.modeset); cleanup_vga_client_pw_domain_dmc: intel_dmc_ucode_fini(i915); intel_power_domains_driver_remove(i915); -- 2.25.1
Re: [PATCH] drm/vkms: introduce prepare_fb and cleanup_fb functions
On Fri, Jan 06, 2023 at 09:10:17AM +0100, Thomas Zimmermann wrote: > Hi > > Am 05.01.23 um 19:43 schrieb Melissa Wen: > > On 01/05, Maíra Canal wrote: > > > With commit 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, > > > end}_fb_access with vmap"), the behavior of the shadow-plane helpers > > > changed and the vunmap is now performed at the end of > > > the current pageflip, instead of the end of the following pageflip. > > > > > > By performing the vunmap at the end of the current pageflip, invalid > > > memory is accessed by the vkms during the plane composition, as the data > > > is being unmapped before being used. > > > > Hi Maíra, > > > > Thanks for investigating this issue. Can you add in the commit message > > the kernel Oops caused by this change? > > > > Besides that, I wonder if the right thing would be to restore the > > previous behavior of vunmap in shadow-plane helpers, instead of > > reintroduce driver-specific hooks for vmap/vunmap correctly to vkms. > > > > Maybe Thomas has some inputs on this shadow-plane changing to help us on > > figuring out the proper fix (?) > > The fix looks good. I left some minor-important comments below. > > I would suggest to rethink the overall driver design. Instead of keeping > these buffer pinned for long. It might be better to have a per-plane > intermediate buffer that you update on each call to atomic_update. That's > how real drivers interact with their hardware. That would mean more copying, and vkms already copies a lot by recomputing the crc every frame. Fundamentally with vkms the cpu is the display engine, and it needs a mapping for as long as the buffer is in use. Also I guess we really need gitlab ci going, it's a bit silly we're breaking pure sw drivers :-/ -Daniel > > > > > Best Regards, > > > > Melissa > > > > > > > > Therefore, introduce again prepare_fb and cleanup_fb functions to the > > > vkms, which were previously removed on commit b43e2ec03b0d ("drm/vkms: > > > Let shadow-plane helpers prepare the plane's FB"). > > > > > > Fixes: 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, > > > end}_fb_access with vmap") > > > Signed-off-by: Maíra Canal > > Acked-by: Thomas Zimmermann > > > > --- > > > drivers/gpu/drm/vkms/vkms_plane.c | 36 ++- > > > 1 file changed, 35 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c > > > b/drivers/gpu/drm/vkms/vkms_plane.c > > > index c3a845220e10..b3f8a115cc23 100644 > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c > > > @@ -160,10 +160,44 @@ static int vkms_plane_atomic_check(struct drm_plane > > > *plane, > > > return 0; > > > } > > > > > > +static int vkms_prepare_fb(struct drm_plane *plane, > > > +struct drm_plane_state *state) > > > +{ > > > + struct drm_shadow_plane_state *shadow_plane_state; > > > + struct drm_framebuffer *fb = state->fb; > > > + int ret; > > > + > > > + if (!fb) > > > + return 0; > > IIRC this cannot be NULL. Only active planes will be 'prepared'. > > > > + > > > + shadow_plane_state = to_drm_shadow_plane_state(state); > > > + > > > + ret = drm_gem_plane_helper_prepare_fb(plane, state); > > > + if (ret) > > > + return ret; > > > + > > > + return drm_gem_fb_vmap(fb, shadow_plane_state->map, > > > shadow_plane_state->data); > > > +} > > > + > > > +static void vkms_cleanup_fb(struct drm_plane *plane, > > > + struct drm_plane_state *state) > > > +{ > > > + struct drm_shadow_plane_state *shadow_plane_state; > > > + struct drm_framebuffer *fb = state->fb; > > > + > > > + if (!fb) > > > + return; > > Same here. This function won't be called if there has not been a > framebuffer. > > > > + > > > + shadow_plane_state = to_drm_shadow_plane_state(state); > > > + > > > + drm_gem_fb_vunmap(fb, shadow_plane_state->map); > > > +} > > > + > > > static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { > > > .atomic_update = vkms_plane_atomic_update, > > > .atomic_check = vkms_plane_atomic_check, > > > - DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, > > You're still installing {being/end}_fb_access, which should not be necessary > now. Open-coding DRM_GEM_SHADOW_PLANE_HELPER_FUNCS without those helpers > would fix that. > > Best regards > Thomas > > > > + .prepare_fb = vkms_prepare_fb, > > > + .cleanup_fb = vkms_cleanup_fb, > > > }; > > > > > > struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, > > > -- > > > 2.39.0 > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
On Thu, Jan 05, 2023 at 07:38:26PM +0200, Oded Gabbay wrote: > On Thu, Jan 5, 2023 at 6:25 PM Jeffrey Hugo wrote: > > > > On 1/5/2023 5:57 AM, Daniel Vetter wrote: > > > On Thu, Dec 08, 2022 at 12:07:27PM +0100, Jacek Lawrynowicz wrote: > > >> +static const struct drm_driver driver = { > > >> +.driver_features = DRIVER_GEM | DRIVER_COMPUTE_ACCEL, > > > > > > So I was wondering whether this is a bright idea, and whether we shouldn't > > > just go ahead and infuse more meaning into accel vs render nodes. > > > > > > The uapi relevant part of render nodes is that they're multi-user safe, at > > > least as much as feasible. Every new open() gives you a new private > > > accelerator. This also has implications on how userspace drivers iterate > > > them, they just open them all in turn and check whether it's the right > > > one - because userspace apis allow applications to enumerate them all. > > > Which also means that any devicie initialization at open() time is a > > > really bad idea. > > > > > > A lot of the compute accelerators otoh (well habanalabs) are single user, > > > init can be done at open() time because you only open this when you > > > actually know you're going to use it. > > > > > > So given this, shouldn't multi-user inference engines be more like render > > > drivers, and less like accel? So DRIVER_RENDER, but still under > > > drivers/accel. > > > > > > This way that entire separate /dev node would actually become meaningful > > > beyond just the basic bikeshed: > > > - render nodes are multi user, safe to iterate and open() just for > > >iteration > > > - accel nodes are single user, you really should not ever open them unless > > >you want to use them > > > > > > Of course would need a doc patch :-) > > > > > > Thoughts? > > > -Daniel > > > > Hmm. > > > > I admit, I thought DRIVER_ACCEL was the same as DRIVER_RENDER, except > > that DRIVER_ACCEL dropped the "legacy" dual node setup and also avoided > > "legacy" userspace. > > > > qaic is multi-user. I thought habana was the same, at-least for > > inference. Oded, am I wrong? > Habana's devices support a single user at a time acquiring the device > and working on it. > Both for training and inference. > > > > So, if DRIVER_ACCEL is for single-user (training?), and multi-user ends > > up in DRIVER_RENDER, that would seem to mean qaic ends up using > > DRIVER_RENDER and not DRIVER_ACCEL. Then qaic ends up over under > > /dev/dri with both a card node (never used) and a render node. That > > would seem to mean that the "legacy" userspace would open qaic nodes by > > default - something I understood Oded was trying to avoid. > > > > If there really a usecase for DRIVER_ACCEL to support single-user? I > > wonder why we can't default to multi-user, and if a particular > > user/driver has a single-user usecase, it enforces that in a driver > > specific manner? > > > > -Jeff > > Honestly, Daniel, I don't like this suggestion. I don't understand why > you make a connection between render/accel to single/multi user. > > As Jeff has said, one of the goals was to expose accelerator devices > to userspace with new device char nodes so we won't be bothered by > legacy userspace graphics software. This is something we all agreed on > and I don't see why we should change it now, even if you think it's > bike-shedding (which I disagree with). > > But in any case, creating a new device char nodes had nothing to do > with whether the device supports single or multi user. I can > definitely see in the future training devices that support multiple > users. > > The common drm/accel ioctls should of course not be limited to a > single user, and I agree with Jeff here, if a specific driver has such > a limitation (e.g. Habana), then that driver should handle it on its > own. > Maybe if there will be multiple drivers with such a limitation, we can > make that "handling" to be common code. > > Bottom line, I prefer we keep things as we all agreed upon in LPC. The problem is going to happen as soon as you have cross-vendor userspace. Which I'm kinda hoping is at least still the aspiration. Because with cross-vendor userspace you generally iterate & open all devices before you select the one you're going to use. And so we do kinda need a distinction, or we need that the single-user drivers also guarantee that open() is cheap. With just habana and vpu there's no problem because you'll never see these in the same machine. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] drm/i915: Add missing check and destroy for alloc_workqueue
On Fri, Jan 06, 2023 at 05:09:34PM +0800, Jiasheng Jiang wrote: > Add checks for the return value of alloc_workqueue and > alloc_ordered_workqueue as they may return NULL pointer and cause NULL > pointer dereference. > Moreover, destroy them when fails later in order to avoid memory leak. > Because in `drivers/gpu/drm/i915/i915_driver.c`, if > intel_modeset_init_noirq fails, its workqueues will not be destroyed. > > Fixes: c26a058680dc ("drm/i915: Use a high priority wq for nonblocking plane > updates") > Fixes: 757fffcfdffb ("drm/i915: Put all non-blocking modesets onto an ordered > wq") > Signed-off-by: Jiasheng Jiang Reviewed-by: Stanislaw Gruszka > drivers/gpu/drm/i915/display/intel_display.c | 21 > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 6c2686ecb62a..58f6840d6bd8 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -8655,26 +8655,35 @@ int intel_modeset_init_noirq(struct drm_i915_private > *i915) > intel_dmc_ucode_init(i915); > > i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0); > + if (!i915->display.wq.modeset) { > + ret = -ENOMEM; > + goto cleanup_vga_client_pw_domain_dmc; > + } > + > i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI | > WQ_UNBOUND, > WQ_UNBOUND_MAX_ACTIVE); > + if (!i915->display.wq.flip) { > + ret = -ENOMEM; > + goto cleanup_modeset; > + } > > intel_mode_config_init(i915); > > ret = intel_cdclk_init(i915); > if (ret) > - goto cleanup_vga_client_pw_domain_dmc; > + goto cleanup_flip; > > ret = intel_color_init(i915); > if (ret) > - goto cleanup_vga_client_pw_domain_dmc; > + goto cleanup_flip; > > ret = intel_dbuf_init(i915); > if (ret) > - goto cleanup_vga_client_pw_domain_dmc; > + goto cleanup_flip; > > ret = intel_bw_init(i915); > if (ret) > - goto cleanup_vga_client_pw_domain_dmc; > + goto cleanup_flip; > > init_llist_head(&i915->display.atomic_helper.free_list); > INIT_WORK(&i915->display.atomic_helper.free_work, > @@ -8686,6 +8695,10 @@ int intel_modeset_init_noirq(struct drm_i915_private > *i915) > > return 0; > > +cleanup_flip: > + destroy_workqueue(i915->display.wq.flip); > +cleanup_modeset: > + destroy_workqueue(i915->display.wq.modeset); > cleanup_vga_client_pw_domain_dmc: > intel_dmc_ucode_fini(i915); > intel_power_domains_driver_remove(i915); > -- > 2.25.1 >
[PATCH][RFC] drm/fb-helper: Replace bpp/depth parameter by color mode
Replace the combination of bpp and depth with a single color-mode argument. Handle special cases in simpledrm and ofdrm. Hard-code XRGB as fallback format for cases where no given format works. The color-mode argument accepts the same values as the kernel's video parameter. These are mostly bpp values between 1 and 32. The exceptions are 15, which has a color depth of 15 and a bpp value of 16; and 32, which has a color depth of 24 and a bpp value of 32. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_fb_helper.c | 64 - drivers/gpu/drm/drm_fbdev_generic.c | 5 --- drivers/gpu/drm/tiny/ofdrm.c| 7 +++- drivers/gpu/drm/tiny/simpledrm.c| 7 +++- 4 files changed, 47 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1369ca4ae39b..8dcbe3693fc0 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1756,24 +1756,21 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const return DRM_FORMAT_INVALID; } -static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helper, - const uint32_t *formats, size_t format_count, - const struct drm_cmdline_mode *cmdline_mode) +static uint32_t drm_fb_helper_find_color_mode_format(struct drm_fb_helper *fb_helper, +const uint32_t *formats, size_t format_count, +unsigned int color_mode) { struct drm_device *dev = fb_helper->dev; uint32_t bpp, depth; - if (!cmdline_mode->bpp_specified) - return DRM_FORMAT_INVALID; - - switch (cmdline_mode->bpp) { + switch (color_mode) { case 1: case 2: case 4: case 8: case 16: case 24: - bpp = depth = cmdline_mode->bpp; + bpp = depth = color_mode; break; case 15: bpp = 16; @@ -1784,14 +1781,14 @@ static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helpe depth = 24; break; default: - drm_info(dev, "unsupported bpp value of %d\n", cmdline_mode->bpp); + drm_info(dev, "unsupported color mode of %d\n", color_mode); return DRM_FORMAT_INVALID; } return drm_fb_helper_find_format(fb_helper, formats, format_count, bpp, depth); } -static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int preferred_bpp, +static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, unsigned int color_mode, struct drm_fb_helper_surface_size *sizes) { struct drm_client_dev *client = &fb_helper->client; @@ -1817,10 +1814,10 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe drm_client_for_each_connector_iter(connector, &conn_iter) { struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode; - surface_format = drm_fb_helper_find_cmdline_format(fb_helper, - plane->format_types, - plane->format_count, - cmdline_mode); + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + cmdline_mode->bpp); if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ } @@ -1829,17 +1826,23 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ - /* try preferred bpp/depth */ - surface_format = drm_fb_helper_find_format(fb_helper, plane->format_types, - plane->format_count, preferred_bpp, - dev->mode_config.preferred_depth); + /* try preferred color mode */ + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plan
Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
On Fri, Jan 06, 2023 at 09:56:40AM +0100, Gerd Hoffmann wrote: > On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > > Setting this property will allow the userspace to look for new modes or > > position info when a hotplug event occurs. > > This works just fine for modes today. > > I assume this is this need to have userspace also check for position > info updates added by patch #1)? What does this thing even do? Quick grep says qxl and vmwgfx also use this, but it's not documented anywhere, and it's also not done with any piece of common code. Which all looks really fishy. I think we need to do a bit of refactoring/documenting here first. Also in principle, userspace needs to look at everything in the connector again when it gets a hotplug event. We do have hotplug events for specific properties nowadays, but those are fairly new. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 22/27] fbdev: remove tmiofb driver
On 1/5/23 14:46, Arnd Bergmann wrote: From: Arnd Bergmann With the TMIO MFD support removed, the framebuffer driver can be removed as well. Cc: Helge Deller Cc: linux-fb...@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: Arnd Bergmann Acked-by: Helge Deller Arnd, I assume you will push the whole series through the ARM tree (which I'd prefer) ? Helge --- drivers/video/fbdev/Kconfig | 22 - drivers/video/fbdev/Makefile |1 - drivers/video/fbdev/tmiofb.c | 1040 -- 3 files changed, 1063 deletions(-) delete mode 100644 drivers/video/fbdev/tmiofb.c diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index 28febf400666..3152f1a06a39 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -1871,28 +1871,6 @@ config FB_SH_MOBILE_LCDC help Frame buffer driver for the on-chip SH-Mobile LCD controller. -config FB_TMIO - tristate "Toshiba Mobile IO FrameBuffer support" - depends on FB && (MFD_TMIO || COMPILE_TEST) - select FB_CFB_FILLRECT - select FB_CFB_COPYAREA - select FB_CFB_IMAGEBLIT - help - Frame buffer driver for the Toshiba Mobile IO integrated as found - on the Sharp SL-6000 series - - This driver is also available as a module ( = code which can be - inserted and removed from the running kernel whenever you want). The - module will be called tmiofb. If you want to compile it as a module, - say M here and read . - - If unsure, say N. - -config FB_TMIO_ACCELL - bool "tmiofb acceleration" - depends on FB_TMIO - default y - config FB_S3C tristate "Samsung S3C framebuffer support" depends on FB && HAVE_CLK && HAS_IOMEM diff --git a/drivers/video/fbdev/Makefile b/drivers/video/fbdev/Makefile index 1bb870b98848..e5206c3331d6 100644 --- a/drivers/video/fbdev/Makefile +++ b/drivers/video/fbdev/Makefile @@ -85,7 +85,6 @@ obj-$(CONFIG_FB_PXA168) += pxa168fb.o obj-$(CONFIG_PXA3XX_GCU)+= pxa3xx-gcu.o obj-$(CONFIG_MMP_DISP) += mmp/ obj-$(CONFIG_FB_W100) += w100fb.o -obj-$(CONFIG_FB_TMIO)+= tmiofb.o obj-$(CONFIG_FB_AU1100) += au1100fb.o obj-$(CONFIG_FB_AU1200) += au1200fb.o obj-$(CONFIG_FB_VT8500) += vt8500lcdfb.o diff --git a/drivers/video/fbdev/tmiofb.c b/drivers/video/fbdev/tmiofb.c deleted file mode 100644 index 50111966c981..
Re: [PATCH] drm/vkms: Add a DRM render node to vkms
On Thu, Jan 05, 2023 at 01:40:28PM -0800, Tao Wu(吴涛@Eng) wrote: > Hi Daniel, > > May I know what's the requirement for adding render node support to a > "gpu"? Why we just export render node for every drm devices? > I read document here > https://www.kernel.org/doc/html/v4.8/gpu/drm-uapi.html#render-nodes Thus far we've only done it when there's actual rendering capability, which generally means at least some private ioctls. Which vkms just doens't have. And it's by far not the only such case. Also note that display drivers side is _not_ shareable. -Daniel > and it seems render node allow multiple unprivileged clients > to work with the same gpu, I am not sure why we just enable it for all > kms-only device. > What's wrong if we enable it for all kms-only devices and also let > mesa to use llvmpipe with those devices by default. > > Thanks! > > On Thu, Jan 5, 2023 at 7:35 AM Daniel Vetter wrote: > > > > On Fri, Jan 06, 2023 at 12:16:07AM +0900, Yi Xie wrote: > > > On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter wrote: > > > > > > > > On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote: > > > > > On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter wrote: > > > > > > > > > > > > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote: > > > > > > > > This doesn't sound like a good idea to me. Devices without > > > > > > > > render > > > > > > > > capabilities should not fake it. > > > > > > > > > > > > > > > > User-space (e.g. wlroots) relies on "no render node" to enable > > > > > > > > software rendering (Pixman instead of GL). > > > > > > > > > > > > > > We have virtgpu driver that exports a render node even when virgl > > > > > > > is > > > > > > > not supported. > > > > > > > Mesa has special code path to enable software rendering on it: > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296 > > > > > > > We can do the same for vkms to force software rendering. > > > > > > > > > > > > Yeah that is the old kmsro mesa issue, for every combination of kms > > > > > > and > > > > > > gem device you need one to make this work. > > > > > > > > > > > > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter > > > > > > > wrote: > > > > > > > > > > > > > > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote: > > > > > > > > > Some libraries including Mesa and virglrenderer require a > > > > > > > > > render node to > > > > > > > > > fully function. By adding a render node to vkms those > > > > > > > > > libraries will > > > > > > > > > work properly, supporting use cases like running crosvm with > > > > > > > > > virgl GPU > > > > > > > > > support via llvmpipe on a headless virtual machine. > > > > > > > > > > > > > > > > This is what vgem exists for. More or less at least ... I'm > > > > > > > > honestly not > > > > > > > > really understanding what you're trying to fix here, it sounds > > > > > > > > a bit like > > > > > > > > userspace being stupid. > > > > > > > > -Daniel > > > > > > > The problem with vgem is that it crashes llvmpipe while working > > > > > > > with vkms. > > > > > > > Looks like it's due to the same reason as described in this > > > > > > > thread in Mesa: > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830 > > > > > > > > > > > > I'm not finding any bug description in there and how/why something > > > > > > crashes? > > > > > > > > > > The discussion is in the comment section under the first comment by > > > > > Emil Velikov. > > > > > It's folded by default (inside "6 replies" at the bottom). > > > > > > > > > > > > > > > > > > Importing buffers allocated by vgem to vkms seems to be > > > > > > > unexpected and > > > > > > > causes the crash. If we create a render node on vkms then > > > > > > > llvmpipe will use > > > > > > > vkms to allocate buffers and it no longer crashes. > > > > > > > > > > > > Uh importing vgem into virtio might not work because those > > > > > > sometimes need > > > > > > special buffers iirc. But importing vgem into vkms really should > > > > > > work, > > > > > > there's no technical reason it cannot. If it doesn't, then the > > > > > > right fix > > > > > > would be to fix that, not paper around it. > > > > > > > > > > The crash stack trace looks like this: > > > > > https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2 > > > > > > > > > > Even if we fix the crash issue with vgem, we still need to workaround > > > > > quite a few > > > > > places that has explicitly blocked vgem. A notable example is > > > > > virglrenderer: > > > > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121 > > > > > > > > > > Actually I have tried to force running virglrenderer on vgem and it > > > > > didn't work. I > > > > > didn't look into why it wasn't working but I guess that's the reason > > > > > for blocking > > > > > vgem in the first place. Virglrenderer works well on vkms with render > > > > > node > > > > > enabled
Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
On Fri, Jan 06, 2023 at 10:28:15AM +0100, Daniel Vetter wrote: > On Thu, Jan 05, 2023 at 07:38:26PM +0200, Oded Gabbay wrote: > > On Thu, Jan 5, 2023 at 6:25 PM Jeffrey Hugo wrote: > > > > > > On 1/5/2023 5:57 AM, Daniel Vetter wrote: > > > > On Thu, Dec 08, 2022 at 12:07:27PM +0100, Jacek Lawrynowicz wrote: > > > >> +static const struct drm_driver driver = { > > > >> +.driver_features = DRIVER_GEM | DRIVER_COMPUTE_ACCEL, > > > > > > > > So I was wondering whether this is a bright idea, and whether we > > > > shouldn't > > > > just go ahead and infuse more meaning into accel vs render nodes. > > > > > > > > The uapi relevant part of render nodes is that they're multi-user safe, > > > > at > > > > least as much as feasible. Every new open() gives you a new private > > > > accelerator. This also has implications on how userspace drivers iterate > > > > them, they just open them all in turn and check whether it's the right > > > > one - because userspace apis allow applications to enumerate them all. > > > > Which also means that any devicie initialization at open() time is a > > > > really bad idea. > > > > > > > > A lot of the compute accelerators otoh (well habanalabs) are single > > > > user, > > > > init can be done at open() time because you only open this when you > > > > actually know you're going to use it. > > > > > > > > So given this, shouldn't multi-user inference engines be more like > > > > render > > > > drivers, and less like accel? So DRIVER_RENDER, but still under > > > > drivers/accel. > > > > > > > > This way that entire separate /dev node would actually become meaningful > > > > beyond just the basic bikeshed: > > > > - render nodes are multi user, safe to iterate and open() just for > > > >iteration > > > > - accel nodes are single user, you really should not ever open them > > > > unless > > > >you want to use them > > > > > > > > Of course would need a doc patch :-) > > > > > > > > Thoughts? > > > > -Daniel > > > > > > Hmm. > > > > > > I admit, I thought DRIVER_ACCEL was the same as DRIVER_RENDER, except > > > that DRIVER_ACCEL dropped the "legacy" dual node setup and also avoided > > > "legacy" userspace. > > > > > > qaic is multi-user. I thought habana was the same, at-least for > > > inference. Oded, am I wrong? > > Habana's devices support a single user at a time acquiring the device > > and working on it. > > Both for training and inference. > > > > > > So, if DRIVER_ACCEL is for single-user (training?), and multi-user ends > > > up in DRIVER_RENDER, that would seem to mean qaic ends up using > > > DRIVER_RENDER and not DRIVER_ACCEL. Then qaic ends up over under > > > /dev/dri with both a card node (never used) and a render node. That > > > would seem to mean that the "legacy" userspace would open qaic nodes by > > > default - something I understood Oded was trying to avoid. > > > > > > If there really a usecase for DRIVER_ACCEL to support single-user? I > > > wonder why we can't default to multi-user, and if a particular > > > user/driver has a single-user usecase, it enforces that in a driver > > > specific manner? > > > > > > -Jeff > > > > Honestly, Daniel, I don't like this suggestion. I don't understand why > > you make a connection between render/accel to single/multi user. > > > > As Jeff has said, one of the goals was to expose accelerator devices > > to userspace with new device char nodes so we won't be bothered by > > legacy userspace graphics software. This is something we all agreed on > > and I don't see why we should change it now, even if you think it's > > bike-shedding (which I disagree with). > > > > But in any case, creating a new device char nodes had nothing to do > > with whether the device supports single or multi user. I can > > definitely see in the future training devices that support multiple > > users. > > > > The common drm/accel ioctls should of course not be limited to a > > single user, and I agree with Jeff here, if a specific driver has such > > a limitation (e.g. Habana), then that driver should handle it on its > > own. > > Maybe if there will be multiple drivers with such a limitation, we can > > make that "handling" to be common code. > > > > Bottom line, I prefer we keep things as we all agreed upon in LPC. > > The problem is going to happen as soon as you have cross-vendor userspace. > Which I'm kinda hoping is at least still the aspiration. Because with > cross-vendor userspace you generally iterate & open all devices before you > select the one you're going to use. And so we do kinda need a distinction, > or we need that the single-user drivers also guarantee that open() is > cheap. FWIW we had good support in ivpu for probe open's in form of lazy context allocation. It was removed recently due to review feedback that this is unnecessary, but we can add it back. Regards Stanislaw
[PATCH] drm/radeon: free iio for atombios when driver shutdown
Fix below kmemleak when unload radeon driver: unreferenced object 0x9f8608ede200 (size 512): comm "systemd-udevd", pid 326, jiffies 4294682822 (age 716.338s) hex dump (first 32 bytes): 00 00 00 00 c4 aa ec aa 14 ab 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 backtrace: [<62fadebe>] kmem_cache_alloc_trace+0x2f1/0x500 [] atom_parse+0x117/0x230 [radeon] [<158c23fd>] radeon_atombios_init+0xab/0x170 [radeon] [<683f672e>] si_init+0x57/0x750 [radeon] [<566cc31f>] radeon_device_init+0x559/0x9c0 [radeon] [<46efabb3>] radeon_driver_load_kms+0xc1/0x1a0 [radeon] [ ] drm_dev_register+0xdd/0x1d0 [<45fec835>] radeon_pci_probe+0xbd/0x100 [radeon] [ ] pci_device_probe+0xe1/0x160 [<19484b76>] really_probe.part.0+0xc1/0x2c0 [<3f2649da>] __driver_probe_device+0x96/0x130 [<231c5bb1>] driver_probe_device+0x24/0xf0 [<00a42377>] __driver_attach+0x77/0x190 [ ] bus_for_each_dev+0x7f/0xd0 [<633166d2>] driver_attach+0x1e/0x30 [<313b05b8>] bus_add_driver+0x12c/0x1e0 iio was allocated in atom_index_iio() called by atom_parse(), but it doesn't got released when the dirver is shutdown. Fix this kmemleak by free it in radeon_atombios_fini(). Signed-off-by: Liwei Song --- drivers/gpu/drm/radeon/radeon_device.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 92905ebb7b45..1c005e0ddd38 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -1022,6 +1022,7 @@ void radeon_atombios_fini(struct radeon_device *rdev) { if (rdev->mode_info.atom_context) { kfree(rdev->mode_info.atom_context->scratch); + kfree(rdev->mode_info.atom_context->iio); } kfree(rdev->mode_info.atom_context); rdev->mode_info.atom_context = NULL; -- 2.33.1
Re: [PATCH] drm/vkms: Add a DRM render node to vkms
I have figured out the problem with importing buffers across vgem/vkms. It's intentionally blocked by kernel here: https://github.com/torvalds/linux/blob/a140a6a2d5ec0329ad05cd3532a91ad0ce58dceb/drivers/gpu/drm/drm_gem.c#L307 >From the original patch https://patchwork.freedesktop.org/patch/172242/: Reject mapping an imported dma-buf since it's an invalid use-case. Looks like importing dumb buffers across different devices is disallowed. Removing this check and then everything is working well on vkms with vgem. According to the patch thread we should use native map instead of dumb map on imported buffers. Since there is no native map ioctl in both vgem and vkms, I'm thinking about adding a dumb_map_offset implementation in both of them with that check removed. >From my testing vkms and vgem are now working happily together without any (obvious) issues. There are other drivers doing the same thing, for example virtgpu: https://github.com/torvalds/linux/blob/a140a6a2d5ec0329ad05cd3532a91ad0ce58dceb/drivers/gpu/drm/virtio/virtgpu_gem.c#L102 Does this sound like a better idea than adding a render node? On Fri, Jan 6, 2023 at 6:54 PM Daniel Vetter wrote: > > On Thu, Jan 05, 2023 at 01:40:28PM -0800, Tao Wu(吴涛@Eng) wrote: > > Hi Daniel, > > > > May I know what's the requirement for adding render node support to a > > "gpu"? Why we just export render node for every drm devices? > > I read document here > > https://www.kernel.org/doc/html/v4.8/gpu/drm-uapi.html#render-nodes > > Thus far we've only done it when there's actual rendering capability, > which generally means at least some private ioctls. > > Which vkms just doens't have. And it's by far not the only such case. > > Also note that display drivers side is _not_ shareable. > -Daniel > > > and it seems render node allow multiple unprivileged clients > > to work with the same gpu, I am not sure why we just enable it for all > > kms-only device. > > What's wrong if we enable it for all kms-only devices and also let > > mesa to use llvmpipe with those devices by default. > > > > Thanks! > > > > On Thu, Jan 5, 2023 at 7:35 AM Daniel Vetter wrote: > > > > > > On Fri, Jan 06, 2023 at 12:16:07AM +0900, Yi Xie wrote: > > > > On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter wrote: > > > > > > > > > > On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote: > > > > > > On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter > > > > > > wrote: > > > > > > > > > > > > > > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote: > > > > > > > > > This doesn't sound like a good idea to me. Devices without > > > > > > > > > render > > > > > > > > > capabilities should not fake it. > > > > > > > > > > > > > > > > > > User-space (e.g. wlroots) relies on "no render node" to enable > > > > > > > > > software rendering (Pixman instead of GL). > > > > > > > > > > > > > > > > We have virtgpu driver that exports a render node even when > > > > > > > > virgl is > > > > > > > > not supported. > > > > > > > > Mesa has special code path to enable software rendering on it: > > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296 > > > > > > > > We can do the same for vkms to force software rendering. > > > > > > > > > > > > > > Yeah that is the old kmsro mesa issue, for every combination of > > > > > > > kms and > > > > > > > gem device you need one to make this work. > > > > > > > > > > > > > > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote: > > > > > > > > > > Some libraries including Mesa and virglrenderer require a > > > > > > > > > > render node to > > > > > > > > > > fully function. By adding a render node to vkms those > > > > > > > > > > libraries will > > > > > > > > > > work properly, supporting use cases like running crosvm > > > > > > > > > > with virgl GPU > > > > > > > > > > support via llvmpipe on a headless virtual machine. > > > > > > > > > > > > > > > > > > This is what vgem exists for. More or less at least ... I'm > > > > > > > > > honestly not > > > > > > > > > really understanding what you're trying to fix here, it > > > > > > > > > sounds a bit like > > > > > > > > > userspace being stupid. > > > > > > > > > -Daniel > > > > > > > > The problem with vgem is that it crashes llvmpipe while working > > > > > > > > with vkms. > > > > > > > > Looks like it's due to the same reason as described in this > > > > > > > > thread in Mesa: > > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830 > > > > > > > > > > > > > > I'm not finding any bug description in there and how/why something > > > > > > > crashes? > > > > > > > > > > > > The discussion is in the comment section under the first comment by > > > > > > Emil Velikov. > > > > > > It's folded by default (inside "6 replies" at the bottom). > > > > > > > > > > > > > > > > > > > > > Importing buffers allocated by vgem to
[PATCH v2] drm/fb-helper: Replace bpp/depth parameter by color mode
Replace the combination of bpp and depth with a single color-mode argument. Handle special cases in simpledrm and ofdrm. Hard-code XRGB as fallback format for cases where no given format works. The color-mode argument accepts the same values as the kernel's video parameter. These are mostly bpp values between 1 and 32. The exceptions are 15, which has a color depth of 15 and a bpp value of 16; and 32, which has a color depth of 24 and a bpp value of 32. v2: * minimize changes (Daniel) * use drm_driver_legacy_fb_format() (Daniel) Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_fb_helper.c | 39 +--- drivers/gpu/drm/tiny/ofdrm.c | 7 +- drivers/gpu/drm/tiny/simpledrm.c | 7 +- 3 files changed, 33 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1369ca4ae39b..2feb4b0a1477 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1756,24 +1756,21 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const return DRM_FORMAT_INVALID; } -static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helper, - const uint32_t *formats, size_t format_count, - const struct drm_cmdline_mode *cmdline_mode) +static uint32_t drm_fb_helper_find_color_mode_format(struct drm_fb_helper *fb_helper, +const uint32_t *formats, size_t format_count, +unsigned int color_mode) { struct drm_device *dev = fb_helper->dev; uint32_t bpp, depth; - if (!cmdline_mode->bpp_specified) - return DRM_FORMAT_INVALID; - - switch (cmdline_mode->bpp) { + switch (color_mode) { case 1: case 2: case 4: case 8: case 16: case 24: - bpp = depth = cmdline_mode->bpp; + bpp = depth = color_mode; break; case 15: bpp = 16; @@ -1784,7 +1781,7 @@ static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helpe depth = 24; break; default: - drm_info(dev, "unsupported bpp value of %d\n", cmdline_mode->bpp); + drm_info(dev, "unsupported color mode of %d\n", color_mode); return DRM_FORMAT_INVALID; } @@ -1817,10 +1814,10 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe drm_client_for_each_connector_iter(connector, &conn_iter) { struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode; - surface_format = drm_fb_helper_find_cmdline_format(fb_helper, - plane->format_types, - plane->format_count, - cmdline_mode); + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + cmdline_mode->bpp); if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ } @@ -1829,17 +1826,23 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ - /* try preferred bpp/depth */ - surface_format = drm_fb_helper_find_format(fb_helper, plane->format_types, - plane->format_count, preferred_bpp, - dev->mode_config.preferred_depth); + /* try preferred color mode */ + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + preferred_bpp); if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ } if (surface_format == DRM_FORMAT_INVALID) { + /* +* If none of the given color modes works, fall back +
Re: [PATCH] drm/i915: Fix potential context UAFs
On 05/01/2023 16:00, Tvrtko Ursulin wrote: On 05/01/2023 15:52, Andi Shyti wrote: Hi Rob, On Tue, Jan 03, 2023 at 03:49:46PM -0800, Rob Clark wrote: From: Rob Clark gem_context_register() makes the context visible to userspace, and which point a separate thread can trigger the I915_GEM_CONTEXT_DESTROY ioctl. So we need to ensure that nothing uses the ctx ptr after this. And we need to ensure that adding the ctx to the xarray is the *last* thing that gem_context_register() does with the ctx pointer. Signed-off-by: Rob Clark Reviewed-by: Andi Shyti I also agree with Tvrtko that we should add Stable: and Fixes:. Yeah I'll add them all when merging. Just waiting for full CI results. It will be like this: Fixes: eb4dedae920a ("drm/i915/gem: Delay tracking the GEM context until it is registered") Fixes: a4c1cdd34e2c ("drm/i915/gem: Delay context creation (v3)") Fixes: 49bd54b390c2 ("drm/i915: Track all user contexts per client") Cc: # v5.10+ Pushed to drm-intel-gt-next - thanks for the fix and reviews. Regards, Tvrtko Regards, Tvrtko One little thing, "user after free" is clearer that UAF :) Thanks, Andi --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 24 +++-- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 7f2831efc798..6250de9b9196 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1688,6 +1688,10 @@ void i915_gem_init__contexts(struct drm_i915_private *i915) init_contexts(&i915->gem.contexts); } +/* + * Note that this implicitly consumes the ctx reference, by placing + * the ctx in the context_xa. + */ static void gem_context_register(struct i915_gem_context *ctx, struct drm_i915_file_private *fpriv, u32 id) @@ -1703,10 +1707,6 @@ static void gem_context_register(struct i915_gem_context *ctx, snprintf(ctx->name, sizeof(ctx->name), "%s[%d]", current->comm, pid_nr(ctx->pid)); - /* And finally expose ourselves to userspace via the idr */ - old = xa_store(&fpriv->context_xa, id, ctx, GFP_KERNEL); - WARN_ON(old); - spin_lock(&ctx->client->ctx_lock); list_add_tail_rcu(&ctx->client_link, &ctx->client->ctx_list); spin_unlock(&ctx->client->ctx_lock); @@ -1714,6 +1714,10 @@ static void gem_context_register(struct i915_gem_context *ctx, spin_lock(&i915->gem.contexts.lock); list_add_tail(&ctx->link, &i915->gem.contexts.list); spin_unlock(&i915->gem.contexts.lock); + + /* And finally expose ourselves to userspace via the idr */ + old = xa_store(&fpriv->context_xa, id, ctx, GFP_KERNEL); + WARN_ON(old); } int i915_gem_context_open(struct drm_i915_private *i915, @@ -2199,14 +2203,22 @@ finalize_create_context_locked(struct drm_i915_file_private *file_priv, if (IS_ERR(ctx)) return ctx; + /* + * One for the xarray and one for the caller. We need to grab + * the reference *prior* to making the ctx visble to userspace + * in gem_context_register(), as at any point after that + * userspace can try to race us with another thread destroying + * the context under our feet. + */ + i915_gem_context_get(ctx); + gem_context_register(ctx, file_priv, id); old = xa_erase(&file_priv->proto_context_xa, id); GEM_BUG_ON(old != pc); proto_context_close(file_priv->dev_priv, pc); - /* One for the xarray and one for the caller */ - return i915_gem_context_get(ctx); + return ctx; } struct i915_gem_context * -- 2.38.1
[PATCH v3] drm/fb-helper: Replace bpp/depth parameter by color mode
Replace the combination of bpp and depth with a single color-mode argument. Handle special cases in simpledrm and ofdrm. Hard-code XRGB as fallback format for cases where no given format works. The color-mode argument accepts the same values as the kernel's video parameter. These are mostly bpp values between 1 and 32. The exceptions are 15, which has a color depth of 15 and a bpp value of 16; and 32, which has a color depth of 24 and a bpp value of 32. v3: * fix ofdrm build (Maxime) v2: * minimize changes (Daniel) * use drm_driver_legacy_fb_format() (Daniel) Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_fb_helper.c | 39 +--- drivers/gpu/drm/tiny/ofdrm.c | 7 +- drivers/gpu/drm/tiny/simpledrm.c | 7 +- 3 files changed, 33 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1369ca4ae39b..2feb4b0a1477 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1756,24 +1756,21 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const return DRM_FORMAT_INVALID; } -static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helper, - const uint32_t *formats, size_t format_count, - const struct drm_cmdline_mode *cmdline_mode) +static uint32_t drm_fb_helper_find_color_mode_format(struct drm_fb_helper *fb_helper, +const uint32_t *formats, size_t format_count, +unsigned int color_mode) { struct drm_device *dev = fb_helper->dev; uint32_t bpp, depth; - if (!cmdline_mode->bpp_specified) - return DRM_FORMAT_INVALID; - - switch (cmdline_mode->bpp) { + switch (color_mode) { case 1: case 2: case 4: case 8: case 16: case 24: - bpp = depth = cmdline_mode->bpp; + bpp = depth = color_mode; break; case 15: bpp = 16; @@ -1784,7 +1781,7 @@ static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helpe depth = 24; break; default: - drm_info(dev, "unsupported bpp value of %d\n", cmdline_mode->bpp); + drm_info(dev, "unsupported color mode of %d\n", color_mode); return DRM_FORMAT_INVALID; } @@ -1817,10 +1814,10 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe drm_client_for_each_connector_iter(connector, &conn_iter) { struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode; - surface_format = drm_fb_helper_find_cmdline_format(fb_helper, - plane->format_types, - plane->format_count, - cmdline_mode); + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + cmdline_mode->bpp); if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ } @@ -1829,17 +1826,23 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ - /* try preferred bpp/depth */ - surface_format = drm_fb_helper_find_format(fb_helper, plane->format_types, - plane->format_count, preferred_bpp, - dev->mode_config.preferred_depth); + /* try preferred color mode */ + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + preferred_bpp); if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ } if (surface_format == DRM_FORMAT_INVALID) { + /* +* If none of the given
Re: [PATCH] drm/i915/hwmon: Display clamped PL1 limit
On 12/16/2022 12:47 AM, Ashutosh Dixit wrote: HW allows arbitrary PL1 limits to be set but silently clamps these values to "typical but not guaranteed" min/max values in pkg_power_sku register. Follow the same pattern for sysfs, allow arbitrary PL1 limits to be set but display clamped values when read, so that users see PL1 limits HW is likely using. Otherwise users think HW is using arbitrarily high/low PL1 limits they might have set. The previous write/read I1 power1_crit limit also follows the same clamping pattern. v2: Explain "why" in commit message and include bug link (Jani Nikula) Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7704 Signed-off-by: Ashutosh Dixit Looks good to me. Reviewed-by: Anshuman Gupta --- drivers/gpu/drm/i915/i915_hwmon.c| 39 drivers/gpu/drm/i915/intel_mchbar_regs.h | 2 ++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index cca7a4350ec8f..1225bc432f0d5 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -359,6 +359,38 @@ hwm_power_is_visible(const struct hwm_drvdata *ddat, u32 attr, int chan) } } +/* + * HW allows arbitrary PL1 limits to be set but silently clamps these values to + * "typical but not guaranteed" min/max values in rg.pkg_power_sku. Follow the + * same pattern for sysfs, allow arbitrary PL1 limits to be set but display + * clamped values when read. Write/read I1 also follows the same pattern. + */ +static int +hwm_power_max_read(struct hwm_drvdata *ddat, long *val) +{ + struct i915_hwmon *hwmon = ddat->hwmon; + intel_wakeref_t wakeref; + u64 r, min, max; + + *val = hwm_field_read_and_scale(ddat, + hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1, + hwmon->scl_shift_power, + SF_POWER); + + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) + r = intel_uncore_read64(ddat->uncore, hwmon->rg.pkg_power_sku); + min = REG_FIELD_GET(PKG_MIN_PWR, r); + min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power); + max = REG_FIELD_GET(PKG_MAX_PWR, r); + max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power); + + if (min && max) + *val = clamp_t(u64, *val, min, max); + + return 0; +} + static int hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val) { @@ -368,12 +400,7 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val) switch (attr) { case hwmon_power_max: - *val = hwm_field_read_and_scale(ddat, - hwmon->rg.pkg_rapl_limit, - PKG_PWR_LIM_1, - hwmon->scl_shift_power, - SF_POWER); - return 0; + return hwm_power_max_read(ddat, val); case hwmon_power_rated_max: *val = hwm_field_read_and_scale(ddat, hwmon->rg.pkg_power_sku, diff --git a/drivers/gpu/drm/i915/intel_mchbar_regs.h b/drivers/gpu/drm/i915/intel_mchbar_regs.h index f93e9af43ac35..73900c098d591 100644 --- a/drivers/gpu/drm/i915/intel_mchbar_regs.h +++ b/drivers/gpu/drm/i915/intel_mchbar_regs.h @@ -194,6 +194,8 @@ */ #define PCU_PACKAGE_POWER_SKU _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5930) #define PKG_PKG_TDP GENMASK_ULL(14, 0) +#define PKG_MIN_PWR GENMASK_ULL(30, 16) +#define PKG_MAX_PWR GENMASK_ULL(46, 32) #define PKG_MAX_WIN GENMASK_ULL(54, 48) #define PKG_MAX_WIN_X GENMASK_ULL(54, 53) #define PKG_MAX_WIN_Y GENMASK_ULL(52, 48)
Re: [PATCH v3] drm/fb-helper: Replace bpp/depth parameter by color mode
cc'ing linux-arm-...@vger.kernel.org Supposed to fix the recent console issues. [1] Please test, if possible. Best regards Thomas [1] https://lore.kernel.org/dri-devel/Y7fj1N4blo2MYZDt@phenom.ffwll.local/T/#t Am 06.01.23 um 11:16 schrieb Thomas Zimmermann: Replace the combination of bpp and depth with a single color-mode argument. Handle special cases in simpledrm and ofdrm. Hard-code XRGB as fallback format for cases where no given format works. The color-mode argument accepts the same values as the kernel's video parameter. These are mostly bpp values between 1 and 32. The exceptions are 15, which has a color depth of 15 and a bpp value of 16; and 32, which has a color depth of 24 and a bpp value of 32. v3: * fix ofdrm build (Maxime) v2: * minimize changes (Daniel) * use drm_driver_legacy_fb_format() (Daniel) Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_fb_helper.c | 39 +--- drivers/gpu/drm/tiny/ofdrm.c | 7 +- drivers/gpu/drm/tiny/simpledrm.c | 7 +- 3 files changed, 33 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1369ca4ae39b..2feb4b0a1477 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1756,24 +1756,21 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const return DRM_FORMAT_INVALID; } -static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helper, - const uint32_t *formats, size_t format_count, - const struct drm_cmdline_mode *cmdline_mode) +static uint32_t drm_fb_helper_find_color_mode_format(struct drm_fb_helper *fb_helper, +const uint32_t *formats, size_t format_count, +unsigned int color_mode) { struct drm_device *dev = fb_helper->dev; uint32_t bpp, depth; - if (!cmdline_mode->bpp_specified) - return DRM_FORMAT_INVALID; - - switch (cmdline_mode->bpp) { + switch (color_mode) { case 1: case 2: case 4: case 8: case 16: case 24: - bpp = depth = cmdline_mode->bpp; + bpp = depth = color_mode; break; case 15: bpp = 16; @@ -1784,7 +1781,7 @@ static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helpe depth = 24; break; default: - drm_info(dev, "unsupported bpp value of %d\n", cmdline_mode->bpp); + drm_info(dev, "unsupported color mode of %d\n", color_mode); return DRM_FORMAT_INVALID; } @@ -1817,10 +1814,10 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe drm_client_for_each_connector_iter(connector, &conn_iter) { struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode; - surface_format = drm_fb_helper_find_cmdline_format(fb_helper, - plane->format_types, - plane->format_count, - cmdline_mode); + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + cmdline_mode->bpp); if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ } @@ -1829,17 +1826,23 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ - /* try preferred bpp/depth */ - surface_format = drm_fb_helper_find_format(fb_helper, plane->format_types, - plane->format_count, preferred_bpp, - dev->mode_config.preferred_depth); + /* try preferred color mode */ + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + preferred_bpp);
Re: [RFC 3/3] drm: Update file owner during use
Am 05.01.23 um 13:32 schrieb Daniel Vetter: [SNIP] For the case of an master fd I actually don't see the reason why we should limit that? And fd can become master if it either was master before or has CAP_SYS_ADMIN. Why would we want an extra check for the pid/tgid here? This is just info/debug printing, I don't see the connection to drm_auth/master stuff? Aside from the patch mixes up the master opener and the current user due to fd passing or something like that. That's exactly why my comment meant as well. The connect is that the drm_auth/master code currently the pid/tgid as indicator if the "owner" of the fd has changed and so if an access should be allowed or not. I find that approach a bit questionable. Note that we cannot do that (I think at least, after pondering this some more) because it would break the logind master fd passing scheme - there the receiving compositor is explicitly _not_ allowed to acquire master rights on its own. So the master priviledges must not move with the fd or things can go wrong. That could be the rational behind that, but why doesn't logind then just pass on a normal render node to the compositor? Christian. -Daniel Regards, Christian. Regards, Tvrtko -Daniel return 0; if (!capable(CAP_SYS_ADMIN)) diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index 42f657772025..9d4e3146a2b8 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -90,15 +90,17 @@ static int drm_clients_info(struct seq_file *m, void *data) */ mutex_lock(&dev->filelist_mutex); list_for_each_entry_reverse(priv, &dev->filelist, lhead) { - struct task_struct *task; bool is_current_master = drm_is_current_master(priv); + struct task_struct *task; + struct pid *pid; - rcu_read_lock(); /* locks pid_task()->comm */ - task = pid_task(priv->pid, PIDTYPE_TGID); + rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */ + pid = rcu_dereference(priv->pid); + task = pid_task(pid, PIDTYPE_TGID); uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID; seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", task ? task->comm : "", - pid_vnr(priv->pid), + pid_vnr(pid), priv->minor->index, is_current_master ? 'y' : 'n', priv->authenticated ? 'y' : 'n', diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 20a9aef2b398..3433f9610dba 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -156,7 +156,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor) if (!file) return ERR_PTR(-ENOMEM); - file->pid = get_pid(task_tgid(current)); + rcu_assign_pointer(file->pid, get_pid(task_tgid(current))); file->minor = minor; /* for compatibility root is always authenticated */ @@ -502,6 +502,36 @@ int drm_release(struct inode *inode, struct file *filp) } EXPORT_SYMBOL(drm_release); +void drm_file_update_pid(struct drm_file *filp) +{ + struct drm_device *dev; + struct pid *pid, *old; + + /* Master nodes are not expected to be passed between processes. */ + if (filp->was_master) + return; + + pid = task_tgid(current); + + /* + * Quick unlocked check since the model is a single handover followed by + * exclusive repeated use. + */ + if (pid == rcu_access_pointer(filp->pid)) + return; + + dev = filp->minor->dev; + mutex_lock(&dev->filelist_mutex); + old = rcu_replace_pointer(filp->pid, pid, 1); + mutex_unlock(&dev->filelist_mutex); + + if (pid != old) { + get_pid(pid); + synchronize_rcu(); + put_pid(old); + } +} + /** * drm_release_noglobal - release method for DRM file * @inode: device inode diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 7c9d66ee917d..305b18d9d7b6 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -775,6 +775,9 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, struct drm_device *dev = file_priv->minor->dev; int retcode; + /* Update drm_file owner if fd was passed along. */ + drm_file_update_pid(file_priv); + if (drm_dev_is_unplugged(dev)) return -ENODEV; diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 80f154b6adab..a763d3ee61fb 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -1097,7 +1097,10 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file *fpriv) } get_task_comm(tmpname, current); - snprintf(name, sizeof(name), "%s[%d]", tmpname, pid_nr(fpriv->pid)); + rcu_read_lock(); + snprintf(name, sizeof(name), "%s[%d]", + tmpname, pid_nr(rcu_dereference(fpriv-
[PATCH v2] drm/i915: Do not cover all future platforms in TLB invalidation
From: Tvrtko Ursulin Revert to the original explicit approach and document the reasoning behind it. v2: * DG2 needs to be covered too. (Matt) Signed-off-by: Tvrtko Ursulin Cc: Matt Roper Cc: Balasubramani Vivekanandan Cc: Andrzej Hajda Reviewed-by: Andrzej Hajda # v1 --- Matt, does DG1 need to be in the MCR branch or plain Gen12? --- drivers/gpu/drm/i915/gt/intel_gt.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 75a7cb33..b2556a3d8a3f 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -1070,7 +1070,19 @@ static void mmio_invalidate_full(struct intel_gt *gt) unsigned int num = 0; unsigned long flags; - if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) { + /* +* New platforms should not be added with catch-all-newer (>=) +* condition so that any later platform added triggers the below warning +* and in turn mandates a human cross-check of whether the invalidation +* flows have compatible semantics. +* +* For instance with the 11.00 -> 12.00 transition three out of five +* respective engine registers were moved to masked type. Then after the +* 12.00 -> 12.50 transition multi cast handling is required too. +*/ + + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) && + GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) { regs = NULL; num = ARRAY_SIZE(xehp_regs); } else if (GRAPHICS_VER(i915) == 12) { -- 2.34.1
Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
On Fri, 6 Jan 2023 at 10:56, Stanislaw Gruszka wrote: > > On Fri, Jan 06, 2023 at 10:28:15AM +0100, Daniel Vetter wrote: > > On Thu, Jan 05, 2023 at 07:38:26PM +0200, Oded Gabbay wrote: > > > On Thu, Jan 5, 2023 at 6:25 PM Jeffrey Hugo > > > wrote: > > > > > > > > On 1/5/2023 5:57 AM, Daniel Vetter wrote: > > > > > On Thu, Dec 08, 2022 at 12:07:27PM +0100, Jacek Lawrynowicz wrote: > > > > >> +static const struct drm_driver driver = { > > > > >> +.driver_features = DRIVER_GEM | DRIVER_COMPUTE_ACCEL, > > > > > > > > > > So I was wondering whether this is a bright idea, and whether we > > > > > shouldn't > > > > > just go ahead and infuse more meaning into accel vs render nodes. > > > > > > > > > > The uapi relevant part of render nodes is that they're multi-user > > > > > safe, at > > > > > least as much as feasible. Every new open() gives you a new private > > > > > accelerator. This also has implications on how userspace drivers > > > > > iterate > > > > > them, they just open them all in turn and check whether it's the right > > > > > one - because userspace apis allow applications to enumerate them all. > > > > > Which also means that any devicie initialization at open() time is a > > > > > really bad idea. > > > > > > > > > > A lot of the compute accelerators otoh (well habanalabs) are single > > > > > user, > > > > > init can be done at open() time because you only open this when you > > > > > actually know you're going to use it. > > > > > > > > > > So given this, shouldn't multi-user inference engines be more like > > > > > render > > > > > drivers, and less like accel? So DRIVER_RENDER, but still under > > > > > drivers/accel. > > > > > > > > > > This way that entire separate /dev node would actually become > > > > > meaningful > > > > > beyond just the basic bikeshed: > > > > > - render nodes are multi user, safe to iterate and open() just for > > > > >iteration > > > > > - accel nodes are single user, you really should not ever open them > > > > > unless > > > > >you want to use them > > > > > > > > > > Of course would need a doc patch :-) > > > > > > > > > > Thoughts? > > > > > -Daniel > > > > > > > > Hmm. > > > > > > > > I admit, I thought DRIVER_ACCEL was the same as DRIVER_RENDER, except > > > > that DRIVER_ACCEL dropped the "legacy" dual node setup and also avoided > > > > "legacy" userspace. > > > > > > > > qaic is multi-user. I thought habana was the same, at-least for > > > > inference. Oded, am I wrong? > > > Habana's devices support a single user at a time acquiring the device > > > and working on it. > > > Both for training and inference. > > > > > > > > So, if DRIVER_ACCEL is for single-user (training?), and multi-user ends > > > > up in DRIVER_RENDER, that would seem to mean qaic ends up using > > > > DRIVER_RENDER and not DRIVER_ACCEL. Then qaic ends up over under > > > > /dev/dri with both a card node (never used) and a render node. That > > > > would seem to mean that the "legacy" userspace would open qaic nodes by > > > > default - something I understood Oded was trying to avoid. > > > > > > > > If there really a usecase for DRIVER_ACCEL to support single-user? I > > > > wonder why we can't default to multi-user, and if a particular > > > > user/driver has a single-user usecase, it enforces that in a driver > > > > specific manner? > > > > > > > > -Jeff > > > > > > Honestly, Daniel, I don't like this suggestion. I don't understand why > > > you make a connection between render/accel to single/multi user. > > > > > > As Jeff has said, one of the goals was to expose accelerator devices > > > to userspace with new device char nodes so we won't be bothered by > > > legacy userspace graphics software. This is something we all agreed on > > > and I don't see why we should change it now, even if you think it's > > > bike-shedding (which I disagree with). > > > > > > But in any case, creating a new device char nodes had nothing to do > > > with whether the device supports single or multi user. I can > > > definitely see in the future training devices that support multiple > > > users. > > > > > > The common drm/accel ioctls should of course not be limited to a > > > single user, and I agree with Jeff here, if a specific driver has such > > > a limitation (e.g. Habana), then that driver should handle it on its > > > own. > > > Maybe if there will be multiple drivers with such a limitation, we can > > > make that "handling" to be common code. > > > > > > Bottom line, I prefer we keep things as we all agreed upon in LPC. > > > > The problem is going to happen as soon as you have cross-vendor userspace. > > Which I'm kinda hoping is at least still the aspiration. Because with > > cross-vendor userspace you generally iterate & open all devices before you > > select the one you're going to use. And so we do kinda need a distinction, > > or we need that the single-user drivers also guarantee that open() is > > cheap. > > FWIW we had good support in ivpu for probe open'
Re: [PATCH v4 3/7] accel/ivpu: Add GEM buffer object management
On Thu, Dec 08, 2022 at 12:07:29PM +0100, Jacek Lawrynowicz wrote: > Adds four types of GEM-based BOs for the VPU: > - shmem > - userptr > - internal Uh what do you need this for? Usually the way we do these is just alloce a normal bo, and then pin them. Also, gem shmem helpers should be able to mostly cover you here, why not use those? Might need some work to push basic userptr to them, but we have enough drivers reinventing that wheel to justify that work. Can I guess also be done after merging. -Daniel > - prime > > All types are implemented as struct ivpu_bo, based on > struct drm_gem_object. VPU address is allocated when buffer is created > except for imported prime buffers that allocate it in BO_INFO IOCTL due > to missing file_priv arg in gem_prime_import callback. > Internal buffers are pinned on creation, the rest of buffers types > can be pinned on demand (in SUBMIT IOCTL). > Buffer VPU address, allocated pages and mappings are relased when the > buffer is destroyed. > Eviction mechism is planned for future versions. > > Add three new IOCTLs: BO_CREATE, BO_INFO, BO_USERPTR > > Signed-off-by: Jacek Lawrynowicz > --- > drivers/accel/ivpu/Makefile | 1 + > drivers/accel/ivpu/ivpu_drv.c | 31 +- > drivers/accel/ivpu/ivpu_drv.h | 1 + > drivers/accel/ivpu/ivpu_gem.c | 820 ++ > drivers/accel/ivpu/ivpu_gem.h | 128 ++ > include/uapi/drm/ivpu_drm.h | 127 ++ > 6 files changed, 1106 insertions(+), 2 deletions(-) > create mode 100644 drivers/accel/ivpu/ivpu_gem.c > create mode 100644 drivers/accel/ivpu/ivpu_gem.h > > diff --git a/drivers/accel/ivpu/Makefile b/drivers/accel/ivpu/Makefile > index 37b8bf1d3247..1b4b24ebf5ea 100644 > --- a/drivers/accel/ivpu/Makefile > +++ b/drivers/accel/ivpu/Makefile > @@ -3,6 +3,7 @@ > > intel_vpu-y := \ > ivpu_drv.o \ > + ivpu_gem.o \ > ivpu_hw_mtl.o \ > ivpu_mmu.o \ > ivpu_mmu_context.o > diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c > index a22d41ca5a4b..29e78c5ec7c5 100644 > --- a/drivers/accel/ivpu/ivpu_drv.c > +++ b/drivers/accel/ivpu/ivpu_drv.c > @@ -12,8 +12,10 @@ > #include > #include > #include > +#include > > #include "ivpu_drv.h" > +#include "ivpu_gem.h" > #include "ivpu_hw.h" > #include "ivpu_mmu.h" > #include "ivpu_mmu_context.h" > @@ -49,6 +51,24 @@ struct ivpu_file_priv *ivpu_file_priv_get(struct > ivpu_file_priv *file_priv) > return file_priv; > } > > +struct ivpu_file_priv *ivpu_file_priv_get_by_ctx_id(struct ivpu_device > *vdev, unsigned long id) > +{ > + struct ivpu_file_priv *file_priv; > + > + xa_lock_irq(&vdev->context_xa); > + file_priv = xa_load(&vdev->context_xa, id); > + /* file_priv may still be in context_xa during file_priv_release() */ > + if (file_priv && !kref_get_unless_zero(&file_priv->ref)) > + file_priv = NULL; > + xa_unlock_irq(&vdev->context_xa); > + > + if (file_priv) > + ivpu_dbg(vdev, KREF, "file_priv get by id: ctx %u refcount > %u\n", > + file_priv->ctx.id, kref_read(&file_priv->ref)); > + > + return file_priv; > +} > + > static void file_priv_release(struct kref *ref) > { > struct ivpu_file_priv *file_priv = container_of(ref, struct > ivpu_file_priv, ref); > @@ -57,7 +77,7 @@ static void file_priv_release(struct kref *ref) > ivpu_dbg(vdev, FILE, "file_priv release: ctx %u\n", file_priv->ctx.id); > > ivpu_mmu_user_context_fini(vdev, &file_priv->ctx); > - WARN_ON(xa_erase_irq(&vdev->context_xa, file_priv->ctx.id) != > file_priv); > + drm_WARN_ON(&vdev->drm, xa_erase_irq(&vdev->context_xa, > file_priv->ctx.id) != file_priv); > kfree(file_priv); > } > > @@ -66,7 +86,7 @@ void ivpu_file_priv_put(struct ivpu_file_priv **link) > struct ivpu_file_priv *file_priv = *link; > struct ivpu_device *vdev = file_priv->vdev; > > - WARN_ON(!file_priv); > + drm_WARN_ON(&vdev->drm, !file_priv); > > ivpu_dbg(vdev, KREF, "file_priv put: ctx %u refcount %u\n", >file_priv->ctx.id, kref_read(&file_priv->ref)); > @@ -200,6 +220,9 @@ static void ivpu_postclose(struct drm_device *dev, struct > drm_file *file) > static const struct drm_ioctl_desc ivpu_drm_ioctls[] = { > DRM_IOCTL_DEF_DRV(IVPU_GET_PARAM, ivpu_get_param_ioctl, 0), > DRM_IOCTL_DEF_DRV(IVPU_SET_PARAM, ivpu_set_param_ioctl, 0), > + DRM_IOCTL_DEF_DRV(IVPU_BO_CREATE, ivpu_bo_create_ioctl, 0), > + DRM_IOCTL_DEF_DRV(IVPU_BO_INFO, ivpu_bo_info_ioctl, 0), > + DRM_IOCTL_DEF_DRV(IVPU_BO_USERPTR, ivpu_bo_userptr_ioctl, 0), > }; > > int ivpu_shutdown(struct ivpu_device *vdev) > @@ -233,6 +256,10 @@ static const struct drm_driver driver = { > > .open = ivpu_open, > .postclose = ivpu_postclose, > + .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > + .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > + .gem_prime_import = ivpu_gem_prime_
Re: [RFC 3/3] drm: Update file owner during use
On Fri, Jan 06, 2023 at 11:32:17AM +0100, Christian König wrote: > Am 05.01.23 um 13:32 schrieb Daniel Vetter: > > [SNIP] > > > For the case of an master fd I actually don't see the reason why we should > > > limit that? And fd can become master if it either was master before or has > > > CAP_SYS_ADMIN. Why would we want an extra check for the pid/tgid here? > > This is just info/debug printing, I don't see the connection to > > drm_auth/master stuff? Aside from the patch mixes up the master opener and > > the current user due to fd passing or something like that. > > That's exactly why my comment meant as well. > > The connect is that the drm_auth/master code currently the pid/tgid as > indicator if the "owner" of the fd has changed and so if an access should be > allowed or not. I find that approach a bit questionable. > > > Note that we cannot do that (I think at least, after pondering this some > > more) because it would break the logind master fd passing scheme - there > > the receiving compositor is explicitly _not_ allowed to acquire master > > rights on its own. So the master priviledges must not move with the fd or > > things can go wrong. > > That could be the rational behind that, but why doesn't logind then just > pass on a normal render node to the compositor? Because the compositor wants the kms node. We have 3 access levels in drm - render stuff - modeset stuff (needs a card* node and master rights for changing things) - set/drop master (needs root) logind wants to give the compositor modeset access, but not master drop/set access (because vt switching is controlled by logind). The pid check in drm_auth is for the use-case where you start your compositor on a root vt (or setuid-root), and then want to make sure that after cred dropping, set/drop master keeps working. Because in that case the vt switch dance is done by the compositor. Maybe we should document this stuff a bit better :-) -Daniel > > Christian. > > > -Daniel > > > > > > > > > Regards, > > > Christian. > > > > > > > Regards, > > > > > > > > Tvrtko > > > > > > > > > -Daniel > > > > > > > > > > > > > > > > return 0; > > > > > > if (!capable(CAP_SYS_ADMIN)) > > > > > > diff --git a/drivers/gpu/drm/drm_debugfs.c > > > > > > b/drivers/gpu/drm/drm_debugfs.c > > > > > > index 42f657772025..9d4e3146a2b8 100644 > > > > > > --- a/drivers/gpu/drm/drm_debugfs.c > > > > > > +++ b/drivers/gpu/drm/drm_debugfs.c > > > > > > @@ -90,15 +90,17 @@ static int drm_clients_info(struct seq_file > > > > > > *m, void *data) > > > > > > */ > > > > > > mutex_lock(&dev->filelist_mutex); > > > > > > list_for_each_entry_reverse(priv, &dev->filelist, lhead) { > > > > > > - struct task_struct *task; > > > > > > bool is_current_master = drm_is_current_master(priv); > > > > > > + struct task_struct *task; > > > > > > + struct pid *pid; > > > > > > - rcu_read_lock(); /* locks pid_task()->comm */ > > > > > > - task = pid_task(priv->pid, PIDTYPE_TGID); > > > > > > + rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! > > > > > > */ > > > > > > + pid = rcu_dereference(priv->pid); > > > > > > + task = pid_task(pid, PIDTYPE_TGID); > > > > > > uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID; > > > > > > seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", > > > > > > task ? task->comm : "", > > > > > > - pid_vnr(priv->pid), > > > > > > + pid_vnr(pid), > > > > > > priv->minor->index, > > > > > > is_current_master ? 'y' : 'n', > > > > > > priv->authenticated ? 'y' : 'n', > > > > > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > > > > > > index 20a9aef2b398..3433f9610dba 100644 > > > > > > --- a/drivers/gpu/drm/drm_file.c > > > > > > +++ b/drivers/gpu/drm/drm_file.c > > > > > > @@ -156,7 +156,7 @@ struct drm_file *drm_file_alloc(struct > > > > > > drm_minor *minor) > > > > > > if (!file) > > > > > > return ERR_PTR(-ENOMEM); > > > > > > - file->pid = get_pid(task_tgid(current)); > > > > > > + rcu_assign_pointer(file->pid, get_pid(task_tgid(current))); > > > > > > file->minor = minor; > > > > > > /* for compatibility root is always authenticated */ > > > > > > @@ -502,6 +502,36 @@ int drm_release(struct inode *inode, struct > > > > > > file *filp) > > > > > > } > > > > > > EXPORT_SYMBOL(drm_release); > > > > > > +void drm_file_update_pid(struct drm_file *filp) > > > > > > +{ > > > > > > + struct drm_device *dev; > > > > > > + struct pid *pid, *old; > > > > > > + > > > > > > + /* Master nodes are not expected to be passed between > > > > > > processes. */ > > > > > > + if (filp->was_master) > > > > > > + return; > > > > > > + > > > > > > + pid = task_tgid(current); > > > > > > + > > > > > > + /* > > > > > > +
[PATCH v4] drm/fb-helper: Replace bpp/depth parameter by color mode
Replace the combination of bpp and depth with a single color-mode argument. Handle special cases in simpledrm and ofdrm. Hard-code XRGB as fallback format for cases where no given format works. The color-mode argument accepts the same values as the kernel's video parameter. These are mostly bpp values between 1 and 32. The exceptions are 15, which has a color depth of 15 and a bpp value of 16; and 32, which has a color depth of 24 and a bpp value of 32. v4: * add back lost test for bpp_specified (Maira) * add Fixes tag (Daniel) v3: * fix ofdrm build (Maxime) v2: * minimize changes (Daniel) * use drm_driver_legacy_fb_format() (Daniel) Signed-off-by: Thomas Zimmermann Fixes: 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format selection") Cc: Thomas Zimmermann Cc: Javier Martinez Canillas Cc: Maarten Lankhorst Cc: Maxime Ripard --- drivers/gpu/drm/drm_fb_helper.c | 42 ++-- drivers/gpu/drm/tiny/ofdrm.c | 7 +- drivers/gpu/drm/tiny/simpledrm.c | 7 +- 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1369ca4ae39b..427631706128 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1756,24 +1756,21 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const return DRM_FORMAT_INVALID; } -static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helper, - const uint32_t *formats, size_t format_count, - const struct drm_cmdline_mode *cmdline_mode) +static uint32_t drm_fb_helper_find_color_mode_format(struct drm_fb_helper *fb_helper, +const uint32_t *formats, size_t format_count, +unsigned int color_mode) { struct drm_device *dev = fb_helper->dev; uint32_t bpp, depth; - if (!cmdline_mode->bpp_specified) - return DRM_FORMAT_INVALID; - - switch (cmdline_mode->bpp) { + switch (color_mode) { case 1: case 2: case 4: case 8: case 16: case 24: - bpp = depth = cmdline_mode->bpp; + bpp = depth = color_mode; break; case 15: bpp = 16; @@ -1784,7 +1781,7 @@ static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helpe depth = 24; break; default: - drm_info(dev, "unsupported bpp value of %d\n", cmdline_mode->bpp); + drm_info(dev, "unsupported color mode of %d\n", color_mode); return DRM_FORMAT_INVALID; } @@ -1817,10 +1814,13 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe drm_client_for_each_connector_iter(connector, &conn_iter) { struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode; - surface_format = drm_fb_helper_find_cmdline_format(fb_helper, - plane->format_types, - plane->format_count, - cmdline_mode); + if (!cmdline_mode->bpp_specified) + continue; + + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + cmdline_mode->bpp); if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ } @@ -1829,17 +1829,23 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ - /* try preferred bpp/depth */ - surface_format = drm_fb_helper_find_format(fb_helper, plane->format_types, - plane->format_count, preferred_bpp, - dev->mode_config.preferred_depth); + /* try preferred color mode */ + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, +
Re: [PATCH 22/27] fbdev: remove tmiofb driver
On Fri, Jan 6, 2023, at 10:47, Helge Deller wrote: > On 1/5/23 14:46, Arnd Bergmann wrote: >> From: Arnd Bergmann >> >> With the TMIO MFD support removed, the framebuffer driver can be >> removed as well. >> >> Cc: Helge Deller >> Cc: linux-fb...@vger.kernel.org >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: Arnd Bergmann > > Acked-by: Helge Deller > > Arnd, I assume you will push the whole series through the ARM tree > (which I'd prefer) ? Yes, I think it's best to keep this together here. Arnd
Re: [PATCH] drm/vkms: introduce prepare_fb and cleanup_fb functions
Hi, Thanks for the review! On 1/6/23 05:10, Thomas Zimmermann wrote: Hi Am 05.01.23 um 19:43 schrieb Melissa Wen: On 01/05, Maíra Canal wrote: With commit 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap"), the behavior of the shadow-plane helpers changed and the vunmap is now performed at the end of the current pageflip, instead of the end of the following pageflip. By performing the vunmap at the end of the current pageflip, invalid memory is accessed by the vkms during the plane composition, as the data is being unmapped before being used. Hi Maíra, Thanks for investigating this issue. Can you add in the commit message the kernel Oops caused by this change? Besides that, I wonder if the right thing would be to restore the previous behavior of vunmap in shadow-plane helpers, instead of reintroduce driver-specific hooks for vmap/vunmap correctly to vkms. Maybe Thomas has some inputs on this shadow-plane changing to help us on figuring out the proper fix (?) The fix looks good. I left some minor-important comments below. I would suggest to rethink the overall driver design. Instead of keeping these buffer pinned for long. It might be better to have a per-plane intermediate buffer that you update on each call to atomic_update. That's how real drivers interact with their hardware. Best Regards, Melissa Therefore, introduce again prepare_fb and cleanup_fb functions to the vkms, which were previously removed on commit b43e2ec03b0d ("drm/vkms: Let shadow-plane helpers prepare the plane's FB"). Fixes: 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap") Signed-off-by: Maíra Canal Acked-by: Thomas Zimmermann --- drivers/gpu/drm/vkms/vkms_plane.c | 36 ++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index c3a845220e10..b3f8a115cc23 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -160,10 +160,44 @@ static int vkms_plane_atomic_check(struct drm_plane *plane, return 0; } +static int vkms_prepare_fb(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_shadow_plane_state *shadow_plane_state; + struct drm_framebuffer *fb = state->fb; + int ret; + + if (!fb) + return 0; IIRC this cannot be NULL. Only active planes will be 'prepared'.> + + shadow_plane_state = to_drm_shadow_plane_state(state); + + ret = drm_gem_plane_helper_prepare_fb(plane, state); + if (ret) + return ret; + + return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data); +} + +static void vkms_cleanup_fb(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_shadow_plane_state *shadow_plane_state; + struct drm_framebuffer *fb = state->fb; + + if (!fb) + return; Same here. This function won't be called if there has not been a framebuffer. After removing those two checks, I started to get some NULL pointer dereference errors, so I believe they are somehow necessary. + + shadow_plane_state = to_drm_shadow_plane_state(state); + + drm_gem_fb_vunmap(fb, shadow_plane_state->map); +} + static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { .atomic_update = vkms_plane_atomic_update, .atomic_check = vkms_plane_atomic_check, - DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, You're still installing {being/end}_fb_access, which should not be necessary now. Open-coding DRM_GEM_SHADOW_PLANE_HELPER_FUNCS without those helpers would fix that. I'm sorry but I didn't understand this comment. AFAIK I {being/end}_fb_access are NULL as I removed the DRM_GEM_SHADOW_PLANE_HELPER_FUNCS macro. Best Regards, - Maíra Canal Best regards Thomas + .prepare_fb = vkms_prepare_fb, + .cleanup_fb = vkms_cleanup_fb, }; struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, -- 2.39.0
Re: [PATCH v4] drm/fb-helper: Replace bpp/depth parameter by color mode
On 1/6/23 08:23, Thomas Zimmermann wrote: Replace the combination of bpp and depth with a single color-mode argument. Handle special cases in simpledrm and ofdrm. Hard-code XRGB as fallback format for cases where no given format works. The color-mode argument accepts the same values as the kernel's video parameter. These are mostly bpp values between 1 and 32. The exceptions are 15, which has a color depth of 15 and a bpp value of 16; and 32, which has a color depth of 24 and a bpp value of 32. v4: * add back lost test for bpp_specified (Maira) * add Fixes tag (Daniel) v3: * fix ofdrm build (Maxime) v2: * minimize changes (Daniel) * use drm_driver_legacy_fb_format() (Daniel) Signed-off-by: Thomas Zimmermann Tested-by: Maíra Canal # vc4 and vkms Thanks for taking care of this! Best Regards, - Maíra Canal Fixes: 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format selection") Cc: Thomas Zimmermann Cc: Javier Martinez Canillas Cc: Maarten Lankhorst Cc: Maxime Ripard --- drivers/gpu/drm/drm_fb_helper.c | 42 ++-- drivers/gpu/drm/tiny/ofdrm.c | 7 +- drivers/gpu/drm/tiny/simpledrm.c | 7 +- 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1369ca4ae39b..427631706128 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1756,24 +1756,21 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const return DRM_FORMAT_INVALID; } -static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helper, - const uint32_t *formats, size_t format_count, - const struct drm_cmdline_mode *cmdline_mode) +static uint32_t drm_fb_helper_find_color_mode_format(struct drm_fb_helper *fb_helper, +const uint32_t *formats, size_t format_count, +unsigned int color_mode) { struct drm_device *dev = fb_helper->dev; uint32_t bpp, depth; - if (!cmdline_mode->bpp_specified) - return DRM_FORMAT_INVALID; - - switch (cmdline_mode->bpp) { + switch (color_mode) { case 1: case 2: case 4: case 8: case 16: case 24: - bpp = depth = cmdline_mode->bpp; + bpp = depth = color_mode; break; case 15: bpp = 16; @@ -1784,7 +1781,7 @@ static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helpe depth = 24; break; default: - drm_info(dev, "unsupported bpp value of %d\n", cmdline_mode->bpp); + drm_info(dev, "unsupported color mode of %d\n", color_mode); return DRM_FORMAT_INVALID; } @@ -1817,10 +1814,13 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe drm_client_for_each_connector_iter(connector, &conn_iter) { struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode; - surface_format = drm_fb_helper_find_cmdline_format(fb_helper, - plane->format_types, - plane->format_count, - cmdline_mode); + if (!cmdline_mode->bpp_specified) + continue; + + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + cmdline_mode->bpp); if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ } @@ -1829,17 +1829,23 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ - /* try preferred bpp/depth */ - surface_format = drm_fb_helper_find_format(fb_helper, plane->format_types, - plane->format_count, preferred_bpp, - dev->mode_config.preferred_depth); + /* try preferred color mode */ + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, +
Re: [PATCH 2/2] drm/rockchip: vop: Leave vblank enabled in self-refresh
On 1/6/23 02:40, Brian Norris wrote: > If we disable vblank when entering self-refresh, vblank APIs (like > DRM_IOCTL_WAIT_VBLANK) no longer work. But user space is not aware when > we enter self-refresh, so this appears to be an API violation -- that > DRM_IOCTL_WAIT_VBLANK fails with EINVAL whenever the display is idle and > enters self-refresh. > > The downstream driver used by many of these systems never used to > disable vblank for PSR, and in fact, even upstream, we didn't do that > until radically redesigning the state machine in commit 6c836d965bad > ("drm/rockchip: Use the helpers for PSR"). > > Thus, it seems like a reasonable API fix to simply restore that > behavior, and leave vblank enabled. > > Note that this appears to potentially unbalance the > drm_crtc_vblank_{off,on}() calls in some cases, but: > (a) drm_crtc_vblank_on() documents this as OK and > (b) if I do the naive balancing, I find state machine issues such that > we're not in sync properly; so it's easier to take advantage of (a). > > Backporting notes: > Marking as 'Fixes' commit 6c836d965bad ("drm/rockchip: Use the helpers > for PSR"), but it probably depends on commit bed030a49f3e > ("drm/rockchip: Don't fully disable vop on self refresh") as well. > > We also need the previous patch ("drm/atomic: Allow vblank-enabled + > self-refresh "disable""), of course. > > Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR") > Cc: > Link: https://lore.kernel.org/dri-devel/y5itf0+yniqa6...@sirena.org.uk/ > Reported-by: "kernelci.org bot" > Signed-off-by: Brian Norris > --- > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index fa1f4ee6d195..c541967114b4 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -719,11 +719,11 @@ static void vop_crtc_atomic_disable(struct drm_crtc > *crtc, > > mutex_lock(&vop->vop_lock); > > - drm_crtc_vblank_off(crtc); > - > if (crtc->state->self_refresh_active) > goto out; > > + drm_crtc_vblank_off(crtc); > + > /* >* Vop standby will take effect at end of current frame, >* if dsp hold valid irq happen, it means standby complete. The out label immediately unlocks vop->vop_lock again, seems a bit pointless. :) AFAICT the self_refresh_active case should just return immediately, the out label would also send any pending atomic commit completion event, which seems wrong now that the vblank interrupt is left enabled. (I also wonder if drm_crtc_vblank_off doesn't take care of that already, at least amdgpu doesn't do this explicitly AFAICT) -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
On Fri, Jan 6, 2023 at 12:45 PM Daniel Vetter wrote: > > On Fri, 6 Jan 2023 at 10:56, Stanislaw Gruszka > wrote: > > > > On Fri, Jan 06, 2023 at 10:28:15AM +0100, Daniel Vetter wrote: > > > On Thu, Jan 05, 2023 at 07:38:26PM +0200, Oded Gabbay wrote: > > > > On Thu, Jan 5, 2023 at 6:25 PM Jeffrey Hugo > > > > wrote: > > > > > > > > > > On 1/5/2023 5:57 AM, Daniel Vetter wrote: > > > > > > On Thu, Dec 08, 2022 at 12:07:27PM +0100, Jacek Lawrynowicz wrote: > > > > > >> +static const struct drm_driver driver = { > > > > > >> +.driver_features = DRIVER_GEM | DRIVER_COMPUTE_ACCEL, > > > > > > > > > > > > So I was wondering whether this is a bright idea, and whether we > > > > > > shouldn't > > > > > > just go ahead and infuse more meaning into accel vs render nodes. > > > > > > > > > > > > The uapi relevant part of render nodes is that they're multi-user > > > > > > safe, at > > > > > > least as much as feasible. Every new open() gives you a new private > > > > > > accelerator. This also has implications on how userspace drivers > > > > > > iterate > > > > > > them, they just open them all in turn and check whether it's the > > > > > > right > > > > > > one - because userspace apis allow applications to enumerate them > > > > > > all. > > > > > > Which also means that any devicie initialization at open() time is a > > > > > > really bad idea. > > > > > > > > > > > > A lot of the compute accelerators otoh (well habanalabs) are single > > > > > > user, > > > > > > init can be done at open() time because you only open this when you > > > > > > actually know you're going to use it. > > > > > > > > > > > > So given this, shouldn't multi-user inference engines be more like > > > > > > render > > > > > > drivers, and less like accel? So DRIVER_RENDER, but still under > > > > > > drivers/accel. > > > > > > > > > > > > This way that entire separate /dev node would actually become > > > > > > meaningful > > > > > > beyond just the basic bikeshed: > > > > > > - render nodes are multi user, safe to iterate and open() just for > > > > > >iteration > > > > > > - accel nodes are single user, you really should not ever open them > > > > > > unless > > > > > >you want to use them > > > > > > > > > > > > Of course would need a doc patch :-) > > > > > > > > > > > > Thoughts? > > > > > > -Daniel > > > > > > > > > > Hmm. > > > > > > > > > > I admit, I thought DRIVER_ACCEL was the same as DRIVER_RENDER, except > > > > > that DRIVER_ACCEL dropped the "legacy" dual node setup and also > > > > > avoided > > > > > "legacy" userspace. > > > > > > > > > > qaic is multi-user. I thought habana was the same, at-least for > > > > > inference. Oded, am I wrong? > > > > Habana's devices support a single user at a time acquiring the device > > > > and working on it. > > > > Both for training and inference. > > > > > > > > > > So, if DRIVER_ACCEL is for single-user (training?), and multi-user > > > > > ends > > > > > up in DRIVER_RENDER, that would seem to mean qaic ends up using > > > > > DRIVER_RENDER and not DRIVER_ACCEL. Then qaic ends up over under > > > > > /dev/dri with both a card node (never used) and a render node. That > > > > > would seem to mean that the "legacy" userspace would open qaic nodes > > > > > by > > > > > default - something I understood Oded was trying to avoid. > > > > > > > > > > If there really a usecase for DRIVER_ACCEL to support single-user? I > > > > > wonder why we can't default to multi-user, and if a particular > > > > > user/driver has a single-user usecase, it enforces that in a driver > > > > > specific manner? > > > > > > > > > > -Jeff > > > > > > > > Honestly, Daniel, I don't like this suggestion. I don't understand why > > > > you make a connection between render/accel to single/multi user. > > > > > > > > As Jeff has said, one of the goals was to expose accelerator devices > > > > to userspace with new device char nodes so we won't be bothered by > > > > legacy userspace graphics software. This is something we all agreed on > > > > and I don't see why we should change it now, even if you think it's > > > > bike-shedding (which I disagree with). > > > > > > > > But in any case, creating a new device char nodes had nothing to do > > > > with whether the device supports single or multi user. I can > > > > definitely see in the future training devices that support multiple > > > > users. > > > > > > > > The common drm/accel ioctls should of course not be limited to a > > > > single user, and I agree with Jeff here, if a specific driver has such > > > > a limitation (e.g. Habana), then that driver should handle it on its > > > > own. > > > > Maybe if there will be multiple drivers with such a limitation, we can > > > > make that "handling" to be common code. > > > > > > > > Bottom line, I prefer we keep things as we all agreed upon in LPC. > > > > > > The problem is going to happen as soon as you have cross-vendor userspace. > > > Which I'm kinda hoping is at least still the a
Re: [PATCH] drm/vkms: introduce prepare_fb and cleanup_fb functions
Hi Am 06.01.23 um 12:34 schrieb Maíra Canal: Hi, Thanks for the review! On 1/6/23 05:10, Thomas Zimmermann wrote: Hi Am 05.01.23 um 19:43 schrieb Melissa Wen: On 01/05, Maíra Canal wrote: With commit 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap"), the behavior of the shadow-plane helpers changed and the vunmap is now performed at the end of the current pageflip, instead of the end of the following pageflip. By performing the vunmap at the end of the current pageflip, invalid memory is accessed by the vkms during the plane composition, as the data is being unmapped before being used. Hi Maíra, Thanks for investigating this issue. Can you add in the commit message the kernel Oops caused by this change? Besides that, I wonder if the right thing would be to restore the previous behavior of vunmap in shadow-plane helpers, instead of reintroduce driver-specific hooks for vmap/vunmap correctly to vkms. Maybe Thomas has some inputs on this shadow-plane changing to help us on figuring out the proper fix (?) The fix looks good. I left some minor-important comments below. I would suggest to rethink the overall driver design. Instead of keeping these buffer pinned for long. It might be better to have a per-plane intermediate buffer that you update on each call to atomic_update. That's how real drivers interact with their hardware. Best Regards, Melissa Therefore, introduce again prepare_fb and cleanup_fb functions to the vkms, which were previously removed on commit b43e2ec03b0d ("drm/vkms: Let shadow-plane helpers prepare the plane's FB"). Fixes: 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap") Signed-off-by: Maíra Canal Acked-by: Thomas Zimmermann --- drivers/gpu/drm/vkms/vkms_plane.c | 36 ++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index c3a845220e10..b3f8a115cc23 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -160,10 +160,44 @@ static int vkms_plane_atomic_check(struct drm_plane *plane, return 0; } +static int vkms_prepare_fb(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_shadow_plane_state *shadow_plane_state; + struct drm_framebuffer *fb = state->fb; + int ret; + + if (!fb) + return 0; IIRC this cannot be NULL. Only active planes will be 'prepared'.> + + shadow_plane_state = to_drm_shadow_plane_state(state); + + ret = drm_gem_plane_helper_prepare_fb(plane, state); + if (ret) + return ret; + + return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data); +} + +static void vkms_cleanup_fb(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_shadow_plane_state *shadow_plane_state; + struct drm_framebuffer *fb = state->fb; + + if (!fb) + return; Same here. This function won't be called if there has not been a framebuffer. After removing those two checks, I started to get some NULL pointer dereference errors, so I believe they are somehow necessary. Ok. + + shadow_plane_state = to_drm_shadow_plane_state(state); + + drm_gem_fb_vunmap(fb, shadow_plane_state->map); +} + static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { .atomic_update = vkms_plane_atomic_update, .atomic_check = vkms_plane_atomic_check, - DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, You're still installing {being/end}_fb_access, which should not be necessary now. Open-coding DRM_GEM_SHADOW_PLANE_HELPER_FUNCS without those helpers would fix that. I'm sorry but I didn't understand this comment. AFAIK I {being/end}_fb_access are NULL as I removed the DRM_GEM_SHADOW_PLANE_HELPER_FUNCS macro. Sorry, I misread that line. You're right. Best regards Thomas Best Regards, - Maíra Canal Best regards Thomas + .prepare_fb = vkms_prepare_fb, + .cleanup_fb = vkms_cleanup_fb, }; struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, -- 2.39.0 -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
[PATCH v2] drm/vkms: reintroduce prepare_fb and cleanup_fb functions
With commit 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap"), the behavior of the shadow-plane helpers changed and the vunmap is now performed at the end of the current pageflip, instead of the end of the following pageflip. By performing the vunmap at the end of the current pageflip, invalid memory is accessed by the vkms during the plane composition, as the data is being unmapped before being used, as reported by the following warning: [ 275.866047] BUG: unable to handle page fault for address: b382814e8002 [ 275.866055] #PF: supervisor read access in kernel mode [ 275.866058] #PF: error_code(0x) - not-present page [ 275.866061] PGD 167 P4D 167 PUD 110a067 PMD 46e3067 PTE 0 [ 275.866066] Oops: [#1] PREEMPT SMP PTI [ 275.866070] CPU: 2 PID: 49 Comm: kworker/u8:2 Not tainted 6.1.0-rc6-00018-gb357e7ac1b73-dirty #54 [ 275.866074] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.1-2.fc37 04/01/2014 [ 275.866076] Workqueue: vkms_composer vkms_composer_worker [vkms] [ 275.866084] RIP: 0010:XRGB_to_argb_u16+0x5c/0xa0 [vkms] [ 275.866092] Code: bf 56 0a 0f af 56 70 48 8b 76 28 01 ca 49 83 f8 02 41 b9 01 00 00 00 4d 0f 43 c8 48 01 f2 48 83 c2 02 31 f6 66 c7 04 f0 ff ff <0f> b6 0c b2 89 cf c1 e7 08 09 cf 66 89 7c f0 02 0f b6 4c b2 ff 89 [ 275.866095] RSP: 0018:b382801b7db0 EFLAGS: 00010246 [ 275.866098] RAX: 896336ace000 RBX: 896310e293c0 RCX: [ 275.866101] RDX: b382814e8002 RSI: RDI: b382801b7de8 [ 275.866103] RBP: 1400 R08: 0280 R09: 0280 [ 275.866105] R10: 0010 R11: c011d990 R12: 896302a1ece0 [ 275.866107] R13: R14: R15: 80008001 [ 275.866109] FS: () GS:89637dd0() knlGS: [ 275.866112] CS: 0010 DS: ES: CR0: 80050033 [ 275.866114] CR2: b382814e8002 CR3: 03bb4000 CR4: 06e0 [ 275.866120] Call Trace: [ 275.866123] [ 275.866124] compose_active_planes+0x1c4/0x380 [vkms] [ 275.866132] vkms_composer_worker+0x9f/0x130 [vkms] [ 275.866139] process_one_work+0x1c0/0x370 [ 275.866160] worker_thread+0x221/0x410 [ 275.866164] ? worker_clr_flags+0x50/0x50 [ 275.866167] kthread+0xe1/0x100 [ 275.866172] ? kthread_blkcg+0x30/0x30 [ 275.866176] ret_from_fork+0x22/0x30 [ 275.866181] [ 275.866182] Modules linked in: vkms [ 275.866186] CR2: b382814e8002 [ 275.866191] ---[ end trace ]--- Therefore, introduce again prepare_fb and cleanup_fb functions to the vkms, which were previously removed on commit b43e2ec03b0d ("drm/vkms: Let shadow-plane helpers prepare the plane's FB"). Fixes: 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap") Acked-by: Thomas Zimmermann Signed-off-by: Maíra Canal --- v1 -> v2: https://lore.kernel.org/dri-devel/19951367-2ef0-0f26-ddf0-893259d9a...@igalia.com/T/ - Add kernel oops to the commit description (Melissa Wen). - s/introduce/reintroduce/ in the title (Melissa Wen). - Add Thomas's Acked-by. --- drivers/gpu/drm/vkms/vkms_plane.c | 36 ++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index c3a845220e10..b3f8a115cc23 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -160,10 +160,44 @@ static int vkms_plane_atomic_check(struct drm_plane *plane, return 0; } +static int vkms_prepare_fb(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_shadow_plane_state *shadow_plane_state; + struct drm_framebuffer *fb = state->fb; + int ret; + + if (!fb) + return 0; + + shadow_plane_state = to_drm_shadow_plane_state(state); + + ret = drm_gem_plane_helper_prepare_fb(plane, state); + if (ret) + return ret; + + return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data); +} + +static void vkms_cleanup_fb(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_shadow_plane_state *shadow_plane_state; + struct drm_framebuffer *fb = state->fb; + + if (!fb) + return; + + shadow_plane_state = to_drm_shadow_plane_state(state); + + drm_gem_fb_vunmap(fb, shadow_plane_state->map); +} + static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { .atomic_update = vkms_plane_atomic_update, .atomic_check = vkms_plane_atomic_check, - DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, + .prepare_fb = vkms_prepare_fb, + .cleanup_fb = vkms_cleanup_fb, }; struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev
Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
On Fri, Jan 06, 2023 at 11:44:58AM +0100, Daniel Vetter wrote: > > > The problem is going to happen as soon as you have cross-vendor userspace. > > > Which I'm kinda hoping is at least still the aspiration. Because with > > > cross-vendor userspace you generally iterate & open all devices before you > > > select the one you're going to use. And so we do kinda need a distinction, > > > or we need that the single-user drivers also guarantee that open() is > > > cheap. > > > > FWIW we had good support in ivpu for probe open's in form of lazy context > > allocation. It was removed recently due to review feedback that this is > > unnecessary, but we can add it back. > > Yeah once you have more than 1 multi-user accel chip in the system you > need to do that. Which is really the reason why I think smashing > multi-user client accel things into render is good, it forces drivers > to suck less. > > On that topic, does your userspace still do the drmIoctl() wrapper? Yes, it still does. We released the code BTW, the wrapper can be seen here: https://github.com/intel/linux-vpu-driver/blob/b6ed73cabf87f461cbbe4427e1b9351a548d790b/umd/vpu_driver/source/os_interface/vpu_driver_api.cpp#L41 Regards Stanislaw
Re: [PATCH v7 7/9] dt/bindings: drm/bridge: it6505: Add mode-switch support
On 05/01/2023 14:24, Pin-yen Lin wrote: > ITE IT6505 can be used in systems to switch the DP traffic between > two downstreams, which can be USB Type-C DisplayPort alternate mode > lane or regular DisplayPort output ports. Use subject prefixes matching the subsystem (which you can get for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching). > > Update the binding to accommodate this usage by introducing a > data-lanes and a mode-switch property on endpoints. > > Signed-off-by: Pin-yen Lin > > --- > > Changes in v7: > - Fixed issues reported by dt_binding_check. > - Updated the schema and the example dts for data-lanes. > - Changed to generic naming for the example dts node. > > Changes in v6: > - Remove switches node and use endpoints and data-lanes property to > describe the connections. > > .../bindings/display/bridge/ite,it6505.yaml | 95 --- > 1 file changed, 84 insertions(+), 11 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml > b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml > index b16a9d9127dd..1ee7cd0d2035 100644 > --- a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml > @@ -77,20 +77,45 @@ properties: > unevaluatedProperties: false > description: Video port for DP output > > -properties: > - endpoint: > +patternProperties: > + "^endpoint@[01]$": > $ref: /schemas/graph.yaml#/$defs/endpoint-base > unevaluatedProperties: false > > properties: > + reg: > +maxItems: 1 > + > + remote-endpoint: true > + >data-lanes: > -minItems: 1 > -uniqueItems: true > -items: > - - enum: [ 0, 1 ] > - - const: 1 > - - const: 2 > - - const: 3 > +oneOf: > + - minItems: 1 Drop minItems. > +maxItems: 1 Actually drop this as well and just use items with one item (enum). > +items: > + enum: [0, 1, 2, 3] > + > + - items: > + - const: 0 > + - const: 1 > + > + - items: > + - const: 2 > + - const: 3 > + > + - items: > + - const: 0 > + - const: 1 > + - const: 2 > + - const: 3 > + > + mode-switch: > +type: boolean > +description: Register this node as a Type-C mode switch or > not. > + > +required: > + - reg > + - remote-endpoint > > required: >- port@0 > @@ -102,7 +127,6 @@ required: >- pwr18-supply >- interrupts >- reset-gpios > - - extcon >- ports > > additionalProperties: false > @@ -139,8 +163,11 @@ examples: > }; > > port@1 { > +#address-cells = <1>; > +#size-cells = <0>; > reg = <1>; > -it6505_out: endpoint { > +it6505_out: endpoint@0 { > +reg = <0>; > remote-endpoint = <&dp_in>; > data-lanes = <0 1>; > }; > @@ -148,3 +175,49 @@ examples: > }; > }; > }; > + - | > +#include > + > +i2c3 { Just i2c Best regards, Krzysztof
Re: [PATCH 1/2] dt-bindings: display: panel: document the Visionox VTDR6130 AMOLED DSI Panel bindings
On 03/01/2023 15:22, Neil Armstrong wrote: > Document the 1080x2400 Visionox VTDR6130 AMOLED DSI Panel bindings. > > Signed-off-by: Neil Armstrong > --- > .../bindings/display/panel/visionox,vtdr6130.yaml | 53 > ++ > 1 file changed, 53 insertions(+) Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH 1/2] dt-bindings: display: panel: document the Visionox VTDR6130 AMOLED DSI Panel bindings
Hi Neil, > > > +properties: > > > + compatible: > > > +const: visionox,vtdr6130 > > > + > > > + vddio-supply: true > > > + vci-supply: true > > > + vdd-supply: true > > These 3 looks wrong to me, as the above are not documented in panel-common. > > But maybe I miss something and this is OK? > > It should be OK, the -supply properties are standard properties Thanks for the explanation - patch is then: Reviewed-by: Sam Ravnborg
Re: [RFC 3/3] drm: Update file owner during use
On 05/01/2023 12:32, Daniel Vetter wrote: On Fri, Dec 02, 2022 at 10:01:01AM +0100, Christian König wrote: Am 01.12.22 um 12:09 schrieb Tvrtko Ursulin: On 30/11/2022 14:18, Daniel Vetter wrote: On Wed, Nov 30, 2022 at 01:34:07PM +, Tvrtko Ursulin wrote: From: Tvrtko Ursulin With the typical model where the display server opends the file descriptor and then hands it over to the client we were showing stale data in debugfs. Fix it by updating the drm_file->pid on ioctl access from a different process. The field is also made RCU protected to allow for lockless readers. Update side is protected with dev->filelist_mutex. Signed-off-by: Tvrtko Ursulin Cc: "Christian König" --- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 +++-- drivers/gpu/drm/drm_auth.c | 3 ++- drivers/gpu/drm/drm_debugfs.c | 10 drivers/gpu/drm/drm_file.c | 32 - drivers/gpu/drm/drm_ioctl.c | 3 +++ drivers/gpu/drm/nouveau/nouveau_drm.c | 5 +++- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 6 +++-- include/drm/drm_file.h | 13 -- 8 files changed, 65 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 30e24da1f398..385deb044058 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -959,6 +959,7 @@ static int amdgpu_debugfs_gem_info_show(struct seq_file *m, void *unused) list_for_each_entry(file, &dev->filelist, lhead) { struct task_struct *task; struct drm_gem_object *gobj; + struct pid *pid; int id; /* @@ -968,8 +969,9 @@ static int amdgpu_debugfs_gem_info_show(struct seq_file *m, void *unused) * Therefore, we need to protect this ->comm access using RCU. */ rcu_read_lock(); - task = pid_task(file->pid, PIDTYPE_TGID); - seq_printf(m, "pid %8d command %s:\n", pid_nr(file->pid), + pid = rcu_dereference(file->pid); + task = pid_task(pid, PIDTYPE_TGID); + seq_printf(m, "pid %8d command %s:\n", pid_nr(pid), task ? task->comm : ""); rcu_read_unlock(); diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index cf92a9ae8034..2ed2585ded37 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -235,7 +235,8 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv) static int drm_master_check_perm(struct drm_device *dev, struct drm_file *file_priv) { - if (file_priv->pid == task_pid(current) && file_priv->was_master) + if (file_priv->was_master && + rcu_access_pointer(file_priv->pid) == task_pid(current)) This scares me, and also makes me wonder whether we really want to conflate the original owner with the rendering owner. And also, whether we really want to keep updating that, because for some of the "bind an fd to a pid" use-cases like svm we really do not want to ever again allow a change. So sligthly different idea: - we have a separate render pid/drm_file owner frome the open() owner that we track in drm_auth.c - that one is set the first time a driver specific ioctl is called (which for the "pass me the fd" dri3 mode should never be the compositor) - we start out with nothing and only set it once, which further simplifies the model (still need the mutex for concurrent first ioctl ofc) Simpler solution sounds plausible and mostly works for me. Certainly is attractive to simplify things. And as the disclaimer I put in the cover letter - I wasn't really sure at all if passing a master fd is a thing or not. Happy to implement your version if that will be the decision. The only downside I can think of right now with having two owners is if someone is "naughty" and actually uses the fd for rendering from two sides. That wouldn't conceptually work for what I am doing in the cgroup controller, where I need to attribute GPU usage to a process, which is a lookup from struct pid -> list of drm_files -> etc. So in the two owners scheme I would just need to ignore the "open owner" and rely that "render ownder" truly is the only one doing the rendering. Or maybe I'd need to add support for multiple owners as well.. would be a bit annoying probably. Hm now that I think about more.. the one shot nature of this scheme would have another downside. One could just send the fd back to itself via a throway forked helper, which only does one ioctl before sending it back, and then the "render owner" is forever lost. The proposal as I had it would be immune to this problem at least. Eventually we could then use that to enforce static binding to a pid, which is what we want for svm style models, i.e. if the pid changes, the fd does an -EACCESS or similar. Thoughts? This use case I am not familiar with at all so can't comment. Only intui
Re: [PATCH v4 3/7] accel/ivpu: Add GEM buffer object management
On Fri, Jan 06, 2023 at 11:50:05AM +0100, Daniel Vetter wrote: > On Thu, Dec 08, 2022 at 12:07:29PM +0100, Jacek Lawrynowicz wrote: > > Adds four types of GEM-based BOs for the VPU: > > - shmem > > - userptr > > - internal > > Uh what do you need this for? Usually the way we do these is just alloce a > normal bo, and then pin them. I think we do alloc/pin this way, but all our bo's are GEM based. For those bo's we use internally and other non-shmem we create them with drm_gem_private_object_init(). I think this way is simpler than have separate code for non-GEM and GEM bo's ... > Also, gem shmem helpers should be able to mostly cover you here, why not > use those? Might need some work to push basic userptr to them, but we have > enough drivers reinventing that wheel to justify that work. > > Can I guess also be done after merging. ... but if not, we can add this to TODO. Regards Stanislaw
Re: [PATCH v4 3/7] accel/ivpu: Add GEM buffer object management
Hi On Thu, Jan 05, 2023 at 12:46:51PM -0600, Andrew Davis wrote: > On 12/8/22 5:07 AM, Jacek Lawrynowicz wrote: > > Adds four types of GEM-based BOs for the VPU: > >- shmem > >- userptr > > Do you have some specific need for userptr that would not > be covered by prime import + heaps? I'm just trying to get > a feel for the typical use-cases for these. Honestly, I'm not sure. I think we have use-cases that justify adding userptr, but have to check with our team members that better understand the requirements. Regards Stanislaw
Re: [PATCH v4] drm/fb-helper: Replace bpp/depth parameter by color mode
Hi, I've pushed this fix into drm-misc-next. It will hopefully restore the fbdev consoles. Sorry for the inconvenience. If things still don't work, stating video=1024x768-32 on the kernel command line should be a safe override on most systems. Best regards Thomas Am 06.01.23 um 12:23 schrieb Thomas Zimmermann: Replace the combination of bpp and depth with a single color-mode argument. Handle special cases in simpledrm and ofdrm. Hard-code XRGB as fallback format for cases where no given format works. The color-mode argument accepts the same values as the kernel's video parameter. These are mostly bpp values between 1 and 32. The exceptions are 15, which has a color depth of 15 and a bpp value of 16; and 32, which has a color depth of 24 and a bpp value of 32. v4: * add back lost test for bpp_specified (Maira) * add Fixes tag (Daniel) v3: * fix ofdrm build (Maxime) v2: * minimize changes (Daniel) * use drm_driver_legacy_fb_format() (Daniel) Signed-off-by: Thomas Zimmermann Fixes: 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format selection") Cc: Thomas Zimmermann Cc: Javier Martinez Canillas Cc: Maarten Lankhorst Cc: Maxime Ripard --- drivers/gpu/drm/drm_fb_helper.c | 42 ++-- drivers/gpu/drm/tiny/ofdrm.c | 7 +- drivers/gpu/drm/tiny/simpledrm.c | 7 +- 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1369ca4ae39b..427631706128 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1756,24 +1756,21 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const return DRM_FORMAT_INVALID; } -static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helper, - const uint32_t *formats, size_t format_count, - const struct drm_cmdline_mode *cmdline_mode) +static uint32_t drm_fb_helper_find_color_mode_format(struct drm_fb_helper *fb_helper, +const uint32_t *formats, size_t format_count, +unsigned int color_mode) { struct drm_device *dev = fb_helper->dev; uint32_t bpp, depth; - if (!cmdline_mode->bpp_specified) - return DRM_FORMAT_INVALID; - - switch (cmdline_mode->bpp) { + switch (color_mode) { case 1: case 2: case 4: case 8: case 16: case 24: - bpp = depth = cmdline_mode->bpp; + bpp = depth = color_mode; break; case 15: bpp = 16; @@ -1784,7 +1781,7 @@ static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helpe depth = 24; break; default: - drm_info(dev, "unsupported bpp value of %d\n", cmdline_mode->bpp); + drm_info(dev, "unsupported color mode of %d\n", color_mode); return DRM_FORMAT_INVALID; } @@ -1817,10 +1814,13 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe drm_client_for_each_connector_iter(connector, &conn_iter) { struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode; - surface_format = drm_fb_helper_find_cmdline_format(fb_helper, - plane->format_types, - plane->format_count, - cmdline_mode); + if (!cmdline_mode->bpp_specified) + continue; + + surface_format = drm_fb_helper_find_color_mode_format(fb_helper, + plane->format_types, + plane->format_count, + cmdline_mode->bpp); if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ } @@ -1829,17 +1829,23 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe if (surface_format != DRM_FORMAT_INVALID) break; /* found supported format */ - /* try preferred bpp/depth */ - surface_format = drm_fb_helper_find_format(fb_helper, plane->format_types, - plane->format_count, preferred_bpp, - dev->mod
Re: [Intel-gfx] [PATCH v3 11/11] drm/i915: replace Intel internal tracker with kernel core ref_tracker
Hi Andrzej, On 2/22/2022 12:25 AM, Andrzej Hajda wrote: Beside reusing existing code, the main advantage of ref_tracker is tracking per instance of wakeref. It allows also to catch double put. On the other side we lose information about the first acquire and the last release, but the advantages outweigh it. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- drivers/gpu/drm/i915/Kconfig.debug| 11 +- drivers/gpu/drm/i915/Makefile | 3 - .../drm/i915/display/intel_display_power.c| 2 +- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 2 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 25 +- drivers/gpu/drm/i915/intel_runtime_pm.h | 2 +- drivers/gpu/drm/i915/intel_wakeref.c | 8 +- drivers/gpu/drm/i915/intel_wakeref.h | 72 +- drivers/gpu/drm/i915/intel_wakeref_tracker.c | 234 -- drivers/gpu/drm/i915/intel_wakeref_tracker.h | 76 -- 11 files changed, 87 insertions(+), 350 deletions(-) delete mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.c delete mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.h diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 3bdc73f30a9e1..6c57f3e265f20 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -32,6 +32,7 @@ config DRM_I915_DEBUG select DEBUG_FS select PREEMPT_COUNT select I2C_CHARDEV + select REF_TRACKER select STACKDEPOT select STACKTRACE select DRM_DP_AUX_CHARDEV @@ -46,7 +47,6 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_GEM select DRM_I915_DEBUG_GEM_ONCE select DRM_I915_DEBUG_MMIO - select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM select DRM_I915_DEBUG_WAKEREF select DRM_I915_SW_FENCE_DEBUG_OBJECTS @@ -238,18 +238,13 @@ config DRM_I915_DEBUG_VBLANK_EVADE If in doubt, say "N". -config DRM_I915_TRACK_WAKEREF - depends on STACKDEPOT - depends on STACKTRACE - bool - config DRM_I915_DEBUG_RUNTIME_PM bool "Enable extra state checking for runtime PM" depends on DRM_I915 default n + select REF_TRACKER select STACKDEPOT select STACKTRACE - select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking for the runtime PM functionality. This may introduce overhead during @@ -263,9 +258,9 @@ config DRM_I915_DEBUG_WAKEREF bool "Enable extra tracking for wakerefs" depends on DRM_I915 default n + select REF_TRACKER select STACKDEPOT select STACKTRACE - select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking and usage tracking for the wakerefPM functionality. This may introduce diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 88a403d3294cb..1f8d71430e2e6 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -76,9 +76,6 @@ i915-$(CONFIG_DEBUG_FS) += \ display/intel_display_debugfs.o \ display/intel_pipe_crc.o -i915-$(CONFIG_DRM_I915_TRACK_WAKEREF) += \ - intel_wakeref_tracker.o - i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o # "Graphics Technology" (aka we talk to the gpu) diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 9ebae7ac32356..0e1bf724f89b5 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -2107,7 +2107,7 @@ print_async_put_domains_state(struct i915_power_domains *power_domains) struct drm_i915_private, power_domains); - drm_dbg(&i915->drm, "async_put_wakeref %u\n", + drm_dbg(&i915->drm, "async_put_wakeref %lu\n", power_domains->async_put_wakeref); print_power_domains(power_domains, "async_put_domains[0]", diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index 52e46e7830ff5..cf8cc348942cb 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -273,7 +273,7 @@ void intel_engine_init__pm(struct intel_engine_cs *engine) { struct intel_runtime_pm *rpm = engine->uncore->rpm; - intel_wakeref_init(&engine->wakeref, rpm, &wf_ops); + intel_wakeref_init(&engine->wakeref, rpm, &wf_ops, engine->name); intel_engine_init_heartbeat(engine); } diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index 7ee65a93f926f..01a055d0d0989 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/inte
Re: [PATCH v10 00/18] drm: Add Samsung MIPI DSIM bridge
On Thu, Jan 5, 2023 at 4:24 AM Jagan Teki wrote: > > On Wed, Dec 14, 2022 at 6:29 PM Jagan Teki wrote: > > > > This series supports common bridge support for Samsung MIPI DSIM > > which is used in Exynos and i.MX8MM SoC's. > > > > The final bridge supports both the Exynos and i.MX8M Mini/Nano/Plus. > > > > Patch 0001 - 0004: adding devm_drm_of_dsi_get_bridge > > > > Patch 0005 - 0006: optional PHY, PMS_P offset > > > > Patch 0007 : introduce hw_type > > > > Patch 0008 : fixing host init > > > > Patch 0009 : atomic_check > > > > Patch 0010 : input_bus_flags > > > > Patch 0011 : atomic_get_input_bus_fmts > > > > Patch 0012 - 0013: component vs bridge > > > > Patch 0014 : DSIM bridge > > > > Patch 0015 - 0016: i.MX8M Mini/Nano > > > > Patch 0017 - 0018: i.MX8M Plus > > > > Changes for v10: > > - rebase on drm-misc-next > > - add drm_of_dsi_find_panel_or_bridge > > - add devm_drm_of_dsi_get_bridge > > - fix host initialization (Thanks to Marek Szyprowski) > > - rearrange the tiny patches for easy to review > > - update simple names for enum hw_type > > - add is_hw_exynos macro > > - rework on commit messages > > > > Changes for v9: > > - rebase on drm-misc-next > > - drop drm bridge attach fix for Exynos > > - added prepare_prev_first flag > > - added pre_enable_prev_first flag > > - fix bridge chain order for exynos > > - added fix for Exynos host init for first DSI transfer > > - added MEDIA_BUS_FMT_FIXED > > - return MEDIA_BUS_FMT_RGB888_1X24 output_fmt if supported output_fmt > > list is unsupported. > > - added MEDIA_BUS_FMT_YUYV10_1X20 > > - added MEDIA_BUS_FMT_YUYV12_1X24 > > > > Changes for v8: > > * fixed comment lines > > * fixed commit messages > > * fixed video mode bits > > * collect Marek Ack > > * fixed video mode bit names > > * update input formats logic > > * added imx8mplus support > > > > Changes for v7: > > * fix the drm bridge attach chain for exynos drm dsi driver > > * fix the hw_type checking logic > > > > Changes for v6: > > * handle previous bridge for exynos dsi while attaching bridge > > > > Changes for v5: > > * bridge changes to support multi-arch > > * updated and clear commit messages > > * add hw_type via plat data > > * removed unneeded quirk > > * rebased on linux-next > > > > Changes for v4: > > * include Inki Dae in MAINTAINERS > > * remove dsi_driver probe in exynos_drm_drv to support multi-arch build > > * update init handling to ensure host init done on first cmd transfer > > > > Changes for v3: > > * fix the mult-arch build > > * fix dsi host init > > * updated commit messages > > > > Changes for v2: > > * fix bridge handling > > * fix dsi host init > > * correct the commit messages > > > > Tested in Engicam i.Core MX8M Mini SoM. > > > > Repo: > > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > > > > v9: > > https://lore.kernel.org/all/20221209152343.180139-1-ja...@amarulasolutions.com/ > > > > Any inputs? > > Jagan. > > > > Jagan Teki (16): > > drm: of: Lookup if child node has DSI panel or bridge > > drm: bridge: panel: Add devm_drm_of_dsi_get_bridge helper > > drm: exynos: dsi: Drop explicit call to bridge detach > > drm: exynos: dsi: Switch to devm_drm_of_dsi_get_bridge > > drm: exynos: dsi: Mark PHY as optional > > drm: exynos: dsi: Add platform PLL_P (PMS_P) offset > > drm: exynos: dsi: Introduce hw_type platform data > > drm: exynos: dsi: Add atomic check > > drm: exynos: dsi: Add input_bus_flags > > drm: exynos: dsi: Add atomic_get_input_bus_fmts > > drm: exynos: dsi: Consolidate component and bridge > > drm: exynos: dsi: Add Exynos based host irq hooks > > drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge > > dt-bindings: display: exynos: dsim: Add NXP i.MX8M Mini/Nano support > > drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano support > > dt-bindings: display: exynos: dsim: Add NXP i.MX8M Plus support > > > > Marek Szyprowski (1): > > drm: exynos: dsi: Handle proper host initialization > > > > Marek Vasut (1): > > drm: bridge: samsung-dsim: Add i.MX8M Plus support > > Does anyone have any other comments on this? I would like to send v11 > with a few nits on v10. Please let me know. I got it working on an LVDS display that I have, but I didn't get it working on the HDMI bridge. Since we have a few tested-by people, it'd be nice to see this integrated so we can work on ading more functionality adam > > Thanks, > Jagan.
Re: [PATCH v10 00/18] drm: Add Samsung MIPI DSIM bridge
On Fri, Jan 6, 2023 at 11:34 AM Adam Ford wrote: > I got it working on an LVDS display that I have, but I didn't get it > working on the HDMI bridge. Since we have a few tested-by people, > it'd be nice to see this integrated so we can work on ading more > functionality Agreed. Hopefully, this series can be applied soon so we don't miss another cycle.
Re: [RFC 3/3] drm: Update file owner during use
Am 06.01.23 um 11:53 schrieb Daniel Vetter: On Fri, Jan 06, 2023 at 11:32:17AM +0100, Christian König wrote: Am 05.01.23 um 13:32 schrieb Daniel Vetter: [SNIP] For the case of an master fd I actually don't see the reason why we should limit that? And fd can become master if it either was master before or has CAP_SYS_ADMIN. Why would we want an extra check for the pid/tgid here? This is just info/debug printing, I don't see the connection to drm_auth/master stuff? Aside from the patch mixes up the master opener and the current user due to fd passing or something like that. That's exactly why my comment meant as well. The connect is that the drm_auth/master code currently the pid/tgid as indicator if the "owner" of the fd has changed and so if an access should be allowed or not. I find that approach a bit questionable. Note that we cannot do that (I think at least, after pondering this some more) because it would break the logind master fd passing scheme - there the receiving compositor is explicitly _not_ allowed to acquire master rights on its own. So the master priviledges must not move with the fd or things can go wrong. That could be the rational behind that, but why doesn't logind then just pass on a normal render node to the compositor? Because the compositor wants the kms node. We have 3 access levels in drm - render stuff - modeset stuff (needs a card* node and master rights for changing things) - set/drop master (needs root) logind wants to give the compositor modeset access, but not master drop/set access (because vt switching is controlled by logind). The pid check in drm_auth is for the use-case where you start your compositor on a root vt (or setuid-root), and then want to make sure that after cred dropping, set/drop master keeps working. Because in that case the vt switch dance is done by the compositor. Maybe we should document this stuff a bit better :-) Maybe add a friendly warning? E.g. like "Don't touch it, it works!" :) So basically this is a valid use case where logind set/get the master status of a fd while the compositor uses the master functionality? Christian. -Daniel Christian. -Daniel Regards, Christian. Regards, Tvrtko -Daniel return 0; if (!capable(CAP_SYS_ADMIN)) diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index 42f657772025..9d4e3146a2b8 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -90,15 +90,17 @@ static int drm_clients_info(struct seq_file *m, void *data) */ mutex_lock(&dev->filelist_mutex); list_for_each_entry_reverse(priv, &dev->filelist, lhead) { - struct task_struct *task; bool is_current_master = drm_is_current_master(priv); + struct task_struct *task; + struct pid *pid; - rcu_read_lock(); /* locks pid_task()->comm */ - task = pid_task(priv->pid, PIDTYPE_TGID); + rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */ + pid = rcu_dereference(priv->pid); + task = pid_task(pid, PIDTYPE_TGID); uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID; seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", task ? task->comm : "", - pid_vnr(priv->pid), + pid_vnr(pid), priv->minor->index, is_current_master ? 'y' : 'n', priv->authenticated ? 'y' : 'n', diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 20a9aef2b398..3433f9610dba 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -156,7 +156,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor) if (!file) return ERR_PTR(-ENOMEM); - file->pid = get_pid(task_tgid(current)); + rcu_assign_pointer(file->pid, get_pid(task_tgid(current))); file->minor = minor; /* for compatibility root is always authenticated */ @@ -502,6 +502,36 @@ int drm_release(struct inode *inode, struct file *filp) } EXPORT_SYMBOL(drm_release); +void drm_file_update_pid(struct drm_file *filp) +{ + struct drm_device *dev; + struct pid *pid, *old; + + /* Master nodes are not expected to be passed between processes. */ + if (filp->was_master) + return; + + pid = task_tgid(current); + + /* + * Quick unlocked check since the model is a single handover followed by + * exclusive repeated use. + */ + if (pid == rcu_access_pointer(filp->pid)) + return; + + dev = filp->minor->dev; + mutex_lock(&dev->filelist_mutex); + old = rcu_replace_pointer(filp->pid, pid, 1); + mutex_unlock(&dev->filelist_mutex); + + if (pid != old) { + get_pid(pid); + synchronize_rcu(); + put_pid(old); + } +} + /** * drm_release_noglobal - release method for DRM file * @inode: device inode diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/
Re: [PATCH v4] drm/amd/display: fix PSR-SU/DSC interoperability support
On 1/5/23 17:23, Hamza Mahfooz wrote: > Currently, there are issues with enabling PSR-SU + DSC. This stems from > the fact that DSC imposes a slice height on transmitted video data and > we are not conforming to that slice height in PSR-SU regions. So, pass > slice_height into su_y_granularity to feed the DSC slice height into > PSR-SU code. > > Signed-off-by: Hamza Mahfooz Acked-by: Harry Wentland Would be good if you can get Leo's review. Harry > --- > v2: move code to modules/power. > v3: use ASSERT() instead of WARN() and add a condition that clarifies > that PSR-SU + DSC can only be enabled on an eDP connection. > v4: change the logic again to allow non-eDP links to pass the first > filter in psr_su_set_y_granularity() (this allows us to maintain > the v1/v2 behaviour, while still being clear as to the fact that we > only care about eDP connections). > --- > .../drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c | 3 ++ > .../amd/display/modules/power/power_helpers.c | 31 +++ > .../amd/display/modules/power/power_helpers.h | 3 ++ > 3 files changed, 37 insertions(+) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c > index 26291db0a3cf..872d06fe1436 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c > @@ -122,6 +122,9 @@ bool amdgpu_dm_link_setup_psr(struct dc_stream_state > *stream) > psr_config.allow_multi_disp_optimizations = > (amdgpu_dc_feature_mask & DC_PSR_ALLOW_MULTI_DISP_OPT); > > + if (!psr_su_set_y_granularity(dc, link, stream, &psr_config)) > + return false; > + > ret = dc_link_setup_psr(link, stream, &psr_config, > &psr_context); > > } > diff --git a/drivers/gpu/drm/amd/display/modules/power/power_helpers.c > b/drivers/gpu/drm/amd/display/modules/power/power_helpers.c > index 9b5d9b2c9a6a..cf4fa87c7db6 100644 > --- a/drivers/gpu/drm/amd/display/modules/power/power_helpers.c > +++ b/drivers/gpu/drm/amd/display/modules/power/power_helpers.c > @@ -916,3 +916,34 @@ bool mod_power_only_edp(const struct dc_state *context, > const struct dc_stream_s > { > return context && context->stream_count == 1 && > dc_is_embedded_signal(stream->signal); > } > + > +bool psr_su_set_y_granularity(struct dc *dc, struct dc_link *link, > + struct dc_stream_state *stream, > + struct psr_config *config) > +{ > + uint16_t pic_height; > + uint8_t slice_height; > + > + if ((link->connector_signal & SIGNAL_TYPE_EDP) && > + (!dc->caps.edp_dsc_support || > + link->panel_config.dsc.disable_dsc_edp || > + > !link->dpcd_caps.dsc_caps.dsc_basic_caps.fields.dsc_support.DSC_SUPPORT || > + !stream->timing.dsc_cfg.num_slices_v)) > + return true; > + > + pic_height = stream->timing.v_addressable + > + stream->timing.v_border_top + stream->timing.v_border_bottom; > + slice_height = pic_height / stream->timing.dsc_cfg.num_slices_v; > + > + if (slice_height) { > + if (config->su_y_granularity && > + (slice_height % config->su_y_granularity)) { > + ASSERT(0); > + return false; > + } > + > + config->su_y_granularity = slice_height; > + } > + > + return true; > +} > diff --git a/drivers/gpu/drm/amd/display/modules/power/power_helpers.h > b/drivers/gpu/drm/amd/display/modules/power/power_helpers.h > index 316452e9dbc9..bb16b37b83da 100644 > --- a/drivers/gpu/drm/amd/display/modules/power/power_helpers.h > +++ b/drivers/gpu/drm/amd/display/modules/power/power_helpers.h > @@ -59,4 +59,7 @@ void mod_power_calc_psr_configs(struct psr_config > *psr_config, > const struct dc_stream_state *stream); > bool mod_power_only_edp(const struct dc_state *context, > const struct dc_stream_state *stream); > +bool psr_su_set_y_granularity(struct dc *dc, struct dc_link *link, > + struct dc_stream_state *stream, > + struct psr_config *config); > #endif /* MODULES_POWER_POWER_HELPERS_H_ */
Re: [PATCH -next] drm/amd/display: Remove redundant assignment to variable dc
Applied. Thanks! Alex On Thu, Jan 5, 2023 at 2:20 PM Harry Wentland wrote: > > On 12/16/22 05:23, Yi Yang wrote: > > Smatch report warning as follows: > > > > Line 53679: drivers/gpu/drm/amd/display/dc/core/dc_stream.c:402 > > dc_stream_set_cursor_position() warn: variable dereferenced before > > check 'stream' > > > > The value of 'dc' has been assigned after check whether 'stream' is > > NULL. Fix it by remove redundant assignment. > > > > Signed-off-by: Yi Yang > > If this didn't blow up until now we might not even need > the NULL check below, but either way this is correct and > > Reviewed-by: Harry Wentland > > Harry > > > --- > > drivers/gpu/drm/amd/display/dc/core/dc_stream.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c > > b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c > > index 20e534f73513..78d31bb875d1 100644 > > --- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c > > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c > > @@ -408,7 +408,7 @@ bool dc_stream_set_cursor_position( > > struct dc_stream_state *stream, > > const struct dc_cursor_position *position) > > { > > - struct dc *dc = stream->ctx->dc; > > + struct dc *dc; > > bool reset_idle_optimizations = false; > > > > if (NULL == stream) { >
Re: [PATCH -next] drm/amd/display: Remove unneeded semicolon
Applied. Thanks! Alex On Thu, Jan 5, 2023 at 7:30 PM Yang Li wrote: > > ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:7431:3-4: Unneeded > semicolon > ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:7485:4-5: Unneeded > semicolon > ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:7489:3-4: Unneeded > semicolon > > Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=3635 > Reported-by: Abaci Robot > Signed-off-by: Yang Li > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index cabe02cb307c..90dc72e98eb2 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -7428,7 +7428,7 @@ static bool is_content_protection_different(struct > drm_crtc_state *new_crtc_stat > new_conn_state->content_protection = > DRM_MODE_CONTENT_PROTECTION_DESIRED; > pr_debug("[HDCP_DM] ENABLED->DESIRED & mode_changed > %s :true\n", __func__); > return true; > - }; > + } > new_conn_state->content_protection = > DRM_MODE_CONTENT_PROTECTION_ENABLED; > pr_debug("[HDCP_DM] ENABLED -> DESIRED %s :false\n", > __func__); > return false; > @@ -7482,11 +7482,11 @@ static bool is_content_protection_different(struct > drm_crtc_state *new_crtc_stat > pr_debug("[HDCP_DM] DESIRED->DESIRED or > ENABLE->ENABLE mode_change %s :true\n", > __func__); > return true; > - }; > + } > pr_debug("[HDCP_DM] DESIRED->DESIRED & ENABLE->ENABLE > %s :false\n", > __func__); > return false; > - }; > + } > > pr_debug("[HDCP_DM] UNDESIRED->UNDESIRED %s :false\n", > __func__); > return false; > -- > 2.20.1.7.g153144c >
Re: [PATCH -next] drm/amdgpu: clean up some inconsistent indentings
Applied. Thanks! Alex On Thu, Jan 5, 2023 at 8:37 PM Yang Li wrote: > > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:65 amdgpu_gem_fault() warn: > inconsistent indenting > > Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=3639 > Reported-by: Abaci Robot > Signed-off-by: Yang Li > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > index 00edc7002ee8..ed1164a87fce 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > @@ -62,10 +62,10 @@ static vm_fault_t amdgpu_gem_fault(struct vm_fault *vmf) > goto unlock; > } > > -ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot, > - TTM_BO_VM_NUM_PREFAULT); > + ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot, > + TTM_BO_VM_NUM_PREFAULT); > > -drm_dev_exit(idx); > + drm_dev_exit(idx); > } else { > ret = ttm_bo_vm_dummy_page(vmf, vmf->vma->vm_page_prot); > } > -- > 2.20.1.7.g153144c >
Re: [PATCH] drm/radeon: free iio for atombios when driver shutdown
Applied. Thanks! Alex On Fri, Jan 6, 2023 at 5:00 AM Liwei Song wrote: > > Fix below kmemleak when unload radeon driver: > > unreferenced object 0x9f8608ede200 (size 512): > comm "systemd-udevd", pid 326, jiffies 4294682822 (age 716.338s) > hex dump (first 32 bytes): > 00 00 00 00 c4 aa ec aa 14 ab 00 00 00 00 00 00 > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > backtrace: > [<62fadebe>] kmem_cache_alloc_trace+0x2f1/0x500 > [] atom_parse+0x117/0x230 [radeon] > [<158c23fd>] radeon_atombios_init+0xab/0x170 [radeon] > [<683f672e>] si_init+0x57/0x750 [radeon] > [<566cc31f>] radeon_device_init+0x559/0x9c0 [radeon] > [<46efabb3>] radeon_driver_load_kms+0xc1/0x1a0 [radeon] > [ ] drm_dev_register+0xdd/0x1d0 > [<45fec835>] radeon_pci_probe+0xbd/0x100 [radeon] > [ ] pci_device_probe+0xe1/0x160 > [<19484b76>] really_probe.part.0+0xc1/0x2c0 > [<3f2649da>] __driver_probe_device+0x96/0x130 > [<231c5bb1>] driver_probe_device+0x24/0xf0 > [<00a42377>] __driver_attach+0x77/0x190 > [ ] bus_for_each_dev+0x7f/0xd0 > [<633166d2>] driver_attach+0x1e/0x30 > [<313b05b8>] bus_add_driver+0x12c/0x1e0 > > iio was allocated in atom_index_iio() called by atom_parse(), > but it doesn't got released when the dirver is shutdown. > Fix this kmemleak by free it in radeon_atombios_fini(). > > Signed-off-by: Liwei Song > --- > drivers/gpu/drm/radeon/radeon_device.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/radeon/radeon_device.c > b/drivers/gpu/drm/radeon/radeon_device.c > index 92905ebb7b45..1c005e0ddd38 100644 > --- a/drivers/gpu/drm/radeon/radeon_device.c > +++ b/drivers/gpu/drm/radeon/radeon_device.c > @@ -1022,6 +1022,7 @@ void radeon_atombios_fini(struct radeon_device *rdev) > { > if (rdev->mode_info.atom_context) { > kfree(rdev->mode_info.atom_context->scratch); > + kfree(rdev->mode_info.atom_context->iio); > } > kfree(rdev->mode_info.atom_context); > rdev->mode_info.atom_context = NULL; > -- > 2.33.1 >
Re: [PATCH v6 03/11] dt-bindings: display/msm: add sm8350 and sm8450 DSI PHYs
On 07/12/2022 02:22, Dmitry Baryshkov wrote: > SM8350 and SM8450 platforms use the same driver and same bindings as the > existing 7nm DSI PHYs. Add corresponding compatibility strings. > > Acked-by: Krzysztof Kozlowski > Signed-off-by: Dmitry Baryshkov > --- > Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > index c851770bbdf2..bffd161fedfd 100644 > --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > @@ -15,6 +15,8 @@ allOf: > properties: >compatible: > enum: > + - qcom,dsi-phy-5nm-8350 > + - qcom,dsi-phy-5nm-8450 If this patch was not merged (so far nothing in next), can we make it proper SoC compatible? qcom,sm8450-dsi-phy-5nm The SC7280 already uses such pattern. >- qcom,dsi-phy-7nm >- qcom,dsi-phy-7nm-8150 >- qcom,sc7280-dsi-phy-7nm Best regards, Krzysztof
Re: [PATCH 1/6] dt-bindings: display/msm: document the SM8550 DSI PHY
On 04/01/2023 10:08, Neil Armstrong wrote: > Document the SM8550 DSI PHY which is very close from the 7nm > and 5nm DSI PHYs found in earlier platforms. > > Signed-off-by: Neil Armstrong > --- > Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > index bffd161fedfd..f72727f81076 100644 > --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml > @@ -15,6 +15,7 @@ allOf: > properties: >compatible: > enum: > + - qcom,dsi-phy-4nm-8550 >- qcom,dsi-phy-5nm-8350 >- qcom,dsi-phy-5nm-8450 Poor patterns once allowed like to keep growing... I commented here: https://lore.kernel.org/all/ccbb47e4-d780-0b1d-814e-27e86b6c3...@linaro.org/ so let's wait for response about other compatibles. >- qcom,dsi-phy-7nm > Best regards, Krzysztof
Re: [PATCH v6 03/11] dt-bindings: display/msm: add sm8350 and sm8450 DSI PHYs
On 06/01/2023 17:39, Krzysztof Kozlowski wrote: On 07/12/2022 02:22, Dmitry Baryshkov wrote: SM8350 and SM8450 platforms use the same driver and same bindings as the existing 7nm DSI PHYs. Add corresponding compatibility strings. Acked-by: Krzysztof Kozlowski Signed-off-by: Dmitry Baryshkov --- Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml index c851770bbdf2..bffd161fedfd 100644 --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml @@ -15,6 +15,8 @@ allOf: properties: compatible: enum: + - qcom,dsi-phy-5nm-8350 + - qcom,dsi-phy-5nm-8450 If this patch was not merged (so far nothing in next), can we make it proper SoC compatible? Ack. Bjorn has merged the dtsi bits, but I'll send a fixup. qcom,sm8450-dsi-phy-5nm The SC7280 already uses such pattern. - qcom,dsi-phy-7nm - qcom,dsi-phy-7nm-8150 - qcom,sc7280-dsi-phy-7nm Best regards, Krzysztof -- With best wishes Dmitry
Re: [PATCH] fbdev: Replace 0-length array with flexible array
On Thu, Jan 05, 2023 at 11:20:38AM -0800, Kees Cook wrote: > Zero-length arrays are deprecated[1]. Replace struct aperture's "ranges" > 0-length array with a flexible array. (How is the size of this array > verified?) Detected with GCC 13, using -fstrict-flex-arrays=3: > > samples/vfio-mdev/mdpy-fb.c: In function 'mdpy_fb_probe': > samples/vfio-mdev/mdpy-fb.c:169:32: warning: array subscript 0 is outside > array bounds of 'struct aperture[0]' [-Warray-bounds=] > 169 | info->apertures->ranges[0].base = info->fix.smem_start; > | ~~~^~~ > In file included from samples/vfio-mdev/mdpy-fb.c:21: > include/linux/fb.h:510:19: note: while referencing 'ranges' > 510 | } ranges[0]; > | ^~ > > [1] > https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays > > Cc: Helge Deller > Cc: "Gustavo A. R. Silva" > Cc: linux-fb...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Kees Cook Reviewed-by: Gustavo A. R. Silva Thanks! -- Gustavo > --- > include/linux/fb.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/fb.h b/include/linux/fb.h > index 96b96323e9cb..bf59d6a3590f 100644 > --- a/include/linux/fb.h > +++ b/include/linux/fb.h > @@ -507,7 +507,7 @@ struct fb_info { > struct aperture { > resource_size_t base; > resource_size_t size; > - } ranges[0]; > + } ranges[]; > } *apertures; > > bool skip_vt_switch; /* no VT switch on suspend/resume required */ > -- > 2.34.1 >
Re: [PATCH v2] drm/i915: Do not cover all future platforms in TLB invalidation
On Fri, Jan 06, 2023 at 10:38:35AM +, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > Revert to the original explicit approach and document the reasoning > behind it. > > v2: > * DG2 needs to be covered too. (Matt) > > Signed-off-by: Tvrtko Ursulin > Cc: Matt Roper > Cc: Balasubramani Vivekanandan > Cc: Andrzej Hajda > Reviewed-by: Andrzej Hajda # v1 > --- > Matt, does DG1 need to be in the MCR branch or plain Gen12? DG1 should use the same "gen12" branch as TGL/RKL/ADL. Bspec page 66696 is the relevant MMIO table for DG1 and the range containing the TLB invalidation registers is not a multicast/replicated range. The types of engines supported, and the register details for each engine are also the same as TGL/RKL/ADL. > --- > drivers/gpu/drm/i915/gt/intel_gt.c | 14 +- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c > b/drivers/gpu/drm/i915/gt/intel_gt.c > index 75a7cb33..b2556a3d8a3f 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > @@ -1070,7 +1070,19 @@ static void mmio_invalidate_full(struct intel_gt *gt) > unsigned int num = 0; > unsigned long flags; > > - if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) { > + /* > + * New platforms should not be added with catch-all-newer (>=) > + * condition so that any later platform added triggers the below warning > + * and in turn mandates a human cross-check of whether the invalidation > + * flows have compatible semantics. > + * > + * For instance with the 11.00 -> 12.00 transition three out of five > + * respective engine registers were moved to masked type. Then after the > + * 12.00 -> 12.50 transition multi cast handling is required too. > + */ > + > + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) && > + GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) { > regs = NULL; > num = ARRAY_SIZE(xehp_regs); > } else if (GRAPHICS_VER(i915) == 12) { Did you want to switch this one to GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) || GRAPHICS_VER_FULL(i915) == IP_VER(12, 10) so that it won't automatically pick up future 12.xx platforms like PVC, MTL, and whatever else might show up in that category in the future? Matt > -- > 2.34.1 > -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
[PATCH 5/8] iommufd: Use GFP_KERNEL_ACCOUNT for iommu_map()
iommufd follows the same design as KVM and uses memory cgroups to limit the amount of kernel memory a iommufd file descriptor can pin down. The various internal data structures already use GFP_KERNEL_ACCOUNT. However, one of the biggest consumers of kernel memory is the IOPTEs stored under the iommu_domain. Many drivers will allocate these at iommu_map() time and will trivially do the right thing if we pass in GFP_KERNEL_ACCOUNT. Signed-off-by: Jason Gunthorpe --- drivers/iommu/iommufd/pages.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c index 22cc3bb0c6c55a..f8d92c9bb65b60 100644 --- a/drivers/iommu/iommufd/pages.c +++ b/drivers/iommu/iommufd/pages.c @@ -457,7 +457,7 @@ static int batch_iommu_map_small(struct iommu_domain *domain, while (size) { rc = iommu_map(domain, iova, paddr, PAGE_SIZE, prot, - GFP_KERNEL); + GFP_KERNEL_ACCOUNT); if (rc) goto err_unmap; iova += PAGE_SIZE; @@ -502,7 +502,7 @@ static int batch_to_domain(struct pfn_batch *batch, struct iommu_domain *domain, rc = iommu_map(domain, iova, PFN_PHYS(batch->pfns[cur]) + page_offset, next_iova - iova, area->iommu_prot, - GFP_KERNEL); + GFP_KERNEL_ACCOUNT); if (rc) goto err_unmap; iova = next_iova; -- 2.39.0
[PATCH 0/8] Let iommufd charge IOPTE allocations to the memory cgroup
iommufd follows the same design as KVM and uses memory cgroups to limit the amount of kernel memory a iommufd file descriptor can pin down. The various internal data structures already use GFP_KERNEL_ACCOUNT to charge its own memory. However, one of the biggest consumers of kernel memory is the IOPTEs stored under the iommu_domain and these allocations are not tracked. This series is the first step in fixing it. The iommu driver contract already includes a 'gfp' argument to the map_pages op, allowing iommufd to specify GFP_KERNEL_ACCOUNT and then having the driver allocate the IOPTE tables with that flag will capture a significant amount of the allocations. Update the iommu_map() API to pass in the GFP argument, and fix all call sites. Replace iommu_map_atomic(). Audit the "enterprise" iommu drivers to make sure they do the right thing. Intel and S390 ignore the GFP argument and always use GFP_ATOMIC. This is problematic for iommufd anyhow, so fix it. AMD and ARM SMMUv2/3 are already correct. A follow up series will be needed to capture the allocations made when the iommu_domain itself is allocated, which will complete the job. Jason Gunthorpe (8): iommu: Add a gfp parameter to iommu_map() iommu: Remove iommu_map_atomic() iommu: Add a gfp parameter to iommu_map_sg() iommu/dma: Use the gfp parameter in __iommu_dma_alloc_noncontiguous() iommufd: Use GFP_KERNEL_ACCOUNT for iommu_map() iommu/intel: Add a gfp parameter to alloc_pgtable_page() iommu/intel: Support the gfp argument to the map_pages op iommu/s390: Push the gfp parameter to the kmem_cache_alloc()'s arch/arm/mm/dma-mapping.c | 11 +++-- arch/s390/include/asm/pci_dma.h | 5 ++- arch/s390/pci/pci_dma.c | 31 +++-- .../drm/nouveau/nvkm/subdev/instmem/gk20a.c | 3 +- drivers/gpu/drm/tegra/drm.c | 2 +- drivers/gpu/host1x/cdma.c | 2 +- drivers/infiniband/hw/usnic/usnic_uiom.c | 4 +- drivers/iommu/dma-iommu.c | 11 ++--- drivers/iommu/intel/iommu.c | 36 +--- drivers/iommu/intel/iommu.h | 2 +- drivers/iommu/intel/pasid.c | 2 +- drivers/iommu/iommu.c | 43 +-- drivers/iommu/iommufd/pages.c | 6 ++- drivers/iommu/s390-iommu.c| 15 --- drivers/media/platform/qcom/venus/firmware.c | 2 +- drivers/net/ipa/ipa_mem.c | 6 ++- drivers/net/wireless/ath/ath10k/snoc.c| 2 +- drivers/net/wireless/ath/ath11k/ahb.c | 4 +- drivers/remoteproc/remoteproc_core.c | 5 ++- drivers/vfio/vfio_iommu_type1.c | 9 ++-- drivers/vhost/vdpa.c | 2 +- include/linux/iommu.h | 31 +++-- 22 files changed, 109 insertions(+), 125 deletions(-) base-commit: 88603b6dc419445847923fcb7fe5080067a30f98 -- 2.39.0
[PATCH 3/8] iommu: Add a gfp parameter to iommu_map_sg()
Follow the pattern for iommu_map() and remove iommu_map_sg_atomic(). This allows __iommu_dma_alloc_noncontiguous() to use a GFP_KERNEL allocation here, based on the provided gfp flags. Signed-off-by: Jason Gunthorpe --- drivers/iommu/dma-iommu.c | 5 +++-- drivers/iommu/iommu.c | 21 + include/linux/iommu.h | 18 +- 3 files changed, 13 insertions(+), 31 deletions(-) diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 7016db569f81fc..8c2788633c1766 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -833,7 +833,8 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev, arch_dma_prep_coherent(sg_page(sg), sg->length); } - ret = iommu_map_sg_atomic(domain, iova, sgt->sgl, sgt->orig_nents, ioprot); + ret = iommu_map_sg(domain, iova, sgt->sgl, sgt->orig_nents, ioprot, + gfp); if (ret < 0 || ret < size) goto out_free_sg; @@ -1281,7 +1282,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, * We'll leave any physical concatenation to the IOMMU driver's * implementation - it knows better than we do. */ - ret = iommu_map_sg_atomic(domain, iova, sg, nents, prot); + ret = iommu_map_sg(domain, iova, sg, nents, prot, GFP_ATOMIC); if (ret < 0 || ret < iova_len) goto out_free_iova; diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index fee37bb246f3ea..11fb3981e25642 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2465,9 +2465,9 @@ size_t iommu_unmap_fast(struct iommu_domain *domain, } EXPORT_SYMBOL_GPL(iommu_unmap_fast); -static ssize_t __iommu_map_sg(struct iommu_domain *domain, unsigned long iova, - struct scatterlist *sg, unsigned int nents, int prot, - gfp_t gfp) +ssize_t iommu_map_sg(struct iommu_domain *domain, unsigned long iova, +struct scatterlist *sg, unsigned int nents, int prot, +gfp_t gfp) { const struct iommu_domain_ops *ops = domain->ops; size_t len = 0, mapped = 0; @@ -2475,6 +2475,8 @@ static ssize_t __iommu_map_sg(struct iommu_domain *domain, unsigned long iova, unsigned int i = 0; int ret; + might_sleep_if(gfpflags_allow_blocking(gfp)); + while (i <= nents) { phys_addr_t s_phys = sg_phys(sg); @@ -2514,21 +2516,8 @@ static ssize_t __iommu_map_sg(struct iommu_domain *domain, unsigned long iova, return ret; } - -ssize_t iommu_map_sg(struct iommu_domain *domain, unsigned long iova, -struct scatterlist *sg, unsigned int nents, int prot) -{ - might_sleep(); - return __iommu_map_sg(domain, iova, sg, nents, prot, GFP_KERNEL); -} EXPORT_SYMBOL_GPL(iommu_map_sg); -ssize_t iommu_map_sg_atomic(struct iommu_domain *domain, unsigned long iova, - struct scatterlist *sg, unsigned int nents, int prot) -{ - return __iommu_map_sg(domain, iova, sg, nents, prot, GFP_ATOMIC); -} - /** * report_iommu_fault() - report about an IOMMU fault to the IOMMU framework * @domain: the iommu domain where the fault has happened diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 521cd79700f4d8..d5c16dc33c87de 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -474,10 +474,8 @@ extern size_t iommu_unmap_fast(struct iommu_domain *domain, unsigned long iova, size_t size, struct iommu_iotlb_gather *iotlb_gather); extern ssize_t iommu_map_sg(struct iommu_domain *domain, unsigned long iova, - struct scatterlist *sg, unsigned int nents, int prot); -extern ssize_t iommu_map_sg_atomic(struct iommu_domain *domain, - unsigned long iova, struct scatterlist *sg, - unsigned int nents, int prot); + struct scatterlist *sg, unsigned int nents, + int prot, gfp_t gfp); extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova); extern void iommu_set_fault_handler(struct iommu_domain *domain, iommu_fault_handler_t handler, void *token); @@ -791,14 +789,7 @@ static inline size_t iommu_unmap_fast(struct iommu_domain *domain, static inline ssize_t iommu_map_sg(struct iommu_domain *domain, unsigned long iova, struct scatterlist *sg, - unsigned int nents, int prot) -{ - return -ENODEV; -} - -static inline ssize_t iommu_map_sg_atomic(struct iommu_domain *domain, - unsigned long iova, struct scatterlist *sg, - unsigned int nents, int prot) + unsigned int nents, int prot, gfp_t gfp) {
[PATCH 4/8] iommu/dma: Use the gfp parameter in __iommu_dma_alloc_noncontiguous()
Change the sg_alloc_table_from_pages() allocation that was hardwired to GFP_KERNEL to use the gfp parameter like the other allocations in this function. Auditing says this is never called from an atomic context, so it is safe as is, but reads wrong. Signed-off-by: Jason Gunthorpe --- drivers/iommu/dma-iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 8c2788633c1766..e4bf1bb159f7c7 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -822,7 +822,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev, if (!iova) goto out_free_pages; - if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, GFP_KERNEL)) + if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, gfp)) goto out_free_iova; if (!(ioprot & IOMMU_CACHE)) { -- 2.39.0
[PATCH 8/8] iommu/s390: Push the gfp parameter to the kmem_cache_alloc()'s
dma_alloc_cpu_table() and dma_alloc_page_table() are eventually called by iommufd through s390_iommu_map_pages() and it should not be forced to atomic. Thread the gfp parameter through the call chain starting from s390_iommu_map_pages(). Signed-off-by: Jason Gunthorpe --- arch/s390/include/asm/pci_dma.h | 5 +++-- arch/s390/pci/pci_dma.c | 31 +-- drivers/iommu/s390-iommu.c | 15 +-- 3 files changed, 29 insertions(+), 22 deletions(-) diff --git a/arch/s390/include/asm/pci_dma.h b/arch/s390/include/asm/pci_dma.h index 91e63426bdc53f..7119c04c51c5c8 100644 --- a/arch/s390/include/asm/pci_dma.h +++ b/arch/s390/include/asm/pci_dma.h @@ -186,9 +186,10 @@ static inline unsigned long *get_st_pto(unsigned long entry) /* Prototypes */ void dma_free_seg_table(unsigned long); -unsigned long *dma_alloc_cpu_table(void); +unsigned long *dma_alloc_cpu_table(gfp_t gfp); void dma_cleanup_tables(unsigned long *); -unsigned long *dma_walk_cpu_trans(unsigned long *rto, dma_addr_t dma_addr); +unsigned long *dma_walk_cpu_trans(unsigned long *rto, dma_addr_t dma_addr, + gfp_t gfp); void dma_update_cpu_trans(unsigned long *entry, phys_addr_t page_addr, int flags); extern const struct dma_map_ops s390_pci_dma_ops; diff --git a/arch/s390/pci/pci_dma.c b/arch/s390/pci/pci_dma.c index ea478d11fbd132..2d9b01d7ca4c5c 100644 --- a/arch/s390/pci/pci_dma.c +++ b/arch/s390/pci/pci_dma.c @@ -27,11 +27,11 @@ static int zpci_refresh_global(struct zpci_dev *zdev) zdev->iommu_pages * PAGE_SIZE); } -unsigned long *dma_alloc_cpu_table(void) +unsigned long *dma_alloc_cpu_table(gfp_t gfp) { unsigned long *table, *entry; - table = kmem_cache_alloc(dma_region_table_cache, GFP_ATOMIC); + table = kmem_cache_alloc(dma_region_table_cache, gfp); if (!table) return NULL; @@ -45,11 +45,11 @@ static void dma_free_cpu_table(void *table) kmem_cache_free(dma_region_table_cache, table); } -static unsigned long *dma_alloc_page_table(void) +static unsigned long *dma_alloc_page_table(gfp_t gfp) { unsigned long *table, *entry; - table = kmem_cache_alloc(dma_page_table_cache, GFP_ATOMIC); + table = kmem_cache_alloc(dma_page_table_cache, gfp); if (!table) return NULL; @@ -63,7 +63,7 @@ static void dma_free_page_table(void *table) kmem_cache_free(dma_page_table_cache, table); } -static unsigned long *dma_get_seg_table_origin(unsigned long *rtep) +static unsigned long *dma_get_seg_table_origin(unsigned long *rtep, gfp_t gfp) { unsigned long old_rte, rte; unsigned long *sto; @@ -72,7 +72,7 @@ static unsigned long *dma_get_seg_table_origin(unsigned long *rtep) if (reg_entry_isvalid(rte)) { sto = get_rt_sto(rte); } else { - sto = dma_alloc_cpu_table(); + sto = dma_alloc_cpu_table(gfp); if (!sto) return NULL; @@ -90,7 +90,7 @@ static unsigned long *dma_get_seg_table_origin(unsigned long *rtep) return sto; } -static unsigned long *dma_get_page_table_origin(unsigned long *step) +static unsigned long *dma_get_page_table_origin(unsigned long *step, gfp_t gfp) { unsigned long old_ste, ste; unsigned long *pto; @@ -99,7 +99,7 @@ static unsigned long *dma_get_page_table_origin(unsigned long *step) if (reg_entry_isvalid(ste)) { pto = get_st_pto(ste); } else { - pto = dma_alloc_page_table(); + pto = dma_alloc_page_table(gfp); if (!pto) return NULL; set_st_pto(&ste, virt_to_phys(pto)); @@ -116,18 +116,19 @@ static unsigned long *dma_get_page_table_origin(unsigned long *step) return pto; } -unsigned long *dma_walk_cpu_trans(unsigned long *rto, dma_addr_t dma_addr) +unsigned long *dma_walk_cpu_trans(unsigned long *rto, dma_addr_t dma_addr, + gfp_t gfp) { unsigned long *sto, *pto; unsigned int rtx, sx, px; rtx = calc_rtx(dma_addr); - sto = dma_get_seg_table_origin(&rto[rtx]); + sto = dma_get_seg_table_origin(&rto[rtx], gfp); if (!sto) return NULL; sx = calc_sx(dma_addr); - pto = dma_get_page_table_origin(&sto[sx]); + pto = dma_get_page_table_origin(&sto[sx], gfp); if (!pto) return NULL; @@ -170,7 +171,8 @@ static int __dma_update_trans(struct zpci_dev *zdev, phys_addr_t pa, return -EINVAL; for (i = 0; i < nr_pages; i++) { - entry = dma_walk_cpu_trans(zdev->dma_table, dma_addr); + entry = dma_walk_cpu_trans(zdev->dma_table, dma_addr, + GFP_ATOMIC); if (!entry) { rc = -ENOMEM;
[PATCH 1/8] iommu: Add a gfp parameter to iommu_map()
The internal mechanisms support this, but instead of exposting the gfp to the caller it wrappers it into iommu_map() and iommu_map_atomic() Fix this instead of adding more variants for GFP_KERNEL_ACCOUNT. Signed-off-by: Jason Gunthorpe --- arch/arm/mm/dma-mapping.c | 11 +++ .../gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c | 3 ++- drivers/gpu/drm/tegra/drm.c | 2 +- drivers/gpu/host1x/cdma.c | 2 +- drivers/infiniband/hw/usnic/usnic_uiom.c| 4 ++-- drivers/iommu/dma-iommu.c | 2 +- drivers/iommu/iommu.c | 17 ++--- drivers/iommu/iommufd/pages.c | 6 -- drivers/media/platform/qcom/venus/firmware.c| 2 +- drivers/net/ipa/ipa_mem.c | 6 -- drivers/net/wireless/ath/ath10k/snoc.c | 2 +- drivers/net/wireless/ath/ath11k/ahb.c | 4 ++-- drivers/remoteproc/remoteproc_core.c| 5 +++-- drivers/vfio/vfio_iommu_type1.c | 9 + drivers/vhost/vdpa.c| 2 +- include/linux/iommu.h | 4 ++-- 16 files changed, 43 insertions(+), 38 deletions(-) diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index c135f6e37a00ca..8bc01071474ab7 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -984,7 +984,8 @@ __iommu_create_mapping(struct device *dev, struct page **pages, size_t size, len = (j - i) << PAGE_SHIFT; ret = iommu_map(mapping->domain, iova, phys, len, - __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs)); + __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs), + GFP_KERNEL); if (ret < 0) goto fail; iova += len; @@ -1207,7 +1208,8 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, prot = __dma_info_to_prot(dir, attrs); - ret = iommu_map(mapping->domain, iova, phys, len, prot); + ret = iommu_map(mapping->domain, iova, phys, len, prot, + GFP_KERNEL); if (ret < 0) goto fail; count += len >> PAGE_SHIFT; @@ -1379,7 +1381,8 @@ static dma_addr_t arm_iommu_map_page(struct device *dev, struct page *page, prot = __dma_info_to_prot(dir, attrs); - ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot); + ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, + prot, GFP_KERNEL); if (ret < 0) goto fail; @@ -1443,7 +1446,7 @@ static dma_addr_t arm_iommu_map_resource(struct device *dev, prot = __dma_info_to_prot(dir, attrs) | IOMMU_MMIO; - ret = iommu_map(mapping->domain, dma_addr, addr, len, prot); + ret = iommu_map(mapping->domain, dma_addr, addr, len, prot, GFP_KERNEL); if (ret < 0) goto fail; diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c index 648ecf5a8fbc2a..a4ac94a2ab57fc 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c @@ -475,7 +475,8 @@ gk20a_instobj_ctor_iommu(struct gk20a_instmem *imem, u32 npages, u32 align, u32 offset = (r->offset + i) << imem->iommu_pgshift; ret = iommu_map(imem->domain, offset, node->dma_addrs[i], - PAGE_SIZE, IOMMU_READ | IOMMU_WRITE); + PAGE_SIZE, IOMMU_READ | IOMMU_WRITE, + GFP_KERNEL); if (ret < 0) { nvkm_error(subdev, "IOMMU mapping failure: %d\n", ret); diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 7bd2e65c2a16c5..6ca9f396e55be4 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -1057,7 +1057,7 @@ void *tegra_drm_alloc(struct tegra_drm *tegra, size_t size, dma_addr_t *dma) *dma = iova_dma_addr(&tegra->carveout.domain, alloc); err = iommu_map(tegra->domain, *dma, virt_to_phys(virt), - size, IOMMU_READ | IOMMU_WRITE); + size, IOMMU_READ | IOMMU_WRITE, GFP_KERNEL); if (err < 0) goto free_iova; diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c index 103fda055394ab..4ddfcd2138c95b 100644 --- a/drivers/gpu/host1x/cdma.c +++ b/drivers/gpu/host1x/cdma.c @@ -105,7 +105,7 @@ static int host1x_pushbuffer_init(struct push_buffer *pb) pb->dma = iova_dma_addr(&host1x->iova, alloc); err = iommu_map(host1x->domain, pb->dma, pb->phys, size, - IOMMU_READ); +
[PATCH 6/8] iommu/intel: Add a gfp parameter to alloc_pgtable_page()
This is eventually called by iommufd through intel_iommu_map_pages() and it should not be forced to atomic. Push the GFP_ATOMIC to all callers. Signed-off-by: Jason Gunthorpe --- drivers/iommu/intel/iommu.c | 14 +++--- drivers/iommu/intel/iommu.h | 2 +- drivers/iommu/intel/pasid.c | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 59df7e42fd533c..e3807776971563 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -362,12 +362,12 @@ static int __init intel_iommu_setup(char *str) } __setup("intel_iommu=", intel_iommu_setup); -void *alloc_pgtable_page(int node) +void *alloc_pgtable_page(int node, gfp_t gfp) { struct page *page; void *vaddr = NULL; - page = alloc_pages_node(node, GFP_ATOMIC | __GFP_ZERO, 0); + page = alloc_pages_node(node, gfp | __GFP_ZERO, 0); if (page) vaddr = page_address(page); return vaddr; @@ -612,7 +612,7 @@ struct context_entry *iommu_context_addr(struct intel_iommu *iommu, u8 bus, if (!alloc) return NULL; - context = alloc_pgtable_page(iommu->node); + context = alloc_pgtable_page(iommu->node, GFP_ATOMIC); if (!context) return NULL; @@ -935,7 +935,7 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain, if (!dma_pte_present(pte)) { uint64_t pteval; - tmp_page = alloc_pgtable_page(domain->nid); + tmp_page = alloc_pgtable_page(domain->nid, GFP_ATOMIC); if (!tmp_page) return NULL; @@ -1186,7 +1186,7 @@ static int iommu_alloc_root_entry(struct intel_iommu *iommu) { struct root_entry *root; - root = (struct root_entry *)alloc_pgtable_page(iommu->node); + root = (struct root_entry *)alloc_pgtable_page(iommu->node, GFP_ATOMIC); if (!root) { pr_err("Allocating root entry for %s failed\n", iommu->name); @@ -2676,7 +2676,7 @@ static int copy_context_table(struct intel_iommu *iommu, if (!old_ce) goto out; - new_ce = alloc_pgtable_page(iommu->node); + new_ce = alloc_pgtable_page(iommu->node, GFP_KERNEL); if (!new_ce) goto out_unmap; @@ -4136,7 +4136,7 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width) domain->max_addr = 0; /* always allocate the top pgd */ - domain->pgd = alloc_pgtable_page(domain->nid); + domain->pgd = alloc_pgtable_page(domain->nid, GFP_ATOMIC); if (!domain->pgd) return -ENOMEM; domain_flush_cache(domain, domain->pgd, PAGE_SIZE); diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h index 06e61e4748567a..ca9a035e0110af 100644 --- a/drivers/iommu/intel/iommu.h +++ b/drivers/iommu/intel/iommu.h @@ -737,7 +737,7 @@ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc, extern int dmar_ir_support(void); -void *alloc_pgtable_page(int node); +void *alloc_pgtable_page(int node, gfp_t gfp); void free_pgtable_page(void *vaddr); void iommu_flush_write_buffer(struct intel_iommu *iommu); struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn); diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index fb3c7020028d07..c5bf74e9372d62 100644 --- a/drivers/iommu/intel/pasid.c +++ b/drivers/iommu/intel/pasid.c @@ -200,7 +200,7 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid) retry: entries = get_pasid_table_from_pde(&dir[dir_index]); if (!entries) { - entries = alloc_pgtable_page(info->iommu->node); + entries = alloc_pgtable_page(info->iommu->node, GFP_ATOMIC); if (!entries) return NULL; -- 2.39.0
[PATCH 7/8] iommu/intel: Support the gfp argument to the map_pages op
Flow it down to alloc_pgtable_page() via pfn_to_dma_pte() and __domain_mapping(). Signed-off-by: Jason Gunthorpe --- drivers/iommu/intel/iommu.c | 24 +++- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index e3807776971563..a1a66798e1f06c 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -908,7 +908,8 @@ void dmar_fault_dump_ptes(struct intel_iommu *iommu, u16 source_id, #endif static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain, - unsigned long pfn, int *target_level) + unsigned long pfn, int *target_level, + gfp_t gfp) { struct dma_pte *parent, *pte; int level = agaw_to_level(domain->agaw); @@ -935,7 +936,7 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain, if (!dma_pte_present(pte)) { uint64_t pteval; - tmp_page = alloc_pgtable_page(domain->nid, GFP_ATOMIC); + tmp_page = alloc_pgtable_page(domain->nid, gfp); if (!tmp_page) return NULL; @@ -2150,7 +2151,8 @@ static void switch_to_super_page(struct dmar_domain *domain, while (start_pfn <= end_pfn) { if (!pte) - pte = pfn_to_dma_pte(domain, start_pfn, &level); + pte = pfn_to_dma_pte(domain, start_pfn, &level, +GFP_ATOMIC); if (dma_pte_present(pte)) { dma_pte_free_pagetable(domain, start_pfn, @@ -2172,7 +2174,8 @@ static void switch_to_super_page(struct dmar_domain *domain, static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn, -unsigned long phys_pfn, unsigned long nr_pages, int prot) +unsigned long phys_pfn, unsigned long nr_pages, int prot, +gfp_t gfp) { struct dma_pte *first_pte = NULL, *pte = NULL; unsigned int largepage_lvl = 0; @@ -2202,7 +2205,8 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn, largepage_lvl = hardware_largepage_caps(domain, iov_pfn, phys_pfn, nr_pages); - pte = pfn_to_dma_pte(domain, iov_pfn, &largepage_lvl); + pte = pfn_to_dma_pte(domain, iov_pfn, &largepage_lvl, +gfp); if (!pte) return -ENOMEM; first_pte = pte; @@ -2368,7 +2372,7 @@ static int iommu_domain_identity_map(struct dmar_domain *domain, return __domain_mapping(domain, first_vpfn, first_vpfn, last_vpfn - first_vpfn + 1, - DMA_PTE_READ|DMA_PTE_WRITE); + DMA_PTE_READ|DMA_PTE_WRITE, GFP_KERNEL); } static int md_domain_init(struct dmar_domain *domain, int guest_width); @@ -4298,7 +4302,7 @@ static int intel_iommu_map(struct iommu_domain *domain, the low bits of hpa would take us onto the next page */ size = aligned_nrpages(hpa, size); return __domain_mapping(dmar_domain, iova >> VTD_PAGE_SHIFT, - hpa >> VTD_PAGE_SHIFT, size, prot); + hpa >> VTD_PAGE_SHIFT, size, prot, gfp); } static int intel_iommu_map_pages(struct iommu_domain *domain, @@ -4333,7 +4337,8 @@ static size_t intel_iommu_unmap(struct iommu_domain *domain, /* Cope with horrid API which requires us to unmap more than the size argument if it happens to be a large-page mapping. */ - BUG_ON(!pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &level)); + BUG_ON(!pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &level, + GFP_ATOMIC)); if (size < VTD_PAGE_SIZE << level_to_offset_bits(level)) size = VTD_PAGE_SIZE << level_to_offset_bits(level); @@ -4392,7 +4397,8 @@ static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain *domain, int level = 0; u64 phys = 0; - pte = pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &level); + pte = pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &level, +GFP_ATOMIC); if (pte && dma_pte_present(pte)) phys = dma_pte_addr(pte) + (iova & (BIT_MASK(level_to_offset_bits(level) + -- 2.39.0
[PATCH 2/8] iommu: Remove iommu_map_atomic()
There is only one call site and it can now just pass the GFP_ATOMIC to the normal iommu_map(). Signed-off-by: Jason Gunthorpe --- drivers/iommu/dma-iommu.c | 2 +- drivers/iommu/iommu.c | 7 --- include/linux/iommu.h | 9 - 3 files changed, 1 insertion(+), 17 deletions(-) diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 8bdb65e7686ff9..7016db569f81fc 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -713,7 +713,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys, if (!iova) return DMA_MAPPING_ERROR; - if (iommu_map_atomic(domain, iova, phys - iova_off, size, prot)) { + if (iommu_map(domain, iova, phys - iova_off, size, prot, GFP_ATOMIC)) { iommu_dma_free_iova(cookie, iova, size, NULL); return DMA_MAPPING_ERROR; } diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index fe29fc2140b132..fee37bb246f3ea 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2376,13 +2376,6 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova, } EXPORT_SYMBOL_GPL(iommu_map); -int iommu_map_atomic(struct iommu_domain *domain, unsigned long iova, - phys_addr_t paddr, size_t size, int prot) -{ - return iommu_map(domain, iova, paddr, size, prot, GFP_ATOMIC); -} -EXPORT_SYMBOL_GPL(iommu_map_atomic); - static size_t __iommu_unmap_pages(struct iommu_domain *domain, unsigned long iova, size_t size, struct iommu_iotlb_gather *iotlb_gather) diff --git a/include/linux/iommu.h b/include/linux/iommu.h index d2020994f292db..521cd79700f4d8 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -468,8 +468,6 @@ extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev); extern struct iommu_domain *iommu_get_dma_domain(struct device *dev); extern int iommu_map(struct iommu_domain *domain, unsigned long iova, phys_addr_t paddr, size_t size, int prot, gfp_t gfp); -extern int iommu_map_atomic(struct iommu_domain *domain, unsigned long iova, - phys_addr_t paddr, size_t size, int prot); extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size); extern size_t iommu_unmap_fast(struct iommu_domain *domain, @@ -778,13 +776,6 @@ static inline int iommu_map(struct iommu_domain *domain, unsigned long iova, return -ENODEV; } -static inline int iommu_map_atomic(struct iommu_domain *domain, - unsigned long iova, phys_addr_t paddr, - size_t size, int prot) -{ - return -ENODEV; -} - static inline size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) { -- 2.39.0
[PATCH RESEND 2/4] backlight: arcxcnn: Use backlight helper
Instead of retrieving the backlight brightness in struct backlight_properties manually, and then checking whether the backlight should be on at all, use backlight_get_brightness() which does all this and insulates this from future changes. Signed-off-by: Stephen Kitt --- drivers/video/backlight/arcxcnn_bl.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/video/backlight/arcxcnn_bl.c b/drivers/video/backlight/arcxcnn_bl.c index 555b036643fb..e610d7a1d13d 100644 --- a/drivers/video/backlight/arcxcnn_bl.c +++ b/drivers/video/backlight/arcxcnn_bl.c @@ -130,12 +130,9 @@ static int arcxcnn_set_brightness(struct arcxcnn *lp, u32 brightness) static int arcxcnn_bl_update_status(struct backlight_device *bl) { struct arcxcnn *lp = bl_get_data(bl); - u32 brightness = bl->props.brightness; + u32 brightness = backlight_get_brightness(bl); int ret; - if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK)) - brightness = 0; - ret = arcxcnn_set_brightness(lp, brightness); if (ret) return ret; -- 2.30.2
[PATCH RESEND 4/4] backlight: tosa: Use backlight helper
Instead of retrieving the backlight brightness in struct backlight_properties manually, and then checking whether the backlight should be on at all, use backlight_get_brightness() which does all this and insulates this from future changes. Signed-off-by: Stephen Kitt --- drivers/video/backlight/tosa_bl.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/video/backlight/tosa_bl.c b/drivers/video/backlight/tosa_bl.c index 77b71f6c19b5..e338b1f00f6a 100644 --- a/drivers/video/backlight/tosa_bl.c +++ b/drivers/video/backlight/tosa_bl.c @@ -50,13 +50,8 @@ static void tosa_bl_set_backlight(struct tosa_bl_data *data, int brightness) static int tosa_bl_update_status(struct backlight_device *dev) { - struct backlight_properties *props = &dev->props; struct tosa_bl_data *data = bl_get_data(dev); - int power = max(props->power, props->fb_blank); - int brightness = props->brightness; - - if (power) - brightness = 0; + int brightness = backlight_get_brightness(dev); tosa_bl_set_backlight(data, brightness); -- 2.30.2
[PATCH RESEND 1/4] backlight: aat2870: Use backlight helper
Instead of retrieving the backlight brightness in struct backlight_properties manually, and then checking whether the backlight should be on at all, use backlight_get_brightness() which does all this and insulates this from future changes. Signed-off-by: Stephen Kitt --- drivers/video/backlight/aat2870_bl.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/video/backlight/aat2870_bl.c b/drivers/video/backlight/aat2870_bl.c index a7af9adafad6..1cbb303e9c88 100644 --- a/drivers/video/backlight/aat2870_bl.c +++ b/drivers/video/backlight/aat2870_bl.c @@ -59,7 +59,7 @@ static int aat2870_bl_update_status(struct backlight_device *bd) struct aat2870_bl_driver_data *aat2870_bl = bl_get_data(bd); struct aat2870_data *aat2870 = dev_get_drvdata(aat2870_bl->pdev->dev.parent); - int brightness = bd->props.brightness; + int brightness = backlight_get_brightness(bd); int ret; if ((brightness < 0) || (bd->props.max_brightness < brightness)) { @@ -70,11 +70,6 @@ static int aat2870_bl_update_status(struct backlight_device *bd) dev_dbg(&bd->dev, "brightness=%d, power=%d, state=%d\n", bd->props.brightness, bd->props.power, bd->props.state); - if ((bd->props.power != FB_BLANK_UNBLANK) || - (bd->props.state & BL_CORE_FBBLANK) || - (bd->props.state & BL_CORE_SUSPENDED)) - brightness = 0; - ret = aat2870->write(aat2870, AAT2870_BLM, (u8)aat2870_brightness(aat2870_bl, brightness)); if (ret < 0) -- 2.30.2
Re: [PATCH 1/8] iommu: Add a gfp parameter to iommu_map()
On 2023-01-06 16:42, Jason Gunthorpe wrote: The internal mechanisms support this, but instead of exposting the gfp to the caller it wrappers it into iommu_map() and iommu_map_atomic() Fix this instead of adding more variants for GFP_KERNEL_ACCOUNT. FWIW, since we *do* have two variants already, I think I'd have a mild preference for leaving the regular map calls as-is (i.e. implicit GFP_KERNEL), and just generalising the _atomic versions for the special cases. However, echoing the recent activity over on the DMA API side of things, I think it's still worth proactively constraining the set of permissible flags, lest we end up with more weird problems if stuff that doesn't really make sense, like GFP_COMP or zone flags, manages to leak through (that may have been part of the reason for having the current wrappers rather than a bare gfp argument in the first place, I forget now). Thanks, Robin. Signed-off-by: Jason Gunthorpe --- arch/arm/mm/dma-mapping.c | 11 +++ .../gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c | 3 ++- drivers/gpu/drm/tegra/drm.c | 2 +- drivers/gpu/host1x/cdma.c | 2 +- drivers/infiniband/hw/usnic/usnic_uiom.c| 4 ++-- drivers/iommu/dma-iommu.c | 2 +- drivers/iommu/iommu.c | 17 ++--- drivers/iommu/iommufd/pages.c | 6 -- drivers/media/platform/qcom/venus/firmware.c| 2 +- drivers/net/ipa/ipa_mem.c | 6 -- drivers/net/wireless/ath/ath10k/snoc.c | 2 +- drivers/net/wireless/ath/ath11k/ahb.c | 4 ++-- drivers/remoteproc/remoteproc_core.c| 5 +++-- drivers/vfio/vfio_iommu_type1.c | 9 + drivers/vhost/vdpa.c| 2 +- include/linux/iommu.h | 4 ++-- 16 files changed, 43 insertions(+), 38 deletions(-) diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index c135f6e37a00ca..8bc01071474ab7 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -984,7 +984,8 @@ __iommu_create_mapping(struct device *dev, struct page **pages, size_t size, len = (j - i) << PAGE_SHIFT; ret = iommu_map(mapping->domain, iova, phys, len, - __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs)); + __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs), + GFP_KERNEL); if (ret < 0) goto fail; iova += len; @@ -1207,7 +1208,8 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, prot = __dma_info_to_prot(dir, attrs); - ret = iommu_map(mapping->domain, iova, phys, len, prot); + ret = iommu_map(mapping->domain, iova, phys, len, prot, + GFP_KERNEL); if (ret < 0) goto fail; count += len >> PAGE_SHIFT; @@ -1379,7 +1381,8 @@ static dma_addr_t arm_iommu_map_page(struct device *dev, struct page *page, prot = __dma_info_to_prot(dir, attrs); - ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot); + ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, + prot, GFP_KERNEL); if (ret < 0) goto fail; @@ -1443,7 +1446,7 @@ static dma_addr_t arm_iommu_map_resource(struct device *dev, prot = __dma_info_to_prot(dir, attrs) | IOMMU_MMIO; - ret = iommu_map(mapping->domain, dma_addr, addr, len, prot); + ret = iommu_map(mapping->domain, dma_addr, addr, len, prot, GFP_KERNEL); if (ret < 0) goto fail; diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c index 648ecf5a8fbc2a..a4ac94a2ab57fc 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c @@ -475,7 +475,8 @@ gk20a_instobj_ctor_iommu(struct gk20a_instmem *imem, u32 npages, u32 align, u32 offset = (r->offset + i) << imem->iommu_pgshift; ret = iommu_map(imem->domain, offset, node->dma_addrs[i], - PAGE_SIZE, IOMMU_READ | IOMMU_WRITE); + PAGE_SIZE, IOMMU_READ | IOMMU_WRITE, + GFP_KERNEL); if (ret < 0) { nvkm_error(subdev, "IOMMU mapping failure: %d\n", ret); diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 7bd2e65c2a16c5..6ca9f396e55be4 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -1057,7 +1057,7 @@ void *tegra_drm_alloc(struct tegra_drm *tegra, size_t size, dma_addr_t *dma) *dma = iova_dma_addr(&tegra->carveout.domain, alloc); err = iomm
Re: [PATCH 1/8] iommu: Add a gfp parameter to iommu_map()
On Fri, Jan 06, 2023 at 05:15:28PM +, Robin Murphy wrote: > On 2023-01-06 16:42, Jason Gunthorpe wrote: > > The internal mechanisms support this, but instead of exposting the gfp to > > the caller it wrappers it into iommu_map() and iommu_map_atomic() > > > > Fix this instead of adding more variants for GFP_KERNEL_ACCOUNT. > > FWIW, since we *do* have two variants already, I think I'd have a mild > preference for leaving the regular map calls as-is (i.e. implicit > GFP_KERNEL), and just generalising the _atomic versions for the special > cases. I think it is just better to follow kernel convention and have allocation functions include the GFP because it is a clear signal to the user that there is an allocation hidden inside the API. The whole point of gfp is not to have multitudes of every function for every allocation mode. There are not so many callers that it seems worth worrying about removing the extra GFP_KERNEL argument. > However, echoing the recent activity over on the DMA API side of things, I > think it's still worth proactively constraining the set of permissible > flags, lest we end up with more weird problems if stuff that doesn't really > make sense, like GFP_COMP or zone flags, manages to leak through (that may > have been part of the reason for having the current wrappers rather than a > bare gfp argument in the first place, I forget now). Yeah, that can be done Thanks, Jason
Re: [PULL] drm-intel-fixes
On Thu, Jan 05, 2023 at 03:02:20PM -0500, Rodrigo Vivi wrote: > Hi Dave and Daniel, > > Only GVT related fixes for this round. > > I have another fix queued for i915_vma_unbind_async from Nirmoy that will > target stable 5.18, but I figured it out late so I didn't run CI on that yet. > So I'm holding this for now. Maybe and extra PR tomorrow or it will > wait for the next week. > > Here goes drm-intel-fixes-2023-01-05: > > Only gvt-fixes: > - debugfs fixes (Zhenyu) > - fix up for vgpu status (Zhi) > - double free fix in split_2MB_gtt_entry (Zheng) > > Thanks, > Rodrigo. > > The following changes since commit 88603b6dc419445847923fcb7fe5080067a30f98: > > Linux 6.2-rc2 (2023-01-01 13:53:16 -0800) > > are available in the Git repository at: > > git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-fixes-2023-01-05 Pulled, thanks a lot. -Daniel > > for you to fetch changes up to 87809d3196c2a7a015ab80ca1cb8c19b659bc5f6: > > Merge tag 'gvt-fixes-2023-01-05' of https://github.com/intel/gvt-linux into > drm-intel-fixes (2023-01-05 08:03:38 -0500) > > > Only gvt-fixes: > - debugfs fixes (Zhenyu) > - fix up for vgpu status (Zhi) > - double free fix in split_2MB_gtt_entry (Zheng) > > > Dan Carpenter (1): > drm/i915: unpin on error in intel_vgpu_shadow_mm_pin() > > Rodrigo Vivi (1): > Merge tag 'gvt-fixes-2023-01-05' of https://github.com/intel/gvt-linux > into drm-intel-fixes > > Zheng Wang (1): > drm/i915/gvt: fix double free bug in split_2MB_gtt_entry > > Zhenyu Wang (2): > drm/i915/gvt: fix gvt debugfs destroy > drm/i915/gvt: fix vgpu debugfs clean in remove > > Zhi Wang (1): > drm/i915/gvt: use atomic operations to change the vGPU status > > drivers/gpu/drm/i915/gvt/debugfs.c | 36 > +++- > drivers/gpu/drm/i915/gvt/dmabuf.c| 3 ++- > drivers/gpu/drm/i915/gvt/gtt.c | 21 +++-- > drivers/gpu/drm/i915/gvt/gvt.h | 15 ++- > drivers/gpu/drm/i915/gvt/interrupt.c | 2 +- > drivers/gpu/drm/i915/gvt/kvmgt.c | 35 +-- > drivers/gpu/drm/i915/gvt/scheduler.c | 4 +++- > drivers/gpu/drm/i915/gvt/vgpu.c | 12 +--- > 8 files changed, 80 insertions(+), 48 deletions(-) -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] drm/gem: Check for valid formats
On Thu, Jan 05, 2023 at 08:04:13PM +, Simon Ser wrote: > To be honest, your suggestion to put the check inside framebuffer_check() > sounds like a better idea: we wouldn't even hit any driver-specific > code-path when the check fails. Daniel, do you think there'd be an issue > with this approach? framebuffer_check probably even better, since it'll stop nonsense before a driver potentially blows up. Which would be a CVE. So even better at catching potential issues. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH RESEND 1/4] backlight: aat2870: Use backlight helper
On Fri, Jan 06, 2023 at 05:48:52PM +0100, Stephen Kitt wrote: > Instead of retrieving the backlight brightness in struct > backlight_properties manually, and then checking whether the backlight > should be on at all, use backlight_get_brightness() which does all > this and insulates this from future changes. > > Signed-off-by: Stephen Kitt Lee/Daniel, will you pick these up, or should I smash them all into drm-misc-next for 6.3? Stephen, I see to be missing 3/4 here, but maybe it'll show up. -Daniel > --- > drivers/video/backlight/aat2870_bl.c | 7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/video/backlight/aat2870_bl.c > b/drivers/video/backlight/aat2870_bl.c > index a7af9adafad6..1cbb303e9c88 100644 > --- a/drivers/video/backlight/aat2870_bl.c > +++ b/drivers/video/backlight/aat2870_bl.c > @@ -59,7 +59,7 @@ static int aat2870_bl_update_status(struct backlight_device > *bd) > struct aat2870_bl_driver_data *aat2870_bl = bl_get_data(bd); > struct aat2870_data *aat2870 = > dev_get_drvdata(aat2870_bl->pdev->dev.parent); > - int brightness = bd->props.brightness; > + int brightness = backlight_get_brightness(bd); > int ret; > > if ((brightness < 0) || (bd->props.max_brightness < brightness)) { > @@ -70,11 +70,6 @@ static int aat2870_bl_update_status(struct > backlight_device *bd) > dev_dbg(&bd->dev, "brightness=%d, power=%d, state=%d\n", >bd->props.brightness, bd->props.power, bd->props.state); > > - if ((bd->props.power != FB_BLANK_UNBLANK) || > - (bd->props.state & BL_CORE_FBBLANK) || > - (bd->props.state & BL_CORE_SUSPENDED)) > - brightness = 0; > - > ret = aat2870->write(aat2870, AAT2870_BLM, >(u8)aat2870_brightness(aat2870_bl, brightness)); > if (ret < 0) > -- > 2.30.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH RESEND 1/4] backlight: aat2870: Use backlight helper
Hi Stephen, On Fri, Jan 06, 2023 at 05:48:52PM +0100, Stephen Kitt wrote: > Instead of retrieving the backlight brightness in struct > backlight_properties manually, and then checking whether the backlight > should be on at all, use backlight_get_brightness() which does all > this and insulates this from future changes. > > Signed-off-by: Stephen Kitt Nice cleanup. Reviewed-by: Sam Ravnborg > --- > drivers/video/backlight/aat2870_bl.c | 7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/video/backlight/aat2870_bl.c > b/drivers/video/backlight/aat2870_bl.c > index a7af9adafad6..1cbb303e9c88 100644 > --- a/drivers/video/backlight/aat2870_bl.c > +++ b/drivers/video/backlight/aat2870_bl.c > @@ -59,7 +59,7 @@ static int aat2870_bl_update_status(struct backlight_device > *bd) > struct aat2870_bl_driver_data *aat2870_bl = bl_get_data(bd); > struct aat2870_data *aat2870 = > dev_get_drvdata(aat2870_bl->pdev->dev.parent); > - int brightness = bd->props.brightness; > + int brightness = backlight_get_brightness(bd); > int ret; > > if ((brightness < 0) || (bd->props.max_brightness < brightness)) { > @@ -70,11 +70,6 @@ static int aat2870_bl_update_status(struct > backlight_device *bd) > dev_dbg(&bd->dev, "brightness=%d, power=%d, state=%d\n", >bd->props.brightness, bd->props.power, bd->props.state); > > - if ((bd->props.power != FB_BLANK_UNBLANK) || > - (bd->props.state & BL_CORE_FBBLANK) || > - (bd->props.state & BL_CORE_SUSPENDED)) > - brightness = 0; > - > ret = aat2870->write(aat2870, AAT2870_BLM, >(u8)aat2870_brightness(aat2870_bl, brightness)); > if (ret < 0) > -- > 2.30.2
Re: [PATCH RESEND 2/4] backlight: arcxcnn: Use backlight helper
On Fri, Jan 06, 2023 at 05:48:53PM +0100, Stephen Kitt wrote: > Instead of retrieving the backlight brightness in struct > backlight_properties manually, and then checking whether the backlight > should be on at all, use backlight_get_brightness() which does all > this and insulates this from future changes. > > Signed-off-by: Stephen Kitt Reviewed-by: Sam Ravnborg > --- > drivers/video/backlight/arcxcnn_bl.c | 5 + > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/video/backlight/arcxcnn_bl.c > b/drivers/video/backlight/arcxcnn_bl.c > index 555b036643fb..e610d7a1d13d 100644 > --- a/drivers/video/backlight/arcxcnn_bl.c > +++ b/drivers/video/backlight/arcxcnn_bl.c > @@ -130,12 +130,9 @@ static int arcxcnn_set_brightness(struct arcxcnn *lp, > u32 brightness) > static int arcxcnn_bl_update_status(struct backlight_device *bl) > { > struct arcxcnn *lp = bl_get_data(bl); > - u32 brightness = bl->props.brightness; > + u32 brightness = backlight_get_brightness(bl); > int ret; > > - if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK)) > - brightness = 0; > - > ret = arcxcnn_set_brightness(lp, brightness); > if (ret) > return ret; > -- > 2.30.2
Re: [PATCH RESEND 4/4] backlight: tosa: Use backlight helper
On Fri, Jan 06, 2023 at 05:48:55PM +0100, Stephen Kitt wrote: > Instead of retrieving the backlight brightness in struct > backlight_properties manually, and then checking whether the backlight > should be on at all, use backlight_get_brightness() which does all > this and insulates this from future changes. > > Signed-off-by: Stephen Kitt Reviewed-by: Sam Ravnborg > --- > drivers/video/backlight/tosa_bl.c | 7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/video/backlight/tosa_bl.c > b/drivers/video/backlight/tosa_bl.c > index 77b71f6c19b5..e338b1f00f6a 100644 > --- a/drivers/video/backlight/tosa_bl.c > +++ b/drivers/video/backlight/tosa_bl.c > @@ -50,13 +50,8 @@ static void tosa_bl_set_backlight(struct tosa_bl_data > *data, int brightness) > > static int tosa_bl_update_status(struct backlight_device *dev) > { > - struct backlight_properties *props = &dev->props; > struct tosa_bl_data *data = bl_get_data(dev); > - int power = max(props->power, props->fb_blank); > - int brightness = props->brightness; > - > - if (power) > - brightness = 0; > + int brightness = backlight_get_brightness(dev); > > tosa_bl_set_backlight(data, brightness); > > -- > 2.30.2
Re: [PATCH 1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable"
On Fri, Jan 06, 2023 at 08:04:19AM +0100, Greg KH wrote: > On Thu, Jan 05, 2023 at 05:40:17PM -0800, Brian Norris wrote: > > The self-refresh helper framework overloads "disable" to sometimes mean > > "go into self-refresh mode," and this mode activates automatically > > (e.g., after some period of unchanging display output). In such cases, > > the display pipe is still considered "on", and user-space is not aware > > that we went into self-refresh mode. Thus, users may expect that > > vblank-related features (such as DRM_IOCTL_WAIT_VBLANK) still work > > properly. > > > > However, we trigger the WARN_ONCE() here if a CRTC driver tries to leave > > vblank enabled here. > > > > Add a new exception, such that we allow CRTCs to be "disabled" (with > > self-refresh active) with vblank interrupts still enabled. > > > > Cc: # dependency for subsequent patch > > "subsequent" doesn't mean much when it is committed, give it a name > perhaps? It also looks a bit funny tbh, and a bit much like duct-tape. I need to think through how this is supposed to work really. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable"
On Thu, Jan 05, 2023 at 05:40:17PM -0800, Brian Norris wrote: > The self-refresh helper framework overloads "disable" to sometimes mean > "go into self-refresh mode," and this mode activates automatically > (e.g., after some period of unchanging display output). In such cases, > the display pipe is still considered "on", and user-space is not aware > that we went into self-refresh mode. Thus, users may expect that > vblank-related features (such as DRM_IOCTL_WAIT_VBLANK) still work > properly. > > However, we trigger the WARN_ONCE() here if a CRTC driver tries to leave > vblank enabled here. > > Add a new exception, such that we allow CRTCs to be "disabled" (with > self-refresh active) with vblank interrupts still enabled. > > Cc: # dependency for subsequent patch > Signed-off-by: Brian Norris > --- > > drivers/gpu/drm/drm_atomic_helper.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index d579fd8f7cb8..7b5eddadebd5 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1207,6 +1207,12 @@ disable_outputs(struct drm_device *dev, struct > drm_atomic_state *old_state) > > if (!drm_dev_has_vblank(dev)) > continue; > + /* > + * Self-refresh is not a true "disable"; let vblank remain > + * enabled. > + */ > + if (new_crtc_state->self_refresh_active) > + continue; This very fishy, because we check in crtc_needs_disable whether this output should stay on due to self-refresh. Which means you should never end up in here. And yes vblank better work in self refresh :-) If it doesn't, then you need to fake it with a timer, that's at least what i915 has done for transparent self-refresh. We might need a few more helpers. Also, probably more igt, or is this something igt testing has uncovered? If so, please cite the igt testcase which hits this. -Daniel > > ret = drm_crtc_vblank_get(crtc); > WARN_ONCE(ret != -EINVAL, "driver forgot to call > drm_crtc_vblank_off()\n"); > -- > 2.39.0.314.g84b9a713c41-goog > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v11 0/3] drm: exynos: dsi: Restore the bridge chain
On Mon, Dec 12, 2022 at 11:59 PM Jagan Teki wrote: > > Split the Exynos DSI bridge chain update patches from Samsung DSIM > bridge driver for easy to apply. > > Changes for v11: > - enable bridge.pre_enable_prev_first > Changes for v10: > - collect Marek.V Review tag Any update on this? Jagan.
Re: [PATCH v2] drm/vkms: reintroduce prepare_fb and cleanup_fb functions
On Fri, Jan 06, 2023 at 08:57:59AM -0300, Maíra Canal wrote: > With commit 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, > end}_fb_access with vmap"), the behavior of the shadow-plane helpers > changed and the vunmap is now performed at the end of > the current pageflip, instead of the end of the following pageflip. > > By performing the vunmap at the end of the current pageflip, invalid > memory is accessed by the vkms during the plane composition, as the data > is being unmapped before being used, as reported by the following > warning: > > [ 275.866047] BUG: unable to handle page fault for address: b382814e8002 > [ 275.866055] #PF: supervisor read access in kernel mode > [ 275.866058] #PF: error_code(0x) - not-present page > [ 275.866061] PGD 167 P4D 167 PUD 110a067 PMD 46e3067 PTE 0 > [ 275.866066] Oops: [#1] PREEMPT SMP PTI > [ 275.866070] CPU: 2 PID: 49 Comm: kworker/u8:2 Not tainted > 6.1.0-rc6-00018-gb357e7ac1b73-dirty #54 > [ 275.866074] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > 1.16.1-2.fc37 04/01/2014 > [ 275.866076] Workqueue: vkms_composer vkms_composer_worker [vkms] > [ 275.866084] RIP: 0010:XRGB_to_argb_u16+0x5c/0xa0 [vkms] > [ 275.866092] Code: bf 56 0a 0f af 56 70 48 8b 76 28 01 ca 49 83 f8 02 > 41 b9 01 00 00 00 4d 0f 43 c8 48 01 f2 48 83 c2 02 31 f6 66 c7 04 f0 ff > ff <0f> b6 0c b2 89 cf c1 e7 08 09 cf 66 89 7c f0 02 0f b6 4c b2 ff 89 > [ 275.866095] RSP: 0018:b382801b7db0 EFLAGS: 00010246 > [ 275.866098] RAX: 896336ace000 RBX: 896310e293c0 RCX: > > [ 275.866101] RDX: b382814e8002 RSI: RDI: > b382801b7de8 > [ 275.866103] RBP: 1400 R08: 0280 R09: > 0280 > [ 275.866105] R10: 0010 R11: c011d990 R12: > 896302a1ece0 > [ 275.866107] R13: R14: R15: > 80008001 > [ 275.866109] FS: () GS:89637dd0() > knlGS: > [ 275.866112] CS: 0010 DS: ES: CR0: 80050033 > [ 275.866114] CR2: b382814e8002 CR3: 03bb4000 CR4: > 06e0 > [ 275.866120] Call Trace: > [ 275.866123] > [ 275.866124] compose_active_planes+0x1c4/0x380 [vkms] > [ 275.866132] vkms_composer_worker+0x9f/0x130 [vkms] > [ 275.866139] process_one_work+0x1c0/0x370 > [ 275.866160] worker_thread+0x221/0x410 > [ 275.866164] ? worker_clr_flags+0x50/0x50 > [ 275.866167] kthread+0xe1/0x100 > [ 275.866172] ? kthread_blkcg+0x30/0x30 > [ 275.866176] ret_from_fork+0x22/0x30 > [ 275.866181] > [ 275.866182] Modules linked in: vkms > [ 275.866186] CR2: b382814e8002 > [ 275.866191] ---[ end trace ]--- > > Therefore, introduce again prepare_fb and cleanup_fb functions to the > vkms, which were previously removed on commit b43e2ec03b0d ("drm/vkms: > Let shadow-plane helpers prepare the plane's FB"). > > Fixes: 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access > with vmap") > Acked-by: Thomas Zimmermann > Signed-off-by: Maíra Canal btw for stuff that fixes regression, pls also cc everyone involved in that old patch. $ dim fixes 359c6649cd9a should give you a good idea. This way reviewers can learn a bit more where they missed something :-) Reviewed-by: Daniel Vetter > --- > > v1 -> v2: > https://lore.kernel.org/dri-devel/19951367-2ef0-0f26-ddf0-893259d9a...@igalia.com/T/ > > - Add kernel oops to the commit description (Melissa Wen). > - s/introduce/reintroduce/ in the title (Melissa Wen). > - Add Thomas's Acked-by. > > --- > drivers/gpu/drm/vkms/vkms_plane.c | 36 ++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c > b/drivers/gpu/drm/vkms/vkms_plane.c > index c3a845220e10..b3f8a115cc23 100644 > --- a/drivers/gpu/drm/vkms/vkms_plane.c > +++ b/drivers/gpu/drm/vkms/vkms_plane.c > @@ -160,10 +160,44 @@ static int vkms_plane_atomic_check(struct drm_plane > *plane, > return 0; > } > > +static int vkms_prepare_fb(struct drm_plane *plane, > +struct drm_plane_state *state) > +{ > + struct drm_shadow_plane_state *shadow_plane_state; > + struct drm_framebuffer *fb = state->fb; > + int ret; > + > + if (!fb) > + return 0; > + > + shadow_plane_state = to_drm_shadow_plane_state(state); > + > + ret = drm_gem_plane_helper_prepare_fb(plane, state); > + if (ret) > + return ret; > + > + return drm_gem_fb_vmap(fb, shadow_plane_state->map, > shadow_plane_state->data); > +} > + > +static void vkms_cleanup_fb(struct drm_plane *plane, > + struct drm_plane_state *state) > +{ > + struct drm_shadow_plane_state *shadow_plane_state; > + struct drm_framebuffer *fb = state->fb; > + > + if (!fb) > + return; > + > + shadow_plane_state = t
Re: [PATCH v10 1/2] drm: exynos: dsi: Fix MIPI_DSI*_NO_* mode flags
On Mon, Dec 12, 2022 at 8:28 PM Jagan Teki wrote: > > HFP/HBP/HSA/EOT_PACKET modes in Exynos DSI host specifies > 0 = Enable and 1 = Disable. > > The logic for checking these mode flags was correct before > the MIPI_DSI*_NO_* mode flag conversion. > > This patch is trying to fix this MIPI_DSI*_NO_* mode flags handling > Exynos DSI host and update the mode_flags in relevant panel drivers. > > Fixes: <0f3b68b66a6d> ("drm/dsi: Add _NO_ to MIPI_DSI_* flags disabling > features") > Reviewed-by: Marek Vasut > Reviewed-by: Nicolas Boichat > Reported-by: Sébastien Szymanski > Signed-off-by: Jagan Teki > --- Any update on this?
Re: [RFC 3/3] drm: Update file owner during use
On Fri, Jan 06, 2023 at 03:53:13PM +0100, Christian König wrote: > Am 06.01.23 um 11:53 schrieb Daniel Vetter: > > On Fri, Jan 06, 2023 at 11:32:17AM +0100, Christian König wrote: > > > Am 05.01.23 um 13:32 schrieb Daniel Vetter: > > > > [SNIP] > > > > > For the case of an master fd I actually don't see the reason why we > > > > > should > > > > > limit that? And fd can become master if it either was master before > > > > > or has > > > > > CAP_SYS_ADMIN. Why would we want an extra check for the pid/tgid here? > > > > This is just info/debug printing, I don't see the connection to > > > > drm_auth/master stuff? Aside from the patch mixes up the master opener > > > > and > > > > the current user due to fd passing or something like that. > > > That's exactly why my comment meant as well. > > > > > > The connect is that the drm_auth/master code currently the pid/tgid as > > > indicator if the "owner" of the fd has changed and so if an access should > > > be > > > allowed or not. I find that approach a bit questionable. > > > > > > > Note that we cannot do that (I think at least, after pondering this some > > > > more) because it would break the logind master fd passing scheme - there > > > > the receiving compositor is explicitly _not_ allowed to acquire master > > > > rights on its own. So the master priviledges must not move with the fd > > > > or > > > > things can go wrong. > > > That could be the rational behind that, but why doesn't logind then just > > > pass on a normal render node to the compositor? > > Because the compositor wants the kms node. We have 3 access levels in drm > > > > - render stuff > > - modeset stuff (needs a card* node and master rights for changing things) > > - set/drop master (needs root) > > > > logind wants to give the compositor modeset access, but not master > > drop/set access (because vt switching is controlled by logind). > > > > The pid check in drm_auth is for the use-case where you start your > > compositor on a root vt (or setuid-root), and then want to make sure > > that after cred dropping, set/drop master keeps working. Because in that > > case the vt switch dance is done by the compositor. > > > > Maybe we should document this stuff a bit better :-) > > Maybe add a friendly warning? E.g. like "Don't touch it, it works!" :) I think Tvrtko just volunteered for that :-) Maybe addition in the drm-uapi.rst section would be good that fills out the gaps we have. > So basically this is a valid use case where logind set/get the master status > of a fd while the compositor uses the master functionality? Yup, and the compositor is _not_ allowed to call these. Despite that it's the exact sime struct file - it has to be the same struct file in both loging and compositor, otherwise logind cannot orchestratet the vt switch dance for the compositors. Which unlike non-logind vt switching has the nice property that if a compositor dies/goes rogue, logind can still force the switch. With vt-only switching you need the sysrq to reset the console to text and kill the foreground process for the same effect. -Daniel > > Christian. > > > -Daniel > > > > > Christian. > > > > > > > -Daniel > > > > > > > > > > > > > > > > > Regards, > > > > > Christian. > > > > > > > > > > > Regards, > > > > > > > > > > > > Tvrtko > > > > > > > > > > > > > -Daniel > > > > > > > > > > > > > > > > > > > > > > return 0; > > > > > > > > if (!capable(CAP_SYS_ADMIN)) > > > > > > > > diff --git a/drivers/gpu/drm/drm_debugfs.c > > > > > > > > b/drivers/gpu/drm/drm_debugfs.c > > > > > > > > index 42f657772025..9d4e3146a2b8 100644 > > > > > > > > --- a/drivers/gpu/drm/drm_debugfs.c > > > > > > > > +++ b/drivers/gpu/drm/drm_debugfs.c > > > > > > > > @@ -90,15 +90,17 @@ static int drm_clients_info(struct seq_file > > > > > > > > *m, void *data) > > > > > > > > */ > > > > > > > > mutex_lock(&dev->filelist_mutex); > > > > > > > > list_for_each_entry_reverse(priv, &dev->filelist, > > > > > > > > lhead) { > > > > > > > > - struct task_struct *task; > > > > > > > > bool is_current_master = > > > > > > > > drm_is_current_master(priv); > > > > > > > > + struct task_struct *task; > > > > > > > > + struct pid *pid; > > > > > > > > - rcu_read_lock(); /* locks pid_task()->comm */ > > > > > > > > - task = pid_task(priv->pid, PIDTYPE_TGID); > > > > > > > > + rcu_read_lock(); /* Locks priv->pid and > > > > > > > > pid_task()->comm! */ > > > > > > > > + pid = rcu_dereference(priv->pid); > > > > > > > > + task = pid_task(pid, PIDTYPE_TGID); > > > > > > > > uid = task ? __task_cred(task)->euid : > > > > > > > > GLOBAL_ROOT_UID; > > > > > > > > seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", > > > > > > > > task ? task->comm : "", > > > > > > > > - pid_vnr(priv->pid), > > > > > > > > + pid_vnr(pid), > > > > > > > >
[PATCH RESEND 3/4] backlight: ipaq_micro: Use backlight helper
Instead of retrieving the backlight brightness in struct backlight_properties manually, and then checking whether the backlight should be on at all, use backlight_get_brightness() which does all this and insulates this from future changes. Signed-off-by: Stephen Kitt --- drivers/video/backlight/ipaq_micro_bl.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/video/backlight/ipaq_micro_bl.c b/drivers/video/backlight/ipaq_micro_bl.c index 85b16cc82878..f595b8c8cbb2 100644 --- a/drivers/video/backlight/ipaq_micro_bl.c +++ b/drivers/video/backlight/ipaq_micro_bl.c @@ -16,17 +16,12 @@ static int micro_bl_update_status(struct backlight_device *bd) { struct ipaq_micro *micro = dev_get_drvdata(&bd->dev); - int intensity = bd->props.brightness; + int intensity = backlight_get_brightness(bd); struct ipaq_micro_msg msg = { .id = MSG_BACKLIGHT, .tx_len = 3, }; - if (bd->props.power != FB_BLANK_UNBLANK) - intensity = 0; - if (bd->props.state & (BL_CORE_FBBLANK | BL_CORE_SUSPENDED)) - intensity = 0; - /* * Message format: * Byte 0: backlight instance (usually 1) -- 2.30.2
Re: [PATCH 1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable"
Hi Daniel, On Fri, Jan 06, 2023 at 06:53:49PM +0100, Daniel Vetter wrote: > On Thu, Jan 05, 2023 at 05:40:17PM -0800, Brian Norris wrote: > > The self-refresh helper framework overloads "disable" to sometimes mean > > "go into self-refresh mode," and this mode activates automatically > > (e.g., after some period of unchanging display output). In such cases, > > the display pipe is still considered "on", and user-space is not aware > > that we went into self-refresh mode. Thus, users may expect that > > vblank-related features (such as DRM_IOCTL_WAIT_VBLANK) still work > > properly. > > > > However, we trigger the WARN_ONCE() here if a CRTC driver tries to leave > > vblank enabled here. > > > > Add a new exception, such that we allow CRTCs to be "disabled" (with > > self-refresh active) with vblank interrupts still enabled. > > > > Cc: # dependency for subsequent patch > > Signed-off-by: Brian Norris > > --- > > > > drivers/gpu/drm/drm_atomic_helper.c | 6 ++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c > > index d579fd8f7cb8..7b5eddadebd5 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1207,6 +1207,12 @@ disable_outputs(struct drm_device *dev, struct > > drm_atomic_state *old_state) > > > > if (!drm_dev_has_vblank(dev)) > > continue; > > + /* > > +* Self-refresh is not a true "disable"; let vblank remain > > +* enabled. > > +*/ > > + if (new_crtc_state->self_refresh_active) > > + continue; > > This very fishy, because we check in crtc_needs_disable whether this > output should stay on due to self-refresh. Which means you should never > end up in here. That's not what crtc_needs_disable() does w.r.t. self-refresh. In fact, it's the opposite; see, for example, the |new_state->self_refresh_active| clause. That clause means that if we're entering self-refresh, we *intend* to disable (i.e., we return 'true'). That's because like I mention above, the self-refresh helpers overload what "disable" means. I'll also add my caveat again that I'm a bit new to DRM, so feel free to continue to correct me if I'm wrong :) Or perhaps Sean Paul could provide second opinions, as I believe he wrote this stuff. > And yes vblank better work in self refresh :-) If it doesn't, then you > need to fake it with a timer, that's at least what i915 has done for > transparent self-refresh. OK! Then that sounds like it at least ACKs my general idea for this series. (Michel and I poked at a few ideas in the thread at [1] and landed on approx. this solution, or else a fake/timer like you suggest.) > We might need a few more helpers. Also, probably more igt, or is this > something igt testing has uncovered? If so, please cite the igt testcase > which hits this. The current patch only fixes a warning that comes when I try to do the second patch. The second patch is a direct product of an IGT test failure (a few of kms_vblank's subtests), and I linked [1] the KernelCI report there. Brian [1] Link: https://lore.kernel.org/dri-devel/y5itf0+yniqa6...@sirena.org.uk/ Reported-by: "kernelci.org bot"
Re: [PATCH RESEND 0/4] video/backlight: Use backlight helpers
Not sure why git send-email didn't thread this, I can resend if necessary. Le 06/01/2023 17:48, Stephen Kitt a écrit : This started with work on the removal of backlight_properties' deprecated fb_blank field, much of which can be taken care of by using helper functions provided by backlight.h instead of directly accessing fields in backlight_properties. This patch series doesn't involve fb_blank, but it still seems useful to use helper functions where appropriate. Stephen Kitt (4): backlight: aat2870: Use backlight helper backlight: arcxcnn: Use backlight helper backlight: ipaq_micro: Use backlight helper backlight: tosa: Use backlight helper drivers/video/backlight/aat2870_bl.c| 7 +-- drivers/video/backlight/arcxcnn_bl.c| 5 + drivers/video/backlight/ipaq_micro_bl.c | 7 +-- drivers/video/backlight/tosa_bl.c | 7 +-- 4 files changed, 4 insertions(+), 22 deletions(-)
Re: [PATCH 1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable"
On Fri, Jan 06, 2023 at 06:53:49PM +0100, Daniel Vetter wrote: > On Thu, Jan 05, 2023 at 05:40:17PM -0800, Brian Norris wrote: > > The self-refresh helper framework overloads "disable" to sometimes mean > > "go into self-refresh mode," and this mode activates automatically > > (e.g., after some period of unchanging display output). In such cases, > > the display pipe is still considered "on", and user-space is not aware > > that we went into self-refresh mode. Thus, users may expect that > > vblank-related features (such as DRM_IOCTL_WAIT_VBLANK) still work > > properly. > > > > However, we trigger the WARN_ONCE() here if a CRTC driver tries to leave > > vblank enabled here. > > > > Add a new exception, such that we allow CRTCs to be "disabled" (with > > self-refresh active) with vblank interrupts still enabled. > > > > Cc: # dependency for subsequent patch > > Signed-off-by: Brian Norris > > --- > > > > drivers/gpu/drm/drm_atomic_helper.c | 6 ++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c > > index d579fd8f7cb8..7b5eddadebd5 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1207,6 +1207,12 @@ disable_outputs(struct drm_device *dev, struct > > drm_atomic_state *old_state) > > > > if (!drm_dev_has_vblank(dev)) > > continue; > > + /* > > +* Self-refresh is not a true "disable"; let vblank remain > > +* enabled. > > +*/ > > + if (new_crtc_state->self_refresh_active) > > + continue; > > This very fishy, because we check in crtc_needs_disable whether this > output should stay on due to self-refresh. Which means you should never > end up in here. > > And yes vblank better work in self refresh :-) If it doesn't, then you > need to fake it with a timer, that's at least what i915 has done for > transparent self-refresh. > > We might need a few more helpers. Also, probably more igt, or is this > something igt testing has uncovered? If so, please cite the igt testcase > which hits this. Ok I think I was a bit slow here, and it makes sense. Except this now means we loose this check, and I'm also not sure whether we really want drivers to implement this all. What I think we want here is a bit more: - for the self-refresh case check that the vblank all still works - check that drivers which use self_refresh are not using drm_atomic_helper_wait_for_vblanks(), because that would defeat the point - have a drm_crtc_vblank_off/on which take the crtc state, so they can look at the self-refresh state - fake vblanks with hrtimer, because on most hw when you turn off the crtc the vblanks are also turned off, and so your compositor would still hang. The vblank machinery already has all the code to make this happen (and if it's not all, then i915 psr code should have it). - I think kunit tests for this all would be really good, it's a rather complex state machinery between modesets and vblank functionality. You can speed up the kunit tests with some really high refresh rate, which isn't possible on real hw. I'm also wondering why we've had this code for years and only hit issues now? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable"
On Fri, Jan 06, 2023 at 10:08:53AM -0800, Brian Norris wrote: > Hi Daniel, > > On Fri, Jan 06, 2023 at 06:53:49PM +0100, Daniel Vetter wrote: > > On Thu, Jan 05, 2023 at 05:40:17PM -0800, Brian Norris wrote: > > > The self-refresh helper framework overloads "disable" to sometimes mean > > > "go into self-refresh mode," and this mode activates automatically > > > (e.g., after some period of unchanging display output). In such cases, > > > the display pipe is still considered "on", and user-space is not aware > > > that we went into self-refresh mode. Thus, users may expect that > > > vblank-related features (such as DRM_IOCTL_WAIT_VBLANK) still work > > > properly. > > > > > > However, we trigger the WARN_ONCE() here if a CRTC driver tries to leave > > > vblank enabled here. > > > > > > Add a new exception, such that we allow CRTCs to be "disabled" (with > > > self-refresh active) with vblank interrupts still enabled. > > > > > > Cc: # dependency for subsequent patch > > > Signed-off-by: Brian Norris > > > --- > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 6 ++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > > b/drivers/gpu/drm/drm_atomic_helper.c > > > index d579fd8f7cb8..7b5eddadebd5 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -1207,6 +1207,12 @@ disable_outputs(struct drm_device *dev, struct > > > drm_atomic_state *old_state) > > > > > > if (!drm_dev_has_vblank(dev)) > > > continue; > > > + /* > > > + * Self-refresh is not a true "disable"; let vblank remain > > > + * enabled. > > > + */ > > > + if (new_crtc_state->self_refresh_active) > > > + continue; > > > > This very fishy, because we check in crtc_needs_disable whether this > > output should stay on due to self-refresh. Which means you should never > > end up in here. > > That's not what crtc_needs_disable() does w.r.t. self-refresh. In fact, > it's the opposite; see, for example, the > |new_state->self_refresh_active| clause. That clause means that if we're > entering self-refresh, we *intend* to disable (i.e., we return 'true'). > That's because like I mention above, the self-refresh helpers overload > what "disable" means. > > I'll also add my caveat again that I'm a bit new to DRM, so feel free to > continue to correct me if I'm wrong :) Or perhaps Sean Paul could > provide second opinions, as I believe he wrote this stuff. I already replied in another thread with hopefully less nonsense from my side :-) > > And yes vblank better work in self refresh :-) If it doesn't, then you > > need to fake it with a timer, that's at least what i915 has done for > > transparent self-refresh. > > OK! Then that sounds like it at least ACKs my general idea for this > series. (Michel and I poked at a few ideas in the thread at [1] and > landed on approx. this solution, or else a fake/timer like you suggest.) Yeah once I stopped looking at this the wrong way round it does make sense what you're doing. See my other reply, I think with just this series here the vblanks still stall out? Or does your hw actually keep generating vblank irq with the display off? > > We might need a few more helpers. Also, probably more igt, or is this > > something igt testing has uncovered? If so, please cite the igt testcase > > which hits this. > > The current patch only fixes a warning that comes when I try to do the > second patch. The second patch is a direct product of an IGT test > failure (a few of kms_vblank's subtests), and I linked [1] the KernelCI > report there. Ah yeah that makes sense. Would be good to cite that in this patch too, because I guess the same kms_vblank tests can also hit this warning here? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v4 3/7] accel/ivpu: Add GEM buffer object management
On Fri, 6 Jan 2023 at 14:23, Stanislaw Gruszka wrote: > > On Fri, Jan 06, 2023 at 11:50:05AM +0100, Daniel Vetter wrote: > > On Thu, Dec 08, 2022 at 12:07:29PM +0100, Jacek Lawrynowicz wrote: > > > Adds four types of GEM-based BOs for the VPU: > > > - shmem > > > - userptr > > > - internal > > > > Uh what do you need this for? Usually the way we do these is just alloce a > > normal bo, and then pin them. > > I think we do alloc/pin this way, but all our bo's are GEM based. > For those bo's we use internally and other non-shmem we create them > with drm_gem_private_object_init(). I think this way is simpler than > have separate code for non-GEM and GEM bo's ... They should be all gem bo, I guess you mean shmem vs non-shmem? And the allocate+pin is the standard approach for drivers that have somewhat dynamic bo (i.e. not using dma_alloc) and need some of them (hopefully only for driver internal objects, not for userspace) pinned in place. So you handrolling a perma-pinned gem bo for internal objects is rather strange by drm driver standards. > > Also, gem shmem helpers should be able to mostly cover you here, why not > > use those? Might need some work to push basic userptr to them, but we have > > enough drivers reinventing that wheel to justify that work. > > > > Can I guess also be done after merging. > > ... but if not, we can add this to TODO. Yeah I'm fine with todo to cut these over to shmem helpers, this driver has been stuck in limbo for way too long anyway. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH RESEND 3/4] backlight: ipaq_micro: Use backlight helper
On Fri, Jan 06, 2023 at 05:48:54PM +0100, Stephen Kitt wrote: > Instead of retrieving the backlight brightness in struct > backlight_properties manually, and then checking whether the backlight > should be on at all, use backlight_get_brightness() which does all > this and insulates this from future changes. > > Signed-off-by: Stephen Kitt Reviewed-by: Sam Ravnborg > --- > drivers/video/backlight/ipaq_micro_bl.c | 7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/video/backlight/ipaq_micro_bl.c > b/drivers/video/backlight/ipaq_micro_bl.c > index 85b16cc82878..f595b8c8cbb2 100644 > --- a/drivers/video/backlight/ipaq_micro_bl.c > +++ b/drivers/video/backlight/ipaq_micro_bl.c > @@ -16,17 +16,12 @@ > static int micro_bl_update_status(struct backlight_device *bd) > { > struct ipaq_micro *micro = dev_get_drvdata(&bd->dev); > - int intensity = bd->props.brightness; > + int intensity = backlight_get_brightness(bd); > struct ipaq_micro_msg msg = { > .id = MSG_BACKLIGHT, > .tx_len = 3, > }; > > - if (bd->props.power != FB_BLANK_UNBLANK) > - intensity = 0; > - if (bd->props.state & (BL_CORE_FBBLANK | BL_CORE_SUSPENDED)) > - intensity = 0; > - > /* >* Message format: >* Byte 0: backlight instance (usually 1) > -- > 2.30.2
Re: [PATCH] drm/i915: Add missing check and destroy for alloc_workqueue
On Fri, Jan 06, 2023 at 05:09:34PM +0800, Jiasheng Jiang wrote: > Add checks for the return value of alloc_workqueue and > alloc_ordered_workqueue as they may return NULL pointer and cause NULL > pointer dereference. > Moreover, destroy them when fails later in order to avoid memory leak. > Because in `drivers/gpu/drm/i915/i915_driver.c`, if > intel_modeset_init_noirq fails, its workqueues will not be destroyed. > > Fixes: c26a058680dc ("drm/i915: Use a high priority wq for nonblocking plane > updates") > Fixes: 757fffcfdffb ("drm/i915: Put all non-blocking modesets onto an ordered > wq") > Signed-off-by: Jiasheng Jiang So you have an entire pile of these, and I think that's a really good excuse to - create a drmm_alloc_workqueue helper for these (and drmm_alloc_ordered_workqueue) using the drmm_add_action_or_reset() function for automatic drm_device cleanup - use that instead in all drivers, which means you do not have to make any error handling mor complex Up for that? In that case please also convert any existing alloc*workqueue callsites in drm, and make this all a patch series (since then there would be dependencies). Cheers, Daniel > --- > drivers/gpu/drm/i915/display/intel_display.c | 21 > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 6c2686ecb62a..58f6840d6bd8 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -8655,26 +8655,35 @@ int intel_modeset_init_noirq(struct drm_i915_private > *i915) > intel_dmc_ucode_init(i915); > > i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0); > + if (!i915->display.wq.modeset) { > + ret = -ENOMEM; > + goto cleanup_vga_client_pw_domain_dmc; > + } > + > i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI | > WQ_UNBOUND, > WQ_UNBOUND_MAX_ACTIVE); > + if (!i915->display.wq.flip) { > + ret = -ENOMEM; > + goto cleanup_modeset; > + } > > intel_mode_config_init(i915); > > ret = intel_cdclk_init(i915); > if (ret) > - goto cleanup_vga_client_pw_domain_dmc; > + goto cleanup_flip; > > ret = intel_color_init(i915); > if (ret) > - goto cleanup_vga_client_pw_domain_dmc; > + goto cleanup_flip; > > ret = intel_dbuf_init(i915); > if (ret) > - goto cleanup_vga_client_pw_domain_dmc; > + goto cleanup_flip; > > ret = intel_bw_init(i915); > if (ret) > - goto cleanup_vga_client_pw_domain_dmc; > + goto cleanup_flip; > > init_llist_head(&i915->display.atomic_helper.free_list); > INIT_WORK(&i915->display.atomic_helper.free_work, > @@ -8686,6 +8695,10 @@ int intel_modeset_init_noirq(struct drm_i915_private > *i915) > > return 0; > > +cleanup_flip: > + destroy_workqueue(i915->display.wq.flip); > +cleanup_modeset: > + destroy_workqueue(i915->display.wq.modeset); > cleanup_vga_client_pw_domain_dmc: > intel_dmc_ucode_fini(i915); > intel_power_domains_driver_remove(i915); > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [RFC PATCH v3 0/3] Support for Solid Fill Planes
On Fri, Jan 06, 2023 at 05:43:23AM +0200, Dmitry Baryshkov wrote: > On Fri, 6 Jan 2023 at 02:38, Jessica Zhang wrote: > > > > > > > > On 1/5/2023 3:33 AM, Daniel Vetter wrote: > > > On Wed, Jan 04, 2023 at 03:40:33PM -0800, Jessica Zhang wrote: > > >> Introduce and add support for a solid_fill property. When the solid_fill > > >> property is set, and the framebuffer is set to NULL, memory fetch will be > > >> disabled. > > >> > > >> In addition, loosen the NULL FB checks within the atomic commit callstack > > >> to allow a NULL FB when the solid_fill property is set and add FB checks > > >> in methods where the FB was previously assumed to be non-NULL. > > >> > > >> Finally, have the DPU driver use drm_plane_state.solid_fill and instead > > >> of > > >> dpu_plane_state.color_fill, and add extra checks in the DPU atomic commit > > >> callstack to account for a NULL FB in cases where solid_fill is set. > > >> > > >> Some drivers support hardware that have optimizations for solid fill > > >> planes. This series aims to expose these capabilities to userspace as > > >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android > > >> hardware composer HAL) that can be set by apps like the Android Gears > > >> app. > > >> > > >> Userspace can set the solid_fill property to a blob containing the > > >> appropriate version number and solid fill color (in RGB323232 format) and > > >> setting the framebuffer to NULL. > > >> > > >> Note: Currently, there's only one version of the solid_fill blob > > >> property. > > >> However if other drivers want to support a similar feature, but require > > >> more than just the solid fill color, they can extend this feature by > > >> creating additional versions of the drm_solid_fill struct. > > >> > > >> Changes in V2: > > >> - Dropped SOLID_FILL_FORMAT property (Simon) > > >> - Switched to implementing solid_fill property as a blob (Simon, Dmitry) > > >> - Changed to checks for if solid_fill_blob is set (Dmitry) > > >> - Abstracted (plane_state && !solid_fill_blob) checks to helper method > > >>(Dmitry) > > >> - Removed DPU_PLANE_COLOR_FILL_FLAG > > >> - Fixed whitespace and indentation issues (Dmitry) > > > > > > Now that this is a blob, I do wonder again whether it's not cleaner to set > > > the blob as the FB pointer. Or create some kind other kind of special data > > > source objects (because solid fill is by far not the only such thing). > > > > > > We'd still end up in special cases like when userspace that doesn't > > > understand solid fill tries to read out such a framebuffer, but these > > > cases already exist anyway for lack of priviledges. > > > > > > So I still think that feels like the more consistent way to integrate this > > > feature. Which doesn't mean it has to happen like that, but the > > > patches/cover letter should at least explain why we don't do it like this. > > > > Hi Daniel, > > > > IIRC we were facing some issues with this check [1] when trying to set > > FB to a PROP_BLOB instead. Which is why we went with making it a > > separate property instead. Will mention this in the cover letter. > > What kind of issues? Could you please describe them? We switched from bitmask to enum style for prop types, which means it's not possible to express with the current uapi a property which accepts both an object or a blob. Which yeah sucks a bit ... But! blob properties are kms objects (like framebuffers), so it should be possible to stuff a blob into an object property as-is. Of course you need to update the validation code to make sure we accept either an fb or a blob for the internal representation. But that kind of split internally is required no matter what I think. -Daniel > > > > > [1] > > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_property.c#L71 > > > > Thanks, > > > > Jessica Zhang > > > > > -Daniel > > > > > >> > > >> Changes in V3: > > >> - Fixed some logic errors in atomic checks (Dmitry) > > >> - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() > > >> helper > > >>methods (Dmitry) > > >> > > >> Jessica Zhang (3): > > >>drm: Introduce solid fill property for drm plane > > >>drm: Adjust atomic checks for solid fill color > > >>drm/msm/dpu: Use color_fill property for DPU planes > > >> > > >> drivers/gpu/drm/drm_atomic.c | 136 +- > > >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++--- > > >> drivers/gpu/drm/drm_atomic_state_helper.c | 9 ++ > > >> drivers/gpu/drm/drm_atomic_uapi.c | 59 ++ > > >> drivers/gpu/drm/drm_blend.c | 17 +++ > > >> drivers/gpu/drm/drm_plane.c | 8 +- > > >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +- > > >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 +++ > > >> include/drm/drm_atomic_helper.h | 5 +- > > >> include/drm/drm_blend.h | 1 + > > >> include/drm/drm_plane.h
Re: [PATCH] drm/radeon: free iio for atombios when driver shutdown
Just a quick drive-by. For these simple cases where we just need to make sure that memory is freed using drmm_kmalloc and friends should help simplify things. Probably not worth it for radeon, but figured I'll throw it out there. For more functional code switching to drmm is harder because you need the right order. But for these all that matters is that stuff gets freed so there's no leak, and drmm can take care of that without ordering constraints. -Daniel On Fri, Jan 06, 2023 at 10:36:53AM -0500, Alex Deucher wrote: > Applied. Thanks! > > Alex > > On Fri, Jan 6, 2023 at 5:00 AM Liwei Song wrote: > > > > Fix below kmemleak when unload radeon driver: > > > > unreferenced object 0x9f8608ede200 (size 512): > > comm "systemd-udevd", pid 326, jiffies 4294682822 (age 716.338s) > > hex dump (first 32 bytes): > > 00 00 00 00 c4 aa ec aa 14 ab 00 00 00 00 00 00 > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > backtrace: > > [<62fadebe>] kmem_cache_alloc_trace+0x2f1/0x500 > > [] atom_parse+0x117/0x230 [radeon] > > [<158c23fd>] radeon_atombios_init+0xab/0x170 [radeon] > > [<683f672e>] si_init+0x57/0x750 [radeon] > > [<566cc31f>] radeon_device_init+0x559/0x9c0 [radeon] > > [<46efabb3>] radeon_driver_load_kms+0xc1/0x1a0 [radeon] > > [ ] drm_dev_register+0xdd/0x1d0 > > [<45fec835>] radeon_pci_probe+0xbd/0x100 [radeon] > > [ ] pci_device_probe+0xe1/0x160 > > [<19484b76>] really_probe.part.0+0xc1/0x2c0 > > [<3f2649da>] __driver_probe_device+0x96/0x130 > > [<231c5bb1>] driver_probe_device+0x24/0xf0 > > [<00a42377>] __driver_attach+0x77/0x190 > > [ ] bus_for_each_dev+0x7f/0xd0 > > [<633166d2>] driver_attach+0x1e/0x30 > > [<313b05b8>] bus_add_driver+0x12c/0x1e0 > > > > iio was allocated in atom_index_iio() called by atom_parse(), > > but it doesn't got released when the dirver is shutdown. > > Fix this kmemleak by free it in radeon_atombios_fini(). > > > > Signed-off-by: Liwei Song > > --- > > drivers/gpu/drm/radeon/radeon_device.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/radeon/radeon_device.c > > b/drivers/gpu/drm/radeon/radeon_device.c > > index 92905ebb7b45..1c005e0ddd38 100644 > > --- a/drivers/gpu/drm/radeon/radeon_device.c > > +++ b/drivers/gpu/drm/radeon/radeon_device.c > > @@ -1022,6 +1022,7 @@ void radeon_atombios_fini(struct radeon_device *rdev) > > { > > if (rdev->mode_info.atom_context) { > > kfree(rdev->mode_info.atom_context->scratch); > > + kfree(rdev->mode_info.atom_context->iio); > > } > > kfree(rdev->mode_info.atom_context); > > rdev->mode_info.atom_context = NULL; > > -- > > 2.33.1 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [Intel-gfx] [PATCH v2 4/5] drm/i915/guc: Add GuC CT specific debug print wrappers
On 12/6/2022 03:06, Tvrtko Ursulin wrote: On 05/12/2022 18:44, Michal Wajdeczko wrote: On 05.12.2022 14:16, Tvrtko Ursulin wrote: On 02/12/2022 20:14, John Harrison wrote: [snip] Random meaningless (to me) message that is apparently a display thing: drm_dbg_kms(&dev_priv->drm, "disabling %s\n", pll->info->name); i915 :00:02.0: [drm:intel_disable_shared_dpll [i915]] disabling PORT PLL B Plan is to not touch outside gt/. For some unexplicable reason that means it is almost impossible to see the actual problems in most CI dmesg logs because they are swamped with irrelevant display messages that cannot be filtered out. For example, I recently manually grep'd out all the display spam from a bug report log. The dmesg file went from 12MB to 700KB. That is a significant problem that makes bug triage way harder than it needs to be. Maybe as a way forward work could be split? If John wants to deal with gt_xxx macros, avoid touching GuC (putting his original motivation aside) and you want to convert the gt/uc folder? Assuming John you are okay with "GuC:" and "CT:" prefixes. Meaning just repost patch #1 only and expand to more intel_gt_* files? Sure, if someone will actually reply to that patch with some kind of r-b first so I know I'm not still wasting my time on a huge re-write that will to be redone multiple times when someone objects to the use of a colon or the lack of spaces, braces or whatever. John.
Re: [PATCH] drm/drv: Make use of local variable driver in drm_dev_register()
On Thu, Jan 05, 2023 at 09:16:36PM +0100, Uwe Kleine-König wrote: > Hello Daniel, > > On Thu, Jan 05, 2023 at 03:33:13PM +0100, Daniel Vetter wrote: > > On Tue, Dec 20, 2022 at 08:24:18AM +0100, Thomas Zimmermann wrote: > > > Hi > > > > > > Am 19.12.22 um 19:31 schrieb Uwe Kleine-König: > > > > There is a local variable that contains dev->driver. Make use of it > > > > instead of "open coding" it. > > > > > > > > Signed-off-by: Uwe Kleine-König > > > > > > Added to drm-misc-next. Thanks a lot. > > > > Given that Uwe has a pile of drm commits all over, time for drm-misc > > commit rights? > > I feel honored, but if you ask me, that's not necessary/sensible. At > least up to now my patches are more or less drive-by changes and letting > them go through someone else with commit access is fine for me. There is > no driver in the drm area I feel responsible for. > > Or is this about reducing maintainer load on your end? Yes :-) Essentially it gives you the keys to drive your own patches, all you have to do is find someone to review them for you (trading review works best). And since all regulars have commit rights there's otherwise big confusion whether patches should be pushed or not. Which means they routinely fall through cracks and need extra attention from everyone involved. Also note that commit rights does not mean you assume maintainer responsibilities for anything (ofc your review is appreciated where you can help out), that's still separate and tracked in MAINTAINERS as usual. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch