Re: [Xen-devel] [PATCH] drm: Split out drm_probe_helper.h
Hi Daniel, Thank you for the patch. On Tuesday, 15 January 2019 12:41:37 EET Daniel Vetter wrote: > Having the probe helper stuff (which pretty much everyone needs) in > the drm_crtc_helper.h file (which atomic drivers should never need) is > confusing. Split them out. > > To make sure I actually achieved the goal here I went through all > drivers. And indeed, all atomic drivers are now free of > drm_crtc_helper.h includes. > > v2: Make it compile. There was so much compile fail on arm drivers > that I figured I'll better not include any of the acks on v1. > > v3: Massive rebase because i915 has lost a lot of drmP.h includes, but > not all: Through drm_crtc_helper.h > drm_modeset_helper.h -> drmP.h > there was still one, which this patch largely removes. Which means > rolling out lots more includes all over. > > This will also conflict with ongoing drmP.h cleanup by others I > expect. > > v3: Rebase on top of atomic bochs. > > Cc: Sam Ravnborg > Cc: Jani Nikula > Cc: Laurent Pinchart > Acked-by: Rodrigo Vivi (v2) > Acked-by: Benjamin Gaignard (v2) > Signed-off-by: Daniel Vetter > Cc: linux-arm-ker...@lists.infradead.org > Cc: virtualizat...@lists.linux-foundation.org > Cc: etna...@lists.freedesktop.org > Cc: linux-samsung-...@vger.kernel.org > Cc: intel-...@lists.freedesktop.org > Cc: linux-media...@lists.infradead.org > Cc: linux-amlo...@lists.infradead.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: spice-de...@lists.freedesktop.org > Cc: amd-...@lists.freedesktop.org > Cc: linux-renesas-...@vger.kernel.org > Cc: linux-rockc...@lists.infradead.org > Cc: linux-st...@st-md-mailman.stormreply.com > Cc: linux-te...@vger.kernel.org > Cc: xen-de...@lists.xen.org > --- > Merging this is going to be a bit a mess due to all the ongoing drmP.h > cleanups. I think the following should work: > - Apply Sam's prep patches for removing drmP.h from > drm_modeset_helper.h > - Get the i915 drmP.h cleanup backmerged into drm-misc-next > - Apply this patch. > - Apply Sam's patch to remove drmP.h from drm_modeset_helper.h > - All through drm-misc-next, which has some potential for trivial > conflicts around #includes with other drivers unfortunately. > > I hope there's no other driver who'll blow up accidentally because > someone else is doing a drmP.h cleanup. Laurent maybe? There's a drmP.h cleanup in the R-Car DU driver, but it doesn't conflict with this patch, the combination of both compiles fine. > Jani, ack on this? > -Daniel > --- [snip] > drivers/gpu/drm/bridge/adv7511/adv7511.h | 5 +- > drivers/gpu/drm/bridge/analogix-anx78xx.c | 2 +- > drivers/gpu/drm/bridge/dumb-vga-dac.c | 2 +- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- > drivers/gpu/drm/bridge/ti-tfp410.c| 2 +- > drivers/gpu/drm/drm_atomic_helper.c | 1 - > drivers/gpu/drm/drm_dp_mst_topology.c | 2 +- > drivers/gpu/drm/drm_modeset_helper.c | 2 +- > drivers/gpu/drm/drm_probe_helper.c| 2 +- > drivers/gpu/drm/drm_simple_kms_helper.c | 2 +- > drivers/gpu/drm/omapdrm/omap_connector.c | 2 +- > drivers/gpu/drm/omapdrm/omap_crtc.c | 2 +- > drivers/gpu/drm/omapdrm/omap_drv.c| 2 +- > drivers/gpu/drm/omapdrm/omap_drv.h| 2 +- > drivers/gpu/drm/omapdrm/omap_encoder.c| 2 +- > drivers/gpu/drm/omapdrm/omap_fb.c | 2 +- > drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 2 +- > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +- > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 2 +- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +- > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 2 +- > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 2 +- > drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +- > drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 1 + > drivers/gpu/drm/shmobile/shmob_drm_drv.c | 1 + > drivers/gpu/drm/shmobile/shmob_drm_kms.c | 1 + > include/drm/drm_crtc_helper.h | 16 -- > include/drm/drm_probe_helper.h| 50 +++ For the above files, with the comments below addressed, Reviewed-by: Laurent Pinchart > 227 files changed, 289 insertions(+), 200 deletions(-) > create mode 100644 include/drm/drm_probe_helper.h [snip] > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h > b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 73d8ccb97742..d52ffab41eb4 > 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > @@ -14,8 +14,11 @@ > #i
Re: [Xen-devel] [PATCH 0/1] cameraif: Add ABI for para-virtualized
Hi Oleksandr, Thank you for the patch. On Tuesday, 31 July 2018 12:31:41 EEST Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko > > Hello! > > At the moment Xen [1] already supports some virtual multimedia > features [2] such as virtual display, sound. It supports keyboards, > pointers and multi-touch devices all allowing Xen to be used in > automotive appliances, In-Vehicle Infotainment (IVI) systems > and many more. > > This work adds a new Xen para-virtualized protocol for a virtual > camera device which extends multimedia capabilities of Xen even > farther: video conferencing, IVI, high definition maps etc. > > The initial goal is to support most needed functionality with the > final idea to make it possible to extend the protocol if need be: > > 1. Provide means for base virtual device configuration: > - pixel formats > - resolutions > - frame rates > 2. Support basic camera controls: > - contrast > - brightness > - hue > - saturation > 3. Support streaming control > 4. Support zero-copying use-cases > > I hope that Xen and V4L and other communities could give their > valuable feedback on this work, so I can update the protocol > to better fit any additional requirements I might have missed. I'll start with a question : what are the expected use cases ? The ones listed above sound like they would better be solved by passing the corresponding device(s) to the guest. > [1] https://www.xenproject.org/ > [2] https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=xen/include/public/io > > Oleksandr Andrushchenko (1): > cameraif: add ABI for para-virtual camera > > xen/include/public/io/cameraif.h | 981 +++++++ > 1 file changed, 981 insertions(+) > create mode 100644 xen/include/public/io/cameraif.h -- Regards, Laurent Pinchart ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v2 20/21] drm/xlnx: Initialize DRM driver instance with CMA helper macro
Hi Thomas,
Thank you for the patch.
On Tue, Sep 15, 2020 at 04:59:57PM +0200, Thomas Zimmermann wrote:
> The xlnx driver uses CMA helpers with default callback functions.
> Initialize the driver structure with the rsp CMA helper macro. The
> driver is being converted to use GEM object functions as part of
> this change.
>
> Two callbacks, .dumb_destroy and .gem_prime_import, were initialized
> to their default implementations, so they are just kept empty now.
>
> v2:
> * initialize with DRM_GEM_CMA_DRIVER_OPS_WITH_DUMB_CREATE (Laurent)
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 14 +-
> 1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> index 8e69303aad3f..f3ffc3703a0e 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> @@ -80,19 +80,7 @@ static struct drm_driver zynqmp_dpsub_drm_driver = {
> .driver_features= DRIVER_MODESET | DRIVER_GEM |
> DRIVER_ATOMIC,
>
> - .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> - .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_export = drm_gem_prime_export,
> - .gem_prime_import = drm_gem_prime_import,
> - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
> - .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> - .gem_prime_vmap = drm_gem_cma_prime_vmap,
> - .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
> - .gem_prime_mmap = drm_gem_cma_prime_mmap,
> - .gem_free_object_unlocked = drm_gem_cma_free_object,
> - .gem_vm_ops = &drm_gem_cma_vm_ops,
> - .dumb_create= zynqmp_dpsub_dumb_create,
> - .dumb_destroy = drm_gem_dumb_destroy,
> + DRM_GEM_CMA_DRIVER_OPS_WITH_DUMB_CREATE(zynqmp_dpsub_dumb_create),
>
> .fops = &zynqmp_dpsub_drm_fops,
>
--
Regards,
Laurent Pinchart
Re: [PATCH 19/20] drm/xlnx: Initialize DRM driver instance with CMA helper macro
Hi Thomas,
Thank you for the patch.
On Thu, Aug 13, 2020 at 10:36:43AM +0200, Thomas Zimmermann wrote:
> The xlnx driver uses CMA helpers with default callback functions.
> Initialize the driver structure with the rsp CMA helper macro. The
> driver is being converted to use GEM object functions as part of
> this change.
>
> Two callbacks, .dumb_destroy and .gem_prime_import, were initialized
> to their default implementations, so they are just kept empty now.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 14 +-
> 1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> index 26328c76305b..058044dcc062 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> @@ -80,19 +80,7 @@ static struct drm_driver zynqmp_dpsub_drm_driver = {
> .driver_features= DRIVER_MODESET | DRIVER_GEM |
> DRIVER_ATOMIC,
>
> - .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> - .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_export = drm_gem_prime_export,
> - .gem_prime_import = drm_gem_prime_import,
> - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
> - .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> - .gem_prime_vmap = drm_gem_cma_prime_vmap,
> - .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
> - .gem_prime_mmap = drm_gem_cma_prime_mmap,
> - .gem_free_object_unlocked = drm_gem_cma_free_object,
> - .gem_vm_ops = &drm_gem_cma_vm_ops,
> - .dumb_create= zynqmp_dpsub_dumb_create,
> - .dumb_destroy = drm_gem_dumb_destroy,
> + DRM_GEM_CMA_DRIVER_OPS_VMAP_WITH_DUMB_CREATE(zynqmp_dpsub_dumb_create),
The only effective change here is
- .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
- .gem_prime_mmap = drm_gem_cma_prime_mmap,
+ .gem_prime_import_sg_table =
drm_gem_cma_prime_import_sg_table_vmap,
+ .gem_prime_mmap = drm_gem_prime_mmap,
The change is significant, and I have a hard time following the code to
verify that it's correct, or if it's an undesired side effect. If it's
correct, could the change be mentioned in the commit message, with at
least a brief explanation of why this is correct, and what the
consequences here ?
>
> .fops = &zynqmp_dpsub_drm_fops,
>
--
Regards,
Laurent Pinchart
Re: [PATCH 10/20] drm/omapdrm: Introduce GEM object functions
Hi Thomas,
Thank you for the patch.
On Thu, Aug 13, 2020 at 10:36:34AM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in omapdrm.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/omapdrm/omap_drv.c | 9 -
> drivers/gpu/drm/omapdrm/omap_gem.c | 16 +++-
> drivers/gpu/drm/omapdrm/omap_gem.h | 1 -
> 3 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 53d5e184ee77..2e598b8b72af 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -521,12 +521,6 @@ static int dev_open(struct drm_device *dev, struct
> drm_file *file)
> return 0;
> }
>
> -static const struct vm_operations_struct omap_gem_vm_ops = {
> - .fault = omap_gem_fault,
> - .open = drm_gem_vm_open,
> - .close = drm_gem_vm_close,
> -};
> -
> static const struct file_operations omapdriver_fops = {
> .owner = THIS_MODULE,
> .open = drm_open,
> @@ -549,10 +543,7 @@ static struct drm_driver omap_drm_driver = {
> #endif
> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_export = omap_gem_prime_export,
> .gem_prime_import = omap_gem_prime_import,
> - .gem_free_object_unlocked = omap_gem_free_object,
> - .gem_vm_ops = &omap_gem_vm_ops,
> .dumb_create = omap_gem_dumb_create,
> .dumb_map_offset = omap_gem_dumb_map_offset,
> .ioctls = ioctls,
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c
> index d0d12d5dd76c..d68dc63dea0a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -487,7 +487,7 @@ static vm_fault_t omap_gem_fault_2d(struct drm_gem_object
> *obj,
> * vma->vm_private_data points to the GEM object that is backing this
> * mapping.
> */
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf)
> +static vm_fault_t omap_gem_fault(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
> struct drm_gem_object *obj = vma->vm_private_data;
> @@ -1169,6 +1169,18 @@ static bool omap_gem_validate_flags(struct drm_device
> *dev, u32 flags)
> return true;
> }
>
> +static const struct vm_operations_struct omap_gem_vm_ops = {
> + .fault = omap_gem_fault,
> + .open = drm_gem_vm_open,
> + .close = drm_gem_vm_close,
> +};
> +
> +static const struct drm_gem_object_funcs omap_gem_object_funcs = {
> + .free = omap_gem_free_object,
> + .export = omap_gem_prime_export,
> + .vm_ops = &omap_gem_vm_ops,
> +};
> +
> /* GEM buffer object constructor */
> struct drm_gem_object *omap_gem_new(struct drm_device *dev,
> union omap_gem_size gsize, u32 flags)
> @@ -1236,6 +1248,8 @@ struct drm_gem_object *omap_gem_new(struct drm_device
> *dev,
> size = PAGE_ALIGN(gsize.bytes);
> }
>
> + obj->funcs = &omap_gem_object_funcs;
> +
> /* Initialize the GEM object. */
> if (!(flags & OMAP_BO_MEM_SHMEM)) {
> drm_gem_private_object_init(dev, obj, size);
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.h
> b/drivers/gpu/drm/omapdrm/omap_gem.h
> index 729b7812a815..9e6b5c8195d9 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.h
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.h
> @@ -69,7 +69,6 @@ struct dma_buf *omap_gem_prime_export(struct drm_gem_object
> *obj, int flags);
> struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
> struct dma_buf *buffer);
>
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf);
I like how this function now becomes internal to omap_gem.c.
Reviewed-by: Laurent Pinchart
> int omap_gem_roll(struct drm_gem_object *obj, u32 roll);
> void omap_gem_cpu_sync_page(struct drm_gem_object *obj, int pgoff);
> void omap_gem_dma_sync_buffer(struct drm_gem_object *obj,
--
Regards,
Laurent Pinchart
Re: [Xen-devel] [PATCH 04/52] drm: Set final_kfree in drm_dev_alloc
Hi Daniel, Thank you for the patch. On Wed, Feb 19, 2020 at 11:20:34AM +0100, Daniel Vetter wrote: > I also did a full review of all callers, and only the xen driver > forgot to call drm_dev_put in the failure path. Fix that up too. I'd split this patch in two then, with the Xen first coming first, and with an explanation in the commit message of the second patch about why you call drmm_add_final_kfree() in drm_dev_alloc(). > v2: I noticed that xen has a drm_driver.release hook, and uses > drm_dev_alloc(). We need to remove the kfree from > xen_drm_drv_release(). > > bochs also has a release hook, but leaked the drm_device ever since > > commit 0a6659bdc5e8221da99eebb176fd9591435e38de > Author: Gerd Hoffmann > Date: Tue Dec 17 18:04:46 2013 +0100 > > drm/bochs: new driver > > This patch here fixes that leak. > > Same for virtio, started leaking with > > commit b1df3a2b24a917f8853d43fe9683c0e360d2c33a > Author: Gerd Hoffmann > Date: Tue Feb 11 14:58:04 2020 +0100 > > drm/virtio: add drm_driver.release callback. > > Cc: Gerd Hoffmann > Cc: Oleksandr Andrushchenko > Cc: xen-devel@lists.xenproject.org > > Signed-off-by: Daniel Vetter > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Oleksandr Andrushchenko > Cc: xen-devel@lists.xenproject.org > --- > drivers/gpu/drm/drm_drv.c | 3 +++ > drivers/gpu/drm/xen/xen_drm_front.c | 2 +- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 3e5627d6eba6..9e62e28bbc62 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -39,6 +39,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -819,6 +820,8 @@ struct drm_device *drm_dev_alloc(struct drm_driver > *driver, > return ERR_PTR(ret); > } > > + drmm_add_final_kfree(dev, dev); drmm_add_final_kfree() can only be called once. Does this mean that a driver using drm_dev_alloc() isn't allowed to use drmm_add_final_kfree() to tract its own private structure ? > + > return dev; > } > EXPORT_SYMBOL(drm_dev_alloc); > diff --git a/drivers/gpu/drm/xen/xen_drm_front.c > b/drivers/gpu/drm/xen/xen_drm_front.c > index 4be49c1aef51..d22b5da38935 100644 > --- a/drivers/gpu/drm/xen/xen_drm_front.c > +++ b/drivers/gpu/drm/xen/xen_drm_front.c > @@ -461,7 +461,6 @@ static void xen_drm_drv_release(struct drm_device *dev) > drm_mode_config_cleanup(dev); > > drm_dev_fini(dev); > - kfree(dev); > > if (front_info->cfg.be_alloc) > xenbus_switch_state(front_info->xb_dev, > @@ -561,6 +560,7 @@ static int xen_drm_drv_init(struct xen_drm_front_info > *front_info) > fail_modeset: > drm_kms_helper_poll_fini(drm_dev); > drm_mode_config_cleanup(drm_dev); > + drm_dev_put(drm_dev); > fail: > kfree(drm_info); > return ret; -- Regards, Laurent Pinchart ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
